reactiveui / ReactiveUI

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms that is inspired by functional reactive programming. ReactiveUI allows you to abstract mutable state away from your user interfaces, express the idea around a feature in one readable place and improve the testability of your application.
https://www.reactiveui.net
MIT License
8.05k stars 1.12k forks source link

(Disregard - issue reported under wrong project.) Ability to share a common HttpClient across multiple AddRefit registrations. #3335

Closed search4sound closed 2 years ago

search4sound commented 2 years ago

Is your feature request related to a problem? Please describe.

It is generally best practice to reuse HttpClient where feasible, such as when different API calls can use the same HttpClient with the same base address. While all the methods for a given Refit-enabled interface will use the same HttpClient, some methods may be better suited to be defined in different interfaces, following the Interface Segregation Principle that is the "I" in SOLID. At the least, a high-level segregation between OAuth methods (AuthGrantFromCode, AuthGrantFromRefreshToken, AuthGrantFromClientCreds) and other API data access and CRUD methods would make sense. If I have two interfaces, ISomeSiteAuth, and ISomeSiteData, and they both use the same base address, then if I register them like this:

builder.Services.AddRefitClient<ISomeSiteAuth>().ConfigureHttpClient(httpClient => httpClient.BaseAddress = new Uri("https://foo"));
builder.Services.AddRefitClient<ISomeSiteData>().ConfigureHttpClient(httpClient => httpClient.BaseAddress = new Uri("https://foo"));

Then the underlying HttpClients will be different instances, because the underlying call to AddTypedClient does this for each interface (inside internal static method Microsoft.Extensions.DependencyInjection.AddTypedClientCore)...

builder.Services.AddTransient<TClient>(s =>
{
    IHttpClientFactory httpClientFactory = s.GetRequiredService<IHttpClientFactory>();
    HttpClient httpClient = httpClientFactory.CreateClient(builder.Name);

    return factory(httpClient, s);
});

I was able to prove this by adding to each interface a property get of type HttpClient and property name Client. For reusability, I put this property in a new interface:

public interface IHttpClientProvider
{
    HttpClient Client { get; }
}

Then I made each interface implement it:

public interface ISomeSiteAuth : IHttpClientProvider { /* ISomeSiteAuth details omitted */ }
public interface ISomeSiteData : IHttpClientProvider { /* ISomeSiteData details omitted */ }

Then I created the following unit test:

[Fact]
public void RefitClientsGenericWithoutSettingsGetTheirOwnHttpClients()
{
    var services = new ServiceCollection();

    services.AddRefitClient<ISomeSiteAuth>();
    services.AddRefitClient<ISomeSiteData>();

    var authApi = services.BuildServiceProvider().GetRequiredService<ISomeSiteAuth>();
    var dataApi = services.BuildServiceProvider().GetRequiredService<ISomeSiteData>();
    var sameClient = ReferenceEquals(authApi.Client, dataApi.Client);

    Assert.False(sameClient);
}

I found this test passes - the references of the Client property of the authApi and the dataApi are different. I ran this in an actual full fledge application and got the same result. Note I have a unit test for the non-generic AddRefit method as well and get the same result.

Describe the solution you'd like

I would like to have the ability to optionally make these two interfaces use the same HttpClient instance. When calling AddRefit, have information in the parameters (new properties in RefitSettings would be good) which the AddRefit method could use to determine whether to behave as it currently does or provide new behavior where the HttpClient used for one Refit interface type could be used for another Refit interface type.

Describe alternatives you've considered

I looked into whether this behavior could be changed with the existing Refit library and whether Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.AddTypedClient could be called in such a way that reusing the HttpClient could be possible, but I could not achieve this - there is too much hidden inside internal methods. I suppose a PR on HttpClientBuilderExtensions could be requested, but this seems to me to be more a limitation of how Refit works, where you only have interfaces and rely greatly on the underlying behavior.

Describe suggestions on how to achieve the feature

I suggest that in AddRefitClient where we are currently always calling AddTypedClient, we conditionally call it - call it if there are no RefitSettings or if the RefitSettings has no property set to request the HttpClient to be reused. However if RefitSettings does have such a property set (not null or empty), then the code should keep track of registered HttpClients and provide an existing one if it matches the criteria in settings, otherwise provide a new HttpClient.

One way to implement this is to add two properties to the RefitSettings class:

public string? HttpClientName { get; set; }
public Uri? HttpClientBaseAddress { get; set; }

We may only need one of these properties to establish which HtpClient a Refit interface should use, however I think having these two provides flexibility - you may want more than one HttpClient per BaseAddress (one use case I've encountered was where we wanted one HttpClient for long polling calls and another for regular fast returning calls, may be other use cases applicable to Refit).

I would have the code track HttpClients in a dictionary with key being a string (set to settings.HttpClientName.) and value being the HttpClient When creating an HttpClient, I would set the BaseAddress to the settings.HttpClientBaseAddress, this way it does not need to be set following an AddRefitClient call and things simply behave as expected based on the settings.

I implemented a solution and created unit tests for this functionality. The first test ensures multiple interfaces share the same HttpClient when they share the same string value for HttpClientName in RefitSettings:

[Fact]
public void RefitClientsGenericWithSameSettingsHttpClientNameShareTheSameHttpClient()
{
    var services = new ServiceCollection();

    var refitSettings = new RefitSettings
    {
        HttpClientName = "ClientName1", HttpClientBaseAddress = new Uri("https://localhost:5001"),
    };
    services.AddTransient(_ => refitSettings);

    services.AddRefitClient<IBoringCrudWithClientApi<User, string>>(refitSettings);
    services.AddRefitClient<IBoringOtherCrudWithClientApi<Role, string>>(refitSettings);

    var boringApi = services.BuildServiceProvider().GetRequiredService<IBoringCrudWithClientApi<User, string>>();
    var boringOtherApi = services.BuildServiceProvider().GetRequiredService<IBoringOtherCrudWithClientApi<Role, string>>();
    var sameClient = ReferenceEquals(boringApi.Client, boringOtherApi.Client);

    Assert.True(sameClient);

    // Also confirm the client has the same base address as the settings base address.
    Assert.Equal(boringApi.Client.BaseAddress?.AbsoluteUri, refitSettings.HttpClientBaseAddress?.AbsoluteUri);
}

Note in the above: IBoringCrudWithClientApi<User, string> implements IBoringCrudApi<User, string> and IHttpClientProvider, and IBoringOtherCrudWithClientApi<User, string> implements IBoringOtherCrudApi<User, string> and IHttpClientProvider, thus providing access to each api implementation's Client property.

Both the RefitClientsGenericWithoutSettingsGetTheirOwnHttpClients test and the RefitClientsGenericWithSameSettingsHttpClientNameShareTheSameHttpClient test pass with my implementation.

I also wrote a test for the case where multiple interfaces do not the same HttpClient when they gave different string values for HttpClientName in RefitSettings:

[Fact]
public void RefitClientsGenericWithDifferentSettingsHttpClientNameGetTheirOwnHttpClients()
{
    var services = new ServiceCollection();

    var refitSettings = new RefitSettings
    {
        HttpClientName = "ClientName1", HttpClientBaseAddress = new Uri("https://foo:5001"),
    };
    services.AddTransient(_ => refitSettings);

    services.AddRefitClient<IBoringCrudWithClientApi<User, string>>(refitSettings);

    var refitSettings2 = new RefitSettings
    {
        HttpClientName = "ClientName2", HttpClientBaseAddress = new Uri("https://bar:5002"),
    };
    services.AddTransient(_ => refitSettings2);

    services.AddRefitClient<IBoringOtherCrudWithClientApi<Role, string>>(refitSettings2);

    var boringApi = services.BuildServiceProvider().GetRequiredService<IBoringCrudWithClientApi<User, string>>();
    var boringOtherApi = services.BuildServiceProvider().GetRequiredService<IBoringOtherCrudWithClientApi<Role, string>>();
    var sameClient = ReferenceEquals(boringApi.Client, boringOtherApi.Client);

    Assert.False(sameClient);

    // Also confirm each client base address equals the corresponding settings base address.
    Assert.Equal(boringApi.Client.BaseAddress?.AbsoluteUri, refitSettings.HttpClientBaseAddress?.AbsoluteUri);
    Assert.Equal(boringOtherApi.Client.BaseAddress?.AbsoluteUri, refitSettings2.HttpClientBaseAddress?.AbsoluteUri);
}

I have a corresponding set of unit tests for the non-generic version of the AddRefit method as well.

Additional context

I am prepared to submit a PR if this functionality is deemed acceptable.

search4sound commented 2 years ago

Sorry, accidentally logged this under ReactiveUI, not Refit.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.