#2376 Refactor: implement extensible IExceptionMapper strategy for handling HTTP exceptions and add InvalidRequestError support

src/Ocelot/DependencyInjection/OcelotBuilder.cs

@@ -121,6 +121,10 @@ public OcelotBuilder(IServiceCollection services, IConfiguration configurationRo
         Services.TryAddSingleton<IDownstreamRequestCreator, DownstreamRequestCreator>();
         Services.TryAddSingleton<IFrameworkDescription, FrameworkDescription>();
         Services.TryAddSingleton<IQoSFactory, QoSFactory>();
+        Services.AddSingleton<IExceptionMapper, TimeoutExceptionMapper>();
+        Services.AddSingleton<IExceptionMapper, OperationCanceledExceptionMapper>();
+        Services.AddSingleton<IExceptionMapper, BadHttpRequestExceptionMapper>();
+        Services.AddSingleton<IExceptionMapper, HttpRequestExceptionMapper>();
         Services.TryAddSingleton<IExceptionToErrorMapper, HttpExceptionToErrorMapper>();
         Services.TryAddSingleton<IVersionCreator, HttpVersionCreator>();
         Services.TryAddSingleton<IVersionPolicyCreator, HttpVersionPolicyCreator>();

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,
 }

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)
+    {
+    }
+}

src/Ocelot/Requester/HttpExceptionToErrorMapper.cs

@@ -7,12 +7,13 @@ namespace Ocelot.Requester;
 
 public class HttpExceptionToErrorMapper : IExceptionToErrorMapper
 {
-    /// <summary>This is a dictionary of custom mappers for exceptions.</summary>
     private readonly IDictionary<Type, Func<Exception, Error>> _mappers;
+    private readonly IEnumerable<IExceptionMapper> _handlers;
 
-    public HttpExceptionToErrorMapper(IServiceProvider serviceProvider)
+    public HttpExceptionToErrorMapper(IServiceProvider serviceProvider, IEnumerable<IExceptionMapper> handlers)
     {
         _mappers = serviceProvider.GetService<IDictionary<Type, Func<Exception, Error>>>();
+        _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);

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);
+    }
+}

src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs

@@ -61,6 +61,11 @@ public int Map(List<Error> errors)
             return 413;
         }
 
+        if (errors.Any(e => e.Code == OcelotErrorCode.InvalidRequestError))
+        {
+            return 400;
+        }
+
         return 404;
     }
 }

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<IExceptionMapper, TimeoutExceptionMapper>();
+        _services.AddSingleton<IExceptionMapper, OperationCanceledExceptionMapper>();
+        _services.AddSingleton<IExceptionMapper, BadHttpRequestExceptionMapper>();
+        _services.AddSingleton<IExceptionMapper, HttpRequestExceptionMapper>();
         var provider = _services.BuildServiceProvider(true);
-        _mapper = new HttpExceptionToErrorMapper(provider);
+        var handlers = provider.GetServices<IExceptionMapper>();
+        _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<IExceptionMapper>();
 
-        _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<InvalidRequestError>(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<InvalidRequestError>(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<InvalidRequestError>(error);
+        Assert.Equal(42, (int)error.Code);
+        Assert.Equal(400, error.HttpStatusCode);
+        Assert.Equal("test", error.Message);
+    }
 }

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<OcelotErrorCode>().Length.ShouldBe(42, "Looks like the number of error codes has changed. Do you need to modify ErrorsToHttpStatusCodeMapper?");
+        Enum.GetNames<OcelotErrorCode>().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)