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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 1335
Merged at revision: 1334
Proposed branch: lp:~jtv/maas/bug-1075597
Merge into: lp:maas
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
1=== modified file 'src/maasserver/api.py'
2--- src/maasserver/api.py 2012-11-07 17:08:42 +0000
3+++ src/maasserver/api.py 2012-11-08 08:35:21 +0000
4@@ -1961,7 +1961,11 @@
5 def describe(request):
6 """Return a description of the whole MAAS API.
7
8- Returns a JSON object describing the whole MAAS API.
9+ :param request: The http request for this document. This is used to
10+ derive the URL where the client expects to see the MAAS API.
11+ :return: A JSON object describing the whole MAAS API. Links to the API
12+ will use the same scheme and hostname that the client used in
13+ `request`.
14 """
15 from maasserver import urls_api as urlconf
16 resources = [
17
18=== modified file 'src/maasserver/tests/test_api.py'
19--- src/maasserver/tests/test_api.py 2012-11-07 17:08:42 +0000
20+++ src/maasserver/tests/test_api.py 2012-11-08 08:35:21 +0000
21@@ -38,7 +38,11 @@
22 from celery.app import app_or_default
23 from django.conf import settings
24 from django.contrib.auth.models import AnonymousUser
25-from django.core.urlresolvers import reverse
26+import django.core.urlresolvers
27+from django.core.urlresolvers import (
28+ reverse,
29+ get_script_prefix,
30+ )
31 from django.http import QueryDict
32 from django.test.client import RequestFactory
33 from fixtures import Fixture
34@@ -4448,19 +4452,53 @@
35 scenarios = multiply_scenarios(
36 scenarios_schemes, scenarios_paths)
37
38- def test_handler_uris_are_absolute(self):
39- server = factory.make_name("server").lower()
40- extra = {
41+ def make_params(self):
42+ """Create parameters for http request, based on current scenario."""
43+ return {
44 "PATH_INFO": self.path_info,
45 "SCRIPT_NAME": self.script_name,
46- "SERVER_NAME": server,
47+ "SERVER_NAME": factory.make_name('server').lower(),
48 "wsgi.url_scheme": self.scheme,
49- }
50- request = RequestFactory().get(
51- "/%s/describe" % factory.make_name("path"), **extra)
52+ }
53+
54+ def get_description(self, params):
55+ """GET the API description (at a random API path), as JSON."""
56+ path = '/%s/describe' % factory.make_name('path')
57+ request = RequestFactory().get(path, **params)
58 response = describe(request)
59- self.assertEqual(httplib.OK, response.status_code, response.content)
60- description = json.loads(response.content)
61+ self.assertEqual(
62+ httplib.OK, response.status_code,
63+ "API description failed with code %s:\n%s"
64+ % (response.status_code, response.content))
65+ return json.loads(response.content)
66+
67+ def patch_script_prefix(self, script_name):
68+ """Patch up Django's and Piston's notion of the script_name prefix.
69+
70+ This manipulates how Piston gets Django's version of script_name
71+ which it needs so that it can prefix script_name to URL paths.
72+ """
73+ # Patching up get_script_prefix doesn't seem to do the trick,
74+ # and patching it in the right module requires unwarranted
75+ # intimacy with Piston. So just go through the proper call and
76+ # set the prefix. But clean this up after the test or it will
77+ # break other tests!
78+ original_prefix = get_script_prefix()
79+ self.addCleanup(
80+ django.core.urlresolvers.set_script_prefix, original_prefix)
81+ django.core.urlresolvers.set_script_prefix(script_name)
82+
83+ def test_handler_uris_are_absolute(self):
84+ params = self.make_params()
85+ server = params['SERVER_NAME']
86+
87+ # Without this, the test wouldn't be able to detect accidental
88+ # duplication of the script_name portion of the URL path:
89+ # /MAAS/MAAS/api/...
90+ self.patch_script_prefix(self.script_name)
91+
92+ description = self.get_description(params)
93+
94 expected_uri = AfterPreprocessing(
95 urlparse, MatchesStructure(
96 scheme=Equals(self.scheme), hostname=Equals(server),
97
98=== modified file 'src/maasserver/utils/__init__.py'
99--- src/maasserver/utils/__init__.py 2012-11-01 11:45:29 +0000
100+++ src/maasserver/utils/__init__.py 2012-11-08 08:35:21 +0000
101@@ -87,20 +87,17 @@
102
103
104 def build_absolute_uri(request, path):
105- """Given a full app-relative path, returns an absolute URI.
106-
107- The path ordinarily starts with a forward-slash... imagine that you've
108- magically chroot'ed to your Django application; the path is the absolute
109- path to the view within that environment.
110-
111- The URI returned uses the request to figure out how to make an absolute
112- URL. This means that the URI returned will use the same IP address or
113- alias that the request came in on.
114+ """Return absolute URI corresponding to given absolute path.
115+
116+ :param request: An http request to the API. This is needed in order to
117+ figure out how the client is used to addressing
118+ the API on the network.
119+ :param path: The absolute http path to a given resource.
120+ :return: Full, absolute URI to the resource, taking its networking
121+ portion from `request` but the rest from `path`.
122 """
123- script_name = request.META["SCRIPT_NAME"]
124- return "%s://%s%s%s" % (
125- "https" if request.is_secure() else "http",
126- request.get_host(), script_name, path)
127+ scheme = "https" if request.is_secure() else "http"
128+ return "%s://%s%s" % (scheme, request.get_host(), path)
129
130
131 def strip_domain(hostname):
132
133=== modified file 'src/maasserver/utils/tests/test_utils.py'
134--- src/maasserver/utils/tests/test_utils.py 2012-11-02 09:22:01 +0000
135+++ src/maasserver/utils/tests/test_utils.py 2012-11-08 08:35:21 +0000
136@@ -134,12 +134,13 @@
137 "http://example.com:1234/fred",
138 build_absolute_uri(request, "/fred"))
139
140- def test_script_name_is_prefixed(self):
141- # The script name is always prefixed to the given path.
142+ def test_script_name_is_ignored(self):
143+ # The given path already includes the script_name, so the
144+ # script_name passed in the request is not included again.
145 request = self.make_request(script_name="/foo/bar")
146 self.assertEqual(
147 "http://example.com/foo/bar/fred",
148- build_absolute_uri(request, "/fred"))
149+ build_absolute_uri(request, "/foo/bar/fred"))
150
151 def test_secure(self):
152 request = self.make_request(port=443, is_secure=True)
153@@ -153,21 +154,13 @@
154 "https://example.com:9443/fred",
155 build_absolute_uri(request, "/fred"))
156
157- def test_no_leading_forward_slash(self):
158- # No attempt is made to ensure that the given path is separated from
159- # the to-be-prefixed path.
160- request = self.make_request(script_name="/foo")
161- self.assertEqual(
162- "http://example.com/foobar",
163- build_absolute_uri(request, "bar"))
164-
165 def test_preserve_two_leading_slashes(self):
166 # Whilst this shouldn't ordinarily happen, two leading slashes in the
167 # path should be preserved, and not treated specially.
168- request = self.make_request(script_name="//foo")
169+ request = self.make_request()
170 self.assertEqual(
171- "http://example.com//foo/fred",
172- build_absolute_uri(request, "/fred"))
173+ "http://example.com//foo",
174+ build_absolute_uri(request, "//foo"))
175
176
177 class TestStripDomain(TestCase):