refactor(router): improve exception handling (#1263)

Author: innocenzi

packages/http/src/HttpException.php

@@ -3,11 +3,12 @@
 namespace Tempest\Http;
 
 use Exception;
+use Tempest\Core\HasContext;
 
 /**
  * Represents an HTTP exception.
  */
-final class HttpException extends Exception
+final class HttpException extends Exception implements HasContext
 {
     public function __construct(
         public readonly Status $status,
@@ -17,4 +18,14 @@ public function __construct(
     ) {
         parent::__construct($message ?: '', $status->value, $previous);
     }
+
+    public function context(): array
+    {
+        return [
+            'status' => $this->status->value,
+            'message' => $this->message,
+            'response' => $this->response,
+            'previous' => $this->getPrevious()?->getMessage(),
+        ];
+    }
 }

packages/http/src/Responses/ServerError.php

@@ -4,6 +4,7 @@
 
 namespace Tempest\Http\Responses;
 
+use Exception;
 use Generator;
 use Tempest\Http\IsResponse;
 use Tempest\Http\Response;
@@ -14,9 +15,12 @@ final class ServerError implements Response
 {
     use IsResponse;
 
-    public function __construct(View|Generator|string|array|null $body = null)
+    private(set) ?Exception $exception = null;
+
+    public function __construct(View|Generator|string|array|null $body = null, ?Exception $exception = null)
     {
         $this->status = Status::INTERNAL_SERVER_ERROR;
         $this->body = $body;
+        $this->exception = $exception;
     }
 }

packages/router/src/Exceptions/HttpErrorResponseProcessor.php

@@ -6,35 +6,35 @@
 use Tempest\Http\HttpException;
 use Tempest\Http\Response;
 use Tempest\Router\ResponseProcessor;
+use Tempest\Router\RouteConfig;
 
 final readonly class HttpErrorResponseProcessor implements ResponseProcessor
 {
     public function __construct(
         private AppConfig $appConfig,
+        private RouteConfig $routeConfig,
     ) {}
 
     public function process(Response $response): Response
     {
-        // Throwing an HttpException during tests would make testing more
-        // complex, and is not strictly needed. During development, we
-        // don't need exceptions either, since the exception handler
-        // is different. For this reason, we skip processing here.
-        if (! $this->appConfig->environment->isProduction()) {
+        // If the response is not a server or client error, we don't need to
+        // handle it. In this case, we simply return back the response.
+        if (! $response->status->isServerError() && ! $response->status->isClientError()) {
             return $response;
         }
 
-        // Don't handle responses that already have a body. This is to avoid
-        // interferring with error responses voluntarily thrown in userland.
+        // If the response already has a body, it means it is most likely
+        // meant to be returned as-is, so we don't have to throw an exception.
         if ($response->body) {
             return $response;
         }
 
-        // We throw an exception on server and client errors,
-        // which is handled in the HTTP exception handler.
-        if ($response->status->isServerError() || $response->status->isClientError()) {
-            throw new HttpException(status: $response->status, response: $response);
+        // During tests, the router is generally configured to not throw HTTP exceptions in order
+        // to perform assertions on the responses. In this case, we return the response as is.
+        if (! $this->routeConfig->throwHttpExceptions) {
+            return $response;
         }
 
-        return $response;
+        throw new HttpException(status: $response->status, response: $response);
     }
 }

packages/router/src/GenericRouter.php

@@ -5,8 +5,8 @@
 namespace Tempest\Router;
 
 use BackedEnum;
+use Exception;
 use Psr\Http\Message\ServerRequestInterface as PsrRequest;
-use ReflectionException;
 use Tempest\Container\Container;
 use Tempest\Core\AppConfig;
 use Tempest\Http\GenericRequest;
@@ -18,6 +18,7 @@
 use Tempest\Http\Responses\Invalid;
 use Tempest\Http\Responses\NotFound;
 use Tempest\Http\Responses\Ok;
+use Tempest\Http\Responses\ServerError;
 use Tempest\Mapper\ObjectFactory;
 use Tempest\Reflection\ClassReflector;
 use Tempest\Router\Exceptions\ControllerActionHasNoReturn;
@@ -32,24 +33,15 @@
 use function Tempest\Support\Regex\replace;
 use function Tempest\Support\str;
 
-final class GenericRouter implements Router
+final readonly class GenericRouter implements Router
 {
-    private bool $handleExceptions = true;
-
     public function __construct(
-        private readonly Container $container,
-        private readonly RouteMatcher $routeMatcher,
-        private readonly AppConfig $appConfig,
-        private readonly RouteConfig $routeConfig,
+        private Container $container,
+        private RouteMatcher $routeMatcher,
+        private AppConfig $appConfig,
+        private RouteConfig $routeConfig,
     ) {}
 
-    public function throwExceptions(): self
-    {
-        $this->handleExceptions = false;
-
-        return $this;
-    }
-
     public function dispatch(Request|PsrRequest $request): Response
     {
         return $this->processResponse(
@@ -69,25 +61,16 @@ private function processRequest(Request|PsrRequest $request): Response
             return new NotFound();
         }
 
-        $this->container->singleton(
-            MatchedRoute::class,
-            fn () => $matchedRoute,
-        );
-
-        $callable = $this->getCallable($matchedRoute);
+        $this->container->singleton(MatchedRoute::class, fn () => $matchedRoute);
 
-        if ($this->handleExceptions) {
-            try {
-                $request = $this->resolveRequest($request, $matchedRoute);
-                $response = $callable($request);
-            } catch (NotFoundException) {
-                return new NotFound();
-            } catch (ValidationException $validationException) {
-                return new Invalid($validationException->subject, $validationException->failingRules);
-            }
-        } else {
+        try {
+            $callable = $this->getCallable($matchedRoute);
             $request = $this->resolveRequest($request, $matchedRoute);
             $response = $callable($request);
+        } catch (NotFoundException) {
+            return new NotFound();
+        } catch (ValidationException $validationException) {
+            return new Invalid($validationException->subject, $validationException->failingRules);
         }
 
         return $response;

packages/router/src/RouteConfig.php

@@ -25,6 +25,8 @@ public function __construct(
         public Middleware $middleware = new Middleware(
             SetCookieMiddleware::class,
         ),
+
+        public bool $throwHttpExceptions = true,
     ) {}
 
     public function apply(RouteConfig $newConfig): void

packages/router/src/Static/StaticGenerateCommand.php

@@ -18,6 +18,7 @@
 use Tempest\Http\Status;
 use Tempest\HttpClient\HttpClient;
 use Tempest\Router\DataProvider;
+use Tempest\Router\RouteConfig;
 use Tempest\Router\Router;
 use Tempest\Router\Static\Exceptions\DeadLinksDetectedException;
 use Tempest\Router\Static\Exceptions\InvalidStatusCodeException;
@@ -44,6 +45,7 @@ final class StaticGenerateCommand
 
     public function __construct(
         private readonly AppConfig $appConfig,
+        private readonly RouteConfig $routeConfig,
         private readonly Console $console,
         private readonly Kernel $kernel,
         private readonly Container $container,
@@ -96,6 +98,8 @@ public function __invoke(
             };
         });
 
+        $this->routeConfig->throwHttpExceptions = false;
+
         foreach ($this->staticPageConfig->staticPages as $staticPage) {
             /** @var DataProvider $dataProvider */
             $dataProvider = $this->container->get($staticPage->dataProviderClass ?? GenericDataProvider::class);

src/Tempest/Framework/Testing/Http/HttpRouterTester.php

@@ -11,13 +11,15 @@
 use Tempest\Http\Mappers\RequestToPsrRequestMapper;
 use Tempest\Http\Method;
 use Tempest\Http\Request;
-use Tempest\Router\GenericRouter;
+use Tempest\Router\RouteConfig;
 use Tempest\Router\Router;
 
 use function Tempest\map;
 
 final class HttpRouterTester
 {
+    private bool $throwHttpExceptions = false;
+
     public function __construct(
         private Container $container,
     ) {}
@@ -34,17 +36,6 @@ public function get(string $uri, array $headers = []): TestResponseHelper
         );
     }
 
-    public function throwExceptions(): self
-    {
-        $router = $this->container->get(Router::class);
-
-        if ($router instanceof GenericRouter) {
-            $router->throwExceptions();
-        }
-
-        return $this;
-    }
-
     public function head(string $uri, array $headers = []): TestResponseHelper
     {
         return $this->sendRequest(
@@ -146,11 +137,23 @@ public function sendRequest(Request $request): TestResponseHelper
         /** @var Router $router */
         $router = $this->container->get(Router::class);
 
+        $this->container->get(RouteConfig::class)->throwHttpExceptions = $this->throwHttpExceptions;
+
         return new TestResponseHelper(
             $router->dispatch(map($request)->with(RequestToPsrRequestMapper::class)->do()),
         );
     }
 
+    /**
+     * Instructs the router to throw {@see Tempest\Http\HttpException} errors when an error response is returned. This mimics production behavior.
+     */
+    public function throwExceptions(bool $throw = true): self
+    {
+        $this->throwHttpExceptions = $throw;
+
+        return $this;
+    }
+
     public function makePsrRequest(
         string $uri,
         Method $method = Method::GET,

tests/Integration/Application/HttpApplicationTest.php

@@ -14,7 +14,6 @@ final class HttpApplicationTest extends FrameworkIntegrationTestCase
     public function test_http_application_run(): void
     {
         $this->http
-            ->throwExceptions()
             ->get('/')
             ->assertOk();
     }

tests/Integration/Http/GenericResponseSenderTest.php

@@ -119,6 +119,7 @@ public function test_stream(): void
         $response = new EventStream(fn () => yield 'hello');
         $responseSender = $this->container->get(GenericResponseSender::class);
         $responseSender->send($response);
+
         $output = ob_get_clean();
 
         // restore phpunit's output buffer
@@ -137,6 +138,7 @@ public function test_stream_with_custom_event(): void
         });
         $responseSender = $this->container->get(GenericResponseSender::class);
         $responseSender->send($response);
+
         $output = ob_get_clean();
 
         // restore phpunit's output buffer

tests/Integration/Route/Fixtures/Http500Controller.php

@@ -4,16 +4,25 @@
 
 use Exception;
 use Tempest\Http\Responses\ServerError;
+use Tempest\Router\Get;
 
 final class Http500Controller
 {
-    public function basic500(): string
+    #[Get('/throws-exception')]
+    public function throwsException(): string
     {
         throw new Exception('oops');
     }
 
-    public function custom500(): ServerError
+    #[Get('/returns-server-error')]
+    public function serverError(): ServerError
     {
-        return new ServerError('internal server error');
+        return new ServerError();
+    }
+
+    #[Get('/returns-server-error-with-body')]
+    public function serverErrorWithBody(): ServerError
+    {
+        return new ServerError('custom error');
     }
 }