From a3c9d9086e65c771699fa93d089f8adc2eb8413f Mon Sep 17 00:00:00 2001 From: Todd Date: Fri, 3 Nov 2023 11:42:34 -0500 Subject: [PATCH] #770 FlurlClientCache.Add throws if client doesn't exist GetOrAdd should be used to add clients on demand --- .../Configuration/FlurlClientCache.cs | 47 ++++++++++++------- src/Flurl.Http/FlurlHttp.cs | 2 +- test/Flurl.Test/Http/FlurlClientCacheTests.cs | 20 ++++---- 3 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/Flurl.Http/Configuration/FlurlClientCache.cs b/src/Flurl.Http/Configuration/FlurlClientCache.cs index 40fca678..82354ce3 100644 --- a/src/Flurl.Http/Configuration/FlurlClientCache.cs +++ b/src/Flurl.Http/Configuration/FlurlClientCache.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Concurrent; +using System.Collections.Generic; namespace Flurl.Http.Configuration { @@ -17,14 +18,14 @@ public interface IFlurlClientCache IFlurlClientBuilder Add(string name, string baseUrl = null); /// - /// Gets a named IFlurlClient, creating one if it doesn't exist or has been disposed. + /// Gets a preconfigured named IFlurlClient. /// /// The client name. /// The cached IFlurlClient. IFlurlClient Get(string name); /// - /// Gets a named IFlurlClient, creating and configuring one if it doesn't exist or has been disposed. + /// Gets a named IFlurlClient, creating and (optionally) configuring one if it doesn't exist or has been disposed. /// /// The client name. /// The base URL associated with the new client, if it doesn't exist. @@ -76,38 +77,44 @@ public static IFlurlClientCache Add(this IFlurlClientCache cache, string name, s /// /// Default implementation of IFlurlClientCache. /// - public class FlurlClientCache : IFlurlClientCache { + public class FlurlClientCache : IFlurlClientCache + { private readonly ConcurrentDictionary> _clients = new(); - private Action _configureAll; + private readonly List> _configureAll = new(); /// public IFlurlClientBuilder Add(string name, string baseUrl = null) { if (name == null) throw new ArgumentNullException(nameof(name)); - var builder = new FlurlClientBuilder(baseUrl); - Lazy Create() { - _configureAll?.Invoke(builder); - return new Lazy(builder.Build); - } - - if (!_clients.TryAdd(name, Create())) - throw new ArgumentException($"A client named '{name}' was already registered with this factory. Add should be called just once per client at startup."); + var builder = CreateBuilder(baseUrl); + if (!_clients.TryAdd(name, new Lazy(builder.Build))) + throw new ArgumentException($"A client named '{name}' was already registered. Add should be called just once per client at startup."); return builder; } /// - public virtual IFlurlClient Get(string name) => GetOrAdd(name); + public virtual IFlurlClient Get(string name) { + if (name == null) + throw new ArgumentNullException(nameof(name)); + + if (!_clients.TryGetValue(name, out var cli)) + throw new ArgumentException($"A client named '{name}' was not found. Either preconfigure the client using Add (typically at startup), or use GetOrAdd to add/configure one on demand when needed."); + + if (cli.Value.IsDisposed) + throw new Exception($"A client named '{name}' was not found but has been disposed and cannot be reused."); + + return cli.Value; + } /// public IFlurlClient GetOrAdd(string name, string baseUrl = null, Action configure = null) { if (name == null) throw new ArgumentNullException(nameof(name)); - var builder = new FlurlClientBuilder(baseUrl); Lazy Create() { - _configureAll?.Invoke(builder); + var builder = CreateBuilder(baseUrl); configure?.Invoke(builder); return new Lazy(builder.Build); } @@ -117,7 +124,8 @@ Lazy Create() { /// public IFlurlClientCache ConfigureAll(Action configure) { - _configureAll = configure; + if (configure != null) + _configureAll.Add(configure); return this; } @@ -135,5 +143,12 @@ public IFlurlClientCache Clear() { Remove(key); return this; } + + private IFlurlClientBuilder CreateBuilder(string baseUrl) { + var builder = new FlurlClientBuilder(baseUrl); + foreach (var config in _configureAll) + config(builder); + return builder; + } } } diff --git a/src/Flurl.Http/FlurlHttp.cs b/src/Flurl.Http/FlurlHttp.cs index 937f5dd4..67f01156 100644 --- a/src/Flurl.Http/FlurlHttp.cs +++ b/src/Flurl.Http/FlurlHttp.cs @@ -26,7 +26,7 @@ public static class FlurlHttp /// /// Gets or creates the IFlurlClient that would be selected for sending the given IFlurlRequest when the clientless pattern is used. /// - public static IFlurlClient GetClientForRequest(IFlurlRequest req) => Clients.Get(_cachingStrategy(req)); + public static IFlurlClient GetClientForRequest(IFlurlRequest req) => Clients.GetOrAdd(_cachingStrategy(req)); /// /// Sets a global caching strategy for getting or creating an IFlurlClient instance when the clientless pattern is used, e.g. url.GetAsync. diff --git a/test/Flurl.Test/Http/FlurlClientCacheTests.cs b/test/Flurl.Test/Http/FlurlClientCacheTests.cs index d7846d33..2ffaad23 100644 --- a/test/Flurl.Test/Http/FlurlClientCacheTests.cs +++ b/test/Flurl.Test/Http/FlurlClientCacheTests.cs @@ -44,14 +44,14 @@ public void can_get_or_add_client() { } [Test] - public void can_get_unconfigured_client() { + public void can_get_or_add_unconfigured_client() { var cache = new FlurlClientCache(); - var cli = cache.Get("foo"); + var cli = cache.GetOrAdd("foo"); + Assert.IsNull(cli.BaseUrl); Assert.AreEqual(100, cli.Settings.Timeout.Value.TotalSeconds); - - Assert.AreSame(cli, cache.Get("foo"), "should reuse same-named clients."); - Assert.AreNotSame(cli, cache.Get("bar"), "different-named clients should be different instances."); + Assert.AreSame(cli, cache.GetOrAdd("foo"), "should reuse same-named clients."); + Assert.AreNotSame(cli, cache.GetOrAdd("bar"), "different-named clients should be different instances."); } [Test] @@ -66,7 +66,7 @@ public void can_configure_all() { var cache = new FlurlClientCache() .ConfigureAll(b => b.WithSettings(s => s.Timeout = TimeSpan.FromSeconds(123))); - var cli1 = cache.Get("foo"); + var cli1 = cache.GetOrAdd("foo"); cache.Add("bar").WithSettings(s => { s.Timeout = TimeSpan.FromSeconds(456); @@ -75,8 +75,8 @@ public void can_configure_all() { s.Timeout = TimeSpan.FromSeconds(789); })); - var cli2 = cache.Get("bar"); - var cli3 = cache.Get("buzz"); + var cli2 = cache.GetOrAdd("bar"); + var cli3 = cache.GetOrAdd("buzz"); Assert.AreEqual(123, cli1.Settings.Timeout.Value.TotalSeconds, "fetched the client before changing the defaults, so original should stick"); Assert.AreEqual(456, cli2.Settings.Timeout.Value.TotalSeconds, "client-specific settings should always win, even if defaults were changed after"); @@ -92,7 +92,9 @@ public void can_remove() { var cli1 = cache.Get("foo"); var cli2 = cache.Get("foo"); cache.Remove("foo"); - var cli3 = cache.Get("foo"); + + Assert.Throws(() => cache.Get("foo")); + var cli3 = cache.GetOrAdd("foo"); Assert.AreSame(cli1, cli2); Assert.AreNotSame(cli1, cli3);