launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/repair-cobbler-response into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/repair-cobbler-response/+merge/92067
-- 
https://code.launchpad.net/~allenap/maas/repair-cobbler-response/+merge/92067
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/repair-cobbler-response into lp:maas.

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-02-06 11:51:58 +0000
+++ src/provisioningserver/cobblerclient.py	2012-02-08 16:44:54 +0000
@@ -44,6 +44,47 @@
 DEFAULT_TIMEOUT = 30
 
 
+def tilde_to_None(data):
+    """Repair Cobbler's XML-RPC response.
+
+    Cobbler has an annoying function, `cobbler.utils.strip_none`, that is
+    applied to every data structure that it sends back through its XML-RPC API
+    service. It "encodes" `None` as `"~"`, and does so recursively in `list`s
+    and `dict`s. It also forces all dictionary keys to be `str`, so `None`
+    keys become `"None"`.
+
+    This function attempts to repair this damage. Sadly, it may get things
+    wrong - it will "repair" genuine tildes to `None` - but it's likely to be
+    more correct than doing nothing - and having tildes everwhere.
+
+    This also does not attempt to repair `"None"` dictionary keys.
+    """
+    if data == "~":
+        return None
+    elif isinstance(data, list):
+        return [tilde_to_None(value) for value in data]
+    elif isinstance(data, dict):
+        return {key: tilde_to_None(value) for key, value in data.iteritems()}
+    else:
+        return data
+
+
+class CobblerXMLRPCProxy(Proxy):
+    """An XML-RPC proxy that attempts to repair Cobbler's broken responses.
+
+    See `tilde_to_None` for an explanation.
+    """
+
+    def callRemote(self, method, *args):
+        """See `Proxy.callRemote`.
+
+        Uses `tilde_to_None` to repair the response.
+        """
+        d = Proxy.callRemote(self, method, *args)
+        d.addCallback(tilde_to_None)
+        return d
+
+
 def looks_like_auth_expiry(exception):
     """Does `exception` look like an authentication token expired?"""
     if not hasattr(exception, 'faultString'):
@@ -87,7 +128,7 @@
         # here, and hope it doesn't lead to breakage in Twisted.  We'll
         # figure out what to do about non-ASCII characters in URLs
         # later.
-        return Proxy(self.url.encode('ascii'))
+        return CobblerXMLRPCProxy(self.url.encode('ascii'))
 
     def record_state(self):
         """Return a cookie representing the session's current state.

=== added file 'src/provisioningserver/tests/test_cobblerclient.py'
--- src/provisioningserver/tests/test_cobblerclient.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_cobblerclient.py	2012-02-08 16:44:54 +0000
@@ -0,0 +1,65 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `provisioningserver.cobblerclient`."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from provisioningserver.cobblerclient import (
+    CobblerXMLRPCProxy,
+    tilde_to_None,
+    )
+from testtools import TestCase
+from twisted.test.proto_helpers import MemoryReactor
+
+
+class TestRepairingCobblerResponses(TestCase):
+    """See `tilde_to_None`."""
+
+    def test_tilde_to_None(self):
+        self.assertIsNone(tilde_to_None("~"))
+
+    def test_tilde_to_None_list(self):
+        self.assertEqual(
+            [1, 2, 3, None, 5],
+            tilde_to_None([1, 2, 3, "~", 5]))
+
+    def test_tilde_to_None_nested_list(self):
+        self.assertEqual(
+            [1, 2, [3, None], 5],
+            tilde_to_None([1, 2, [3, "~"], 5]))
+
+    def test_tilde_to_None_dict(self):
+        self.assertEqual(
+            {"one": 1, "two": None},
+            tilde_to_None({"one": 1, "two": "~"}))
+
+    def test_tilde_to_None_nested_dict(self):
+        self.assertEqual(
+            {"one": 1, "two": {"three": None}},
+            tilde_to_None({"one": 1, "two": {"three": "~"}}))
+
+    def test_tilde_to_None_nested_mixed(self):
+        self.assertEqual(
+            {"one": 1, "two": [3, 4, None]},
+            tilde_to_None({"one": 1, "two": [3, 4, "~"]}))
+
+    def test_CobblerXMLRPCProxy(self):
+        reactor = MemoryReactor()
+        proxy = CobblerXMLRPCProxy(
+            "http://localhost:1234/nowhere";, reactor=reactor)
+        d = proxy.callRemote("cobble", 1, 2, 3)
+        # A connection has been initiated.
+        self.assertEqual(1, len(reactor.tcpClients))
+        [client] = reactor.tcpClients
+        self.assertEqual("localhost", client[0])
+        self.assertEqual(1234, client[1])
+        # A "broken" response from Cobbler is "repaired".
+        d.callback([1, 2, "~"])
+        self.assertEqual([1, 2, None], d.result)