launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Revert "Make series redirections compatible with object lookup"

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1888716 in Launchpad itself: "OOPS trying to download livefs build logs (buildd images)"
  https://bugs.launchpad.net/launchpad/+bug/1888716

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

This reverts commit 4176a750bdbf1656fee93ac7c18a69fa8e8eff8b.

This is insufficiently-understood, doesn't quite fix what it was originally intended to fix (non-canonical series URLs in webservice method parameters) and has caused at least one regression, so let's back it out for now and try again later.

I've left the lazr.restful upgrade in place; I think that part of it should be harmless enough.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:revert-object-lookup-redirection into launchpad:master.

diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index 8d566e0..41aa5cd 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -190,14 +190,12 @@ class DistributionNavigation(
     @stepthrough('+series')
     def traverse_series(self, name):
         series, _ = self._resolveSeries(name)
-        return self.redirectSubTree(
-            canonical_url(series, request=self.request), status=303)
+        return self.redirectSubTree(canonical_url(series), status=303)
 
     def traverse(self, name):
         series, is_alias = self._resolveSeries(name)
         if is_alias:
-            return self.redirectSubTree(
-                canonical_url(series, request=self.request), status=303)
+            return self.redirectSubTree(canonical_url(series), status=303)
         else:
             return series
 
diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index e7dbe7e..790ff91 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -287,8 +287,7 @@ class ProductNavigation(
     @stepthrough('+series')
     def traverse_series(self, name):
         series = self.context.getSeries(name)
-        return self.redirectSubTree(
-            canonical_url(series, request=self.request), status=303)
+        return self.redirectSubTree(canonical_url(series), 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 7e4c989..904462f 100644
--- a/lib/lp/registry/browser/tests/test_distribution.py
+++ b/lib/lp/registry/browser/tests/test_distribution.py
@@ -9,30 +9,23 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 from fixtures import FakeLogger
-from lazr.restful.fields import Reference
-from lazr.restful.interfaces import (
-    IFieldMarshaller,
-    IJSONRequestCache,
-    )
+from lazr.restful.interfaces import 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,
@@ -88,29 +81,6 @@ 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 714e84f..52544c0 100644
--- a/lib/lp/registry/browser/tests/test_product.py
+++ b/lib/lp/registry/browser/tests/test_product.py
@@ -9,11 +9,7 @@ __all__ = ['make_product_form']
 
 import re
 
-from lazr.restful.fields import Reference
-from lazr.restful.interfaces import (
-    IFieldMarshaller,
-    IJSONRequestCache,
-    )
+from lazr.restful.interfaces import IJSONRequestCache
 from six.moves.urllib.parse import (
     urlencode,
     urlsplit,
@@ -28,10 +24,7 @@ from testtools.matchers import (
     Not,
     )
 import transaction
-from zope.component import (
-    getMultiAdapter,
-    getUtility,
-    )
+from zope.component import getUtility
 from zope.schema.vocabulary import SimpleVocabulary
 from zope.security.proxy import removeSecurityProxy
 
@@ -56,7 +49,6 @@ 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
@@ -64,7 +56,6 @@ 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,
@@ -117,32 +108,6 @@ 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 f01466d..a67601e 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 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,7 +36,6 @@ 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
@@ -1082,7 +1081,7 @@ class Navigation:
 
 
 @implementer(IBrowserPublisher)
-class RedirectionView(URLDereferencingMixin):
+class RedirectionView:
 
     def __init__(self, target, request, status=None, cache_view=None):
         self.target = target
@@ -1107,34 +1106,6 @@ class RedirectionView(URLDereferencingMixin):
     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.")
-        if urlparse(self.target).query:
-            raise AttributeError(
-                "RedirectionView.context is not supported for URLs with "
-                "query strings.")
-        return self.dereference_url_as_object(self.target)
-
 
 @implementer(IBrowserPublisher)
 class RenamedView: