Merge lp:~leonardr/lazr.restful/consistent-error-exposing into lp:lazr.restful

[Leonard Richardson]

Currently it's very difficult to get lazr.restful to associate a certain HTTP response code with a given exception. There are two techniques for associating a response code with an exception _class_, but it turns out these techniques actually do nothing whatsoever. To get this to work, you have to call expose() on an exception _instance_. And that only works when you the HTTP response code you want to use is 400.

This branch gets to the bottom of the problem and fixes it. Now you can use expose() to associate an exception instance with _any_ HTTP response code. (This fixes bug 631711.) You can use the @error_status decorator to set a response code for all instances of a given class. Every single exception will have its associated status code set, if it has an associated response code. Exceptions that correspond to 4xx response codes will be handled by lazr.restful; all other exceptions will be re-raised so they can be handled by the publisher (eg. retrying transient database errors or doing OOPS processing).

[Leonard Richardson (leonardr)]

Here's one change we might not like:

- >>> put_request.traverse(app)()
- Traceback (most recent call last):
- ...
- Unauthorized: (<Recipe object...>, 'dish', ...)
+ >>> print put_request.traverse(app)()
+ (<Recipe object...>, 'dish', ...)

The publisher puts the name of the exception class before the string. lazr.restful doesn't do this. This wasn't a problem before because lazr.restful only handled 400 errors. Now it's handling all 4xx errors.

Possible solutions:

1. lazr.restful re-raises all but 400 errors.
2. lazr.restful re-raises *all* errors, after setting the status (effectively abdicating any responsibility for the content of the response)
3. lazr.restful sticks the name of the exception class before the string.
4. Leave it as is. 401 means "Unauthorized", so there's no loss of information as far as the client is concerned.

[Leonard Richardson (leonardr)]

Oh, duh--the problem isn't the name of the exception class. The problem is that calling the resource used to raise an exception, and now it just returns the error string. I think this is fine.

[Tim Penhey (thumper)]

Looks good, and will help LP :-)

review: Approve

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-02-17 18:13:25 +0000
+++ src/lazr/restful/_resource.py	2011-03-02 19:52:53 +0000
@@ -84,8 +84,7 @@
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.lifecycle.snapshot import Snapshot
 from lazr.restful.interfaces import (
-    IClientError,
-    IClientErrorView,
+    IWebServiceErrorView,
     ICollectionField,
     ICollectionResource,
     IEntry,
@@ -917,18 +916,17 @@
                 self.request.response.setStatus(405)
                 self.request.response.setHeader("Allow", allow_string)
         except Exception, e:
-            # If the exception 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)
+            view = getMultiAdapter((e, self.request), name="index.html")
+            self.request.response.setStatus(view.status)
+            if view.status / 100 == 4:
+                # This exception corresponds to a client-side error
                 result = view()
-                self.request.response.setStatus(view.status)
             else:
-                raise
-
+                # This exception corresponds to a server-side error
+                # (or, less likely, some non-error condition). The
+                # publisher will know better than lazr.restful how to
+                # handle it.
+                raise e
         return result
 
 

=== modified file 'src/lazr/restful/configure.zcml'
--- src/lazr/restful/configure.zcml	2010-09-06 19:25:17 +0000
+++ src/lazr/restful/configure.zcml	2011-03-02 19:52:53 +0000
@@ -285,12 +285,13 @@
 
     <adapter factory="lazr.restful.render_field_to_html" />
 
-    <!-- System Errors -->
+    <!-- System Errors, and errors published to the web service using
+         annotations or the expose() method. -->
     <adapter
         for="zope.interface.common.interfaces.IException
              lazr.restful.interfaces.IWebServiceLayer"
         provides="zope.interface.Interface"
-        factory="lazr.restful.error.SystemErrorView"
+        factory="lazr.restful.error.WebServiceExceptionView"
         name="index.html"
         />
 
@@ -312,13 +313,4 @@
         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/declarations.py'
--- src/lazr/restful/declarations.py	2011-01-24 22:00:56 +0000
+++ src/lazr/restful/declarations.py	2011-03-02 19:52:53 +0000
@@ -42,7 +42,10 @@
 import sys
 
 from zope.component import getUtility, getGlobalSiteManager
-from zope.interface import classImplements
+from zope.interface import (
+    implements,
+    classImplements,
+    )
 from zope.interface.advice import addClassAdvisor
 from zope.interface.interface import fromFunction, InterfaceClass, TAGGED_DATA
 from zope.interface.interfaces import IInterface, IMethod
@@ -60,9 +63,17 @@
 from lazr.restful.fields import CollectionField, Reference
 from lazr.restful.interface import copy_field
 from lazr.restful.interfaces import (
-    ICollection, IEntry, IReference, IResourceDELETEOperation,
-    IResourceGETOperation, IResourcePOSTOperation, IWebServiceConfiguration,
-    IWebServiceVersion, LAZR_WEBSERVICE_NAME, LAZR_WEBSERVICE_NS)
+    ICollection,
+    IEntry,
+    IReference,
+    IResourceDELETEOperation,
+    IResourceGETOperation,
+    IResourcePOSTOperation,
+    IWebServiceConfiguration,
+    IWebServiceVersion,
+    LAZR_WEBSERVICE_NAME,
+    LAZR_WEBSERVICE_NS,
+    )
 from lazr.restful import (
     Collection, Entry, EntryAdapterUtility, ResourceOperation, ObjectLink)
 from lazr.restful.security import protect_schema
@@ -373,6 +384,7 @@
 
     f_locals[WEBSERVICE_ERROR] = int(status)
 
+
 def error_status(status):
     """Make a Python 2.6 class decorator for the given status.
 

=== modified file 'src/lazr/restful/docs/webservice.txt'
--- src/lazr/restful/docs/webservice.txt	2011-01-25 16:14:36 +0000
+++ src/lazr/restful/docs/webservice.txt	2011-03-02 19:52:53 +0000
@@ -1824,10 +1824,11 @@
     >>> body = simplejson.dumps(author)
     >>> put_request = create_web_service_request(
     ...     beard_url, body=body, environ=headers, method='PUT')
-    >>> put_request.traverse(app)()
-    Traceback (most recent call last):
-    ...
-    Unauthorized: (<Recipe object...>, 'dish', ...)
+    >>> print put_request.traverse(app)()
+    (<Recipe object...>, 'dish', ...)
+
+    >>> print put_request.response.getStatus()
+    401
 
 Stored file resources
 =====================

=== modified file 'src/lazr/restful/error.py'
--- src/lazr/restful/error.py	2011-01-31 22:34:54 +0000
+++ src/lazr/restful/error.py	2011-03-02 19:52:53 +0000
@@ -31,7 +31,9 @@
         def isSystemError():
             """Return a boolean indicating whether the error is a system error"""
 
-from lazr.restful.interfaces import IWebServiceConfiguration, IClientError
+from lazr.restful.interfaces import (
+    IWebServiceConfiguration,
+    )
 
 
 class WebServiceExceptionView:
@@ -48,7 +50,11 @@
         By default, use the __lazr_webservice_error__ attribute on
         the exception.
         """
-        return self.context.__lazr_webservice_error__
+        return getattr(self.context, '__lazr_webservice_error__', 500)
+
+    def isSystemError(self):
+        """See `ISystemErrorView`."""
+        return self.status / 100 == 5
 
     def __call__(self):
         """Generate the HTTP response describing the exception."""
@@ -76,19 +82,14 @@
         return ''.join(result)
 
 
+# lazr/restful/configure.zcml registers these classes as adapter
+# factories for common Zope exceptions.
 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."""
@@ -96,13 +97,6 @@
 
     status = 500
 
-    def isSystemError(self):
-        """See `ISystemErrorView`.
-
-        We want these logged in the SiteError log.
-        """
-        return True
-
 
 class NotFoundView(WebServiceExceptionView):
     """View for NotFound."""
@@ -115,6 +109,7 @@
 
     status = 401
 
+
 # This is currently only configured/tested in Launchpad.
 class RequestExpiredView(WebServiceExceptionView):
     """View for RequestExpired exception."""
@@ -122,18 +117,19 @@
     status = 503
 
 
-def expose(exception):
+def expose(exception, status=400):
     """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')
+        raise ValueError('This function only accepts exception instances. '
+                         'Use the @error_status decorator to publish an '
+                         'exception class as a web service error.')
+
+    exception.__lazr_webservice_error__ = status
+
     # 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	2011-02-17 12:48:46 +0000
+++ src/lazr/restful/interfaces/_rest.py	2011-03-02 19:52:53 +0000
@@ -22,8 +22,6 @@
 __all__ = [
     'IByteStorage',
     'IByteStorageResource',
-    'IClientError',
-    'IClientErrorView',
     'ICollection',
     'ICollectionResource',
     'IEntry',
@@ -52,6 +50,7 @@
     'LAZR_WEBSERVICE_NS',
     'IWebBrowserOriginatingRequest',
     'IWebServiceClientRequest',
+    'IWebServiceErrorView',
     'IWebServiceLayer',
     'IWebServiceVersion',
     ]
@@ -237,16 +236,7 @@
         """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):
+class IWebServiceErrorView(Interface):
     """The view that defines how errors are related to API clients."""
 
 

=== modified file 'src/lazr/restful/tests/test_error.py'
--- src/lazr/restful/tests/test_error.py	2010-11-04 18:21:58 +0000
+++ src/lazr/restful/tests/test_error.py	2011-03-02 19:52:53 +0000
@@ -12,9 +12,12 @@
 from zope.component import getMultiAdapter
 from zope.interface import Interface
 
+from lazr.restful.declarations import error_status
 from lazr.restful._resource import ReadWriteResource, UnknownEntryAdapter
 from lazr.restful.error import expose, ClientErrorView, SystemErrorView
-from lazr.restful.interfaces import IClientErrorView
+from lazr.restful.interfaces import (
+    IWebServiceErrorView,
+    )
 from lazr.restful.testing.webservice import FakeRequest
 
 
@@ -28,17 +31,55 @@
         return self.callable()
 
 
+class ExposeTestCase(unittest.TestCase):
+    # Test the behavior of the expose() method.
+
+    def test_exposed_does_not_work_on_class(self):
+        self.assertRaises(ValueError, expose, Exception)
+
+    def test_default_exposed_status_is_400(self):
+        exception = expose(ValueError())
+        self.assertEquals(exception.__lazr_webservice_error__, 400)
+
+    def test_expose_can_specify_status(self):
+        exception = expose(ValueError(), 409)
+        self.assertEquals(exception.__lazr_webservice_error__, 409)
+
+
 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):
+    def test_decorated_exception_class_becomes_error_view(self):
+        # An exposed exception, when raised, will set the response
+        # status code appropriately, and set the response text to the
+        # exception body.
+        def broken():
+            @error_status(404)
+            class MyException(Exception):
+                pass
+            raise MyException("something broke")
+
+        request = FakeRequest(version='trunk')
+        resource = TestResource(broken, request)
+        result = resource()
+        self.assertEquals(request.response.status, 404)
+        self.assert_(result.startswith('something broke'))
+
+    def test_decorated_exception_instance_becomes_error_view(self):
+        # An exposed exception, when raised, will set the response
+        # status code appropriately, and set the response text to the
+        # exception body.
+        def broken():
+            error = RuntimeError('something broke')
+            error = expose(error, 404)
+            raise error
+
+        request = FakeRequest(version='trunk')
+        resource = TestResource(broken, request)
+        result = resource()
+        self.assertEquals(request.response.status, 404)
+        self.assert_(result.startswith('something broke'))
+
+    def test_undecorated_exception_is_propagated(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).
@@ -50,21 +91,31 @@
         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 test_exception_decorated_as_system_error_is_propagated(self):
+        # If an exception is decorated with a response code that
+        # indicates a server-side problem, the exception percolates
+        # all the way out of the resource (to be handled by the
+        # publisher).
         def broken():
-            raise expose(RuntimeError('something broke'))
+            raise expose(RuntimeError('something broke'), 503)
 
         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'))
+        self.assertRaises(RuntimeError, resource)
+        self.assertEquals(request.response.getStatus(), 503)
+
+    def test_exception_decorated_as_nonerror_is_propagated(self):
+        # If an exception is decorated with a response code that
+        # indicates a non-error condition, the exception percolates
+        # all the way out of the resource (to be handled by the
+        # publisher).
+        def if_you_say_so():
+            raise expose(RuntimeError("everything's fine"), 200)
+
+        request = FakeRequest(version='trunk')
+        resource = TestResource(if_you_say_so, request)
+        self.assertRaises(RuntimeError, resource)
+        self.assertEquals(request.response.getStatus(), 200)
 
     def test_passing_bad_things_to_expose(self):
         # The expose function only accepts instances of exceptions.  It