From 8a671163a7a005e77b73de13700d699207a2dd60 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Sun, 21 May 2023 17:32:37 +0200 Subject: [PATCH 1/3] Simplify AddStrategy method --- .../Internals/Helper.StrategyPipeline.cs | 8 +-- .../ResilienceStrategyRegistryTests.cs | 10 ++-- .../ResilienceStrategyBuilderTests.cs | 54 ++++++++----------- src/Polly.Core/ResilienceStrategyBuilder.cs | 13 +++-- .../PollyServiceCollectionExtensionTests.cs | 4 +- ...s.OnCircuitBreakWithServiceProvider_796.cs | 2 +- ...esilienceStrategyBuilderExtensionsTests.cs | 4 +- 7 files changed, 41 insertions(+), 54 deletions(-) diff --git a/src/Polly.Core.Benchmarks/Internals/Helper.StrategyPipeline.cs b/src/Polly.Core.Benchmarks/Internals/Helper.StrategyPipeline.cs index f8b35057c1a..efb11546d0f 100644 --- a/src/Polly.Core.Benchmarks/Internals/Helper.StrategyPipeline.cs +++ b/src/Polly.Core.Benchmarks/Internals/Helper.StrategyPipeline.cs @@ -1,5 +1,4 @@ using Polly; -using Polly.Strategy; namespace Polly.Core.Benchmarks; @@ -13,14 +12,9 @@ internal static partial class Helper { for (var i = 0; i < count; i++) { - builder.AddStrategy(new EmptyResilienceStrategy(), new BenchmarkResilienceStrategyOptions()); + builder.AddStrategy(new EmptyResilienceStrategy()); } }), _ => throw new NotSupportedException() }; - - private class BenchmarkResilienceStrategyOptions : ResilienceStrategyOptions - { - public override string StrategyType => "Benchmark"; - } } diff --git a/src/Polly.Core.Tests/Registry/ResilienceStrategyRegistryTests.cs b/src/Polly.Core.Tests/Registry/ResilienceStrategyRegistryTests.cs index 079cc8e6260..1ade2d37721 100644 --- a/src/Polly.Core.Tests/Registry/ResilienceStrategyRegistryTests.cs +++ b/src/Polly.Core.Tests/Registry/ResilienceStrategyRegistryTests.cs @@ -42,7 +42,7 @@ public void Clear_Ok() { var registry = new ResilienceStrategyRegistry(); - registry.TryAddBuilder("C", (_, b) => b.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions())); + registry.TryAddBuilder("C", (_, b) => b.AddStrategy(new TestResilienceStrategy())); registry.TryAdd("A", new TestResilienceStrategy()); registry.TryAdd("B", new TestResilienceStrategy()); @@ -74,7 +74,7 @@ public void Remove_Ok() public void RemoveBuilder_Ok() { var registry = new ResilienceStrategyRegistry(); - registry.TryAddBuilder("A", (_, b) => b.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions())); + registry.TryAddBuilder("A", (_, b) => b.AddStrategy(new TestResilienceStrategy())); registry.RemoveBuilder("A").Should().BeTrue(); registry.RemoveBuilder("A").Should().BeFalse(); @@ -88,7 +88,7 @@ public void GetStrategy_BuilderMultiInstance_EnsureMultipleInstances() var builderName = "A"; var registry = CreateRegistry(); var strategies = new HashSet(); - registry.TryAddBuilder(StrategyId.Create(builderName), (_, builder) => builder.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions())); + registry.TryAddBuilder(StrategyId.Create(builderName), (_, builder) => builder.AddStrategy(new TestResilienceStrategy())); for (int i = 0; i < 100; i++) { @@ -112,7 +112,7 @@ public void AddBuilder_GetStrategy_EnsureCalled() var called = 0; registry.TryAddBuilder(StrategyId.Create("A"), (key, builder) => { - builder.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions()); + builder.AddStrategy(new TestResilienceStrategy()); builder.Properties.Set(StrategyId.ResilienceKey, key); called++; }); @@ -142,7 +142,7 @@ public void AddBuilder_EnsureStrategyKey() var registry = CreateRegistry(); registry.TryAddBuilder(StrategyId.Create("A"), (_, builder) => { - builder.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions()); + builder.AddStrategy(new TestResilienceStrategy()); builder.BuilderName.Should().Be("A"); builder.Properties.TryGetValue(TelemetryUtil.StrategyKey, out var val).Should().BeTrue(); val.Should().Be("Instance1"); diff --git a/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs b/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs index 632721baf91..88cf5413642 100644 --- a/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs +++ b/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs @@ -29,7 +29,7 @@ public void AddStrategy_Single_Ok() After = (_, _) => executions.Add(3), }; - builder.AddStrategy(first, new TestResilienceStrategyOptions()); + builder.AddStrategy(first); // act var strategy = builder.Build(); @@ -63,9 +63,9 @@ public void AddStrategy_Multiple_Ok() After = (_, _) => executions.Add(5), }; - builder.AddStrategy(first, new TestResilienceStrategyOptions()); - builder.AddStrategy(second, new TestResilienceStrategyOptions()); - builder.AddStrategy(third, new TestResilienceStrategyOptions()); + builder.AddStrategy(first); + builder.AddStrategy(second); + builder.AddStrategy(third); // act var strategy = builder.Build(); @@ -88,8 +88,8 @@ public void AddStrategy_Duplicate_Throws() // arrange var executions = new List(); var builder = new ResilienceStrategyBuilder() - .AddStrategy(NullResilienceStrategy.Instance, new TestResilienceStrategyOptions()) - .AddStrategy(NullResilienceStrategy.Instance, new TestResilienceStrategyOptions()); + .AddStrategy(NullResilienceStrategy.Instance) + .AddStrategy(NullResilienceStrategy.Instance); builder.Invoking(b => b.Build()) .Should() @@ -119,9 +119,9 @@ public void AddStrategy_MultipleNonDelegating_Ok() After = () => executions.Add(5), }; - builder.AddStrategy(first, new TestResilienceStrategyOptions()); - builder.AddStrategy(second, new TestResilienceStrategyOptions()); - builder.AddStrategy(third, new TestResilienceStrategyOptions()); + builder.AddStrategy(first); + builder.AddStrategy(second); + builder.AddStrategy(third); // act var strategy = builder.Build(); @@ -147,7 +147,7 @@ public void AddStrategy_AfterUsed_Throws() builder.Build(); builder - .Invoking(b => b.AddStrategy(NullResilienceStrategy.Instance, new TestResilienceStrategyOptions())) + .Invoking(b => b.AddStrategy(NullResilienceStrategy.Instance)) .Should() .Throw() .WithMessage("Cannot add any more resilience strategies to the builder after it has been used to build a strategy once."); @@ -173,31 +173,13 @@ The BuilderName field is required. """); } - [Fact] - public void AddStrategy_InvalidOptions_Throws() - { - var builder = new ResilienceStrategyBuilder(); - - builder - .Invoking(b => b.AddStrategy(NullResilienceStrategy.Instance, new TestResilienceStrategyOptions { StrategyName = null! })) - .Should() - .Throw() - .WithMessage( -""" -The 'ResilienceStrategyOptions' options are not valid. - -Validation Errors: -The StrategyName field is required. -"""); - } - [Fact] public void AddStrategy_NullFactory_Throws() { var builder = new ResilienceStrategyBuilder(); builder - .Invoking(b => b.AddStrategy((Func)null!, new TestResilienceStrategyOptions())) + .Invoking(b => b.AddStrategy(null!, new TestResilienceStrategyOptions())) .Should() .Throw() .And.ParamName @@ -221,17 +203,17 @@ public void AddStrategy_CombinePipelines_Ok() After = (_, _) => executions.Add(6), }; - var pipeline1 = new ResilienceStrategyBuilder().AddStrategy(first, new TestResilienceStrategyOptions()).AddStrategy(second, new TestResilienceStrategyOptions()).Build(); + var pipeline1 = new ResilienceStrategyBuilder().AddStrategy(first).AddStrategy(second).Build(); var third = new TestResilienceStrategy { Before = (_, _) => executions.Add(3), After = (_, _) => executions.Add(5), }; - var pipeline2 = new ResilienceStrategyBuilder().AddStrategy(third, new TestResilienceStrategyOptions()).Build(); + var pipeline2 = new ResilienceStrategyBuilder().AddStrategy(third).Build(); // act - var strategy = new ResilienceStrategyBuilder().AddStrategy(pipeline1, new TestResilienceStrategyOptions()).AddStrategy(pipeline2, new TestResilienceStrategyOptions()).Build(); + var strategy = new ResilienceStrategyBuilder().AddStrategy(pipeline1).AddStrategy(pipeline2).Build(); // assert strategy.Execute(_ => executions.Add(4)); @@ -305,7 +287,7 @@ public void Build_OnCreatingStrategy_EnsureRespected() } }; - builder.AddStrategy(strategy, new TestResilienceStrategyOptions()); + builder.AddStrategy(strategy); // act var finalStrategy = builder.Build(); @@ -314,6 +296,12 @@ public void Build_OnCreatingStrategy_EnsureRespected() finalStrategy.Should().BeOfType(); } + [Fact] + public void EmptyOptions_Ok() + { + ResilienceStrategyBuilder.EmptyOptions.Instance.StrategyType.Should().Be("Empty"); + } + private class Strategy : ResilienceStrategy { public Action? Before { get; set; } diff --git a/src/Polly.Core/ResilienceStrategyBuilder.cs b/src/Polly.Core/ResilienceStrategyBuilder.cs index ef3171f09cb..97c8d6daf90 100644 --- a/src/Polly.Core/ResilienceStrategyBuilder.cs +++ b/src/Polly.Core/ResilienceStrategyBuilder.cs @@ -46,15 +46,13 @@ public class ResilienceStrategyBuilder /// Adds an already created strategy instance to the builder. /// /// The strategy instance. - /// The options associated with the strategy. If none are provided the default instance of is created. /// The same builder instance. /// Thrown when is null. - public ResilienceStrategyBuilder AddStrategy(ResilienceStrategy strategy, ResilienceStrategyOptions options) + public ResilienceStrategyBuilder AddStrategy(ResilienceStrategy strategy) { Guard.NotNull(strategy); - Guard.NotNull(options); - return AddStrategy(_ => strategy, options); + return AddStrategy(_ => strategy, EmptyOptions.Instance); } /// @@ -121,4 +119,11 @@ private ResilienceStrategy CreateResilienceStrategy(Entry entry) } private sealed record Entry(Func Factory, ResilienceStrategyOptions Properties); + + internal sealed class EmptyOptions : ResilienceStrategyOptions + { + public static readonly EmptyOptions Instance = new(); + + public override string StrategyType => "Empty"; + } } diff --git a/src/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs b/src/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs index 5772626765c..e26619686d1 100644 --- a/src/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs +++ b/src/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs @@ -49,7 +49,7 @@ public void AddResilienceStrategy_EnsureRegisteredServices() public void AddResilienceStrategy_MultipleRegistries_Ok() { AddResilienceStrategy(Key); - _services.AddResilienceStrategy(10, context => context.AddStrategy(new TestStrategy(), new TestResilienceStrategyOptions())); + _services.AddResilienceStrategy(10, context => context.AddStrategy(new TestStrategy())); var serviceProvider = _services.BuildServiceProvider(); @@ -67,7 +67,7 @@ public void AddResilienceStrategy_EnsureContextFilled() context.Key.Should().Be(Key); builder.Should().NotBeNull(); context.ServiceProvider.Should().NotBeNull(); - builder.AddStrategy(new TestStrategy(), new TestResilienceStrategyOptions()); + builder.AddStrategy(new TestStrategy()); asserted = true; }); diff --git a/src/Polly.Extensions.Tests/Issues/IssuesTests.OnCircuitBreakWithServiceProvider_796.cs b/src/Polly.Extensions.Tests/Issues/IssuesTests.OnCircuitBreakWithServiceProvider_796.cs index 76f76bf977e..f3836983415 100644 --- a/src/Polly.Extensions.Tests/Issues/IssuesTests.OnCircuitBreakWithServiceProvider_796.cs +++ b/src/Polly.Extensions.Tests/Issues/IssuesTests.OnCircuitBreakWithServiceProvider_796.cs @@ -33,7 +33,7 @@ public async Task OnCircuitBreakWithServiceProvider_796() var serviceCollection = new ServiceCollection().AddResilienceStrategy("my-strategy", (builder, context) => { builder - .AddStrategy(new ServiceProviderStrategy(context.ServiceProvider), new TestResilienceStrategyOptions()) + .AddStrategy(new ServiceProviderStrategy(context.ServiceProvider)) .AddAdvancedCircuitBreaker(options); }); diff --git a/src/Polly.Extensions.Tests/Telemetry/TelemetryResilienceStrategyBuilderExtensionsTests.cs b/src/Polly.Extensions.Tests/Telemetry/TelemetryResilienceStrategyBuilderExtensionsTests.cs index 524d52761fd..d0dc8a2c84d 100644 --- a/src/Polly.Extensions.Tests/Telemetry/TelemetryResilienceStrategyBuilderExtensionsTests.cs +++ b/src/Polly.Extensions.Tests/Telemetry/TelemetryResilienceStrategyBuilderExtensionsTests.cs @@ -13,7 +13,7 @@ public void EnableTelemetry_EnsureDiagnosticSourceUpdated() { _builder.EnableTelemetry(NullLoggerFactory.Instance); _builder.Properties.GetValue(new ResiliencePropertyKey("DiagnosticSource"), null).Should().BeOfType(); - _builder.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions()).Build().Should().NotBeOfType(); + _builder.AddStrategy(new TestResilienceStrategy()).Build().Should().NotBeOfType(); } [Fact] @@ -21,7 +21,7 @@ public void EnableTelemetry_EnsureLogging() { using var factory = TestUtilities.CreateLoggerFactory(out var fakeLogger); _builder.EnableTelemetry(factory); - _builder.AddStrategy(new TestResilienceStrategy(), new TestResilienceStrategyOptions()).Build().Execute(_ => { }); + _builder.AddStrategy(new TestResilienceStrategy()).Build().Execute(_ => { }); fakeLogger.GetRecords().Should().NotBeEmpty(); fakeLogger.GetRecords().Should().HaveCount(2); From be35f416903bb4586002e29f1ed0beae6097ec43 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Sun, 21 May 2023 17:38:04 +0200 Subject: [PATCH 2/3] docs improvements --- src/Polly.Core/ResilienceStrategyBuilder.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Polly.Core/ResilienceStrategyBuilder.cs b/src/Polly.Core/ResilienceStrategyBuilder.cs index 97c8d6daf90..c8060f1bceb 100644 --- a/src/Polly.Core/ResilienceStrategyBuilder.cs +++ b/src/Polly.Core/ResilienceStrategyBuilder.cs @@ -48,6 +48,7 @@ public class ResilienceStrategyBuilder /// The strategy instance. /// The same builder instance. /// Thrown when is null. + /// Thrown when this builder was already used to create a strategy. The builder cannot be modified after it has been used. public ResilienceStrategyBuilder AddStrategy(ResilienceStrategy strategy) { Guard.NotNull(strategy); @@ -62,6 +63,8 @@ public ResilienceStrategyBuilder AddStrategy(ResilienceStrategy strategy) /// The options associated with the strategy. If none are provided the default instance of is created. /// The same builder instance. /// Thrown when is null. + /// Thrown when this builder was already used to create a strategy. The builder cannot be modified after it has been used. + /// Thrown when the are invalid. public ResilienceStrategyBuilder AddStrategy(Func factory, ResilienceStrategyOptions options) { Guard.NotNull(factory); From 46c30c53192dbb69877511ca25cb815bb248ba8f Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Sun, 21 May 2023 17:40:59 +0200 Subject: [PATCH 3/3] add back missed test --- .../ResilienceStrategyBuilderTests.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs b/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs index 88cf5413642..8d980cc5045 100644 --- a/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs +++ b/src/Polly.Core.Tests/ResilienceStrategyBuilderTests.cs @@ -173,6 +173,24 @@ The BuilderName field is required. """); } + [Fact] + public void AddStrategy_InvalidOptions_Throws() + { + var builder = new ResilienceStrategyBuilder(); + + builder + .Invoking(b => b.AddStrategy(_ => NullResilienceStrategy.Instance, new TestResilienceStrategyOptions { StrategyName = null! })) + .Should() + .Throw() + .WithMessage( +""" +The 'ResilienceStrategyOptions' options are not valid. + +Validation Errors: +The StrategyName field is required. +"""); + } + [Fact] public void AddStrategy_NullFactory_Throws() {