-
-
Notifications
You must be signed in to change notification settings - Fork 565
Fix TestClient passing broken request objects to endpoints
#1680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
e9086f9
ca31832
525a2bc
67963ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,25 +2,15 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from json import loads as json_loads | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any, Callable, Dict, List, Optional, Tuple, Union | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from unittest.mock import Mock | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from urllib.parse import urljoin | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from django.http import QueryDict, StreamingHttpResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from django.http.request import HttpHeaders, HttpRequest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from django.http.request import HttpRequest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from ninja import NinjaAPI, Router | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from ninja.responses import NinjaJSONEncoder | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from ninja.responses import Response as HttpResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def build_absolute_uri(location: Optional[str] = None) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base = "http://testlocation/" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if location: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base = urljoin(base, location) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return base | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: this should be changed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # maybe add here urlconf object and add urls from here | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class NinjaClientBase: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -117,8 +107,8 @@ def urls(self) -> List: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self._urls_cache | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _resolve( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, method: str, path: str, data: Dict, request_params: Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Tuple[Callable, Mock, Dict]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, method: str, path: str, data: Dict, request_params: Dict[str, Any] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Tuple[Callable, HttpRequest, Dict]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| url_path = path.split("?")[0].lstrip("/") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for url in self.urls: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match = url.resolve(url_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -128,74 +118,83 @@ def _resolve( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise Exception(f'Cannot resolve "{path}"') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _build_request( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, method: str, path: str, data: Dict, request_params: Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Mock: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request = Mock(spec=HttpRequest) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, method: str, path: str, data: Dict, request_params: Dict[str, Any] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> HttpRequest: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request = HttpRequest() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.method = method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.path = path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.body = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.COOKIES = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body = request_params.pop("body", b"") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request._body = body.encode() if isinstance(body, str) else body | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request._dont_enforce_csrf_checks = True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.is_secure.return_value = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.build_absolute_uri = build_absolute_uri | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.auth = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.user = Mock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "user" not in request_params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.user = Mock() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.user.is_authenticated = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.user.is_staff = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.user.is_superuser = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.META = request_params.pop("META", {"REMOTE_ADDR": "127.0.0.1"}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.FILES = request_params.pop("FILES", {}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.META.update({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"HTTP_{k.replace('-', '_')}": v | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for k, v in request_params.pop("headers", {}).items() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.headers = HttpHeaders(request.META) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| files = request_params.pop("FILES", None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if files is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.FILES = files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(data, QueryDict): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.POST = data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.POST = QueryDict(mutable=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(data, (str, bytes)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request_params["body"] = data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for k, v in data.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.POST[k] = v | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif isinstance(data, (str, bytes)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request._body = data.encode() if isinstance(data, str) else data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif data: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for k, v in data.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.POST[k] = v | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| query_params = request_params.pop("query_params", None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "?" in path: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET = QueryDict(path.split("?")[1]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| query_params = request_params.pop("query_params", None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if query_params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| query_dict = QueryDict(mutable=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for k, v in query_params.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(v, list): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for item in v: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| query_dict.appendlist(k, item) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| query_dict[k] = v | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET = query_dict | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET = QueryDict() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path, query_string = path.split("?", maxsplit=1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET = QueryDict(query_string) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif query_params is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for k, v in query_params.items(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(v, list): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for item in v: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET.appendlist(k, item) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.GET[k] = v | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.path = path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If "settings.FORCE_SCRIPT_NAME" is set, "request.path_info" ought | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # to respect it, but this class skips the Django URL resolver, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # so don't bother | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| request.path_info = path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+173
to
+177
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "SERVER_PORT": "80", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "SERVER_PROTOCOL": "HTTP/1.1", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "REMOTE_ADDR": "127.0.0.1", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+170
to
+182
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
Copilot
AI
Mar 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. This was an existing bug, but I'll fix it here.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -507,8 +507,8 @@ def test_case9(): | |
| response = client.get("/items_9?skip=5").json() | ||
| assert response == { | ||
| "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", | ||
|
Comment on lines
509
to
+511
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Evidence of the previously broken behavior of the |
||
| } | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ class MyParser(Parser): | |
|
|
||
| def parse_body(self, request: HttpRequest): | ||
| "just splitting body to lines" | ||
| return request.body.encode().splitlines() | ||
| return request.body.splitlines() | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| def parse_querydict( | ||
| self, data: QueryDict, list_fields: List[str], request: HttpRequest | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ def request_build_absolute_uri(request): | |
|
|
||
| @router.get("/request/build_absolute_uri/location") | ||
| def request_build_absolute_uri_location(request): | ||
| return request.build_absolute_uri("location") | ||
| return request.build_absolute_uri("/different-location") | ||
|
|
||
|
|
||
| @router.get("/test") | ||
|
|
@@ -37,17 +37,36 @@ def get_cookies(request): | |
| return dict(request.COOKIES) | ||
|
|
||
|
|
||
| @router.get("/test-host") | ||
| def get_host(request): | ||
| return { | ||
| "host": request.get_host(), | ||
| } | ||
|
|
||
|
|
||
| @router.get("/test-path") | ||
| def get_path(request): | ||
| return { | ||
| "path": request.path, | ||
| "full_path": request.get_full_path(), | ||
| } | ||
|
|
||
|
|
||
| client = TestClient(router) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "path,expected_status,expected_response", | ||
| [ | ||
| ("/request/build_absolute_uri", HTTPStatus.OK, "http://testlocation/"), | ||
| ( | ||
| "/request/build_absolute_uri", | ||
| HTTPStatus.OK, | ||
| "http://testserver/request/build_absolute_uri", | ||
| ), | ||
| ( | ||
| "/request/build_absolute_uri/location", | ||
| HTTPStatus.OK, | ||
| "http://testlocation/location", | ||
| "http://testserver/different-location", | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strengthen this test case, to ensure passing a parameter actually returns a changed URL. |
||
| ), | ||
| ], | ||
| ) | ||
|
|
@@ -124,3 +143,20 @@ def test_headered_client_request_with_default_cookies(): | |
| def test_headered_client_request_with_overwritten_and_additional_cookies(): | ||
| r = cookied_client.get("/test-cookies", COOKIES={"A": "na", "C": "nc"}) | ||
| assert r.json() == {"A": "na", "B": "b", "C": "nc"} | ||
|
|
||
|
|
||
| def test_client_host(): | ||
| r = client.get("/test-host") | ||
| assert r.json() == { | ||
| "host": "testserver", | ||
| } | ||
|
|
||
|
|
||
| def test_client_path_query_params(): | ||
| r = client.get("/test-path", query_params={"foo": "bar"}) | ||
| assert r.json() == {"path": "/test-path", "full_path": "/test-path?foo=bar"} | ||
|
|
||
|
|
||
| def test_client_path_query_string(): | ||
| r = client.get("/test-path?foo=bar") | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a separate code pathway for parsing a query string from the path as a string. Previously, the value of |
||
| assert r.json() == {"path": "/test-path", "full_path": "/test-path?foo=bar"} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://wsgi.readthedocs.io/en/latest/definitions.html
Some of these (like
SERVER_NAME) must be set or Django will raiseKeyErrorwhen calling underlying methods.