Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some thoughts on CallWebApiForUserAsync2<TInput, TOutput> #503

Closed
NikolaosWakem opened this issue Aug 26, 2020 · 12 comments
Closed

Some thoughts on CallWebApiForUserAsync2<TInput, TOutput> #503

NikolaosWakem opened this issue Aug 26, 2020 · 12 comments
Assignees
Labels
enhancement New feature or request investigate
Milestone

Comments

@NikolaosWakem
Copy link

I have switched some existing code to use IDownstreamWebApi which is great..

In the end I i knocked together the following so the app could access an existing API

public static async Task<TOutput> CallWebApiForUserAsync2<TInput, TOutput>(this IDownstreamWebApi downstreamWebApi, string optionsInstanceName, TInput input, Action<DownstreamWebApiOptions> downstreamWebApiOptionsOverride = null, ClaimsPrincipal user = null)
        {
            HttpResponseMessage response = await downstreamWebApi.CallWebApiForUserAsync(
                optionsInstanceName,
                downstreamWebApiOptionsOverride,
                user,
                new StringContent(JsonSerializer.Serialize(input), Encoding.UTF8, "application/json")
                ).ConfigureAwait(false);

            try
            {
                response.EnsureSuccessStatusCode();
            }
            catch
            {
                string error = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

                throw new HttpRequestException($"{(int)response.StatusCode} {response.StatusCode} {error}");
            }

            string content = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

            if (string.IsNullOrWhiteSpace(content))
                return default;

            return JsonSerializer.Deserialize<TOutput>(content, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
        }
  1. Current CallWebApiForUserAsync only accepts OK status code
  2. Current CallWebApiForUserAsync seems to not be able to accept Ok() returned from webapi i.e. blank string
  3. Current CallWebApiForUserAsync only accepts TOutput as class so no OK(332)

I imagine IDownstreamWebApi might be still a work in progress?.. be great to hear how best to utilize it.

@NikolaosWakem NikolaosWakem added the enhancement New feature or request label Aug 26, 2020
@pmaytak
Copy link
Contributor

pmaytak commented Aug 26, 2020

@NikolaosWakem thanks for your feedback. The idea behind DownstreamWebApi classes was to encapsulate some common code (that accesses protected APIs) for reuse. Wiki has more info. Also this sample uses these classes like here and here.

@NikolaosWakem
Copy link
Author

NikolaosWakem commented Aug 26, 2020

All the example code seems to return 200 and the object e.g. OK(Todo) which isn't how a lot of apis work. Will look out for further updates

Thanks!

@jennyf19
Copy link
Collaborator

@NikolaosWakem going to reopen, as, based on your analysis, looks like we a bug, and/or overlooked an important scenario.

@jennyf19 jennyf19 reopened this Aug 27, 2020
@jennyf19 jennyf19 added this to the 0.3.1-preview milestone Aug 27, 2020
@jmprieur
Copy link
Collaborator

jmprieur commented Aug 27, 2020

Thanks for your suggestion, @NikolaosWakem, and for being an early adopter.
Would you see value in having other generic methods (for instance CallWebApiForUserAsync?)

@jennyf19
Copy link
Collaborator

@NikolaosWakem I have a PR w/your code suggestions, if you want to take a look.

@NikolaosWakem
Copy link
Author

NikolaosWakem commented Aug 31, 2020

Thanks for your suggestion, @NikolaosWakem, and for being an early adopter.
Would you see value in having other generic methods (for instance CallWebApiForUserAsync?)

Thanks for getting back to me... I just use the following...

public static class MicrosoftIdentityWebExtensions
    {
        public static async Task<TOutput> GetWebApiForUserAsync<TOutput>(this IDownstreamWebApi downstreamWebApi, string serviceName, string relativePath)
        {
            return await downstreamWebApi.CallWebApiForUserAsync<TOutput>(serviceName, options =>
            {
                options.HttpMethod = HttpMethod.Get;
                options.RelativePath = relativePath;
            });
        }

        public static async Task PostWebApiForUserAsync(this IDownstreamWebApi downstreamWebApi, string serviceName, string relativePath)
        {
            await downstreamWebApi.CallWebApiForUserAsync(serviceName, options =>
            {
                options.HttpMethod = HttpMethod.Post;
                options.RelativePath = relativePath;
            });
        }

        public static async Task<TOutput> PostWebApiForUserAsync<TOutput, TInput>(this IDownstreamWebApi downstreamWebApi, string serviceName, string relativePath, TInput data)
        {
            return await downstreamWebApi.CallWebApiForUserAsync<TInput, TOutput>(serviceName, data, options =>
            {
                options.HttpMethod = HttpMethod.Post;
                options.RelativePath = relativePath;
            });
        }

        public static async Task PutWebApiForUserAsync<TInput>(this IDownstreamWebApi downstreamWebApi, string serviceName, string relativePath, TInput data)
        {
            await downstreamWebApi.CallWebApiForUserAsync(serviceName, data, options =>
            {
                options.HttpMethod = HttpMethod.Put;
                options.RelativePath = relativePath;
            });
        }

        public static async Task<TOutput> PutWebApiForUserAsync<TOutput, TInput>(this IDownstreamWebApi downstreamWebApi, string serviceName, string relativePath, TInput data)
        {
            return await downstreamWebApi.CallWebApiForUserAsync<TInput, TOutput>(serviceName, data, options =>
            {
                options.HttpMethod = HttpMethod.Put;
                options.RelativePath = relativePath;
            });
        }

        private static async Task<TOutput> CallWebApiForUserAsync<TOutput>(this IDownstreamWebApi downstreamWebApi, string serviceName, Action<DownstreamWebApiOptions> downstreamWebApiOptionsOverride = null, ClaimsPrincipal user = null)
        {
            var response = await downstreamWebApi.CallWebApiForUserAsync(
                serviceName,
                downstreamWebApiOptionsOverride,
                user,
                null
                ).ConfigureAwait(false);

            var content = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

            if (!response.IsSuccessStatusCode)
                throw new HttpRequestException($"{(int)response.StatusCode} {response.StatusCode} {content}");

            return !string.IsNullOrWhiteSpace(content) ? JsonSerializer.Deserialize<TOutput>(content, new JsonSerializerOptions { PropertyNameCaseInsensitive = true }) : default;
        }

        private static async Task CallWebApiForUserAsync<TInput>(this IDownstreamWebApi downstreamWebApi, string serviceName, TInput input, Action<DownstreamWebApiOptions> downstreamWebApiOptionsOverride = null, ClaimsPrincipal user = null)
        {
            var response = await downstreamWebApi.CallWebApiForUserAsync(
                serviceName,
                downstreamWebApiOptionsOverride,
                user,
                new StringContent(JsonSerializer.Serialize(input), Encoding.UTF8, "application/json")
                ).ConfigureAwait(false);

            if (!response.IsSuccessStatusCode)
                throw new HttpRequestException($"{(int)response.StatusCode} {response.StatusCode} {await response.Content.ReadAsStringAsync().ConfigureAwait(false)}");
        }

        private static async Task<TOutput> CallWebApiForUserAsync<TInput, TOutput>(this IDownstreamWebApi downstreamWebApi, string serviceName, TInput input, Action<DownstreamWebApiOptions> downstreamWebApiOptionsOverride = null, ClaimsPrincipal user = null)
        {
            var response = await downstreamWebApi.CallWebApiForUserAsync(
                serviceName,
                downstreamWebApiOptionsOverride,
                user,
                new StringContent(JsonSerializer.Serialize(input), Encoding.UTF8, "application/json")
                ).ConfigureAwait(false);

            var content = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

            if (!response.IsSuccessStatusCode)
                throw new HttpRequestException($"{(int)response.StatusCode} {response.StatusCode} {content}");

            return !string.IsNullOrWhiteSpace(content) ? JsonSerializer.Deserialize<TOutput>(content, new JsonSerializerOptions { PropertyNameCaseInsensitive = true }) : default;
        }

        public static bool IsMsalUiRequiredException(this Exception exception)
        {
            if (exception is Microsoft.Identity.Client.MsalUiRequiredException)
                return true;
            else if (exception.InnerException != null)
                return IsMsalUiRequiredException(exception.InnerException);
            else
                return false;
        }
    }

@rollandjb
Copy link

It would also be helpful to be able to set up DefaultRequestHeaders on the HttpClient, for example, to add an api key to every call to the api.

@jmprieur
Copy link
Collaborator

jmprieur commented Sep 4, 2020

@rollandjb : we were advised against doing it by the ASP.NET Core team (HttpClient is pooled).
Do you think it would be harmless to do it?

@rollandjb
Copy link

rollandjb commented Sep 6, 2020

@jmprieur : I am no expert, but according to .NET microservices - Architecture e-book HttpMessageHandlers are pooled, not HttpClient. The document encourages the use of IHttpClientFactory as provided by Typed Clients, described as

... just an HttpClient that's pre-configured ...[with some] ... specific values such as the base server, HTTP headers or time outs.

Of course, DownstreamWebApi is just such a typed client.

The document also encourages the use of DelegatingHandlers for fine tuning requests on the way out and in. It also has a reference to Make HTTP requests using IHttpClientFactory in ASP.NET Core describing the pattern in more detail.

To be able to do this, though, one needs access to the IHttpClientBuilder returned from AddHttpClient.

In a production app, one would most likely write a dedicated typed client in any respect. However, providing a way to configure and add handlers to DownstreamWebApi would, together with the suggestions above, make it more flexible.

Maybe an overload of AddDownstreamApi that accepts Action<IHttpClientBuilder> would do the trick?

What do you think?

@jennyf19
Copy link
Collaborator

Included in 0.4.0-preview release. we have another issue, above, tracking more improvements here.

@parwejp
Copy link

parwejp commented Mar 15, 2021

@jmprieur : I am no expert, but according to .NET microservices - Architecture e-book HttpMessageHandlers are pooled, not HttpClient. The document encourages the use of IHttpClientFactory as provided by Typed Clients, described as

... just an HttpClient that's pre-configured ...[with some] ... specific values such as the base server, HTTP headers or time outs.

Of course, DownstreamWebApi is just such a typed client.

The document also encourages the use of DelegatingHandlers for fine tuning requests on the way out and in. It also has a reference to Make HTTP requests using IHttpClientFactory in ASP.NET Core describing the pattern in more detail.

To be able to do this, though, one needs access to the IHttpClientBuilder returned from AddHttpClient.

In a production app, one would most likely write a dedicated typed client in any respect. However, providing a way to configure and add handlers to DownstreamWebApi would, together with the suggestions above, make it more flexible.

Maybe an overload of AddDownstreamApi that accepts Action<IHttpClientBuilder> would do the trick?

What do you think?

@jennyf19 : I am not able to find out how to send request headers while using get requests on iDownStreamWebAPI. I recently got an error while calling MS graph api to add "ConsistencyLevel = eventual" header.
Microsoft Graph advanced queries Header
Have switched to ITokenAquisition implementation for now but would be nice to use IDownStreamWebAPI for it. As @rollandjb mentioned it would be possible to set request headers with pooled HttpClient with proper isolation. I am no expert but creating HttpRequestMessage could be quick fix.
If it is implemented already, could you please be kind enough to point me out to documentation?

Many Thanks

@jmprieur
Copy link
Collaborator

Thanks @parwejp these are fair points.
Would you mind opening a new issue with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request investigate
Projects
None yet
Development

No branches or pull requests

6 participants