ThreeMammals#1550 ThreeMammals#1706 Addressing the QoS options Except…

…ionsAllowedBeforeBreaking issue (ThreeMammals#1753)

* When using the QoS option "ExceptionsAllowedBeforeBreaking" the circuit breaker never opens the circuit.

* merge issue, PortFinder

* some code improvements, using httpresponsemessage status codes as a base for circuit breaker

* Adding more unit tests, and trying to mitigate the test issues with the method "GivenThereIsAPossiblyBrokenServiceRunningOn"

* fixing some test issues

* setting timeout value to 5000 to avoid side effects

* again timing issues

* timing issues again

* ok, first one ok

* Revert "ok, first one ok"

This reverts commit 2e4a673.

* inline method

* putting back logging for http request exception

* removing logger configuration, back to default

* adding a bit more tests to check the policy wrap

* Removing TimeoutStrategy from parameters, it's set by default to pessimistic, at least one policy will be returned, so using First() in circuit breaker and removing the branch Policy == null from delegating handler.

* Fix StyleCop warnings

* Format parameters

* Sort usings

* since we might have two policies wrapped,  timeout and circuit breaker, we can't use the name CircuitBreaker for polly qos provider, it's not right. Using PollyPolicyWrapper and AsnycPollyPolicy instead.

* modifying circuit breaker delegating handler name, usin Polly policies instead

* renaming CircuitBreakerFactory to PolicyWrapperFactory in tests

* DRY for FileConfiguration, using FileConfigurationFactory

* Add copy constructor

* Refactor setup

* Use expression body for method

* Fix acceptance test

* IDE1006 Naming rule violation: These words must begin with upper case characters

* CA1816 Change ReturnsErrorTests.Dispose() to call GC.SuppressFinalize(object)

* Sort usings

* Use expression body for method

* Return back named arguments

---------

Co-authored-by: raman-m <dotnet044@gmail.com>

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

@@ -9,7 +9,7 @@
 using Ocelot.Requester;
 using Polly.CircuitBreaker;
 using Polly.Timeout;
-
+
 namespace Ocelot.Provider.Polly;
 
 public static class OcelotBuilderExtensions

src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs

@@ -43,10 +43,21 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
         var qoSProvider = GetQoSProvider();
 
         // At least one policy (timeout) will be returned
-        // AsyncPollyPolicy can't be null
-        // AsyncPollyPolicy constructor will throw if no policy is provided
-        var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy;
+            // AsyncPollyPolicy can't be null
+            // AsyncPollyPolicy constructor will throw if no policy is provided
+            var policy = qoSProvider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy;
 
-        return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
+            return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
+        }
+        catch (BrokenCircuitException ex)
+        {
+            _logger.LogError("Reached to allowed number of exceptions. Circuit is open", ex);
+            throw;
+        }
+        catch (HttpRequestException ex)
+        {
+            _logger.LogError($"Error in {nameof(PollyPoliciesDelegatingHandler)}.{nameof(SendAsync)}", ex);
+            throw;
+        }
     }
 }

src/Ocelot.Provider.Polly/PollyPolicyWrapper.cs

@@ -11,6 +11,7 @@ public class PollyPolicyWrapper<TResult>
     public PollyPolicyWrapper(params IAsyncPolicy<TResult>[] policies)
     {
         var allPolicies = policies.Where(p => p != null).ToArray();
+        AsyncPollyPolicy = allPolicies.First();
 
         AsyncPollyPolicy = allPolicies.Length > 1 ? 
             Policy.WrapAsync(allPolicies) : 

src/Ocelot/Configuration/File/FileCacheOptions.cs

@@ -4,18 +4,20 @@ public class FileCacheOptions
     {
         public FileCacheOptions()
         {
+            Header = string.Empty;
             Region = string.Empty;
             TtlSeconds = 0;
         }
 
         public FileCacheOptions(FileCacheOptions from)
         {
+            Header = from.Header;
             Region = from.Region;
             TtlSeconds = from.TtlSeconds;
         }
-
-        public int TtlSeconds { get; set; }
-        public string Region { get; set; }
+
         public string Header { get; set; }
+        public string Region { get; set; }
+        public int TtlSeconds { get; set; }
     }
 }

test/Ocelot.AcceptanceTests/PollyQoSTests.cs

@@ -1,6 +1,6 @@
-using Microsoft.AspNetCore.Http;
-using Ocelot.Configuration;
-using Ocelot.Configuration.File;
+using Microsoft.AspNetCore.Http;
+using Ocelot.Configuration;
+using Ocelot.Configuration.File;
 
 namespace Ocelot.AcceptanceTests
 {
@@ -9,11 +9,11 @@ public class PollyQoSTests : IDisposable
         private readonly Steps _steps;
         private readonly ServiceHandler _serviceHandler;
 
-        public PollyQoSTests()
-        {
-            _serviceHandler = new ServiceHandler();
-            _steps = new Steps();
-        }
+    public PollyQoSTests()
+    {
+        _serviceHandler = new ServiceHandler();
+        _steps = new Steps();
+    }
 
         private static FileConfiguration FileConfigurationFactory(int port, QoSOptions options, string httpMethod = nameof(HttpMethods.Get))
             => new()
@@ -87,24 +87,24 @@ public void Should_open_circuit_breaker_then_close()
             var port = PortFinder.GetRandomPort();
             var configuration = FileConfigurationFactory(port, new QoSOptions(1, 500, 1000, null));
 
-            this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port}", "Hello from Laura"))
-                .Given(x => _steps.GivenThereIsAConfiguration(configuration))
-                .Given(x => _steps.GivenOcelotIsRunningWithPolly())
-                .When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
-                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
-                .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
-                .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
-                .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
-                .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
-                .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
-                .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
-                .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
-                .Given(x => GivenIWaitMilliseconds(3000))
-                .When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
-                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
-                .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
-                .BDDfy();
-        }
+        this.Given(x => x.GivenThereIsAPossiblyBrokenServiceRunningOn($"http://localhost:{port}", "Hello from Laura"))
+            .Given(x => _steps.GivenThereIsAConfiguration(configuration))
+            .Given(x => _steps.GivenOcelotIsRunningWithPolly())
+            .When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
+            .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+            .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
+            .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
+            .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
+            .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
+            .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
+            .Given(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
+            .Given(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
+            .Given(x => GivenIWaitMilliseconds(3000))
+            .When(x => _steps.WhenIGetUrlOnTheApiGateway("/"))
+            .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+            .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura"))
+            .BDDfy();
+    }
 
         [Fact]
         public void Open_circuit_should_not_effect_different_route()
@@ -156,7 +156,7 @@ public void Should_timeout_per_default_after_90_seconds()
                 .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.ServiceUnavailable))
                 .BDDfy();
         }
-
+
         private static void GivenIWaitMilliseconds(int ms) => Thread.Sleep(ms);
 
         private void GivenThereIsABrokenServiceRunningOn(string url)
@@ -184,21 +184,20 @@ private void GivenThereIsAPossiblyBrokenServiceRunningOn(string url, string resp
             });
         }
 
-        private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout)
-        {
-            _serviceHandler.GivenThereIsAServiceRunningOn(url, async context =>
-            {
-                Thread.Sleep(timeout);
-                context.Response.StatusCode = statusCode;
-                await context.Response.WriteAsync(responseBody);
-            });
-        }
-
-        public void Dispose()
-        {
-            _serviceHandler?.Dispose();
-            _steps.Dispose();
-            GC.SuppressFinalize(this);
-        }
-    }
+    private void GivenThereIsAServiceRunningOn(string url, int statusCode, string responseBody, int timeout)
+    {
+        _serviceHandler.GivenThereIsAServiceRunningOn(url, async context =>
+        {
+            Thread.Sleep(timeout);
+            context.Response.StatusCode = statusCode;
+            await context.Response.WriteAsync(responseBody);
+        });
+    }
+
+    public void Dispose()
+    {
+        _serviceHandler?.Dispose();
+        _steps.Dispose();
+        GC.SuppressFinalize(this);
+    }
 }

test/Ocelot.AcceptanceTests/ReturnsErrorTests.cs

@@ -123,7 +123,7 @@ private void GivenThereIsAServiceRunningOn(string url)
         public void Dispose()
         {
             _serviceHandler?.Dispose();
-            _steps.Dispose();
+            _steps.Dispose();
             GC.SuppressFinalize(this);
         }
     }