Merge lp:~rvb/maas/use-reverse into lp:maas/trunk

[Raphaël Badin]

Use Django's reverse in src/metadataserver/tests/test_api.py

This branch fixes a problem I spotted when I was working on the pre-req branch.

The metadata application should be hooked up (in src/maas/urls.py) like this:

urlpatterns = patterns('',
    url(r'^', include('maasserver.urls')),
    url(r'^metadata/', include('metadataserver.urls')),
)

Note the trailing slash at the end of '^metadata/'. Otherwise, reverse(...) does not return a correct path.

See https://docs.djangoproject.com/en/dev/topics/http/urls/#naming-url-patterns for details.

I refactored src/metadataserver/tests/test_api.py to use Django's reverse method everywhere. The main benefit is that now we exercise the name=... part in all the urls definitions in src/metadataserver/urls.py (augmented test coverage). Also, note that this whole branch is neutral in terms of added/removed lines.

[Francesco Banconi (frankban)]

Looks good.

review: Approve (code*)

[Gavin Panella (allenap)]

review: Approve

[lp:~rvb/maas/use-reverse]

661. By Raphaël Badin on 2012-06-20

Merged pxe-off-preseed into use-reverse.

662. By Raphaël Badin on 2012-06-20

Merged pxe-off-preseed into use-reverse.

663. By Raphaël Badin on 2012-06-20

Merged pxe-off-preseed into use-reverse.

664. By Raphaël Badin on 2012-06-20

Merged pxe-off-preseed into use-reverse.

=== modified file 'src/maas/urls.py'
--- src/maas/urls.py	2012-04-16 10:00:51 +0000
+++ src/maas/urls.py	2012-06-20 16:33:18 +0000
@@ -23,7 +23,7 @@
 
 urlpatterns = patterns('',
     url(r'^', include('maasserver.urls')),
-    url(r'^metadata', include('metadataserver.urls')),
+    url(r'^metadata/', include('metadataserver.urls')),
 )
 
 if settings.STATIC_LOCAL_SERVE:

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-06-20 16:33:18 +0000
+++ src/metadataserver/tests/test_api.py	2012-06-20 16:33:18 +0000
@@ -18,6 +18,7 @@
 
 from django.conf import settings
 from django.core.exceptions import PermissionDenied
+from django.core.urlresolvers import reverse
 from maasserver.enum import NODE_STATUS
 from maasserver.exceptions import (
     MAASAPINotFound,
@@ -128,33 +129,6 @@
         token = NodeKey.objects.get_token_for_node(node)
         return OAuthAuthenticatedClient(get_node_init_user(), token)
 
-    def make_url(self, path):
-        """Create an absolute URL for `path` on the metadata API.
-
-        :param path: Path within the metadata API to access.  Should start
-            with a slash.
-        :return: An absolute URL for the given path within the metadata
-            service tree.
-        """
-        assert path.startswith('/'), "Give absolute metadata API path."
-        # Root of the metadata API service.
-        metadata_root = "/metadata"
-        return metadata_root + path
-
-    def get(self, path, client=None):
-        """GET a resource from the metadata API.
-
-        :param path: Path within the metadata API to access.  Should start
-            with a slash.
-        :param token: If given, authenticate the request using this token.
-        :type token: oauth.oauth.OAuthToken
-        :return: A response to the GET request.
-        :rtype: django.http.HttpResponse
-        """
-        if client is None:
-            client = self.client
-        return client.get(self.make_url(path))
-
     def call_signal(self, client=None, version='latest', files={}, **kwargs):
         """Call the API's signal method.
 
@@ -177,37 +151,43 @@
         for name, content in files.items():
             params[name] = BytesIO(content)
             params[name].name = name
-        return client.post(self.make_url('/%s/' % version), params)
+        url = reverse('metadata-version', args=[version])
+        return client.post(url, params)
 
     def test_no_anonymous_access(self):
-        self.assertEqual(httplib.UNAUTHORIZED, self.get('/').status_code)
+        self.assertEqual(
+            httplib.UNAUTHORIZED,
+            self.client.get(reverse('metadata')).status_code)
 
     def test_metadata_index_shows_latest(self):
         client = self.make_node_client()
-        self.assertIn('latest', self.get('/', client).content)
+        self.assertIn('latest', client.get(reverse('metadata')).content)
 
     def test_metadata_index_shows_only_known_versions(self):
         client = self.make_node_client()
-        for item in self.get('/', client).content.splitlines():
+        for item in client.get(reverse('metadata')).content.splitlines():
             check_version(item)
         # The test is that we get here without exception.
         pass
 
     def test_version_index_shows_meta_data(self):
         client = self.make_node_client()
-        items = self.get('/latest/', client).content.splitlines()
+        url = reverse('metadata-version', args=['latest'])
+        items = client.get(url).content.splitlines()
         self.assertIn('meta-data', items)
 
     def test_version_index_does_not_show_user_data_if_not_available(self):
         client = self.make_node_client()
-        items = self.get('/latest/', client).content.splitlines()
+        url = reverse('metadata-version', args=['latest'])
+        items = client.get(url).content.splitlines()
         self.assertNotIn('user-data', items)
 
     def test_version_index_shows_user_data_if_available(self):
         node = factory.make_node()
         NodeUserData.objects.set_user_data(node, b"User data for node")
         client = self.make_node_client(node)
-        items = self.get('/latest/', client).content.splitlines()
+        url = reverse('metadata-version', args=['latest'])
+        items = client.get(url).content.splitlines()
         self.assertIn('user-data', items)
 
     def test_meta_data_view_lists_fields(self):
@@ -215,20 +195,23 @@
         user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
         node = factory.make_node(owner=user)
         client = self.make_node_client(node=node)
-        response = self.get('/latest/meta-data/', client)
+        url = reverse('metadata-meta-data', args=['latest', ''])
+        response = client.get(url)
         self.assertIn('text/plain', response['Content-Type'])
         self.assertItemsEqual(
             MetaDataHandler.fields, response.content.split())
 
     def test_meta_data_view_is_sorted(self):
         client = self.make_node_client()
-        response = self.get('/latest/meta-data/', client)
+        url = reverse('metadata-meta-data', args=['latest', ''])
+        response = client.get(url)
         attributes = response.content.split()
         self.assertEqual(sorted(attributes), attributes)
 
     def test_meta_data_unknown_item_is_not_found(self):
         client = self.make_node_client()
-        response = self.get('/latest/meta-data/UNKNOWN-ITEM-HA-HA-HA', client)
+        url = reverse('metadata-meta-data', args=['latest', 'UNKNOWN-ITEM'])
+        response = client.get(url)
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_get_attribute_producer_supports_all_fields(self):
@@ -239,7 +222,8 @@
     def test_meta_data_local_hostname_returns_hostname(self):
         hostname = factory.getRandomString()
         client = self.make_node_client(factory.make_node(hostname=hostname))
-        response = self.get('/latest/meta-data/local-hostname', client)
+        url = reverse('metadata-meta-data', args=['latest', 'local-hostname'])
+        response = client.get(url)
         self.assertEqual(
             (httplib.OK, hostname),
             (response.status_code, response.content.decode('ascii')))
@@ -248,7 +232,8 @@
     def test_meta_data_instance_id_returns_system_id(self):
         node = factory.make_node()
         client = self.make_node_client(node)
-        response = self.get('/latest/meta-data/instance-id', client)
+        url = reverse('metadata-meta-data', args=['latest', 'instance-id'])
+        response = client.get(url)
         self.assertEqual(
             (httplib.OK, node.system_id),
             (response.status_code, response.content.decode('ascii')))
@@ -259,39 +244,45 @@
         node = factory.make_node()
         NodeUserData.objects.set_user_data(node, data)
         client = self.make_node_client(node)
-        response = self.get('/latest/user-data', client)
+        response = client.get(reverse('metadata-user-data', args=['latest']))
         self.assertEqual('application/octet-stream', response['Content-Type'])
         self.assertIsInstance(response.content, str)
         self.assertEqual(
             (httplib.OK, data), (response.status_code, response.content))
 
     def test_user_data_for_node_without_user_data_returns_not_found(self):
-        response = self.get('/latest/user-data', self.make_node_client())
+        client = self.make_node_client()
+        response = client.get(reverse('metadata-user-data', args=['latest']))
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_public_keys_not_listed_for_node_without_public_keys(self):
-        response = self.get('/latest/meta-data/', self.make_node_client())
+        url = reverse('metadata-meta-data', args=['latest', ''])
+        client = self.make_node_client()
+        response = client.get(url)
         self.assertNotIn(
             'public-keys', response.content.decode('ascii').split('\n'))
 
     def test_public_keys_listed_for_node_with_public_keys(self):
         user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
         node = factory.make_node(owner=user)
-        response = self.get(
-            '/latest/meta-data/', self.make_node_client(node=node))
+        url = reverse('metadata-meta-data', args=['latest', ''])
+        client = self.make_node_client(node=node)
+        response = client.get(url)
         self.assertIn(
             'public-keys', response.content.decode('ascii').split('\n'))
 
     def test_public_keys_for_node_without_public_keys_returns_not_found(self):
-        response = self.get(
-            '/latest/meta-data/public-keys', self.make_node_client())
+        url = reverse('metadata-meta-data', args=['latest', 'public-keys'])
+        client = self.make_node_client()
+        response = client.get(url)
         self.assertEqual(httplib.NOT_FOUND, response.status_code)
 
     def test_public_keys_for_node_returns_list_of_keys(self):
         user, _ = factory.make_user_with_keys(n_keys=2, username='my-user')
         node = factory.make_node(owner=user)
-        response = self.get(
-            '/latest/meta-data/public-keys', self.make_node_client(node=node))
+        url = reverse('metadata-meta-data', args=['latest', 'public-keys'])
+        client = self.make_node_client(node=node)
+        response = client.get(url)
         self.assertEqual(httplib.OK, response.status_code)
         keys = SSHKey.objects.filter(user=user).values_list('key', flat=True)
         expected_response = '\n'.join(keys)
@@ -320,7 +311,8 @@
     def test_signaling_requires_status_code(self):
         node = factory.make_node(status=NODE_STATUS.COMMISSIONING)
         client = self.make_node_client(node=node)
-        response = client.post(self.make_url('/latest/'), {'op': 'signal'})
+        url = reverse('metadata-version', args=['latest'])
+        response = client.post(url, {'op': 'signal'})
         self.assertEqual(httplib.BAD_REQUEST, response.status_code)
 
     def test_signaling_rejects_unknown_status_code(self):
@@ -505,8 +497,10 @@
 
     def test_api_retrieves_node_metadata_by_mac(self):
         mac = factory.make_mac_address()
-        response = self.get(
-            '/latest/by-mac/%s/meta-data/instance-id/' % mac.mac_address)
+        url = reverse(
+            'metadata-meta-data-by-mac',
+            args=['latest', mac.mac_address, 'instance-id'])
+        response = self.client.get(url)
         self.assertEqual(
             (httplib.OK, mac.node.system_id),
             (response.status_code, response.content))
@@ -515,7 +509,9 @@
         mac = factory.make_mac_address()
         user_data = factory.getRandomString().encode('ascii')
         NodeUserData.objects.set_user_data(mac.node, user_data)
-        response = self.get('/latest/by-mac/%s/user-data' % mac.mac_address)
+        url = reverse(
+            'metadata-user-data-by-mac', args=['latest', mac.mac_address])
+        response = self.client.get(url)
         self.assertEqual(
             (httplib.OK, user_data),
             (response.status_code, response.content))
@@ -523,29 +519,33 @@
     def test_api_normally_disallows_anonymous_node_metadata_access(self):
         self.patch(settings, 'ALLOW_ANONYMOUS_METADATA_ACCESS', False)
         mac = factory.make_mac_address()
-        response = self.get(
-            '/latest/by-mac/%s/meta-data/instance-id/' % mac.mac_address)
+        url = reverse(
+            'metadata-meta-data-by-mac',
+            args=['latest', mac.mac_address, 'instance-id'])
+        response = self.client.get(url)
         self.assertEqual(httplib.FORBIDDEN, response.status_code)
 
     def test_netboot_off(self):
         node = factory.make_node(netboot=True)
         client = self.make_node_client(node=node)
-        response = client.post(
-            self.make_url('/latest/'), {'op': 'netboot_off'})
+        url = reverse('metadata-version', args=['latest'])
+        response = client.post(url, {'op': 'netboot_off'})
         node = reload_object(node)
         self.assertFalse(node.netboot, response)
 
     def test_netboot_on(self):
         node = factory.make_node(netboot=False)
         client = self.make_node_client(node=node)
-        response = client.post(self.make_url('/latest/'), {'op': 'netboot_on'})
+        url = reverse('metadata-version', args=['latest'])
+        response = client.post(url, {'op': 'netboot_on'})
         node = reload_object(node)
         self.assertTrue(node.netboot, response)
 
     def test_anonymous_netboot_off(self):
         node = factory.make_node(netboot=True)
-        anon_netboot_off_url = self.make_url(
-            '/latest/%s/edit/' % node.system_id)
+        anon_netboot_off_url = reverse(
+            'metadata-anon-node-edit', args=['latest',
+            node.system_id])
         response = self.client.post(
             anon_netboot_off_url, {'op': 'netboot_off'})
         node = reload_object(node)