Opened 18 years ago

Closed 18 years ago

Last modified 12 years ago

#736 closed defect (fixed)

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

Reported by: django@… Owned by: Adrian Holovaty
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (3)

mvd.patch (13.5 KB ) - added by django@… 18 years ago.
[patch] QueryDict fix + doctests
mvd_admin_fix.patch (688 bytes ) - added by django@… 18 years ago.
Minor supplementary fix for meta/init.py, which had been retrieving internals from the old MultiValueDict
mvd_patch_r1323.diff (14.7 KB ) - added by django@… 18 years ago.
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.

Download all attachments as: .zip

Change History (9)

by django@…, 18 years ago

Attachment: mvd.patch added

[patch] QueryDict fix + doctests

by django@…, 18 years ago

Attachment: mvd_admin_fix.patch added

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

by django@…, 18 years ago

Attachment: mvd_patch_r1323.diff added

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.

comment:1 by django@…, 18 years ago

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.

comment:2 by Adrian Holovaty, 18 years ago

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

comment:3 by Adrian Holovaty, 18 years ago

Resolution: fixed
Status: newclosed

(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.

comment:4 by django@…, 18 years ago

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?

  1. 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
  1. 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())

comment:5 by Adrian Holovaty, 18 years ago

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
    
  1. 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.
  1. Done in [1507]. Thanks.

comment:6 by Aymeric Augustin, 12 years ago

In [17464]:

(The changeset message doesn't reference this ticket)

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