Skip to content

Commit 024d2d2

Browse files
committed
Quick code review by @raman-m
1 parent 04d2653 commit 024d2d2

File tree

6 files changed

+69
-58
lines changed

6 files changed

+69
-58
lines changed

src/Ocelot/Configuration/HttpHandlerOptions.cs

+1-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
1-
using System.Net.Http;
2-
3-
namespace Ocelot.Configuration
1+
namespace Ocelot.Configuration
42
{
53
/// <summary>
64
/// Describes configuration parameters for http handler, that is created to handle a request to service.

src/Ocelot/Configuration/HttpHandlerOptionsBuilder.cs

+8-4
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,13 @@ public HttpHandlerOptionsBuilder WithUseDefaultCredentials(bool input)
5454
return this;
5555
}
5656

57-
public HttpHandlerOptions Build()
58-
{
59-
return new HttpHandlerOptions(_allowAutoRedirect, _useCookieContainer, _useTracing, _useProxy, _maxConnectionPerServer, _pooledConnectionLifetime, _useDefaultCredentials);
60-
}
57+
public HttpHandlerOptions Build() => new(
58+
_allowAutoRedirect,
59+
_useCookieContainer,
60+
_useTracing,
61+
_useProxy,
62+
_maxConnectionPerServer,
63+
_pooledConnectionLifetime,
64+
_useDefaultCredentials);
6165
}
6266
}

src/Ocelot/Requester/MessageInvokerPool.cs

+10-8
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ public class MessageInvokerPool : IMessageInvokerPool
1212

1313
public MessageInvokerPool(IDelegatingHandlerHandlerFactory handlerFactory, IOcelotLoggerFactory loggerFactory)
1414
{
15-
_handlerFactory = handlerFactory ?? throw new ArgumentNullException(nameof(handlerFactory));
15+
ArgumentNullException.ThrowIfNull(handlerFactory);
16+
_handlerFactory = handlerFactory;
1617
_handlersPool = new ConcurrentDictionary<MessageInvokerCacheKey, Lazy<HttpMessageInvoker>>();
1718

1819
ArgumentNullException.ThrowIfNull(loggerFactory);
@@ -70,17 +71,18 @@ private HttpMessageInvoker CreateMessageInvoker(DownstreamRoute downstreamRoute)
7071

7172
private HttpMessageHandler CreateHandler(DownstreamRoute downstreamRoute)
7273
{
74+
var options = downstreamRoute.HttpHandlerOptions;
7375
var handler = new SocketsHttpHandler
7476
{
75-
AllowAutoRedirect = downstreamRoute.HttpHandlerOptions.AllowAutoRedirect,
76-
UseCookies = downstreamRoute.HttpHandlerOptions.UseCookieContainer,
77-
UseProxy = downstreamRoute.HttpHandlerOptions.UseProxy,
78-
MaxConnectionsPerServer = downstreamRoute.HttpHandlerOptions.MaxConnectionsPerServer,
79-
PooledConnectionLifetime = downstreamRoute.HttpHandlerOptions.PooledConnectionLifeTime,
80-
Credentials = downstreamRoute.HttpHandlerOptions.UseDefaultCredentials ? CredentialCache.DefaultCredentials : null,
77+
AllowAutoRedirect = options.AllowAutoRedirect,
78+
UseCookies = options.UseCookieContainer,
79+
UseProxy = options.UseProxy,
80+
MaxConnectionsPerServer = options.MaxConnectionsPerServer,
81+
PooledConnectionLifetime = options.PooledConnectionLifeTime,
82+
Credentials = options.UseDefaultCredentials ? CredentialCache.DefaultCredentials : null,
8183
};
8284

83-
if (downstreamRoute.HttpHandlerOptions.UseCookieContainer)
85+
if (options.UseCookieContainer)
8486
{
8587
handler.CookieContainer = new CookieContainer();
8688
}

test/Ocelot.UnitTests/Configuration/HttpHandlerOptionsCreatorTests.cs

+32-25
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public HttpHandlerOptionsCreatorTests()
2323
}
2424

2525
[Fact]
26-
public void should_not_use_tracing_if_fake_tracer_registered()
26+
public void Should_not_use_tracing_if_fake_tracer_registered()
2727
{
2828
var fileRoute = new FileRoute
2929
{
@@ -42,7 +42,7 @@ public void should_not_use_tracing_if_fake_tracer_registered()
4242
}
4343

4444
[Fact]
45-
public void should_use_tracing_if_real_tracer_registered()
45+
public void Should_use_tracing_if_real_tracer_registered()
4646
{
4747
var fileRoute = new FileRoute
4848
{
@@ -62,7 +62,7 @@ public void should_use_tracing_if_real_tracer_registered()
6262
}
6363

6464
[Fact]
65-
public void should_create_options_with_useCookie_false_and_allowAutoRedirect_true_as_default()
65+
public void Should_create_options_with_useCookie_false_and_allowAutoRedirect_true_as_default()
6666
{
6767
var fileRoute = new FileRoute();
6868
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
7474
}
7575

7676
[Fact]
77-
public void should_create_options_with_specified_useCookie_and_allowAutoRedirect()
77+
public void Should_create_options_with_specified_useCookie_and_allowAutoRedirect()
7878
{
7979
var fileRoute = new FileRoute
8080
{
@@ -95,7 +95,7 @@ public void should_create_options_with_specified_useCookie_and_allowAutoRedirect
9595
}
9696

9797
[Fact]
98-
public void should_create_options_with_useproxy_true_as_default()
98+
public void Should_create_options_with_useproxy_true_as_default()
9999
{
100100
var fileRoute = new FileRoute
101101
{
@@ -111,7 +111,7 @@ public void should_create_options_with_useproxy_true_as_default()
111111
}
112112

113113
[Fact]
114-
public void should_create_options_with_specified_useproxy()
114+
public void Should_create_options_with_specified_useproxy()
115115
{
116116
var fileRoute = new FileRoute
117117
{
@@ -130,7 +130,7 @@ public void should_create_options_with_specified_useproxy()
130130
}
131131

132132
[Fact]
133-
public void should_create_options_with_specified_MaxConnectionsPerServer()
133+
public void Should_create_options_with_specified_MaxConnectionsPerServer()
134134
{
135135
var fileRoute = new FileRoute
136136
{
@@ -149,7 +149,7 @@ public void should_create_options_with_specified_MaxConnectionsPerServer()
149149
}
150150

151151
[Fact]
152-
public void should_create_options_fixing_specified_MaxConnectionsPerServer_range()
152+
public void Should_create_options_fixing_specified_MaxConnectionsPerServer_range()
153153
{
154154
var fileRoute = new FileRoute
155155
{
@@ -168,7 +168,7 @@ public void should_create_options_fixing_specified_MaxConnectionsPerServer_range
168168
}
169169

170170
[Fact]
171-
public void should_create_options_fixing_specified_MaxConnectionsPerServer_range_when_zero()
171+
public void Should_create_options_fixing_specified_MaxConnectionsPerServer_range_when_zero()
172172
{
173173
var fileRoute = new FileRoute
174174
{
@@ -187,38 +187,46 @@ public void should_create_options_fixing_specified_MaxConnectionsPerServer_range
187187
}
188188

189189
[Fact]
190+
[Trait("Feat", "657")]
190191
public void Should_create_options_with_useDefaultCredentials_false_as_default()
191192
{
193+
// Arrange
192194
var fileRoute = new FileRoute
193195
{
194-
HttpHandlerOptions = new FileHttpHandlerOptions(),
196+
HttpHandlerOptions = new(),
195197
};
196-
197-
var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime, false);
198-
199-
this.Given(x => GivenTheFollowing(fileRoute))
200-
.When(x => WhenICreateHttpHandlerOptions())
201-
.Then(x => ThenTheFollowingOptionsReturned(expectedOptions))
202-
.BDDfy();
198+
var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime,
199+
useDefaultCredentials: false);
200+
GivenTheFollowing(fileRoute);
201+
202+
// Act
203+
WhenICreateHttpHandlerOptions();
204+
205+
// Assert
206+
ThenTheFollowingOptionsReturned(expectedOptions);
203207
}
204208

205209
[Fact]
206-
public void Should_create_options_with_useDefaultCredentials_true_if_set()
210+
[Trait("Feat", "657")]
211+
public void Should_create_options_with_UseDefaultCredentials_true_if_set()
207212
{
213+
// Arrange
208214
var fileRoute = new FileRoute
209215
{
210-
HttpHandlerOptions = new FileHttpHandlerOptions
216+
HttpHandlerOptions = new()
211217
{
212218
UseDefaultCredentials = true,
213219
},
214220
};
221+
var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime,
222+
useDefaultCredentials: true);
223+
GivenTheFollowing(fileRoute);
215224

216-
var expectedOptions = new HttpHandlerOptions(false, false, false, true, int.MaxValue, DefaultPooledConnectionLifeTime, true);
225+
// Act
226+
WhenICreateHttpHandlerOptions();
217227

218-
this.Given(x => GivenTheFollowing(fileRoute))
219-
.When(x => WhenICreateHttpHandlerOptions())
220-
.Then(x => ThenTheFollowingOptionsReturned(expectedOptions))
221-
.BDDfy();
228+
// Assert
229+
ThenTheFollowingOptionsReturned(expectedOptions);
222230
}
223231

224232
private void GivenTheFollowing(FileRoute fileRoute)
@@ -244,7 +252,6 @@ private void ThenTheFollowingOptionsReturned(HttpHandlerOptions expected)
244252

245253
private void GivenARealTracer()
246254
{
247-
var tracer = new FakeTracer();
248255
_serviceCollection.AddSingleton<ITracer, FakeTracer>();
249256
_serviceProvider = _serviceCollection.BuildServiceProvider();
250257
_httpHandlerOptionsCreator = new HttpHandlerOptionsCreator(_serviceProvider);

test/Ocelot.UnitTests/Requester/DelegatingHandlerHandlerProviderFactoryTests.cs

+15-13
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public DelegatingHandlerHandlerProviderFactoryTests()
3636
}
3737

3838
[Fact]
39-
public void should_follow_ordering_add_specifics()
39+
public void Should_follow_ordering_add_specifics()
4040
{
4141
var qosOptions = new QoSOptionsBuilder()
4242
.WithTimeoutValue(1)
@@ -72,7 +72,7 @@ public void should_follow_ordering_add_specifics()
7272
}
7373

7474
[Fact]
75-
public void should_follow_ordering_order_specifics_and_globals()
75+
public void Should_follow_ordering_order_specifics_and_globals()
7676
{
7777
var qosOptions = new QoSOptionsBuilder()
7878
.WithTimeoutValue(1)
@@ -109,7 +109,7 @@ public void should_follow_ordering_order_specifics_and_globals()
109109
}
110110

111111
[Fact]
112-
public void should_follow_ordering_order_specifics()
112+
public void Should_follow_ordering_order_specifics()
113113
{
114114
var qosOptions = new QoSOptionsBuilder()
115115
.WithTimeoutValue(1)
@@ -145,7 +145,7 @@ public void should_follow_ordering_order_specifics()
145145
}
146146

147147
[Fact]
148-
public void should_follow_ordering_order_and_only_add_specifics_in_config()
148+
public void Should_follow_ordering_order_and_only_add_specifics_in_config()
149149
{
150150
var qosOptions = new QoSOptionsBuilder()
151151
.WithTimeoutValue(1)
@@ -179,7 +179,7 @@ public void should_follow_ordering_order_and_only_add_specifics_in_config()
179179
}
180180

181181
[Fact]
182-
public void should_follow_ordering_dont_add_specifics()
182+
public void Should_follow_ordering_dont_add_specifics()
183183
{
184184
var qosOptions = new QoSOptionsBuilder()
185185
.WithTimeoutValue(1)
@@ -208,7 +208,7 @@ public void should_follow_ordering_dont_add_specifics()
208208
}
209209

210210
[Fact]
211-
public void should_apply_re_route_specific()
211+
public void Should_apply_re_route_specific()
212212
{
213213
var qosOptions = new QoSOptionsBuilder()
214214
.Build();
@@ -233,7 +233,7 @@ public void should_apply_re_route_specific()
233233
}
234234

235235
[Fact]
236-
public void should_all_from_all_routes_provider_and_qos()
236+
public void Should_all_from_all_routes_provider_and_qos()
237237
{
238238
var qosOptions = new QoSOptionsBuilder()
239239
.WithTimeoutValue(1)
@@ -258,7 +258,7 @@ public void should_all_from_all_routes_provider_and_qos()
258258
}
259259

260260
[Fact]
261-
public void should_return_provider_with_no_delegates()
261+
public void Should_return_provider_with_no_delegates()
262262
{
263263
var qosOptions = new QoSOptionsBuilder()
264264
.Build();
@@ -277,7 +277,7 @@ public void should_return_provider_with_no_delegates()
277277
}
278278

279279
[Fact]
280-
public void should_return_provider_with_qos_delegate()
280+
public void Should_return_provider_with_qos_delegate()
281281
{
282282
var qosOptions = new QoSOptionsBuilder()
283283
.WithTimeoutValue(1)
@@ -301,7 +301,7 @@ public void should_return_provider_with_qos_delegate()
301301
}
302302

303303
[Fact]
304-
public void should_return_provider_with_qos_delegate_when_timeout_value_set()
304+
public void Should_return_provider_with_qos_delegate_when_timeout_value_set()
305305
{
306306
var qosOptions = new QoSOptionsBuilder()
307307
.WithTimeoutValue(1)
@@ -323,7 +323,7 @@ public void should_return_provider_with_qos_delegate_when_timeout_value_set()
323323
}
324324

325325
[Fact]
326-
public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_error()
326+
public void Should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_error()
327327
{
328328
var qosOptions = new QoSOptionsBuilder()
329329
.WithTimeoutValue(1)
@@ -353,7 +353,7 @@ public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factor
353353
}
354354

355355
[Fact]
356-
public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_null()
356+
public void Should_log_error_and_return_no_qos_provider_delegate_when_qos_factory_returns_null()
357357
{
358358
var qosOptions = new QoSOptionsBuilder()
359359
.WithTimeoutValue(1)
@@ -384,7 +384,9 @@ public void should_log_error_and_return_no_qos_provider_delegate_when_qos_factor
384384

385385
private void ThenTheWarningIsLogged()
386386
{
387-
_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);
387+
_logger.Verify(x => x.LogWarning(It.Is<Func<string>>(
388+
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!")),
389+
Times.Once);
388390
}
389391

390392
private void ThenHandlerAtPositionIs<T>(int pos)

test/Ocelot.UnitTests/Requester/MessageInvokerPoolTests.cs

+3-5
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ private void ThenTheDangerousAcceptAnyServerCertificateValidatorWarningIsLogged(
162162

163163
private void ThenTheCookieIsSet()
164164
{
165-
_response.Headers.TryGetValues("Set-Cookie", out var test).ShouldBeTrue();
165+
_response.Headers.TryGetValues("Set-Cookie", out _).ShouldBeTrue();
166166
}
167167

168168
private void GivenADownstreamService()
@@ -249,8 +249,6 @@ private void WhenGettingMessageInvokerForBothRoutes()
249249

250250
private void ThenTheInvokersShouldNotBeTheSame() => Assert.NotEqual(_firstInvoker, _secondInvoker);
251251

252-
private void GivenARequest(string url) => GivenARequestWithAUrlAndMethod(_downstreamRoute1, url, HttpMethod.Get);
253-
254252
private void GivenARequest() =>
255253
GivenARequestWithAUrlAndMethod(_downstreamRoute1, "http://localhost:5003", HttpMethod.Get);
256254

@@ -317,15 +315,15 @@ private void GivenTheFactoryReturns(List<Func<DelegatingHandler>> handlers)
317315
.Returns(new OkResponse<List<Func<DelegatingHandler>>>(handlers));
318316
}
319317

320-
private Mock<IDelegatingHandlerHandlerFactory> GetHandlerFactory()
318+
private static Mock<IDelegatingHandlerHandlerFactory> GetHandlerFactory()
321319
{
322320
var handlerFactory = new Mock<IDelegatingHandlerHandlerFactory>();
323321
handlerFactory.Setup(x => x.Get(It.IsAny<DownstreamRoute>()))
324322
.Returns(new OkResponse<List<Func<DelegatingHandler>>>(new()));
325323
return handlerFactory;
326324
}
327325

328-
private DownstreamRoute DownstreamRouteFactory(string path)
326+
private static DownstreamRoute DownstreamRouteFactory(string path)
329327
{
330328
var downstreamRoute = new DownstreamRouteBuilder()
331329
.WithDownstreamPathTemplate(path)

0 commit comments

Comments
 (0)