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.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@1504 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/views/main.py

@@ -100,13 +100,12 @@ class ChangeList(object):
     def get_search_parameters(self, request):
         # Get search parameters from the query string.
         try:
-            self.req_get = request.GET
             self.page_num = int(request.GET.get(PAGE_VAR, 0))
         except ValueError:
             self.page_num = 0
         self.show_all = request.GET.has_key(ALL_VAR)
         self.is_popup = request.GET.has_key(IS_POPUP_VAR)
-        self.params = dict(request.GET.copy())
+        self.params = dict((k, v) for k, v in request.GET.items())
         if self.params.has_key(PAGE_VAR):
             del self.params[PAGE_VAR]
 

django/core/meta/__init__.py

@@ -1678,7 +1678,6 @@ def manipulator_save(opts, klass, add, change, self, new_data):
                 param = f.get_default()
         params[f.attname] = param
 
-
     if change:
         params[opts.pk.attname] = self.obj_key
 
@@ -1710,7 +1709,7 @@ def manipulator_save(opts, klass, add, change, self, new_data):
                 if change and was_changed:
                     self.fields_changed.append(f.verbose_name)
 
-    expanded_data = DotExpandedDict(new_data.data)
+    expanded_data = DotExpandedDict(dict(new_data))
     # Save many-to-one objects. Example: Add the Choice objects for a Poll.
     for related in opts.get_all_related_objects():
         # Create obj_list, which is a DotExpandedDict such as this:
@@ -1724,7 +1723,6 @@ def manipulator_save(opts, klass, add, change, self, new_data):
             obj_list.sort(lambda x, y: cmp(int(x[0]), int(y[0])))
             params = {}
 
-
             # For each related item...
             for _, rel_new_data in obj_list:
 
@@ -1769,7 +1767,6 @@ def manipulator_save(opts, klass, add, change, self, new_data):
                     if param != None:
                        params[f.attname] = param
 
-
                     # Related links are a special case, because we have to
                     # manually set the "content_type_id" and "object_id" fields.
                     if opts.has_related_links and related.opts.module_name == 'relatedlinks':
@@ -1780,8 +1777,6 @@ def manipulator_save(opts, klass, add, change, self, new_data):
                 # Create the related item.
                 new_rel_obj = related.opts.get_model_module().Klass(**params)
 
-
-
                 # If all the core fields were provided (non-empty), save the item.
                 if all_cores_given:
                     new_rel_obj.save()
@@ -1814,7 +1809,6 @@ def manipulator_save(opts, klass, add, change, self, new_data):
                     new_rel_obj.delete()
                     self.fields_deleted.append('%s "%s"' % (related.opts.verbose_name, old_rel_obj))
 
-
     # Save the order, if applicable.
     if change and opts.get_ordered_objects():
         order = new_data['order_'] and map(int, new_data['order_'].split(',')) or []

django/utils/datastructures.py

@@ -43,9 +43,9 @@ class MergeDict:
 class MultiValueDictKeyError(KeyError):
     pass
 
-class MultiValueDict:
+class MultiValueDict(dict):
     """
-    A dictionary-like class customized to deal with multiple values for the same key.
+    A subclass of dictionary customized to handle multiple values for the same key.
 
     >>> d = MultiValueDict({'name': ['Adrian', 'Simon'], 'position': ['Developer']})
     >>> d['name']
@@ -60,35 +60,32 @@ class MultiValueDict:
     which returns a list for every key, even though most Web forms submit
     single name-value pairs.
     """
-    def __init__(self, key_to_list_mapping=None):
+    def __init__(self, key_to_list_mapping=()):
-        self.data = key_to_list_mapping or {}
+        dict.__init__(self, key_to_list_mapping)
-
-    def __repr__(self):
-        return repr(self.data)
 
     def __getitem__(self, key):
-        "Returns the data value for this key; raises KeyError if not found"
+        """
-        if self.data.has_key(key):
+        Returns the last data value for this key, or [] if it's an empty list;
+        raises KeyError if not found.
+        """
         try:
-                return self.data[key][-1] # in case of duplicates, use last value ([-1])
+            list_ = dict.__getitem__(self, key)
+        except KeyError:
+            raise MultiValueDictKeyError, "Key %r not found in MultiValueDict %r" % (key, self)
+        try:
+            return list_[-1]
         except IndexError:
             return []
-        raise MultiValueDictKeyError, "Key '%s' not found in MultiValueDict %s" % (key, self.data)
 
-    def __setitem__(self, key, value):
+    def _setitem_list(self, key, value):
-        self.data[key] = [value]
+        dict.__setitem__(self, key, [value])
+    __setitem__ = _setitem_list
 
-    def __len__(self):
+    def get(self, key, default=None):
-        return len(self.data)
-
-    def __contains__(self, key):
-        return self.data.has_key(key)
-
-    def get(self, key, default):
         "Returns the default value if the requested data doesn't exist"
         try:
             val = self[key]
-        except (KeyError, IndexError):
+        except KeyError:
             return default
         if val == []:
             return default
@@ -97,47 +94,64 @@ class MultiValueDict:
     def getlist(self, key):
         "Returns an empty list if the requested data doesn't exist"
         try:
-            return self.data[key]
+            return dict.__getitem__(self, key)
         except KeyError:
             return []
 
     def setlist(self, key, list_):
-        self.data[key] = list_
+        dict.__setitem__(self, key, list_)
 
-    def appendlist(self, key, item):
+    def setdefault(self, key, default=None):
+        if key not in self:
+            self[key] = default
+        return self[key]
+
+    def setlistdefault(self, key, default_list=()):
+        if key not in self:
+            self.setlist(key, default_list)
+        return self.getlist(key)
+
+    def appendlist(self, key, value):
         "Appends an item to the internal list associated with key"
-        try:
+        self.setlistdefault(key, [])
-            self.data[key].append(item)
+        dict.__setitem__(self, key, self.getlist(key) + [value])
-        except KeyError:
-            self.data[key] = [item]
-
-    def has_key(self, key):
-        return self.data.has_key(key)
 
     def items(self):
-        # we don't just return self.data.items() here, because we want to use
+        """
-        # self.__getitem__() to access the values as *strings*, not lists
+        Returns a list of (key, value) pairs, where value is the last item in
-        return [(key, self[key]) for key in self.data.keys()]
+        the list associated with the key.
+        """
+        return [(key, self[key]) for key in self.keys()]
 
-    def keys(self):
+    def lists(self):
-        return self.data.keys()
+        "Returns a list of (key, list) pairs."
+        return dict.items(self)
 
-    def update(self, other_dict):
+    def values(self):
-        if isinstance(other_dict, MultiValueDict):
+        "Returns a list of the last value on every key list."
-            for key, value_list in other_dict.data.items():
+        return [self[key] for key in self.keys()]
-                self.data.setdefault(key, []).extend(value_list)
-        elif type(other_dict) == type({}):
-            for key, value in other_dict.items():
-                self.data.setdefault(key, []).append(value)
-        else:
-            raise ValueError, "MultiValueDict.update() takes either a MultiValueDict or dictionary"
 
     def copy(self):
-        "Returns a copy of this object"
+        "Returns a copy of this object."
         import copy
+        # Our custom __setitem__ must be disabled for copying machinery.
+        MultiValueDict.__setitem__ = dict.__setitem__
         cp = copy.deepcopy(self)
+        MultiValueDict.__setitem__ = MultiValueDict._setitem_list
         return cp
 
+    def update(self, other_dict):
+        "update() extends rather than replaces existing key lists."
+        if isinstance(other_dict, MultiValueDict):
+            for key, value_list in other_dict.lists():
+                self.setlistdefault(key, []).extend(value_list)
+        else:
+            try:
+                for key, value in other_dict.items():
+                    self.setlistdefault(key, []).append(value)
+            except TypeError:
+                raise ValueError, "MultiValueDict.update() takes either a MultiValueDict or dictionary"
+
 class DotExpandedDict(dict):
     """
     A special dictionary constructor that takes a dictionary in which the keys

django/utils/httpwrappers.py

@@ -67,63 +67,62 @@ class QueryDict(MultiValueDict):
     """A specialized MultiValueDict that takes a query string when initialized.
     This is immutable unless you create a copy of it."""
     def __init__(self, query_string):
-        if not query_string:
+        MultiValueDict.__init__(self)
-            self.data = {}
+        self._mutable = True
-            self._keys = []
+        for key, value in parse_qsl(query_string, True): # keep_blank_values=True
-        else:
+            self.appendlist(key, value)
-            self.data = {}
-            self._keys = []
-            for name, value in parse_qsl(query_string, True): # keep_blank_values=True
-                if name in self.data:
-                    self.data[name].append(value)
-                else:
-                    self.data[name] = [value]
-                if name not in self._keys:
-                    self._keys.append(name)
         self._mutable = False
 
-    def __setitem__(self, key, value):
+    def _assert_mutable(self):
         if not self._mutable:
             raise AttributeError, "This QueryDict instance is immutable"
-        else:
+
-            self.data[key] = [value]
+    def _setitem_if_mutable(self, key, value):
-            if not key in self._keys:
+        self._assert_mutable()
-                self._keys.append(key)
+        MultiValueDict.__setitem__(self, key, value)
+    __setitem__ = _setitem_if_mutable
 
     def setlist(self, key, list_):
-        if not self._mutable:
+        self._assert_mutable()
-            raise AttributeError, "This QueryDict instance is immutable"
+        MultiValueDict.setlist(self, key, list_)
-        else:
+
-            self.data[key] = list_
+    def appendlist(self, key, value):
-            if not key in self._keys:
+        self._assert_mutable()
-                self._keys.append(key)
+        MultiValueDict.appendlist(self, key, value)
+
+    def update(self, other_dict):
+        self._assert_mutable()
+        MultiValueDict.update(self, other_dict)
+
+    def pop(self, key):
+        self._assert_mutable()
+        return MultiValueDict.pop(self, key)
+
+    def popitem(self):
+        self._assert_mutable()
+        return MultiValueDict.popitem(self)
+
+    def clear(self):
+        self._assert_mutable()
+        MultiValueDict.clear(self)
+
+    def setdefault(self, *args):
+        self._assert_mutable()
+        return MultiValueDict.setdefault(self, *args)
 
     def copy(self):
-        "Returns a mutable copy of this object"
+        "Returns a mutable copy of this object."
-        cp = MultiValueDict.copy(self)
+        import copy
+        # Our custom __setitem__ must be disabled for copying machinery.
+        QueryDict.__setitem__ = dict.__setitem__
+        cp = copy.deepcopy(self)
+        QueryDict.__setitem__ = QueryDict._setitem_if_mutable
         cp._mutable = True
         return cp
 
-    def assert_synchronized(self):
-        assert(len(self._keys) == len(self.data.keys())), \
-            "QueryDict data structure is out of sync: %s %s" % (str(self._keys), str(self.data))
-
-    def items(self):
-        "Respect order preserved by self._keys"
-        self.assert_synchronized()
-        items = []
-        for key in self._keys:
-            if key in self.data:
-                items.append((key, self.data[key][0]))
-        return items
-
-    def keys(self):
-        self.assert_synchronized()
-        return self._keys
-
     def urlencode(self):
         output = []
-        for k, list_ in self.data.items():
+        for k, list_ in self.lists():
             output.extend([urlencode({k: v}) for v in list_])
         return '&'.join(output)
 

docs/request_response.txt

@@ -140,45 +140,67 @@ multiple values for the same key.
 That means you can't change attributes of ``request.POST`` and ``request.GET``
 directly.
 
-``QueryDict`` implements the following standard dictionary methods:
+``QueryDict`` implements the all standard dictionary methods, because it's a
-
+subclass of dictionary. Exceptions are outlined here:
-    * ``__repr__()``
 
     * ``__getitem__(key)`` -- Returns the value for the given key. If the key
       has more than one value, ``__getitem__()`` returns the last value.
 
     * ``__setitem__(key, value)`` -- Sets the given key to ``[value]``
-      (a Python list whose single element is ``value``).
+      (a Python list whose single element is ``value``). Note that this, as
+      other dictionary functions that have side effects, can only be called on
+      an immutable ``QueryDict`` (one that was created via ``copy()``).
 
-    * ``__contains__(key)`` -- **New in Django development version.*** Returns
+    * ``__contains__(key)`` -- **New in Django development version.** Returns
-      ``True`` if the given key exists. This lets you do, e.g.,
+      ``True`` if the given key is set. This lets you do, e.g.,
       ``if "foo" in request.GET``.
 
-    * ``__len__()``
-
     * ``get(key, default)`` -- Uses the same logic as ``__getitem__()`` above,
       with a hook for returning a default value if the key doesn't exist.
 
     * ``has_key(key)``
 
+    * ``setdefault(key, default)`` -- Just like the standard dictionary
+      ``setdefault()`` method, except it uses ``__setitem__`` internally.
+
+    * ``update(other_dict)`` -- Takes either a ``QueryDict`` or standard
+      dictionary. Just like the standard dictionary ``update()`` method, except
+      it *appends* to the current dictionary items rather than replacing them.
+      For example::
+
+          >>> q = QueryDict('a=1')
+          >>> q = q.copy() # to make it mutable
+          >>> q.update({'a': '2'})
+          >>> q.getlist('a')
+          ['1', '2']
+          >>> q['a'] # returns the last
+          ['2']
+
     * ``items()`` -- Just like the standard dictionary ``items()`` method,
-      except this retains the order for values of duplicate keys, if any. For
+      except this uses the same last-value logic as ``__getitem()__``. For
-      example, if the original query string was ``"a=1&b=2&b=3"``, ``items()``
+      example::
-      will return ``[("a", ["1"]), ("b", ["2", "3"])]``, where the order of
-      ``["2", "3"]`` is guaranteed, but the order of ``a`` vs. ``b`` isn't.
 
-    * ``keys()``
+           >>> q = QueryDict('a=1&a=2&a=3')
+           >>> q.items()
+           [('a', '3')]
 
-    * ``update(other_dict)``
+    * ``values()`` -- Just like the standard dictionary ``values()`` method,
+      except this uses the same last-value logic as ``__getitem()__``. For
+      example::
 
-In addition, it has the following methods:
+           >>> q = QueryDict('a=1&a=2&a=3')
+           >>> q.values()
+           ['3']
+
+In addition, ``QueryDict`` has the following methods:
 
     * ``copy()`` -- Returns a copy of the object, using ``copy.deepcopy()``
       from the Python standard library. The copy will be mutable -- that is,
       you can change its values.
 
     * ``getlist(key)`` -- Returns the data with the requested key, as a Python
-      list. Returns an empty list if the key doesn't exist.
+      list. Returns an empty list if the key doesn't exist. It's guaranteed to
+      return a list of some sort.
 
     * ``setlist(key, list_)`` -- Sets the given key to ``list_`` (unlike
       ``__setitem__()``).
@@ -186,6 +208,16 @@ In addition, it has the following methods:
     * ``appendlist(key, item)`` -- Appends an item to the internal list
       associated with key.
 
+    * ``setlistdefault(key, default_list)`` -- Just like ``setdefault``, except
+      it takes a list of values instead of a single value.
+
+    * ``lists()`` -- Like ``items()``, except it includes all values, as a list,
+      for each member of the dictionary. For example::
+
+           >>> q = QueryDict('a=1&a=2&a=3')
+           >>> q.lists()
+           [('a', ['1', '2', '3'])]
+
     * ``urlencode()`` -- Returns a string of the data in query-string format.
       Example: ``"a=2&b=3&b=5"``.
 

tests/othertests/httpwrappers.py

@@ -0,0 +1,358 @@
+"""
+###################
+# Empty QueryDict #
+###################
+
+>>> q = QueryDict('')
+
+>>> q['foo']
+Traceback (most recent call last):
+...
+MultiValueDictKeyError: "Key 'foo' not found in MultiValueDict {}"
+
+>>> q['something'] = 'bar'
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.get('foo', 'default')
+'default'
+
+>>> q.getlist('foo')
+[]
+
+>>> q.setlist('foo', ['bar', 'baz'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.appendlist('foo', ['bar'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.has_key('foo')
+False
+
+>>> q.items()
+[]
+
+>>> q.lists()
+[]
+
+>>> q.keys()
+[]
+
+>>> q.values()
+[]
+
+>>> len(q)
+0
+
+>>> q.update({'foo': 'bar'})
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.pop('foo')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.popitem()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.clear()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.setdefault('foo', 'bar')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.urlencode()
+''
+
+###################################
+# Mutable copy of empty QueryDict #
+###################################
+
+>>> q = q.copy()
+
+>>> q['foo']
+Traceback (most recent call last):
+...
+MultiValueDictKeyError: "Key 'foo' not found in MultiValueDict {}"
+
+>>> q['name'] = 'john'
+
+>>> q['name']
+'john'
+
+>>> q.get('foo', 'default')
+'default'
+
+>>> q.get('name', 'default')
+'john'
+
+>>> q.getlist('name')
+['john']
+
+>>> q.getlist('foo')
+[]
+
+>>> q.setlist('foo', ['bar', 'baz'])
+
+>>> q.get('foo', 'default')
+'baz'
+
+>>> q.getlist('foo')
+['bar', 'baz']
+
+>>> q.appendlist('foo', 'another')
+
+>>> q.getlist('foo')
+['bar', 'baz', 'another']
+
+>>> q['foo']
+'another'
+
+>>> q.has_key('foo')
+True
+
+>>> q.items()
+[('foo', 'another'), ('name', 'john')]
+
+>>> q.lists()
+[('foo', ['bar', 'baz', 'another']), ('name', ['john'])]
+
+>>> q.keys()
+['foo', 'name']
+
+>>> q.values()
+['another', 'john']
+
+>>> len(q)
+2
+
+>>> q.update({'foo': 'hello'})
+
+# Displays last value
+>>> q['foo']
+'hello'
+
+>>> q.get('foo', 'not available')
+'hello'
+
+>>> q.getlist('foo')
+['bar', 'baz', 'another', 'hello']
+
+>>> q.pop('foo')
+['bar', 'baz', 'another', 'hello']
+
+>>> q.get('foo', 'not there')
+'not there'
+
+>>> q.setdefault('foo', 'bar')
+'bar'
+
+>>> q['foo']
+'bar'
+
+>>> q.getlist('foo')
+['bar']
+
+>>> q.urlencode()
+'foo=bar&name=john'
+
+>>> q.clear()
+
+>>> len(q)
+0
+
+#####################################
+# QueryDict with one key/value pair #
+#####################################
+
+>>> q = QueryDict('foo=bar')
+
+>>> q['foo']
+'bar'
+
+>>> q['bar']
+Traceback (most recent call last):
+...
+MultiValueDictKeyError: "Key 'bar' not found in MultiValueDict {'foo': ['bar']}"
+
+>>> q['something'] = 'bar'
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.get('foo', 'default')
+'bar'
+
+>>> q.get('bar', 'default')
+'default'
+
+>>> q.getlist('foo')
+['bar']
+
+>>> q.getlist('bar')
+[]
+
+>>> q.setlist('foo', ['bar', 'baz'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.appendlist('foo', ['bar'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.has_key('foo')
+True
+
+>>> q.has_key('bar')
+False
+
+>>> q.items()
+[('foo', 'bar')]
+
+>>> q.lists()
+[('foo', ['bar'])]
+
+>>> q.keys()
+['foo']
+
+>>> q.values()
+['bar']
+
+>>> len(q)
+1
+
+>>> q.update({'foo': 'bar'})
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.pop('foo')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.popitem()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.clear()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.setdefault('foo', 'bar')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.urlencode()
+'foo=bar'
+
+#####################################################
+# QueryDict with two key/value pairs with same keys #
+#####################################################
+
+>>> q = QueryDict('vote=yes&vote=no')
+
+>>> q['vote']
+'no'
+
+>>> q['something'] = 'bar'
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.get('vote', 'default')
+'no'
+
+>>> q.get('foo', 'default')
+'default'
+
+>>> q.getlist('vote')
+['yes', 'no']
+
+>>> q.getlist('foo')
+[]
+
+>>> q.setlist('foo', ['bar', 'baz'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.appendlist('foo', ['bar'])
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.has_key('vote')
+True
+
+>>> q.has_key('foo')
+False
+
+>>> q.items()
+[('vote', 'no')]
+
+>>> q.lists()
+[('vote', ['yes', 'no'])]
+
+>>> q.keys()
+['vote']
+
+>>> q.values()
+['no']
+
+>>> len(q)
+1
+
+>>> q.update({'foo': 'bar'})
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.pop('foo')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.popitem()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.clear()
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.setdefault('foo', 'bar')
+Traceback (most recent call last):
+...
+AttributeError: This QueryDict instance is immutable
+
+>>> q.urlencode()
+'vote=yes&vote=no'
+
+"""
+
+from django.utils.httpwrappers import QueryDict
+
+if __name__ == "__main__":
+    import doctest
+    doctest.testmod()