launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/tag-sanity into lp:maas.

Commit message:
Add a validator for the Tag.name field. This restricts tag names to only including '\w-' rather than being any-possible characters.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/tag-sanity/+merge/126602

This adds a validator to Tag.name. It is just a basic '\w' and '-' characters.

That gives us the option to put more stuff into the tag constraint in the future (like possible boolean operations). I don't think it restricts too much from how people want to be using tags.

I'm also explicitly restricting it from including Unicode (non-ascii) in tags. We can change this if we want (use a validator with re.UNICODE set, or set it in the regex itself). It seemed reasonable as a first step, though I can see people wanting localized tags. However, can we pass those through the Juju --constraints field correctly?
-- 
https://code.launchpad.net/~jameinel/maas/tag-sanity/+merge/126602
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/tag-sanity into lp:maas.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2012-09-23 14:14:20 +0000
+++ src/maasserver/forms.py	2012-09-27 05:37:21 +0000
@@ -756,7 +756,3 @@
             '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-26 10:00:14 +0000
+++ src/maasserver/models/tag.py	2012-09-27 05:37:21 +0000
@@ -17,6 +17,7 @@
 from django.core.exceptions import (
     PermissionDenied,
     )
+from django.core.validators import RegexValidator
 from django.db.models import (
     CharField,
     Manager,
@@ -85,10 +86,13 @@
     :ivar objects: The :class:`TagManager`.
     """
 
+    _tag_name_regex = '^[\w-]+$'
+
     class Meta(DefaultMeta):
         """Needed for South to recognize this model."""
 
-    name = CharField(max_length=256, unique=True, editable=True)
+    name = CharField(max_length=256, unique=True, editable=True,
+                     validators=[RegexValidator(_tag_name_regex)])
     definition = TextField()
     comment = TextField(blank=True)
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-09-26 10:00:14 +0000
+++ src/maasserver/tests/test_api.py	2012-09-27 05:37:21 +0000
@@ -2361,6 +2361,27 @@
         self.assertEqual(definition, parsed_result['definition'])
         self.assertTrue(Tag.objects.filter(name=name).exists())
 
+    def test_POST_new_invalid_tag_name(self):
+        self.become_admin()
+        # We do not check the full possible set of invalid names here, a more
+        # thorough check is done in test_tag, we just check that we get a
+        # reasonable error here.
+        invalid = 'invalid:name'
+        definition = '//node'
+        comment = factory.getRandomString()
+        response = self.client.post(
+            self.get_uri('tags/'),
+            {
+                'op': 'new',
+                'name': invalid,
+                'comment': comment,
+                'definition': definition,
+            })
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code,
+            'We did not get BAD_REQUEST for an invalid tag name: %r'
+            % (invalid,))
+        self.assertFalse(Tag.objects.filter(name=invalid).exists())
+
     def test_POST_new_populates_nodes(self):
         self.become_admin()
         node1 = factory.make_node()

=== modified file 'src/maasserver/tests/test_tag.py'
--- src/maasserver/tests/test_tag.py	2012-09-26 12:26:54 +0000
+++ src/maasserver/tests/test_tag.py	2012-09-27 05:37:21 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = []
 
+from django.core.exceptions import ValidationError
 from django.db import transaction
 from django.db.utils import DatabaseError
 from maastesting.djangotestcase import TransactionTestCase
@@ -43,6 +44,12 @@
         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_validate_traps_invalid_tag_name(self):
+        for invalid in ['invalid:name', 'no spaces', 'no\ttabs',
+                        'no&ampersand', 'no!shouting', '',
+                        'too-long'*33, '\xb5']:
+            self.assertRaises(ValidationError, factory.make_tag, name=invalid)
+
     def test_populate_nodes_applies_tags_to_nodes(self):
         node1 = factory.make_node()
         node1.set_hardware_details('<node><child /></node>')