#8278 closed (fixed)
QueryDict.update eats data when other_dict is a MultiValueDict
Reported by: | Jeremy Dunck | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
django.http.QueryDict supports .update, but it seems to assume that other_dict is a (builtin) dict.
This is dangerous when mixing two QueryDicts.
>>> x = QueryDict('a=1&a=2') >>> y = QueryDict('a=3&a=4') >>> print x <QueryDict: {u'a': [u'1', u'2']}> >>> print y <QueryDict: {u'a': [u'3', u'4']}> >>> x._mutable = True >>> x.update(y) >>> x._mutable=False >>> print x <QueryDict: {u'a': [u'1', u'2', u'4']}>
Note that a=3 was lost.
As far as I can see, there's no really nice fix to this. QueryDict.update doesn't just defer to MultiValueDict because it needs to force keys and values into unicode.
Defering to MultiValueDict first and then forcing means duplicate keys for items not yet coerced.
Forcing individual items results in lists from QueryDict becoming (single) items in MultiValueDict.
i.e.
dict([(f(k), f(v)) for k, v in other_dict.items()]) -> dict([(f(k), [f(v) for v in values]) for k, values in other_dict.lists()])
This seems to be a leaky abstraction here; I think QueryDict is not really a MultiValueDict, since MultiValueDict can have any (hashable) objects as keys and values, while QueryDict insists on unicode objects only.
No patch from me since the solution isn't obvious to me. Marking as 1.0 since this is a bug, though.
Attachments (2)
Change History (10)
by , 16 years ago
Attachment: | 8278.querydict.diff added |
---|
comment:1 by , 16 years ago
I think there is a combination of two issues here:
dict
eats duplicate keys:
>>> dict([(u'a', u'1'), (u'a', u'2')]) dict: {u'a': u'2'}
- And
MultiValueDict
is not prepared to receive values as lists.
Hopefully the attached patch fixes that.
comment:2 by , 16 years ago
Has patch: | set |
---|
Sorry, I meant: MultiValueDict.update()
is not prepared...
follow-up: 4 comment:3 by , 16 years ago
Patch needs improvement: | set |
---|
From a quick scan of your patch:
- Your tests don't actually check for the regression of updating a
QueryDict
with a normaldict
(which leads to...)
- It looks like this will break for updating normal
dict
s
- It'd be better to use
QueryDict('...', mutable=True)
rather than messing around with internal variables in the test
comment:4 by , 16 years ago
Replying to SmileyChris:
From a quick scan of your patch:
- Your tests don't actually check for the regression of updating a
QueryDict
with a normaldict
(which leads to...)
- It looks like this will break for updating normal
dict
s
- It'd be better to use
QueryDict('...', mutable=True)
rather than messing around with internal variables in the test
I agree, I realised the patch was wrong shortly after I posted. Trying to give it another go.
comment:5 by , 16 years ago
Ok, I've amended the path and now all tests pass. The trick I used was to iterate through the list of values and updated value by value. Again, I'm not sure if that's the best approach, but at least it seems to work.
comment:6 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:7 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Patch and regression tests