Merge lp:~thumper/launchpad/better-errors-for-translate-path into lp:launchpad

[Tim Penhey]

Catch the CannotHaveLinkedBranch exception in translatePath

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

[Robert Collins (lifeless)]

"returns XXX" - that will confuse folk ;)

I sometimes find creating a constant tuple for exceptions useful. You might find it nice here.

caught_errors = (...)
except caught_errors:
  ...

mainly useful when you want the same set in multiple places.

review: Approve

[Robert Collins (lifeless)]

This is broken in prod, its a regression.

review: Approve (release-critical)

[Jeroen T. Vermeulen (jtv)]

One broken comment, otherwise fine. It'd be nice if the surrounding code could be cleaned up someday, but that's not for an RC. Approvedl

review: Approve (code)

[Jonathan Lange (jml)]

Is the CannotHaveLinkedBranch error still being raised for cases where, actually, a linked branch is possible but none exists? If so, when is that going to be fixed.

=== 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 04:26:20 +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 04:26:20 +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 a not found when there is an invalid product name.
+        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.