diff --git a/src/Polly.Extensions/DependencyInjection/PollyServiceCollectionExtensions.cs b/src/Polly.Extensions/DependencyInjection/PollyServiceCollectionExtensions.cs index c310f22976e..ad98e604eff 100644 --- a/src/Polly.Extensions/DependencyInjection/PollyServiceCollectionExtensions.cs +++ b/src/Polly.Extensions/DependencyInjection/PollyServiceCollectionExtensions.cs @@ -234,7 +234,6 @@ public static IServiceCollection AddResiliencePipelineRegistry(this IServi return services; } - services.AddOptions(); services.Add(RegistryMarker.ServiceDescriptor); services.AddResiliencePipelineBuilder(); diff --git a/src/Polly/Bulkhead/AsyncBulkheadTResultSyntax.cs b/src/Polly/Bulkhead/AsyncBulkheadTResultSyntax.cs index a174fda319e..935960d60cc 100644 --- a/src/Polly/Bulkhead/AsyncBulkheadTResultSyntax.cs +++ b/src/Polly/Bulkhead/AsyncBulkheadTResultSyntax.cs @@ -11,10 +11,7 @@ public partial class Policy /// The maximum number of concurrent actions that may be executing through the policy. /// The policy instance. public static AsyncBulkheadPolicy BulkheadAsync(int maxParallelization) - { - Func doNothingAsync = _ => TaskHelper.EmptyTask; - return BulkheadAsync(maxParallelization, 0, doNothingAsync); - } + => BulkheadAsync(maxParallelization, 0, EmptyHandler); /// /// Builds a bulkhead isolation , which limits the maximum concurrency of actions executed through the policy. Imposing a maximum concurrency limits the potential of governed actions, when faulting, to bring down the system. @@ -40,10 +37,7 @@ public static AsyncBulkheadPolicy BulkheadAsync(int maxParalle /// maxParallelization;Value must be greater than zero. /// maxQueuingActions;Value must be greater than or equal to zero. public static AsyncBulkheadPolicy BulkheadAsync(int maxParallelization, int maxQueuingActions) - { - Func doNothingAsync = _ => TaskHelper.EmptyTask; - return BulkheadAsync(maxParallelization, maxQueuingActions, doNothingAsync); - } + => BulkheadAsync(maxParallelization, maxQueuingActions, EmptyHandler); /// /// Builds a bulkhead isolation , which limits the maximum concurrency of actions executed through the policy. Imposing a maximum concurrency limits the potential of governed actions, when faulting, to bring down the system. @@ -79,4 +73,6 @@ public static AsyncBulkheadPolicy BulkheadAsync(int maxParalle maxQueuingActions, onBulkheadRejectedAsync); } + + private static Task EmptyHandler(Context context) => TaskHelper.EmptyTask; } diff --git a/src/Polly/Caching/CacheSyntax.cs b/src/Polly/Caching/CacheSyntax.cs index 7babe72907f..fc275845184 100644 --- a/src/Polly/Caching/CacheSyntax.cs +++ b/src/Polly/Caching/CacheSyntax.cs @@ -90,9 +90,7 @@ public static CachePolicy Cache(ISyncCacheProvider cacheProvider, ITtlStrategy t throw new ArgumentNullException(nameof(cacheKeyStrategy)); } - Action emptyDelegate = (_, _) => { }; - - return Cache(cacheProvider, ttlStrategy, cacheKeyStrategy.GetCacheKey, emptyDelegate, emptyDelegate, emptyDelegate, onCacheError, onCacheError); + return Cache(cacheProvider, ttlStrategy, cacheKeyStrategy.GetCacheKey, EmptyCallback, EmptyCallback, EmptyCallback, onCacheError, onCacheError); } /// @@ -142,9 +140,7 @@ public static CachePolicy Cache(ISyncCacheProvider cacheProvider, ITtlStrategy t throw new ArgumentNullException(nameof(cacheKeyStrategy)); } - Action emptyDelegate = (_, _) => { }; - - return Cache(cacheProvider, ttlStrategy, cacheKeyStrategy, emptyDelegate, emptyDelegate, emptyDelegate, onCacheError, onCacheError); + return Cache(cacheProvider, ttlStrategy, cacheKeyStrategy, EmptyCallback, EmptyCallback, EmptyCallback, onCacheError, onCacheError); } /// diff --git a/src/Polly/Policy.ContextAndKeys.cs b/src/Polly/Policy.ContextAndKeys.cs index 565e3af5313..2e8ce6a5110 100644 --- a/src/Polly/Policy.ContextAndKeys.cs +++ b/src/Polly/Policy.ContextAndKeys.cs @@ -25,16 +25,7 @@ public Policy WithPolicyKey(string policyKey) /// /// The unique, used-definable key to assign to this instance. /// An instance of . - ISyncPolicy ISyncPolicy.WithPolicyKey(string policyKey) - { - if (policyKeyInternal != null) - { - throw PolicyKeyMustBeImmutableException(nameof(policyKey)); - } - - policyKeyInternal = policyKey; - return this; - } + ISyncPolicy ISyncPolicy.WithPolicyKey(string policyKey) => WithPolicyKey(policyKey); } #pragma warning disable CA1724 @@ -64,14 +55,5 @@ public Policy WithPolicyKey(string policyKey) /// /// The unique, used-definable key to assign to this instance. /// An instance of . - ISyncPolicy ISyncPolicy.WithPolicyKey(string policyKey) - { - if (policyKeyInternal != null) - { - throw PolicyKeyMustBeImmutableException(nameof(policyKey)); - } - - policyKeyInternal = policyKey; - return this; - } + ISyncPolicy ISyncPolicy.WithPolicyKey(string policyKey) => WithPolicyKey(policyKey); } diff --git a/src/Polly/Registry/PolicyRegistry.cs b/src/Polly/Registry/PolicyRegistry.cs index 21306f75062..09efa7b05b1 100644 --- a/src/Polly/Registry/PolicyRegistry.cs +++ b/src/Polly/Registry/PolicyRegistry.cs @@ -7,7 +7,7 @@ /// Uses ConcurrentDictionary to store the collection. public class PolicyRegistry : IConcurrentPolicyRegistry { - private readonly IDictionary _registry = new ConcurrentDictionary(); + private readonly ConcurrentDictionary _registry = []; /// /// Initializes a new instance of the class, with keys. @@ -26,22 +26,9 @@ public PolicyRegistry() /// /// a dictionary containing keys and policies used for testing. internal PolicyRegistry(IDictionary registry) - { -#pragma warning disable S3236 // Remove this argument from the method call; it hides the caller information. - Debug.Assert(registry != null, "This constructor is for testing only, and should not be called with a null registry."); -#pragma warning restore S3236 // Remove this argument from the method call; it hides the caller information. - _registry = registry; - } - - private ConcurrentDictionary ThrowIfNotConcurrentImplementation() - { - if (_registry is ConcurrentDictionary concurrentRegistry) - { - return concurrentRegistry; - } - - throw new InvalidOperationException($"This {nameof(PolicyRegistry)} is not configured for concurrent operations. This exception should never be thrown in production code as the only public constructors create {nameof(PolicyRegistry)} instances of the correct form."); - } +#pragma warning disable IDE0306 // Simplify collection initialization + => _registry = new(registry); +#pragma warning restore IDE0306 // Simplify collection initialization /// /// Gets the total number of policies in the registry. @@ -57,7 +44,11 @@ private ConcurrentDictionary ThrowIfNotConcurrentImplementatio /// Thrown when is . /// A Policy with same already exists. public void Add(string key, TPolicy policy) - where TPolicy : IsPolicy => _registry.Add(key, policy); + where TPolicy : IsPolicy + { + IDictionary dictionary = _registry; + dictionary.Add(key, policy); + } /// /// Adds a policy with the provided key and policy to the registry. @@ -68,11 +59,7 @@ public void Add(string key, TPolicy policy) /// True if Policy was added. False otherwise. public bool TryAdd(string key, TPolicy policy) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return registry.TryAdd(key, policy); - } + => _registry.TryAdd(key, policy); /// /// Gets of sets the with the specified key. @@ -145,8 +132,11 @@ public bool ContainsKey(string key) => /// The of the policy to remove. /// True if the policy is successfully removed. Otherwise false. /// is . - public bool Remove(string key) => - _registry.Remove(key); + public bool Remove(string key) + { + IDictionary dictionary = _registry; + return dictionary.Remove(key); + } /// /// Removes the policy stored under the specified from the registry. @@ -162,10 +152,8 @@ public bool Remove(string key) => public bool TryRemove(string key, out TPolicy policy) where TPolicy : IsPolicy { - var registry = ThrowIfNotConcurrentImplementation(); - policy = default; - bool got = registry.TryRemove(key, out IsPolicy value); + bool got = _registry.TryRemove(key, out IsPolicy value); if (got) { @@ -188,11 +176,7 @@ public bool TryRemove(string key, out TPolicy policy) /// public bool TryUpdate(string key, TPolicy newPolicy, TPolicy comparisonPolicy) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return registry.TryUpdate(key, newPolicy, comparisonPolicy); - } + => _registry.TryUpdate(key, newPolicy, comparisonPolicy); /// /// Adds a policy with the provided key and policy to the registry @@ -206,11 +190,7 @@ public bool TryUpdate(string key, TPolicy newPolicy, TPolicy comparison /// if the key was not in the registry. public TPolicy GetOrAdd(string key, Func policyFactory) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return (TPolicy)registry.GetOrAdd(key, k => policyFactory(k)); - } + => (TPolicy)_registry.GetOrAdd(key, k => policyFactory(k)); /// /// Adds a key/policy pair to the registry @@ -223,11 +203,7 @@ public TPolicy GetOrAdd(string key, Func policyFactory /// key is already in the registry, or the new policy if the key was not in the registry. public TPolicy GetOrAdd(string key, TPolicy policy) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return (TPolicy)registry.GetOrAdd(key, policy); - } + => (TPolicy)_registry.GetOrAdd(key, policy); /// /// Adds a key/policy pair to the registry if the key does not already @@ -243,11 +219,7 @@ public TPolicy GetOrAdd(string key, TPolicy policy) /// absent) or the result of updatePolicyFactory (if the key was present). public TPolicy AddOrUpdate(string key, Func addPolicyFactory, Func updatePolicyFactory) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return (TPolicy)registry.AddOrUpdate(key, k => addPolicyFactory(k), (k, e) => updatePolicyFactory(k, (TPolicy)e)); - } + => (TPolicy)_registry.AddOrUpdate(key, k => addPolicyFactory(k), (k, e) => updatePolicyFactory(k, (TPolicy)e)); /// /// Adds a key/policy pair to the registry if the key does not already @@ -263,11 +235,7 @@ public TPolicy AddOrUpdate(string key, Func addPolicyF /// absent) or the result of updatePolicyFactory (if the key was present). public TPolicy AddOrUpdate(string key, TPolicy addPolicy, Func updatePolicyFactory) where TPolicy : IsPolicy - { - var registry = ThrowIfNotConcurrentImplementation(); - - return (TPolicy)registry.AddOrUpdate(key, addPolicy, (k, e) => updatePolicyFactory(k, (TPolicy)e)); - } + => (TPolicy)_registry.AddOrUpdate(key, addPolicy, (k, e) => updatePolicyFactory(k, (TPolicy)e)); /// Returns an enumerator that iterates through the policy objects in the . diff --git a/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs b/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs index 6c651d3eec8..9b8ee3eb652 100644 --- a/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs +++ b/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs @@ -22,24 +22,34 @@ public class PollyServiceCollectionExtensionTests public void AddResiliencePipeline_ArgValidation() { _services = null!; - Assert.Throws(() => AddResiliencePipeline(Key)); - Assert.Throws(() => AddResiliencePipeline(Key)); + Assert.Throws("services", () => AddResiliencePipeline(Key)); + Assert.Throws("services", () => AddResiliencePipeline(Key)); + Assert.Throws("services", () => _services.AddResiliencePipeline(Key, (_) => { })); + Assert.Throws("services", () => _services.AddResiliencePipeline(Key, (_, _) => { })); + Assert.Throws("services", () => _services.AddResiliencePipeline(Key, (_) => { })); + Assert.Throws("services", () => _services.AddResiliencePipeline(Key, (_, _) => { })); _services = []; - Assert.Throws(() => _services.AddResiliencePipeline( + Assert.Throws("configure", () => _services.AddResiliencePipeline( Key, (Action>)null!)); - Assert.Throws(() => _services.AddResiliencePipeline( + Assert.Throws("configure", () => _services.AddResiliencePipeline( Key, (Action, AddResiliencePipelineContext>)null!)); - Assert.Throws(() => _services.AddResiliencePipeline( + Assert.Throws("configure", () => _services.AddResiliencePipeline( Key, (Action)null!)); - Assert.Throws(() => _services.AddResiliencePipeline( + Assert.Throws("configure", () => _services.AddResiliencePipeline( Key, (Action>)null!)); + + _services.AddResiliencePipelines(ctx => + { + Assert.Throws("configure", () => ctx.AddResiliencePipeline(Key, null!)); + Assert.Throws("configure", () => ctx.AddResiliencePipeline(Key, null!)); + }); } [InlineData(true)] @@ -62,6 +72,7 @@ public void AddResiliencePipeline_EnsureRegisteredServices(bool generic) serviceProvider.GetServices>().ShouldNotBeNull(); serviceProvider.GetServices>().ShouldNotBeNull(); serviceProvider.GetServices().ShouldNotBeSameAs(serviceProvider.GetServices()); + serviceProvider.GetRequiredService>>().ShouldNotBeNull(); } [Fact] diff --git a/test/Polly.Extensions.Tests/DisposablePipelineTests.cs b/test/Polly.Extensions.Tests/DisposablePipelineTests.cs index fbccde14edf..cc549f09560 100644 --- a/test/Polly.Extensions.Tests/DisposablePipelineTests.cs +++ b/test/Polly.Extensions.Tests/DisposablePipelineTests.cs @@ -68,6 +68,23 @@ public async Task DisposePipeline_EnsureExternalPipelineNotDisposed() Should.Throw(() => pipeline2.Execute(() => string.Empty)); } + [Fact] + public void DisposePipeline_OnPipelineDisposed_Throws_NullArg() + { + var asserted = false; + + var provider = new ServiceCollection() + .AddResiliencePipeline("my-pipeline", (_, context) => + { + Assert.Throws("callback", () => context.OnPipelineDisposed(null!)); + asserted = true; + }) + .BuildServiceProvider(); + + provider.GetRequiredService>().GetPipeline("my-pipeline"); + asserted.ShouldBeTrue(); + } + private static bool IsDisposed(RateLimiter limiter) { try diff --git a/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs b/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTests.cs similarity index 53% rename from test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs rename to test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTests.cs index 312072b0963..711d4bab6fe 100644 --- a/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs +++ b/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTests.cs @@ -7,6 +7,22 @@ namespace Polly.Extensions.Tests.Registry; public class ConfigureBuilderContextExtensionsTests { + [Fact] + public async Task ConfigureReloads_NullArguments_Throws() + { + await using var registry = new ResiliencePipelineRegistry(); + registry.GetOrAddPipeline("pipeline", (_, builderContext) => + { + ConfigureBuilderContext nullContext = null!; + IOptionsMonitor nullMonitor = null!; + + var monitor = Substitute.For>(); + + Assert.Throws("context", () => nullContext.EnableReloads(monitor)); + Assert.Throws("optionsMonitor", () => builderContext.EnableReloads(nullMonitor)); + }); + } + [Fact] public async Task ConfigureReloads_MonitorRegistrationReturnsNull_DoesNotThrow() { @@ -27,5 +43,7 @@ public async Task ConfigureReloads_MonitorRegistrationReturnsNull_DoesNotThrow() listener.Events.ShouldBeEmpty(); } +#pragma warning disable S2094 // Classes should not be empty public class Options; +#pragma warning restore S2094 // Classes should not be empty } diff --git a/test/Polly.Extensions.Tests/Telemetry/TelemetryOptionsTests.cs b/test/Polly.Extensions.Tests/Telemetry/TelemetryOptionsTests.cs index 7993f20281f..2f13d5cc326 100644 --- a/test/Polly.Extensions.Tests/Telemetry/TelemetryOptionsTests.cs +++ b/test/Polly.Extensions.Tests/Telemetry/TelemetryOptionsTests.cs @@ -24,6 +24,13 @@ public void Ctor_EnsureDefaults() options.ResultFormatter(resilienceContext, response).ShouldBe(200); } + [Fact] + public void CopyCtor_OtherNull_Throws() + { + var options = new TelemetryOptions(); + Assert.Throws("other", () => new TelemetryOptions(null!)); + } + [Fact] public void CopyCtor_Ok() { diff --git a/test/Polly.Extensions.Tests/Telemetry/TelemetryResiliencePipelineBuilderExtensionsTests.cs b/test/Polly.Extensions.Tests/Telemetry/TelemetryResiliencePipelineBuilderExtensionsTests.cs index 12e0699e9be..e704ade017e 100644 --- a/test/Polly.Extensions.Tests/Telemetry/TelemetryResiliencePipelineBuilderExtensionsTests.cs +++ b/test/Polly.Extensions.Tests/Telemetry/TelemetryResiliencePipelineBuilderExtensionsTests.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Polly.Telemetry; @@ -28,6 +29,24 @@ public void ConfigureTelemetry_EnsureLogging() fakeLogger.GetRecords().Count().ShouldBe(2); } + [Fact] + public void ConfigureTelemetry_NullArguments_Throws() + { + ResiliencePipelineBuilder builder = null!; + + using var loggerFactory = TestUtilities.CreateLoggerFactory(out var fakeLogger); + var options = new TelemetryOptions(); + + Assert.Throws("builder", () => builder.ConfigureTelemetry(loggerFactory)); + Assert.Throws("builder", () => builder.ConfigureTelemetry(options)); + + ILoggerFactory nulLoggerFactory = null!; + TelemetryOptions nulOptions = null!; + + Assert.Throws("loggerFactory", () => _builder.ConfigureTelemetry(nulLoggerFactory)); + Assert.Throws("options", () => _builder.ConfigureTelemetry(nulOptions)); + } + [Fact] public void ConfigureTelemetry_InvalidOptions_Throws() { diff --git a/test/Polly.Specs/Bulkhead/BulkheadTResultAsyncSpecs.cs b/test/Polly.Specs/Bulkhead/BulkheadTResultAsyncSpecs.cs index e92906d5ffb..f29bef747c3 100644 --- a/test/Polly.Specs/Bulkhead/BulkheadTResultAsyncSpecs.cs +++ b/test/Polly.Specs/Bulkhead/BulkheadTResultAsyncSpecs.cs @@ -72,6 +72,14 @@ public void Should_throw_when_onBulkheadRejected_is_null() .ParamName.ShouldBe("onBulkheadRejectedAsync"); } + [Fact] + public void Should_not_throw_when_arguments_valid() + { + Action policy = () => Policy.BulkheadAsync(1); + + Should.NotThrow(policy); + } + #endregion #region onBulkheadRejected delegate diff --git a/test/Polly.Specs/Caching/CacheSpecs.cs b/test/Polly.Specs/Caching/CacheSpecs.cs index 5143df28cd5..ad1f7d9acbf 100644 --- a/test/Polly.Specs/Caching/CacheSpecs.cs +++ b/test/Polly.Specs/Caching/CacheSpecs.cs @@ -59,6 +59,54 @@ public void Should_throw_when_action_is_null() .ParamName.ShouldBe("action"); } + [Fact] + public void Should_not_throw_when_arguments_valid() + { + ISyncCacheProvider cacheProvider = new StubCacheProvider(); + var ttl = TimeSpan.MaxValue; + ITtlStrategy ttlStrategy = new ContextualTtl(); + ICacheKeyStrategy cacheKeyStrategy = new StubCacheKeyStrategy(context => context.OperationKey + context["id"]); + Func cacheKeyStrategyFunc = (_) => string.Empty; + Action onCache = (_, _) => { }; + Action? onCacheError = (_, _, _) => { }; + + Action action = () => Policy.Cache(cacheProvider, ttl, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttl, cacheKeyStrategy, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, cacheKeyStrategy, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttl, cacheKeyStrategyFunc, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, cacheKeyStrategyFunc, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttl, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttl, cacheKeyStrategy, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, cacheKeyStrategy, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttl, cacheKeyStrategyFunc, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + + action = () => Policy.Cache(cacheProvider, ttlStrategy, cacheKeyStrategyFunc, onCache, onCache, onCache, onCacheError, onCacheError); + Should.NotThrow(action); + } + [Fact] public void Should_throw_when_cache_provider_is_null() { diff --git a/test/Polly.Specs/PolicyAsyncSpecs.cs b/test/Polly.Specs/PolicyAsyncSpecs.cs index d058f293839..29e32f9e1f7 100644 --- a/test/Polly.Specs/PolicyAsyncSpecs.cs +++ b/test/Polly.Specs/PolicyAsyncSpecs.cs @@ -150,7 +150,7 @@ public async Task Executing_the_policy_action_should_throw_when_context_data_is_ .Handle() .RetryAsync((_, _, _) => { }); - await Should.ThrowAsync(() => policy.ExecuteAsync(_ => TaskHelper.EmptyTask, (IDictionary)null!)); + await Assert.ThrowsAsync("contextData", () => policy.ExecuteAsync(_ => TaskHelper.EmptyTask, (IDictionary)null!)); } [Fact]