launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/tag_create_populates into lp:maas with lp:~jameinel/maas/tag-and-populate-apis as a prerequisite.

Commit message:
Add Tag.populate_nodes() as a helper to mark a tag for all nodes that match the definition.
This needs to be called when we create a new tag, as well as when we change an existing tag.
Also update the api so that populate_nodes() gets called when appropriate.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/tag_create_populates/+merge/125882

This actually depends on 2 branches, my lp:~jameinel/tag-list-api and lp:~gz/maas/add_node_hardware_details. Hopefully I got a good prerequisite branch so the review of this will be straightforward (I created a branch at the point where I merged the other code bases).

See commit message.

I did check, and maasserver_node_tags has a btree on both node_id and tag_id, so the clear() and update queries should all be reasonably fast.
I do wonder if we could get fancier with the SQL, such as building a temp table, and then only updating rows that don't match the new table. (Rather than deleting everything, and then re-adding everything that matched.) My SQL magic isn't quite good enough for that yet, though.
-- 
https://code.launchpad.net/~jameinel/maas/tag_create_populates/+merge/125882
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jameinel/maas/tag_create_populates into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-09-23 15:00:27 +0000
+++ src/maasserver/api.py	2012-09-23 15:00:27 +0000
@@ -1272,10 +1272,17 @@
         """
         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)
+        model_dict = model_to_dict(tag)
+        old_definition = model_dict['definition']
+        data = get_overrided_query_dict(model_dict, request.data)
         form = TagForm(data, instance=tag)
         if form.is_valid():
-            return form.save()
+            new_tag = form.save(commit=False)
+            new_tag.save()
+            if new_tag.definition != old_definition:
+                new_tag.populate_nodes()
+            form.save_m2m()
+            return new_tag
         else:
             raise ValidationError(form.errors)
 
@@ -1336,7 +1343,11 @@
         raise PermissionDenied()
     form = TagForm(request.data)
     if form.is_valid():
-        return form.save()
+        new_tag = form.save(commit=False)
+        new_tag.save()
+        new_tag.populate_nodes()
+        form.save_m2m()
+        return new_tag
     else:
         raise ValidationError(form.errors)
 

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-09-23 15:00:27 +0000
+++ src/maasserver/forms.py	2012-09-23 15:00:27 +0000
@@ -756,3 +756,7 @@
             'comment',
             'definition',
             )
+
+    def clean(self):
+        cleaned_data = super(TagForm, self).clean()
+        return cleaned_data

=== modified file 'src/maasserver/models/tag.py'
--- src/maasserver/models/tag.py	2012-09-23 15:00:27 +0000
+++ src/maasserver/models/tag.py	2012-09-23 15:00:27 +0000
@@ -78,3 +78,18 @@
 
     def __unicode__(self):
         return self.name
+
+    def populate_nodes(self):
+        """Find all nodes that match this tag, and update them."""
+        # Local import to avoid circular reference
+        from maasserver.models import Node
+        # First destroy the existing tags
+        self.node_set.clear()
+        # Now figure out what matches the new definition
+        for node in Node.objects.raw(
+                'SELECT id FROM maasserver_node'
+                ' WHERE xpath_exists(%s, hardware_details)',
+                [self.definition]):
+            # Is there an efficiency difference between doing
+            # 'node.tags.add(self)' and 'self.node_set.add(node)' ?
+            node.tags.add(self)

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-23 15:00:27 +0000
+++ src/maasserver/tests/test_api.py	2012-09-23 15:00:27 +0000
@@ -2234,6 +2234,23 @@
         self.assertEqual(0, Tag.objects.filter(name=tag.name).count())
         self.assertEqual(1, Tag.objects.filter(name='new-tag-name').count())
 
+    def test_PUT_updates_node_associations(self):
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><foo /></node>')
+        node2 = factory.make_node()
+        node2.set_hardware_details('<node><bar /></node>')
+        tag = factory.make_tag(definition='/node/foo')
+        tag.populate_nodes()
+        self.assertItemsEqual([tag.name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+        self.become_admin()
+        response = self.client.put(self.get_tag_uri(tag),
+            {'definition': '/node/bar'})
+        node1 = reload_object(node1)
+        node2 = reload_object(node2)
+        self.assertItemsEqual([], node1.tag_names())
+        self.assertItemsEqual([tag.name], node2.tag_names())
+
     def test_POST_nodes_with_no_nodes(self):
         tag = factory.make_tag()
         response = self.client.post(self.get_tag_uri(tag), {'op': 'nodes'})
@@ -2295,6 +2312,30 @@
         self.assertEqual(definition, parsed_result['definition'])
         self.assertEqual(1, Tag.objects.filter(name=name).count())
 
+    def test_POST_new_populates_nodes(self):
+        self.become_admin()
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><child /></node>')
+        # Create another node that doesn't have a 'child'
+        node2 = factory.make_node()
+        node2.set_hardware_details('<node />')
+        self.assertItemsEqual([], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+        name = factory.getRandomString()
+        definition = '/node/child'
+        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)
+        self.assertItemsEqual([name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+
 
 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 15:00:27 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-23 15:00:27 +0000
@@ -12,6 +12,9 @@
 __metaclass__ = type
 __all__ = []
 
+from maasserver.testing import (
+    reload_object,
+    )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 
@@ -37,3 +40,41 @@
         node.tags.add(tag)
         self.assertEqual([tag.id], [t.id for t in node.tags.all()])
         self.assertEqual([node.id], [n.id for n in tag.node_set.all()])
+
+    def test_populate_nodes_applies_tags_to_nodes(self):
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><child /></node>')
+        node2 = factory.make_node()
+        node2.set_hardware_details('<node />')
+        tag = factory.make_tag(definition='/node/child')
+        tag.populate_nodes()
+        self.assertItemsEqual([tag.name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+
+    def test_populate_nodes_removes_old_values(self):
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><foo /></node>')
+        node2 = factory.make_node()
+        node2.set_hardware_details('<node><bar /></node>')
+        tag = factory.make_tag(definition='/node/foo')
+        tag.populate_nodes()
+        self.assertItemsEqual([tag.name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+        tag.definition = '/node/bar'
+        tag.populate_nodes()
+        self.assertItemsEqual([], node1.tag_names())
+        self.assertItemsEqual([tag.name], node2.tag_names())
+
+    def test_populate_nodes_doesnt_touch_other_tags(self):
+        node1 = factory.make_node()
+        node1.set_hardware_details('<node><foo /></node>')
+        node2 = factory.make_node()
+        node2.set_hardware_details('<node><bar /></node>')
+        tag1 = factory.make_tag(definition='/node/foo')
+        tag1.populate_nodes()
+        self.assertItemsEqual([tag1.name], node1.tag_names())
+        self.assertItemsEqual([], node2.tag_names())
+        tag2 = factory.make_tag(definition='/node/bar')
+        tag2.populate_nodes()
+        self.assertItemsEqual([tag1.name], node1.tag_names())
+        self.assertItemsEqual([tag2.name], node2.tag_names())