launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:object-lookup-redirection into launchpad:master.

Commit message:
Make series redirections compatible with object lookup

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/387051

lazr.restful.marshallers.ObjectLookupFieldMarshaller expects dereferencing the URL to result in a resource with a "context" attribute; without this, passing new-style series URLs (e.g. /ubuntu/+series/focal) as arguments to webservice methods results in 500 errors.  To support this, add a "context" property to RedirectionView that recursively traverses the URL to find the appropriate context object.

Dependencies MP: https://code.launchpad.net/~cjwatson/lp-source-dependencies/+git/lp-source-dependencies/+merge/387050
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:object-lookup-redirection into launchpad:master.

diff --git a/constraints.txt b/constraints.txt
index c282f40..dd23a2a 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -229,7 +229,7 @@ lazr.delegates==2.0.4
 lazr.enum==1.2
 lazr.jobrunner==0.16
 lazr.lifecycle==1.2
-lazr.restful==0.22.0
+lazr.restful==0.22.1
 lazr.restfulclient==0.14.3
 lazr.sshserver==0.1.10
 lazr.uri==1.0.5
diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index 41aa5cd..8d566e0 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -190,12 +190,14 @@ class DistributionNavigation(
     @stepthrough('+series')
     def traverse_series(self, name):
         series, _ = self._resolveSeries(name)
-        return self.redirectSubTree(canonical_url(series), status=303)
+        return self.redirectSubTree(
+            canonical_url(series, request=self.request), status=303)
 
     def traverse(self, name):
         series, is_alias = self._resolveSeries(name)
         if is_alias:
-            return self.redirectSubTree(canonical_url(series), status=303)
+            return self.redirectSubTree(
+                canonical_url(series, request=self.request), status=303)
         else:
             return series
 
diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index 790ff91..e7dbe7e 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -287,7 +287,8 @@ class ProductNavigation(
     @stepthrough('+series')
     def traverse_series(self, name):
         series = self.context.getSeries(name)
-        return self.redirectSubTree(canonical_url(series), status=303)
+        return self.redirectSubTree(
+            canonical_url(series, request=self.request), status=303)
 
     def traverse(self, name):
         return self.context.getSeries(name)
diff --git a/lib/lp/registry/browser/tests/test_distribution.py b/lib/lp/registry/browser/tests/test_distribution.py
index 904462f..7e4c989 100644
--- a/lib/lp/registry/browser/tests/test_distribution.py
+++ b/lib/lp/registry/browser/tests/test_distribution.py
@@ -9,23 +9,30 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 from fixtures import FakeLogger
-from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.fields import Reference
+from lazr.restful.interfaces import (
+    IFieldMarshaller,
+    IJSONRequestCache,
+    )
 import soupmatchers
 from testtools.matchers import (
     MatchesAll,
     MatchesAny,
     Not,
     )
+from zope.component import getMultiAdapter
 from zope.schema.vocabulary import SimpleVocabulary
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
 from lp.registry.enums import EXCLUSIVE_TEAM_POLICY
+from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.ociproject import OCI_PROJECT_ALLOW_CREATE
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp import canonical_url
 from lp.services.webapp.publisher import RedirectionView
+from lp.services.webapp.servers import WebServiceTestRequest
 from lp.testing import (
     admin_logged_in,
     login_celebrity,
@@ -81,6 +88,29 @@ class TestDistributionNavigation(TestCaseWithFactory):
             "http://launchpad.test/%s/%s"; % (
                 distroseries.distribution.name, distroseries.name))
 
+    def test_new_series_url_supports_object_lookup(self):
+        # New-style +series URLs are compatible with webservice object
+        # lookup.
+        field = Reference(schema=IDistroSeries)
+        request = WebServiceTestRequest()
+        request.setVirtualHostRoot(names=["devel"])
+        marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+        distroseries = self.factory.makeDistroSeries()
+        distroseries_url = "/%s/+series/%s" % (
+            distroseries.distribution.name, distroseries.name)
+        self.assertEqual(
+            distroseries, marshaller.marshall_from_json_data(distroseries_url))
+        # Objects subordinate to the redirected series work too.
+        distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=distroseries)
+        distroarchseries_url = "/%s/+series/%s/%s" % (
+            distroarchseries.distroseries.distribution.name,
+            distroarchseries.distroseries.name,
+            distroarchseries.architecturetag)
+        self.assertEqual(
+            distroarchseries,
+            marshaller.marshall_from_json_data(distroarchseries_url))
+
 
 class TestDistributionPage(TestCaseWithFactory):
     """A TestCase for the distribution index page."""
diff --git a/lib/lp/registry/browser/tests/test_product.py b/lib/lp/registry/browser/tests/test_product.py
index 52544c0..714e84f 100644
--- a/lib/lp/registry/browser/tests/test_product.py
+++ b/lib/lp/registry/browser/tests/test_product.py
@@ -9,7 +9,11 @@ __all__ = ['make_product_form']
 
 import re
 
-from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.fields import Reference
+from lazr.restful.interfaces import (
+    IFieldMarshaller,
+    IJSONRequestCache,
+    )
 from six.moves.urllib.parse import (
     urlencode,
     urlsplit,
@@ -24,7 +28,10 @@ from testtools.matchers import (
     Not,
     )
 import transaction
-from zope.component import getUtility
+from zope.component import (
+    getMultiAdapter,
+    getUtility,
+    )
 from zope.schema.vocabulary import SimpleVocabulary
 from zope.security.proxy import removeSecurityProxy
 
@@ -49,6 +56,7 @@ from lp.registry.interfaces.product import (
     IProductSet,
     License,
     )
+from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.model.product import Product
 from lp.services.config import config
 from lp.services.database.interfaces import IStore
@@ -56,6 +64,7 @@ from lp.services.webapp.publisher import (
     canonical_url,
     RedirectionView,
     )
+from lp.services.webapp.servers import WebServiceTestRequest
 from lp.services.webapp.vhosts import allvhosts
 from lp.testing import (
     BrowserTestCase,
@@ -108,6 +117,32 @@ class TestProductNavigation(TestCaseWithFactory):
             "http://launchpad.test/%s/%s"; % (
                 productseries.product.name, productseries.name))
 
+    def test_new_series_url_supports_object_lookup(self):
+        # New-style +series URLs are compatible with webservice object
+        # lookup.
+        field = Reference(schema=IProductSeries)
+        request = WebServiceTestRequest()
+        request.setVirtualHostRoot(names=["devel"])
+        marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+        productseries = self.factory.makeProductSeries()
+        productseries_url = "/%s/+series/%s" % (
+            productseries.product.name, productseries.name)
+        resource = marshaller.dereference_url(productseries_url)
+        self.assertIsInstance(resource, RedirectionView)
+        self.assertEqual(
+            productseries,
+            marshaller.marshall_from_json_data(productseries_url))
+
+        # Objects subordinate to the redirected series work too.
+        productrelease = self.factory.makeProductRelease(
+            productseries=productseries)
+        productrelease_url = "/%s/+series/%s/%s" % (
+            productrelease.product.name, productrelease.productseries.name,
+            productrelease.version)
+        self.assertEqual(
+            productrelease,
+            marshaller.marshall_from_json_data(productrelease_url))
+
 
 class TestProductConfiguration(BrowserTestCase):
     """Tests the configuration links and helpers."""
diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index a3b8e70..7a49017 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.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).
 
 """Publisher of objects as web pages.
@@ -36,6 +36,7 @@ from lazr.restful import (
     )
 from lazr.restful.declarations import error_status
 from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.marshallers import URLDereferencingMixin
 from lazr.restful.tales import WebLayerAPI
 from lazr.restful.utils import get_current_browser_request
 import simplejson
@@ -1078,7 +1079,7 @@ class Navigation:
 
 
 @implementer(IBrowserPublisher)
-class RedirectionView:
+class RedirectionView(URLDereferencingMixin):
 
     def __init__(self, target, request, status=None, cache_view=None):
         self.target = target
@@ -1103,6 +1104,30 @@ class RedirectionView:
     def browserDefault(self, request):
         return self, ()
 
+    @property
+    def context(self):
+        """Find the context object corresponding to the redirection target.
+
+        Passing objects as arguments to webservice methods is done by URL,
+        and ObjectLookupFieldMarshaller constructs an internal request to
+        traverse this and find the appropriate object.  If the URL in
+        question results in a redirection, then we must recursively traverse
+        the target URL until we find something that the webservice
+        understands.
+        """
+        # Circular import.
+        from lp.services.webapp.servers import WebServiceClientRequest
+
+        if not isinstance(self.request, WebServiceClientRequest):
+            # Raise AttributeError here (as if the "context" attribute
+            # didn't exist) so that e.g.
+            # lp.testing.publication.test_traverse knows to fall back to
+            # other approaches.
+            raise AttributeError(
+                "RedirectionView.context is only supported for webservice "
+                "requests.")
+        return self.dereference_url_as_object(self.target)
+
 
 @implementer(IBrowserPublisher)
 class RenamedView: