Use static HttpClient

[GGAlanSmithee]

As previously discussed, we are using .NET's HttpClient as a disposable. This is not recommended as per https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ and other sources.

We should look in to keeping an internal static HttpClient and use it where it makes sense. Take note that we cannot make assumptions on how this SDK will be used, so things such as DI that relies on registering services etc are probably out of scope, unless we provide our own HttpClient service that the user can register.

Alternatives of the top of my head:

1. Keep an internal static HttpClient and use it where applicable
   • This is probably only applicable in some places, such as CallAsync
2. Let the user provide their own HttpClient
   • We do set headers prior to making requests, so we would probably have to do some house keeping with this
   • Could the above lead to potential race conditions in multithreaded environments?
3. Provide a service that the user can register for DI, and use that internally
   • Not sure if this would provide any benefit over the previous point unless we can make such that the user can re-use their own HttpClient

CC @brokenprogrammer - any thouights on this?

[brokenprogrammer]

My personal preference would be alternative 1. Its a small code change and the only downside would be that we would have to deal with the default request headers in some sane way.

We provide the following function:

internal static async Task<AuthorizationResponse> GetAccessTokenAsync(string authorizationCode, string clientSecret, string apiEndpoint)
{
        using (var client = new HttpClient())
        {
            client.DefaultRequestHeaders.Add("Client-Secret", clientSecret);
            client.DefaultRequestHeaders.Add("Authorization-Code", authorizationCode);
            client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

            var response = await client.GetAsync(apiEndpoint);

            return await GetResponseAsync<AuthorizationResponse>(response);
        }
}

This sets the header "Authorization-Code" which is only relevant for this specific request. All other requests using the FortnoxAPIClient are using the header "Access-Token" instead. So for this to work in a sane way I'd prefer some sort of initialization step for the static HttpClient setting the correct headers and for the GetAccessTokenAsync perhaps remove and then restore the headers on the fly?

As for point 2, letting users provide their own HttpClient would be a great addition in my oppinion. However this should not be mandatory. My only question about this would be how we would handle both the request headers and also if the set HttpClient should be re-used.

The http headers for the provided HttpClient can always be removed after its use but shifting focus to the HttpClient should it be bound to the FortnoxAPIClient or bound to the request itself, example:

// Idea 1
var client = new HttpClient();
// All requests made with this request object will use the same http client
var request = new InvoiceListRequest(this.connectionSettings.AccessToken, this.connectionSettings.ClientSecret, client);
var invoiceList = await InvoiceService.GetInvoicesAsync(request);

// Idea 2
var client = new HttpClient();
var request = new InvoiceListRequest(this.connectionSettings.AccessToken, this.connectionSettings.ClientSecret);

// InvocieList's request is made using the http client created above but secondInvoiceList is created using the internal HttpClient
var invoiceList = await InvoiceService.GetInvoicesAsync(request, client);
var secondInvoiceList = await InvoiceService.GetInvoicesAsync(request);

I don't like the third option due to the big change.

CC @BaconFinger - Would you like to join in on this discussion?

[BaconFinger]

I agree with option 1. Is the article implying that we could just have a static HttpClient in classes (that is, a 'new' static HttpClient every time), or is it saying that we should have one static HttpClient instance that is accessible everywhere (that is, a singleton)?

Headers and such can be done via a fluent interface, and should be cleared after a request. If we want to have some kind of initializer-esque method that is called to get the static instance, we could clear the stuff there also. Like:
var client = InternalHttpClient.New().Blah().Blah().Blah().Get(url)

The client should only come into the picture at the last possible second. So inside of GetInvoicesAsync the method would get the static instance itself. I really am not a fan of passing connection objects (and configuration, to a lesser extent) as parameters of functions they have nothing to do with or in constructors where we aren't DI-ing some config object like in an ASP.NET application. The good lord gave us fluent interfaces for a reason:
var request = new InvoiceListRequest().AccessToken(token).ClientSecret(secret).Client(staticClient)
It doesn't particularly matter in this instance, though. If we are doing option 1 then there is no reason to pass in the client - the request can just get the static instance of the client itself. Or call some helper/service/whatever to get it.

Conceptually, I do think it a little strange that the request is doing the sending. Shouldn't our client/client abstraction take a request and send it? But that's a little more off-topic.

Users providing their own HttpClient is a nice-to-have, but I don't really see how it fixes the problem. If they just pass in the default HttpClient then it's like we did no change at all.

Option 3 is a bit too much for right now.

[GGAlanSmithee]

I agree with your sentiments that option 1 is the most valid one. @BaconFinger doing re-writes of the API for the sake of architecture is off topic for now.

Users providing their own HttpClient is a nice-to-have, but I don't really see how it fixes the problem. If they just pass in the default HttpClient then it's like we did no change at all.

The idea behind this would be to allow them to have one static HTTP Client across their application lifetime. The way we do it now, (or according to what is discussed in this issue) would mitigate the problem internally to this library, but not necisarily outside it (as it is used in an application)