Merge lp:~thumper/launchpad/xmlrpc-tests into lp:launchpad/db-devel

[Tim Penhey]

Correctly resolve lp path names to use the +branch alias.

This branch addresses issues found when QA testing the new lp:short-name for private branches.

When going through the XMLRPC resolve_lp_path tests, I seem to have forgotten the complete point of the change, which was to not look up the branch in most cases.

In order to return the http urls for branches accessed using lp:foo a branch lookup is still needed until the branch rewrite map can understand the aliases.

Many tests were removed as they are now pointless as the checking for the linked branch has been passed on to the code path that resolves the aliased name (an earlier branch).

tests:
  TestExpandURL

[Tim Penhey (thumper)]

The Makefile change was a drive-by that I needed in order to run some interactive testing. The hosted_branches make target calls a broken script right now, and that was out of scope for this change.

[Graham Binns (gmb)]

review: Approve (release-critical)

=== modified file 'Makefile'
--- Makefile	2010-09-05 20:29:40 +0000
+++ Makefile	2010-09-07 05:51:45 +0000
@@ -234,7 +234,7 @@
 		-r librarian,google-webservice
 		> ${LPCONFIG}-nohup.out 2>&1 &
 
-run_all: check_schema inplace stop hosted_branches
+run_all: check_schema inplace stop
 	$(RM) thread*.request
 	bin/run -r librarian,sftp,mailman,codebrowse,google-webservice,memcached \
 	    -i $(LPCONFIG)
@@ -248,7 +248,7 @@
 stop_codebrowse:
 	$(PY) scripts/stop-loggerhead.py
 
-run_codehosting: check_schema inplace stop hosted_branches
+run_codehosting: check_schema inplace stop
 	$(RM) thread*.request
 	bin/run -r librarian,sftp,codebrowse -i $(LPCONFIG)
 

=== modified file 'lib/lp/code/xmlrpc/branch.py'
--- lib/lp/code/xmlrpc/branch.py	2010-09-02 21:56:09 +0000
+++ lib/lp/code/xmlrpc/branch.py	2010-09-07 05:51:45 +0000
@@ -15,6 +15,8 @@
     ]
 
 
+from xmlrpclib import Fault
+
 from bzrlib import urlutils
 from zope.component import getUtility
 from zope.interface import (
@@ -214,9 +216,9 @@
         'http': _compose_http_url,
         }
 
-    def _getResultDict(self, branch, lp_path, suffix=None,
-                       supported_schemes=None):
-        """Return a result dict with a list of URLs for the given branch.
+    def _getUrlsForBranch(self, branch, lp_path, suffix=None,
+                          supported_schemes=None):
+        """Return a list of URLs for the given branch.
 
         :param branch: A Branch object.
         :param lp_path: The path that was used to traverse to the branch.
@@ -226,8 +228,8 @@
         """
         if branch.branch_type == BranchType.REMOTE:
             if branch.url is None:
-                return faults.NoUrlForBranch(branch.unique_name)
-            return dict(urls=[branch.url])
+                raise faults.NoUrlForBranch(branch.unique_name)
+            return [branch.url]
         else:
             return self._getUniqueNameResultDict(
                 branch.unique_name, suffix, supported_schemes, lp_path)
@@ -238,11 +240,8 @@
             supported_schemes = SUPPORTED_SCHEMES
         if path is None:
             path = unique_name
-        result = dict(urls=[])
-        for scheme in supported_schemes:
-            result['urls'].append(
-                self.scheme_funcs[scheme](unique_name, path, suffix))
-        return result
+        return [self.scheme_funcs[scheme](unique_name, path, suffix)
+                for scheme in supported_schemes]
 
     @return_fault
     def _resolve_lp_path(self, path):
@@ -254,11 +253,39 @@
         strip_path = path.strip('/')
         if strip_path == '':
             raise faults.InvalidBranchIdentifier(path)
-        supported_schemes = SUPPORTED_SCHEMES
+        supported_schemes = list(SUPPORTED_SCHEMES)
         hot_products = [product.strip() for product
                         in config.codehosting.hot_products.split(',')]
-        if strip_path in hot_products:
-            supported_schemes = ['http']
+        # If we have been given something that looks like a branch name, just
+        # look that up.
+        if strip_path.startswith('~'):
+            urls = self._getBranchPaths(strip_path, supported_schemes)
+        else:
+            # We only check the hot product code when accessed through the
+            # short name, so we can check it here.
+            if strip_path in hot_products:
+                supported_schemes = ['http']
+                urls = []
+            else:
+                urls = [self.scheme_funcs['bzr+ssh'](None, strip_path, None)]
+                supported_schemes.remove('bzr+ssh')
+            # Try to look up the branch at that url and add alternative URLs.
+            # This may well fail, and if it does, we just return the aliased
+            # url.
+            try:
+                urls.extend(
+                    self._getBranchPaths(strip_path, supported_schemes))
+            except Fault:
+                pass
+        return dict(urls=urls)
+
+    def _getBranchPaths(self, strip_path, supported_schemes):
+        """Get the specific paths for a branch.
+
+        If the branch is not found, but it looks like a branch name, then we
+        return a writable URL for it.  If it doesn't look like a branch name a
+        fault is raised.
+        """
         branch_set = getUtility(IBranchLookup)
         try:
             branch, suffix = branch_set.getByLPPath(strip_path)
@@ -267,7 +294,9 @@
             # resolve it anyway, treating the path like a branch's unique
             # name. This lets people push new branches up to Launchpad using
             # lp: URL syntax.
-            return self._getUniqueNameResultDict(strip_path)
+            supported_schemes = ['bzr+ssh']
+            return self._getUniqueNameResultDict(
+                strip_path, supported_schemes=supported_schemes)
         # XXX: JonathanLange 2009-03-21 bug=347728: All of this is repetitive
         # and thus error prone. Alternatives are directly raising faults from
         # the model code(blech) or some automated way of reraising as faults
@@ -298,7 +327,8 @@
             # E.g. 'project/trunk/filename.txt' the suffix is 'filename.txt'
             # we want lp_path to be 'project/trunk'.
             lp_path = lp_path[:-(len(suffix)+1)]
-        return self._getResultDict(branch, lp_path, suffix, supported_schemes)
+        return self._getUrlsForBranch(
+            branch, lp_path, suffix, supported_schemes)
 
     def resolve_lp_path(self, path):
         """See `IPublicCodehostingAPI`."""

=== modified file 'lib/lp/code/xmlrpc/tests/test_branch.py'
--- lib/lp/code/xmlrpc/tests/test_branch.py	2010-09-03 04:22:16 +0000
+++ lib/lp/code/xmlrpc/tests/test_branch.py	2010-09-07 05:51:45 +0000
@@ -59,7 +59,10 @@
         if lp_path is None:
             ssh_branch_path = public_branch_path
         else:
-            ssh_branch_path = '%s/%s' % (BRANCH_ALIAS_PREFIX, lp_path)
+            if lp_path.startswith('~'):
+                ssh_branch_path = lp_path
+            else:
+                ssh_branch_path = '%s/%s' % (BRANCH_ALIAS_PREFIX, lp_path)
         # This improves the error message if results happens to be a fault.
         if isinstance(results, LaunchpadFault):
             raise results
@@ -70,6 +73,10 @@
             else:
                 self.assertEqual('/' + ssh_branch_path, uri.path)
 
+    def assertOnlyWritableResolves(self, lp_url_path):
+        """Only the bzr+ssh url is returned."""
+        self.assertResolves(lp_url_path, None, lp_url_path)
+
     def assertFault(self, lp_url_path, expected_fault):
         """Trying to resolve lp_url_path raises the expected fault."""
         api = PublicCodehostingAPI(None, None)
@@ -122,54 +129,15 @@
         lp_path='%s/%s' % (product.name, product.development_focus.name)
         self.assertResolves(lp_path, trunk.unique_name, lp_path)
 
-    def test_product_doesnt_exist(self):
-        # Return a NoSuchProduct fault if the product doesn't exist.
-        self.assertFault(
-            'doesntexist', faults.NoSuchProduct('doesntexist'))
-        self.assertFault(
-            'doesntexist/trunk', faults.NoSuchProduct('doesntexist'))
-
-    def test_project_group(self):
-        # Resolving lp:///project_group_name' should explain that project
-        # groups don't have default branches.
-        project_group = self.factory.makeProject()
-        fault = self.assertFault(
-            project_group.name, faults.CannotHaveLinkedBranch(project_group))
-        self.assertIn('project group', fault.faultString)
-
-    def test_distro_name(self):
-        # Resolving lp:///distro_name' should explain that distributions don't
-        # have default branches.
-        distro = self.factory.makeDistribution()
-        self.assertFault(distro.name, faults.CannotHaveLinkedBranch(distro))
-
-    def test_distroseries_name(self):
-        # Resolving lp:///distro/series' should explain that distribution
-        # series don't have default branches.
-        series = self.factory.makeDistroSeries()
-        self.assertFault(
-            '%s/%s' % (series.distribution.name, series.name),
-            faults.CannotHaveLinkedBranch(series))
-
-    def test_invalid_product_name(self):
-        # If we get a string that cannot be a name for a product where we
-        # expect the name of a product, we should error appropriately.
-        invalid_name = '_' + self.factory.getUniqueString()
-        self.assertFault(
-            invalid_name,
-            faults.InvalidProductIdentifier(invalid_name))
-
-    def test_invalid_product_name_non_ascii(self):
-        # lp:<non-ascii-string> returns InvalidProductIdentifier with the name
-        # escaped.
-        invalid_name = '_' + NON_ASCII_NAME
-        self.assertFault(
-            invalid_name,
-            faults.InvalidProductIdentifier(urlutils.escape(invalid_name)))
+    def test_target_doesnt_exist(self):
+        # The resolver doesn't care if the product exists or not.
+        self.assertOnlyWritableResolves('doesntexist')
+        self.assertOnlyWritableResolves('doesntexist/trunk')
 
     def test_product_and_series(self):
-        # lp:product/series expands to the branch associated with the product
-        # series 'series' on 'product'.
+        # lp:product/series expands to the writable alias for product/series
+        # and to the branch associated with the product series 'series' on
+        # 'product'.
         product = self.factory.makeProduct()
         branch = branch=self.factory.makeProductBranch(product=product)
         series = self.factory.makeSeries(product=product, branch=branch)
@@ -177,79 +145,23 @@
         self.assertResolves(lp_path, branch.unique_name, lp_path)
 
     def test_development_focus_has_no_branch(self):
-        # Return a NoLinkedBranch fault if the development focus has no branch
-        # associated with it.
+        # A product with no trunk resolves to the writable alias.
         product = self.factory.makeProduct()
-        self.assertEqual(None, product.development_focus.branch)
-        self.assertFault(product.name, faults.NoLinkedBranch(product))
+        self.assertOnlyWritableResolves(product.name)
 
     def test_series_has_no_branch(self):
-        # Return a NoLinkedBranch fault if the series has no branch
-        # associated with it.
+        # A series with no branch resolves to the writable alias.
         series = self.factory.makeSeries(branch=None)
-        self.assertFault(
-            '%s/%s' % (series.product.name, series.name),
-            faults.NoLinkedBranch(series))
-
-    def test_no_such_product_series(self):
-        # Return a NoSuchProductSeries fault if there is no series of the
-        # given name associated with the product.
-        product = self.factory.makeProduct()
-        self.assertFault(
-            '%s/%s' % (product.name, "doesntexist"),
-            faults.NoSuchProductSeries("doesntexist", product))
+        self.assertOnlyWritableResolves(
+            '%s/%s' % (series.product.name, series.name))
 
     def test_no_such_product_series_non_ascii(self):
-        # lp:product/<non-ascii-string> returns NoSuchProductSeries with the
+        # lp:product/<non-ascii-string> resolves to the branch alias with the
         # name escaped.
         product = self.factory.makeProduct()
-        self.assertFault(
-            '%s/%s' % (product.name, NON_ASCII_NAME),
-            faults.NoSuchProductSeries(
-                urlutils.escape(NON_ASCII_NAME), product))
-
-    def test_no_such_distro_series(self):
-        # Return a NoSuchDistroSeries fault if there is no series of the given
-        # name on that distribution.
-        distro = self.factory.makeDistribution()
-        self.assertFault(
-            '%s/doesntexist/whocares' % distro.name,
-            faults.NoSuchDistroSeries("doesntexist"))
-
-    def test_no_such_distro_series_non_ascii(self):
-        # lp:distro/<non-ascii-string>/whatever returns NoSuchDistroSeries
-        # with the name escaped.
-        distro = self.factory.makeDistribution()
-        self.assertFault(
-            '%s/%s/whocares' % (distro.name, NON_ASCII_NAME),
-            faults.NoSuchDistroSeries(urlutils.escape(NON_ASCII_NAME)))
-
-    def test_no_such_source_package(self):
-        # Return a NoSuchSourcePackageName fault if there is no source package
-        # of the given name.
-        distroseries = self.factory.makeDistroRelease()
-        distribution = distroseries.distribution
-        self.assertFault(
-            '%s/%s/doesntexist' % (distribution.name, distroseries.name),
-            faults.NoSuchSourcePackageName('doesntexist'))
-
-    def test_no_such_source_package_non_ascii(self):
-        # lp:distro/series/<non-ascii-name> returns NoSuchSourcePackageName
-        # with the name escaped.
-        distroseries = self.factory.makeDistroRelease()
-        distribution = distroseries.distribution
-        self.assertFault(
-            '%s/%s/%s' % (
-                distribution.name, distroseries.name, NON_ASCII_NAME),
-            faults.NoSuchSourcePackageName(urlutils.escape(NON_ASCII_NAME)))
-
-    def test_no_linked_branch_for_source_package(self):
-        # Return a NoLinkedBranch fault if there's no linked branch for the
-        # sourcepackage.
-        suite_sourcepackage = self.factory.makeSuiteSourcePackage()
-        self.assertFault(
-            suite_sourcepackage.path,
-            faults.NoLinkedBranch(suite_sourcepackage))
+        lp_path = '%s/%s' % (product.name, NON_ASCII_NAME)
+        escaped_name = urlutils.escape(lp_path)
+        self.assertResolves(lp_path, None, escaped_name)
 
     def test_branch(self):
         # The unique name of a branch resolves to the unique name of the
@@ -396,35 +308,27 @@
         # private branch, and tests are currently running as an anonymous
         # user.
         unique_name = removeSecurityProxy(branch).unique_name
-        self.assertResolves(unique_name, unique_name)
+        self.assertOnlyWritableResolves(unique_name)
 
     def test_private_branch_on_series(self):
-        # We resolve invisible branches as if they don't exist.  For
-        # references to product series, this means returning a
-        # NoLinkedBranch fault.
+        # We resolve private linked branches using the writable alias.
         #
         # Removing security proxy because we need to be able to get at
         # attributes of a private branch and these tests are running as an
         # anonymous user.
         branch = self.factory.makeAnyBranch(private=True)
         series = self.factory.makeSeries(branch=branch)
-        self.assertFault(
-            '%s/%s' % (series.product.name, series.name),
-            faults.NoLinkedBranch(series))
+        lp_path='%s/%s' % (series.product.name, series.name)
+        self.assertOnlyWritableResolves(lp_path)
 
     def test_private_branch_as_development_focus(self):
-        # We resolve invisible branches as if they don't exist.
-        #
-        # References to a product resolve to the branch associated with the
-        # development focus. If that branch is private, other views will
-        # indicate that there is no branch on the development focus. We do the
-        # same.
+        # We resolve private linked branches using the writable alias.
         product, trunk = self.makeProdutWithTrunk()
         removeSecurityProxy(trunk).private = True
-        self.assertFault(product.name, faults.NoLinkedBranch(product))
+        self.assertOnlyWritableResolves(product.name)
 
     def test_private_branch_as_user(self):
-        # We resolve invisible branches as if they don't exist.
+        # We resolve private branches as if they don't exist.
         #
         # References to a product resolve to the branch associated with the
         # development focus. If that branch is private, other views will
@@ -436,7 +340,7 @@
         owner = self.factory.makePerson()
         branch = self.factory.makeAnyBranch(owner=owner, private=True)
         path = removeSecurityProxy(branch).unique_name
-        self.assertResolves(path, path)
+        self.assertOnlyWritableResolves(path)
 
     def test_remote_branch(self):
         # For remote branches, return results that link to the actual remote