Merge ~cjwatson/launchpad:distribution-traversal into launchpad:master

[Colin Watson]

Add configurable distribution default traversal rules

We can use redirect_default_traversal to configure whether the canonical URL for a distroseries is (e.g.) /ubuntu/focal or /ubuntu/+series/focal. In the future we'll also be able to use default_traversal_policy to configure the default traversal for a distribution to mean something else: for example it might well make sense to use it for distribution source packages rather than for series.

DB MP: https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/393730

[Thiago F. Pappacena (pappacena)]

LGTM

review: Approve

[Otto Co-Pilot (otto-copilot)]

Merging failed
https://jenkins.ols.canonical.com/online-services/job/launchpad/946/

diff --git a/lib/lp/registry/browser/configure.zcml b/lib/lp/registry/browser/configure.zcml
index a718640..777652f 100644
--- a/lib/lp/registry/browser/configure.zcml
+++ b/lib/lp/registry/browser/configure.zcml
@@ -99,8 +99,7 @@
         />
     <browser:url
         for="lp.registry.interfaces.distroseries.IDistroSeries"
-        path_expression="name"
-        attribute_to_parent="distribution"
+        urldata="lp.registry.browser.distroseries.DistroSeriesURL"
         />
     <browser:defaultView
         name="+index"
diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index 8d566e0..04d6dc1 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -95,6 +95,7 @@ from lp.registry.browser.pillar import (
     PillarNavigationMixin,
     PillarViewMixin,
     )
+from lp.registry.enums import DistributionDefaultTraversalPolicy
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionMirrorMenuMarker,
@@ -189,13 +190,28 @@ class DistributionNavigation(
 
     @stepthrough('+series')
     def traverse_series(self, name):
-        series, _ = self._resolveSeries(name)
-        return self.redirectSubTree(
-            canonical_url(series, request=self.request), status=303)
+        series, redirect = self._resolveSeries(name)
+        if not redirect:
+            policy = self.context.default_traversal_policy
+            if (policy == DistributionDefaultTraversalPolicy.SERIES and
+                    not self.context.redirect_default_traversal):
+                redirect = True
+        if redirect:
+            return self.redirectSubTree(
+                canonical_url(series, request=self.request), status=303)
+        else:
+            return series
 
     def traverse(self, name):
-        series, is_alias = self._resolveSeries(name)
-        if is_alias:
+        series, redirect = self._resolveSeries(name)
+        if series is None:
+            return None
+        if not redirect:
+            policy = self.context.default_traversal_policy
+            if (policy == DistributionDefaultTraversalPolicy.SERIES and
+                    self.context.redirect_default_traversal):
+                redirect = True
+        if redirect:
             return self.redirectSubTree(
                 canonical_url(series, request=self.request), status=303)
         else:
@@ -965,6 +981,8 @@ class DistributionEditView(RegistryEditFormView,
         'translations_usage',
         'answers_usage',
         'translation_focus',
+        'default_traversal_policy',
+        'redirect_default_traversal',
         ]
 
     custom_widget_icon = CustomWidgetFactory(
diff --git a/lib/lp/registry/browser/distroseries.py b/lib/lp/registry/browser/distroseries.py
index 89114b9..75c0468 100644
--- a/lib/lp/registry/browser/distroseries.py
+++ b/lib/lp/registry/browser/distroseries.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 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).
 
 """View classes related to `IDistroSeries`."""
@@ -17,6 +17,7 @@ __all__ = [
     'DistroSeriesPackageSearchView',
     'DistroSeriesPackagesView',
     'DistroSeriesUniquePackagesView',
+    'DistroSeriesURL',
     'DistroSeriesView',
     ]
 
@@ -27,7 +28,10 @@ from zope.component import getUtility
 from zope.event import notify
 from zope.formlib import form
 from zope.formlib.widget import CustomWidgetFactory
-from zope.interface import Interface
+from zope.interface import (
+    implementer,
+    Interface,
+    )
 from zope.lifecycleevent import ObjectCreatedEvent
 from zope.schema import (
     Choice,
@@ -67,6 +71,7 @@ from lp.registry.browser import (
     MilestoneOverlayMixin,
     )
 from lp.registry.enums import (
+    DistributionDefaultTraversalPolicy,
     DistroSeriesDifferenceStatus,
     DistroSeriesDifferenceType,
     )
@@ -86,6 +91,7 @@ from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.batching import BatchNavigator
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.escaping import structured
+from lp.services.webapp.interfaces import ICanonicalUrlData
 from lp.services.webapp.menu import (
     ApplicationMenu,
     enabled_with_permission,
@@ -129,6 +135,34 @@ def get_dsd_source():
     return getUtility(IDistroSeriesDifferenceSource)
 
 
+@implementer(ICanonicalUrlData)
+class DistroSeriesURL:
+    """Distro series URL creation rules.
+
+    The canonical URL for a distro series depends on the values of
+    `default_traversal_policy` and `redirect_default_traversal` on the
+    context distribution.
+    """
+
+    rootsite = None
+
+    def __init__(self, context):
+        self.context = context
+
+    @property
+    def inside(self):
+        return self.context.distribution
+
+    @property
+    def path(self):
+        policy = self.context.distribution.default_traversal_policy
+        if (policy == DistributionDefaultTraversalPolicy.SERIES and
+                not self.context.distribution.redirect_default_traversal):
+            return self.context.name
+        else:
+            return u"+series/%s" % self.context.name
+
+
 class DistroSeriesNavigation(GetitemNavigation, BugTargetTraversalMixin,
     StructuralSubscriptionTargetTraversalMixin):
 
diff --git a/lib/lp/registry/browser/tests/distribution-views.txt b/lib/lp/registry/browser/tests/distribution-views.txt
index 810f118..533d559 100644
--- a/lib/lp/registry/browser/tests/distribution-views.txt
+++ b/lib/lp/registry/browser/tests/distribution-views.txt
@@ -132,7 +132,8 @@ The view accepts most of the distribution fields.
      'package_derivatives_email', 'icon',
      'logo', 'mugshot', 'official_malone', 'enable_bug_expiration',
      'blueprints_usage', 'translations_usage', 'answers_usage',
-     'translation_focus']
+     'translation_focus',
+     'default_traversal_policy', 'redirect_default_traversal']
 
     >>> del form['field.name']
     >>> del form['field.actions.save']
diff --git a/lib/lp/registry/browser/tests/test_distribution.py b/lib/lp/registry/browser/tests/test_distribution.py
index d47eb99..b92d7b2 100644
--- a/lib/lp/registry/browser/tests/test_distribution.py
+++ b/lib/lp/registry/browser/tests/test_distribution.py
@@ -72,6 +72,42 @@ class TestDistributionNavigation(TestCaseWithFactory):
             "http://launchpad.test/%s/%s" % (
                 distroseries.distribution.name, distroseries.name))
 
+    def test_classic_series_url_redirects(self):
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        self.assertRedirects(
+            "http://launchpad.test/%s/%s" % (
+                distroseries.distribution.name, distroseries.name),
+            "http://launchpad.test/%s/+series/%s" % (
+                distroseries.distribution.name, distroseries.name))
+
+    def test_classic_series_url_with_alias_redirects(self):
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        distroseries.distribution.development_series_alias = "devel"
+        self.assertRedirects(
+            "http://launchpad.test/%s/devel" % distroseries.distribution.name,
+            "http://launchpad.test/%s/+series/%s" % (
+                distroseries.distribution.name, distroseries.name))
+
+    def test_new_series_url(self):
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        obj, _, _ = test_traverse(
+            "http://launchpad.test/%s/+series/%s" % (
+                distroseries.distribution.name, distroseries.name))
+        self.assertEqual(distroseries, obj)
+
+    def test_new_series_url_with_alias(self):
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        distroseries.distribution.development_series_alias = "devel"
+        self.assertRedirects(
+            "http://launchpad.test/%s/+series/devel" % (
+                distroseries.distribution.name),
+            "http://launchpad.test/%s/+series/%s" % (
+                distroseries.distribution.name, distroseries.name))
+
     def test_new_series_url_redirects(self):
         distroseries = self.factory.makeDistroSeries()
         self.assertRedirects(
@@ -97,6 +133,54 @@ class TestDistributionNavigation(TestCaseWithFactory):
         self.assertIsInstance(marshaller.dereference_url(url), RedirectionView)
         self.assertEqual(expected_obj, marshaller.marshall_from_json_data(url))
 
+    def test_classic_series_url_supports_object_lookup(self):
+        # Classic series URLs (without +series) are compatible with
+        # webservice object lookup, even if the distribution is configured
+        # to redirect the default traversal.
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        distroseries_url = "/%s/%s" % (
+            distroseries.distribution.name, distroseries.name)
+        self.assertDereferences(distroseries_url, distroseries)
+
+        # Objects subordinate to the redirected series work too.
+        distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=distroseries)
+        distroarchseries_url = "/%s/%s/%s" % (
+            distroarchseries.distroseries.distribution.name,
+            distroarchseries.distroseries.name,
+            distroarchseries.architecturetag)
+        self.assertDereferences(distroarchseries_url, distroarchseries)
+
+    def test_classic_series_url_supports_object_lookup_https(self):
+        # Classic series URLs (without +series) are compatible with
+        # webservice object lookup, even if the distribution is configured
+        # to redirect the default traversal and the vhost is configured to
+        # use HTTPS.  "SERVER_URL": None exposes a bug in lazr.restful <
+        # 0.22.2.
+        self.addCleanup(allvhosts.reload)
+        self.pushConfig("vhosts", use_https=True)
+        allvhosts.reload()
+
+        distroseries = self.factory.makeDistroSeries()
+        distroseries.distribution.redirect_default_traversal = True
+        distroseries_url = "/%s/%s" % (
+            distroseries.distribution.name, distroseries.name)
+        self.assertDereferences(
+            distroseries_url, distroseries,
+            environ={"HTTPS": "on", "SERVER_URL": None})
+
+        # Objects subordinate to the redirected series work too.
+        distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=distroseries)
+        distroarchseries_url = "/%s/%s/%s" % (
+            distroarchseries.distroseries.distribution.name,
+            distroarchseries.distroseries.name,
+            distroarchseries.architecturetag)
+        self.assertDereferences(
+            distroarchseries_url, distroarchseries,
+            environ={"HTTPS": "on", "SERVER_URL": None})
+
     def test_new_series_url_supports_object_lookup(self):
         # New-style +series URLs are compatible with webservice object
         # lookup.
diff --git a/lib/lp/registry/configure.zcml b/lib/lp/registry/configure.zcml
index 1c73f4b..659b8d9 100644
--- a/lib/lp/registry/configure.zcml
+++ b/lib/lp/registry/configure.zcml
@@ -1820,6 +1820,7 @@
             set_attributes="
                 bug_reported_acknowledgement
                 bug_reporting_guidelines
+                default_traversal_policy
                 description
                 development_series_alias
                 display_name
@@ -1837,6 +1838,7 @@
                 official_malone
                 owner
                 package_derivatives_email
+                redirect_default_traversal
                 redirect_release_uploads
                 summary
                 vcs
diff --git a/lib/lp/registry/enums.py b/lib/lp/registry/enums.py
index 631ba6f..30f4a79 100644
--- a/lib/lp/registry/enums.py
+++ b/lib/lp/registry/enums.py
@@ -1,4 +1,4 @@
-# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Enums for the Registry app."""
@@ -7,6 +7,7 @@ __metaclass__ = type
 __all__ = [
     'BranchSharingPolicy',
     'BugSharingPolicy',
+    'DistributionDefaultTraversalPolicy',
     'DistroSeriesDifferenceStatus',
     'DistroSeriesDifferenceType',
     'EXCLUSIVE_TEAM_POLICY',
@@ -425,3 +426,19 @@ class VCSType(DBEnumeratedType):
 
         The Git DVCS is used as the default project or distribution VCS.
         """)
+
+
+class DistributionDefaultTraversalPolicy(DBEnumeratedType):
+    """Policy for the default traversal from a distribution.
+
+    This determines what the "name" segment in a URL such as
+    "/{distro}/{name}" (with no intervening segment such as "+source")
+    means.
+    """
+
+    SERIES = DBItem(0, """
+        Series
+
+        The default traversal from a distribution is used for series of that
+        distribution.
+        """)
diff --git a/lib/lp/registry/interfaces/distribution.py b/lib/lp/registry/interfaces/distribution.py
index ecd6c49..8c23b90 100644
--- a/lib/lp/registry/interfaces/distribution.py
+++ b/lib/lp/registry/interfaces/distribution.py
@@ -73,7 +73,10 @@ from lp.bugs.interfaces.bugtarget import (
 from lp.bugs.interfaces.structuralsubscription import (
     IStructuralSubscriptionTarget,
     )
-from lp.registry.enums import VCSType
+from lp.registry.enums import (
+    DistributionDefaultTraversalPolicy,
+    VCSType,
+    )
 from lp.registry.interfaces.announcement import IMakesAnnouncements
 from lp.registry.interfaces.distributionmirror import IDistributionMirror
 from lp.registry.interfaces.distroseries import DistroSeriesNameField
@@ -390,6 +393,20 @@ class IDistributionPublic(
             description=_(
                 "Version control system for this distribution's code.")))
 
+    default_traversal_policy = exported(Choice(
+        title=_("Default traversal policy"),
+        description=_(
+            "The type of object that /{distro}/{name} URLs for this "
+            "distribution resolve to."),
+        vocabulary=DistributionDefaultTraversalPolicy,
+        readonly=False, required=False))
+    redirect_default_traversal = exported(Bool(
+        title=_("Redirect the default traversal"),
+        description=_(
+            "If true, the default traversal is for migration and redirects "
+            "to a different canonical URL."),
+        readonly=False, required=False))
+
     def getArchiveIDList(archive=None):
         """Return a list of archive IDs suitable for sqlvalues() or quote().
 
diff --git a/lib/lp/registry/model/distribution.py b/lib/lp/registry/model/distribution.py
index b9baba7..ba9b432 100644
--- a/lib/lp/registry/model/distribution.py
+++ b/lib/lp/registry/model/distribution.py
@@ -88,6 +88,7 @@ from lp.code.interfaces.seriessourcepackagebranch import (
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
+    DistributionDefaultTraversalPolicy,
     SpecificationSharingPolicy,
     VCSType,
     )
@@ -262,6 +263,10 @@ class Distribution(SQLBase, BugTargetBase, MakesAnnouncements,
     redirect_release_uploads = BoolCol(notNull=True, default=False)
     development_series_alias = StringCol(notNull=False, default=None)
     vcs = EnumCol(enum=VCSType, notNull=False)
+    default_traversal_policy = EnumCol(
+        enum=DistributionDefaultTraversalPolicy, notNull=False,
+        default=DistributionDefaultTraversalPolicy.SERIES)
+    redirect_default_traversal = BoolCol(notNull=False, default=False)
 
     def __repr__(self):
         display_name = self.display_name.encode('ASCII', 'backslashreplace')
diff --git a/lib/lp/registry/tests/test_distribution.py b/lib/lp/registry/tests/test_distribution.py
index 9eb671d..a053738 100644
--- a/lib/lp/registry/tests/test_distribution.py
+++ b/lib/lp/registry/tests/test_distribution.py
@@ -31,6 +31,7 @@ from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
+    DistributionDefaultTraversalPolicy,
     EXCLUSIVE_TEAM_POLICY,
     INCLUSIVE_TEAM_POLICY,
     )
@@ -345,6 +346,31 @@ class TestDistribution(TestCaseWithFactory):
         self.assertEqual(1, result.count())
         self.assertEqual(first_project, result[0])
 
+    def test_default_traversal(self):
+        # By default, a distribution's default traversal refers to its
+        # series.
+        distro = self.factory.makeDistribution()
+        self.assertEqual(
+            DistributionDefaultTraversalPolicy.SERIES,
+            distro.default_traversal_policy)
+        self.assertFalse(distro.redirect_default_traversal)
+
+    def test_default_traversal_permissions(self):
+        # Only distribution owners can change the default traversal
+        # behaviour.
+        distro = self.factory.makeDistribution()
+        with person_logged_in(self.factory.makePerson()):
+            self.assertRaises(
+                Unauthorized, setattr, distro, 'default_traversal_policy',
+                DistributionDefaultTraversalPolicy.SERIES)
+            self.assertRaises(
+                Unauthorized, setattr, distro, 'redirect_default_traversal',
+                True)
+        with person_logged_in(distro.owner):
+            distro.default_traversal_policy = (
+                DistributionDefaultTraversalPolicy.SERIES)
+            distro.redirect_default_traversal = True
+
 
 class TestDistributionCurrentSourceReleases(
     CurrentSourceReleasesMixin, TestCase):