Quick code review by @raman-m

ThreeMammals · Apr 25, 2024 · 024d2d2 · 024d2d2
1 parent 04d2653
commit 024d2d2
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 58 deletions.
diff --git a/src/Ocelot/Configuration/HttpHandlerOptions.cs b/src/Ocelot/Configuration/HttpHandlerOptions.cs
@@ -1,6 +1,4 @@
-using System.Net.Http;
-
-namespace Ocelot.Configuration
+namespace Ocelot.Configuration
 {
     /// <summary>
     /// Describes configuration parameters for http handler, that is created to handle a request to service.

diff --git a/src/Ocelot/Configuration/HttpHandlerOptionsBuilder.cs b/src/Ocelot/Configuration/HttpHandlerOptionsBuilder.cs
@@ -54,9 +54,13 @@ public HttpHandlerOptionsBuilder WithUseDefaultCredentials(bool input)
             return this;
         }
 
-        public HttpHandlerOptions Build()
-        {
-            return new HttpHandlerOptions(_allowAutoRedirect, _useCookieContainer, _useTracing, _useProxy, _maxConnectionPerServer, _pooledConnectionLifetime, _useDefaultCredentials);
-        }
+        public HttpHandlerOptions Build() => new(
+            _allowAutoRedirect,
+            _useCookieContainer,
+            _useTracing,
+            _useProxy,
+            _maxConnectionPerServer,
+            _pooledConnectionLifetime,
+            _useDefaultCredentials);
     }
 }
diff --git a/src/Ocelot/Requester/MessageInvokerPool.cs b/src/Ocelot/Requester/MessageInvokerPool.cs
@@ -12,7 +12,8 @@ public class MessageInvokerPool : IMessageInvokerPool
 
     public MessageInvokerPool(IDelegatingHandlerHandlerFactory handlerFactory, IOcelotLoggerFactory loggerFactory)
     {
-        _handlerFactory = handlerFactory ?? throw new ArgumentNullException(nameof(handlerFactory));
+        ArgumentNullException.ThrowIfNull(handlerFactory);
+        _handlerFactory = handlerFactory;
         _handlersPool = new ConcurrentDictionary<MessageInvokerCacheKey, Lazy<HttpMessageInvoker>>();
 
         ArgumentNullException.ThrowIfNull(loggerFactory);
@@ -70,17 +71,18 @@ private HttpMessageInvoker CreateMessageInvoker(DownstreamRoute downstreamRoute)
 
     private HttpMessageHandler CreateHandler(DownstreamRoute downstreamRoute)
     {
+        var options = downstreamRoute.HttpHandlerOptions;
         var handler = new SocketsHttpHandler
         {
-            AllowAutoRedirect = downstreamRoute.HttpHandlerOptions.AllowAutoRedirect,
-            UseCookies = downstreamRoute.HttpHandlerOptions.UseCookieContainer,
-            UseProxy = downstreamRoute.HttpHandlerOptions.UseProxy,
-            MaxConnectionsPerServer = downstreamRoute.HttpHandlerOptions.MaxConnectionsPerServer,
-            PooledConnectionLifetime = downstreamRoute.HttpHandlerOptions.PooledConnectionLifeTime,
-            Credentials = downstreamRoute.HttpHandlerOptions.UseDefaultCredentials ? CredentialCache.DefaultCredentials : null,
+            AllowAutoRedirect = options.AllowAutoRedirect,
+            UseCookies = options.UseCookieContainer,
+            UseProxy = options.UseProxy,
+            MaxConnectionsPerServer = options.MaxConnectionsPerServer,
+            PooledConnectionLifetime = options.PooledConnectionLifeTime,
+            Credentials = options.UseDefaultCredentials ? CredentialCache.DefaultCredentials : null,
         };
 
-        if (downstreamRoute.HttpHandlerOptions.UseCookieContainer)
+        if (options.UseCookieContainer)
         {
             handler.CookieContainer = new CookieContainer();
         }

diff --git a/test/Ocelot.UnitTests/Configuration/HttpHandlerOptionsCreatorTests.cs b/test/Ocelot.UnitTests/Configuration/HttpHandlerOptionsCreatorTests.cs
@@ -23,7 +23,7 @@ public HttpHandlerOptionsCreatorTests()
         }
 
         [Fact]
-        public void should_not_use_tracing_if_fake_tracer_registered()
+        public void Should_not_use_tracing_if_fake_tracer_registered()
         {
             var fileRoute = new FileRoute
             {
@@ -42,7 +42,7 @@ public void should_not_use_tracing_if_fake_tracer_registered()
         }
 
         [Fact]
-        public void should_use_tracing_if_real_tracer_registered()
+        public void Should_use_tracing_if_real_tracer_registered()
         {
             var fileRoute = new FileRoute
             {
@@ -62,7 +62,7 @@ public void should_use_tracing_if_real_tracer_registered()
         }
 
         [Fact]
-        public void should_create_options_with_useCookie_false_and_allowAutoRedirect_true_as_default()
+        public void Should_create_options_with_useCookie_false_and_allowAutoRedirect_true_as_default()
         {
             var fileRoute = new FileRoute();
             var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime, false);
@@ -74,7 +74,7 @@ public void should_create_options_with_useCookie_false_and_allowAutoRedirect_tru
         }
 
         [Fact]
-        public void should_create_options_with_specified_useCookie_and_allowAutoRedirect()
+        public void Should_create_options_with_specified_useCookie_and_allowAutoRedirect()
         {
             var fileRoute = new FileRoute
             {
@@ -95,7 +95,7 @@ public void should_create_options_with_specified_useCookie_and_allowAutoRedirect
         }
 
         [Fact]
-        public void should_create_options_with_useproxy_true_as_default()
+        public void Should_create_options_with_useproxy_true_as_default()
         {
             var fileRoute = new FileRoute
             {
@@ -111,7 +111,7 @@ public void should_create_options_with_useproxy_true_as_default()
         }
 
         [Fact]
-        public void should_create_options_with_specified_useproxy()
+        public void Should_create_options_with_specified_useproxy()
         {
             var fileRoute = new FileRoute
             {
@@ -130,7 +130,7 @@ public void should_create_options_with_specified_useproxy()
         }
 
         [Fact]
-        public void should_create_options_with_specified_MaxConnectionsPerServer()
+        public void Should_create_options_with_specified_MaxConnectionsPerServer()
         {
             var fileRoute = new FileRoute
             {
@@ -149,7 +149,7 @@ public void should_create_options_with_specified_MaxConnectionsPerServer()
         }
 
         [Fact]
-        public void should_create_options_fixing_specified_MaxConnectionsPerServer_range()
+        public void Should_create_options_fixing_specified_MaxConnectionsPerServer_range()
         {
             var fileRoute = new FileRoute
             {
@@ -168,7 +168,7 @@ public void should_create_options_fixing_specified_MaxConnectionsPerServer_range
         }
 
         [Fact]
-        public void should_create_options_fixing_specified_MaxConnectionsPerServer_range_when_zero()
+        public void Should_create_options_fixing_specified_MaxConnectionsPerServer_range_when_zero()
         {
             var fileRoute = new FileRoute
             {
@@ -187,38 +187,46 @@ public void should_create_options_fixing_specified_MaxConnectionsPerServer_range
         }
 
         [Fact]
+        [Trait("Feat", "657")]
         public void Should_create_options_with_useDefaultCredentials_false_as_default()
         {
+            // Arrange
             var fileRoute = new FileRoute
             {
-                HttpHandlerOptions = new FileHttpHandlerOptions(),
+                HttpHandlerOptions = new(),
             };
-
-            var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime, false);
-
-            this.Given(x => GivenTheFollowing(fileRoute))
-                .When(x => WhenICreateHttpHandlerOptions())
-                .Then(x => ThenTheFollowingOptionsReturned(expectedOptions))
-                .BDDfy();
+            var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime,
+                useDefaultCredentials: false);
+            GivenTheFollowing(fileRoute);
+
+            // Act
+            WhenICreateHttpHandlerOptions();
+
+            // Assert
+            ThenTheFollowingOptionsReturned(expectedOptions);
         }
 
         [Fact]
-        public void Should_create_options_with_useDefaultCredentials_true_if_set()
+        [Trait("Feat", "657")]
+        public void Should_create_options_with_UseDefaultCredentials_true_if_set()
         {
+            // Arrange
             var fileRoute = new FileRoute
             {
-                HttpHandlerOptions = new FileHttpHandlerOptions
+                HttpHandlerOptions = new()
                 {
                     UseDefaultCredentials = true,
                 },
             };
+            var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime,
+                useDefaultCredentials: true);
+            GivenTheFollowing(fileRoute);
 
-            var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime, true);
+            // Act
+            WhenICreateHttpHandlerOptions();
 
-            this.Given(x => GivenTheFollowing(fileRoute))
-                .When(x => WhenICreateHttpHandlerOptions())
-                .Then(x => ThenTheFollowingOptionsReturned(expectedOptions))
-                .BDDfy();
+            // Assert
+            ThenTheFollowingOptionsReturned(expectedOptions);
         }
 
         private void GivenTheFollowing(FileRoute fileRoute)
@@ -244,7 +252,6 @@ private void ThenTheFollowingOptionsReturned(HttpHandlerOptions expected)
 
         private void GivenARealTracer()
         {
-            var tracer = new FakeTracer();
             _serviceCollection.AddSingleton<ITracer, FakeTracer>();
             _serviceProvider = _serviceCollection.BuildServiceProvider();
             _httpHandlerOptionsCreator = new HttpHandlerOptionsCreator(_serviceProvider);

diff --git a/test/Ocelot.UnitTests/Requester/DelegatingHandlerHandlerProviderFactoryTests.cs b/test/Ocelot.UnitTests/Requester/DelegatingHandlerHandlerProviderFactoryTests.cs
@@ -36,7 +36,7 @@ public DelegatingHandlerHandlerProviderFactoryTests()
         }
 
         [Fact]
-        public void should_follow_ordering_add_specifics()
+        public void Should_follow_ordering_add_specifics()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -72,7 +72,7 @@ public void should_follow_ordering_add_specifics()
         }
 
         [Fact]
-        public void should_follow_ordering_order_specifics_and_globals()
+        public void Should_follow_ordering_order_specifics_and_globals()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -109,7 +109,7 @@ public void should_follow_ordering_order_specifics_and_globals()
         }
 
         [Fact]
-        public void should_follow_ordering_order_specifics()
+        public void Should_follow_ordering_order_specifics()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -145,7 +145,7 @@ public void should_follow_ordering_order_specifics()
         }
 
         [Fact]
-        public void should_follow_ordering_order_and_only_add_specifics_in_config()
+        public void Should_follow_ordering_order_and_only_add_specifics_in_config()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -179,7 +179,7 @@ public void should_follow_ordering_order_and_only_add_specifics_in_config()
         }
 
         [Fact]
-        public void should_follow_ordering_dont_add_specifics()
+        public void Should_follow_ordering_dont_add_specifics()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -208,7 +208,7 @@ public void should_follow_ordering_dont_add_specifics()
         }
 
         [Fact]
-        public void should_apply_re_route_specific()
+        public void Should_apply_re_route_specific()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .Build();
@@ -233,7 +233,7 @@ public void should_apply_re_route_specific()
         }
 
         [Fact]
-        public void should_all_from_all_routes_provider_and_qos()
+        public void Should_all_from_all_routes_provider_and_qos()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -258,7 +258,7 @@ public void should_all_from_all_routes_provider_and_qos()
         }
 
         [Fact]
-        public void should_return_provider_with_no_delegates()
+        public void Should_return_provider_with_no_delegates()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .Build();
@@ -277,7 +277,7 @@ public void should_return_provider_with_no_delegates()
         }
 
         [Fact]
-        public void should_return_provider_with_qos_delegate()
+        public void Should_return_provider_with_qos_delegate()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -301,7 +301,7 @@ public void should_return_provider_with_qos_delegate()
         }
 
         [Fact]
-        public void should_return_provider_with_qos_delegate_when_timeout_value_set()
+        public void Should_return_provider_with_qos_delegate_when_timeout_value_set()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -323,7 +323,7 @@ public void should_return_provider_with_qos_delegate_when_timeout_value_set()
         }
 
         [Fact]
-        public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_error()
+        public void Should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_error()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -353,7 +353,7 @@ public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factor
         }
 
         [Fact]
-        public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_null()
+        public void Should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_null()
         {
             var qosOptions = new QoSOptionsBuilder()
                 .WithTimeoutValue(1)
@@ -384,7 +384,9 @@ public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factor
 
         private void ThenTheWarningIsLogged()
         {
-            _logger.Verify(x => x.LogWarning(It.Is<Func<string>>(y => y.Invoke() == $"Route {_downstreamRoute.UpstreamPathTemplate} specifies use QoS but no QosHandler found in DI container. Will use not use a QosHandler, please check your setup!")), Times.Once);
+            _logger.Verify(x => x.LogWarning(It.Is<Func<string>>(
+                y => y.Invoke() == $"Route {_downstreamRoute.UpstreamPathTemplate} specifies use QoS but no QosHandler found in DI container. Will use not use a QosHandler, please check your setup!")),
+                Times.Once);
         }
 
         private void ThenHandlerAtPositionIs<T>(int pos)

diff --git a/test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs b/test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs
@@ -162,7 +162,7 @@ private void ThenTheDangerousAcceptAnyServerCertificateValidatorWarningIsLogged(
 
     private void ThenTheCookieIsSet()
     {
-        _response.Headers.TryGetValues("Set-Cookie", out var test).ShouldBeTrue();
+        _response.Headers.TryGetValues("Set-Cookie", out _).ShouldBeTrue();
     }
 
     private void GivenADownstreamService()
@@ -249,8 +249,6 @@ private void WhenGettingMessageInvokerForBothRoutes()
 
     private void ThenTheInvokersShouldNotBeTheSame() => Assert.NotEqual(_firstInvoker, _secondInvoker);
 
-    private void GivenARequest(string url) => GivenARequestWithAUrlAndMethod(_downstreamRoute1, url, HttpMethod.Get);
-
     private void GivenARequest() =>
         GivenARequestWithAUrlAndMethod(_downstreamRoute1, "http://localhost:5003", HttpMethod.Get);
 
@@ -317,15 +315,15 @@ private void GivenTheFactoryReturns(List<Func<DelegatingHandler>> handlers)
             .Returns(new OkResponse<List<Func<DelegatingHandler>>>(handlers));
     }
 
-    private Mock<IDelegatingHandlerHandlerFactory> GetHandlerFactory()
+    private static Mock<IDelegatingHandlerHandlerFactory> GetHandlerFactory()
     {
         var handlerFactory = new Mock<IDelegatingHandlerHandlerFactory>();
         handlerFactory.Setup(x => x.Get(It.IsAny<DownstreamRoute>()))
             .Returns(new OkResponse<List<Func<DelegatingHandler>>>(new()));
         return handlerFactory;
     }
 
-    private DownstreamRoute DownstreamRouteFactory(string path)
+    private static DownstreamRoute DownstreamRouteFactory(string path)
     {
         var downstreamRoute = new DownstreamRouteBuilder()
             .WithDownstreamPathTemplate(path)