Merge lp:~allenap/maas/api-uri-bug-1059645 into lp:maas

Proposed by Gavin Panella
Status: Merged
Approved by: Gavin Panella
Approved revision: 1331
Merged at revision: 1324
Proposed branch: lp:~allenap/maas/api-uri-bug-1059645
Merge into: lp:maas
Diff against target: 332 lines (+163/-16)
6 files modified
src/maasserver/api.py (+20/-4)
src/maasserver/apidoc.py (+3/-8)
src/maasserver/tests/test_api.py (+55/-0)
src/maasserver/tests/test_apidoc.py (+4/-4)
src/maasserver/utils/__init__.py (+18/-0)
src/maasserver/utils/tests/test_utils.py (+63/-0)
To merge this branch: bzr merge lp:~allenap/maas/api-uri-bug-1059645
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+132527@code.launchpad.net

This proposal supersedes a proposal from 2012-10-30.

Commit message

In the API description, make handler URIs absolute with respect to the request.

Previously, DEFAULT_MAAS_URL was always used.

Description of the change

I had to put a lot more work in to satisfy the review comments from jtv and jam, so I'm putting this back up for review.

The biggest change is that describe_handler now sets only a "path" field with the URI template; the describe() API function has to manufacture the "uri" field. I originally was using Django's HttpRequest.build_absolute_uri for this, but the additional tests I added showed that it was returning unexpected (and, imo, fairly useless) things, so I had to create a version that works.

These tests also highlighted that AnonFilesHandler was missing a resource_uri() method.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

This tests if the 'scheme' matches appropriately. Would you also want to test a nested case? (so rather than foobar.com/MAAS/1.0/api something like foo.com/bar/MAAS/1.0/api).

Otherwise it seems good to me.

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote : Posted in a previous version of this proposal

The comment “Make all API URLs absolute” in api.py seems a bit terse. Please leave a note so that some future visitor knows not to let this regress.

In test_handler_uris_are_absolute you make conditional assertions inside nested loops. That has its drawbacks: a failure stops the entire test, and you're not verifying that you're actually exercising the assertions. Better to gather your results and your expectations, and verify equality in one go.

Finally, in test_apidoc.py, why does the URL lack protocol and hostname?

Jeroen

review: Approve
Revision history for this message
Raphaël Badin (rvb) wrote :

Looks good to me.

[0]

272 + def make_request(
273 + self, host="example.com", port=80, script_name="", is_secure=False):

Weird formatting here.

review: Approve
lp:~allenap/maas/api-uri-bug-1059645 updated
1331. By Gavin Panella

Fix typo and formatting.

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-10-30 10:58:09 +0000
3+++ src/maasserver/api.py 2012-11-02 09:26:19 +0000
4@@ -164,6 +164,7 @@
5 from maasserver.server_address import get_maas_facing_server_address
6 from maasserver.utils import (
7 absolute_reverse,
8+ build_absolute_uri,
9 map_enum,
10 strip_domain,
11 )
12@@ -1021,6 +1022,10 @@
13
14 get = operation(idempotent=True, exported_as='get')(get_file)
15
16+ @classmethod
17+ def resource_uri(cls, *args, **kwargs):
18+ return ('files_handler', [])
19+
20
21 class FilesHandler(OperationsHandler):
22 """File management operations."""
23@@ -1935,12 +1940,23 @@
24 Returns a JSON object describing the whole MAAS API.
25 """
26 from maasserver import urls_api as urlconf
27+ resources = [
28+ describe_resource(resource)
29+ for resource in find_api_resources(urlconf)
30+ ]
31+ # Make all URIs absolute. Clients - maas-cli in particular - expect that
32+ # all handler URIs are absolute, not just paths. The handler URIs returned
33+ # by describe_resource() are relative paths.
34+ absolute = partial(build_absolute_uri, request)
35+ for resource in resources:
36+ for handler_type in "anon", "auth":
37+ handler = resource[handler_type]
38+ if handler is not None:
39+ handler["uri"] = absolute(handler["path"])
40+ # Package it all up.
41 description = {
42 "doc": "MAAS API",
43- "resources": [
44- describe_resource(resource)
45- for resource in find_api_resources(urlconf)
46- ],
47+ "resources": resources,
48 }
49 # For backward compatibility, add "handlers" as an alias for all not-None
50 # anon and auth handlers in "resources".
51
52=== modified file 'src/maasserver/apidoc.py'
53--- src/maasserver/apidoc.py 2012-10-06 22:42:45 +0000
54+++ src/maasserver/apidoc.py 2012-11-02 09:26:19 +0000
55@@ -19,9 +19,7 @@
56
57 from inspect import getdoc
58 from itertools import izip_longest
59-from urlparse import urljoin
60
61-from django.conf import settings
62 from django.core.urlresolvers import (
63 get_resolver,
64 RegexURLPattern,
65@@ -137,11 +135,8 @@
66 if isinstance(handler, BaseHandler):
67 handler = type(handler)
68
69- uri_template = generate_doc(handler).resource_uri_template
70- if uri_template is None:
71- uri_template = settings.DEFAULT_MAAS_URL
72- else:
73- uri_template = urljoin(settings.DEFAULT_MAAS_URL, uri_template)
74+ path = generate_doc(handler).resource_uri_template
75+ path = "" if path is None else path
76
77 resource_uri = getattr(handler, "resource_uri", lambda: ())
78 view_name, uri_params, uri_kw = merge(resource_uri(), (None, (), {}))
79@@ -154,7 +149,7 @@
80 "doc": getdoc(handler),
81 "name": handler.__name__,
82 "params": uri_params,
83- "uri": uri_template,
84+ "path": path,
85 }
86
87
88
89=== modified file 'src/maasserver/tests/test_api.py'
90--- src/maasserver/tests/test_api.py 2012-10-30 10:58:09 +0000
91+++ src/maasserver/tests/test_api.py 2012-11-02 09:26:19 +0000
92@@ -27,10 +27,12 @@
93 import httplib
94 from itertools import izip
95 import json
96+from operator import itemgetter
97 import os
98 import random
99 import shutil
100 import sys
101+from urlparse import urlparse
102
103 from apiclient.maas_client import MAASClient
104 from celery.app import app_or_default
105@@ -38,9 +40,11 @@
106 from django.contrib.auth.models import AnonymousUser
107 from django.core.urlresolvers import reverse
108 from django.http import QueryDict
109+from django.test.client import RequestFactory
110 from fixtures import Fixture
111 from maasserver import api
112 from maasserver.api import (
113+ describe,
114 DISPLAYED_NODEGROUP_FIELDS,
115 extract_constraints,
116 extract_oauth_key,
117@@ -130,10 +134,15 @@
118 from provisioningserver.pxe import tftppath
119 from provisioningserver.testing.boot_images import make_boot_image_params
120 from testresources import FixtureResource
121+from testscenarios import multiply_scenarios
122 from testtools.matchers import (
123+ AfterPreprocessing,
124 AllMatch,
125 Contains,
126 Equals,
127+ Is,
128+ MatchesAll,
129+ MatchesAny,
130 MatchesListwise,
131 MatchesStructure,
132 StartsWith,
133@@ -4349,3 +4358,49 @@
134 self.assertSetEqual(
135 {"doc", "handlers", "resources"}, set(description))
136 self.assertIsInstance(description["handlers"], list)
137+
138+
139+class TestDescribeAbsoluteURIs(AnonAPITestCase):
140+ """Tests for the `describe` view's URI manipulation."""
141+
142+ scenarios_schemes = (
143+ ("http", dict(scheme="http")),
144+ ("https", dict(scheme="https")),
145+ )
146+
147+ scenarios_paths = (
148+ ("script-at-root", dict(script_name="", path_info="")),
149+ ("script-below-root-1", dict(script_name="/foo/bar", path_info="")),
150+ ("script-below-root-2", dict(script_name="/foo", path_info="/bar")),
151+ )
152+
153+ scenarios = multiply_scenarios(
154+ scenarios_schemes, scenarios_paths)
155+
156+ def test_handler_uris_are_absolute(self):
157+ server = factory.make_name("server").lower()
158+ extra = {
159+ "PATH_INFO": self.path_info,
160+ "SCRIPT_NAME": self.script_name,
161+ "SERVER_NAME": server,
162+ "wsgi.url_scheme": self.scheme,
163+ }
164+ request = RequestFactory().get(
165+ "/%s/describe" % factory.make_name("path"), **extra)
166+ response = describe(request)
167+ self.assertEqual(httplib.OK, response.status_code, response.content)
168+ description = json.loads(response.content)
169+ expected_uri = AfterPreprocessing(
170+ urlparse, MatchesStructure(
171+ scheme=Equals(self.scheme), hostname=Equals(server),
172+ # The path is always the script name followed by "api/"
173+ # because all API calls are within the "api" tree.
174+ path=StartsWith(self.script_name + "/api/")))
175+ expected_handler = MatchesAny(
176+ Is(None), AfterPreprocessing(itemgetter("uri"), expected_uri))
177+ expected_resource = MatchesAll(
178+ AfterPreprocessing(itemgetter("anon"), expected_handler),
179+ AfterPreprocessing(itemgetter("auth"), expected_handler))
180+ resources = description["resources"]
181+ self.assertNotEqual([], resources)
182+ self.assertThat(resources, AllMatch(expected_resource))
183
184=== modified file 'src/maasserver/tests/test_apidoc.py'
185--- src/maasserver/tests/test_apidoc.py 2012-10-08 10:23:16 +0000
186+++ src/maasserver/tests/test_apidoc.py 2012-11-02 09:26:19 +0000
187@@ -203,7 +203,7 @@
188 observed = describe_handler(ExampleHandler)
189 # The description contains several entries.
190 self.assertSetEqual(
191- {"actions", "doc", "name", "params", "uri"},
192+ {"actions", "doc", "name", "params", "path"},
193 set(observed))
194 self.assertEqual(ExampleHandler.__doc__, observed["doc"])
195 self.assertEqual(ExampleHandler.__name__, observed["name"])
196@@ -231,11 +231,11 @@
197 }
198 self.assertSetEqual(expected_actions, observed_actions)
199 self.assertSetEqual({"system_id"}, set(description["params"]))
200- # The URI is a URI Template <http://tools.ietf.org/html/rfc6570>, the
201+ # The path is a URI Template <http://tools.ietf.org/html/rfc6570>, the
202 # components of which correspond to the parameters declared.
203 self.assertEqual(
204- "http://example.com/api/1.0/nodes/{system_id}/",
205- description["uri"])
206+ "/api/1.0/nodes/{system_id}/",
207+ description["path"])
208
209 def test_describe_resource_anonymous_resource(self):
210 # When the resource does not require authentication, any configured
211
212=== modified file 'src/maasserver/utils/__init__.py'
213--- src/maasserver/utils/__init__.py 2012-10-25 12:17:28 +0000
214+++ src/maasserver/utils/__init__.py 2012-11-02 09:26:19 +0000
215@@ -12,6 +12,7 @@
216 __metaclass__ = type
217 __all__ = [
218 'absolute_reverse',
219+ 'build_absolute_uri',
220 'get_db_state',
221 'ignore_unused',
222 'map_enum',
223@@ -85,6 +86,23 @@
224 return url
225
226
227+def build_absolute_uri(request, path):
228+ """Given a full app-relative path, returns an absolute URI.
229+
230+ The path ordinarily starts with a forward-slash... imagine that you've
231+ magically chroot'ed to your Django application; the path is the absolute
232+ path to the view within that environment.
233+
234+ The URI returned uses the request to figure out how to make an absolute
235+ URL. This means that the URI returned will use the same IP address or
236+ alias that the request came in on.
237+ """
238+ script_name = request.META["SCRIPT_NAME"]
239+ return "%s://%s%s%s" % (
240+ "https" if request.is_secure() else "http",
241+ request.get_host(), script_name, path)
242+
243+
244 def strip_domain(hostname):
245 """Return `hostname` with the domain part removed."""
246 return hostname.split('.', 1)[0]
247
248=== modified file 'src/maasserver/utils/tests/test_utils.py'
249--- src/maasserver/utils/tests/test_utils.py 2012-10-25 12:17:28 +0000
250+++ src/maasserver/utils/tests/test_utils.py 2012-11-02 09:26:19 +0000
251@@ -16,11 +16,13 @@
252
253 from django.conf import settings
254 from django.core.urlresolvers import reverse
255+from django.http import HttpRequest
256 from maasserver.enum import NODE_STATUS_CHOICES
257 from maasserver.testing.factory import factory
258 from maasserver.testing.testcase import TestCase as DjangoTestCase
259 from maasserver.utils import (
260 absolute_reverse,
261+ build_absolute_uri,
262 get_db_state,
263 map_enum,
264 strip_domain,
265@@ -107,6 +109,67 @@
266 self.assertEqual(status, get_db_state(node, 'status'))
267
268
269+class TestBuildAbsoluteURI(TestCase):
270+ """Tests for `build_absolute_uri`."""
271+
272+ def make_request(self, host="example.com", port=80, script_name="",
273+ is_secure=False):
274+ """Return a :class:`HttpRequest` with the given parameters."""
275+ request = HttpRequest()
276+ request.META["SERVER_NAME"] = host
277+ request.META["SERVER_PORT"] = port
278+ request.META["SCRIPT_NAME"] = script_name
279+ request.is_secure = lambda: is_secure
280+ return request
281+
282+ def test_simple(self):
283+ request = self.make_request()
284+ self.assertEqual(
285+ "http://example.com/fred",
286+ build_absolute_uri(request, "/fred"))
287+
288+ def test_different_port(self):
289+ request = self.make_request(port=1234)
290+ self.assertEqual(
291+ "http://example.com:1234/fred",
292+ build_absolute_uri(request, "/fred"))
293+
294+ def test_script_name_is_prefixed(self):
295+ # The script name is always prefixed to the given path.
296+ request = self.make_request(script_name="/foo/bar")
297+ self.assertEqual(
298+ "http://example.com/foo/bar/fred",
299+ build_absolute_uri(request, "/fred"))
300+
301+ def test_secure(self):
302+ request = self.make_request(port=443, is_secure=True)
303+ self.assertEqual(
304+ "https://example.com/fred",
305+ build_absolute_uri(request, "/fred"))
306+
307+ def test_different_port_and_secure(self):
308+ request = self.make_request(port=9443, is_secure=True)
309+ self.assertEqual(
310+ "https://example.com:9443/fred",
311+ build_absolute_uri(request, "/fred"))
312+
313+ def test_no_leading_forward_slash(self):
314+ # No attempt is made to ensure that the given path is separated from
315+ # the to-be-prefixed path.
316+ request = self.make_request(script_name="/foo")
317+ self.assertEqual(
318+ "http://example.com/foobar",
319+ build_absolute_uri(request, "bar"))
320+
321+ def test_preserve_two_leading_slashes(self):
322+ # Whilst this shouldn't ordinarily happen, two leading slashes in the
323+ # path should be preserved, and not treated specially.
324+ request = self.make_request(script_name="//foo")
325+ self.assertEqual(
326+ "http://example.com//foo/fred",
327+ build_absolute_uri(request, "/fred"))
328+
329+
330 class TestStripDomain(TestCase):
331
332 def test_strip_domain(self):