[Django] #34063: request.POST not populated for multipart/form-data via AsyncClient
Django
---------------------------------------------+------------------------
Reporter: Timo Ludwig | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
In test cases with the `AsyncClient` and requests with the default content
type `"multipart/form-data"`, I cannot test views which access
`request.POST` before accessing `request.body`.
I'm very sorry if this is a duplicate of #32189 and this is the expected
behavior, but the way I understood it is that `request.POST` should be
populated for all form requests, so both `application/x-www-form-
urlencoded` and `multipart/form-data`, right?
If I just copy the
[https://github.com/django/django/blob/9b0c9821ed4dd9920cc7c5e7b657720d91a89bdc/tests/test_client/tests.py#L132-L140
test_post] case from the sync `ClientTest` (and adapt it to the async
structure as follows):
{{{
diff --git a/tests/test_client/tests.py b/tests/test_client/tests.py
index 57dc22ea0c..8cebaae9e7 100644
--- a/tests/test_client/tests.py
+++ b/tests/test_client/tests.py
@@ -1103,6 +1103,16 @@ class AsyncClientTest(TestCase):
response = await self.async_client.get("/get_view/", {"var":
"val"})
self.assertContains(response, "This is a test. val is the
value.")
+ async def test_post(self):
+ "POST some data to a view"
+ post_data = {"value": 37}
+ response = await self.async_client.post("/post_view/", post_data)
+
+ # Check some response details
+ self.assertContains(response, "Data received")
+ self.assertEqual(response.context["data"], "37")
+ self.assertEqual(response.templates[0].name, "POST Template")
+
@override_settings(ROOT_URLCONF="test_client.urls")
class AsyncRequestFactoryTest(SimpleTestCase):
}}}
the test case fails with:
{{{
FAIL: test_post (test_client.tests.AsyncClientTest)
POST some data to a view
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python3.10/unittest/case.py", line 59, in
testPartExecutor
yield
File "/usr/lib/python3.10/unittest/case.py", line 591, in run
self._callTestMethod(testMethod)
File "/usr/lib/python3.10/unittest/case.py", line 549, in
_callTestMethod
method()
File "/home/user/.local/lib/python3.10/site-packages/asgiref/sync.py",
line 218, in __call__
return call_result.result()
File "/usr/lib/python3.10/concurrent/futures/_base.py", line 451, in
result
return self.__get_result()
File "/usr/lib/python3.10/concurrent/futures/_base.py", line 403, in
__get_result
raise self._exception
File "/home/user/.local/lib/python3.10/site-packages/asgiref/sync.py",
line 284, in main_wrap
result = await self.awaitable(*args, **kwargs)
File "/home/django/tests/test_client/tests.py", line 1109, in test_post
response = await self.async_client.post("/post_view/", post_data)
File "/home/user/django/django/test/client.py", line 1072, in request
self.check_exception(response)
File "/home/user/django/django/test/client.py", line 666, in
check_exception
raise exc_value
File "/home/user/.local/lib/python3.10/site-packages/asgiref/sync.py",
line 472, in thread_handler
raise exc_info[1]
File "/home/user/django/django/core/handlers/exception.py", line 42, in
inner
response = await get_response(request)
File "/home/user/django/django/core/handlers/base.py", line 253, in
_get_response_async
response = await wrapped_callback(
File "/home/user/.local/lib/python3.10/site-packages/asgiref/sync.py",
line 435, in __call__
ret = await asyncio.wait_for(future, timeout=None)
File "/usr/lib/python3.10/asyncio/tasks.py", line 408, in wait_for
return await fut
File "/home/user/.local/lib/python3.10/site-
packages/asgiref/current_thread_executor.py", line 22, in run
result = self.fn(*self.args, **self.kwargs)
File "/home/user/.local/lib/python3.10/site-packages/asgiref/sync.py",
line 476, in thread_handler
return func(*args, **kwargs)
File "/home/django/tests/test_client/views.py", line 83, in post_view
if request.POST:
File "/home/user/django/django/core/handlers/asgi.py", line 113, in
_get_post
self._load_post_and_files()
File "/home/user/django/django/http/request.py", line 386, in
_load_post_and_files
self._post, self._files = self.parse_file_upload(self.META, data)
File "/home/user/django/django/http/request.py", line 334, in
parse_file_upload
return parser.parse()
File "/home/user/django/django/http/multipartparser.py", line 165, in
parse
for item_type, meta_data, field_stream in Parser(stream,
self._boundary):
File "/home/user/django/django/http/multipartparser.py", line 703, in
__iter__
for sub_stream in boundarystream:
File "/home/user/django/django/http/multipartparser.py", line 533, in
__next__
return LazyStream(BoundaryIter(self._stream, self._boundary))
File "/home/user/django/django/http/multipartparser.py", line 560, in
__init__
unused_char = self._stream.read(1)
File "/home/user/django/django/http/multipartparser.py", line 427, in
read
return b"".join(parts())
File "/home/user/django/django/http/multipartparser.py", line 418, in
parts
chunk = next(self)
File "/home/user/django/django/http/multipartparser.py", line 440, in
__next__
output = next(self._producer)
File "/home/user/django/django/http/multipartparser.py", line 507, in
__next__
data = self.flo.read(self.chunk_size)
File "/home/user/django/django/http/request.py", line 421, in read
return self._stream.read(*args, **kwargs)
File "/home/user/django/django/test/client.py", line 82, in read
assert (
AssertionError: Cannot read more than the available bytes from the HTTP
incoming data.
}}}
While the same test case (and the same content type) succeeds for the sync
`Client`.
I would be willing to provide a patch if someone could point me in the
right direction.
--
Ticket URL: <https://code.djangoproject.com/ticket/34063>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Timo Ludwig | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
OK, yes, good test. That should work.
> I would be willing to provide a patch if someone could point me in the
right direction.
I need to have a dig around to say, but yes, thanks!
(AsyncClient was added in fc0fa72ff4cdbf5861a366e31cb8bbacd44da22d for
#31224.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:1>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* owner: nobody => Leo Tom
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:2>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by Leo Tom):
* owner: Leo Tom => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:3>
Django
Reporter: Timo Ludwig | Owner: Kevan
| Swanberg
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Kevan Swanberg):
* owner: (none) => Kevan Swanberg
* status: new => assigned
Comment:
This seems like something funky going on with FakePayload, where the test
client expects it to behave a little differently than it does. Patch to
follow.
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Kevan
| Swanberg
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Scott Halgrim):
I've been researching this ticket for a few hours at DjangoCon with
Carlton. He suggested (and I agree), that now would be a good time to
summarize what we've learned. This is not a full understanding of the
issue, but merely a status report, so to speak.
We've found we're able to reduce the surface area, so to speak, by adding
this smaller test to `test_fakepayload.py`, which also errors out in the
same way
{{{#!python
def test_read_small_file(self):
payload = FakePayload(b'--BoUnDaRyStRiNg\r\nContent-Disposition: form-
data; name="value"\r\n\r\n37\r\n--BoUnDaRyStRiNg--\r\n')
payload.read(65536)
}}}
This is basically what's happening in the example test `test_post`
provided above. The `FakePayload` object has its `read` method called by
`ChunkIter` in `MultiPartParser` with a value of 65_536.
So maybe the question now is, should `FakePayload` handle this in a
different way, or `MultiPartParser` and `ChunkIter` not be sending in a
number larger than the length of the body?
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:5>
Django
-------------------------------------+-------------------------------------
| Halgrim
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Kevan Swanberg => Scott Halgrim
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Type: Bug | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/16210 Pull request] ensuring async
client and request factory allow large `read()` values beyond the request
body size.
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Component: Testing framework | Version: 4.0
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c4eaa67e2b880db778c9fe6d9854fbdfcc16ecd2" c4eaa67]:
{{{
#!CommitTicketReference repository=""
revision="c4eaa67e2b880db778c9fe6d9854fbdfcc16ecd2"
Fixed #34063 -- Fixed reading request body with async request factory and
client.
Co-authored-by: Kevan Swanberg <kevsw...@gmail.com>
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Type: Bug | Status: closed
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
I stumbled over this in IRC. Now `WSGIRequest` wraps `wsgi.input` in a
`LimitedStream`:
https://github.com/django/django/blob/c179ad9fe7e82dcb80261aa016f2fe18c8fcc181/django/core/handlers/wsgi.py#L91
`ASGIRequest` does not:
https://github.com/django/django/blob/c179ad9fe7e82dcb80261aa016f2fe18c8fcc181/django/core/handlers/asgi.py#L101
This makes me wonder if it was considered to fix this in `ASGIRequest`
instead of the test client, even if it doesn't seem (?) to occur normally.
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Type: Bug | Status: closed
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Florian.
I did think about it but it doesn't really apply in the ASGI case... For
WSGI we use LimitedStream to stop requests reading beyond their limits,
but each request under ASGI has its own body file — reading beyond that's
not something that can occur.
The error here is an artefact of the testing setup, which sets a
`FakePayload`, which fails loudly for an out of bounds read (rather than
just giving you want it's got) for reasons of its own (which stem from the
dawn of time). (The first pass at DjangoCon was ''"Why doesn't
`FakePayload` behave better?"', but there are tests depending on it doing
what it's doing...)
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Type: Bug | Status: closed
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
So essentially you are saying that using `LimitedStream` makes the faked
body behave more like a file. Makes sense, thank you for the
clarification.
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Timo Ludwig | Owner: Scott
| Halgrim
Type: Bug | Status: closed
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
For anyone encountering this issue on older Django versions, I have
created a repository demonstrating how to backport the fix into your
project: https://github.com/adamchainz/django-ticket-34063-backport
--
Ticket URL: <https://code.djangoproject.com/ticket/34063#comment:12>