Fix XML handling of member links (bug 1156594)

This is a was revealed as a blocker for the default_project_id scoping
tests.

Also improves test coverage of XML de/serialization invertibility. The
tests were previously sensitive to things like attribute ordering which
are not relevant to test results. The fix is to compare canonically
serialized XML instead of whatever lxml spews out.

Change-Id: I003583038421d35aadbc781144d4cafb09392db5

keystone/common/serializer.py

@@ -71,35 +71,10 @@ class XmlDeserializer(object):
     def __call__(self, xml_str):
         """Returns a dictionary populated by decoding the given xml string."""
         dom = etree.fromstring(xml_str.strip(), PARSER)
-        links_json = self._find_and_remove_links_from_root(dom, True)
+        return self.walk_element(dom, True)
-        obj_json = self.walk_element(dom, True)
-        if links_json:
-            obj_json['links'] = links_json['links']
-        return obj_json
 
-    def _deserialize_links(self, links, links_json):
+    def _deserialize_links(self, links):
-        for link in links:
+        return dict((x.attrib['rel'], x.attrib['href']) for x in links)
-            links_json['links'][link.attrib['rel']] = link.attrib['href']
-
-    def _find_and_remove_links_from_root(self, dom, namespace):
-        """Special-case links element
-
-        If "links" is in the elements, convert it and remove it from root
-        element. "links" will be placed back into the root of the converted
-        JSON object.
-
-        """
-        for element in dom:
-            decoded_tag = XmlDeserializer._tag_name(element.tag, namespace)
-            if decoded_tag == 'links':
-                links_json = {'links': {}}
-                self._deserialize_links(element, links_json)
-                dom.remove(element)
-                # TODO(gyee): are 'next' and 'previous' mandatory? If so,
-                # setting them to None if they don't exist?
-                links_json['links'].setdefault('previous')
-                links_json['links'].setdefault('next')
-                return links_json
 
     @staticmethod
     def _tag_name(tag, namespace):
@@ -161,23 +136,31 @@ class XmlDeserializer(object):
             else:
                 list_item_tag = decoded_tag[:-1]
 
-        # links is a special dict
         if decoded_tag == 'links':
-            links_json = {'links': {}}
+            return {'links': self._deserialize_links(element)}
-            self._deserialize_links(element, links_json)
-            return links_json
 
+        links = None
         for child in [self.walk_element(x) for x in element
                       if not isinstance(x, ENTITY_TYPE)]:
             if list_item_tag:
-                # FIXME(gyee): special-case lists for now unti we
+                # FIXME(gyee): special-case lists for now until we
                 # figure out how to properly handle them.
                 # If any key ends with an 's', we are assuming it is a list.
-                values.append(child[list_item_tag])
+                if list_item_tag in child:
+                    values.append(child[list_item_tag])
+                else:
+                    links = child['links']
             else:
                 values = dict(values.items() + child.items())
 
-        return {XmlDeserializer._tag_name(element.tag, namespace): values}
+        d = {XmlDeserializer._tag_name(element.tag, namespace): values}
+
+        if links:
+            d['links'] = links
+            d['links'].setdefault('next')
+            d['links'].setdefault('previous')
+
+        return d
 
 
 class XmlSerializer(object):

keystone/test.py

@@ -190,6 +190,9 @@ class TestCase(NoModule, unittest.TestCase):
         self._overrides = []
         self._group_overrides = {}
 
+        # show complete diffs on failure
+        self.maxDiff = None
+
     def setUp(self):
         super(TestCase, self).setUp()
         self.config([etcdir('keystone.conf.sample'),

tests/test_serializer.py

@@ -15,44 +15,52 @@
 # under the License.
 
 import copy
-import re
+import StringIO
+
+from lxml import etree
 
 from keystone.common import serializer
 from keystone import test
 
 
 class XmlSerializerTestCase(test.TestCase):
-    def assertEqualIgnoreWhitespace(self, a, b):
+    def assertEqualXML(self, a, b):
-        """Splits two strings into lists and compares them.
+        """Parses two XML documents from strings and compares the results.
 
         This provides easy-to-read failures from nose.
 
         """
-        try:
+        parser = etree.XMLParser(remove_blank_text=True)
-            self.assertEqual(a, b)
+
-        except AssertionError:
+        def canonical_xml(s):
-            a = re.sub('[ \n]+', ' ', a).strip().split()
+            s = s.strip()
-            b = re.sub('[ \n]+', ' ', b).strip().split()
+
-            self.assertEqual(a, b)
+            fp = StringIO.StringIO()
+            dom = etree.fromstring(s, parser)
+            dom.getroottree().write_c14n(fp)
+            s = fp.getvalue()
+
+            dom = etree.fromstring(s, parser)
+            return etree.tostring(dom, pretty_print=True)
+
+        a = canonical_xml(a)
+        b = canonical_xml(b)
+        self.assertEqual(a.split('\n'), b.split('\n'))
 
     def assertSerializeDeserialize(self, d, xml, xmlns=None):
-        self.assertEqualIgnoreWhitespace(serializer.to_xml(d, xmlns), xml)
+        self.assertEqualXML(
+            serializer.to_xml(copy.deepcopy(d), xmlns),
+            xml)
         self.assertEqual(serializer.from_xml(xml), d)
 
-        # operations should be invertable
+        # operations should be invertible
         self.assertEqual(
-            serializer.from_xml(serializer.to_xml(d, xmlns)),
+            serializer.from_xml(serializer.to_xml(copy.deepcopy(d), xmlns)),
             d)
-        self.assertEqualIgnoreWhitespace(
+        self.assertEqualXML(
             serializer.to_xml(serializer.from_xml(xml), xmlns),
             xml)
 
-    def test_none(self):
-        d = None
-        xml = None
-
-        self.assertSerializeDeserialize(d, xml)
-
     def test_auth_request(self):
         d = {
             "auth": {
@@ -155,7 +163,7 @@ class XmlSerializerTestCase(test.TestCase):
                 <policy id="ab12cd"/>
             </policies>
         """
-        self.assertEqualIgnoreWhitespace(serializer.to_xml(d), xml)
+        self.assertEqualXML(serializer.to_xml(d), xml)
 
     def test_values_list(self):
         d = {
@@ -176,7 +184,7 @@ class XmlSerializerTestCase(test.TestCase):
             </objects>
         """
 
-        self.assertEqualIgnoreWhitespace(serializer.to_xml(d), xml)
+        self.assertEqualXML(serializer.to_xml(d), xml)
 
     def test_collection_list(self):
         d = {
@@ -222,6 +230,26 @@ class XmlSerializerTestCase(test.TestCase):
                 </links>
             </objects>
         """
-        self.assertEqualIgnoreWhitespace(
+        self.assertSerializeDeserialize(d, xml)
-            serializer.to_xml(copy.deepcopy(d)), xml)
+
-        self.assertDictEqual(serializer.from_xml(xml), d)
+    def test_collection_member(self):
+        d = {
+            "object": {
+                "attribute": "value",
+                "links": {
+                    "self": "http://localhost:5000/v3/objects/abc123def",
+                    "anotherobj": "http://localhost:5000/v3/anotherobjs/123"}}}
+
+        xml = """
+            <?xml version="1.0" encoding="UTF-8"?>
+            <object xmlns="http://docs.openstack.org/identity/api/v2.0"
+                attribute="value">
+                    <links>
+                        <link rel="self"
+                            href="http://localhost:5000/v3/objects/abc123def"/>
+                        <link rel="anotherobj"
+                            href="http://localhost:5000/v3/anotherobjs/123"/>
+                    </links>
+            </object>
+        """
+        self.assertSerializeDeserialize(d, xml)