Opened 2 years ago

Closed 2 years ago

Last modified 23 months ago

#34063 closed Bug (fixed)

request.POST not populated for multipart/form-data via AsyncClient

Reported by: Timo Ludwig Owned by: Scott Halgrim
Component: Testing framework Version: 4.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 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.

Change History (12)

comment:1 by Carlton Gibson, 2 years ago

Triage Stage: UnreviewedAccepted

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.)

comment:2 by Leo Tom, 2 years ago

Owner: changed from nobody to Leo Tom
Status: newassigned

comment:3 by Leo Tom, 2 years ago

Owner: Leo Tom removed
Status: assignednew

comment:4 by Kevan Swanberg, 2 years ago

Owner: set to Kevan Swanberg
Status: newassigned

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.

comment:5 by Scott Halgrim, 2 years ago

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

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?

By contrast, TestClient (i.e., not AsyncTestClient) ends up calling read on LimitedStream, not FakePayload, which has this if clause in _read_limited, which is https://github.com/django/django/blob/5c2c7277d4554db34c585477b269bb1acfcbbe56/django/core/handlers/wsgi.py#L24-L25 here

Last edited 2 years ago by Scott Halgrim (previous) (diff)

comment:6 by Kevan Swanberg, 2 years ago

Owner: changed from Kevan Swanberg to Scott Halgrim

comment:7 by Carlton Gibson, 2 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

Pull request ensuring async client and request factory allow large read() values beyond the request body size.

comment:8 by Carlton Gibson <carlton@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In c4eaa67:

Fixed #34063 -- Fixed reading request body with async request factory and client.

Co-authored-by: Kevan Swanberg <kevswanberg@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:9 by Florian Apolloner, 23 months ago

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.

comment:10 by Carlton Gibson, 23 months ago

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...)

comment:11 by Florian Apolloner, 23 months ago

So essentially you are saying that using LimitedStream makes the faked body behave more like a file. Makes sense, thank you for the clarification.

comment:12 by Adam Johnson, 23 months ago

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

Note: See TracTickets for help on using tickets.
Back to Top