launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/recursive-branch-resolution-failure into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #645544 Recursive failure when going to +branch/nonexistantproject
  https://bugs.launchpad.net/bugs/645544


Bug 645544 describes a recursive failure when navigating to a url like +/branch/foo. This is a bug resulting from work done on lp:~wallyworld/lanchpad/tales-linkify-broken-links for bug 404131.

The problem is that when a url is entered directly, the http referer is None and the redirection logic tries to send the browser to an invalid url.
 
Proposed fix

Simple fix in the error handling logic to simply raise an exception if there is no referer in the http request header. This is how the system used to work before the fix for bug 404131 was done.

Implementation Details

The only code changes were to:
    - lib/canonical/launchpad/browser/launchpad.py
    - lib/canonical/launchpad/browser/tests/test_launchpad.py

Tests

    bin/test -vvm canonical.launchpad.browser.tests.test_launchpad TestBranchTraversal

    A new test was written to test for the case where the referring url is None: 
    - test_nonexistent_product_without_referer

The TestBranchTraversal class (and friends) needed to be changed to pass a default referring url to the business logic being tested since previously the referring url was always None. The test cases never made the distinction between navigating from a valid url or hacking the url directly.

lint

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/browser/tests/test_launchpad.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/recursive-branch-resolution-failure/+merge/36403
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/recursive-branch-resolution-failure into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2010-09-15 06:40:16 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2010-09-23 01:51:09 +0000
@@ -542,6 +542,11 @@
 
         except (CannotHaveLinkedBranch, InvalidNamespace,
                 InvalidProductName, NotFoundError) as e:
+            # If are aren't arriving at this invalid branch URL from another
+            # page then we just raise an exception, otherwise we end up in a
+            # bad recursion loop.
+            if url is None:
+                raise e
             error_msg = str(e)
             if error_msg == '':
                 error_msg = "Invalid branch lp:%s." % path

=== modified file 'lib/canonical/launchpad/browser/tests/test_launchpad.py'
--- lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-09-16 06:02:36 +0000
+++ lib/canonical/launchpad/browser/tests/test_launchpad.py	2010-09-23 01:51:09 +0000
@@ -33,6 +33,11 @@
 from lp.testing.views import create_view
 
 
+# We set the request header HTTP_REFERER  when we want to simulate navigation
+# from a valid page. This is used in the assertDisplaysNotification check.
+DEFAULT_REFERER = 'http://launchpad.dev'
+
+
 class TraversalMixin:
 
     def _validateNotificationContext(
@@ -73,28 +78,35 @@
         """
 
         redirection = self.traverse(path)
-        self.assertIs(redirection.target, None)
+        self.assertIs(redirection.target, DEFAULT_REFERER)
         self._validateNotificationContext(
             redirection.request, notification, level)
 
-    def assertNotFound(self, path):
-        self.assertRaises(NotFound, self.traverse, path)
+    def assertNotFound(self, path, use_default_referer=True):
+        self.assertRaises(
+            NotFound, self.traverse, path,
+            use_default_referer=use_default_referer)
 
     def assertRedirects(self, segments, url):
         redirection = self.traverse(segments)
         self.assertEqual(url, redirection.target)
 
-    def traverse(self, path, first_segment):
+    def traverse(self, path, first_segment, use_default_referer=True):
         """Traverse to 'segments' using a 'LaunchpadRootNavigation' object.
 
         Using the Zope traversal machinery, traverse to the path given by
         'segments', starting at a `LaunchpadRootNavigation` object.
 
         :param segments: A list of path segments.
+        :param use_default_referer: If True, set the referer attribute in the
+            request header to DEFAULT_REFERER = "http://launchpad.dev";
+            (otherwise it remains as None)
         :return: The object found.
         """
-        request = LaunchpadTestRequest(
-            PATH_INFO=urlappend('/%s' % first_segment, path))
+        extra = {'PATH_INFO': urlappend('/%s' % first_segment, path)}
+        if use_default_referer:
+            extra['HTTP_REFERER'] = DEFAULT_REFERER
+        request = LaunchpadTestRequest(**extra)
         segments = reversed(path.split('/'))
         request.setTraversalStack(segments)
         traverser = LaunchpadRootNavigation(None, request=request)
@@ -110,8 +122,9 @@
 
     layer = DatabaseFunctionalLayer
 
-    def traverse(self, path):
-        return super(TestBranchTraversal, self).traverse(path, '+branch')
+    def traverse(self, path, **kwargs):
+        return super(TestBranchTraversal, self).traverse(
+            path, '+branch', **kwargs)
 
     def test_unique_name_traversal(self):
         # Traversing to /+branch/<unique_name> redirects to the page for that
@@ -178,6 +191,12 @@
             non_existent, requiredMessage,
             BrowserNotificationLevel.ERROR)
 
+    def test_nonexistent_product_without_referer(self):
+        # Traversing to /+branch/<no-such-product> without a referer results
+        # in a 404 error. This happens if the user hacks the URL rather than
+        # navigating via a link
+        self.assertNotFound('non-existent', use_default_referer=False)
+
     def test_product_without_dev_focus(self):
         # Traversing to a product without a development focus displays a
         # user message on the same page.