fix(middleware): error responses skipped the entire middleware stack - #16
Merged
Conversation
… all middleware 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by actually running the framework: every response built from an exception — a router 404/405, or a handler raising HTTPException — completely bypassed middleware. Root cause: route resolution and exception-to-response conversion both happened outside build_middleware_stack, in Velocix.call/_process_request's own try/except. Each middleware's
response = await self.app(request)never got a Response in the error case — the exception just unwound past every middleware. Confirmed live against a real app: rate-limit bypass on 404 floods, no request-id/metrics/rate-limit headers on any error response, only on 200s.Two-part fix: call now defers route resolution into _execute_handler's own (previously dead) fallback when middleware is configured, and a new _dispatch() is the actual terminal callable passed to build_middleware_stack — it catches every exception and converts to a Response right there, so self.app(request) always returns normally.
That exposed a second bug in the fallback _dispatch now exercises for every request: it resolved the handler but discarded the resolved path_params, so every dynamic route with middleware configured returned 422 (missing required path param) instead of reaching the handler. Fixed alongside.
Added tests/test_middleware_error_paths.py covering all four combinations. 256/256 tests, mypy clean, ruff clean — verified in a fresh venv and against a real running app.