From 167a8b7a3ed8f60018826a2d73d4af3b2ae7b66f Mon Sep 17 00:00:00 2001 From: Dolph Mathews Date: Mon, 18 Mar 2013 08:21:34 -0500 Subject: [PATCH] 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 | 53 +++++++++--------------- keystone/test.py | 3 ++ tests/test_serializer.py | 76 ++++++++++++++++++++++++----------- 3 files changed, 73 insertions(+), 59 deletions(-) diff --git a/keystone/common/serializer.py b/keystone/common/serializer.py index e5f21a7364..9b9df418ac 100644 --- a/keystone/common/serializer.py +++ b/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) - obj_json = self.walk_element(dom, True) - if links_json: - obj_json['links'] = links_json['links'] - return obj_json + return self.walk_element(dom, True) - def _deserialize_links(self, links, links_json): - for link 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 + def _deserialize_links(self, links): + return dict((x.attrib['rel'], x.attrib['href']) for x in links) @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': {}} - self._deserialize_links(element, links_json) - return links_json + return {'links': self._deserialize_links(element)} + 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): diff --git a/keystone/test.py b/keystone/test.py index 21b1924dc9..3a28b5ff81 100644 --- a/keystone/test.py +++ b/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'), diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 8ea4901597..b440c815e1 100644 --- a/tests/test_serializer.py +++ b/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): - """Splits two strings into lists and compares them. + def assertEqualXML(self, a, b): + """Parses two XML documents from strings and compares the results. This provides easy-to-read failures from nose. """ - try: - self.assertEqual(a, b) - except AssertionError: - a = re.sub('[ \n]+', ' ', a).strip().split() - b = re.sub('[ \n]+', ' ', b).strip().split() - self.assertEqual(a, b) + parser = etree.XMLParser(remove_blank_text=True) + + def canonical_xml(s): + s = s.strip() + + 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): """ - 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): """ - 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): """ - self.assertEqualIgnoreWhitespace( - serializer.to_xml(copy.deepcopy(d)), xml) - self.assertDictEqual(serializer.from_xml(xml), d) + self.assertSerializeDeserialize(d, xml) + + 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 = """ + + + + + + + + """ + self.assertSerializeDeserialize(d, xml)