launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06326
[Merge] lp:~jtv/maas/nodes-id-filter into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/nodes-id-filter into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/nodes-id-filter/+merge/92503
This adds an “id” parameter to the “list” operation on the nodes set in the REST API, for use by our Juju machine provider. So besides http://<host>/api/nodes/?op=list you can now also do http://<host>/api/nodes/?op=list&id=node123
Only nodes with matching ids will be returned. Why am I using the plural? Because you can also repeat the parameter:
http://<host>/api/nodes/?op=list&id=node123&id=node234&id=node345
The collective id arguments form an additional filter. If some of the ids do not match any nodes that the user might be able to see, that just diminishes the result set. There is no error for this condition.
I reorganized the API tests a bit along the way:
* Separated test cases for /api/nodes/ and /api/nodes/<node>/
* Stripped node/nodes distinction from test method names, but inserted operation name.
* Added some tests, so make test names more specific.
* De-indented some over-indented dicts, and added trailing commas to the final items.
* Hard-coded ordering: removed where accidental, tested explicitly where intentional.
I rejigged the model tests a bit as well: the make_user_and_node helper wasn't quite right for what I needed, and it sort of drifted away from its original state in stages.
--
https://code.launchpad.net/~jtv/maas/nodes-id-filter/+merge/92503
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/nodes-id-filter into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-02-10 11:50:35 +0000
+++ src/maasserver/api.py 2012-02-10 16:07:33 +0000
@@ -259,8 +259,12 @@
@api_exported('list', 'GET')
def list(self, request):
- """Read all Nodes."""
- return Node.objects.get_visible_nodes(request.user).order_by('id')
+ """List Nodes visible to the user, optionally filtered by id."""
+ match_ids = request.GET.getlist('id')
+ if match_ids == []:
+ match_ids = None
+ nodes = Node.objects.get_visible_nodes(request.user, ids=match_ids)
+ return nodes.order_by('id')
@api_exported('new', 'POST')
def new(self, request):
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-02-10 08:49:01 +0000
+++ src/maasserver/models.py 2012-02-10 16:07:33 +0000
@@ -134,11 +134,13 @@
class NodeManager(models.Manager):
"""A utility to manage the collection of Nodes."""
- def get_visible_nodes(self, user):
+ def get_visible_nodes(self, user, ids=None):
"""Fetch all the Nodes visible by a User_.
:param user: The user that should be used in the permission check.
:type user: User_
+ :param ids: If given, limit result to nodes with these system_ids.
+ :type ids: Sequence.
.. _User: https://
docs.djangoproject.com/en/dev/topics/auth/
@@ -146,10 +148,14 @@
"""
if user.is_superuser:
- return self.all()
+ visible_nodes = self.all()
else:
- return self.filter(
+ visible_nodes = self.filter(
models.Q(owner__isnull=True) | models.Q(owner=user))
+ if ids is None:
+ return visible_nodes
+ else:
+ return visible_nodes.filter(system_id__in=ids)
def get_visible_node_or_404(self, system_id, user):
"""Fetch a `Node` by system_id. Raise exceptions if no `Node` with
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-02-09 17:09:23 +0000
+++ src/maasserver/testing/factory.py 2012-02-10 16:07:33 +0000
@@ -28,7 +28,7 @@
class Factory():
- def getRandomString(self, size):
+ def getRandomString(self, size=10):
return "".join(
random.choice(string.letters + string.digits)
for x in xrange(size))
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-02-08 15:20:37 +0000
+++ src/maasserver/tests/test_api.py 2012-02-10 16:07:33 +0000
@@ -94,6 +94,11 @@
self.client = OAuthAuthenticatedClient(self.logged_in_user)
+def extract_system_ids(parsed_result):
+ """List the system_ids of the nodes in `parsed_result`."""
+ return [node.get('system_id') for node in parsed_result]
+
+
class NodeAPILoggedInTest(LoggedInTestCase):
def test_nodes_GET_logged_in(self):
@@ -103,27 +108,13 @@
parsed_result = json.loads(response.content)
self.assertEqual(httplib.OK, response.status_code)
- self.assertEqual(
- [node.system_id], [node['system_id'] for node in parsed_result])
-
-
-class NodeAPITest(APITestMixin):
-
- def test_nodes_GET(self):
- # The api allows for fetching the list of Nodes.
- node1 = factory.make_node()
- node2 = factory.make_node(
- set_hostname=True, status=NODE_STATUS.DEPLOYED,
- owner=self.logged_in_user)
- response = self.client.get('/api/nodes/', {'op': 'list'})
- parsed_result = json.loads(response.content)
-
- self.assertEqual(httplib.OK, response.status_code)
- self.assertEqual(2, len(parsed_result))
- self.assertEqual(node1.system_id, parsed_result[0]['system_id'])
- self.assertEqual(node2.system_id, parsed_result[1]['system_id'])
-
- def test_node_GET(self):
+ self.assertEqual([node.system_id], extract_system_ids(parsed_result))
+
+
+class TestNodeAPI(APITestMixin):
+ """Tests for /api/nodes/<node>/."""
+
+ def test_GET_returns_node(self):
# The api allows for fetching a single Node (using system_id).
node = factory.make_node(set_hostname=True)
response = self.client.get('/api/nodes/%s/' % node.system_id)
@@ -133,7 +124,7 @@
self.assertEqual(node.hostname, parsed_result['hostname'])
self.assertEqual(node.system_id, parsed_result['system_id'])
- def test_node_GET_non_visible_node(self):
+ def test_GET_refuses_to_access_invisible_node(self):
# The request to fetch a single node is denied if the node isn't
# visible by the user.
other_node = factory.make_node(
@@ -143,89 +134,14 @@
self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
- def test_node_GET_not_found(self):
+ def test_GET_refuses_to_access_nonexistent_node(self):
# When fetching a Node, the api returns a 'Not Found' (404) error
# if no node is found.
response = self.client.get('/api/nodes/invalid-uuid/')
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_nodes_POST(self):
- # The API allows a Node to be created and associated with MAC
- # Addresses.
- response = self.client.post(
- '/api/nodes/',
- {
- 'op': 'new',
- 'hostname': 'diane',
- 'after_commissioning_action': '2',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff']
- })
- parsed_result = json.loads(response.content)
-
- self.assertEqual(httplib.OK, response.status_code)
- self.assertEqual(
- 'application/json; charset=utf-8', response['Content-Type'])
- self.assertEqual('diane', parsed_result['hostname'])
- self.assertEqual(41, len(parsed_result.get('system_id')))
- self.assertEqual(1, Node.objects.filter(hostname='diane').count())
- node = Node.objects.get(hostname='diane')
- self.assertEqual(2, node.after_commissioning_action)
- self.assertSequenceEqual(
- ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
- [mac.mac_address for mac in node.macaddress_set.all()])
-
- def test_nodes_POST_no_operation(self):
- # If there is no operation ('op=operation_name') specified in the
- # request data, a 'Bad request' response is returned.
- response = self.client.post(
- '/api/nodes/',
- {
- 'hostname': 'diane',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
- })
-
- self.assertEqual(httplib.BAD_REQUEST, response.status_code)
- self.assertEqual(
- 'text/html; charset=utf-8', response['Content-Type'])
- self.assertEqual('Unknown operation.', response.content)
-
- def test_nodes_POST_bad_operation(self):
- # If the operation ('op=operation_name') specified in the
- # request data is unknown, a 'Bad request' response is returned.
- response = self.client.post(
- '/api/nodes/',
- {
- 'op': 'invalid_operation',
- 'hostname': 'diane',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
- })
-
- self.assertEqual(httplib.BAD_REQUEST, response.status_code)
- self.assertEqual(
- "Unknown operation: 'invalid_operation'.", response.content)
-
- def test_nodes_POST_invalid(self):
- # If the data provided to create a node with MAC Addresse is invalid,
- # a 'Bad request' response is returned.
- response = self.client.post(
- '/api/nodes/',
- {
- 'op': 'new',
- 'hostname': 'diane',
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid']
- })
- parsed_result = json.loads(response.content)
-
- self.assertEqual(httplib.BAD_REQUEST, response.status_code)
- self.assertEqual(
- 'application/json; charset=utf-8', response['Content-Type'])
- self.assertEqual(['mac_addresses'], list(parsed_result))
- self.assertEqual(
- ["One or more MAC Addresses is invalid."],
- parsed_result['mac_addresses'])
-
- def test_node_PUT(self):
+ def test_PUT_updates_node(self):
# The api allows to update a Node.
node = factory.make_node(hostname='diane')
response = self.client.put(
@@ -237,7 +153,7 @@
self.assertEqual(0, Node.objects.filter(hostname='diane').count())
self.assertEqual(1, Node.objects.filter(hostname='francis').count())
- def test_node_resource_uri(self):
+ def test_resource_uri_points_back_at_node(self):
# When a Node is returned by the API, the field 'resource_uri'
# provides the URI for this Node.
node = factory.make_node(hostname='diane')
@@ -249,7 +165,7 @@
'/api/nodes/%s/' % (parsed_result['system_id']),
parsed_result['resource_uri'])
- def test_node_PUT_invalid(self):
+ def test_PUT_rejects_invalid_data(self):
# If the data provided to update a node is invalid, a 'Bad request'
# response is returned.
node = factory.make_node(hostname='diane')
@@ -259,7 +175,7 @@
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
- def test_node_PUT_non_visible_node(self):
+ def test_PUT_refuses_to_update_invisible_node(self):
# The request to update a single node is denied if the node isn't
# visible by the user.
other_node = factory.make_node(
@@ -269,24 +185,14 @@
self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
- def test_node_PUT_not_found(self):
+ def test_PUT_refuses_to_update_nonexistent_node(self):
# When updating a Node, the api returns a 'Not Found' (404) error
# if no node is found.
response = self.client.put('/api/nodes/no-node-here/')
self.assertEqual(httplib.NOT_FOUND, response.status_code)
- def test_node_DELETE_non_visible_node(self):
- # The request to delete a single node is denied if the node isn't
- # visible by the user.
- other_node = factory.make_node(
- status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
-
- response = self.client.delete('/api/nodes/%s/' % other_node.system_id)
-
- self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
-
- def test_node_DELETE(self):
+ def test_DELETE_deletes_node(self):
# The api allows to delete a Node.
node = factory.make_node(set_hostname=True)
system_id = node.system_id
@@ -296,7 +202,17 @@
self.assertEqual(
[], list(Node.objects.filter(system_id=system_id)))
- def test_node_DELETE_not_found(self):
+ def test_DELETE_refuses_to_delete_invisible_node(self):
+ # The request to delete a single node is denied if the node isn't
+ # visible by the user.
+ other_node = factory.make_node(
+ status=NODE_STATUS.DEPLOYED, owner=factory.make_user())
+
+ response = self.client.delete('/api/nodes/%s/' % other_node.system_id)
+
+ self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
+
+ def test_DELETE_refuses_to_delete_nonexistent_node(self):
# When deleting a Node, the api returns a 'Not Found' (404) error
# if no node is found.
response = self.client.delete('/api/nodes/no-node-here/')
@@ -304,6 +220,158 @@
self.assertEqual(httplib.NOT_FOUND, response.status_code)
+class TestNodesAPI(APITestMixin):
+ """Tests for /api/nodes/."""
+
+ def test_GET_list_lists_nodes(self):
+ # The api allows for fetching the list of Nodes.
+ node1 = factory.make_node()
+ node2 = factory.make_node(
+ set_hostname=True, status=NODE_STATUS.DEPLOYED,
+ owner=self.logged_in_user)
+ response = self.client.get('/api/nodes/', {'op': 'list'})
+ parsed_result = json.loads(response.content)
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertItemsEqual(
+ [node1.system_id, node2.system_id],
+ extract_system_ids(parsed_result))
+
+ def test_GET_list_without_nodes_returns_empty_list(self):
+ # If there are no nodes to list, the "list" op still works but
+ # returns an empty list.
+ response = self.client.get('/api/nodes/', {'op': 'list'})
+ self.assertItemsEqual([], json.loads(response.content))
+
+ def test_GET_list_orders_by_id(self):
+ # Nodes are returned in id order.
+ nodes = [factory.make_node() for counter in range(3)]
+ response = self.client.get('/api/nodes/', {'op': 'list'})
+ parsed_result = json.loads(response.content)
+ self.assertSequenceEqual(
+ [node.system_id for node in nodes],
+ extract_system_ids(parsed_result))
+
+ def test_GET_list_with_id_returns_matching_nodes(self):
+ # The "list" operation takes optional "id" parameters. Only
+ # nodes with matching ids will be returned.
+ ids = [factory.make_node().system_id for counter in range(3)]
+ matching_id = ids[0]
+ response = self.client.get('/api/nodes/', {
+ 'op': 'list',
+ 'id': [matching_id],
+ })
+ parsed_result = json.loads(response.content)
+ self.assertItemsEqual(
+ [matching_id], extract_system_ids(parsed_result))
+
+ def test_GET_list_with_nonexistent_id_returns_empty_list(self):
+ # Trying to list a nonexistent node id returns a list containing
+ # no nodes -- even if other (non-matching) nodes exist.
+ existing_id = factory.make_node().system_id
+ nonexistent_id = existing_id + factory.getRandomString()
+ response = self.client.get('/api/nodes/', {
+ 'op': 'list',
+ 'id': [nonexistent_id],
+ })
+ self.assertItemsEqual([], json.loads(response.content))
+
+ def test_GET_list_with_ids_orders_by_id(self):
+ # Even when ids are passed to "list," nodes are returned in id
+ # order, not necessarily in the order of the id arguments.
+ ids = [factory.make_node().system_id for counter in range(3)]
+ response = self.client.get('/api/nodes/', {
+ 'op': 'list',
+ 'id': list(reversed(ids)),
+ })
+ parsed_result = json.loads(response.content)
+ self.assertSequenceEqual(ids, extract_system_ids(parsed_result))
+
+ def test_GET_list_with_some_matching_ids_returns_matching_nodes(self):
+ # If some nodes match the requested ids and some don't, only the
+ # matching ones are returned.
+ existing_id = factory.make_node().system_id
+ nonexistent_id = existing_id + factory.getRandomString()
+ response = self.client.get('/api/nodes/', {
+ 'op': 'list',
+ 'id': [existing_id, nonexistent_id],
+ })
+ parsed_result = json.loads(response.content)
+ self.assertItemsEqual(
+ [existing_id], extract_system_ids(parsed_result))
+
+ def test_POST_new_creates_node(self):
+ # The API allows a Node to be created and associated with MAC
+ # Addresses.
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'op': 'new',
+ 'hostname': 'diane',
+ 'after_commissioning_action': '2',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ })
+ parsed_result = json.loads(response.content)
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertIn('application/json', response['Content-Type'])
+ self.assertEqual('diane', parsed_result['hostname'])
+ self.assertNotEqual(0, len(parsed_result.get('system_id')))
+ [diane] = Node.objects.filter(hostname='diane')
+ self.assertEqual(2, diane.after_commissioning_action)
+ self.assertItemsEqual(
+ ['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
+ [mac.mac_address for mac in diane.macaddress_set.all()])
+
+ def test_POST_fails_without_operation(self):
+ # If there is no operation ('op=operation_name') specified in the
+ # request data, a 'Bad request' response is returned.
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'hostname': 'diane',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid'],
+ })
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertIn('text/html', response['Content-Type'])
+ self.assertEqual("Unknown operation.", response.content)
+
+ def test_POST_fails_with_bad_operation(self):
+ # If the operation ('op=operation_name') specified in the
+ # request data is unknown, a 'Bad request' response is returned.
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'op': 'invalid_operation',
+ 'hostname': 'diane',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid'],
+ })
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertEqual(
+ "Unknown operation: 'invalid_operation'.", response.content)
+
+ def test_POST_new_rejects_invalid_data(self):
+ # If the data provided to create a node with an invalid MAC
+ # Address, a 'Bad request' response is returned.
+ response = self.client.post(
+ '/api/nodes/',
+ {
+ 'op': 'new',
+ 'hostname': 'diane',
+ 'mac_addresses': ['aa:bb:cc:dd:ee:ff', 'invalid'],
+ })
+ parsed_result = json.loads(response.content)
+
+ self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertIn('application/json', response['Content-Type'])
+ self.assertItemsEqual(['mac_addresses'], parsed_result)
+ self.assertEqual(
+ ["One or more MAC Addresses is invalid."],
+ parsed_result['mac_addresses'])
+
+
class MACAddressAPITest(APITestMixin):
def setUp(self):
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-02-10 08:49:01 +0000
+++ src/maasserver/tests/test_models.py 2012-02-10 16:07:33 +0000
@@ -68,53 +68,74 @@
class NodeManagerTest(TestCase):
- def make_user_and_node(self, admin=False):
- if admin:
- user = factory.make_admin()
+ def make_node(self, user=None):
+ """Create a node, allocated to `user` if given."""
+ if user is None:
+ status = NODE_STATUS.COMMISSIONED
else:
- user = factory.make_user()
- node = factory.make_node(
- set_hostname=True, status=NODE_STATUS.DEPLOYED,
- owner=user)
- return user, node
-
- def test_get_visible_nodes_user(self):
- """get_visible_nodes returns the nodes a user has access to."""
- anon_node = factory.make_node()
- admin, admin_node = self.make_user_and_node(admin=True)
- user, user_node = self.make_user_and_node()
-
- visible_nodes = Node.objects.get_visible_nodes(user)
-
- self.assertSequenceEqual([anon_node, user_node], visible_nodes)
-
- def test_get_visible_nodes_admin(self):
- """get_visible_nodes returns all the nodes if the user used for the
- permission check is an admin."""
- anon_node = factory.make_node()
- user, user_node = self.make_user_and_node()
- admin, admin_node = self.make_user_and_node(admin=True)
- visible_nodes = Node.objects.get_visible_nodes(admin)
-
- self.assertSequenceEqual(
- [anon_node, user_node, admin_node], visible_nodes)
+ status = NODE_STATUS.DEPLOYED
+ return factory.make_node(set_hostname=True, status=status, owner=user)
+
+ def test_get_visible_nodes_for_user_lists_visible_nodes(self):
+ """get_visible_nodes lists the nodes a user has access to.
+
+ When run for a regular user it returns unowned nodes, and nodes
+ owned by that user.
+
+ """
+ user = factory.make_user()
+ visible_nodes = [self.make_node(owner) for owner in [None, user]]
+ self.make_node(factory.make_user())
+ self.assertItemsEqual(
+ visible_nodes, Node.objects.get_visible_nodes(user))
+
+ def test_get_visible_nodes_admin_lists_all_nodes(self):
+ """get_visible_nodes for an admin lists all nodes."""
+ admin = factory.make_admin()
+ owners = [
+ None,
+ factory.make_user(),
+ factory.make_admin(),
+ admin,
+ ]
+ nodes = [self.make_node(owner) for owner in owners]
+ self.assertItemsEqual(nodes, Node.objects.get_visible_nodes(admin))
+
+ def test_get_visible_nodes_filters_by_id(self):
+ """get_visible_nodes optionally filters nodes by system_id."""
+ user = factory.make_user()
+ nodes = [self.make_node(user) for counter in range(5)]
+ ids = [node.system_id for node in nodes]
+ wanted_slice = slice(0, 3)
+ self.assertItemsEqual(
+ nodes[wanted_slice],
+ Node.objects.get_visible_nodes(user, ids=ids[wanted_slice]))
+
+ def test_get_visible_nodes_with_ids_still_hides_invisible_nodes(self):
+ """Even when passing ids, a user won't get nodes they can't see."""
+ user = factory.make_user()
+ visible_nodes = [self.make_node(user), self.make_node()]
+ invisible_nodes = [self.make_node(factory.make_user())]
+ all_ids = [
+ node.system_id for node in (visible_nodes + invisible_nodes)]
+ self.assertItemsEqual(
+ visible_nodes, Node.objects.get_visible_nodes(user, ids=all_ids))
def test_get_visible_node_or_404_ok(self):
"""get_visible_node_or_404 fetches nodes by system_id."""
- user, user_node = self.make_user_and_node()
- node = Node.objects.get_visible_node_or_404(user_node.system_id, user)
-
- self.assertEqual(user_node, node)
-
- def test_get_visible_node_or_404_raise_PermissionDenied(self):
- """get_visible_node_or_404 raise PermissionDenied if the provided user
- cannot access the returned node."""
- _, user_node = self.make_user_and_node()
- another_user = factory.make_user()
-
+ user = factory.make_user()
+ node = self.make_node(user)
+ self.assertEqual(
+ node, Node.objects.get_visible_node_or_404(node.system_id, user))
+
+ def test_get_visible_node_or_404_raises_PermissionDenied(self):
+ """get_visible_node_or_404 raises PermissionDenied if the provided
+ user cannot access the returned node."""
+ user_node = self.make_node(factory.make_user())
self.assertRaises(
- PermissionDenied, Node.objects.get_visible_node_or_404,
- user_node.system_id, another_user)
+ PermissionDenied,
+ Node.objects.get_visible_node_or_404,
+ user_node.system_id, factory.make_user())
class MACAddressTest(TestCase):
