launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/ppa-distro-traversal into lp:launchpad with lp:~wgrant/launchpad/getPPAByName-cross-distro as a prerequisite.

Commit message:
Introduce a third PPA URL form: /~wgrant/+archive/ubuntu/ppa. Feature-flagged for now, but it will replace /~wgrant/+archive/ppa.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/ppa-distro-traversal/+merge/225600

Introduce a third PPA URL form: /~wgrant/+archive/ubuntu/ppa. This lets us support non-Ubuntu PPAs. /~wgrant/+archive/ppa now redirects, and the old /~wgrant/+archive redirect still works.

1.0 API clients are exempt from the /~wgrant/+archive/ppa redirect, since the Python 2 implementation of add-apt-repository doesn't follow redirects. Those clients will get a 200 at the old URL, but the returned resource will still include the new URLs.

The soyuz.ppa.distroful_urls feature flag controls which form is canonical, just in case we discover that something breaks. It will be removed soon.
-- 
https://code.launchpad.net/~wgrant/launchpad/ppa-distro-traversal/+merge/225600
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ppa-distro-traversal into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2014-05-08 14:40:02 +0000
+++ lib/lp/registry/browser/person.py	2014-07-07 02:30:47 +0000
@@ -192,6 +192,7 @@
 from lp.registry.model.person import get_recipients
 from lp.services.config import config
 from lp.services.database.sqlbase import flush_database_updates
+from lp.services.features import getFeatureFlag
 from lp.services.feeds.browser import FeedsMixin
 from lp.services.geoip.interfaces import IRequestPreferredLanguages
 from lp.services.gpg.interfaces import (
@@ -408,30 +409,51 @@
 
     @stepto('+archive')
     def traverse_archive(self):
-
-        if self.request.stepstogo:
-            # If the URL has something that could be a PPA name in it,
-            # use that, but just in case it fails, keep a copy
-            # of the traversal stack so we can try using the default
-            # archive afterwards:
-            traversal_stack = self.request.getTraversalStack()
-            ppa_name = self.request.stepstogo.consume()
-
-            try:
-                from lp.soyuz.browser.archive import traverse_named_ppa
-                return traverse_named_ppa(self.context.name, ppa_name)
-            except NotFoundError:
-                self.request.setTraversalStack(traversal_stack)
-                # and simply continue below...
-
-        # Otherwise try to get the default PPA and if it exists redirect
-        # to the new-style URL, if it doesn't, return None (to trigger a
-        # NotFound error).
-        default_ppa = self.context.archive
-        if default_ppa is None:
-            return None
-
-        return self.redirectSubTree(canonical_url(default_ppa))
+        from lp.soyuz.browser.archive import traverse_named_ppa
+
+        # 1.0 API requests are exempt from non-canonical redirects,
+        # since some manually construct URLs and don't cope with
+        # redirects (most notably the Python 2 implementation of
+        # apt-add-repository).
+        redirect_allowed = not (
+            IWebServiceClientRequest.providedBy(self.request)
+            and self.request.annotations.get(
+                self.request.VERSION_ANNOTATION) == '1.0')
+
+        # There are three cases, in order of preference:
+        #  - 2014 onwards: /~wgrant/+archive/ubuntu/ppa:
+        #    The next two URL segments are names of a distribution and a PPA.
+        #
+        #  - 2009-2014: /~wgrant/+archive/ppa:
+        #    The distribution is assumed to be "ubuntu".
+        #
+        #  - 2007-2009: /~wgrant/+archive:
+        #    The distribution is assumed to be "ubuntu" and the PPA "ppa".
+        #
+        # Only the first is canonical, with the others redirecting to it.
+        distroful_urls = bool(getFeatureFlag('soyuz.ppa.distroful_urls'))
+        bits = list(reversed(self.request.getTraversalStack()[-2:]))
+        attempts = []
+        if len(bits) == 2:
+            attempts.append(
+                (bits[0], bits[1], 2, redirect_allowed and not distroful_urls))
+        if len(bits) >= 1:
+            attempts.append(
+                ("ubuntu", bits[0], 1, redirect_allowed and distroful_urls))
+        attempts.append(("ubuntu", "ppa", 0, True))
+
+        # Go through the attempts in order.
+        for distro, ppa, segments, redirect in attempts:
+            ppa = traverse_named_ppa(self.context, distro, ppa)
+            if ppa is not None:
+                for i in range(segments):
+                    self.request.stepstogo.consume()
+                if redirect:
+                    return self.redirectSubTree(
+                        canonical_url(ppa, request=self.request))
+                else:
+                    return ppa
+        return None
 
     @stepthrough('+email')
     def traverse_email(self, email):

=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py	2014-06-19 06:38:53 +0000
+++ lib/lp/registry/browser/tests/test_person.py	2014-07-07 02:30:47 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -15,9 +15,11 @@
     LessThan,
     Not,
     )
+from testtools.testcase import ExpectedException
 import transaction
 from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.browser.lazrjs import TextAreaEditorWidget
 from lp.app.enums import InformationType
@@ -39,6 +41,7 @@
 from lp.registry.model.milestone import milestone_sort_key
 from lp.scripts.garbo import PopulateLatestPersonSourcePackageReleaseCache
 from lp.services.config import config
+from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
 from lp.services.log.logger import FakeLogger
@@ -49,6 +52,7 @@
 from lp.services.webapp import canonical_url
 from lp.services.webapp.escaping import html_escape
 from lp.services.webapp.interfaces import ILaunchBag
+from lp.services.webapp.publisher import RedirectionView
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -81,12 +85,80 @@
     find_tag_by_id,
     setupBrowserForUser,
     )
+from lp.testing.publication import test_traverse
 from lp.testing.views import (
     create_initialized_view,
     create_view,
     )
 
 
+class TestPersonNavigation(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def assertRedirect(self, path, redirect):
+        view = test_traverse(path)[1]
+        self.assertIsInstance(view, RedirectionView)
+        self.assertEqual(':/' + redirect, removeSecurityProxy(view).target)
+
+    def test_traverse_archive_distroful(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        in_suf = '/~%s/+archive/%s/%s' % (
+            archive.owner.name, archive.distribution.name, archive.name)
+        old_suf = '/~%s/+archive/%s' % (archive.owner.name, archive.name)
+        # A feature flag makes the distroful URLs canonical.
+        with FeatureFixture({'soyuz.ppa.distroful_urls': 'on'}):
+            self.assertEqual(archive, test_traverse(in_suf)[0])
+            self.assertEqual(archive, test_traverse('/api/devel' + in_suf)[0])
+            self.assertEqual(archive, test_traverse('/api/1.0' + in_suf)[0])
+        # Without the flag set requests will redirect to the old URL,
+        # unless a request is on the 1.0 API.
+        self.assertRedirect(in_suf, old_suf)
+        self.assertRedirect('/api/devel' + in_suf, '/api/devel' + old_suf)
+        self.assertEqual(archive, test_traverse('/api/1.0' + in_suf)[0])
+
+    def test_traverse_archive_distroless(self):
+        # Pre-mid-2014 distroless PPA URLs redirect to the new ones.
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        in_suf = '/~%s/+archive/%s' % (archive.owner.name, archive.name)
+        out_suf = '/~%s/+archive/%s/%s' % (
+            archive.owner.name, archive.distribution.name, archive.name)
+        with FeatureFixture({'soyuz.ppa.distroful_urls': 'on'}):
+            self.assertRedirect(in_suf, out_suf)
+            self.assertRedirect('/api/devel' + in_suf, '/api/devel' + out_suf)
+            # 1.0 API requests don't redirect, since some manually construct
+            # URLs and don't cope with redirects (most notably the Python 2
+            # implementation of apt-add-repository).
+            self.assertEqual(archive, test_traverse('/api/1.0' + out_suf)[0])
+        self.assertEqual(archive, test_traverse(in_suf)[0])
+        self.assertEqual(archive, test_traverse('/api/devel' + in_suf)[0])
+        self.assertEqual(archive, test_traverse('/api/1.0' + in_suf)[0])
+
+    def test_traverse_archive_distroless_implies_ubuntu(self):
+        # The distroless PPA redirect only finds Ubuntu PPAs, since
+        # distroful URLs were implemented as a requirement for
+        # non-Ubuntu PPAs.
+        other_archive = self.factory.makeArchive(
+            purpose=ArchivePurpose.PPA,
+            distribution=self.factory.makeDistribution())
+        with ExpectedException(NotFound):
+            test_traverse(
+                '/~%s/+archive/%s' % (
+                    other_archive.owner.name, other_archive.name))
+
+    def test_traverse_archive_redirects_nameless(self):
+        # Pre-2009 nameless PPA URLs redirect to the new ones.
+        archive = self.factory.makeArchive(
+            purpose=ArchivePurpose.PPA, name="ppa")
+        in_suf = '/~%s/+archive' % archive.owner.name
+        out_suf = '/~%s/+archive/%s/%s' % (
+            archive.owner.name, archive.distribution.name, archive.name)
+        with FeatureFixture({'soyuz.ppa.distroful_urls': 'on'}):
+            self.assertRedirect(in_suf, out_suf)
+            self.assertRedirect('/api/devel' + in_suf, '/api/devel' + out_suf)
+            self.assertRedirect('/api/1.0' + in_suf, '/api/1.0' + out_suf)
+
+
 class PersonViewOpenidIdentityUrlTestCase(TestCaseWithFactory):
     """Tests for the public OpenID identifier shown on the profile page."""
 

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2014-07-05 00:22:24 +0000
+++ lib/lp/soyuz/browser/archive.py	2014-07-07 02:30:47 +0000
@@ -84,6 +84,7 @@
     ISourcePackageRecipeBuildSource,
     )
 from lp.registry.enums import PersonVisibility
+from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -94,6 +95,7 @@
     get_user_agent_distroseries,
     )
 from lp.services.database.bulk import load_related
+from lp.services.features import getFeatureFlag
 from lp.services.helpers import english_list
 from lp.services.job.model.job import Job
 from lp.services.librarian.browser import FileNavigationMixin
@@ -176,20 +178,20 @@
         return "This archive is private."
 
 
-def traverse_named_ppa(person_name, ppa_name):
+def traverse_named_ppa(person, distro_name, ppa_name):
     """For PPAs, traverse the right place.
 
-    :param person_name: The person part of the URL
-    :param ppa_name: The PPA name part of the URL
+    :param person: The PPA owner.
+    :param distro_name: The Distribution name part of the URL.
+    :param ppa_name: The PPA name part of the URL.
     """
-    person = getUtility(IPersonSet).getByName(person_name)
+    distro = getUtility(IDistributionSet).getByName(distro_name)
+    if distro is None:
+        return None
     try:
-        archive = person.getPPAByName(
-            getUtility(ILaunchpadCelebrities).ubuntu, ppa_name)
+        return person.getPPAByName(distro, ppa_name)
     except NoSuchPPA:
-        raise NotFoundError("%s/%s", (person_name, ppa_name))
-
-    return archive
+        return None
 
 
 class DistributionArchiveURL:
@@ -228,7 +230,11 @@
 
     @property
     def path(self):
-        return u"+archive/%s" % self.context.name
+        if getFeatureFlag('soyuz.ppa.distroful_urls'):
+            return u"+archive/%s/%s" % (
+                self.context.distribution.name, self.context.name)
+        else:
+            return u"+archive/%s" % self.context.name
 
 
 class ArchiveNavigation(Navigation, FileNavigationMixin):