launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/better-errors-for-translate-path into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


Originally I was wanting to get the nice contextually aware error messages that existed when the bazaar client got full branch names back for official linked branches from the XMLRPC server rather than the +branch/alias that it gets now to enable accessing private official branches and pushing to series branches to create links.

However that isn't possible with the way that bzr and lp communicate with the launchpad transport.  Ideally we want to extend bzr to get nicer error messages by having an early smart server message that specifies the branch and read or write needs of the connection.

So... what this branch does is very simple.  It catches the CannotHaveLinkedBranch error and makes sure it is passed back the same way the other exceptions are.

tests:
    CodehostingTest
-- 
https://code.launchpad.net/~thumper/launchpad/better-errors-for-translate-path/+merge/38178
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/better-errors-for-translate-path into lp:launchpad/devel.

=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-10-12 03:58:59 +0000
@@ -42,6 +42,7 @@
 from lp.code.enums import BranchType
 from lp.code.errors import (
     BranchCreationException,
+    CannotHaveLinkedBranch,
     InvalidNamespace,
     NoLinkedBranch,
     UnknownBranchTypeError,
@@ -69,7 +70,10 @@
     IPersonSet,
     NoSuchPerson,
     )
-from lp.registry.interfaces.product import NoSuchProduct
+from lp.registry.interfaces.product import (
+    InvalidProductName,
+    NoSuchProduct,
+    )
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 from lp.services.utils import iter_split
 
@@ -339,7 +343,12 @@
                             raise faults.PathTranslationError(path)
                         branch, trailing = getUtility(
                             IBranchLookup).getByLPPath(lp_path)
-                    except (NameLookupFailed, InvalidNamespace, NoLinkedBranch):
+                    except (InvalidProductName, NoLinkedBranch,
+                            CannotHaveLinkedBranch):
+                        # If we get one of these errors, then there is no
+                        # point walking back through the path parts.
+                        break
+                    except (NameLookupFailed, InvalidNamespace):
                         # The reason we're doing it is that getByLPPath thinks
                         # that 'foo/.bzr' is a request for the '.bzr' series
                         # of a product.

=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-10-04 19:50:45 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-10-12 03:58:59 +0000
@@ -645,11 +645,6 @@
             (branch.control_format, branch.branch_format,
              branch.repository_format))
 
-    def assertCannotTranslate(self, requester, path):
-        """Assert that we cannot translate 'path'."""
-        fault = self.codehosting_api.translatePath(requester.id, path)
-        self.assertEqual(faults.PathTranslationError(path), fault)
-
     def assertNotFound(self, requester, path):
         """Assert that the given path cannot be found."""
         if requester not in [LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES]:
@@ -684,7 +679,7 @@
         # couldn't translate.
         requester = self.factory.makePerson()
         path = escape(u'/untranslatable')
-        self.assertCannotTranslate(requester, path)
+        self.assertNotFound(requester, path)
 
     def test_translatePath_no_preceding_slash(self):
         requester = self.factory.makePerson()
@@ -966,6 +961,23 @@
         path = '/%s/%s' % (BRANCH_ALIAS_PREFIX, product.name)
         self.assertNotFound(requester, path)
 
+    def test_translatePath_branch_alias_no_linked_sourcepackage_branch(self):
+        # translatePath returns a not found when there's no linked branch for
+        # a distro series source package.
+        requester = self.factory.makePerson()
+        sourcepackage = self.factory.makeSourcePackage()
+        distro = sourcepackage.distribution
+        path = '/%s/%s/%s' % (
+            BRANCH_ALIAS_PREFIX, distro.name, sourcepackage.sourcepackagename)
+        self.assertNotFound(requester, path)
+
+    def test_translatePath_branch_alias_invalid_product_name(self):
+        # translatePath returns XXX
+        requester = self.factory.makePerson()
+        invalid_name = '_' + self.factory.getUniqueString()
+        path = '/%s/%s' % (BRANCH_ALIAS_PREFIX, invalid_name)
+        self.assertNotFound(requester, path)
+
     def test_translatePath_branch_alias_bzrdir_content(self):
         # translatePath('/+branch/.bzr/.*') *must* return not found, otherwise
         # bzr will look for it and we don't have a global bzr dir.