Merge lp:~benji/lazr.restful/bug-413174 into lp:lazr.restful

[Benji York]

When raising exceptions it is sometimes useful to allow web service API
users to see the error details. OOPS-1318S626 and OOPS-1694EB1486 are
examples of this kind of situation.

Therefore this branch adds a method (lazr.restful.error.expose) that
lets you signal to lazr.restful that the message associated with the
error should be relayed to the web service API user.

The approach was discussed with Gary Poster and Leonard Richardson.

The expose function decorates the exception instance with a marker
interface which has an exception view (ClientErrorView) associated with
it.

The test for this feature can be run with the command

    bin/test -t TestLaunchpadlibAPI

[Gary Poster (gary)]

Very nice catch with the check for isinstance of BaseException.

I question the view pattern for the registration and lookup--using "index.html" as the distinguishing marker. I'd prefer a custom interface myself. If, in retrospect, you agree, great. If not, I can see where you are coming from: leave as is.

We need a linter. Either ``from types import ClassType`` in src/lazr/restful/error.py shouldn't be there now, or it should have been caught earlier. Which is it?

In error.py, you say "# Bug 11512 describes the possibility of controlling the HTTP status of...." I think you mean bug 631711.

review: Approve

[Benji York (benji)]

On Mon, Sep 6, 2010 at 1:48 PM, Gary Poster <email address hidden> wrote:
> Review: Approve
> Very nice catch with the check for isinstance of BaseException.
>
> I question the view pattern for the registration and lookup--using
> "index.html" as the distinguishing marker.  I'd prefer a custom interface
> myself.  If, in retrospect, you agree, great.  If not, I can see where you
> are coming from: leave as is.

I was following the pattern used for the other error views. I don't feel
strongly about it one way or the other so I'll change it.

> We need a linter.

Yep. I'll take a quick look to see if pyflakes or similar is easily
usable with our lazr buildouts.

> Either ``from types import ClassType`` in
> src/lazr/restful/error.py shouldn't be there now, or it should have been
> caught earlier.  Which is it?

That's left over from a previous revision of the code. Removed.

> In error.py, you say "# Bug 11512 describes the possibility of controlling
> the HTTP status of...."  I think you mean bug 631711.

Indeed. Fixed.
--
Benji York

[lp:~benji/lazr.restful/bug-413174]

147. By Benji York on 2010-09-06

fix bug in bug number

148. By Benji York on 2010-09-06

integrate review suggestion to use a custom interface for the client
error view

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2010-08-24 22:12:38 +0000
+++ src/lazr/restful/_resource.py	2010-09-06 19:28:41 +0000
@@ -77,6 +77,7 @@
 from lazr.enum import BaseItem
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
+from lazr.restful.interfaces import IClientError, IClientErrorView
 
 from lazr.restful.interfaces import (
     ICollection, ICollectionField, ICollectionResource, IEntry, IEntryField,
@@ -894,31 +895,45 @@
         """Handle a GET, PUT, or PATCH request."""
         result = ""
         method = self.getRequestMethod()
-        if method is None:
-            result = self.HTTP_METHOD_OVERRIDE_ERROR
-        elif method == "GET":
-            result = self.do_GET()
-        elif method in ["PUT", "PATCH"]:
-            media_type = self.handleConditionalWrite()
-            if media_type is not None:
-                stream = self.request.bodyStream
-                representation = stream.getCacheStream().read()
-                if method == "PUT":
-                    result = self.do_PUT(media_type, representation)
-                else:
-                    result = self.do_PATCH(media_type, representation)
-        elif method == "POST" and self.implementsPOST():
-            result = self.do_POST()
-        elif method == "DELETE" and self.implementsDELETE():
-            result = self.do_DELETE()
-        else:
-            allow_string = "GET PUT PATCH"
-            if self.implementsPOST():
-                allow_string += " POST"
-            if self.implementsDELETE():
-                allow_string += " DELETE"
-            self.request.response.setStatus(405)
-            self.request.response.setHeader("Allow", allow_string)
+        try:
+            if method is None:
+                result = self.HTTP_METHOD_OVERRIDE_ERROR
+            elif method == "GET":
+                result = self.do_GET()
+            elif method in ["PUT", "PATCH"]:
+                media_type = self.handleConditionalWrite()
+                if media_type is not None:
+                    stream = self.request.bodyStream
+                    representation = stream.getCacheStream().read()
+                    if method == "PUT":
+                        result = self.do_PUT(media_type, representation)
+                    else:
+                        result = self.do_PATCH(media_type, representation)
+            elif method == "POST" and self.implementsPOST():
+                result = self.do_POST()
+            elif method == "DELETE" and self.implementsDELETE():
+                result = self.do_DELETE()
+            else:
+                allow_string = "GET PUT PATCH"
+                if self.implementsPOST():
+                    allow_string += " POST"
+                if self.implementsDELETE():
+                    allow_string += " DELETE"
+                self.request.response.setStatus(405)
+                self.request.response.setHeader("Allow", allow_string)
+        except Exception as e:
+            # If the execption was decorated in such a way to indicate that
+            # the error is the client's fault (i.e., not a bug in the web
+            # service), then the error should be communicated to the client
+            # without the publisher framework treating the response as an
+            # error.
+            if IClientError.providedBy(e):
+                view = getMultiAdapter((e, self.request), IClientErrorView)
+                result = view()
+                self.request.response.setStatus(view.status)
+            else:
+                raise
+
         return result
 
 

=== modified file 'src/lazr/restful/configure.zcml'
--- src/lazr/restful/configure.zcml	2009-05-28 16:51:15 +0000
+++ src/lazr/restful/configure.zcml	2010-09-06 19:28:41 +0000
@@ -312,4 +312,13 @@
         name="index.html"
         />
 
+    <!-- Exceptions that are explicitly marked to be exposed via the web
+         service. -->
+    <adapter
+        for="lazr.restful.interfaces.IClientError
+             lazr.restful.interfaces.IWebServiceLayer"
+        provides="lazr.restful.interfaces.IClientErrorView"
+        factory="lazr.restful.error.ClientErrorView"
+        />
+
 </configure>

=== modified file 'src/lazr/restful/directives/__init__.py'
--- src/lazr/restful/directives/__init__.py	2010-02-11 17:57:16 +0000
+++ src/lazr/restful/directives/__init__.py	2010-09-06 19:28:41 +0000
@@ -18,7 +18,7 @@
 from lazr.restful.interfaces import (
     IServiceRootResource, IWebServiceClientRequest, IWebServiceConfiguration,
     IWebServiceLayer, IWebServiceVersion)
-from lazr.restful import (
+from lazr.restful._resource import (
     register_versioned_request_utility, ServiceRootResource)
 from lazr.restful.simple import (
     BaseWebServiceConfiguration, Publication, Request,

=== modified file 'src/lazr/restful/error.py'
--- src/lazr/restful/error.py	2009-04-16 17:29:43 +0000
+++ src/lazr/restful/error.py	2010-09-06 19:28:41 +0000
@@ -4,6 +4,8 @@
 
 __metaclass__ = type
 __all__ = [
+    'ClientErrorView',
+    'expose',
     'NotFoundView',
     'RequestExpiredView',
     'SystemErrorView',
@@ -14,7 +16,8 @@
 import traceback
 
 from zope.component import getUtility
-from zope.interface import implements
+from zope.interface import implements, directlyProvides
+
 try:
     # This import brings some big ugly dependencies, and is not strictly
     # necessary.
@@ -24,11 +27,12 @@
     class ISystemErrorView(Interface):
         """Error views that can classify their contexts as system errors
         """
-    
+
         def isSystemError():
             """Return a boolean indicating whether the error is a system error"""
 
-from lazr.restful.interfaces import IWebServiceConfiguration
+from lazr.restful.interfaces import IWebServiceConfiguration, IClientError
+
 
 class WebServiceExceptionView:
     """Generic view handling exceptions on the web service."""
@@ -71,6 +75,20 @@
         return ''.join(result)
 
 
+class ClientErrorView(WebServiceExceptionView):
+    """Client-induced error."""
+    implements(ISystemErrorView)
+
+    status = 400
+
+    def isSystemError(self):
+        """See `ISystemErrorView`.
+
+        We *do not* want these logged in the SiteError log.
+        """
+        return False
+
+
 class SystemErrorView(WebServiceExceptionView):
     """Server error."""
     implements(ISystemErrorView)
@@ -101,3 +119,20 @@
     """View for RequestExpired exception."""
 
     status = 503
+
+
+def expose(exception):
+    """Tell lazr.restful to deliver details about the exception to the client.
+    """
+    # Bug 631711 describes the possibility of controlling the HTTP status of
+    # the response (so it can be something other than 400).
+
+    # Since Python lets us raise exception types without instantiating them
+    # (like "raise RuntimeError" instead of "raise RuntimeError()", we want to
+    # make sure the caller doesn't get confused and try that with us.
+    if not isinstance(exception, BaseException):
+        raise ValueError('this function only accepts exception instanaces')
+    # Mark the exception to indicate that its details should be sent to the
+    # web service client.
+    directlyProvides(exception, IClientError)
+    return exception

=== modified file 'src/lazr/restful/interfaces/_rest.py'
--- src/lazr/restful/interfaces/_rest.py	2010-08-03 15:52:46 +0000
+++ src/lazr/restful/interfaces/_rest.py	2010-09-06 19:28:41 +0000
@@ -22,6 +22,8 @@
 __all__ = [
     'IByteStorage',
     'IByteStorageResource',
+    'IClientError',
+    'IClientErrorView',
     'ICollection',
     'ICollectionResource',
     'IEntry',
@@ -230,6 +232,19 @@
         """Render the given string as HTML."""
 
 
+class IClientError(Interface):
+    """This denotes that an exception should be relayed to the API client.
+
+    This is a marker interface that is "slammed" onto exceptions by users of
+    lazr.restful by calling lazr.restful.error.expose on an exception before
+    they raise it.
+    """
+
+
+class IClientErrorView(Interface):
+    """The view that defines how errors are related to API clients."""
+
+
 class IEntryField(Interface):
     """An individual field of an entry."""
 

=== modified file 'src/lazr/restful/testing/webservice.py'
--- src/lazr/restful/testing/webservice.py	2010-08-05 14:08:20 +0000
+++ src/lazr/restful/testing/webservice.py	2010-09-06 19:28:41 +0000
@@ -98,8 +98,7 @@
 class FakeResponse:
     """Simple response wrapper object."""
     def __init__(self):
-        self.status = 599
-        self.headers = {}
+        self.reset()
 
     def setStatus(self, new_status):
         self.status = new_status
@@ -115,6 +114,21 @@
         """Return the response status code."""
         return self.status
 
+    def reset(self):
+        """Set response values back to their initial state."""
+        self.status = 599
+        self.headers = {}
+        self.result = ''
+
+    def setResult(self, result):
+        if result is None:
+            result = ''
+
+        if not isinstance(result, basestring):
+            raise ValueError('only strings and None results are handled')
+
+        self.result = result
+
 
 class FakeRequest:
     """Simple request object for testing purpose."""
@@ -136,6 +150,7 @@
         self.query_string_params = {}
         self.method = 'GET'
         self.URL = 'http://api.example.org/'
+        self.headers = {}
 
 
     def getTraversalStack(self):

=== added file 'src/lazr/restful/tests/test_error.py'
--- src/lazr/restful/tests/test_error.py	1970-01-01 00:00:00 +0000
+++ src/lazr/restful/tests/test_error.py	2010-09-06 19:28:41 +0000
@@ -0,0 +1,88 @@
+# Copyright 2008 Canonical Ltd.  All rights reserved.
+
+"""Tests of lazr.restful navigation."""
+
+__metaclass__ = type
+
+from pkg_resources import resource_filename
+import os
+import unittest
+
+from van.testing.layer import zcml_layer
+from zope.component import getMultiAdapter
+
+from lazr.restful._resource import ReadWriteResource
+from lazr.restful.error import expose, ClientErrorView, SystemErrorView
+from lazr.restful.interfaces import IClientErrorView
+from lazr.restful.testing.webservice import FakeRequest
+
+
+class TestResource(ReadWriteResource):
+
+    def __init__(self, callable, request):
+        self.callable = callable
+        super(TestResource, self).__init__(context=None, request=request)
+
+    def do_GET(self):
+        return self.callable()
+
+
+class ErrorsTestCase(unittest.TestCase):
+
+    def test_client_error_view_lookup(self):
+        # An error decorated with lazr.restful.error.exposed will be adaptable
+        # to IClientErrorView.
+        request = FakeRequest(version='trunk')
+        exception_view = getMultiAdapter(
+            (expose(ValueError()), request), IClientErrorView)
+        self.assertEquals(exception_view.__class__, ClientErrorView)
+
+    def test_undecorated_exception(self):
+        # If an undecorated exception (a regular Python exception) is
+        # generated during a request the exception percolates all the way out
+        # of the resource (to be handled by the publisher).
+
+        def broken():
+            raise RuntimeError('something broke')
+
+        request = FakeRequest(version='trunk')
+        resource = TestResource(broken, request)
+        self.assertRaises(RuntimeError, resource)
+
+    def test_decorated_exception(self):
+        # If an undecorated exception is generated during a request the
+        # SystemErrorView is used to generate the response.
+
+        def broken():
+            raise expose(RuntimeError('something broke'))
+
+        request = FakeRequest(version='trunk')
+        resource = TestResource(broken, request)
+        result = resource()
+        # The client error view sets the status code to 400 (client error).
+        self.assertEqual(request.response.getStatus(), 400)
+        # The message used to construct the exception is included in the body
+        # of the response.
+        self.assert_(result.startswith('something broke'))
+
+    def test_passing_bad_things_to_expose(self):
+        # The expose function only accepts instances of exceptions.  It
+        # generates errors otherwise.
+        self.assertRaises(ValueError, expose, 1)
+        self.assertRaises(ValueError, expose, 'x')
+        self.assertRaises(ValueError, expose, RuntimeError)
+
+
+class FunctionalLayer:
+    allow_teardown = False
+    zcml = os.path.abspath(resource_filename('lazr.restful', 'ftesting.zcml'))
+
+
+zcml_layer(FunctionalLayer)
+
+
+def additional_tests():
+    """See `zope.testing.testrunner`."""
+    suite = unittest.TestLoader().loadTestsFromName(__name__)
+    suite.layer = FunctionalLayer
+    return suite