From 669309515a3ffa749e5b0e071ccb2c7cf673aa88 Mon Sep 17 00:00:00 2001 From: peter-csala Date: Thu, 9 May 2024 16:25:02 +0200 Subject: [PATCH 1/8] Replace public properties to private fields --- .../Simmy/Outcomes/ChaosOutcomeStrategy.cs | 16 +++++++--------- ...aosOutcomePipelineBuilderExtensionsTests.cs | 18 +++++++++++------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs index 1a6c075ff03..a026a6b5118 100644 --- a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs +++ b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs @@ -5,32 +5,30 @@ namespace Polly.Simmy.Outcomes; internal class ChaosOutcomeStrategy : ChaosStrategy { private readonly ResilienceStrategyTelemetry _telemetry; + private readonly Func, ValueTask>? _onOutcomeInjected; + private readonly Func?>> _outcomeGenerator; public ChaosOutcomeStrategy(ChaosOutcomeStrategyOptions options, ResilienceStrategyTelemetry telemetry) : base(options) { _telemetry = telemetry; - OnOutcomeInjected = options.OnOutcomeInjected; - OutcomeGenerator = options.OutcomeGenerator; + _onOutcomeInjected = options.OnOutcomeInjected; + _outcomeGenerator = options.OutcomeGenerator; } - public Func, ValueTask>? OnOutcomeInjected { get; } - - public Func?>> OutcomeGenerator { get; } - protected internal override async ValueTask> ExecuteCore(Func>> callback, ResilienceContext context, TState state) { try { if (await ShouldInjectAsync(context).ConfigureAwait(context.ContinueOnCapturedContext)) { - var outcome = await OutcomeGenerator(new(context)).ConfigureAwait(context.ContinueOnCapturedContext); + var outcome = await _outcomeGenerator(new(context)).ConfigureAwait(context.ContinueOnCapturedContext); var args = new OnOutcomeInjectedArguments(context, outcome.Value); _telemetry.Report(new(ResilienceEventSeverity.Information, ChaosOutcomeConstants.OnOutcomeInjectedEvent), context, args); - if (OnOutcomeInjected is not null) + if (_onOutcomeInjected is not null) { - await OnOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); + await _onOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); } return new Outcome(outcome.Value.Result); diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomePipelineBuilderExtensionsTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomePipelineBuilderExtensionsTests.cs index 0bcc4f11207..192948c5506 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomePipelineBuilderExtensionsTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomePipelineBuilderExtensionsTests.cs @@ -12,26 +12,27 @@ public class ChaosOutcomePipelineBuilderExtensionsTests { builder => { - builder.AddChaosOutcome(new ChaosOutcomeStrategyOptions + var options = new ChaosOutcomeStrategyOptions { InjectionRate = 0.6, Randomizer = () => 0.5, OutcomeGenerator = (_) => new ValueTask?>(Outcome.FromResult(100)) - }); + }; + builder.AddChaosOutcome(options); - AssertResultStrategy(builder, true, 0.6, new(100)); + AssertResultStrategy(builder, options, true, 0.6, new(100)); }, }; #pragma warning restore IDE0028 - private static void AssertResultStrategy(ResiliencePipelineBuilder builder, bool enabled, double injectionRate, Outcome outcome) + private static void AssertResultStrategy(ResiliencePipelineBuilder builder, ChaosOutcomeStrategyOptions options, bool enabled, double injectionRate, Outcome outcome) { var context = ResilienceContextPool.Shared.Get(); var strategy = builder.Build().GetPipelineDescriptor().FirstStrategy.StrategyInstance.Should().BeOfType>().Subject; strategy.EnabledGenerator.Invoke(new(context)).Preserve().GetAwaiter().GetResult().Should().Be(enabled); strategy.InjectionRateGenerator.Invoke(new(context)).Preserve().GetAwaiter().GetResult().Should().Be(injectionRate); - strategy.OutcomeGenerator!.Invoke(new(context)).Preserve().GetAwaiter().GetResult().Should().Be(outcome); + options.OutcomeGenerator!.Invoke(new(context)).Preserve().GetAwaiter().GetResult().Should().Be(outcome); } [MemberData(nameof(ResultStrategy))] @@ -46,11 +47,14 @@ internal void AddResult_Options_Ok(Action> config public void AddResult_Shortcut_Generator_Option_Ok() { var builder = new ResiliencePipelineBuilder(); - builder + var pipeline = builder .AddChaosOutcome(0.5, () => 120) .Build(); - AssertResultStrategy(builder, true, 0.5, new(120)); + var descriptor = pipeline.GetPipelineDescriptor(); + var options = Assert.IsType>(descriptor.Strategies[0].Options); + + AssertResultStrategy(builder, options, true, 0.5, new(120)); } [Fact] From e34a9c23b4915bfdc8eff508de3cb258407b6d5a Mon Sep 17 00:00:00 2001 From: peter-csala Date: Thu, 9 May 2024 16:43:13 +0200 Subject: [PATCH 2/8] Throw exception if needed --- .../Simmy/Outcomes/ChaosOutcomeStrategy.cs | 5 +++ .../Outcomes/ChaosOutcomeStrategyTests.cs | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs index a026a6b5118..cc29ef98fb7 100644 --- a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs +++ b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs @@ -31,6 +31,11 @@ protected internal override async ValueTask> ExecuteCore(Func await _onOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); } + if (outcome.Value.Exception is not null) + { + return new Outcome(outcome.Value.Exception); + } + return new Outcome(outcome.Value.Result); } diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs index b5359a41618..fd524cfaf0c 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs @@ -155,6 +155,39 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu _onOutcomeInjectedExecuted.Should().BeFalse(); } + [Fact] + public async Task Given_enabled_and_randomly_within_threshold_should_inject_exception() + { + var exception = new InvalidOperationException(); + var options = new ChaosOutcomeStrategyOptions + { + InjectionRate = 0.6, + Randomizer = () => 0.5, + OutcomeGenerator = (_) => new ValueTask?>( + Outcome.FromException(exception)), + OnOutcomeInjected = args => + { + args.Outcome.Value.Result.Should().Be(default(int)); + args.Outcome.Value.Exception.Should().Be(exception); + _onOutcomeInjectedExecuted = true; + return default; + } + }; + + var sut = CreateSut(options); + await sut.Invoking(s => s.ExecuteAsync(async _ => + { + _userDelegateExecuted = true; + return await Task.FromResult(42); + }, CancellationToken.None) + .AsTask()) + .Should() + .ThrowAsync(); + + _userDelegateExecuted.Should().BeFalse(); + _onOutcomeInjectedExecuted.Should().BeTrue(); + } + [Fact] public async Task Should_not_execute_user_delegate_when_it_was_cancelled_running_the_strategy() { From 221da2b74edf74d2cab50aec167c4cb706238a75 Mon Sep 17 00:00:00 2001 From: peter-csala Date: Thu, 9 May 2024 18:27:09 +0200 Subject: [PATCH 3/8] Fix build --- .../Simmy/Outcomes/ChaosOutcomeStrategyTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs index fd524cfaf0c..8bc92eaaebf 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs @@ -167,8 +167,8 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_exce Outcome.FromException(exception)), OnOutcomeInjected = args => { - args.Outcome.Value.Result.Should().Be(default(int)); - args.Outcome.Value.Exception.Should().Be(exception); + args.Outcome.Result.Should().Be(default(int)); + args.Outcome.Exception.Should().Be(exception); _onOutcomeInjectedExecuted = true; return default; } From 7d11148852296e45f36581fc9407ffc6f8121665 Mon Sep 17 00:00:00 2001 From: peter-csala Date: Thu, 9 May 2024 18:46:05 +0200 Subject: [PATCH 4/8] Fix build --- .../Simmy/Outcomes/ChaosOutcomeStrategyTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs index 8bc92eaaebf..907b70bb232 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs @@ -167,7 +167,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_exce Outcome.FromException(exception)), OnOutcomeInjected = args => { - args.Outcome.Result.Should().Be(default(int)); + args.Outcome.Result.Should().Be(default); args.Outcome.Exception.Should().Be(exception); _onOutcomeInjectedExecuted = true; return default; From 8832da808e13cdd6d1bc40f0632dede82bce7d1e Mon Sep 17 00:00:00 2001 From: peter-csala Date: Fri, 10 May 2024 08:31:56 +0200 Subject: [PATCH 5/8] Handle each outcome case explicitly --- src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs index cc29ef98fb7..0918a4b9fa1 100644 --- a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs +++ b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs @@ -31,12 +31,17 @@ protected internal override async ValueTask> ExecuteCore(Func await _onOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); } - if (outcome.Value.Exception is not null) + if (outcome.HasValue is false) { - return new Outcome(outcome.Value.Exception); + return new Outcome(default(T?)); } - return new Outcome(outcome.Value.Result); + if (outcome.Value.HasResult) + { + return new Outcome(outcome.Value.Result); + } + + return new Outcome(outcome.Value.Exception!); } return await StrategyHelper.ExecuteCallbackSafeAsync(callback, context, state).ConfigureAwait(context.ContinueOnCapturedContext); From a17a644bd9e6d6813aa675a6e63272010b29c161 Mon Sep 17 00:00:00 2001 From: peter-csala Date: Fri, 10 May 2024 08:43:14 +0200 Subject: [PATCH 6/8] Fix sonar rule violation --- src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs index 0918a4b9fa1..a1407281765 100644 --- a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs +++ b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs @@ -31,7 +31,7 @@ protected internal override async ValueTask> ExecuteCore(Func await _onOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); } - if (outcome.HasValue is false) + if (!outcome.HasValue) { return new Outcome(default(T?)); } From 1f0a4d123430a5476236e9b25ea04f53a5699aac Mon Sep 17 00:00:00 2001 From: peter-csala Date: Fri, 10 May 2024 11:18:13 +0200 Subject: [PATCH 7/8] Apply suggestion --- .../Simmy/Outcomes/ChaosOutcomeStrategy.cs | 17 ++--- .../Outcomes/ChaosOutcomeStrategyTests.cs | 63 +++++++++++++------ 2 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs index a1407281765..0f9a2e44d00 100644 --- a/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs +++ b/src/Polly.Core/Simmy/Outcomes/ChaosOutcomeStrategy.cs @@ -20,10 +20,10 @@ protected internal override async ValueTask> ExecuteCore(Func { try { - if (await ShouldInjectAsync(context).ConfigureAwait(context.ContinueOnCapturedContext)) + if (await ShouldInjectAsync(context).ConfigureAwait(context.ContinueOnCapturedContext) + && await _outcomeGenerator(new(context)).ConfigureAwait(context.ContinueOnCapturedContext) is Outcome outcome) { - var outcome = await _outcomeGenerator(new(context)).ConfigureAwait(context.ContinueOnCapturedContext); - var args = new OnOutcomeInjectedArguments(context, outcome.Value); + var args = new OnOutcomeInjectedArguments(context, outcome); _telemetry.Report(new(ResilienceEventSeverity.Information, ChaosOutcomeConstants.OnOutcomeInjectedEvent), context, args); if (_onOutcomeInjected is not null) @@ -31,17 +31,12 @@ protected internal override async ValueTask> ExecuteCore(Func await _onOutcomeInjected(args).ConfigureAwait(context.ContinueOnCapturedContext); } - if (!outcome.HasValue) + if (outcome.HasResult) { - return new Outcome(default(T?)); + return new Outcome(outcome.Result); } - if (outcome.Value.HasResult) - { - return new Outcome(outcome.Value.Result); - } - - return new Outcome(outcome.Value.Exception!); + return new Outcome(outcome.Exception!); } return await StrategyHelper.ExecuteCallbackSafeAsync(callback, context, state).ConfigureAwait(context.ContinueOnCapturedContext); diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs index 907b70bb232..1f4e50ed730 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs @@ -61,7 +61,7 @@ public void Given_not_enabled_should_not_inject_result() InjectionRate = 0.6, Enabled = false, Randomizer = () => 0.5, - OutcomeGenerator = (_) => new ValueTask?>(Outcome.FromResult(fakeResult)) + OutcomeGenerator = _ => new ValueTask?>(Outcome.FromResult(fakeResult)) }; var sut = CreateSut(options); @@ -81,7 +81,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu { InjectionRate = 0.6, Randomizer = () => 0.5, - OutcomeGenerator = (_) => new ValueTask?>(Outcome.FromResult(fakeResult)), + OutcomeGenerator = _ => new ValueTask?>(Outcome.FromResult(fakeResult)), OnOutcomeInjected = args => { args.Context.Should().NotBeNull(); @@ -92,10 +92,10 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu }; var sut = CreateSut(options); - var response = await sut.ExecuteAsync(async _ => + var response = await sut.ExecuteAsync(_ => { _userDelegateExecuted = true; - return await Task.FromResult(HttpStatusCode.OK); + return ValueTask.FromResult(HttpStatusCode.OK); }); response.Should().Be(fakeResult); @@ -117,7 +117,7 @@ public void Given_enabled_and_randomly_not_within_threshold_should_not_inject_re InjectionRate = 0.3, Enabled = false, Randomizer = () => 0.5, - OutcomeGenerator = (_) => new ValueTask?>(Outcome.FromResult(fakeResult)) + OutcomeGenerator = _ => new ValueTask?>(Outcome.FromResult(fakeResult)) }; var sut = CreateSut(options); @@ -133,21 +133,21 @@ public void Given_enabled_and_randomly_not_within_threshold_should_not_inject_re } [Fact] - public async Task Given_enabled_and_randomly_within_threshold_should_inject_result_even_as_null() + public async Task Given_enabled_and_randomly_within_threshold_should_inject_result_if_it_is_null() { - Outcome? nullOutcome = Outcome.FromResult(null); + Outcome? outcomeWithNullResult = Outcome.FromResult(null); var options = new ChaosOutcomeStrategyOptions { InjectionRate = 0.6, Randomizer = () => 0.5, - OutcomeGenerator = (_) => new ValueTask?>(nullOutcome) + OutcomeGenerator = _ => new ValueTask?>(outcomeWithNullResult) }; var sut = CreateSut(options); - var response = await sut.ExecuteAsync(async _ => + var response = await sut.ExecuteAsync(_ => { _userDelegateExecuted = true; - return await Task.FromResult(HttpStatusCode.OK); + return ValueTask.FromResult(HttpStatusCode.OK); }); response.Should().Be(null); @@ -155,6 +155,34 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu _onOutcomeInjectedExecuted.Should().BeFalse(); } + [Fact] + public async Task Given_enabled_and_randomly_within_threshold_should_not_inject_if_generator_returns_null() + { + Outcome? nullOutcome = null; + var options = new ChaosOutcomeStrategyOptions + { + InjectionRate = 0.6, + Randomizer = () => 0.5, + OutcomeGenerator = _ => new ValueTask?>(nullOutcome), + OnOutcomeInjected = args => + { + _onOutcomeInjectedExecuted = true; + return default; + } + }; + + var sut = CreateSut(options); + var response = await sut.ExecuteAsync(_ => + { + _userDelegateExecuted = true; + return ValueTask.FromResult(42); + }); + + response.Should().Be(42); + _userDelegateExecuted.Should().BeTrue(); + _onOutcomeInjectedExecuted.Should().BeFalse(); + } + [Fact] public async Task Given_enabled_and_randomly_within_threshold_should_inject_exception() { @@ -163,8 +191,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_exce { InjectionRate = 0.6, Randomizer = () => 0.5, - OutcomeGenerator = (_) => new ValueTask?>( - Outcome.FromException(exception)), + OutcomeGenerator = _ => new ValueTask?>(Outcome.FromException(exception)), OnOutcomeInjected = args => { args.Outcome.Result.Should().Be(default); @@ -175,10 +202,10 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_exce }; var sut = CreateSut(options); - await sut.Invoking(s => s.ExecuteAsync(async _ => + await sut.Invoking(s => s.ExecuteAsync(_ => { _userDelegateExecuted = true; - return await Task.FromResult(42); + return ValueTask.FromResult(42); }, CancellationToken.None) .AsTask()) .Should() @@ -196,19 +223,19 @@ public async Task Should_not_execute_user_delegate_when_it_was_cancelled_running { InjectionRate = 0.6, Randomizer = () => 0.5, - EnabledGenerator = (_) => + EnabledGenerator = _ => { cts.Cancel(); return new ValueTask(true); }, - OutcomeGenerator = (_) => new ValueTask?>(Outcome.FromResult(HttpStatusCode.TooManyRequests)) + OutcomeGenerator = _ => new ValueTask?>(Outcome.FromResult(HttpStatusCode.TooManyRequests)) }; var sut = CreateSut(options); - await sut.Invoking(s => s.ExecuteAsync(async _ => + await sut.Invoking(s => s.ExecuteAsync(_ => { _userDelegateExecuted = true; - return await Task.FromResult(HttpStatusCode.OK); + return ValueTask.FromResult(HttpStatusCode.OK); }, cts.Token) .AsTask()) .Should() From d689f145de5f62a170a1e992a294b28ac4dbec1f Mon Sep 17 00:00:00 2001 From: peter-csala Date: Fri, 10 May 2024 11:35:00 +0200 Subject: [PATCH 8/8] Fix .net481 support --- .../Simmy/Outcomes/ChaosOutcomeStrategyTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs index 1f4e50ed730..e3aa3101714 100644 --- a/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs +++ b/test/Polly.Core.Tests/Simmy/Outcomes/ChaosOutcomeStrategyTests.cs @@ -95,7 +95,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu var response = await sut.ExecuteAsync(_ => { _userDelegateExecuted = true; - return ValueTask.FromResult(HttpStatusCode.OK); + return new ValueTask(HttpStatusCode.OK); }); response.Should().Be(fakeResult); @@ -147,7 +147,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_resu var response = await sut.ExecuteAsync(_ => { _userDelegateExecuted = true; - return ValueTask.FromResult(HttpStatusCode.OK); + return new ValueTask(HttpStatusCode.OK); }); response.Should().Be(null); @@ -175,7 +175,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_not_inject_ var response = await sut.ExecuteAsync(_ => { _userDelegateExecuted = true; - return ValueTask.FromResult(42); + return new ValueTask(42); }); response.Should().Be(42); @@ -205,7 +205,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_exce await sut.Invoking(s => s.ExecuteAsync(_ => { _userDelegateExecuted = true; - return ValueTask.FromResult(42); + return new ValueTask(42); }, CancellationToken.None) .AsTask()) .Should() @@ -235,7 +235,7 @@ public async Task Should_not_execute_user_delegate_when_it_was_cancelled_running await sut.Invoking(s => s.ExecuteAsync(_ => { _userDelegateExecuted = true; - return ValueTask.FromResult(HttpStatusCode.OK); + return new ValueTask(HttpStatusCode.OK); }, cts.Token) .AsTask()) .Should()