diff --git a/README.md b/README.md index 22614c9..12e7299 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ + [Declaring a mapping](#declaring-a-mapping) + [Composing multiple mappings](#composing-multiple-mappings) + [Resolving the response from the exception](#resolving-the-response-from-the-exception) + + [Unmapped exceptions that carry their own status](#unmapped-exceptions-that-carry-their-own-status) + [Logging and displaying error details](#logging-and-displaying-error-details) + [Disabling the fallback for unmapped exceptions](#disabling-the-fallback-for-unmapped-exceptions) * [License](#license) @@ -15,18 +16,23 @@ ## Overview -Provides a PSR-15 middleware that captures exceptions thrown by downstream handlers and translates them into -structured JSON error responses. Each consumer declares an `ExceptionMapping`, a class whose `mappings()` method -returns an `ExceptionMappingTable` that turns each known exception into a `MappedError` (machine-readable code, HTTP -status between 400 and 599, human-readable message, optional headers). The consumer declares only the rules it owns, -and the middleware composes the tables of every configured mapping into a single first-match-wins lookup, so neither -the consumer nor the middleware repeats the composition. Unmapped exceptions either short-circuit to a generic 500 -fallback or rethrow, depending on the builder configuration. +Provides a PSR-15 middleware that captures exceptions thrown by downstream handlers and translates them into structured +JSON error responses. Each consumer declares an `ExceptionMapping`, a class whose `mappings()` method returns an +`ExceptionMappingTable` that turns each known exception into a `MappedError` (machine-readable code, HTTP status between +400 and 599, human-readable message, optional headers). The consumer declares only the rules it owns, and the middleware +composes the tables of every configured mapping into a single first-match-wins lookup, so neither the consumer nor the +middleware repeats the composition. Unmapped exceptions either short-circuit to the fallback response or rethrow, +depending on the builder configuration. + +The fallback distinguishes a client mistake from a server failure. An unmapped exception that already declares a 4xx +status as its exception code is answered with that status, and every other unmapped exception is answered with a generic +`500`. This matters for framework exceptions the consumer never maps, most notably the routing exceptions a PSR-15 stack +raises for an unknown path or an unsupported method: without it, a caller typing the wrong URL is told the server +failed, and a client that retries on 5xx keeps retrying a request that can never succeed. The library integrates with [tiny-blocks/http-correlation-id](https://github.com/tiny-blocks/http-correlation-id) -to enrich every log entry with the request's correlation identifier when one is present on the request -attributes, so error logs can be grouped across services without any extra plumbing in the consumer's log -calls. +to enrich every log entry with the request's correlation identifier when one is present on the request attributes, so +error logs can be grouped across services without any extra plumbing in the consumer's log calls. ## Installation @@ -87,8 +93,8 @@ $middleware = ErrorMiddleware::create() ### Composing multiple mappings When several verticals each own a mapping (for example, a write side and a read side), pass them all to -`withMappings`. The middleware composes them into a single first-match-wins lookup, evaluating the mappings in the -order given, so the consumer never writes the composition by hand. +`withMappings`. The middleware composes them into a single first-match-wins lookup, evaluating the mappings in the order +given, so the consumer never writes the composition by hand. ```php withMapping(mapping: $mapping) + ->build(); + +# A request for a path with no route reaches the middleware as HttpNotFoundException, whose code is 404. +# The response is 404 {"code":"NOT_FOUND","message":"Not Found."} instead of a 500. +``` + +An unmapped exception carrying a 5xx code, or a code that is not an HTTP status at all (a driver error number, for +example), stays a generic 500. Only client errors are adopted, because a server-side failure the consumer did not map is +exactly what the generic fallback is for. + ### Logging and displaying error details -Enables structured error logging and the optional inclusion of exception details in the response body. The -defaults are silent and secure: nothing is logged and no stack traces are returned to the client. +Enables structured error logging and the optional inclusion of exception details in the response body. The defaults are +silent and secure: nothing is logged and no stack traces are returned to the client. + +The log level follows the status of the response that is actually returned. A 4xx is logged as a warning and everything +else as an error, whether the status came from a mapping or from the fallback. A validation failure the consumer maps to +422 is a client mistake, and logging it as an error inflates every error-rate signal built on top of the logs. ```php getCode()); + + return $code?->isClientError() === true ? $code : null; + } + + public static function matches(int $status): bool + { + return $status >= Code::BAD_REQUEST->value && $status < Code::INTERNAL_SERVER_ERROR->value; + } +} diff --git a/src/Internal/ErrorLogger.php b/src/Internal/ErrorLogger.php index 5d6ffff..56994b5 100644 --- a/src/Internal/ErrorLogger.php +++ b/src/Internal/ErrorLogger.php @@ -23,7 +23,7 @@ public static function from(?LoggerInterface $logger, ErrorHandlingSettings $set return new ErrorLogger(logger: $logger, settings: $settings); } - public function log(ServerRequestInterface $request, Throwable $exception): void + public function log(int $status, ServerRequestInterface $request, Throwable $exception): void { if (is_null($this->logger) || !$this->settings->logErrors) { return; @@ -43,6 +43,8 @@ public function log(ServerRequestInterface $request, Throwable $exception): void $context['trace'] = $exception->getTraceAsString(); } - $logger->error('error', $context); + ClientErrorStatus::matches(status: $status) + ? $logger->warning('error', $context) + : $logger->error('error', $context); } } diff --git a/src/Internal/ErrorOutcome.php b/src/Internal/ErrorOutcome.php index 41242e1..987ed58 100644 --- a/src/Internal/ErrorOutcome.php +++ b/src/Internal/ErrorOutcome.php @@ -28,10 +28,16 @@ public function resolve(ServerRequestInterface $request, Throwable $exception): throw $exception; } - $this->errorLogger->log(request: $request, exception: $exception); - - return is_null($mapped) + $response = is_null($mapped) ? FallbackResponse::from(settings: $this->settings, exception: $exception)->toResponse() : MappedResponse::from(mapped: $mapped)->toResponse(); + + $this->errorLogger->log( + status: $response->getStatusCode(), + request: $request, + exception: $exception + ); + + return $response; } } diff --git a/src/Internal/FallbackResponse.php b/src/Internal/FallbackResponse.php index d8b8f7c..39f4861 100644 --- a/src/Internal/FallbackResponse.php +++ b/src/Internal/FallbackResponse.php @@ -23,6 +23,15 @@ public static function from(ErrorHandlingSettings $settings, Throwable $exceptio public function toResponse(): ResponseInterface { + $clientError = ClientErrorStatus::from(exception: $this->exception); + + if (!is_null($clientError)) { + return Response::from( + body: ['code' => $clientError->name, 'message' => sprintf('%s.', $clientError->message())], + code: $clientError + ); + } + if ($this->settings->displayErrorDetails) { return Response::from( body: [ diff --git a/tests/Integration/Drivers/Slim/RoutingExceptionTest.php b/tests/Integration/Drivers/Slim/RoutingExceptionTest.php new file mode 100644 index 0000000..ff68c47 --- /dev/null +++ b/tests/Integration/Drivers/Slim/RoutingExceptionTest.php @@ -0,0 +1,71 @@ +createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new HttpNotFoundException($request)); + + /** @And a middleware that maps none of the framework exceptions */ + $middleware = ErrorMiddleware::create() + ->withMapping(mapping: new UnmappedExceptions()) + ->build(); + + /** @When the middleware processes the request */ + $actual = $middleware->process($request, $handler); + + /** @Then the caller receives the not found status instead of an internal error */ + self::assertSame(Code::NOT_FOUND->value, $actual->getStatusCode()); + + /** @And the body follows the standard error envelope */ + self::assertJsonStringEqualsJsonString( + '{"code":"NOT_FOUND","message":"Not Found."}', + (string)$actual->getBody() + ); + } + + public function testProcessWhenSlimReportsMethodNotAllowedThenMethodNotAllowedIsReturned(): void + { + /** @Given a request using a method the route does not support */ + $request = new ServerRequest('DELETE', '/v1/users'); + + /** @And a handler that throws the routing exception Slim raises for it */ + $handler = $this->createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new HttpMethodNotAllowedException($request)); + + /** @And a middleware that maps none of the framework exceptions */ + $middleware = ErrorMiddleware::create() + ->withMapping(mapping: new UnmappedExceptions()) + ->build(); + + /** @When the middleware processes the request */ + $actual = $middleware->process($request, $handler); + + /** @Then the caller receives the method not allowed status instead of an internal error */ + self::assertSame(Code::METHOD_NOT_ALLOWED->value, $actual->getStatusCode()); + + /** @And the body follows the standard error envelope */ + self::assertJsonStringEqualsJsonString( + '{"code":"METHOD_NOT_ALLOWED","message":"Method Not Allowed."}', + (string)$actual->getBody() + ); + } +} diff --git a/tests/Unit/ErrorMiddlewareTest.php b/tests/Unit/ErrorMiddlewareTest.php index a2767aa..a9de030 100644 --- a/tests/Unit/ErrorMiddlewareTest.php +++ b/tests/Unit/ErrorMiddlewareTest.php @@ -439,6 +439,104 @@ public function testProcessWhenLoggerProvidedAndLogErrorsEnabledAndHandlerThrows self::assertSame(Code::INTERNAL_SERVER_ERROR->value, $actual->getStatusCode()); } + public function testProcessWhenUnmappedExceptionCarriesClientErrorCodeThenThatStatusIsUsed(): void + { + /** @Given a request for a path no route was registered at */ + $request = new ServerRequest('GET', '/unknown'); + + /** @And a handler that throws an unmapped exception carrying 404 as its code */ + $handler = $this->createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new RuntimeException('Not found.', 404)); + + /** @And a middleware with a mapping that declares no rules */ + $middleware = ErrorMiddleware::create() + ->withMapping(mapping: new UnmappedExceptions()) + ->build(); + + /** @When the middleware processes the request */ + $actual = $middleware->process($request, $handler); + + /** @Then the response carries the client error status the exception declared */ + self::assertSame(Code::NOT_FOUND->value, $actual->getStatusCode()); + + /** @And the body names that status instead of an internal error */ + self::assertJsonStringEqualsJsonString( + '{"code":"NOT_FOUND","message":"Not Found."}', + (string)$actual->getBody() + ); + } + + public function testProcessWhenUnmappedExceptionCarriesServerErrorCodeThenInternalErrorIsReturned(): void + { + /** @Given a request */ + $request = new ServerRequest('GET', '/orders'); + + /** @And a handler that throws an unmapped exception carrying 503 as its code */ + $handler = $this->createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new RuntimeException('Upstream down.', 503)); + + /** @And a middleware with a mapping that declares no rules */ + $middleware = ErrorMiddleware::create() + ->withMapping(mapping: new UnmappedExceptions()) + ->build(); + + /** @When the middleware processes the request */ + $actual = $middleware->process($request, $handler); + + /** @Then the response stays an internal error, since only client errors are adopted */ + self::assertSame(Code::INTERNAL_SERVER_ERROR->value, $actual->getStatusCode()); + } + + public function testProcessWhenUnmappedExceptionCarriesNonHttpCodeThenInternalErrorIsReturned(): void + { + /** @Given a request */ + $request = new ServerRequest('GET', '/orders'); + + /** @And a handler that throws an unmapped exception whose code is not an HTTP status */ + $handler = $this->createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new RuntimeException('Driver failure.', 1045)); + + /** @And a middleware with a mapping that declares no rules */ + $middleware = ErrorMiddleware::create() + ->withMapping(mapping: new UnmappedExceptions()) + ->build(); + + /** @When the middleware processes the request */ + $actual = $middleware->process($request, $handler); + + /** @Then the response is an internal error */ + self::assertSame(Code::INTERNAL_SERVER_ERROR->value, $actual->getStatusCode()); + } + + public function testProcessWhenResponseIsClientErrorThenItIsLoggedAsWarning(): void + { + /** @Given a request */ + $request = new ServerRequest('POST', '/users'); + + /** @And a handler that throws an exception mapped to a client error */ + $handler = $this->createStub(RequestHandlerInterface::class); + $handler->method('handle')->willThrowException(new LogicException('Email is not valid.')); + + /** @And a logger that expects a warning and never an error */ + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('warning'); + $logger->expects(self::never())->method('error'); + + /** @And a middleware mapping that exception to 422 with logErrors enabled */ + $middleware = ErrorMiddleware::create() + ->withLogger(logger: $logger) + ->withMapping(mapping: new UnprocessableExceptions()) + ->withSettings(settings: ErrorHandlingSettings::from( + logErrors: true, + logErrorDetails: false, + displayErrorDetails: false + )) + ->build(); + + /** @When the middleware processes the request */ + $middleware->process($request, $handler); + } + public function testProcessWhenMappingReturnsNullAndFallbackEnabledThenFallbackResponseIsReturned(): void { /** @Given a request */ @@ -472,7 +570,7 @@ public function testProcessWhenMappingReturnsNullAndFallbackEnabledThenFallbackR self::assertArrayNotHasKey('trace', $body); } - public function testProcessWhenHandlerThrowsAndMappingResolvesAndLogErrorsEnabledThenErrorIsLogged(): void + public function testProcessWhenHandlerThrowsAndMappingResolvesAndLogErrorsEnabledThenWarningIsLogged(): void { /** @Given a request */ $request = new ServerRequest('POST', '/transactions'); @@ -496,10 +594,10 @@ public function mappings(): ExceptionMappingTable } }; - /** @And a logger that expects exactly one error call */ + /** @And a logger that expects exactly one warning call, since 422 is a client error */ $logger = $this->createMock(LoggerInterface::class); $logger->expects(self::once()) - ->method('error') + ->method('warning') ->with( 'error', self::callback(function (array $context) use ($exceptionMessage): bool { @@ -525,7 +623,7 @@ public function mappings(): ExceptionMappingTable self::assertSame(Code::UNPROCESSABLE_ENTITY->value, $actual->getStatusCode()); } - public function testProcessWhenFallbackDisabledAndMappingResolvesAndLogErrorsEnabledThenErrorIsLogged(): void + public function testProcessWhenFallbackDisabledAndMappingResolvesAndLogErrorsEnabledThenWarningIsLogged(): void { /** @Given a request */ $request = new ServerRequest('PUT', '/config'); @@ -545,10 +643,10 @@ public function mappings(): ExceptionMappingTable } }; - /** @And a logger that expects exactly one error call */ + /** @And a logger that expects exactly one warning call, since 400 is a client error */ $logger = $this->createMock(LoggerInterface::class); $logger->expects(self::once()) - ->method('error') + ->method('warning') ->with( 'error', self::callback(function (array $context) use ($exceptionMessage): bool { diff --git a/tests/Unit/UnprocessableExceptions.php b/tests/Unit/UnprocessableExceptions.php new file mode 100644 index 0000000..2fd3f7c --- /dev/null +++ b/tests/Unit/UnprocessableExceptions.php @@ -0,0 +1,24 @@ +when(exceptionClass: LogicException::class) + ->mapsTo( + code: 'INVALID_REQUEST', + status: Code::UNPROCESSABLE_ENTITY->value, + message: 'The request payload is not valid.' + ); + } +}