launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bug-nick-validator into lp:launchpad with lp:~jcsackett/launchpad/move-validators as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #192135 Bug nickname field needs better validation
  https://bugs.launchpad.net/bugs/192135

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bug-nick-validator/+merge/51033

Summary
=======

Bug name, while not available in the web ui, can still be accessed and changed via the webservice. The validator being used for it is more permissive than the validation code used in the database, so nicks like '1.1' trigger IntegrityErrors.

This creates a new validator that matches the database constraint, and updates the bug interface to use that.

Implementation
==============

lib/lp/app/validators/name.py
-----------------------------
A new validator, bug_name_validator was created, incorporating the rules used in the database constraint.

lib/lp/bugs/interfaces/bug.py
-----------------------------
The name attribute is changed to use bug_name_validator as its validator.

Tests
=====

bin/test -vvct TestBugConstraints

Demo & QA
=========

Change a bug's name to '1.1' or similar over the webservice; it will throw back an HTTPError rather than failing. Changing to something starting with a letter will be fine.

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/validators/name.py
  lib/lp/bugs/interfaces/bug.py
  lib/lp/bugs/tests/test_bugs_webservice.py

./lib/lp/bugs/interfaces/bug.py
     445: E301 expected 1 blank line, found 0
    1177: E301 expected 1 blank line, found 2

-- 
https://code.launchpad.net/~jcsackett/launchpad/bug-nick-validator/+merge/51033
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bug-nick-validator into lp:launchpad.

=== modified file 'lib/lp/app/validators/name.py'
--- lib/lp/app/validators/name.py	2011-02-23 23:23:41 +0000
+++ lib/lp/app/validators/name.py	2011-02-23 23:23:42 +0000
@@ -15,6 +15,7 @@
 
 
 valid_name_pattern = re.compile(r"^[a-z0-9][a-z0-9\+\.\-]+$")
+valid_bug_name_pattern = re.compile(r"^[a-z][a-z0-9\+\.\-]+$")
 invalid_name_pattern = re.compile(r"^[^a-z0-9]+|[^a-z0-9\\+\\.\\-]+")
 
 
@@ -57,6 +58,13 @@
     return False
 
 
+def valid_bug_name(name):
+    """Return True if the bug name is valid, otherwise False."""
+    if valid_bug_name_pattern.match(name):
+        return True
+    return False
+
+
 def name_validator(name):
     """Return True if the name is valid, or raise a
     LaunchpadValidationError.
@@ -71,3 +79,19 @@
 
         raise LaunchpadValidationError(structured(message))
     return True
+
+
+def bug_name_validator(name):
+    """Return True if the name is valid, or raise a
+    LaunchpadValidationError.
+    """
+    if not valid_bug_name(name):
+        message = _(dedent("""
+            Invalid name '${name}'. Names must be at least two characters long
+            and start with a letter. All letters must be lower-case.
+            The characters <samp>+</samp>, <samp>-</samp> and <samp>.</samp>
+            are also allowed after the first character."""),
+            mapping={'name': escape(name)})
+
+        raise LaunchpadValidationError(structured(message))
+    return True

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-02-23 23:23:41 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-02-23 23:23:42 +0000
@@ -68,7 +68,7 @@
 from lp.app.validators.attachment import (
     attachment_size_constraint,
     )
-from lp.app.validators.name import name_validator
+from lp.app.validators.name import bug_name_validator
 from lp.app.errors import NotFoundError
 from lp.bugs.interfaces.bugactivity import IBugActivity
 from lp.bugs.interfaces.bugattachment import IBugAttachment
@@ -208,7 +208,7 @@
             description=_("""A short and unique name.
                 Add one only if you often need to retype the URL
                 but have trouble remembering the bug number."""),
-            constraint=name_validator))
+            constraint=bug_name_validator))
     title = exported(
         Title(title=_('Summary'), required=True,
               description=_("""A one-line summary of the problem.""")))
@@ -707,7 +707,7 @@
 
     def getMessagesForView(slice_info):
         """Return BugMessage,Message,MessageChunks for renderinger.
-        
+
         This eager loads message.owner validity associated with the
         bugmessages.
 

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2011-02-21 12:03:43 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2011-02-23 23:23:42 +0000
@@ -9,12 +9,12 @@
 
 from BeautifulSoup import BeautifulSoup
 from lazr.lifecycle.interfaces import IDoNotSnapshot
+from lazr.restfulclient.errors import HTTPError
 from simplejson import dumps
 from storm.store import Store
 from testtools.matchers import (
     Equals,
     LessThan,
-    MatchesAny,
     )
 from zope.component import getMultiAdapter
 
@@ -31,7 +31,10 @@
     )
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
 from lp.testing.matchers import HasQueryCount
 from lp.testing.sampledata import (
     ADMIN_EMAIL,
@@ -40,8 +43,36 @@
 from lp.testing._webservice import QueryCollector
 
 
+class TestBugConstraints(TestCaseWithFactory):
+    """Test constrainsts on bug inputs over the API."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugConstraints, self).setUp()
+        product = self.factory.makeProduct(name='foo')
+        bug = self.factory.makeBug(product=product)
+        lp = launchpadlib_for('testing', product.owner)
+        self.bug = lp.bugs[bug.id]
+
+    def _update_bug(self, nick):
+        self.bug.name = nick
+        self.bug.lp_save()
+
+    def test_numeric_nicknames_fail(self):
+        self.assertRaises(
+            HTTPError,
+            self._update_bug,
+            '1.1')
+
+    def test_non_numeric_nicknames_pass(self):
+        self._update_bug('bunny')
+        self.assertEqual('bunny', self.bug.name)
+
+
 class TestBugDescriptionRepresentation(TestCaseWithFactory):
     """Test ways of interacting with Bug webservice representations."""
+
     layer = DatabaseFunctionalLayer
 
     def setUp(self):