Fix stable ordering for multidict iterations, add more tests

Author: asvetlov

aiohttp/multidict.py

@@ -1,10 +1,24 @@
 import pprint
-from itertools import chain
+from itertools import chain, filterfalse
 from collections import abc
 
 _marker = object()
 
 
+def _unique_everseen(iterable):
+    """List unique elements, preserving order.
+    Remember all elements ever seen.
+    Recipe from
+    https://docs.python.org/3/library/itertools.html#itertools-recipes"""
+    # unique_everseen('AAAABBBCCDAABBB') --> A B C D
+    # unique_everseen('ABBCcAD', str.lower) --> A B C D
+    seen = set()
+    seen_add = seen.add
+    for element in filterfalse(seen.__contains__, iterable):
+        seen_add(element)
+        yield element
+
+
 class MultiDict(abc.Mapping):
     """Read-only ordered dictionary that can have multiple values for each key.
 
@@ -25,10 +39,8 @@ def __init__(self, *args, **kwargs):
                 args = list(args[0])
                 for arg in args:
                     if not len(arg) == 2:
-                        raise TypeError(
-                            "MultiDict takes either dict or list of \
-                            (key, value) tuples"
-                        )
+                        raise TypeError("MultiDict takes either dict "
+                                        "or list of (key, value) tuples")
 
         self._fill(chain(args, kwargs.items()))
 
@@ -217,13 +229,13 @@ def __delitem__(self, key):
         super().__delitem__(key.upper())
 
 
-class _ItemsView(abc.ItemsView):
+class _ViewBase:
 
     def __init__(self, items, *, getall=False):
         self._getall = getall
         self._keys = [item[0] for item in items]
         if not getall:
-            self._keys = set(self._keys)
+            self._keys = list(_unique_everseen(self._keys))
 
         items_to_use = []
         if getall:
@@ -238,6 +250,9 @@ def __init__(self, items, *, getall=False):
 
         super().__init__(items_to_use)
 
+
+class _ItemsView(_ViewBase, abc.ItemsView):
+
     def __contains__(self, item):
         assert isinstance(item, tuple) or isinstance(item, list)
         assert len(item) == 2
@@ -247,27 +262,7 @@ def __iter__(self):
         yield from self._mapping
 
 
-class _ValuesView(abc.ValuesView):
-
-    def __init__(self, items, *, getall=False):
-        self._getall = getall
-        self._keys = [item[0] for item in items]
-        if not getall:
-            self._keys = set(self._keys)
-
-        items_to_use = []
-        if getall:
-            items_to_use = items
-        else:
-            for key in self._keys:
-                for k, v in items:
-                    if k == key:
-                        items_to_use.append((k, v))
-                        break
-
-        assert len(items_to_use) == len(self._keys)
-
-        super().__init__(items_to_use)
+class _ValuesView(_ViewBase, abc.ValuesView):
 
     def __contains__(self, value):
         for item in self._mapping:
@@ -280,26 +275,7 @@ def __iter__(self):
             yield item[1]
 
 
-class _KeysView(abc.KeysView):
-
-    def __init__(self, items, *, getall=False):
-        self._getall = getall
-        self._keys = [item[0] for item in items]
-        if not getall:
-            self._keys = set(self._keys)
-
-        items_to_use = []
-        if getall:
-            items_to_use = items
-        else:
-            for key in self._keys:
-                for k, v in items:
-                    if k == key:
-                        items_to_use.append((k, v))
-                        break
-        assert len(items_to_use) == len(self._keys)
-
-        super().__init__(items_to_use)
+class _KeysView(_ViewBase, abc.KeysView):
 
     def __contains__(self, key):
         return key in self._keys

tests/test_multidict.py

@@ -67,13 +67,29 @@ def test_getone(self):
         with self.assertRaises(KeyError):
             d.getone('key2')
 
+        self.assertEqual('default', d.getone('key2', 'default'))
+
     def test_copy(self):
         d1 = self.make_dict(key='value', a='b')
 
         d2 = d1.copy()
         self.assertEqual(d1, d2)
         self.assertIsNot(d1, d2)
 
+    def test_keys__contains(self):
+        d = self.make_dict([('key', 'one'), ('key2', 'two'), ('key', 3)])
+        self.assertEqual(list(d.keys()), ['key', 'key2'])
+        self.assertEqual(list(d.keys(getall=True)), ['key', 'key2', 'key'])
+
+        self.assertIn('key', d.keys())
+        self.assertIn('key2', d.keys())
+
+        self.assertIn('key', d.keys(getall=True))
+        self.assertIn('key2', d.keys(getall=True))
+
+        self.assertNotIn('foo', d.keys())
+        self.assertNotIn('foo', d.keys(getall=True))
+
     def test_values__contains(self):
         d = self.make_dict([('key', 'one'), ('key', 'two'), ('key', 3)])
         self.assertEqual(list(d.values()), ['one'])
@@ -107,6 +123,10 @@ def test_items__contains(self):
         self.assertNotIn(('foo', 'bar'), d.items())
         self.assertNotIn(('foo', 'bar'), d.items(getall=True))
 
+    def test_cannot_create_from_unaccepted(self):
+        with self.assertRaises(TypeError):
+            self.make_dict([(1, 2, 3)])
+
 
 class MultiDictTests(_BaseTest, unittest.TestCase):
 
@@ -134,6 +154,12 @@ def test_getall(self):
         default = object()
         self.assertIs(d.getall('some_key', default), default)
 
+    def test_preserve_stable_ordering(self):
+        d = self.make_dict([('a', 1), ('b', '2'), ('a', 3)])
+        s = '&'.join('{}={}'.format(k, v) for k, v in d.items(getall=True))
+
+        self.assertEqual('a=1&b=2&a=3', s)
+
 
 class CaseInsensitiveMultiDictTests(unittest.TestCase):