launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/invalid-product-names into lp:launchpad.

Commit message:
Updates handling of InvalidProductName on bzr push to return an error message to the user rather than OOPSing.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798954 in Launchpad itself: "InvalidProductName: Invalid name for product: bookmark:galapagos. "
  https://bugs.launchpad.net/launchpad/+bug/798954

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/invalid-product-names/+merge/132405

Summary
=======
When someone pushes to an invalid product name (i.e. one that doesn't satisfy
our name validation), an unexpected error is triggered.

We should just handle it as we do non-existant products and products you don't
have permission to post to, rather than OOPSing.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
The InvalidProductName exception is now caught in a block that handles all the
other expected errors when getting branch information from the path. In its
place, an xmlrpc fault.InvalidProductName is returned.

This is trapped and and returned as a PermissionDenied fault, as that is the
only fault that we return in order to avoid the bzr client being "clever" by
suggesting --create-prrefix or other solutions be tried.

Also: updated InvalidProductIdentifier to be InvalidProductName--there's no
reason to have two separate names for the same error.

Tests were written, which required adding a clause to check if the product
name is valid an if not raise the expected error in the InMemory backend. I
confess I can't figure out the purpose of the inmemory backend, as it seems to
be a mock of some sort but we test everything directly against the db. 

Tests
=====
bin/test -vvct test_createBranch_invalid_product

QA
==
Attempt to push a branch to fiz:bar/trunk on qastaging. You should get back a
permission denied error about the branch being an invalid name, rather than an
unexpected error dump.

LoC
===
I have ~300 LoC of credit.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/xmlrpc/faults.py
  lib/lp/code/xmlrpc/codehosting.py
  lib/lp/code/xmlrpc/tests/test_codehosting.py
  lib/lp/xmlrpc/configure.zcml
  lib/lp/codehosting/vfs/branchfs.py
  lib/lp/codehosting/inmemory.py
  lib/lp/code/model/tests/test_branchlookup.py
  lib/lp/code/xmlrpc/branch.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/invalid-product-names/+merge/132405
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/invalid-product-names into lp:launchpad.

=== modified file 'lib/lp/code/model/tests/test_branchlookup.py'
--- lib/lp/code/model/tests/test_branchlookup.py	2012-09-18 18:36:09 +0000
+++ lib/lp/code/model/tests/test_branchlookup.py	2012-10-31 19:20:28 +0000
@@ -446,7 +446,7 @@
         self.assertRaises(NoSuchProduct, self.traverser.traverse, 'bb')
 
     def test_invalid_product(self):
-        # `traverse` raises `InvalidProductIdentifier` when resolving an lp
+        # `traverse` raises `InvalidProductName` when resolving an lp
         # path for a completely invalid product development focus branch.
         self.assertRaises(
             InvalidProductName, self.traverser.traverse, 'b')

=== modified file 'lib/lp/code/xmlrpc/branch.py'
--- lib/lp/code/xmlrpc/branch.py	2012-07-04 18:06:32 +0000
+++ lib/lp/code/xmlrpc/branch.py	2012-10-31 19:20:28 +0000
@@ -304,7 +304,7 @@
         # the model code(blech) or some automated way of reraising as faults
         # or using a narrower range of faults (e.g. only one "NoSuch" fault).
         except InvalidProductName as e:
-            raise faults.InvalidProductIdentifier(urlutils.escape(e.name))
+            raise faults.InvalidProductName(urlutils.escape(e.name))
         except NoSuchProductSeries as e:
             raise faults.NoSuchProductSeries(
                 urlutils.escape(e.name), e.product)

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2012-07-24 06:39:54 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2012-10-31 19:20:28 +0000
@@ -65,7 +65,10 @@
     IPersonSet,
     NoSuchPerson,
     )
-from lp.registry.interfaces.product import NoSuchProduct
+from lp.registry.interfaces.product import (
+    NoSuchProduct,
+    InvalidProductName,
+    )
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 from lp.services.webapp import LaunchpadXMLRPCView
@@ -219,6 +222,8 @@
             except NoSuchProduct as e:
                 return faults.NotFound(
                     "Project '%s' does not exist." % e.name)
+            except InvalidProductName as e:
+                return faults.InvalidProductName(escape(e.name))
             except NoSuchSourcePackageName as e:
                 try:
                     getUtility(ISourcePackageNameSet).new(e.name)

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2012-09-18 19:41:02 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2012-10-31 19:20:28 +0000
@@ -297,6 +297,16 @@
             owner.id, escape('/~%s/no-such-product/%s' % (owner.name, name)))
         self.assertEqual(faults.NotFound(message), fault)
 
+    def test_createBranch_invalid_product(self):
+        # Creating a branch with an invalid product name fails.
+        owner = self.factory.makePerson()
+        name = self.factory.getUniqueString()
+        from lp.code.interfaces.codehosting import BRANCH_ALIAS_PREFIX
+        branch_name = "/%s/fiz:buzz/%s" % (BRANCH_ALIAS_PREFIX, name)
+        fault = self.codehosting_api.createBranch(
+            owner.id, branch_name)
+        self.assertEqual(faults.InvalidProductName(escape('fiz:buzz')), fault)
+
     def test_createBranch_other_user(self):
         # Creating a branch under another user's directory fails.
         creator = self.factory.makePerson()

=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2012-09-19 01:19:35 +0000
+++ lib/lp/codehosting/inmemory.py	2012-10-31 19:20:28 +0000
@@ -651,6 +651,8 @@
         if data['product'] == '+junk':
             product = None
         elif data['product'] is not None:
+            if not valid_name(data['product']):
+                raise faults.InvalidProductName(escape(data['product']))
             product = self._product_set.getByName(data['product'])
             if product is None:
                 raise faults.NotFound(

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2012-07-05 14:53:47 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2012-10-31 19:20:28 +0000
@@ -607,7 +607,7 @@
             # parent directories", which is just misleading.
             fault = trap_fault(
                 fail, faults.NotFound, faults.PermissionDenied,
-                faults.InvalidSourcePackageName)
+                faults.InvalidSourcePackageName, faults.InvalidProductName)
             faultString = fault.faultString
             if isinstance(faultString, unicode):
                 faultString = faultString.encode('utf-8')

=== modified file 'lib/lp/xmlrpc/configure.zcml'
--- lib/lp/xmlrpc/configure.zcml	2012-01-20 06:58:13 +0000
+++ lib/lp/xmlrpc/configure.zcml	2012-10-31 19:20:28 +0000
@@ -150,7 +150,7 @@
     <require like_class="xmlrpclib.Fault" />
   </class>
 
-  <class class="lp.xmlrpc.faults.InvalidProductIdentifier">
+  <class class="lp.xmlrpc.faults.InvalidProductName">
     <require like_class="xmlrpclib.Fault" />
   </class>
 

=== modified file 'lib/lp/xmlrpc/faults.py'
--- lib/lp/xmlrpc/faults.py	2012-01-20 06:58:13 +0000
+++ lib/lp/xmlrpc/faults.py	2012-10-31 19:20:28 +0000
@@ -20,7 +20,7 @@
     'InvalidBranchIdentifier',
     'InvalidBranchName',
     'InvalidBranchUniqueName',
-    'InvalidProductIdentifier',
+    'InvalidProductName',
     'InvalidBranchUrl',
     'InvalidSourcePackageName',
     'OopsOccurred',
@@ -294,7 +294,7 @@
             component_type=component_type)
 
 
-class InvalidProductIdentifier(LaunchpadFault):
+class InvalidProductName(LaunchpadFault):
     """Raised when we are passed an invalid name for a product.
 
     This is for when users try to specify a product using a silly name