[patch] QueryDict items() and mutability protection fix with doctests

[django@…]

A few issues with httpwrappers.QueryDict. The docs say:

  items() -- Just like the standard dictionary items() method, except
  this retains the order for values of duplicate keys, if any. For
  example, if the original query string was "a=1&b=2&b=3", items()
  will return [("a", ["1"]), ("b", ["2", "3"])], where the order of
  ["2", "3"] is guaranteed, but the order of a vs. b isn't.

items() actually returns [("a", "1"), ("b", "2")], rather than lists. QueryDict.getitem() returns the last item in the list so it would seem most consistent to me for items() to return the last item in the list too (as does the superclass MultiValueDict), but instead the existing implementation is returning the first item in the list.

Additionally, existing non-mutability protection is a bit weak, allowing mutation via some methods (appendlist) and not others.

This patch makes the behaviour of QueryDict consistent with the documentation and adds mutability-checks where appropriate. I have replaced MultiValueDict with a dict subclass which simplifies the implementation and also provides useful things like setdefault and iteration over keys. I have added a lists() method to MultiValueDict that returns lists for each key and my QueryDict.items() delegates to this to achieve the documented behaviour, though I would prefer that QueryDict was consistent with its superclass here - is there a reason to have it otherwise?

I have provided doctest coverage so someone can check that my expectation of the behaviour fits with Django's. I have beed running this patch without problems.

[django@…]

[patch] QueryDict fix + doctests

[django@…]

Minor supplementary fix for meta/init.py, which had been retrieving internals from the old MultiValueDict

[django@…]

Updated patch for r1323. Further fix for admin app that is currently depending on existing (?incorrect) behaviour of QueryDict rather than documented behaviour. Note that this patch also fixes #709.

[django@…]

I have updated this patch against trunk r1323. There is still an inconsistency between the documented and actual behaviour of QueryDict. This patch matches the documented behaviour but better, I think, would be if QueryDict.items() behaved exactly as the underlying MultiValueDict. If that were the case, the patch to django/contrib/admin/views/main.py would be unnecessary.

Note that this patch also addresses #709.

[Adrian Holovaty]

(In [1502]) Small importing change in django.utils.httpwrappers. Refs #736.

[Adrian Holovaty]

(In [1504]) Fixed #736 -- Changed behavior of QueryDict items() to be more consistent, fixed mutability holes, gave MultiValueDict many more dictionary methods and added unit tests. Thanks, Kieran Holland. This is slightly backwards-incompatible if you happened to rely on the behavior of QueryDict.items(), which is highly unlikely.

[django@…]

Hi Adrian,

Thanks for picking up the bug in MultiValueDict.setdefault().

A few queries:

1. You slightly altered MultiValueDict.get() - are you sure you want to return

None if the key doesn't exist? Shouldn't it raise a KeyError?

2. Why the change in QueryDict.init() from:

for key, value in parse_qsl(query_string, True): # keep_blank_values=True

to:

for key, value in parse_qsl((query_string or ''), True): # keep_blank_values=True

3. In django.views.main this:

self.params = dict([(k, v) for k, v in request.GET.items()])

can be simplified to:

self.params = dict(request.GET.items())

[Adrian Holovaty]

Hey Kieran,

1. MultiValueDict.get() follows the behavior of standard Python dictionaries:

   >>> a = {'firstname': 'adrian'}
   >>> print a.get('firstname')
   adrian
   >>> print a.get('foo')
   None
   

2. The parse_qsl((query_string or ''), True) change was necessary because, for some reason (possibly [1503]), when I made this change live on my mod_python server, it was complaining that parse_sql was being given None as its first parameter. This was a quick fix. It would be great if you could figure out exactly what the problem was there, and if there's a more elegant way of handling it.

3. Done in [1507]. Thanks.

[Aymeric Augustin]

In [17464]:

(The changeset message doesn't reference this ticket)