launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/api-pillar-redirects-715992 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #715992 Pillar aliases redirect outside of webservice on webservice calls
  https://bugs.launchpad.net/bugs/715992

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/api-pillar-redirects-715992/+merge/49279

Summary
=======

Pillars can have aliases, which are linked back to their actual names/pages through use of canonical_url. This use of canonical_url, when dealing with webservice requests, accidentally redirects the request outside of the api layer. This branch deals with that by making sure the use of canonical_url is aware of the current request, so it doesn't have to guess at it.

Preimplementation
=================

Spoke with Leonard Richardson.

Followed up with Robert Collins on some confusing code in canonical_url.

Proposed Fix
============

Per Leonard Richardson, find the method that forgets the current request, and make sure request is passed in.

Implementation
==============

lib/canonical/launchpad/browser/launchpad.py
--------------------------------------------

The use of canonical_url to find the url of the pillar with the supplied alias now has the request passed into it.

lib/canonical/launchpad/webapp/publisher.py
-------------------------------------------

Comment changes on canonical_url to make it clearer what's going on with the request parameter.

lib/lp/registry/tests/test_product_webservice.py
------------------------------------------------

Created tests.

Tests
=====

bin/test -t test_product_webservice

Demo & QA
=========

Open http://qastaging.api.launchpad.net/ubuntufontbetatesting. It should redirect to http://qastaging.api.launchpad.net/ubuntu-font-family.

Lint
====

make lint output:


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/webapp/publisher.py
  lib/lp/registry/tests/test_product_webservice.py

-- 
https://code.launchpad.net/~jcsackett/launchpad/api-pillar-redirects-715992/+merge/49279
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/api-pillar-redirects-715992 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2011-02-02 15:43:31 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2011-02-10 19:56:21 +0000
@@ -668,7 +668,8 @@
             if pillar.name != name:
                 # This pillar was accessed through one of its aliases, so we
                 # must redirect to its canonical URL.
-                return self.redirectSubTree(canonical_url(pillar), status=301)
+                return self.redirectSubTree(
+                    canonical_url(pillar, self.request), status=301)
             return pillar
         return None
 

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-02-10 19:56:21 +0000
@@ -226,6 +226,7 @@
         return self._user
 
     _is_beta = None
+
     @property
     def isBetaUser(self):
         """Return True if the user is in the beta testers team."""
@@ -474,9 +475,9 @@
     the request.  If a request is not provided, but a web-request is in
     progress, the protocol, host and port are taken from the current request.
 
-    If there is no request available, the protocol, host and port are taken
-    from the root_url given in launchpad.conf.
-
+    :param request: The web request; if not provided, canonical_url attempts
+        to guess at the current request, using the protocol, host, and port
+        taken from the root_url given in launchpad.conf.
     :param path_only_if_possible: If the protocol and hostname can be omitted
         for the current request, return a url containing only the path.
     :param view_name: Provide the canonical url for the specified view,
@@ -550,8 +551,7 @@
     if ((path_only_if_possible and
          request is not None and
          root_url.startswith(request.getApplicationURL()))
-        or force_local_path
-        ):
+        or force_local_path):
         return unicode('/' + path)
     return unicode(root_url + path)
 
@@ -651,7 +651,6 @@
         return RedirectionView(target, self.request, status)
 
     # The next methods are for use by the Zope machinery.
-
     def publishTraverse(self, request, name):
         """Shim, to set objects in the launchbag when traversing them.
 

=== added file 'lib/lp/registry/tests/test_product_webservice.py'
--- lib/lp/registry/tests/test_product_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_product_webservice.py	2011-02-10 19:56:21 +0000
@@ -0,0 +1,28 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TestProductAlias(TestCaseWithFactory):
+    """Aliases should behave well with the webservice."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_alias_redirects_in_webservice(self):
+        # When a redirect occurs for a product, it should remain in the
+        # webservice.
+        product = self.factory.makeProduct(name='lemur')
+        removeSecurityProxy(product).setAliases(['monkey'])
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        response = webservice.get('/monkey')
+        self.assertEqual(
+            'http://api.launchpad.dev/beta/lemur',
+            response.getheader('location'))