Opened 14 years ago
Last modified 8 months ago
#17235 assigned Bug
Multipartparser shouldn't leave request.POST/request.FILES mutable
| Reported by: | Florian Apolloner | Owned by: | bcail |
|---|---|---|---|
| Component: | HTTP handling | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | hirokiky@…, bcail | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| 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)
Change History (18)
comment:1 by , 14 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 13 years ago
comment:3 by , 13 years ago
| Cc: | added |
|---|
by , 13 years ago
| Attachment: | t17235.patch added |
|---|
Improved requests POST and FILES to be handled as immutable.
comment:4 by , 13 years ago
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 by , 13 years ago
| Has patch: | set |
|---|---|
| Needs documentation: | set |
| Needs tests: | set |
| Owner: | changed from to |
| Patch needs improvement: | set |
| Status: | new → assigned |
comment:7 by , 11 years ago
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 by , 9 years ago
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
| Owner: | removed |
| Patch needs improvement: | unset |
| Status: | assigned → new |
comment:10 by , 9 years ago
| Has patch: | unset |
|---|---|
| Summary: | Multipartparser shouldn't leave the QueryDict mutable → Multipartparser shouldn't leave request.POST/request.FILES mutable |
The ticket remains open to address request.FILES.
comment:12 by , 5 years ago
| Has patch: | set |
|---|
comment:14 by , 20 months ago
| Cc: | added |
|---|---|
| Owner: | changed from to |
| Patch needs improvement: | unset |
I opened a new PR, based on the previous one. I updated the code to make MultiValueDict able to be mutable or immutable, like QueryDict. Is that a good direction to go?
comment:15 by , 19 months ago
| Patch needs improvement: | set |
|---|
Is that a good direction to go?
I think it looks good to me as a direction, added some comments to the PR.
comment:16 by , 8 months ago
| Owner: | changed from to |
|---|
Hi there, I'll give this a try as per https://github.com/django/django/pull/17991#issuecomment-2107642203. Let me know if you'd prefer to continue working on it yourself!
comment:17 by , 8 months ago
| Owner: | changed from to |
|---|
Apologies, in my haste i missed reading this comment on the PR. Since I'm still a beginner, I'll try to pick an easier issue to contribute to. Sorry for the noise.
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.