Merge lp:~jtv/maas/bug-1075597 into lp:~maas-committers/maas/trunk

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 1334
Proposed branch: lp:~jtv/maas/bug-1075597
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 177 lines (+70/-38)
4 files modified
src/maasserver/api.py (+5/-1)
src/maasserver/tests/test_api.py (+48/-10)
src/maasserver/utils/__init__.py (+10/-13)
src/maasserver/utils/tests/test_utils.py (+7/-14)
To merge this branch: bzr merge lp:~jtv/maas/bug-1075597
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+133328@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

81 + django.core.urlresolvers.set_script_prefix(self.script_name)

It took me a while to work out where self.script_name was set and then realised it's part of the test scenarios. It might be worth passing it into this function (patch_script_prefix) as a parameter otherwise it looks like the usual testing antipattern stuff.

Everything else looks good. Cheers.

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

(Made the requested change, but forgot to reload the MP)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-11-07 17:08:42 +0000
+++ src/maasserver/api.py 2012-11-08 08:35:21 +0000
@@ -1961,7 +1961,11 @@
1961def describe(request):1961def describe(request):
1962 """Return a description of the whole MAAS API.1962 """Return a description of the whole MAAS API.
19631963
1964 Returns a JSON object describing the whole MAAS API.1964 :param request: The http request for this document. This is used to
1965 derive the URL where the client expects to see the MAAS API.
1966 :return: A JSON object describing the whole MAAS API. Links to the API
1967 will use the same scheme and hostname that the client used in
1968 `request`.
1965 """1969 """
1966 from maasserver import urls_api as urlconf1970 from maasserver import urls_api as urlconf
1967 resources = [1971 resources = [
19681972
=== 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-08 08:35:21 +0000
@@ -38,7 +38,11 @@
38from celery.app import app_or_default38from celery.app import app_or_default
39from django.conf import settings39from django.conf import settings
40from django.contrib.auth.models import AnonymousUser40from django.contrib.auth.models import AnonymousUser
41from django.core.urlresolvers import reverse41import django.core.urlresolvers
42from django.core.urlresolvers import (
43 reverse,
44 get_script_prefix,
45 )
42from django.http import QueryDict46from django.http import QueryDict
43from django.test.client import RequestFactory47from django.test.client import RequestFactory
44from fixtures import Fixture48from fixtures import Fixture
@@ -4448,19 +4452,53 @@
4448 scenarios = multiply_scenarios(4452 scenarios = multiply_scenarios(
4449 scenarios_schemes, scenarios_paths)4453 scenarios_schemes, scenarios_paths)
44504454
4451 def test_handler_uris_are_absolute(self):4455 def make_params(self):
4452 server = factory.make_name("server").lower()4456 """Create parameters for http request, based on current scenario."""
4453 extra = {4457 return {
4454 "PATH_INFO": self.path_info,4458 "PATH_INFO": self.path_info,
4455 "SCRIPT_NAME": self.script_name,4459 "SCRIPT_NAME": self.script_name,
4456 "SERVER_NAME": server,4460 "SERVER_NAME": factory.make_name('server').lower(),
4457 "wsgi.url_scheme": self.scheme,4461 "wsgi.url_scheme": self.scheme,
4458 }4462 }
4459 request = RequestFactory().get(4463
4460 "/%s/describe" % factory.make_name("path"), **extra)4464 def get_description(self, params):
4465 """GET the API description (at a random API path), as JSON."""
4466 path = '/%s/describe' % factory.make_name('path')
4467 request = RequestFactory().get(path, **params)
4461 response = describe(request)4468 response = describe(request)
4462 self.assertEqual(httplib.OK, response.status_code, response.content)4469 self.assertEqual(
4463 description = json.loads(response.content)4470 httplib.OK, response.status_code,
4471 "API description failed with code %s:\n%s"
4472 % (response.status_code, response.content))
4473 return json.loads(response.content)
4474
4475 def patch_script_prefix(self, script_name):
4476 """Patch up Django's and Piston's notion of the script_name prefix.
4477
4478 This manipulates how Piston gets Django's version of script_name
4479 which it needs so that it can prefix script_name to URL paths.
4480 """
4481 # Patching up get_script_prefix doesn't seem to do the trick,
4482 # and patching it in the right module requires unwarranted
4483 # intimacy with Piston. So just go through the proper call and
4484 # set the prefix. But clean this up after the test or it will
4485 # break other tests!
4486 original_prefix = get_script_prefix()
4487 self.addCleanup(
4488 django.core.urlresolvers.set_script_prefix, original_prefix)
4489 django.core.urlresolvers.set_script_prefix(script_name)
4490
4491 def test_handler_uris_are_absolute(self):
4492 params = self.make_params()
4493 server = params['SERVER_NAME']
4494
4495 # Without this, the test wouldn't be able to detect accidental
4496 # duplication of the script_name portion of the URL path:
4497 # /MAAS/MAAS/api/...
4498 self.patch_script_prefix(self.script_name)
4499
4500 description = self.get_description(params)
4501
4464 expected_uri = AfterPreprocessing(4502 expected_uri = AfterPreprocessing(
4465 urlparse, MatchesStructure(4503 urlparse, MatchesStructure(
4466 scheme=Equals(self.scheme), hostname=Equals(server),4504 scheme=Equals(self.scheme), hostname=Equals(server),
44674505
=== 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-08 08:35:21 +0000
@@ -87,20 +87,17 @@
8787
8888
89def build_absolute_uri(request, path):89def build_absolute_uri(request, path):
90 """Given a full app-relative path, returns an absolute URI.90 """Return absolute URI corresponding to given absolute path.
9191
92 The path ordinarily starts with a forward-slash... imagine that you've92 :param request: An http request to the API. This is needed in order to
93 magically chroot'ed to your Django application; the path is the absolute93 figure out how the client is used to addressing
94 path to the view within that environment.94 the API on the network.
9595 :param path: The absolute http path to a given resource.
96 The URI returned uses the request to figure out how to make an absolute96 :return: Full, absolute URI to the resource, taking its networking
97 URL. This means that the URI returned will use the same IP address or97 portion from `request` but the rest from `path`.
98 alias that the request came in on.
99 """98 """
100 script_name = request.META["SCRIPT_NAME"]99 scheme = "https" if request.is_secure() else "http"
101 return "%s://%s%s%s" % (100 return "%s://%s%s" % (scheme, request.get_host(), path)
102 "https" if request.is_secure() else "http",
103 request.get_host(), script_name, path)
104101
105102
106def strip_domain(hostname):103def strip_domain(hostname):
107104
=== 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-08 08:35:21 +0000
@@ -134,12 +134,13 @@
134 "http://example.com:1234/fred",134 "http://example.com:1234/fred",
135 build_absolute_uri(request, "/fred"))135 build_absolute_uri(request, "/fred"))
136136
137 def test_script_name_is_prefixed(self):137 def test_script_name_is_ignored(self):
138 # The script name is always prefixed to the given path.138 # The given path already includes the script_name, so the
139 # script_name passed in the request is not included again.
139 request = self.make_request(script_name="/foo/bar")140 request = self.make_request(script_name="/foo/bar")
140 self.assertEqual(141 self.assertEqual(
141 "http://example.com/foo/bar/fred",142 "http://example.com/foo/bar/fred",
142 build_absolute_uri(request, "/fred"))143 build_absolute_uri(request, "/foo/bar/fred"))
143144
144 def test_secure(self):145 def test_secure(self):
145 request = self.make_request(port=443, is_secure=True)146 request = self.make_request(port=443, is_secure=True)
@@ -153,21 +154,13 @@
153 "https://example.com:9443/fred",154 "https://example.com:9443/fred",
154 build_absolute_uri(request, "/fred"))155 build_absolute_uri(request, "/fred"))
155156
156 def test_no_leading_forward_slash(self):
157 # No attempt is made to ensure that the given path is separated from
158 # the to-be-prefixed path.
159 request = self.make_request(script_name="/foo")
160 self.assertEqual(
161 "http://example.com/foobar",
162 build_absolute_uri(request, "bar"))
163
164 def test_preserve_two_leading_slashes(self):157 def test_preserve_two_leading_slashes(self):
165 # Whilst this shouldn't ordinarily happen, two leading slashes in the158 # Whilst this shouldn't ordinarily happen, two leading slashes in the
166 # path should be preserved, and not treated specially.159 # path should be preserved, and not treated specially.
167 request = self.make_request(script_name="//foo")160 request = self.make_request()
168 self.assertEqual(161 self.assertEqual(
169 "http://example.com//foo/fred",162 "http://example.com//foo",
170 build_absolute_uri(request, "/fred"))163 build_absolute_uri(request, "//foo"))
171164
172165
173class TestStripDomain(TestCase):166class TestStripDomain(TestCase):