Code

Opened 8 years ago

Closed 8 years ago

Last modified 2 years ago

#736 closed defect (fixed)

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

Reported by: django@… Owned by: adrian
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: UI/UX:

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@… 8 years ago.
[patch] QueryDict fix + doctests
mvd_admin_fix.patch (688 bytes) - added by django@… 8 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@… 8 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)

Changed 8 years ago by django@…

[patch] QueryDict fix + doctests

Changed 8 years ago by django@…

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

Changed 8 years ago by 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.

comment:1 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by adrian

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

comment:3 Changed 8 years ago by adrian

  • Resolution set to fixed
  • Status changed from new to closed

(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 Changed 8 years ago by 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?

  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 Changed 8 years ago by adrian

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 Changed 2 years ago by aaugustin

In [17464]:

(The changeset message doesn't reference this ticket)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.