Fix TestClient passing broken request objects to endpoints#1680
Fix TestClient passing broken request objects to endpoints#1680brianhelba wants to merge 4 commits intovitalik:masterfrom
TestClient passing broken request objects to endpoints#1680Conversation
Endpoints called via `TestClient` received a `request` where any `HttpRequest` method not explicitly stubbed returned a `Mock` object instead of a real value. This was causing all the following bugs, which are now resolved: `request.build_absolute_uri()` was replaced with a stub that ignored the request path entirely, returning `"http://testlocation/"` for every endpoint. Furthermore, this produced incorrect pagination URLs. `request.get_host()` returned a `Mock`. It now returns the value `"testserver"`, which is specifically added to `ALLOWED_HOSTS` by the Django test framework. The use of `testserver` as a host name now allows the `HttpRequest.build_absolute_uri()` to work properly without additional mocking. `request.get_full_path()` returned a `Mock` instead of the path with the full query string. `request.is_secure()` returned a (truthy) `Mock` instead of False. `request.body` returned a `str`, but `HttpRequest.body` returns `bytes`. `request.META` was missing required WSGI keys (`SERVER_NAME`, `SERVER_PORT`, `QUERY_STRING`, `PATH_INFO`), so several parts of Django would raise a `KeyError` when the `request` was used.
b74e33a to
e9086f9
Compare
|
@vitalik Please take a look. I believe this is a significant bugfix to Django Ninja's |
| def parse_body(self, request: HttpRequest): | ||
| "just splitting body to lines" | ||
| return request.body.encode().splitlines() | ||
| return request.body.splitlines() |
There was a problem hiding this comment.
request.body should (per runtime behavior and type annotations) return bytes, which it now does.
| "items": [5, 6, 7, 8, 9], | ||
| "next": "http://testlocation/?skip=10", | ||
| "prev": "http://testlocation/?skip=0", | ||
| "next": "http://testserver/items_9?skip=10", | ||
| "prev": "http://testserver/items_9?skip=0", |
There was a problem hiding this comment.
Evidence of the previously broken behavior of the request.build_absolute_uri() method, now fixed.
| "/request/build_absolute_uri/location", | ||
| HTTPStatus.OK, | ||
| "http://testlocation/location", | ||
| "http://testserver/different-location", |
There was a problem hiding this comment.
Strengthen this test case, to ensure passing a parameter actually returns a changed URL.
|
|
||
|
|
||
| def test_client_path_query_string(): | ||
| r = client.get("/test-path?foo=bar") |
There was a problem hiding this comment.
There's a separate code pathway for parsing a query string from the path as a string. Previously, the value of request.path was broken if the query string was passed this way, so a separate test case to query_params is warranted here.
| # so don't bother | ||
| request.path_info = path | ||
|
|
||
| request.META = request_params.pop( |
There was a problem hiding this comment.
See https://wsgi.readthedocs.io/en/latest/definitions.html
Some of these (like SERVER_NAME) must be set or Django will raise KeyError when calling underlying methods.
There was a problem hiding this comment.
Pull request overview
This PR fixes Django Ninja’s TestClient request construction so endpoint code receives a real django.http.HttpRequest with correct host/path/body/META behavior instead of a partially-stubbed Mock, resolving several request-method/attribute inconsistencies that affected pagination URLs and other request-dependent logic.
Changes:
- Replace mocked
HttpRequestconstruction with a realHttpRequest()and populate body, GET, path/path_info, and WSGI-likeMETA. - Update/extend tests to assert correct
build_absolute_uri(),get_host(), andget_full_path()behavior. - Align parser and pagination tests with
HttpRequest.bodybeingbytesand the host beingtestserver.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
ninja/testing/client.py |
Reworks TestClient request building to use a real HttpRequest and provide more realistic request attributes/META. |
tests/test_test_client.py |
Adds/updates tests covering build_absolute_uri, host resolution, and full path/query handling. |
tests/test_parser.py |
Updates parser test to treat request.body as bytes. |
tests/test_pagination.py |
Updates expected pagination links to use testserver and include the resolved path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ninja/testing/client.py
Outdated
| }, | ||
| ) | ||
| request.META.update({ | ||
| f"HTTP_{k.replace('-', '_')}": v |
There was a problem hiding this comment.
When translating headers into request.META, the generated WSGI keys preserve the original header casing (e.g., X-Forwarded-For becomes HTTP_X_Forwarded_For). Django and code in this repo (e.g., throttling) expect uppercase HTTP_... keys like HTTP_X_FORWARDED_FOR, so some headers won’t be visible via request.META.get(...). Consider uppercasing the normalized header name when building the HTTP_ keys.
| f"HTTP_{k.replace('-', '_')}": v | |
| f"HTTP_{k.replace('-', '_').upper()}": v |
There was a problem hiding this comment.
Good point. This was an existing bug, but I'll fix it here.
| request.META = request_params.pop( | ||
| "META", | ||
| { | ||
| "REQUEST_METHOD": request.method, | ||
| "SCRIPT_NAME": "", | ||
| "PATH_INFO": request.path_info, | ||
| "QUERY_STRING": request.GET.urlencode(), | ||
| "SERVER_NAME": "testserver", | ||
| "SERVER_PORT": "80", | ||
| "SERVER_PROTOCOL": "HTTP/1.1", | ||
| "REMOTE_ADDR": "127.0.0.1", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
If a caller passes a custom META dict, it completely replaces the default WSGI-like environment. That can reintroduce the original issue (missing required keys such as SERVER_NAME, SERVER_PORT, PATH_INFO, QUERY_STRING) and lead to KeyError in Django helpers. Consider starting from the default META and updating it with the user-provided META (rather than replacing it).
| request.META = request_params.pop( | |
| "META", | |
| { | |
| "REQUEST_METHOD": request.method, | |
| "SCRIPT_NAME": "", | |
| "PATH_INFO": request.path_info, | |
| "QUERY_STRING": request.GET.urlencode(), | |
| "SERVER_NAME": "testserver", | |
| "SERVER_PORT": "80", | |
| "SERVER_PROTOCOL": "HTTP/1.1", | |
| "REMOTE_ADDR": "127.0.0.1", | |
| }, | |
| ) | |
| default_meta = { | |
| "REQUEST_METHOD": request.method, | |
| "SCRIPT_NAME": "", | |
| "PATH_INFO": request.path_info, | |
| "QUERY_STRING": request.GET.urlencode(), | |
| "SERVER_NAME": "testserver", | |
| "SERVER_PORT": "80", | |
| "SERVER_PROTOCOL": "HTTP/1.1", | |
| "REMOTE_ADDR": "127.0.0.1", | |
| } | |
| user_meta = request_params.pop("META", None) | |
| if isinstance(user_meta, dict): | |
| default_meta.update(user_meta) | |
| request.META = default_meta |
There was a problem hiding this comment.
This is the existing behavior. I don't think it should change.
It seems like this supports a powerful escape hatch for users. If they want to pass META=... (which is not typically done), they take full responsibility for ensuring it's valid. Alternatively, they might want to deliberately create an invalid request.META for some test.
| "REQUEST_METHOD": request.method, | ||
| "SCRIPT_NAME": "", | ||
| "PATH_INFO": request.path_info, | ||
| "QUERY_STRING": request.GET.urlencode(), | ||
| "SERVER_NAME": "testserver", |
There was a problem hiding this comment.
QUERY_STRING is derived from request.GET.urlencode(), which can change the raw query string from the URL (e.g., ?emptyparam becomes emptyparam=). That affects request.get_full_path() and any code that relies on the original query string. Consider preserving the original query_string from path when it is provided, and only using urlencode() when building the query from query_params.
There was a problem hiding this comment.
This an extremely niche case. The user must be setting query strings directly in the path string (not using a query_params dict), the query string must have an empty parameter (like ?emptyparam), and the test needs to be disrupted by the fact that request.META.QUERY_STRING would be emptyparam= instead of emptyparam.
However, sure, I'll fix it to preserve the string exactly. Maybe someone downstream would want to test very specific things with query strings.
|
I've resolved some Mypy errors that were pre-existing, but which the change from @vitalik Could you please re-trigger CI and review this? |
Endpoints called via
TestClientreceived arequestwhere anyHttpRequestmethod not explicitly stubbed returned aMockobject instead of a real value.This was causing all the following bugs, which are now resolved:
request.build_absolute_uri()was replaced with a stub that ignored the request path entirely, returning"http://testlocation/"for every endpoint. Furthermore, this produced incorrect pagination URLs.request.get_host()returned aMock. It now returns the value"testserver", which is specifically added toALLOWED_HOSTSby the Django test framework. The use oftestserveras a host name now allows theHttpRequest.build_absolute_uri()to work properly without additional mocking.request.get_full_path()returned aMockinstead of the path with the full query string.request.is_secure()returned a (truthy)Mockinstead of False.request.bodyreturned astr, butHttpRequest.bodyreturnsbytes.request.METAwas missing required WSGI keys (SERVER_NAME,SERVER_PORT,QUERY_STRING,PATH_INFO), so several parts of Django would raise aKeyErrorwhen therequestwas used.