Merge lp:~ltrager/maas/lp1573644 into lp:~maas-committers/maas/trunk

[Lee Trager]

Allow showing tags by node, machine, device, rack-controller, or region-controller

This adds machines, devices, rack-controllers, and region-controllers operations on the tag API endpoint. Each will only show the specifies node type which has the specified tags.

While writing this I noticed that Node.objects.get_nodes() was not checking that the specified from_nodes query set only contained nodes for that managers node type. For example

Machine.objects.get_nodes(admin, NODE_PERMISSION.VIEW, from_nodes=[Machine, Device, RackController]) would return [Machine, Device, RackController] instead of [Machine].

I patched get_nodes to filter for the managers specified node_type.

[Blake Rouse (blake-rouse)]

Nicely done. Looks great.

review: Approve

=== modified file 'src/maasserver/api/tags.py'
--- src/maasserver/api/tags.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/api/tags.py	2016-04-22 21:25:30 +0000
@@ -30,8 +30,11 @@
 )
 from maasserver.forms import TagForm
 from maasserver.models import (
+    Device,
+    Machine,
     Node,
     RackController,
+    RegionController,
     Tag,
 )
 from maasserver.models.node import typecast_to_node_type
@@ -121,12 +124,7 @@
         tag.delete()
         return rc.DELETED
 
-    @operation(idempotent=True)
-    def nodes(self, request, name):
-        """Get the list of nodes that have this tag.
-
-        Returns 404 if the tag is not found.
-        """
+    def _get_node_type(self, model, request, name):
         # Workaround an issue where piston3 will try to use the fields from
         # this handler instead of the fields defined for the returned object.
         # This is done because this operation actually returns a list of nodes
@@ -135,11 +133,51 @@
         tag = Tag.objects.get_tag_or_404(name=name, user=request.user)
         return [
             typecast_to_node_type(node)
-            for node in Node.objects.get_nodes(
+            for node in model.objects.get_nodes(
                 request.user, NODE_PERMISSION.VIEW,
                 from_nodes=tag.node_set.all())
         ]
 
+    @operation(idempotent=True)
+    def nodes(self, request, name):
+        """Get the list of nodes that have this tag.
+
+        Returns 404 if the tag is not found.
+        """
+        return self._get_node_type(Node, request, name)
+
+    @operation(idempotent=True)
+    def machines(self, request, name):
+        """Get the list of machines that have this tag.
+
+        Returns 404 if the tag is not found.
+        """
+        return self._get_node_type(Machine, request, name)
+
+    @operation(idempotent=True)
+    def devices(self, request, name):
+        """Get the list of devices that have this tag.
+
+        Returns 404 if the tag is not found.
+        """
+        return self._get_node_type(Device, request, name)
+
+    @operation(idempotent=True)
+    def rack_controllers(self, request, name):
+        """Get the list of rack controllers that have this tag.
+
+        Returns 404 if the tag is not found.
+        """
+        return self._get_node_type(RackController, request, name)
+
+    @operation(idempotent=True)
+    def region_controllers(self, request, name):
+        """Get the list of region controllers that have this tag.
+
+        Returns 404 if the tag is not found.
+        """
+        return self._get_node_type(RegionController, request, name)
+
     def _get_nodes_for(self, request, param):
         system_ids = get_list_from_dict_or_multidict(request.data, param)
         if system_ids:

=== modified file 'src/maasserver/api/tests/test_tag.py'
--- src/maasserver/api/tests/test_tag.py	2016-02-26 18:39:26 +0000
+++ src/maasserver/api/tests/test_tag.py	2016-04-22 21:25:30 +0000
@@ -130,17 +130,111 @@
 
     def test_GET_nodes_returns_nodes(self):
         tag = factory.make_Tag()
-        node1 = factory.make_Node()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
         # Create a second node that isn't tagged.
         factory.make_Node()
-        node1.tags.add(tag)
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
         response = self.client.get(self.get_tag_uri(tag), {'op': 'nodes'})
 
         self.assertEqual(http.client.OK, response.status_code)
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
-        self.assertEqual([node1.system_id],
-                         [r['system_id'] for r in parsed_result])
+        self.assertItemsEqual(
+            [machine.system_id, device.system_id, rack.system_id,
+             region.system_id],
+            [r['system_id'] for r in parsed_result])
+
+    def test_GET_machines_returns_machines(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(self.get_tag_uri(tag), {'op': 'machines'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual(
+            [machine.system_id],
+            [r['system_id'] for r in parsed_result])
+
+    def test_GET_devices_returns_devices(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(self.get_tag_uri(tag), {'op': 'devices'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual(
+            [device.system_id],
+            [r['system_id'] for r in parsed_result])
+
+    def test_GET_rack_controllers_returns_rack_controllers(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(
+            self.get_tag_uri(tag), {'op': 'rack_controllers'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual(
+            [rack.system_id],
+            [r['system_id'] for r in parsed_result])
+
+    def test_GET_region_controllers_returns_region_controllers(self):
+        tag = factory.make_Tag()
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        # Create a second node that isn't tagged.
+        factory.make_Node()
+        machine.tags.add(tag)
+        device.tags.add(tag)
+        rack.tags.add(tag)
+        region.tags.add(tag)
+        response = self.client.get(
+            self.get_tag_uri(tag), {'op': 'region_controllers'})
+
+        self.assertEqual(http.client.OK, response.status_code)
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertItemsEqual(
+            [region.system_id],
+            [r['system_id'] for r in parsed_result])
 
     def test_GET_nodes_hides_invisible_nodes(self):
         user2 = factory.make_User()

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-04-22 08:42:53 +0000
+++ src/maasserver/models/node.py	2016-04-22 21:25:30 +0000
@@ -442,6 +442,10 @@
         """
         if from_nodes is None:
             from_nodes = self.all()
+        else:
+            # Make sure even if given a query set of multiple node types
+            # get_nodes only returns nodes applicable to this manager.
+            from_nodes = from_nodes.filter(**self.extra_filters)
         nodes = self._filter_visible_nodes(from_nodes, user, perm)
         return self.filter_by_ids(nodes, ids)
 

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2016-04-22 08:42:53 +0000
+++ src/maasserver/models/tests/test_node.py	2016-04-22 21:25:30 +0000
@@ -3263,6 +3263,18 @@
             user=None, perm=NODE_PERMISSION.EDIT, ids=[node.system_id])
         self.assertItemsEqual([], observed)
 
+    def test_get_nodes_only_returns_managed_nodes(self):
+        user = factory.make_User()
+        machine = self.make_node(user)
+        for _ in range(3):
+            self.make_node(user, node_type=NODE_TYPE.DEVICE)
+        self.assertItemsEqual(
+            [machine],
+            Machine.objects.get_nodes(
+                user=user, perm=NODE_PERMISSION.VIEW,
+                from_nodes=Node.objects.all())
+        )
+
     def test_filter_nodes_by_spaces(self):
         # Create a throwaway node and a throwaway space.
         # (to ensure they are filtered out.)

=== modified file 'src/maasserver/websockets/handlers/controller.py'
--- src/maasserver/websockets/handlers/controller.py	2016-04-21 01:26:45 +0000
+++ src/maasserver/websockets/handlers/controller.py	2016-04-22 21:25:30 +0000
@@ -10,7 +10,7 @@
 from maasserver.enum import NODE_PERMISSION
 from maasserver.forms import AdminMachineWithMACAddressesForm
 from maasserver.models.node import (
-    Node,
+    Controller,
     RackController,
     typecast_to_node_type,
 )
@@ -23,7 +23,7 @@
     class Meta(MachineHandler.Meta):
         abstract = False
         queryset = node_prefetch(
-            Node.controllers.all().prefetch_related("interface_set"))
+            Controller.controllers.all().prefetch_related("interface_set"))
         allowed_methods = [
             'list',
             'get',
@@ -76,9 +76,8 @@
 
     def get_queryset(self):
         """Return `QuerySet` for controllers only viewable by `user`."""
-        controllers = super(ControllerHandler, self).get_queryset()
-        return Node.controllers.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=controllers)
+        return Controller.controllers.get_nodes(
+            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
 
     def dehydrate(self, obj, data, for_list=False):
         data = super().dehydrate(obj, data, for_list=for_list)

=== modified file 'src/maasserver/websockets/handlers/device.py'
--- src/maasserver/websockets/handlers/device.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/websockets/handlers/device.py	2016-04-22 21:25:30 +0000
@@ -117,9 +117,8 @@
 
     def get_queryset(self):
         """Return `QuerySet` for devices only viewable by `user`."""
-        devices = super(DeviceHandler, self).get_queryset()
         return Device.objects.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=devices)
+            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
 
     def dehydrate(self, obj, data, for_list=False):
         """Add extra fields to `data`."""

=== modified file 'src/maasserver/websockets/handlers/machine.py'
--- src/maasserver/websockets/handlers/machine.py	2016-03-31 23:34:55 +0000
+++ src/maasserver/websockets/handlers/machine.py	2016-04-22 21:25:30 +0000
@@ -156,9 +156,8 @@
 
     def get_queryset(self):
         """Return `QuerySet` for devices only viewable by `user`."""
-        nodes = super(MachineHandler, self).get_queryset()
         return Machine.objects.get_nodes(
-            self.user, NODE_PERMISSION.VIEW, from_nodes=nodes)
+            self.user, NODE_PERMISSION.VIEW, from_nodes=self._meta.queryset)
 
     def dehydrate(self, obj, data, for_list=False):
         """Add extra fields to `data`."""