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

[Benji York]

Bug 854695 describes several OOPS scenarios caused by lazr.restful
requiring an __traceback__ attribute on exceptions when
LaunchpadTimeoutError doesn't have one.

The fix was to extract the traceback and other bits using sys.exc_info()
instead (as well as adding a test that demonstrated the problem).

[Graham Binns (gmb)]

Hi Benji,

Nice branch, one really minor nitpick:

97 + self.fail('The resource should not have generated an '
98 + 'AttributeError. This is probably because something was '
99 + 'expecting the exception to have a __traceback__ attribute.')

should be formatted as:

            self.fail(
                'The resource should not have generated an '
                'AttributeError. This is probably because something was '
                'expecting the exception to have a __traceback__ attribute.')

(It also has a double-space in there before the "This" that it doesn't need).

review: Approve (code)

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

197. By Benji York on 2011-09-20

change formatting as requested in review

=== modified file 'src/lazr/restful/_resource.py'
--- src/lazr/restful/_resource.py	2011-09-08 20:18:30 +0000
+++ src/lazr/restful/_resource.py	2011-09-20 15:44:24 +0000
@@ -35,6 +35,7 @@
 import os
 import simplejson
 import simplejson.encoder
+import sys
 import time
 
 # Import SHA in a way compatible with both Python 2.4 and Python 2.6.
@@ -928,6 +929,7 @@
                 self.request.response.setStatus(405)
                 self.request.response.setHeader("Allow", allow_string)
         except Exception, e:
+            exception_info = sys.exc_info()
             view = getMultiAdapter((e, self.request), name="index.html")
             try:
                 ws_view = IWebServiceExceptionView(view)
@@ -938,7 +940,7 @@
                 # re-raise the exception, let the publisher look up
                 # the view, and hopefully handle it better.  Note the careful
                 # reraising that ensures the original trackabck is preserved.
-                raise e, None, e.__traceback__
+                raise exception_info[0], exception_info[1], exception_info[2]
 
             self.request.response.setStatus(ws_view.status)
             if ws_view.status / 100 == 4:

=== modified file 'src/lazr/restful/tests/test_error.py'
--- src/lazr/restful/tests/test_error.py	2011-09-07 20:46:50 +0000
+++ src/lazr/restful/tests/test_error.py	2011-09-20 15:44:24 +0000
@@ -53,13 +53,8 @@
 class ErrorsTestCase(unittest.TestCase):
 
     def setUp(self):
-        try:
-            raise RuntimeError('my traceback')
-        except:
-            fake_traceback = sys.exc_info()[2]
-
         class MyException(Exception):
-            __traceback__ = fake_traceback
+            pass
 
         self.exception_class = MyException
         super(ErrorsTestCase, self).setUp()
@@ -133,10 +128,13 @@
         self.assertRaises(RuntimeError, resource)
         self.assertEquals(request.response.getStatus(), 200)
 
-    def _setup_non_web_service_exception(self):
+    def _setup_non_web_service_exception(self, exception_class=None):
+        if exception_class is None:
+            exception_class = self.exception_class
+
         # Define a method that raises the exception.
         def has_a_view():
-            raise self.exception_class()
+            raise exception_class()
 
         # Register a view for the exception that does not subclass
         # WebServiceExceptionView and provides no information about
@@ -145,7 +143,7 @@
             def __init__(*args):
                 pass
         getGlobalSiteManager().registerAdapter(
-            ViewHasNoStatus, (self.exception_class, IWebServiceLayer),
+            ViewHasNoStatus, (exception_class, IWebServiceLayer),
             Interface, name="index.html")
 
         # The exception is re-raised, even though a view is registered
@@ -175,7 +173,7 @@
             resource()
         except:
             pass
-        self.assertTrue('my traceback' in traceback.format_exc())
+        self.assertTrue('raise exception_class()' in traceback.format_exc())
 
     def test_passing_bad_things_to_expose(self):
         # The expose function only accepts instances of exceptions.  It
@@ -230,6 +228,28 @@
         self.assertTrue(
             "raise RuntimeError('something broke')" in traceback.format_exc())
 
+    def test_reporting_original_exception_with_no_trackeback(self):
+        # Sometimes an exception won't have an __traceback__ attribute.  The
+        # re-raising should still work (bug 854695).
+        class TracebacklessException(Exception):
+            pass
+
+        resource = self._setup_non_web_service_exception(
+            exception_class=TracebacklessException)
+        try:
+            resource()
+        except AttributeError:
+            self.fail(
+                'The resource should not have generated an AttributeError. '
+                'This is probably because something was expecting the '
+                'exception to have a __traceback__ attribute.')
+        except (MemoryError, KeyboardInterrupt, SystemExit):
+            raise
+        except:
+            pass
+
+        self.assertTrue("TracebacklessException" in traceback.format_exc())
+
 
 class FunctionalLayer:
     allow_teardown = False