Launchpad itself

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

  1. better-errors-for-translate-path
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11695
Proposed branch: lp:~thumper/launchpad/better-errors-for-translate-path
Merge into: lp:launchpad
Diff against target: 86 lines (+29/-8)
2 files modified
lib/lp/code/xmlrpc/codehosting.py (+11/-2)
lib/lp/code/xmlrpc/tests/test_codehosting.py (+18/-6)
To merge this branch: bzr merge lp:~thumper/launchpad/better-errors-for-translate-path
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) code Approve
Robert Collins (community) release-critical Approve
Launchpad code reviewers release-critical Pending
Review via email: mp+38178@code.launchpad.net

Commit message

Catch the CannotHaveLinkedBranch exception in translatePath

Description of the change

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

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

"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
Revision history for this message
Robert Collins (lifeless) wrote :

This is broken in prod, its a regression.

review: Approve (release-critical)
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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)
Revision history for this message
Jonathan Lange (jml) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
42from lp.code.enums import BranchType42from lp.code.enums import BranchType
43from lp.code.errors import (43from lp.code.errors import (
44 BranchCreationException,44 BranchCreationException,
45 CannotHaveLinkedBranch,
45 InvalidNamespace,46 InvalidNamespace,
46 NoLinkedBranch,47 NoLinkedBranch,
47 UnknownBranchTypeError,48 UnknownBranchTypeError,
@@ -69,7 +70,10 @@
69 IPersonSet,70 IPersonSet,
70 NoSuchPerson,71 NoSuchPerson,
71 )72 )
72from lp.registry.interfaces.product import NoSuchProduct73from lp.registry.interfaces.product import (
74 InvalidProductName,
75 NoSuchProduct,
76 )
73from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet77from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
74from lp.services.utils import iter_split78from lp.services.utils import iter_split
7579
@@ -339,7 +343,12 @@
339 raise faults.PathTranslationError(path)343 raise faults.PathTranslationError(path)
340 branch, trailing = getUtility(344 branch, trailing = getUtility(
341 IBranchLookup).getByLPPath(lp_path)345 IBranchLookup).getByLPPath(lp_path)
342 except (NameLookupFailed, InvalidNamespace, NoLinkedBranch):346 except (InvalidProductName, NoLinkedBranch,
347 CannotHaveLinkedBranch):
348 # If we get one of these errors, then there is no
349 # point walking back through the path parts.
350 break
351 except (NameLookupFailed, InvalidNamespace):
343 # The reason we're doing it is that getByLPPath thinks352 # The reason we're doing it is that getByLPPath thinks
344 # that 'foo/.bzr' is a request for the '.bzr' series353 # that 'foo/.bzr' is a request for the '.bzr' series
345 # of a product.354 # of a product.
346355
=== 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 @@
645 (branch.control_format, branch.branch_format,645 (branch.control_format, branch.branch_format,
646 branch.repository_format))646 branch.repository_format))
647647
648 def assertCannotTranslate(self, requester, path):
649 """Assert that we cannot translate 'path'."""
650 fault = self.codehosting_api.translatePath(requester.id, path)
651 self.assertEqual(faults.PathTranslationError(path), fault)
652
653 def assertNotFound(self, requester, path):648 def assertNotFound(self, requester, path):
654 """Assert that the given path cannot be found."""649 """Assert that the given path cannot be found."""
655 if requester not in [LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES]:650 if requester not in [LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES]:
@@ -684,7 +679,7 @@
684 # couldn't translate.679 # couldn't translate.
685 requester = self.factory.makePerson()680 requester = self.factory.makePerson()
686 path = escape(u'/untranslatable')681 path = escape(u'/untranslatable')
687 self.assertCannotTranslate(requester, path)682 self.assertNotFound(requester, path)
688683
689 def test_translatePath_no_preceding_slash(self):684 def test_translatePath_no_preceding_slash(self):
690 requester = self.factory.makePerson()685 requester = self.factory.makePerson()
@@ -966,6 +961,23 @@
966 path = '/%s/%s' % (BRANCH_ALIAS_PREFIX, product.name)961 path = '/%s/%s' % (BRANCH_ALIAS_PREFIX, product.name)
967 self.assertNotFound(requester, path)962 self.assertNotFound(requester, path)
968963
964 def test_translatePath_branch_alias_no_linked_sourcepackage_branch(self):
965 # translatePath returns a not found when there's no linked branch for
966 # a distro series source package.
967 requester = self.factory.makePerson()
968 sourcepackage = self.factory.makeSourcePackage()
969 distro = sourcepackage.distribution
970 path = '/%s/%s/%s' % (
971 BRANCH_ALIAS_PREFIX, distro.name, sourcepackage.sourcepackagename)
972 self.assertNotFound(requester, path)
973
974 def test_translatePath_branch_alias_invalid_product_name(self):
975 # translatePath returns a not found when there is an invalid product name.
976 requester = self.factory.makePerson()
977 invalid_name = '_' + self.factory.getUniqueString()
978 path = '/%s/%s' % (BRANCH_ALIAS_PREFIX, invalid_name)
979 self.assertNotFound(requester, path)
980
969 def test_translatePath_branch_alias_bzrdir_content(self):981 def test_translatePath_branch_alias_bzrdir_content(self):
970 # translatePath('/+branch/.bzr/.*') *must* return not found, otherwise982 # translatePath('/+branch/.bzr/.*') *must* return not found, otherwise
971 # bzr will look for it and we don't have a global bzr dir.983 # bzr will look for it and we don't have a global bzr dir.