fix(middleware): error responses skipped the entire middleware stack

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())

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: