From 7b780559c6ccdc107d6cd93ab431602beff2b8dc Mon Sep 17 00:00:00 2001 From: magi Date: Sun, 6 Sep 2026 20:53:35 +0530 Subject: [PATCH] fix(middleware): error responses (404/405/handler exceptions) skipped all middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every response built from an exception -- a router 404/405, or a handler raising HTTPException -- completely bypassed the middleware stack. Root cause: route resolution and exception-to-response conversion both happened in Velocix.__call__/​_process_request's own try/except, outside build_middleware_stack entirely. Each middleware's `response = await self.app(request)` never got a Response back in the error case -- the exception just unwound past every middleware on its way to the outer handler, so none of them ever got a chance to add a header, count a metric, or otherwise touch the response. Confirmed live: rate-limit bypass on 404 floods, no request-id/metrics on any error response. Fix has two parts: - __call__ defers route resolution into _execute_handler's existing (but previously dead) fallback when middleware is configured, instead of resolving before build_middleware_stack ever runs -- so a 404/405 is raised from inside the middleware-wrapped call, not before it. - New _dispatch() is now the terminal callable passed to build_middleware_stack (instead of _execute_handler directly): it catches every exception -- resolution failures and handler-raised HTTPExceptions alike -- and converts them to a Response right there, so every middleware's self.app(request) call always returns a real Response, success or error, instead of sometimes raising past it. Fixing this exposed a second, latent bug in the fallback _dispatch now exercises for every request: it resolved the handler but discarded the resolved path_params, silently keeping the stale (empty, in the deferred- resolution case) ones already on the request. Every dynamic route with middleware configured was returning 422 (missing required path param) instead of ever reaching the handler. Fixed by assigning the resolved path_params back onto the request. Added tests/test_middleware_error_paths.py covering all four combinations (router 404, handler-raised exception, dynamic route param binding, and the success path) with and without middleware. 256/256 tests, mypy clean, ruff clean -- verified in a fresh venv matching CI's install steps, and against a real running app (blog demo): X-Request-ID and X-RateLimit-Key now appear on 404s and 401s, not just 200s. --- tests/test_middleware_error_paths.py | 103 +++++++++++++++++++++++++++ velocix/core/app.py | 62 +++++++++++++--- 2 files changed, 155 insertions(+), 10 deletions(-) create mode 100644 tests/test_middleware_error_paths.py diff --git a/tests/test_middleware_error_paths.py b/tests/test_middleware_error_paths.py new file mode 100644 index 0000000..9d182aa --- /dev/null +++ b/tests/test_middleware_error_paths.py @@ -0,0 +1,103 @@ +"""Regression tests: middleware used to be entirely skipped for any response +built from an exception -- a router 404/405, or a handler-raised +HTTPException -- because exceptions unwound past every middleware's own +`response = await self.app(request)` call instead of that call ever +returning a Response. Only the success path ever reached a middleware's +post-processing (adding a header, counting a metric, etc). +""" + +import asyncio + +from velocix import TestClient, Velocix +from velocix.core.exceptions import NotFound +from velocix.core.middleware import BaseMiddleware + + +def _run(coro): + return asyncio.run(coro) + + +class _TagMiddleware(BaseMiddleware): + """Marks every response that passes through it, success or error.""" + + async def __call__(self, request): + response = await self.app(request) + response.headers["X-Tagged"] = "yes" + return response + + +def test_router_404_still_gets_middleware_effects(): + app = Velocix() + + @app.get("/known") + async def known(): + return {} + + app.add_middleware(_TagMiddleware) + + async def scenario(): + async with TestClient(app) as client: + resp = await client.get("/does-not-exist") + assert resp.status_code == 404 + assert resp.headers.get("X-Tagged") == "yes" + + _run(scenario()) + + +def test_handler_raised_exception_still_gets_middleware_effects(): + app = Velocix() + + @app.get("/posts/{post_id}") + async def get_post(post_id: int): + raise NotFound(f"Post {post_id} not found") + + app.add_middleware(_TagMiddleware) + + async def scenario(): + async with TestClient(app) as client: + resp = await client.get("/posts/999") + assert resp.status_code == 404 + assert resp.headers.get("X-Tagged") == "yes" + + _run(scenario()) + + +def test_dynamic_route_path_params_still_bind_with_middleware(): + """Resolution is deferred into the middleware-wrapped path when + middleware is configured; the resolved path_params must survive that, + or every dynamic route fails required-param validation (422) instead + of ever reaching the handler.""" + app = Velocix() + + @app.get("/posts/{post_id}") + async def get_post(post_id: int): + return {"post_id": post_id} + + app.add_middleware(_TagMiddleware) + + async def scenario(): + async with TestClient(app) as client: + resp = await client.get("/posts/42") + assert resp.status_code == 200 + assert resp.json() == {"post_id": 42} + assert resp.headers.get("X-Tagged") == "yes" + + _run(scenario()) + + +def test_success_path_unaffected(): + app = Velocix() + + @app.get("/ok") + async def ok(): + return {"ok": True} + + app.add_middleware(_TagMiddleware) + + async def scenario(): + async with TestClient(app) as client: + resp = await client.get("/ok") + assert resp.status_code == 200 + assert resp.headers.get("X-Tagged") == "yes" + + _run(scenario()) diff --git a/velocix/core/app.py b/velocix/core/app.py index e26baae..f27a236 100644 --- a/velocix/core/app.py +++ b/velocix/core/app.py @@ -494,10 +494,19 @@ async def _handle_http( request: Request | None = None try: - handler, path_params = self.router.resolve(scope["method"], scope["path"]) - if handler is None: - raise NotFound(f"Route not found: {scope['path']}") - response = await self._process_request(scope, receive, handler, path_params) + if self._middleware_stack: + # Defer resolution into _execute_handler's own fallback (it + # already does this when handler is None) instead of raising + # NotFound/405 here, before middleware ever runs. Otherwise + # every unmatched path skips rate limiting, security + # scanning, request-id, metrics -- all of it -- since this + # try/except is outside build_middleware_stack entirely. + response = await self._process_request(scope, receive, None, {}) + else: + handler, path_params = self.router.resolve(scope["method"], scope["path"]) + if handler is None: + raise NotFound(f"Route not found: {scope['path']}") + response = await self._process_request(scope, receive, handler, path_params) except Exception as exc: if request is None: request = Request(scope, receive) @@ -541,7 +550,7 @@ async def _process_request( self, scope: dict[str, Any], receive: Callable[[], Awaitable[dict[str, Any]]], - handler: Callable[..., Any], + handler: Callable[..., Any] | None, path_params: dict[str, Any], ) -> ResponseType: """Process HTTP request with error handling and middleware""" @@ -550,12 +559,15 @@ async def _process_request( if self._middleware_stack: if self._compiled_middleware is None: self._compiled_middleware = build_middleware_stack( - self._execute_handler, self._middleware_stack + self._dispatch, self._middleware_stack ) # One plan lookup per request, stashed on the Request so the # middleware terminal re-reads it without another lookup. - entry = get_plan_and_needs_request(handler) + # handler is None when __call__ deferred resolution here (the + # common case now); _execute_handler resolves it lazily via + # its own fallback, so there's no plan to prefetch yet. + entry = get_plan_and_needs_request(handler) if handler is not None else None request = self._init_request(scope, receive, handler, path_params) request._plan = entry response = await self._compiled_middleware(request) @@ -568,6 +580,11 @@ async def _process_request( # No middleware: pass everything explicitly so _execute_handler # never re-reads handler/plan/path_params off the Request. + # __call__ only reaches this branch after resolving a real + # handler (raising NotFound itself otherwise), so it's never None + # here -- unlike the middleware branch above, which defers + # resolution and so passes None on purpose. + assert handler is not None ( plan, needs_request, @@ -614,7 +631,7 @@ def _init_request( self, scope: dict[str, Any], receive: Callable[[], Awaitable[dict[str, Any]]], - handler: Callable[..., Any], + handler: Callable[..., Any] | None, path_params: dict[str, Any], ) -> Request: """Build a Request and attach the resolved handler/params""" @@ -624,6 +641,24 @@ def _init_request( request._handler = handler return request + async def _dispatch(self, request: Request) -> ResponseType: + """Terminal callable for the compiled middleware stack. + + Catches every exception right here -- a router 404/405 (resolved + lazily inside _execute_handler when request._handler is None), a + handler-raised HTTPException, or an unhandled bug -- and converts it + to a Response before returning, instead of letting it propagate as + an exception. Every middleware's `response = await self.app(request)` + must always get back a real Response, success or error, or it never + gets a chance to add its own effect (a request-id header, a + Retry-After header, a metrics count) to that response at all -- + exceptions unwind straight past every middleware's own logic. + """ + try: + return await self._execute_handler(request) + except Exception as exc: + return await self._handle_exception(request, exc) + async def _execute_handler( self, request: Request | None, @@ -642,10 +677,17 @@ async def _execute_handler( # Called through the middleware stack with a real Request handler = request._handler # type: ignore[union-attr] if handler is None: - # Fallback for handlers invoked outside the normal request path - handler, _ = self.router.resolve(request.method, request.path) # type: ignore[union-attr] + # Resolution was deferred here (by __call__, when middleware + # is configured) so a 404/405 still flows through every + # middleware instead of bypassing all of it. The resolved + # path_params must be kept -- discarding them left every + # deferred-resolution request believing it had none, so a + # dynamic route like /posts/{id} always failed required-path- + # param validation (422) instead of ever reaching the handler. + handler, resolved_params = self.router.resolve(request.method, request.path) # type: ignore[union-attr] if handler is None: raise NotFound() + request.path_params = resolved_params # type: ignore[union-attr] path_params = request.path_params # type: ignore[union-attr] if plan is None: