launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-1075597 into lp:maas.

Commit message:
Don't include the script_name prefix in build_absolute_uri; it was already in the path it appends.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1075597 in MAAS: "Duplicated prefix in the url used by the CLI"
  https://bugs.launchpad.net/maas/+bug/1075597

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-1075597/+merge/133328

Pre-imp'ed with Raphaël.  We were getting duplicate path prefixes in the URLs in the API description, along the lines of “http://mymaas.example.local/MAAS/MAAS/api/1.0/”;  The prefix is the “script_name” which we set in settings.py.

The test should have caught this, except Django seems to initialize its URL prefix at WSGI startup.  Which means that it's just not happening in tests.  And so we needed to set it explicitly for the test (not as straightforward as the code makes it look!) and then change the meaning of build_absolute_uri's “path” parameter.

Previously this parameter was supposed to be the http path from the application root to a given API resource, which meant duplicating the prefix, but now it's actually simpler: it's simply the absolute http path to the resource.

The purpose of this whole URL dance was to allow clients to use these links despite talking to the API on different server addresses — e.g. if the server had multiple network interfaces and addresses facing multiple networks.  So it's actually fine to recompute the entire path; the request is only interesting because of its network addressing.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-1075597/+merge/133328
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-1075597 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-07 17:08:42 +0000
+++ src/maasserver/api.py	2012-11-07 19:43:21 +0000
@@ -1961,7 +1961,11 @@
 def describe(request):
     """Return a description of the whole MAAS API.
 
-    Returns a JSON object describing the whole MAAS API.
+    :param request: The http request for this document.  This is used to
+        derive the URL where the client expects to see the MAAS API.
+    :return: A JSON object describing the whole MAAS API.  Links to the API
+        will use the same scheme and hostname that the client used in
+        `request`.
     """
     from maasserver import urls_api as urlconf
     resources = [

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-07 17:08:42 +0000
+++ src/maasserver/tests/test_api.py	2012-11-07 19:43:21 +0000
@@ -38,7 +38,11 @@
 from celery.app import app_or_default
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
-from django.core.urlresolvers import reverse
+import django.core.urlresolvers
+from django.core.urlresolvers import (
+    reverse,
+    get_script_prefix,
+    )
 from django.http import QueryDict
 from django.test.client import RequestFactory
 from fixtures import Fixture
@@ -4448,19 +4452,53 @@
     scenarios = multiply_scenarios(
         scenarios_schemes, scenarios_paths)
 
-    def test_handler_uris_are_absolute(self):
-        server = factory.make_name("server").lower()
-        extra = {
+    def make_params(self):
+        """Create parameters for http request, based on current scenario."""
+        return {
             "PATH_INFO": self.path_info,
             "SCRIPT_NAME": self.script_name,
-            "SERVER_NAME": server,
+            "SERVER_NAME": factory.make_name('server').lower(),
             "wsgi.url_scheme": self.scheme,
-            }
-        request = RequestFactory().get(
-            "/%s/describe" % factory.make_name("path"), **extra)
+        }
+
+    def get_description(self, params):
+        """GET the API description (at a random API path), as JSON."""
+        path = '/%s/describe' % factory.make_name('path')
+        request = RequestFactory().get(path, **params)
         response = describe(request)
-        self.assertEqual(httplib.OK, response.status_code, response.content)
-        description = json.loads(response.content)
+        self.assertEqual(
+            httplib.OK, response.status_code,
+            "API description failed with code %s:\n%s"
+            % (response.status_code, response.content))
+        return json.loads(response.content)
+
+    def patch_script_prefix(self):
+        """Patch up Django's and Piston's notion of the script_name prefix.
+
+        This manipulates how Piston gets Django's version of script_name
+        which it needs so that it can prefix script_name to URL paths.
+        """
+        # Patching up get_script_prefix doesn't seem to do the trick,
+        # and patching it in the right module requires unwarranted
+        # intimacy with Piston.  So just go through the proper call and
+        # set the prefix.  But clean this up after the test or it will
+        # break other tests!
+        original_prefix = get_script_prefix()
+        self.addCleanup(
+            django.core.urlresolvers.set_script_prefix, original_prefix)
+        django.core.urlresolvers.set_script_prefix(self.script_name)
+
+    def test_handler_uris_are_absolute(self):
+        params = self.make_params()
+        server = params['SERVER_NAME']
+
+        # Without this, the test wouldn't be able to detect accidental
+        # duplication of the script_name portion of the URL path:
+        # /MAAS/MAAS/api/...
+        self.patch_script_prefix()
+
+        description = self.get_description(params)
+
         expected_uri = AfterPreprocessing(
             urlparse, MatchesStructure(
                 scheme=Equals(self.scheme), hostname=Equals(server),

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2012-11-01 11:45:29 +0000
+++ src/maasserver/utils/__init__.py	2012-11-07 19:43:21 +0000
@@ -87,20 +87,17 @@
 
 
 def build_absolute_uri(request, path):
-    """Given a full app-relative path, returns an absolute URI.
-
-    The path ordinarily starts with a forward-slash... imagine that you've
-    magically chroot'ed to your Django application; the path is the absolute
-    path to the view within that environment.
-
-    The URI returned uses the request to figure out how to make an absolute
-    URL. This means that the URI returned will use the same IP address or
-    alias that the request came in on.
+    """Return absolute URI corresponding to given absolute path.
+
+    :param request: An http request to the API.  This is needed in order to
+        figure out how the client is used to addressing
+        the API on the network.
+    :param path: The absolute http path to a given resource.
+    :return: Full, absolute URI to the resource, taking its networking
+        portion from `request` but the rest from `path`.
     """
-    script_name = request.META["SCRIPT_NAME"]
-    return "%s://%s%s%s" % (
-        "https" if request.is_secure() else "http",
-        request.get_host(), script_name, path)
+    scheme = "https" if request.is_secure() else "http"
+    return "%s://%s%s" % (scheme, request.get_host(), path)
 
 
 def strip_domain(hostname):

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2012-11-02 09:22:01 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-11-07 19:43:21 +0000
@@ -134,12 +134,13 @@
             "http://example.com:1234/fred";,
             build_absolute_uri(request, "/fred"))
 
-    def test_script_name_is_prefixed(self):
-        # The script name is always prefixed to the given path.
+    def test_script_name_is_ignored(self):
+        # The given path already includes the script_name, so the
+        # script_name passed in the request is not included again.
         request = self.make_request(script_name="/foo/bar")
         self.assertEqual(
             "http://example.com/foo/bar/fred";,
-            build_absolute_uri(request, "/fred"))
+            build_absolute_uri(request, "/foo/bar/fred"))
 
     def test_secure(self):
         request = self.make_request(port=443, is_secure=True)
@@ -153,21 +154,13 @@
             "https://example.com:9443/fred";,
             build_absolute_uri(request, "/fred"))
 
-    def test_no_leading_forward_slash(self):
-        # No attempt is made to ensure that the given path is separated from
-        # the to-be-prefixed path.
-        request = self.make_request(script_name="/foo")
-        self.assertEqual(
-            "http://example.com/foobar";,
-            build_absolute_uri(request, "bar"))
-
     def test_preserve_two_leading_slashes(self):
         # Whilst this shouldn't ordinarily happen, two leading slashes in the
         # path should be preserved, and not treated specially.
-        request = self.make_request(script_name="//foo")
+        request = self.make_request()
         self.assertEqual(
-            "http://example.com//foo/fred";,
-            build_absolute_uri(request, "/fred"))
+            "http://example.com//foo";,
+            build_absolute_uri(request, "//foo"))
 
 
 class TestStripDomain(TestCase):