LP#1838252: Keep the order provided by an OrderedDict that gets passed as attrib mapping during element creation. This was broken in 4.4.0.

3 files changed, 42 insertions, 24 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index 5231f99d..8ac1b59e 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -2,6 +2,16 @@
 lxml changelog
 ==============
 
+4.4.1 (2019-0?-??)
+==================
+
+Bugs fixed
+----------
+
+* LP#1838252: The order of an OrderedDict was lost in 4.4.0 when passing it as
+  attrib mapping during element creation.
+
+
 4.4.0 (2019-07-27)
 ==================
 
diff --git a/src/lxml/apihelpers.pxi b/src/lxml/apihelpers.pxi
index d54bf8d6..a66f127f 100644
--- a/src/lxml/apihelpers.pxi
+++ b/src/lxml/apihelpers.pxi
@@ -292,17 +292,12 @@ cdef _iter_attrib(attrib):
     Create a reproducibly ordered iterable from an attrib mapping.
     Tries to preserve an existing order and sorts if it assumes no order.
     """
-    # attrib will usually be a plain unordered dict
-    if isinstance(attrib, dict):
-        if python.PY_VERSION_HEX >= 0x03060000:
-            # dicts are insertion-ordered in Py3.6+ => keep the user provided order.
-            return attrib.items()
-        return sorted(attrib.items())
-    elif isinstance(attrib, (_Attrib, OrderedDict)):
+    # dicts are insertion-ordered in Py3.6+ => keep the user provided order.
+    if python.PY_VERSION_HEX >= 0x03060000 and isinstance(attrib, dict) or (
+            isinstance(attrib, (_Attrib, OrderedDict))):
         return attrib.items()
-    else:
-        # assume it's an unordered mapping of some kind
-        return sorted(attrib.items())
+    # assume it's an unordered mapping of some kind
+    return sorted(attrib.items())
 
 
 cdef _initNodeAttributes(xmlNode* c_node, _Document doc, attrib, dict extra):
diff --git a/src/lxml/tests/test_etree.py b/src/lxml/tests/test_etree.py
index 7e309468..fc31967d 100644
--- a/src/lxml/tests/test_etree.py
+++ b/src/lxml/tests/test_etree.py
@@ -9,6 +9,7 @@ test_elementtree
 
 from __future__ import absolute_import
 
+from collections import OrderedDict
 import os.path
 import unittest
 import copy
@@ -16,7 +17,6 @@ import sys
 import re
 import gc
 import operator
-import tempfile
 import textwrap
 import zlib
 import gzip
@@ -286,8 +286,8 @@ class ETreeOnlyTestCase(HelperTestCase):
     def test_attrib_order(self):
         Element = self.etree.Element
 
-        keys = ["attr%d" % i for i in range(10)]
-        values = ["TEST-%d" % i for i in range(10)]
+        keys = ["attr%d" % i for i in range(12, 4, -1)]
+        values = ["TEST-%d" % i for i in range(12, 4, -1)]
         items = list(zip(keys, values))
 
         root = Element("root")
@@ -296,19 +296,32 @@ class ETreeOnlyTestCase(HelperTestCase):
         self.assertEqual(keys, root.attrib.keys())
         self.assertEqual(values, root.attrib.values())
 
-        root2 = Element("root2", root.attrib,
-                        attr_99='TOAST-1', attr_98='TOAST-2')
-
+        attr_order = [
+            ('attr_99', 'TOAST-1'),
+            ('attr_98', 'TOAST-2'),
+        ]
+        ordered_dict_types = [OrderedDict, lambda x:x]
         if sys.version_info >= (3, 6):
-            self.assertEqual(['attr_99', 'attr_98'] + keys,
-                             root2.attrib.keys())
-            self.assertEqual(['TOAST-1', 'TOAST-2'] + values,
-                             root2.attrib.values())
+            ordered_dict_types.append(dict)
         else:
-            self.assertEqual(['attr_98', 'attr_99'] + keys,
-                             root2.attrib.keys())
-            self.assertEqual(['TOAST-2', 'TOAST-1'] + values,
-                             root2.attrib.values())
+            # Keyword arguments are not ordered in Py<3.6, and thus get sorted.
+            attr_order.sort()
+        attr_order += items
+        expected_keys = [attr[0] for attr in attr_order]
+        expected_values = [attr[1] for attr in attr_order]
+        expected_items = list(zip(expected_keys, expected_values))
+
+        for dict_type in ordered_dict_types:
+            root2 = Element("root2", dict_type(root.attrib),
+                            attr_99='TOAST-1', attr_98='TOAST-2')
+
+            try:
+                self.assertSequenceEqual(expected_keys, root2.attrib.keys())
+                self.assertSequenceEqual(expected_values, root2.attrib.values())
+                self.assertSequenceEqual(expected_items, root2.attrib.items())
+            except AssertionError as exc:
+                exc.args = ("Order of '%s': %s" % (dict_type.__name__, exc.args[0]),) + exc.args[1:]
+                raise
 
         self.assertEqual(keys, root.attrib.keys())
         self.assertEqual(values, root.attrib.values())