launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/apiclient-fix into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/apiclient-fix/+merge/121782

urljoin is crack.

>>> urljoin("http://10.0.0.9/MAAS";, "/api/thing")
'http://10.0.0.9/api/thing'

This is causing the celeryd to use the wrong API path in the packaged MAAS.  This branch fixes that by not using urljoin and doing it manually.  Yes, I can filch with the slashes that urljoin uses but this way is much more reliable.
-- 
https://code.launchpad.net/~julian-edwards/maas/apiclient-fix/+merge/121782
Your team MAAS Maintainers is requested to review the proposed merge of lp:~julian-edwards/maas/apiclient-fix into lp:maas.

=== modified file 'src/apiclient/maas_client.py'
--- src/apiclient/maas_client.py	2012-05-24 04:03:05 +0000
+++ src/apiclient/maas_client.py	2012-08-29 09:39:31 +0000
@@ -117,7 +117,10 @@
         """
         if not isinstance(path, basestring):
             path = '/'.join(unicode(element) for element in path)
-        return urljoin(self.url, path)
+        # urljoin is massively unreliable when spurious slashes appear
+        # since it removes path parts. This is why joining is done
+        # manually here.
+        return self.url.rstrip("/") + "/" + path.lstrip("/")
 
     def _formulate_get(self, path, params=None):
         """Return URL and headers for a GET request.

=== modified file 'src/apiclient/tests/test_maas_client.py'
--- src/apiclient/tests/test_maas_client.py	2012-05-24 04:03:05 +0000
+++ src/apiclient/tests/test_maas_client.py	2012-08-29 09:39:31 +0000
@@ -28,6 +28,7 @@
     )
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
+from testtools.matchers import StartsWith
 
 
 class TestHelpers(TestCase):
@@ -71,7 +72,7 @@
 
 def make_path():
     """Create an arbitrary resource path."""
-    return '/'.join(factory.getRandomString() for counter in range(2))
+    return "/" + '/'.join(factory.getRandomString() for counter in range(2))
 
 
 class FakeDispatcher:
@@ -107,7 +108,8 @@
     def test_make_url_joins_root_and_path(self):
         path = make_path()
         client = make_client()
-        self.assertEqual(urljoin(client.url, path), client._make_url(path))
+        expected = client.url.rstrip("/") + "/" + path.lstrip("/")
+        self.assertEqual(expected, client._make_url(path))
 
     def test_make_url_converts_sequence_to_path(self):
         path = ['top', 'sub', 'leaf']
@@ -125,7 +127,8 @@
         path = make_path()
         client = make_client()
         url, headers = client._formulate_get(path)
-        self.assertEqual(urljoin(client.url, path), url)
+        expected = client.url.rstrip("/") + "/" + path.lstrip("/")
+        self.assertEqual(expected, url)
 
     def test_formulate_get_adds_parameters_to_url(self):
         params = {
@@ -143,7 +146,8 @@
         path = make_path()
         client = make_client()
         url, headers, body = client._formulate_change(path, {})
-        self.assertEqual(urljoin(client.url, path), url)
+        expected = client.url.rstrip("/") + "/" + path.lstrip("/")
+        self.assertEqual(expected, url)
 
     def test_formulate_change_signs_request(self):
         url, headers, body = make_client()._formulate_change(make_path(), {})