Eliminate Flurl's HttpClientFactory

[tmenier]

NOTE: This issue was superseded by #770. In short, all factories are now gone, and this was an intermediate step in that direction.

---

Both Flurl and ASP.NET Core define a thing called IHttpClientFactory. This can cause confusion, and regardless of which came first, it's not a fight Flurl is likely to win, so I will concede and remove it from Flurl 😃.

But the methods it defines - CreateHttpClient and CreateMessageHandler - aren't simply going away, because they are still useful. Instead, they'll be rolled into IFlurlClientFactory and used in much the same way when you are not managing FlurlClients explicitly. One implication is that you'll no longer be able to assign an HttpClient-creating factory to a FlurlClient's settings directly. But I don't believe that's very useful anyway, since you can optionally pass an HttpClient instance to a FlurlClient constructor. And if you don't, the globally registered IFlurlClientFactory will be invoked to create one, even if you created the FlurlClient explicitly.

These changes should lead to some sweeping simplifications. The broader goal is to get Flurl to a place where it plays more nicely with ASP.NET Core's IHttpClientFactory in general.

BREAKING:

• IHttpClientFactory and its implementations removed. (Methods moved to IFlurlClientFactory.)
• ClientFlurlHttpSettings class removed.

[AeonLucid]

One implication is that you'll no longer be able to assign an HttpClient-creating factory to a FlurlClient's settings directly. But I don't believe that's very useful anyway, since you can optionally #298

May I ask why? I haven't followed the development of the internals but I was under the impression that Flurl would periodically refresh the underlying HttpClientHandler at some point.

[tmenier]

@AeonLucid

I was under the impression that Flurl would periodically refresh the underlying HttpClientHandler at some point.

Nope, Flurl doesn't do that, mainly because HttpClient takes care of those concerns by default since .NET Core 2.1, and older platforms can use the ConnectionLeaseTimeout solution.

[rcdailey]

@tmenier With the removal of HTTP client factory in 4.0-pre4, this no longer works:

public class UntrustedCertClientFactory : DefaultHttpClientFactory
{
    public override HttpMessageHandler CreateMessageHandler()
    {
        return new HttpClientHandler
        {
            ServerCertificateCustomValidationCallback = (_, _, _, _) => true
        };
    }
}

Could you provide an example of how one should disable SSL validation in 4.0? I'll keep tinkering with this, it's possible I eventually figure it out. But I think an example would be really helpful.

[tmenier]

@rcdailey Sure. The answer (per #770) is to use IFlurlClientBuilder.ConfigureInnerHandler, which takes a delegate allowing you to configure a Flurl-created instance of HttpClientHandler. How you get an IFlurlClientBuilder depends on your usage pattern and how "global" you want that setting to be. Here's some options:

1. Global, when using the "clientless" pattern:

FlurlHttp.Clients.WithDefaults(builder => builder.ConfigureInnerHandler(...)

2. Global, when injecting clients via DI:

services.AddSingleton<IFlurlClientCache>(_ => new FlurlClientCache()
    .WithDefaults(builder => builder.ConfigureInnerHandler(...)

Inject instances of IFlurlClientCache into your service classes and call Get(name) to get an IFlurlClient instance. (Instances are reused per name and do not need to be pre-configured.)

3. Specific named client via DI:

services.AddSingleton<IFlurlClientCache>(_ => new FlurlClientCache()
    .Add(name, baseUrl, builder => builder.ConfigureInnerHandler(...)));

Same usage as above but here you've configured it for a specific client.

4. One-off (recommended only for the simplest of cases):

var flurlClient = new FlurlClientBuilder(baseUrl)
    .ConfigureInnerHandler(...)
    .Build();

[rcdailey]

Thank you, I really appreciate the examples @tmenier. The new configuration strategy is a bit difficult for me to adopt. Add(), in particular, seems to require me to know all of my HTTP clients up front. However, in my case, HTTP clients are determined by configuration provided by the user (which isn't known until much later after start up in the application). Essentially, they provide a YAML file with server configurations. And based on that, I have a custom object I use to construct flurl clients and build requests as needed. I don't really put any HTTP setup in my composition root. As an example, here is my builder for clients as it functions in pre3:

public class ServarrRequestBuilder : IServarrRequestBuilder
{
    private readonly ILogger _log;
    private readonly ISettingsProvider _settingsProvider;
    private readonly IFlurlClientFactory _clientFactory;

    public ServarrRequestBuilder(ILogger log, ISettingsProvider settingsProvider, IFlurlClientFactory clientFactory)
    {
        _log = log;
        _settingsProvider = settingsProvider;
        _clientFactory = clientFactory;
    }

    public IFlurlRequest Request(IServiceConfiguration config, params object[] path)
    {
        var client = _clientFactory.Get(config.BaseUrl.AppendPathSegments("api", "v3"));
        client.Settings = GetClientSettings();
        return client.Request(path)
            .WithHeader("X-Api-Key", config.ApiKey);
    }

    private ClientFlurlHttpSettings GetClientSettings()
    {
        var settings = new ClientFlurlHttpSettings
        {
            JsonSerializer = new DefaultJsonSerializer(GlobalJsonSerializerSettings.Services)
        };

        FlurlLogging.SetupLogging(settings, _log);

        // ReSharper disable once InvertIf
        if (!_settingsProvider.Settings.EnableSslCertificateValidation)
        {
            _log.Warning(
                "Security Risk: Certificate validation is being DISABLED because setting " +
                "`enable_ssl_certificate_validation` is set to `false`");
            settings.HttpClientFactory = new UntrustedCertClientFactory();
        }

        return settings;
    }
}

However, with pre5, this implementation is no longer centralized (as far as I'm able to tell). I have to split off my GetClientSettings logic into the ConfigureAll() which I need to add to my composition root. I thought about stuffing that logic in an object I can resolve via DI in the ConfigureAll() callback that I pass the builder into and let it do that stuff. But it feels a little messy.

Then there's Add(name, baseurl) which I need to lazily call in my Request() method above. It's not a drop in replacement for the client factory I'm currently using. According to the Add() docs, I need to call it once per named client at startup somewhere. But I do not want to have to add extra logic to obtain, parse, and iterate my configuration just to set up clients. I prefer that be done when I need them.

Perhaps a solution here is to add an overload of Get() that takes a lambda that gets called when Add() needs to be invoked internally. The FlurlClientCache class also has a flurl client factory that I might be able to tap into, since it already has support for lazily constructed clients, but it does not provide a way to pass in a custom factory (it's hard-coded in the implementation).

Looks like Get() creates the client (with a null base url) if it doesn't exist, so I can't really use that either, otherwise I could do something like this:

var client = _clientCache.Get(name);
if (client is null) {
    client = _clientCache.Add(name, myBaseUrl);
}

[rcdailey]

Here's where I ended up. Seems to work, I think my main feedback is to make the default HttpClientCache more flexible. Add the ability to check if a client already exists and/or introduce an AddOrCreate method of some kind that allows the Base URL to be set when a client is created. I can't use the normal configuration paths for this since setting the base URL requires a configuration object passed down through the call graph; it isn't available via DI since it's context-specific.

Used by my own business logic to get/create a client, and set it up for API calls:

public class ServarrRequestBuilder : IServarrRequestBuilder
{
    private readonly IFlurlClientCache _clientCache;

    public ServarrRequestBuilder(IFlurlClientCache clientCache)
    {
        _clientCache = clientCache;
    }

    public IFlurlRequest Request(IServiceConfiguration config, params object[] path)
    {
        var client = _clientCache.Get(config.InstanceName);
        client.BaseUrl ??= config.BaseUrl.AppendPathSegments("api", "v3");
        return client.Request(path)
            .WithHeader("X-Api-Key", config.ApiKey);
    }
}

Used during creation of the IFlurlClientCache (via the composition root) to set up all new clients:

public class FlurlConfigurator
{
    private readonly ILogger _log;
    private readonly ISettingsProvider _settingsProvider;

    public FlurlConfigurator(ILogger log, ISettingsProvider settingsProvider)
    {
        _log = log;
        _settingsProvider = settingsProvider;
    }

    [SuppressMessage("SonarCloud", "S4830:Server certificates should be verified during SSL/TLS connections")]
    [SuppressMessage("Security", "CA5359:Do Not Disable Certificate Validation")]
    public void Configure(IFlurlClientBuilder builder)
    {
        builder.WithSettings(settings =>
        {
            settings.JsonSerializer = new DefaultJsonSerializer(GlobalJsonSerializerSettings.Services);
            FlurlLogging.SetupLogging(settings, _log);
        });

        builder.ConfigureInnerHandler(handler =>
        {
            if (!_settingsProvider.Settings.EnableSslCertificateValidation)
            {
                _log.Warning(
                    "Security Risk: Certificate validation is being DISABLED because setting " +
                    "`enable_ssl_certificate_validation` is set to `false`");

                handler.SslOptions = new SslClientAuthenticationOptions
                {
                    RemoteCertificateValidationCallback = (_, _, _, _) => true
                };
            }
        });
    }
}

Relevant part of my composition root (which uses Autofac) to set up the relevant registrations:

builder.RegisterType<FlurlConfigurator>();
builder.Register(c =>
    {
        var config = c.Resolve<FlurlConfigurator>();
        return new FlurlClientCache().ConfigureAll(x => config.Configure(x));
    })
    .As<IFlurlClientCache>()
    .SingleInstance();

[tmenier]

I think what you did here is good. For your injected singleton "provider" thing, you basically just replaced IFlurlClientFactory with IFlurlClientCache, which is correct - the latter functions much like the former did with respect to caching. And you're setting up "common" configuration in your composition root. All good!

If I understand you correctly, I think you're requesting something similar to ConcurrentDictionary.GetOrAdd that will allow you to add to the cache any time but be guaranteed to initialize just once? That's actually not a bad idea, I'll consider it. But it's worth noting that Flurl never had that functionality, so nothing was actually "lost" between pre3 and pre4. But I think you may have figured that out at some point between your last 2 comments if I read them right. :)

[rcdailey]

Yeah, I think you're right about not having lost anything. I realized that I was always setting the settings for clients, even if those settings had already been set before. So I think what we're ending up with here is a new feature request, which might be convenient to add with the rest of the churn happening in v4.0. You already do lazy initialization inside the default cache implementation; it's probably just a matter of exposing that in the public interface so that users can perform one-time initialization at the cache level (for setup involving non-DI injected configuration, like I have).

Thanks so much, I'm enjoying the progress you're making!

[rcdailey]

Here is an example of how I would probably use such a new feature:

var client = _clientCache.GetOrAdd(config.InstanceName, client => 
{
    // This lambda is invoked ONCE only when a new client is created. That client
    // is passed to the lambda to perform initialization (in this case, setting the base url)
    client.BaseUrl = config.BaseUrl.AppendPathSegments("api", "v3");
});

You'll notice I currently work around this by using ??= to assign to it after its been created (See my sample code 2 posts up). It's not ideal, though.

A lambda might be too generic of a solution here. If there's not much to configure outside of a Base URL and you're concerned that users might abuse this mechanism of configuration (instead of doing it via ConfigureAll()), I'd be OK with something like this:

var client = _clientCache.GetOrAdd(config.InstanceName, config.BaseUrl.AppendPathSegments("api", "v3"));

Basically the base URL provided would be assigned to the client only when created instead of the hard-coded null value that Get() uses today.

With the addition of a GetOrAdd(), it's probably worth making Get() throw if it doesn't already exist. I personally feel like this is less confusing and error prone, but not something I feel strongly for. I'll leave it up to you to decide. Just seems like you wouldn't want 3 different ways to create new clients in the cache object.

[tmenier]

@rcdailey I agree with your assessment of how GetOrSet and Get should behave. #770 has been updated. Thanks for the feedback, I think these are good suggestions that a lot of people will find useful.

[rcdailey]

@tmenier I finally got around to implementing the changes you made in pre6 and overall it is a huge improvement. You did fantastic work. Here are the things I found were improvements since pre5:

• I can move configuration from my DI root to my "builder" class. This keeps things centralized for my specific use case, which I find a bit cleaner (I don't necessarily want these specific settings applied to all clients, just the ones used in this specific context).
• I can provide the base URL and settings when I "Get" a client from the cache, which I'm obviously happy with for reasons we previously discussed.

I'll provide my "builder" class below so you can see the updated version as well as get an idea of what I'm doing. If I had to give feedback on anything, I'd say that with this change, each time I request a client I'm doing the work to build the base URL each time, even if that value doesn't get used (i.e. when the client already exists). However, this is very minor and not a huge deal.

public class ServarrRequestBuilder(
    ILogger log,
    IFlurlClientCache clientCache,
    ISettingsProvider settingsProvider)
    : IServarrRequestBuilder
{
    public IFlurlRequest Request(IServiceConfiguration config, params object[] path)
    {
        var client = clientCache.GetOrAdd(
            config.InstanceName,
            config.BaseUrl.AppendPathSegments("api", "v3"),
            Configure);

        return client.Request(path)
            .WithHeader("X-Api-Key", config.ApiKey);
    }

    [SuppressMessage("SonarCloud", "S4830:Server certificates should be verified during SSL/TLS connections")]
    [SuppressMessage("Security", "CA5359:Do Not Disable Certificate Validation")]
    private void Configure(IFlurlClientBuilder builder)
    {
        builder.WithSettings(settings =>
        {
            settings.JsonSerializer = new DefaultJsonSerializer(GlobalJsonSerializerSettings.Services);
            FlurlLogging.SetupLogging(settings, log);
        });

        builder.ConfigureInnerHandler(handler =>
        {
            if (!settingsProvider.Settings.EnableSslCertificateValidation)
            {
                log.Warning(
                    "Security Risk: Certificate validation is being DISABLED because setting " +
                    "`enable_ssl_certificate_validation` is set to `false`");

                handler.ServerCertificateCustomValidationCallback = (_, _, _, _) => true;
            }
        });
    }
}

[tmenier]

@rcdailey Great feedback, thank you.

I'm doing the work to build the base URL each time

Yeah, that occurred to me, but I do think that base URL is going to be the most common config needing to be set and I like that you can do it directly and skip the config func entirely if you don't need to do anything else. I also think your case might be a little unusual - I suspect in most cases the base URL will come from some config in its entirety and won't need to be "built". You could just use string concat or interpolation there instead if it makes it feel less heavy. :)

[jassent]

2. Global, when injecting clients via DI:

services.AddSingleton<IFlurlClientCache>(_ => new IFlurlClientCache()
    .ConfigureAll(builder => builder.ConfigureInnerHandler(...)

3. Specific named client via DI:

services.AddSingleton<IFlurlClientCache>(_ => new IFlurlClientCache()
    .Add(name, baseUrl)
    .ConfigureInnerHandler(...)

I understand this is a pre-release but the examples provided for Dependency Injection have invalid syntax and will not compile (even after mocking up ConfigureInnerHandler). I tried both 4.0.0-pre4 and 4.0.0-pre6.

For example:

services.AddSingleton<IFlurlClientCache>(_ => new IFlurlClientCache()
     .Add("testclient", "https://localhost")
     .ConfigureInnerHandler(h => { }));

The error is:
CS0144, Cannot create an instance of the abstract type or interface 'IFlurlClientCache'

PS - Thank you for the awesome library!

[rcdailey]

new IFlurlClientCache() should be new FlurlClientCache(). I use Autofac and I do not put my client configuration in my composition root, so my example won't be 100% relevant but this is what I do:

builder.RegisterType<FlurlClientCache>()
    .As<IFlurlClientCache>()
    .SingleInstance();

I imagine you already knew this and were just pointing out the updates needed to the interim docs. But thought I'd point it out anyway.

[tmenier]

@jassent Fixed, thanks for catching that. It was more or less a typo - IFlurlClientCache is an interface; FlurlClientCache is the default implementation.

[jassent]

@jassent Fixed, thanks for catching that. It was more or less a typo - IFlurlClientCache is an interface; FlurlClientCache is the default implementation.

Thank you for the update. I think there is still in an issue. This does not compile:

services.AddSingleton<IFlurlClientCache>(_ => new FlurlClientCache()
                                .Add("testclient", "https://localhost")
                                .ConfigureInnerHandler(h => { }));

The error is:
CS0266 Cannot implicitly convert type 'Flurl.Http.Configuration.IFlurlClientBuilder' to 'Flurl.Http.Configuration.IFlurlClientCache'. An explicit conversion exists (are you missing a cast?)

This is in Visual Studio 17.8.2 NET7 using either 4.0.0-pre4 and 4.0.0-pre6

Updating to this explicit casting works:

services.AddSingleton<IFlurlClientCache>(_ => (IFlurlClientCache)new FlurlClientCache()
                                .Add("testclient", "https://localhost")
                                .ConfigureInnerHandler(h => { }));

And I am not sure how to use ConfigureInnerHandler properly - but that is my problem :)

[tmenier]

@jassent You're on a roll. :) Comment updated. In order for .Add to return itself (the IFlurlClientCache) you have to use the overload that takes a nested config action.

The previous way could be made to work and requires less nesting but isn't chainable, so it would require multiple steps:

services.AddSingleton<IFlurlClientCache>(_ => {
    var cache = new FlurlClientCache();
    cache.Add("testclient", "https://localhost")
        .ConfigureInnerHandler(h => { }));
    return cache;
});

[tmenier]

@jassent The more I think about this, I think I'm going to drop the overload that sent you down the wrong path. I can see it causing confusion (even without my incorrect code samples 😉), and it can always be added later as a non-breaking enhancement if people want it. I'll update #770.

[astrohart]

Figure 1. Visual Studio showing red errors after upgrading Flurl to the latest version.

Hello, I am a little new to Flurl. When using an older version, I could do (authenticates a user to Coinbase using their API Key and Secret):

      protected internal override void Configure(CoinbaseClient client)
      {
         client.Configure(settings => ApiKeyAuth(settings, client));
      }

      private void ApiKeyAuth(ClientFlurlHttpSettings settings, CoinbaseClient client)
      {
         async Task SetHeaders(FlurlCall http)
         {
            var body = http.RequestBody;
            var method = http.Request.Verb.Method.ToUpperInvariant();
            var url = http.Request.Url.ToUri().PathAndQuery;

            string timestamp;
            if (this.UseTimeApi)
            {
               var timeResult = await client.Data.GetCurrentTimeAsync().ConfigureAwait(false);
               timestamp = timeResult.Data.Epoch.ToString();
            }
            else
            {
               timestamp = TimeHelper.GetCurrentUnixTimestampSeconds().ToString(CultureInfo.CurrentCulture);
            }

            var signature = ApiKeyAuthenticator.GenerateSignature(timestamp, method, url, body, this.ApiSecret);

            http.Request
               .WithHeader(HeaderNames.AccessKey, this.ApiKey)
               .WithHeader(HeaderNames.AccessSign, signature)
               .WithHeader(HeaderNames.AccessTimestamp, timestamp);
         }

         settings.BeforeCallAsync = SetHeaders;
      }

Listing 1. This code worked, now it's broken because ClientFlurlHttpSettings went away.

I am drawing a blank on how to fix this. Can I get some help? See Figure 1 for the things that Visual Studio is painting red. BTW -- the CoinbaseClient class is derived from FlurlClient.

[astrohart]

11:18 01/04/2024

OK, I've looked up in the docs and I changed client.Configure(settings => ...) to client.WithSettings(settings => ...), and I updated the first parameter of ApiKeyAuth to FlurlHttpSettings and it seems to make the compiler errors go away...except that settings.BeforeCallAsync is still angry red...

      protected internal override void Configure(CoinbaseClient client)
      {
         client.WithSettings(settings => ApiKeyAuth(settings, client));
      }

      private void ApiKeyAuth(FlurlHttpSettings settings, CoinbaseClient client)
      {
         async Task SetHeaders(FlurlCall http)
         {
            var body = http.RequestBody;
            var method = http.Request.Verb.Method.ToUpperInvariant();
            var url = http.Request.Url.ToUri().PathAndQuery;

            string timestamp;
            if (this.UseTimeApi)
            {
               var timeResult = await client.Data.GetCurrentTimeAsync().ConfigureAwait(false);
               timestamp = timeResult.Data.Epoch.ToString();
            }
            else
            {
               timestamp = TimeHelper.GetCurrentUnixTimestampSeconds().ToString(CultureInfo.CurrentCulture);
            }

            var signature = ApiKeyAuthenticator.GenerateSignature(timestamp, method, url, body, this.ApiSecret);

            http.Request
               .WithHeader(HeaderNames.AccessKey, this.ApiKey)
               .WithHeader(HeaderNames.AccessSign, signature)
               .WithHeader(HeaderNames.AccessTimestamp, timestamp);
         }

         settings.BeforeCallAsync = SetHeaders;
      }

I have to sign each request by computing a signature and this has to be done on each and every call. I was using BeforeCallAsync to do this. The signature has to include a timestamp that is within a certain temporal window (30 sec) of the server's clock, or otherwise it expires.

What is the new old fashioned way of doing this?

[astrohart]

https://flurl.dev/docs/configuration/#event-handlers

Aha! I consulted the web page above and I did:

      private void ApiKeyAuth(FlurlHttpSettings settings, CoinbaseClient client)
      {
         async Task SetHeaders(FlurlCall http)
         {
            var body = http.RequestBody;
            var method = http.Request.Verb.Method.ToUpperInvariant();
            var url = http.Request.Url.ToUri().PathAndQuery;

            string timestamp;
            if (this.UseTimeApi)
            {
               var timeResult = await client.Data.GetCurrentTimeAsync().ConfigureAwait(false);
               timestamp = timeResult.Data.Epoch.ToString();
            }
            else
            {
               timestamp = TimeHelper.GetCurrentUnixTimestampSeconds().ToString(CultureInfo.CurrentCulture);
            }

            var signature = ApiKeyAuthenticator.GenerateSignature(timestamp, method, url, body, this.ApiSecret);

            http.Request
               .WithHeader(HeaderNames.AccessKey, this.ApiKey)
               .WithHeader(HeaderNames.AccessSign, signature)
               .WithHeader(HeaderNames.AccessTimestamp, timestamp);
         }

         client.BeforeCall(SetHeaders);
      }

This made my compiler errors go away.