From 63e03170e685044f5b98c3f602ae3683c1df97eb Mon Sep 17 00:00:00 2001 From: nhmatsumoto Date: Mon, 13 Apr 2026 17:31:33 +0900 Subject: [PATCH] refactor: implement extensible IExceptionMapper strategy for handling HTTP exceptions and add InvalidRequestError support --- .../DependencyInjection/OcelotBuilder.cs | 4 ++ src/Ocelot/Errors/OcelotErrorCode.cs | 3 +- .../Request/Mapper/InvalidRequestError.cs | 10 +++ .../Requester/HttpExceptionToErrorMapper.cs | 25 ++----- src/Ocelot/Requester/IExceptionMapper.cs | 72 +++++++++++++++++++ .../Responder/ErrorsToHttpStatusCodeMapper.cs | 5 ++ .../HttpExceptionToErrorMapperTests.cs | 62 +++++++++++++++- .../ErrorsToHttpStatusCodeMapperTests.cs | 12 +++- 8 files changed, 168 insertions(+), 25 deletions(-) create mode 100644 src/Ocelot/Request/Mapper/InvalidRequestError.cs create mode 100644 src/Ocelot/Requester/IExceptionMapper.cs diff --git a/src/Ocelot/DependencyInjection/OcelotBuilder.cs b/src/Ocelot/DependencyInjection/OcelotBuilder.cs index ff343fafe..a63afbf15 100644 --- a/src/Ocelot/DependencyInjection/OcelotBuilder.cs +++ b/src/Ocelot/DependencyInjection/OcelotBuilder.cs @@ -121,6 +121,10 @@ public OcelotBuilder(IServiceCollection services, IConfiguration configurationRo Services.TryAddSingleton(); Services.TryAddSingleton(); Services.TryAddSingleton(); + Services.AddSingleton(); + Services.AddSingleton(); + Services.AddSingleton(); + Services.AddSingleton(); Services.TryAddSingleton(); Services.TryAddSingleton(); Services.TryAddSingleton(); diff --git a/src/Ocelot/Errors/OcelotErrorCode.cs b/src/Ocelot/Errors/OcelotErrorCode.cs index 613dc4163..0834c6582 100644 --- a/src/Ocelot/Errors/OcelotErrorCode.cs +++ b/src/Ocelot/Errors/OcelotErrorCode.cs @@ -1,4 +1,4 @@ -namespace Ocelot.Errors; +namespace Ocelot.Errors; public enum OcelotErrorCode { @@ -44,4 +44,5 @@ public enum OcelotErrorCode CouldNotFindLoadBalancerCreator = 39, ErrorInvokingLoadBalancerCreator = 40, PayloadTooLargeError = 41, + InvalidRequestError = 42, } diff --git a/src/Ocelot/Request/Mapper/InvalidRequestError.cs b/src/Ocelot/Request/Mapper/InvalidRequestError.cs new file mode 100644 index 000000000..f09409e9d --- /dev/null +++ b/src/Ocelot/Request/Mapper/InvalidRequestError.cs @@ -0,0 +1,10 @@ +using Ocelot.Errors; + +namespace Ocelot.Request.Mapper; + +public class InvalidRequestError : Error +{ + public InvalidRequestError(Exception exception) : base(exception.Message, OcelotErrorCode.InvalidRequestError, (int) System.Net.HttpStatusCode.BadRequest) + { + } +} diff --git a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs index 35c6fa7fb..71620790b 100644 --- a/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs +++ b/src/Ocelot/Requester/HttpExceptionToErrorMapper.cs @@ -7,12 +7,13 @@ namespace Ocelot.Requester; public class HttpExceptionToErrorMapper : IExceptionToErrorMapper { - /// This is a dictionary of custom mappers for exceptions. private readonly IDictionary> _mappers; + private readonly IEnumerable _handlers; - public HttpExceptionToErrorMapper(IServiceProvider serviceProvider) + public HttpExceptionToErrorMapper(IServiceProvider serviceProvider, IEnumerable handlers) { _mappers = serviceProvider.GetService>>(); + _handlers = handlers; } public Error Map(Exception exception) @@ -28,26 +29,12 @@ public Error Map(Exception exception) } // here are mapped the exceptions thrown from Ocelot core application - if (type == typeof(TimeoutException)) + foreach (var handler in _handlers) { - return new RequestTimedOutError(exception); - } - - if (type == typeof(OperationCanceledException) || type.IsSubclassOf(typeof(OperationCanceledException))) - { - return new RequestCanceledError(exception.Message); - } - - if (type == typeof(HttpRequestException) || type == typeof(TimeoutException)) - { - // Inner exception is a BadHttpRequestException, and only this exception exposes the StatusCode property. - // We check if the inner exception is a BadHttpRequestException and if the StatusCode is 413, we return a PayloadTooLargeError - if (exception.InnerException is BadHttpRequestException { StatusCode: StatusCodes.Status413RequestEntityTooLarge }) + if (handler.CanHandle(exception)) { - return new PayloadTooLargeError(exception); + return handler.Map(exception); } - - return new ConnectionToDownstreamServiceError(exception); } return new UnableToCompleteRequestError(exception); diff --git a/src/Ocelot/Requester/IExceptionMapper.cs b/src/Ocelot/Requester/IExceptionMapper.cs new file mode 100644 index 000000000..9a16386c9 --- /dev/null +++ b/src/Ocelot/Requester/IExceptionMapper.cs @@ -0,0 +1,72 @@ +using Microsoft.AspNetCore.Http; +using Ocelot.Errors; +using Ocelot.Request.Mapper; + +namespace Ocelot.Requester; + +public interface IExceptionMapper +{ + bool CanHandle(Exception ex); + Error Map(Exception ex); +} + +public class TimeoutExceptionMapper : IExceptionMapper +{ + public bool CanHandle(Exception ex) => ex is TimeoutException; + public Error Map(Exception ex) => new RequestTimedOutError(ex); +} + +public class OperationCanceledExceptionMapper : IExceptionMapper +{ + public bool CanHandle(Exception ex) => ex is OperationCanceledException; + public Error Map(Exception ex) => new RequestCanceledError(ex.Message); +} + +public class BadHttpRequestExceptionMapper : IExceptionMapper +{ + public bool CanHandle(Exception ex) => ex is BadHttpRequestException; + + public Error Map(Exception ex) + { + var badHttpRequestException = (BadHttpRequestException)ex; + if (badHttpRequestException.StatusCode == StatusCodes.Status413RequestEntityTooLarge) + { + return new PayloadTooLargeError(ex); + } + + if (badHttpRequestException.StatusCode == StatusCodes.Status400BadRequest) + { + return new InvalidRequestError(ex); + } + + return new UnableToCompleteRequestError(ex); + } +} + +public class HttpRequestExceptionMapper : IExceptionMapper +{ + public bool CanHandle(Exception ex) => ex is HttpRequestException; + + public Error Map(Exception ex) + { + if (ex.InnerException is BadHttpRequestException inner) + { + if (inner.StatusCode == StatusCodes.Status413RequestEntityTooLarge) + { + return new PayloadTooLargeError(ex); + } + + if (inner.StatusCode == StatusCodes.Status400BadRequest) + { + return new InvalidRequestError(ex); + } + } + + if (ex.Message.Contains("Request headers must contain only ASCII characters")) + { + return new InvalidRequestError(ex); + } + + return new ConnectionToDownstreamServiceError(ex); + } +} diff --git a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs index c01b0214b..9b56dbb88 100644 --- a/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs +++ b/src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs @@ -61,6 +61,11 @@ public int Map(List errors) return 413; } + if (errors.Any(e => e.Code == OcelotErrorCode.InvalidRequestError)) + { + return 400; + } + return 404; } } diff --git a/test/Ocelot.UnitTests/Requester/HttpExceptionToErrorMapperTests.cs b/test/Ocelot.UnitTests/Requester/HttpExceptionToErrorMapperTests.cs index c346a414b..cd436111f 100644 --- a/test/Ocelot.UnitTests/Requester/HttpExceptionToErrorMapperTests.cs +++ b/test/Ocelot.UnitTests/Requester/HttpExceptionToErrorMapperTests.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http; using Microsoft.Extensions.DependencyInjection; using Ocelot.Errors; using Ocelot.Request.Mapper; @@ -17,8 +17,13 @@ public class HttpExceptionToErrorMapperTests public HttpExceptionToErrorMapperTests() { _services = new ServiceCollection(); + _services.AddSingleton(); + _services.AddSingleton(); + _services.AddSingleton(); + _services.AddSingleton(); var provider = _services.BuildServiceProvider(true); - _mapper = new HttpExceptionToErrorMapper(provider); + var handlers = provider.GetServices(); + _mapper = new HttpExceptionToErrorMapper(provider, handlers); } [Fact] @@ -76,8 +81,9 @@ public void Should_return_error_from_mapper() _services.AddSingleton(errorMapping); var provider = _services.BuildServiceProvider(true); + var handlers = provider.GetServices(); - _mapper = new HttpExceptionToErrorMapper(provider); + _mapper = new HttpExceptionToErrorMapper(provider, handlers); // Act var error = _mapper.Map(new TaskCanceledException()); @@ -121,4 +127,54 @@ public void Map_BadHttpRequestException_To_PayloadTooLargeError() Assert.Equal(413, error.HttpStatusCode); Assert.Equal("test", error.Message); } + + [Fact] + [Trait("Issue", "2376")] // https://github.com/ThreeMammals/Ocelot/issues/2376 + public void Map_HttpRequestException_With_ASCII_Error_To_InvalidRequestError() + { + // Arrange + var ex = new HttpRequestException("Request headers must contain only ASCII characters"); + + // Act + var error = _mapper.Map(ex); + + // Assert + Assert.IsType(error); + Assert.Equal(42, (int)error.Code); + Assert.Equal(400, error.HttpStatusCode); + Assert.Equal("Request headers must contain only ASCII characters", error.Message); + } + + [Fact] + public void Map_BadHttpRequestException_To_InvalidRequestError() + { + // Arrange + var inner = new BadHttpRequestException("test-inner", 400); + var ex = new HttpRequestException("test", inner); + + // Act + var error = _mapper.Map(ex); + + // Assert + Assert.IsType(error); + Assert.Equal(42, (int)error.Code); + Assert.Equal(400, error.HttpStatusCode); + Assert.Equal("test", error.Message); + } + + [Fact] + public void Map_Direct_BadHttpRequestException_To_InvalidRequestError() + { + // Arrange + var ex = new BadHttpRequestException("test", 400); + + // Act + var error = _mapper.Map(ex); + + // Assert + Assert.IsType(error); + Assert.Equal(42, (int)error.Code); + Assert.Equal(400, error.HttpStatusCode); + Assert.Equal("test", error.Message); + } } diff --git a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs index 444750f13..da2c8af63 100644 --- a/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs +++ b/test/Ocelot.UnitTests/Responder/ErrorsToHttpStatusCodeMapperTests.cs @@ -1,4 +1,4 @@ -using Ocelot.Errors; +using Ocelot.Errors; using Ocelot.Responder; namespace Ocelot.UnitTests.Responder; @@ -82,6 +82,14 @@ public void Should_return_request_entity_too_large() { ShouldMapErrorsToStatusCode(new() { OcelotErrorCode.PayloadTooLargeError }, HttpStatusCode.RequestEntityTooLarge); } + + [Fact] + [Trait("Issue", "2376")] + public void Should_return_bad_request() + { + ShouldMapErrorsToStatusCode(new() { OcelotErrorCode.InvalidRequestError }, HttpStatusCode.BadRequest); + } + [Fact] public void AuthenticationErrorsHaveHighestPriority() @@ -128,7 +136,7 @@ public void Check_we_have_considered_all_errors_in_these_tests() // If this test fails then it's because the number of error codes has changed. // You should make the appropriate changes to the test cases here to ensure // they cover all the error codes, and then modify this assertion. - Enum.GetNames().Length.ShouldBe(42, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?"); + Enum.GetNames().Length.ShouldBe(43, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?"); } private void ShouldMapErrorToStatusCode(OcelotErrorCode errorCode, HttpStatusCode expectedHttpStatusCode)