Do not encourage re-throwing the same exception instance (#1900)

App-vNext · Jan 17, 2024 · dc8248d · dc8248d
1 parent 0d78805
commit dc8248d
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 32 deletions.
diff --git a/src/Polly.Core/PublicAPI.Unshipped.txt b/src/Polly.Core/PublicAPI.Unshipped.txt
@@ -25,8 +25,6 @@ Polly.Simmy.Fault.FaultGeneratorArguments.Context.get -> Polly.ResilienceContext
 Polly.Simmy.Fault.FaultGeneratorArguments.FaultGeneratorArguments() -> void
 Polly.Simmy.Fault.FaultGeneratorArguments.FaultGeneratorArguments(Polly.ResilienceContext! context) -> void
 Polly.Simmy.Fault.FaultStrategyOptions
-Polly.Simmy.Fault.FaultStrategyOptions.Fault.get -> System.Exception?
-Polly.Simmy.Fault.FaultStrategyOptions.Fault.set -> void
 Polly.Simmy.Fault.FaultStrategyOptions.FaultGenerator.get -> System.Func<Polly.Simmy.Fault.FaultGeneratorArguments, System.Threading.Tasks.ValueTask<System.Exception?>>?
 Polly.Simmy.Fault.FaultStrategyOptions.FaultGenerator.set -> void
 Polly.Simmy.Fault.FaultStrategyOptions.FaultStrategyOptions() -> void

diff --git a/src/Polly.Core/Simmy/Fault/FaultChaosStrategy.cs b/src/Polly.Core/Simmy/Fault/FaultChaosStrategy.cs
@@ -9,23 +9,16 @@ internal class FaultChaosStrategy : MonkeyStrategy
     public FaultChaosStrategy(FaultStrategyOptions options, ResilienceStrategyTelemetry telemetry)
         : base(options)
     {
-        if (options.Fault is null && options.FaultGenerator is null)
-        {
-            throw new InvalidOperationException("Either Fault or FaultGenerator is required.");
-        }
-
         _telemetry = telemetry;
-        Fault = options.Fault;
+
         OnFaultInjected = options.OnFaultInjected;
-        FaultGenerator = options.FaultGenerator is not null ? options.FaultGenerator : (_) => new(options.Fault);
+        FaultGenerator = options.FaultGenerator!;
     }
 
     public Func<OnFaultInjectedArguments, ValueTask>? OnFaultInjected { get; }
 
     public Func<FaultGeneratorArguments, ValueTask<Exception?>> FaultGenerator { get; }
 
-    public Exception? Fault { get; }
-
     protected internal override async ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
         Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>> callback,
         ResilienceContext context,

diff --git a/src/Polly.Core/Simmy/Fault/FaultStrategyOptions.cs b/src/Polly.Core/Simmy/Fault/FaultStrategyOptions.cs
@@ -1,4 +1,6 @@
-namespace Polly.Simmy.Fault;
+using System.ComponentModel.DataAnnotations;
+
+namespace Polly.Simmy.Fault;
 
 /// <summary>
 /// Represents the options for the Fault chaos strategy.
@@ -20,14 +22,6 @@ public class FaultStrategyOptions : MonkeyStrategyOptions
     /// Defaults to <see langword="null"/>. Either <see cref="Fault"/> or this property is required.
     /// When this property is <see langword="null"/> the <see cref="Fault"/> is used.
     /// </remarks>
+    [Required]
     public Func<FaultGeneratorArguments, ValueTask<Exception?>>? FaultGenerator { get; set; } = default!;
-
-    /// <summary>
-    /// Gets or sets the outcome to be injected for a given execution.
-    /// </summary>
-    /// <remarks>
-    /// Defaults to <see langword="null"/>. Either <see cref="FaultGenerator"/> or this property is required.
-    /// When this property is <see langword="null"/> the <see cref="FaultGenerator"/> is used.
-    /// </remarks>
-    public Exception? Fault { get; set; }
 }
diff --git a/test/Polly.Core.Tests/Simmy/Fault/FaultChaosPipelineBuilderExtensionsTests.cs b/test/Polly.Core.Tests/Simmy/Fault/FaultChaosPipelineBuilderExtensionsTests.cs
@@ -17,11 +17,10 @@ public class FaultChaosPipelineBuilderExtensionsTests
                 InjectionRate = 0.6,
                 Enabled = true,
                 Randomizer = () => 0.5,
-                Fault = new InvalidOperationException("Dummy exception.")
+                FaultGenerator = _=> new ValueTask<Exception?>( new InvalidOperationException("Dummy exception."))
             });
 
-            AssertFaultStrategy<InvalidOperationException>(builder, true, 0.6)
-            .Fault.Should().BeOfType(typeof(InvalidOperationException));
+            AssertFaultStrategy<InvalidOperationException>(builder, true, 0.6).FaultGenerator.Should().NotBeNull();
         },
     };
 #pragma warning restore IDE0028

diff --git a/test/Polly.Core.Tests/Simmy/Fault/FaultChaosStrategyTests.cs b/test/Polly.Core.Tests/Simmy/Fault/FaultChaosStrategyTests.cs
@@ -59,7 +59,7 @@ public void Given_not_enabled_should_not_inject_fault()
             InjectionRate = 0.6,
             Enabled = false,
             Randomizer = () => 0.5,
-            Fault = fault
+            FaultGenerator = _ => new ValueTask<Exception?>(fault)
         };
 
         var sut = CreateSut(options);
@@ -79,7 +79,7 @@ public async Task Given_not_enabled_should_not_inject_fault_and_return_outcome()
             InjectionRate = 0.6,
             Enabled = false,
             Randomizer = () => 0.5,
-            Fault = fault
+            FaultGenerator = _ => new ValueTask<Exception?>(fault)
         };
 
         var sut = new ResiliencePipelineBuilder<HttpStatusCode>().AddChaosFault(options).Build();
@@ -106,7 +106,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_faul
             InjectionRate = 0.6,
             Enabled = true,
             Randomizer = () => 0.5,
-            Fault = fault,
+            FaultGenerator = _ => new ValueTask<Exception?>(fault),
             OnFaultInjected = args =>
             {
                 args.Context.Should().NotBeNull();
@@ -143,7 +143,7 @@ public async Task Given_enabled_and_randomly_within_threshold_should_inject_faul
             InjectionRate = 0.6,
             Enabled = true,
             Randomizer = () => 0.5,
-            Fault = fault,
+            FaultGenerator = _ => new ValueTask<Exception?>(fault),
             OnFaultInjected = args =>
             {
                 args.Context.Should().NotBeNull();
@@ -181,7 +181,7 @@ public void Given_enabled_and_randomly_not_within_threshold_should_not_inject_fa
             InjectionRate = 0.3,
             Enabled = true,
             Randomizer = () => 0.5,
-            Fault = fault
+            FaultGenerator = _ => new ValueTask<Exception?>(fault)
         };
 
         var sut = CreateSut(options);
@@ -232,7 +232,7 @@ public async Task Should_not_execute_user_delegate_when_it_was_cancelled_running
                 return new ValueTask<bool>(true);
             },
             Randomizer = () => 0.5,
-            Fault = fault
+            FaultGenerator = _ => new ValueTask<Exception?>(fault)
         };
 
         var sut = CreateSut(options);

diff --git a/test/Polly.Core.Tests/Simmy/Fault/FaultStrategyOptionsTests.cs b/test/Polly.Core.Tests/Simmy/Fault/FaultStrategyOptionsTests.cs
@@ -1,5 +1,7 @@
-using Polly.Simmy;
+using System.ComponentModel.DataAnnotations;
+using Polly.Simmy;
 using Polly.Simmy.Fault;
+using Polly.Utils;
 
 namespace Polly.Core.Tests.Simmy.Fault;
 
@@ -14,8 +16,26 @@ public void Ctor_Ok()
         sut.EnabledGenerator.Should().BeNull();
         sut.InjectionRate.Should().Be(MonkeyStrategyConstants.DefaultInjectionRate);
         sut.InjectionRateGenerator.Should().BeNull();
-        sut.Fault.Should().BeNull();
         sut.OnFaultInjected.Should().BeNull();
         sut.FaultGenerator.Should().BeNull();
     }
+
+    [Fact]
+    public void InvalidOptions()
+    {
+        var options = new FaultStrategyOptions
+        {
+            FaultGenerator = null!,
+        };
+
+        options.Invoking(o => ValidationHelper.ValidateObject(new(o, "Invalid Options")))
+            .Should()
+            .Throw<ValidationException>()
+            .WithMessage("""
+            Invalid Options
+            
+            Validation Errors:
+            The FaultGenerator field is required.
+            """);
+    }
 }