Merge lp:~jameinel/maas/tag-list-api into lp:maas/trunk

[John A Meinel]

Expose '/tags/' for CRUD. And expose tags as an attribute of Node.

This adds 2 primary features

'/tags/' to the api, with the ability to create new tags, and PUT/DELETE to update and remove them.

This also tries to expose tags as an attribute of Node, but it seems to be exposing more than I really want. (I don't need the whole tag definition exposed, just the basic name.)

Note that this doesn't actually apply tags to nodes based on the definition, because we need the lshw in the table for that.

[Jeroen T. Vermeulen (jtv)]

You left this TODO comment in src/maasserver/api.py:

89 +# TODO: Add AnonTagsHandler/AnonTagHandler?

Don't land TODO comments. Some of us use a bzr plugin that warns about such comments as signs that the branch is not ready for review. Resolve it if you can, or if it's serious and has to be deferred, make it an XXX with a bug number.

As for the issue that the comment is about, it seems to me that exposing the tagging of nodes (e.g. the "nodes" operation) reveals inventory information that administrators may want to keep private. These things are often worth their own tests.

[Jeroen T. Vermeulen (jtv)]

May I recommend factory.make_name() as a nicer way to generate randomized names?

Also, it's a bit pointless to assert that the row count of a query is zero -- just self.assertFalse(query.exists()). If you're checking that the row count is not zero, self.assertTrue(query.exists()).

In the tests where you assert a row count of 1, there are two different concerns: that the count be nonzero, and that there be no more than one row. The latter is probably guarded by a unique-constraint in the database, in which case you might as well use self.assertTrue(query.exists()). Or if there is a real worry about having multiple rows, consider specifying exactly what you expect to see, so that an unexpected result gets printed in more detail:

    self.assertItemsEqual([tag], Tag.objects.filter(...))

-

Then, a small note about this test:

339 + def test_GET_returns_tag(self):
340 + # The api allows for fetching a single Node (using system_id).
341 + tag = factory.make_tag('tag-name')
342 + response = self.client.get(self.get_uri('tags/tag-name/'))
343 + parsed_result = json.loads(response.content)
344 +
345 + self.assertEqual(httplib.OK, response.status_code)
346 + self.assertEqual(tag.name, parsed_result['name'])
347 + self.assertEqual(tag.definition, parsed_result['definition'])
348 + self.assertEqual(tag.comment, parsed_result['comment'])

Better test the status code _before_ you decode the contents, otherwise an unexpected result will get you a traceback for non-JSON contents before you even get to the status-code check.

[Jeroen T. Vermeulen (jtv)]

As per IRC: I had one big question mark for the "nodes" method being a POST, but that's an infrastructure limitation. Otherwise, once my notes are addressed, I'm quite happy with this branch.

review: Approve

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[John A Meinel (jameinel)]

I'm tweaking.

review: Approve

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-24 11:19:14 +0000
+++ src/maasserver/api.py	2012-09-25 11:23:22 +0000
@@ -69,6 +69,8 @@
     "NodeMacHandler",
     "NodeMacsHandler",
     "NodesHandler",
+    "TagHandler",
+    "TagsHandler",
     "pxeconfig",
     "render_api_docs",
     ]
@@ -133,6 +135,7 @@
     get_node_edit_form,
     NodeGroupInterfaceForm,
     NodeGroupWithInterfacesForm,
+    TagForm,
     )
 from maasserver.models import (
     BootImage,
@@ -143,6 +146,7 @@
     Node,
     NodeGroup,
     NodeGroupInterface,
+    Tag,
     )
 from maasserver.preseed import (
     compose_enlistment_preseed_url,
@@ -429,6 +433,7 @@
     'netboot',
     'power_type',
     'power_parameters',
+    'tag_names',
     )
 
 
@@ -1247,6 +1252,100 @@
 
 
 @api_operations
+class TagHandler(BaseHandler):
+    """Manage individual Tags."""
+    allowed_methods = ('GET', 'DELETE', 'POST', 'PUT')
+    model = Tag
+    fields = (
+        'name',
+        'definition',
+        'comment',
+        )
+
+    def read(self, request, name):
+        """Read a specific Node."""
+        return Tag.objects.get_tag_or_404(name=name, user=request.user)
+
+    def update(self, request, name):
+        """Update a specific `Tag`.
+        """
+        tag = Tag.objects.get_tag_or_404(name=name, user=request.user,
+            to_edit=True)
+        data = get_overrided_query_dict(model_to_dict(tag), request.data)
+        form = TagForm(data, instance=tag)
+        if form.is_valid():
+            return form.save()
+        else:
+            raise ValidationError(form.errors)
+
+    def delete(self, request, name):
+        """Delete a specific Node."""
+        tag = Tag.objects.get_tag_or_404(name=name,
+            user=request.user, to_edit=True)
+        tag.delete()
+        return rc.DELETED
+
+    # XXX: JAM 2012-09-25 This is currently a POST because of bug:
+    #      http://pad.lv/1049933
+    #      Essentially, if you have one 'GET' op, then you can no longer get
+    #      the Tag object itself from a plain 'GET' without op.
+    @api_exported('POST')
+    def nodes(self, request, name):
+        """Get the list of nodes that have this tag."""
+        tag = Tag.objects.get_tag_or_404(name=name, user=request.user)
+        # XXX: JAM 2012-09-25 We need to filter the node set returned by the
+        #      visibility defined by the user.
+        return tag.node_set.all()
+
+    @classmethod
+    def resource_uri(cls, tag=None):
+        # See the comment in NodeHandler.resource_uri
+        tag_name = 'tag_name'
+        if tag is not None:
+            tag_name = tag.name
+        return ('tag_handler', (tag_name, ))
+
+
+@api_operations
+class TagsHandler(BaseHandler):
+    """Manage collection of Tags."""
+    allowed_methods = ('GET', 'POST')
+
+    @api_exported('POST')
+    def new(self, request):
+        """Create a new `Tag`.
+        """
+        return create_tag(request)
+
+    @api_exported('GET')
+    def list(self, request):
+        """List Tags.
+        """
+        return Tag.objects.all()
+
+    @classmethod
+    def resource_uri(cls, *args, **kwargs):
+        return ('tags_handler', [])
+
+
+def create_tag(request):
+    """Service an http request to create a tag.
+
+    :param request: The http request for this node to be created.
+    :return: A `Tag`.
+    :rtype: :class:`maasserver.models.Tag`.
+    :raises: ValidationError
+    """
+    if not request.user.is_superuser:
+        raise PermissionDenied()
+    form = TagForm(request.data)
+    if form.is_valid():
+        return form.save()
+    else:
+        raise ValidationError(form.errors)
+
+
+@api_operations
 class MAASHandler(BaseHandler):
     """Manage the MAAS' itself."""
     allowed_methods = ('POST', 'GET')

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-09-21 09:06:21 +0000
+++ src/maasserver/forms.py	2012-09-25 11:23:22 +0000
@@ -25,6 +25,7 @@
     "SSHKeyForm",
     "UbuntuForm",
     "AdminNodeForm",
+    "TagForm",
     ]
 
 import collections
@@ -73,6 +74,7 @@
     NodeGroup,
     NodeGroupInterface,
     SSHKey,
+    Tag,
     )
 from maasserver.node_action import compile_node_actions
 from maasserver.power_parameters import POWER_TYPE_PARAMETERS
@@ -743,3 +745,14 @@
         nodegroup.status = NODEGROUP_STATUS.PENDING
         nodegroup.save()
         return nodegroup
+
+
+class TagForm(ModelForm):
+
+    class Meta:
+        model = Tag
+        fields = (
+            'name',
+            'comment',
+            'definition',
+            )

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2012-09-24 11:19:14 +0000
+++ src/maasserver/models/node.py	2012-09-25 11:23:22 +0000
@@ -420,6 +420,9 @@
         else:
             return self.system_id
 
+    def tag_names(self):
+        return self.tags.values_list('name', flat=True)
+
     def clean_status(self):
         """Check a node's status transition against the node-status FSM."""
         old_status = get_db_state(self, 'status')

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2012-09-21 06:37:02 +0000
+++ src/maasserver/models/tag.py	2012-09-25 11:23:22 +0000
@@ -14,19 +14,46 @@
     "Tag",
     ]
 
+from django.core.exceptions import (
+    PermissionDenied,
+    )
 from django.db.models import (
     CharField,
     Manager,
     TextField,
     )
+from django.shortcuts import get_object_or_404
 from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
 
 
+# Permission model for tags. Everyone can see all tags, but only superusers can
+# edit tags.
 class TagManager(Manager):
     """A utility to manage the collection of Tags."""
-    pass
+
+    def get_tag_or_404(self, name, user, to_edit=False):
+        """Fetch a `Tag` by name.  Raise exceptions if no `Tag` with
+        this name exist.
+
+        :param name: The Tag.name.
+        :type name: str
+        :param user: The user that should be used in the permission check.
+        :type user: django.contrib.auth.models.User
+        :param to_edit: Are we going to edit this tag, or just view it?
+        :type to_edit: bool
+        :raises: django.http.Http404_,
+            :class:`maasserver.exceptions.PermissionDenied`.
+
+        .. _django.http.Http404: https://
+           docs.djangoproject.com/en/dev/topics/http/views/
+           #the-http404-exception
+        """
+        if to_edit and not user.is_superuser:
+            raise PermissionDenied()
+        tag = get_object_or_404(Tag, name=name)
+        return tag
 
 
 class Tag(CleanSave, TimestampedModel):

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2012-09-20 13:16:13 +0000
+++ src/maasserver/testing/factory.py	2012-09-25 11:23:22 +0000
@@ -225,8 +225,10 @@
         key.save()
         return key
 
-    def make_tag(self, name, definition=None, comment='', created=None,
+    def make_tag(self, name=None, definition=None, comment='', created=None,
                  updated=None):
+        if name is None:
+            name = self.make_name('tag')
         if definition is None:
             # Is there a 'node' in this xml?
             definition = '//node'

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-24 11:19:14 +0000
+++ src/maasserver/tests/test_api.py	2012-09-25 11:23:22 +0000
@@ -66,6 +66,7 @@
     Node,
     NodeGroup,
     NodeGroupInterface,
+    Tag,
     )
 from maasserver.models.user import (
     create_auth_token,
@@ -496,6 +497,7 @@
                 'netboot',
                 'power_type',
                 'power_parameters',
+                'tag_names',
             ],
             list(parsed_result))
 
@@ -570,6 +572,7 @@
                 'power_type',
                 'power_parameters',
                 'resource_uri',
+                'tag_names',
             ],
             list(parsed_result))
 
@@ -710,6 +713,7 @@
                 'power_type',
                 'power_parameters',
                 'resource_uri',
+                'tag_names',
             ],
             list(parsed_result))
 
@@ -852,12 +856,22 @@
         # The api allows for fetching a single Node (using system_id).
         node = factory.make_node(set_hostname=True)
         response = self.client.get(self.get_node_uri(node))
-        parsed_result = json.loads(response.content)
 
         self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
         self.assertEqual(node.hostname, parsed_result['hostname'])
         self.assertEqual(node.system_id, parsed_result['system_id'])
 
+    def test_GET_returns_associated_tag(self):
+        node = factory.make_node(set_hostname=True)
+        tag = factory.make_tag()
+        node.tags.add(tag)
+        response = self.client.get(self.get_node_uri(node))
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual([tag.name], parsed_result['tag_names'])
+
     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.
@@ -2160,6 +2174,138 @@
         self.assertEqual("File not found", response.content)
 
 
+class TestTagAPI(APITestCase):
+    """Tests for /api/1.0/tags/<tagname>/."""
+
+    def get_tag_uri(self, tag):
+        """Get the API URI for `tag`."""
+        return self.get_uri('tags/%s/') % tag.name
+
+    def test_DELETE_requires_admin(self):
+        tag = factory.make_tag()
+        response = self.client.delete(self.get_tag_uri(tag))
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertItemsEqual([tag], Tag.objects.filter(id=tag.id))
+
+    def test_DELETE_removes_tag(self):
+        self.become_admin()
+        tag = factory.make_tag()
+        response = self.client.delete(self.get_tag_uri(tag))
+        self.assertEqual(httplib.NO_CONTENT, response.status_code)
+        self.assertFalse(Tag.objects.filter(id=tag.id).exists())
+
+    def test_DELETE_404(self):
+        self.become_admin()
+        response = self.client.delete(self.get_uri('tags/no-tag/'))
+        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+
+    def test_GET_returns_tag(self):
+        # The api allows for fetching a single Node (using system_id).
+        tag = factory.make_tag('tag-name')
+        response = self.client.get(self.get_uri('tags/tag-name/'))
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual(tag.name, parsed_result['name'])
+        self.assertEqual(tag.definition, parsed_result['definition'])
+        self.assertEqual(tag.comment, parsed_result['comment'])
+
+    def test_GET_refuses_to_access_nonexistent_node(self):
+        # When fetching a Tag, the api returns a 'Not Found' (404) error
+        # if no tag is found.
+        response = self.client.get(self.get_uri('tags/no-such-tag/'))
+        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+
+    def test_PUT_refuses_non_superuser(self):
+        tag = factory.make_tag()
+        response = self.client.put(self.get_tag_uri(tag),
+                                   {'comment': 'A special comment'})
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+    def test_PUT_invalid_field(self):
+        self.become_admin()
+        tag = factory.make_tag()
+        response = self.client.put(self.get_tag_uri(tag),
+            {'not-a-field': 'content'})
+        self.assertEqual(httplib.OK, response.status_code)
+
+    def test_PUT_updates_tag(self):
+        self.become_admin()
+        tag = factory.make_tag()
+        # Note that 'definition' is not being sent
+        response = self.client.put(self.get_tag_uri(tag),
+            {'name': 'new-tag-name', 'comment': 'A random comment'})
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual('new-tag-name', parsed_result['name'])
+        self.assertEqual('A random comment', parsed_result['comment'])
+        self.assertEqual(tag.definition, parsed_result['definition'])
+        self.assertFalse(Tag.objects.filter(name=tag.name).exists())
+        self.assertTrue(Tag.objects.filter(name='new-tag-name').exists())
+
+    def test_POST_nodes_with_no_nodes(self):
+        tag = factory.make_tag()
+        response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual([], parsed_result)
+
+    def test_POST_nodes_returns_nodes(self):
+        tag = factory.make_tag()
+        node1 = factory.make_node()
+        # Create a second node that isn't tagged.
+        factory.make_node()
+        node1.tags.add(tag)
+        response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
+
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual([node1.system_id],
+                         [r['system_id'] for r in parsed_result])
+
+
+class TestTagsAPI(APITestCase):
+
+    def test_GET_list_without_tags_returns_empty_list(self):
+        response = self.client.get(self.get_uri('tags/'), {'op': 'list'})
+        self.assertItemsEqual([], json.loads(response.content))
+
+    def test_POST_new_refuses_non_admin(self):
+        name = factory.getRandomString()
+        response = self.client.post(
+            self.get_uri('tags/'),
+            {
+                'op': 'new',
+                'name': name,
+                'comment': factory.getRandomString(),
+                'definition': factory.getRandomString(),
+            })
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+        self.assertFalse(Tag.objects.filter(name=name).exists())
+
+    def test_POST_new_creates_tag(self):
+        self.become_admin()
+        name = factory.getRandomString()
+        definition = '//node'
+        comment = factory.getRandomString()
+        response = self.client.post(
+            self.get_uri('tags/'),
+            {
+                'op': 'new',
+                'name': name,
+                'comment': comment,
+                'definition': definition,
+            })
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_result = json.loads(response.content)
+        self.assertEqual(name, parsed_result['name'])
+        self.assertEqual(comment, parsed_result['comment'])
+        self.assertEqual(definition, parsed_result['definition'])
+        self.assertTrue(Tag.objects.filter(name=name).exists())
+
+
 class MAASAPIAnonTest(APIv10TestMixin, TestCase):
     # The MAAS' handler is not accessible to anon users.
 

=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py	2012-09-23 13:06:49 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-25 11:23:22 +0000
@@ -32,7 +32,7 @@
 
     def test_add_tag_to_node(self):
         node = factory.make_node()
-        tag = factory.make_tag('tag-name')
+        tag = factory.make_tag()
         tag.save()
         node.tags.add(tag)
         self.assertEqual([tag.id], [t.id for t in node.tags.all()])

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2012-09-18 07:51:29 +0000
+++ src/maasserver/urls_api.py	2012-09-25 11:23:22 +0000
@@ -32,6 +32,8 @@
     NodeMacHandler,
     NodeMacsHandler,
     NodesHandler,
+    TagHandler,
+    TagsHandler,
     pxeconfig,
     RestrictedResource,
     )
@@ -51,6 +53,8 @@
     NodeGroupsHandler, authentication=api_auth)
 boot_images_handler = RestrictedResource(
     BootImagesHandler, authentication=api_auth)
+tag_handler = RestrictedResource(TagHandler, authentication=api_auth)
+tags_handler = RestrictedResource(TagsHandler, authentication=api_auth)
 
 
 # Admin handlers.
@@ -92,6 +96,8 @@
     url(r'files/$', files_handler, name='files_handler'),
     url(r'account/$', account_handler, name='account_handler'),
     url(r'boot-images/$', boot_images_handler, name='boot_images_handler'),
+    url(r'tags/(?P<name>[\w\-]+)/$', tag_handler, name='tag_handler'),
+    url(r'tags/$', tags_handler, name='tags_handler'),
 )