Merge ~cjwatson/launchpad:branch-repository-api-redirects into launchpad:master

[Colin Watson]

Send proper webservice redirects for branches and repositories

launchpadlib handles this correctly nowadays, but only if we send redirects to the correct target host (e.g. api.launchpad.net rather than code.launchpad.net).

[Ioana Lasc (ilasc)]

review: Approve

diff --git a/lib/lp/app/browser/launchpad.py b/lib/lp/app/browser/launchpad.py
index d31df20..eb4b9bb 100644
--- a/lib/lp/app/browser/launchpad.py
+++ b/lib/lp/app/browser/launchpad.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Browser code for the launchpad application."""
@@ -739,7 +739,7 @@ class LaunchpadRootNavigation(Navigation):
         path = '/'.join(self.request.stepstogo)
         try:
             branch, trailing = getUtility(IBranchLookup).getByLPPath(path)
-            target_url = canonical_url(branch)
+            target_url = canonical_url(branch, request=self.request)
             if trailing != '':
                 target_url = urlappend(target_url, trailing)
         except (NoLinkedBranch) as e:
@@ -789,7 +789,7 @@ class LaunchpadRootNavigation(Navigation):
         try:
             repository, trailing = getUtility(IGitLookup).getByPath(path)
             if repository is not None:
-                target_url = canonical_url(repository)
+                target_url = canonical_url(repository, request=self.request)
                 if trailing:
                     target_url = urlappend(target_url, trailing)
         except (InvalidNamespace, InvalidProductName, NameLookupFailed,
@@ -802,7 +802,7 @@ class LaunchpadRootNavigation(Navigation):
         # Attempt a bzr lookup as well:
         try:
             branch, trailing = getUtility(IBranchLookup).getByLPPath(path)
-            bzr_url = canonical_url(branch)
+            bzr_url = canonical_url(branch, request=self.request)
             if trailing != '':
                 bzr_url = urlappend(bzr_url, trailing)
 
diff --git a/lib/lp/app/browser/tests/test_launchpad.py b/lib/lp/app/browser/tests/test_launchpad.py
index a9c6adf..b170634 100644
--- a/lib/lp/app/browser/tests/test_launchpad.py
+++ b/lib/lp/app/browser/tests/test_launchpad.py
@@ -36,7 +36,10 @@ from lp.services.webapp.interfaces import (
     BrowserNotificationLevel,
     ILaunchpadRoot,
     )
-from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.services.webapp.servers import (
+    LaunchpadTestRequest,
+    WebServiceTestRequest,
+    )
 from lp.services.webapp.url import urlappend
 from lp.testing import (
     admin_logged_in,
@@ -108,11 +111,12 @@ class TraversalMixin:
             NotFound, self.traverse, path,
             use_default_referer=use_default_referer)
 
-    def assertRedirects(self, segments, url):
-        redirection = self.traverse(segments)
+    def assertRedirects(self, segments, url, webservice=False):
+        redirection = self.traverse(segments, webservice=webservice)
         self.assertEqual(url, redirection.target)
 
-    def traverse(self, path, first_segment, use_default_referer=True):
+    def traverse(self, path, first_segment, use_default_referer=True,
+                 webservice=False):
         """Traverse to 'path' using a 'LaunchpadRootNavigation' object.
 
         Using the Zope traversal machinery, traverse to the path given by
@@ -125,6 +129,8 @@ class TraversalMixin:
         :param use_default_referer: If True, set the referer attribute in the
             request header to DEFAULT_REFERER = "http://launchpad.test"
             (otherwise it remains as None)
+        :param webservice: If True, use a webservice-like request rather
+            than a normal test request.
         :return: The object found.
         """
         # XXX: What's the difference between first_segment and path? -- mbp
@@ -132,7 +138,9 @@ class TraversalMixin:
         extra = {'PATH_INFO': urlappend('/%s' % first_segment, path)}
         if use_default_referer:
             extra['HTTP_REFERER'] = DEFAULT_REFERER
-        request = LaunchpadTestRequest(**extra)
+        request_factory = (
+            WebServiceTestRequest if webservice else LaunchpadTestRequest)
+        request = request_factory(**extra)
         segments = reversed(path.split('/'))
         request.setTraversalStack(segments)
         traverser = LaunchpadRootNavigation(
@@ -168,6 +176,9 @@ class TestBranchTraversal(TestCaseWithFactory, TraversalMixin):
         # branch.
         branch = self.factory.makeAnyBranch()
         self.assertRedirects(branch.unique_name, canonical_url(branch))
+        self.assertRedirects(
+            branch.unique_name, canonical_url(branch, rootsite='api'),
+            webservice=True)
 
     def test_no_such_unique_name(self):
         # Traversing to /+branch/<unique_name> where 'unique_name' is for a
@@ -346,10 +357,16 @@ class TestCodeTraversal(TestCaseWithFactory, TraversalMixin):
     def test_project_bzr_branch(self):
         branch = self.factory.makeAnyBranch()
         self.assertRedirects(branch.unique_name, canonical_url(branch))
+        self.assertRedirects(
+            branch.unique_name, canonical_url(branch, rootsite='api'),
+            webservice=True)
 
     def test_project_git_branch(self):
         git_repo = self.factory.makeGitRepository()
         self.assertRedirects(git_repo.unique_name, canonical_url(git_repo))
+        self.assertRedirects(
+            git_repo.unique_name, canonical_url(git_repo, rootsite='api'),
+            webservice=True)
 
     def test_no_such_bzr_unique_name(self):
         branch = self.factory.makeAnyBranch()
diff --git a/lib/lp/code/browser/tests/test_branchtraversal.py b/lib/lp/code/browser/tests/test_branchtraversal.py
index 98c9f75..c26a699 100644
--- a/lib/lp/code/browser/tests/test_branchtraversal.py
+++ b/lib/lp/code/browser/tests/test_branchtraversal.py
@@ -16,10 +16,11 @@ from lp.registry.interfaces.personproduct import (
     IPersonProductFactory,
     )
 from lp.services.webapp.publisher import canonical_url
-from lp.testing import (
-    FakeLaunchpadRequest,
-    TestCaseWithFactory,
+from lp.services.webapp.servers import (
+    LaunchpadTestRequest,
+    WebServiceTestRequest,
     )
+from lp.testing import TestCaseWithFactory
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -36,11 +37,11 @@ class TestPersonBranchTraversal(TestCaseWithFactory):
         TestCaseWithFactory.setUp(self)
         self.person = self.factory.makePerson()
 
-    def assertRedirects(self, segments, url):
-        redirection = self.traverse(segments)
+    def assertRedirects(self, segments, url, webservice=False):
+        redirection = self.traverse(segments, webservice=webservice)
         self.assertEqual(url, redirection.target)
 
-    def traverse(self, segments):
+    def traverse(self, segments, webservice=False):
         """Traverse to 'segments' using a 'PersonNavigation' object.
 
         Using the Zope traversal machinery, traverse to the path given by
@@ -48,11 +49,16 @@ class TestPersonBranchTraversal(TestCaseWithFactory):
         'person' attribute.
 
         :param segments: A list of path segments.
+        :param webservice: If True, use a webservice-like request rather
+            than a normal test request.
         :return: The object found.
         """
         stack = list(reversed(segments))
         name = stack.pop()
-        request = FakeLaunchpadRequest(['~' + self.person.name], stack)
+        request_factory = (
+            WebServiceTestRequest if webservice else LaunchpadTestRequest)
+        request = request_factory()
+        request.setTraversalStack(stack)
         traverser = PersonNavigation(self.person, request)
         return traverser.publishTraverse(request, name)
 
@@ -60,11 +66,15 @@ class TestPersonBranchTraversal(TestCaseWithFactory):
         branch = self.factory.makeProductBranch(owner=self.person)
         segments = ['+branch', branch.product.name, branch.name]
         self.assertRedirects(segments, canonical_url(branch))
+        self.assertRedirects(
+            segments, canonical_url(branch, rootsite='api'), webservice=True)
 
     def test_redirect_junk_branch(self):
         branch = self.factory.makePersonalBranch(owner=self.person)
         segments = ['+branch', '+junk', branch.name]
         self.assertRedirects(segments, canonical_url(branch))
+        self.assertRedirects(
+            segments, canonical_url(branch, rootsite='api'), webservice=True)
 
     def test_redirect_branch_not_found(self):
         self.assertRaises(
@@ -78,6 +88,10 @@ class TestPersonBranchTraversal(TestCaseWithFactory):
             ['foo', branch.distroseries.name, branch.sourcepackagename.name,
              branch.name],
             canonical_url(branch))
+        self.assertRedirects(
+            ['foo', branch.distroseries.name, branch.sourcepackagename.name,
+             branch.name],
+            canonical_url(branch, rootsite='api'), webservice=True)
 
     def test_junk_branch(self):
         branch = self.factory.makePersonalBranch(owner=self.person)
@@ -134,8 +148,8 @@ class TestPersonProductBranchTraversal(TestCaseWithFactory):
         """
         stack = list(reversed(segments))
         name = stack.pop()
-        request = FakeLaunchpadRequest(
-            ['~' + self.person.name, self.product.name], stack)
+        request = LaunchpadTestRequest()
+        request.setTraversalStack(stack)
         traverser = PersonProductNavigation(self.person_product, request)
         return traverser.publishTraverse(request, name)
 
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index facd4d2..193df25 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -364,7 +364,8 @@ class BranchTraversalMixin:
         """Redirect to canonical_url."""
         branch = getUtility(IBranchNamespaceSet).traverse(self._getSegments())
         if branch:
-            return self.redirectSubTree(canonical_url(branch))
+            return self.redirectSubTree(
+                canonical_url(branch, request=self.request))
         raise NotFoundError
 
     @stepthrough('+git')
@@ -414,7 +415,8 @@ class BranchTraversalMixin:
             if using_pillar_alias:
                 # This repository was accessed through one of its project's
                 # aliases, so we must redirect to its canonical URL.
-                return self.redirectSubTree(canonical_url(repository))
+                return self.redirectSubTree(
+                    canonical_url(repository, request=self.request))
 
             return repository
         except (NotFoundError, InvalidNamespace, InvalidProductName):
@@ -478,13 +480,15 @@ class BranchTraversalMixin:
             if branch.product.name != pillar_name:
                 # This branch was accessed through one of its project's
                 # aliases, so we must redirect to its canonical URL.
-                return self.redirectSubTree(canonical_url(branch))
+                return self.redirectSubTree(
+                    canonical_url(branch, request=self.request))
 
         if branch.distribution is not None:
             if branch.distribution.name != pillar_name:
                 # This branch was accessed through one of its distribution's
                 # aliases, so we must redirect to its canonical URL.
-                return self.redirectSubTree(canonical_url(branch))
+                return self.redirectSubTree(
+                    canonical_url(branch, request=self.request))
 
         return branch
 
diff --git a/lib/lp/registry/browser/tests/test_person.py b/lib/lp/registry/browser/tests/test_person.py
index 2008a61..07ef8d4 100644
--- a/lib/lp/registry/browser/tests/test_person.py
+++ b/lib/lp/registry/browser/tests/test_person.py
@@ -184,6 +184,32 @@ class TestPersonNavigation(TestCaseWithFactory):
             repository.owner.name, project.name, repository.name)
         self.assertEqual(repository, test_traverse(url)[0])
 
+    def test_traverse_git_repository_project_alias(self):
+        project = self.factory.makeProduct()
+        alias = project.name + "-2"
+        removeSecurityProxy(project).setAliases([alias])
+        repository = self.factory.makeGitRepository(target=project)
+        url = "/~%s/%s/+git/%s" % (
+            repository.owner.name, alias, repository.name)
+        expected_url = "http://code.launchpad.test/~%s/%s/+git/%s" % (
+            repository.owner.name, project.name, repository.name)
+        _, view, _ = test_traverse(url)
+        self.assertIsInstance(view, RedirectionView)
+        self.assertEqual(expected_url, removeSecurityProxy(view).target)
+
+    def test_traverse_git_repository_project_alias_api(self):
+        project = self.factory.makeProduct()
+        alias = project.name + "-2"
+        removeSecurityProxy(project).setAliases([alias])
+        repository = self.factory.makeGitRepository(target=project)
+        url = "http://api.launchpad.test/devel/~%s/%s/+git/%s" % (
+            repository.owner.name, alias, repository.name)
+        expected_url = "http://api.launchpad.test/devel/~%s/%s/+git/%s" % (
+            repository.owner.name, project.name, repository.name)
+        _, view, _ = test_traverse(url)
+        self.assertIsInstance(view, RedirectionView)
+        self.assertEqual(expected_url, removeSecurityProxy(view).target)
+
     def test_traverse_git_repository_package(self):
         dsp = self.factory.makeDistributionSourcePackage()
         repository = self.factory.makeGitRepository(target=dsp)
@@ -192,6 +218,38 @@ class TestPersonNavigation(TestCaseWithFactory):
             dsp.sourcepackagename.name, repository.name)
         self.assertEqual(repository, test_traverse(url)[0])
 
+    def test_traverse_git_repository_package_alias(self):
+        dsp = self.factory.makeDistributionSourcePackage()
+        alias = dsp.distribution.name + "-2"
+        removeSecurityProxy(dsp.distribution).setAliases([alias])
+        repository = self.factory.makeGitRepository(target=dsp)
+        url = "/~%s/%s/+source/%s/+git/%s" % (
+            repository.owner.name, alias,
+            dsp.sourcepackagename.name, repository.name)
+        expected_url = (
+            "http://code.launchpad.test/~%s/%s/+source/%s/+git/%s" % (
+                repository.owner.name, dsp.distribution.name,
+                dsp.sourcepackagename.name, repository.name))
+        _, view, _ = test_traverse(url)
+        self.assertIsInstance(view, RedirectionView)
+        self.assertEqual(expected_url, removeSecurityProxy(view).target)
+
+    def test_traverse_git_repository_package_alias_api(self):
+        dsp = self.factory.makeDistributionSourcePackage()
+        alias = dsp.distribution.name + "-2"
+        removeSecurityProxy(dsp.distribution).setAliases([alias])
+        repository = self.factory.makeGitRepository(target=dsp)
+        url = "http://api.launchpad.test/devel/~%s/%s/+source/%s/+git/%s" % (
+            repository.owner.name, alias,
+            dsp.sourcepackagename.name, repository.name)
+        expected_url = (
+            "http://api.launchpad.test/devel/~%s/%s/+source/%s/+git/%s" % (
+                repository.owner.name, dsp.distribution.name,
+                dsp.sourcepackagename.name, repository.name))
+        _, view, _ = test_traverse(url)
+        self.assertIsInstance(view, RedirectionView)
+        self.assertEqual(expected_url, removeSecurityProxy(view).target)
+
     def test_traverse_git_repository_oci_project(self):
         oci_project = self.factory.makeOCIProject()
         repository = self.factory.makeGitRepository(target=oci_project)