Run prepare hooks for parser errors#12890
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12890 +/- ##
==========================================
+ Coverage 98.94% 98.95% +0.01%
==========================================
Files 131 131
Lines 47099 47861 +762
Branches 2435 2483 +48
==========================================
+ Hits 46600 47359 +759
- Misses 376 377 +1
- Partials 123 125 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
39f6462 to
c8d844a
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
|
CI update: the code/test/doc checks are green now. The only failing check I see is The contributing docs say an aiohttp committer adds the backport label when a bugfix should be backported, so I am leaving that label choice to maintainers rather than adding |
| if err_info is not None and getattr(request, "_match_info", None) is None: | ||
| request_handler: _RequestHandler[_Request] = cast( | ||
| _RequestHandler[_Request], self._make_error_handler(err_info) | ||
| ) |
There was a problem hiding this comment.
Doesn't this end up doing about the same thing as the old code? Seems like it's not going to solve the features requested..
There was a problem hiding this comment.
I think the difference is where this lets the app request handler back in for the parser-error request.
For the default AppRunner path, web_runner._make_request() sees message is ERROR and attaches request._match_info = MatchInfoError(HTTPBadRequest()). With this change, RequestHandler.start() builds that request first; when _match_info exists, it uses the app-level request handler instead of the protocol _make_error_handler fallback.
Then Application._handle() takes the existing system-route match info, builds middleware for SystemRoute, and returns the HTTPBadRequest response through the normal app path. finish_response() still calls resp.prepare(request), so on_response_prepare fires. The fallback error handler remains for custom request factories that do not attach match info.
I rechecked the targeted coverage locally:
PYTHONPATH=$PWD uv run --no-project --with pytest --with pytest-aiohttp --with pytest-mock --with pytest-timeout --with trustme --with cryptography --with proxy.py --with gunicorn --with uvloop --with dirty-equals --with coverage pytest tests/test_web_functional.py::test_signal_on_error_handler tests/test_web_functional.py::test_signal_on_parser_error_handler tests/test_web_functional.py::test_bad_method_for_c_http_parser_not_hangs -q
Result: 2 passed, 1 skipped; the skipped one is the local checkout missing the C HTTP parser.
|
Thanks for taking a look at this. Unfortunately, this task is deceptively complex and there's a number of details that need be addressed. I've created a new PR to resolve this issue instead: #12925. |
What
Parser-error responses now flow through the application system-route path when the request factory can attach app match info. This lets middleware handle the 400 response and lets
on_response_preparecallbacks adjust headers such asServer.Fixes #6463.
Tests
python3 -m compileall -q aiohttp/web_app.py aiohttp/web_protocol.py aiohttp/web_runner.py tests/test_web_functional.pygit diff --checkPYTHONPATH=$PWD uv run --no-project --with pytest --with pytest-aiohttp --with pytest-mock --with pytest-timeout --with trustme --with cryptography --with proxy.py --with gunicorn --with uvloop --with dirty-equals --with coverage pytest tests/test_web_functional.py::test_signal_on_error_handler tests/test_web_functional.py::test_signal_on_parser_error_handler tests/test_web_functional.py::test_bad_method_for_c_http_parser_not_hangs -qThe C-parser-specific test is skipped locally because the C HTTP parser is unavailable in this checkout.
Risk
The change keeps the fallback protocol error handler for custom request factories that do not attach application match info.