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

[API Proposal]: Consider adding methods to the HTTP Client factory that receive Type parameters instead of generic type parameters #104525

Open
paulomorgado opened this issue Jul 7, 2024 · 19 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory
Milestone

Comments

@paulomorgado
Copy link
Contributor

paulomorgado commented Jul 7, 2024

Updated by @CarnaViire

API:

namespace Microsoft.Extensions.DependencyInjection;

public static class HttpClientBuilderExtensions
{
    public static IHttpClientBuilder AddTypedClient(
        this IHttpClientBuilder builder,
        Type clientType,
        Type? ImplementationType = null);
}

Usage:

services.AddHttpClient(name)
    .AddTypedClient(clientType, clientImplementationType);

Original issue by @paulomorgado

Background and motivation

Sometimes I have the need to register HTTP clients from configuration files and, for that, I need to construct a generic method for each type.

API Proposal

namespace Microsoft.Extensions.DependencyInjection;

public static class HttpClientFactoryServiceCollectionExtensions
{
    public static IHttpClientBuilder AddHttpClient(
        this IServiceCollection services,
        Type clientType,
        Type ImplementationType);
}

API Usage

service.AddHttpClient(clientType, implementationType);

Alternative Designs

No response

Risks

There might be a risk for overloads with factory methods, as correctness might be hard to guarantee.

@paulomorgado paulomorgado added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 7, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

Sometimes I have the need to register HTTP clients from configuration files...

Would you mind sharing an example of what you are doing?

I fail to see why you are not using named (but untyped) HTTP client registrations and using IHttpClientFactory explicitly with a dynamic setup. No need to call that overload you are proposing then: you just register the various HttpClients separately.

@paulomorgado
Copy link
Contributor Author

@julealgon,

Sometimes I have the need to register HTTP clients from configuration files...

Would you mind sharing an example of what you are doing?

I fail to see why you are not using named (but untyped) HTTP client registrations and using IHttpClientFactory explicitly with a dynamic setup. No need to call that overload you are proposing then: you just register the various HttpClients separately.

Instead of explicitly registering the HTTP clients in code, I use configuration for that.

At startup, I add all the HTTP clients in the configuration. And the configuration might have type HTTP clients.

@julealgon
Copy link

@paulomorgado I think you misunderstood what I asked. I 100% understand that you are dynamically registering the clients based on configuration, but my point specifically was that you don't need to use the typed registration methods to achieve that, you can just used named registrations instead and then inject IHttpClientFactory into your consumers and grab the named client from that.

If you know how WCF config-based bindings worked in the past, this would be very similar to that.

@paulomorgado
Copy link
Contributor Author

@julealgon,

@paulomorgado I think you misunderstood what I asked. I 100% understand that you are dynamically registering the clients based on configuration, but my point specifically was that you don't need to use the typed registration methods to achieve that, you can just used named registrations instead and then inject IHttpClientFactory into your consumers and grab the named client from that.

If you know how WCF config-based bindings worked in the past, this would be very similar to that.

I know I can do that, but my components have the typed clients injected and I never inject the IHttpClientFactory.

Just because I can inject IHttpClientFactory, doesn't mean I should.

@julealgon
Copy link

I know I can do that, but my components have the typed clients injected and I never inject the IHttpClientFactory.

Just because I can inject IHttpClientFactory, doesn't mean I should.

Fair enough. I was just checking whether the named registrations could be a way for your use case: looks like it isn't.

Would still be nice if you could share a sample of your problem to make a stronger case for the proposed API.

In the meantime, you could just register the types yourself using ActivatorUtilities:

services.AddHttpClient(myHttpClientName, //...configure here...
services.AddTransient(myInterface, provider => ActivatorUtilities.CreateInstance(
    provider, 
    myType, 
    provider.GetRequiredService<IHttpClientFactory>().CreateClient(myHttpClientName))

Which you could, of course, abstract away into your own generic helper method.

The problem I see with the original AddHttpClient<T> methods is that they.... don't scale as an API: eventually, people will request for every single variation that exists for other Add... methods, and I'm personally not that fond of that design: I'd rather see it die in favor of a general purpose "dependent" registration feature. But alas.

@paulomorgado
Copy link
Contributor Author

I have a working solution. I just don't think this is my requirement only and the factory usability would benefit from this.

@julealgon
Copy link

I have a working solution.

Again, I would suggest sharing your use case and your current workaround so more people can see the benefit of the new API and can rely on your workaround in the meantime.

I just don't think this is my requirement only and the factory usability would benefit from this.

I've never seen anyone else request this myself. All dynamic registration discussions revolved around named httpclient registrations or even using the new keyed registration support.

@paulomorgado
Copy link
Contributor Author

I am dynamically registering typed HTTP clients.

The particulars of why I need to do it are not important nor I will discuss them publicly.

@CarnaViire
Copy link
Member

Thanks for the proposal @paulomorgado

Triage:

There are already 20 (!) overloads of AddHttpClient. We are extremely reluctant to expand this even further, as it will hurt the usability more than it will help. Given that there are working solutions for the problem described (incl. the workarounds proposed above, thanks @julealgon), and there is not much customer ask for that (this is the first one that I'm aware of), we're inclined towards closing the issue.

Please let me know if I missed something, or there's some additional reasons why we should reconsider. Thanks!

@CarnaViire CarnaViire closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2024
@paulomorgado
Copy link
Contributor Author

This is what I have working now (in case anyone needs it):

public static class MyHttpClientFactoryDependencyInjectionExtensions
{
    private static readonly MethodInfo _addHttpClientByClientTypeMethodInfo =
        typeof(HttpClientFactoryServiceCollectionExtensions)
            .GetMethod(
                nameof(HttpClientFactoryServiceCollectionExtensions.AddHttpClient),
                1,
                new Type[] { typeof(IServiceCollection) })!;

    private static readonly MethodInfo _addHttpClientByClientTypeWithNameMethodInfo =
        typeof(HttpClientFactoryServiceCollectionExtensions)
            .GetMethod(
                nameof(HttpClientFactoryServiceCollectionExtensions.AddHttpClient),
                1,
                new Type[] { typeof(IServiceCollection), typeof(string) })!;

    private static readonly MethodInfo _addHttpClientByClientAndClientImplementationTypeMethodInfo =
        typeof(HttpClientFactoryServiceCollectionExtensions)
            .GetMethod(
                nameof(HttpClientFactoryServiceCollectionExtensions.AddHttpClient),
                2,
                new Type[] { typeof(IServiceCollection) })!;

    private static readonly MethodInfo _addHttpClientByClientAndClientImplementationTypeWithNameMethodInfo =
        typeof(HttpClientFactoryServiceCollectionExtensions)
            .GetMethod(
                nameof(HttpClientFactoryServiceCollectionExtensions.AddHttpClient),
                2,
                new Type[] { typeof(IServiceCollection), typeof(string) })!;

    public static IServiceCollection AddHttpClients(this IServiceCollection services, IConfiguration configuration)
    {
        // ...
        
        foreach (var section in configuration.GetChildren())
        {
            var name = section.Key;
            var clientTypeName = section.GetSection("ClientType")?.Value ?? name;
            var clientImplementationTypeName = section.GetSection("ClientImplementationType")?.Value;

            var clientType = Type.GetType(clientTypeName);
            var clientImplementationType = string.IsNullOrEmpty(clientImplementationTypeName)
                ? null
                : Type.GetType(clientImplementationTypeName) ?? throw new InvalidOperationException($"The implementation type '{clientImplementationTypeName}' was not found for client '{name}'.");

            if (clientImplementationType is not null && clientType is null)
            {
                throw new InvalidOperationException($"The implementation type '{clientImplementationType.AssemblyQualifiedName}' was configured, but the client type '{clientTypeName}' was not found for client '{name}'.");
            }

            var http = clientType is null
                ? services.AddHttpClient(name)
                : clientImplementationType is null
                    ? services.AddHttpClient(clientType, name)
                    : services.AddHttpClient(clientType, clientImplementationType, name);

            http
                .ConfigureHttpClient(httpClient =>
                {
                    // ...
                });

            ConfigureClientCredentialsTokenHandler(http, section);

            ConfigureStandardResilience(http, section);
        }

        return services;
    }

    public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, Type clientType)
        => (IHttpClientBuilder) _addHttpClientByClientTypeMethodInfo
            .MakeGenericMethod(clientType)
            .Invoke(null, new object[] { services })!;

    public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, Type clientType, string name)
        => (IHttpClientBuilder) _addHttpClientByClientTypeWithNameMethodInfo
            .MakeGenericMethod(clientType)
            .Invoke(null, new object[] { services, name })!;

    public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, Type clientType, Type clientImplementationType)
        => (IHttpClientBuilder) _addHttpClientByClientAndClientImplementationTypeMethodInfo
            .MakeGenericMethod(clientType, clientImplementationType)
            .Invoke(null, new object[] { services })!;

    public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, Type clientType, Type clientImplementationType, string name)
        => (IHttpClientBuilder) _addHttpClientByClientAndClientImplementationTypeWithNameMethodInfo
            .MakeGenericMethod(clientType, clientImplementationType)
            .Invoke(null, new object[] { services, name })!;
}

@CarnaViire
Copy link
Member

Thanks for sharing @paulomorgado!

While that would 100% align with what the factory is doing with the Typed clients (given the actual methods are used), for the sake of simplicity let me also save here how it would look like with the Activator approach:

public static class MyHttpClientFactoryDependencyInjectionExtensions
{
    public static IHttpClientBuilder AddHttpClient(this IServiceCollection services, string name, Type? clientType, Type? clientImplementationType)
    {
        if (clientImplementationType is not null && clientType is null)
        {
            throw new InvalidOperationException($"The implementation type '{clientImplementationType.AssemblyQualifiedName}' was configured, but the client type '{clientTypeName}' was not found for client '{name}'.");
        }

        if (clientType is not null)
        {
            clientImplementationType ??= clientType;

            services.AddTransient(clientType, serviceProvider =>
                ActivatorUtilities.CreateInstance(
                    serviceProvider,
                    clientImplementationType,
                    serviceProvider.GetRequiredService<IHttpClientFactory>().CreateClient(name)));
        }

        return services.AddHttpClient(name);
    }

This could be expanded further to e.g. cache the object activator method like DefaultTypedHttpClientFactory is doing under the hood.

(technically even ITypedHttpClientFactory itself can be used, though this would also require reflection to get access to the TClient ITypedHttpClientFactory<TImplementation>.CreateClient(HttpClient httpClient) method)

The only problem is that if e.g. #89755 gets implemented, then the actual Typed client registration might change a bit (e.g. getting a client as a keyed service instead of a direct factory.CreateClient(name)). One could do smth to adapt to such changes in the future, e.g.

static HttpClient CreateHttpClient(IServiceProvider serviceProvider, string name)
{
    HttpClient? fromKeyed = null;
    if (serviceProvider is IKeyedServiceProvider keyedServiceProvider)
    {
        fromKeyed = keyedServiceProvider.GetKeyedService<HttpClient>(name);
    }

    return fromKeyed ?? serviceProvider.GetRequiredService<IHttpClientFactory>().CreateClient(name);
}

(-- or maybe even switch from typed clients to services with a keyed dependency?)

@paulomorgado
Copy link
Contributor Author

@CarnaViire,

Whatever reason justifies having typed clients in IHttpClientFactory, justifies loading them from a catalog.

By the way, I had created a HTTP client factory that loaded definitions from a catalog before .NET Core had IHttpClientFactory. I'm just dropping it and replacing it with IHttpClientFactory.

My catalogue already as a feature (omitted in the code) to register a keyed HttpClient.

Also, there's a special keyed IHttpClientFactory that creates only one type of client.

As for the ActivatorUtilities, I though about that, but I tried to stay as much away from duplicating the internals of AddHttpClient. But you got me thinking about it again.

@CarnaViire
Copy link
Member

@paulomorgado

My catalogue already as a feature (omitted in the code) to register a keyed HttpClient.

If this feature is already used in production -- would you mind sharing more re: your experience with keyed clients? (here or in the related issue #89755) I don't know the circumstances in which your catalogue is used, but I'm still curious about the perspective. I'm asking because there are some tricky issues, like HttpClient being IDisposable, which leads to it being captured by DI. I wonder whether you've hit them in practice.

Also -- if HttpClientFactory ends up registering keyed clients by default, will that break your code?

Also, there's a special keyed IHttpClientFactory that creates only one type of client.

That is a very interesting idea, actually 👀

I though about that, but I tried to stay as much away from duplicating the internals of AddHttpClient.

Yeah, I know it's a pain. Sorry about that 😞

@paulomorgado
Copy link
Contributor Author

So far, I haven't hit any issues regarding HttpClient being disposable. I always use it as transient and inject the factory for other scopes.

If clients are registered as keyed, I'll have to remove some code. 😄

@CarnaViire
Copy link
Member

@paulomorgado

I've been thinking about it a bit more, and I believe if we add an overload to httpClientBuilder.AddTypedClient then it can fit your case https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.httpclientbuilderextensions.addtypedclient?view=net-8.0

services.AddHttpClient(name)
    .AddTypedClient(clientType, clientImplementationType);

(It's not as heavy as AddHttpClient overload-wise, so we won't be as opposed to that 😄)

I think I can resurrect the issue (and this can also help to gauge interest)

I'd still have to triage it to Future as a nice-to-have at this point (just setting expectations)

@CarnaViire CarnaViire reopened this Jul 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 12, 2024
@CarnaViire CarnaViire added this to the Future milestone Jul 12, 2024
@CarnaViire CarnaViire removed the untriaged New issue has not been triaged by the area owner label Jul 12, 2024
@CarnaViire
Copy link
Member

If you are interested in the feature, please upvote the top post, it will help us prioritize. Thanks!

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Jul 12, 2024

@julealgon,

@paulomorgado

I've been thinking about it a bit more, and I believe if we add an overload to httpClientBuilder.AddTypedClient then it can fit your case https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.httpclientbuilderextensions.addtypedclient?view=net-8.0

services.AddHttpClient(name)
    .AddTypedClient(clientType, clientImplementationType);

(It's not as heavy as AddHttpClient overload-wise, so we won't be as opposed to that 😄)

I think I can resurrect the issue (and this can also help to gauge interest)

I'd still have to triage it to Future as a nice-to-have at this point (just setting expectations)

That would work for me just fine.

I wouldn't even mind not having

services.AddHttpClient(name)
    .AddTypedClient(clientType);

but I'm sure someone will complain about that. 😄

@julealgon
Copy link

but I'm sure someone will complain about that. 😄

From my perspective, the issue is not really the specific methods being suggested, those are completely fine. I have a problem with perpetuating this entire set of special-cased HttpClient APIs.

IMHO, the more we keep piling on it, the worse it will become overall. I've made a comment here with a bit more detail on that thought:

To me, none of these APIs should even exist. They should be replaced and deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-HttpClientFactory
Projects
None yet
Development

No branches or pull requests

3 participants