Django

Code

Ticket #8278 (closed: fixed)

Opened 4 months ago

Last modified 3 months ago

QueryDict.update eats data when other_dict is a MultiValueDict

Reported by: jdunck Assigned to: nobody
Milestone: 1.0 Component: Core framework
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

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

8278.querydict.diff (2.3 kB) - added by julien on 08/13/08 00:33:20.
Patch and regression tests
8278.querydict.2.diff (1.7 kB) - added by julien on 08/13/08 02:08:41.
2nd attempt. All tests pass.

Change History

08/13/08 00:33:20 changed by julien

  • attachment 8278.querydict.diff added.

Patch and regression tests

08/13/08 00:41:24 changed by julien

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.

08/13/08 00:43:04 changed by julien

  • has_patch set to 1.

Sorry, I meant: MultiValueDict.update() is not prepared...

(follow-up: ↓ 4 ) 08/13/08 01:36:48 changed by SmileyChris

  • needs_better_patch set to 1.

From a quick scan of your patch:

1. Your tests don't actually check for the regression of updating a QueryDict with a normal dict (which leads to...)

2. It looks like this will break for updating normal dicts

3. It'd be better to use QueryDict('...', mutable=True) rather than messing around with internal variables in the test

(in reply to: ↑ 3 ) 08/13/08 01:38:23 changed by julien

Replying to SmileyChris:

From a quick scan of your patch: 1. Your tests don't actually check for the regression of updating a QueryDict with a normal dict (which leads to...) 2. It looks like this will break for updating normal dicts 3. 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.

08/13/08 02:02:24 changed by julien

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.

08/13/08 02:08:41 changed by julien

  • attachment 8278.querydict.2.diff added.

2nd attempt. All tests pass.

08/13/08 12:29:07 changed by mtredinnick

  • stage changed from Unreviewed to Accepted.

08/29/08 11:49:21 changed by jacob

  • status changed from new to closed.
  • resolution set to fixed.

(In [8705]) Fixed #8278: fixed QueryDict.update(QueryDict). Thanks, julien.


Add/Change #8278 (QueryDict.update eats data when other_dict is a MultiValueDict)




Change Properties
Action