launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/404-bug-api-generates-oops-701246 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #701246 in Launchpad itself: "missing bug dereference generates OOPS"
  https://bugs.launchpad.net/launchpad/+bug/701246

For more details, see:
https://code.launchpad.net/~deryck/launchpad/404-bug-api-generates-oops-701246/+merge/61407

This branch ensures that the webservice doesn't generate an OOPS when a bug is not found.  Actually, it ensures that we don't generate an OOPS when any web service resource is not found.  We report a 404, but not an OOPS.  Using launchpadlib, the user will get a KeyError.

There is a new test included to ensure that all top level web service objects have 404 status and no OOPS id.  This also fixes a bug in lib/canonical/launchpad/webapp/errorlog.py where the error handler was looking on the exception class rather than the instance for __lazr_webservice_error__ which was causing OOPS to still be written.

The main work is done with the monkeypatch to NotFound, so that any use of NotFound in launchpad will not generate an OOPS if we're in a webservice request.  This has no impact to NotFound outside the web service.
-- 
https://code.launchpad.net/~deryck/launchpad/404-bug-api-generates-oops-701246/+merge/61407
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/404-bug-api-generates-oops-701246 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2010-12-13 18:04:24 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-05-18 20:33:02 +0000
@@ -410,7 +410,7 @@
 
             if WebServiceLayer.providedBy(request):
                 webservice_error = getattr(
-                    info[0], '__lazr_webservice_error__', 500)
+                    info[1], '__lazr_webservice_error__', 500)
                 if webservice_error / 100 != 5:
                     request.oopsid = None
                     # Return so the OOPS is not generated.

=== modified file 'lib/canonical/launchpad/webapp/publisher.py'
--- lib/canonical/launchpad/webapp/publisher.py	2011-04-21 01:54:33 +0000
+++ lib/canonical/launchpad/webapp/publisher.py	2011-05-18 20:33:02 +0000
@@ -51,6 +51,8 @@
     )
 from zope.traversing.browser.interfaces import IAbsoluteURL
 
+from lazr.restful.declarations import error_status
+
 from canonical.launchpad.layers import (
     LaunchpadLayer,
     setFirstLayer,
@@ -75,6 +77,10 @@
 # HTTP Status code constants - define as appropriate.
 HTTP_MOVED_PERMANENTLY = 301
 
+# Monkeypatch NotFound to always avoid generating OOPS
+# from NotFound in web service calls.
+error_status(404)(NotFound)
+
 
 class DecoratorAdvisor:
     """Base class for a function decorator that adds class advice.

=== modified file 'lib/lp/app/browser/tests/test_webservice.py'
--- lib/lp/app/browser/tests/test_webservice.py	2011-02-17 03:59:47 +0000
+++ lib/lp/app/browser/tests/test_webservice.py	2011-05-18 20:33:02 +0000
@@ -1,7 +1,7 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Tests for lp.registry.browser.webservice."""
+"""Tests for webservice features across Launchpad."""
 
 __metaclass__ = type
 
@@ -10,6 +10,7 @@
 from lazr.restful.utils import get_current_web_service_request
 from zope.component import getMultiAdapter
 
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.browser.tales import format_link
 from lp.registry.interfaces.product import IProduct
@@ -46,3 +47,122 @@
             u'<p>\N{SNOWMAN} &lt;email address hidden&gt; '
             '<a href="/bugs/1">bug 1</a></p>',
             renderer(text))
+
+
+class BaseMissingObjectWebService:
+    """Base test of NotFound errors for top-level webservice objects."""
+
+    layer = DatabaseFunctionalLayer
+    object_type = None
+
+    def test_object_not_found(self):
+        """Missing top-level objects generate 404s but not OOPS."""
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        response = webservice.get('/%s/123456789' % self.object_type)
+        self.assertEqual(response.status, 404)
+        self.assertEqual(response.getheader('x-lazr-oopsid'), None)
+
+
+class TestMissingBranches(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice branches requests."""
+
+    object_type = 'branches'
+
+
+class TestMissingBugTrackers(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice bug_trackers requests."""
+
+    object_type = 'bug_trackers'
+
+
+class TestMissingBugs(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice bugs requests."""
+
+    object_type = 'bugs'
+
+
+class TestMissingBuilders(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice builders requests."""
+
+    object_type = 'builders'
+
+
+class TestMissingCountries(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice countries requests."""
+
+    object_type = 'countries'
+
+
+class TestMissingCves(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice cves requests."""
+
+    object_type = 'cves'
+
+
+class TestMissingDistributions(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice distributions requests."""
+
+    object_type = 'distributions'
+
+
+class TestMissingLanguages(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice launguages requests."""
+
+    object_type = 'languages'
+
+
+class TestMissingPackagesets(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice packagesets requests."""
+
+    object_type = 'packagesets'
+
+
+class TestMissingPeople(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice branches requests."""
+
+    object_type = 'people'
+
+
+class TestMissingProjectGroups(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice project_groups requests."""
+
+    object_type = 'project_groups'
+
+
+class TestMissingProjects(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice projects requests."""
+
+    object_type = 'projects'
+
+
+class TestMissingQuestions(BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice questions requests."""
+
+    object_type = 'questions'
+
+
+class TestMissingTemporaryBlobs(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice temporary_blobs requests."""
+
+    object_type = 'temporary_blobs'
+
+
+class TestMissingTranslationGroups(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice translation_groups requests."""
+
+    object_type = 'translation_groups'
+
+
+class TestMissingTranslationImportQueueEntries(
+    BaseMissingObjectWebService, TestCaseWithFactory):
+    """Test NotFound for webservice translation_import_queue_entries requests.
+    """
+
+    object_type = 'translation_import_queue_entries'

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2011-04-12 11:36:11 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2011-05-18 20:33:02 +0000
@@ -1195,14 +1195,16 @@
   Location: http://.../bugs/bugtrackers/bob
   ...
 
-Note the 301 response. We changed the name, so the API URL at which
+Note the 301 response above. We changed the name, so the API URL at which
 the bug tracker can be found has changed.
 
+Now notice that bug trackers (and bugs too) that are not found generate
+a 404 error, but do not generate an OOPS.
+
   >>> print webservice.get(bug_tracker['self_link'])
   HTTP/1.1 404 Not Found...
   Content-Length: ...
   ...
-  X-Lazr-Oopsid: OOPS-...
   <BLANKLINE>
   Object: <...BugTrackerSet object at ...>, name: u'mozilla.org'