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

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 1293
Merged at revision: 1294
Proposed branch: lp:~jtv/maas/1.2-bug-1075597
Merge into: lp:maas/1.2
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/1.2-bug-1075597
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+133429@code.launchpad.net

Commit message

Backport trunk r1334: don't include the script_name prefix in build_absolute_uri; it was already in the path it appends.

Description of the change

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Unmodified backport; self-approving.

review: Approve

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-08 06:40:03 +0000
3+++ src/maasserver/api.py 2012-11-08 09:34:41 +0000
4@@ -1867,7 +1867,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-08 06:40:03 +0000
20+++ src/maasserver/tests/test_api.py 2012-11-08 09:34:41 +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@@ -4292,19 +4296,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-02 09:48:02 +0000
100+++ src/maasserver/utils/__init__.py 2012-11-08 09:34:41 +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:48:02 +0000
135+++ src/maasserver/utils/tests/test_utils.py 2012-11-08 09:34:41 +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):

Subscribers

People subscribed via source and target branches

to status/vote changes: