Use IHttpClientFactory to create the GraphServiceClient #781
Conversation
There was a problem hiding this comment.
LGTM. Thanks very much @damienbod
@darrelmiller : do you have any feedback?
|
|
||
| var httpClientFactory = serviceProvider.GetRequiredService<IHttpClientFactory>(); | ||
| var httpClient = httpClientFactory.CreateClient(); | ||
| httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); |
There was a problem hiding this comment.
| httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); | |
| httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue(Constants.ApplicationJson)); | |
There was a problem hiding this comment.
Yeah, I'm pretty sure this won't work. By creating a new HttpClient instance without any of the default middleware, you won't get an auth token added to the request, it won't do retry logic and it won't handle redirects properly.
If there is a way of getting the HttpMessageHandler that is pooled by IHttpClientFactory then we could use that along with the GraphClientFactory, but I'm not sure how to get hold of that. The other option is to get the array of default message handlers from the GraphClientFactory and use those when constructing the HttpClient. Again, I forget how that is done with IHttpClientFactory.
There was a problem hiding this comment.
I tested this using the test APP WebAppCallsMicrosoftGraph using the scope user.read and it worked. I decided to test again and added another scope User.ReadBasic.All so that the photo will load and I can't get it to work, so maybe the changes break something...
The link underneath does work for both the API and the APP.
https://damienbod.com/2020/11/20/using-microsoft-graph-api-in-asp-net-core/
I believe the IHttpClientFactory should be used to create HttpClients in ASP.NET core applications.
Greetings and thanks for the feedback Damien
There was a problem hiding this comment.
Ok, so I dug in to find out why the auth is working and we have a check to see if the HttpClient has been setup with AuthHandler middleware and if not, we fall back to calling the AuthProvder directly in the BaseRequest. This is very much a backward compatibility solution and will not benefit from features that are being added to AuthHandler such as CAE support and conditional access for resources such as SharePoint.
Damien pointed out that due to the use of AddScoped the current implementation will not scale due to the fact that HttpClient is designed to be a singleton. Having said that, as this is a .NET core 3.1 only solution, I think the code that causes the scaling problem by closing connections on dispose was removed in .NET 3.1, so it is possible that issue is no longer a concern.
I suppose for consistency of experience, we should use the client created by IHttpClientFactory. We just need to find out the incantation required to add the middleware created by GraphClientFactory.CreateDefaultHandlers() to be added to that httpclient instance.
There was a problem hiding this comment.
sounds good. Thanks for looking more into this @darrelmiller and for the heads up @damienbod
On our side, we are investigating the MSAL.NET cache performance issue when CCA is used as a singleton
|
@damienbod : do we want to keep this PR open? |
|
@jmprieur I don't think so because I think Graph API will try to solve the connection management problem internally and I can use the ITokenAcquisition like all the other downstream APIs anyway. Thanks for you time in reviewing. Greetings Damien |
|
thanks for the update, @damienbod |
fixes #780