launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-719637 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-719637/+merge/63159

Bug 719637 was mostly fixed along with bug 730393.  However, a new incarnation of the bug was introduced at that time that caused NotFound (and similar errors) to OOPS if no referer is set.  This happens in production (e.g., OOPS-1970CF506).  This branch fixes this.

The operative change is the "if" statement added to lib/canonical/launchpad/webapp/errorlog.py.

The new tests exercise the different ways the if can be traversed.

The new tests can be run with

    bin/test -c -m canonical.launchpad.webapp.tests -t Test404Oops

The make lint report is clean.
-- 
https://code.launchpad.net/~benji/launchpad/bug-719637/+merge/63159
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-719637 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-05-20 04:48:41 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-06-01 19:29:25 +0000
@@ -377,7 +377,12 @@
             return True
         if strtype in self._ignored_exceptions_for_offsite_referer:
             if request is not None:
-                referer = request.get('HTTP_REFERER', '')
+                referer = request.get('HTTP_REFERER')
+                # If there is no referrer then we can't tell if this exception
+                # should be ignored or not, so we'll be conservative and
+                # ignore it.
+                if referer is None:
+                    return True
                 referer_parts = urlparse.urlparse(referer)
                 root_parts = urlparse.urlparse(
                     allvhosts.configs['mainsite'].rooturl)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-05-19 21:01:54 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-06-01 19:29:25 +0000
@@ -967,5 +967,50 @@
         self.assertIs(None, self.error_utility.getLastOopsReport())
 
 
+class Test404Oops(testtools.TestCase):
+
+    def setUp(self):
+        super(Test404Oops, self).setUp()
+        # ErrorReportingUtility reads the global config to get the
+        # current error directory.
+        test_data = dedent("""
+            [vhost.mainsite]
+            hostname: launchpad.net
+
+            [error_reports]
+            copy_to_zlog: true
+            error_dir: %s
+            """ % tempfile.mkdtemp())
+        config.push('test_data', test_data)
+        shutil.rmtree(config.error_reports.error_dir, ignore_errors=True)
+
+    def tearDown(self):
+        shutil.rmtree(config.error_reports.error_dir, ignore_errors=True)
+        config.pop('test_data')
+        reset_logging()
+        super(Test404Oops, self).tearDown()
+
+    def test_offsite_404_ignored(self):
+        # A request originating from another site that generates a NotFound
+        # (404) is ignored (i.e., no OOPS is logged).
+        utility = ErrorReportingUtility()
+        request = dict(HTTP_REFERER='example.com')
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+    def test_onsite_404_not_ignored(self):
+        # A request originating from a local site that generates a NotFound
+        # (404) produces an OOPS.
+        utility = ErrorReportingUtility()
+        request = dict(HTTP_REFERER='canonical.com')
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+    def test_404_without_referer_is_ignored(self):
+        # If a 404 is generated and there is no HTTP referer, we don't produce
+        # an OOPS.
+        utility = ErrorReportingUtility()
+        request = dict()
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)