Opened 5 years ago

Last modified 3 days ago

#17235 new Bug

Multipartparser shouldn't leave request.POST/request.FILES mutable

Reported by: Florian Apolloner Owned by:
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: hirokiky@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently the multipart parser constructs QueryDicts for POST and FILES as mutable. Since we discourage users to change QueryDicts (and don't allow it for GET and normal POST) the parser should change the flag to False before returning it. This way multipart POSTs would be more consistent with normal POSTs which aren't mutable.

Attachments (1)

t17235.patch (3.7 KB) - added by Hiroki Kiyohara 4 years ago.
Improved requests POST and FILES to be handled as immutable.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by Luke Plant

Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by anonymous

FILES is constructed by MultiValueDict, not QueryDict. MultiValueDict cannot be handle as mutable.
So, It seems some new class (for example, ImmutableMultiValueDict) will be needed.

comment:3 Changed 4 years ago by Hiroki Kiyohara

Cc: hirokiky@… added

Changed 4 years ago by Hiroki Kiyohara

Attachment: t17235.patch added

Improved requests POST and FILES to be handled as immutable.

comment:4 Changed 4 years ago by Hiroki Kiyohara

I added a patch.

But the tests raised 4 errors.
For example...

ERROR: testEditSaveAs (regressiontests.admin_views.tests.AdminViewBasicTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/hirokiky/django/kydjango/tests/regressiontests/admin_views/tests.py", line 225, in testEditSaveAs
    response = self.client.post('/test_admin/%s/admin_views/section/1/' % self.urlbit, post_data)
  File "/home/hirokiky/django/kydjango/tests/django/test/client.py", line 463, in post
    response = super(Client, self).post(path, data=data, content_type=content_type, **extra)
  File "/home/hirokiky/django/kydjango/tests/django/test/client.py", line 297, in post
    return self.request(**r)
  File "/home/hirokiky/django/kydjango/tests/django/test/client.py", line 424, in request
    six.reraise(*exc_info)
  File "/home/hirokiky/django/kydjango/tests/django/utils/six.py", line 313, in reraise
    raise value
  File "/home/hirokiky/django/kydjango/tests/django/core/handlers/base.py", line 115, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/contrib/admin/options.py", line 371, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 91, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/views/decorators/cache.py", line 52, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/contrib/admin/sites.py", line 202, in inner
    return view(request, *args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 25, in _wrapper
    return bound_func(*args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 91, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 21, in bound_func
    return func(self, *args2, **kwargs2)
  File "/home/hirokiky/django/kydjango/tests/django/db/transaction.py", line 208, in inner
    return func(*args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/contrib/admin/options.py", line 1064, in change_view
    current_app=self.admin_site.name))
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 25, in _wrapper
    return bound_func(*args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 91, in _wrapped_view
    response = view_func(request, *args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/utils/decorators.py", line 21, in bound_func
    return func(self, *args2, **kwargs2)
  File "/home/hirokiky/django/kydjango/tests/django/db/transaction.py", line 208, in inner
    return func(*args, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/contrib/admin/options.py", line 989, in add_view
    prefix=prefix, queryset=inline.queryset(request))
  File "/home/hirokiky/django/kydjango/tests/django/forms/models.py", line 717, in __init__
    queryset=qs, **kwargs)
  File "/home/hirokiky/django/kydjango/tests/django/forms/models.py", line 441, in __init__
    super(BaseModelFormSet, self).__init__(**defaults)
  File "/home/hirokiky/django/kydjango/tests/django/forms/formsets.py", line 53, in __init__
    self._construct_forms()
  File "/home/hirokiky/django/kydjango/tests/django/forms/formsets.py", line 122, in _construct_forms
    self.forms.append(self._construct_form(i))
  File "/home/hirokiky/django/kydjango/tests/django/forms/models.py", line 730, in _construct_form
    form.data[form.add_prefix(self._pk_field.name)] = None
  File "/home/hirokiky/django/kydjango/tests/django/http/request.py", line 304, in __setitem__
    self._assert_mutable()
  File "/home/hirokiky/django/kydjango/tests/django/http/request.py", line 301, in _assert_mutable
    raise AttributeError("This QueryDict instance is immutable")
AttributeError: This QueryDict instance is immutable

like this.

One idea solving this is putting a line on a setting to avoid constructing request as immutable.
And next step is solving these errors to handle request as immutable clearly.
Finally, delete the avoiding setting.

Any ideas?

comment:5 Changed 4 years ago by Hiroki Kiyohara

Has patch: set
Needs documentation: set
Needs tests: set
Owner: changed from nobody to Hiroki Kiyohara
Patch needs improvement: set
Status: newassigned

comment:6 Changed 3 years ago by Claude Paroz

#18553 was marked as a duplicate

comment:7 Changed 2 years ago by Tom Christie

Nothing concrete to help progress the ticket at this point, but chiming in that I do agree that having request.POST and request.FILES as immutable data structures would be an improvement.

comment:8 Changed 3 weeks ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Owner: Hiroki Kiyohara deleted
Patch needs improvement: unset
Status: assignednew

comment:9 Changed 3 days ago by Tim Graham <timograham@…>

In 4a246a0:

Refs #17235 -- Made MultiPartParser leave request.POST immutable.

comment:10 Changed 3 days ago by Tim Graham

Has patch: unset
Summary: Multipartparser shouldn't leave the QueryDict mutableMultipartparser shouldn't leave request.POST/request.FILES mutable

The ticket remains open to address request.FILES.

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