Merge lp:~lifeless/python-oops-wsgi/extraction into lp:python-oops-wsgi

[Robert Collins]

I fail at implementing wsgi right. This fixes exc_info handling AFAICT.

[Jeroen T. Vermeulen (jtv)]

Generally good, but a few notes:

=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py 2011-08-18 00:45:58 +0000
+++ oops_wsgi/middleware.py 2011-08-22 07:04:24 +0000
@@ -85,8 +85,23 @@
                 state['write'] = start_response(status, headers)
                 write = state['write']
             write(bytes)
- def oops_start_response(status, headers):
- state['response'] = (status, headers)
+ def oops_start_response(status, headers, exc_info=None):
+ if exc_info is not None:
+ # The app is explicitly signalling an error (rather than
+ # returning a page describing the error). Capture that and then
+ # forward to the containing element verbatim. In future we might
+ # choose to add the OOPS id to the headers (but we can't fiddle
+ # the body as it is being generated by the contained app.
+ report = config.create(
+ dict(exc_info=exc_info, url=construct_url(environ)))

Careful! You seem to be indenting by 8 instead of 4.

+ ids = config.publish(report)
+ try:
+ state['write'] = start_response(status, headers, exc_info)
+ return state['write']
+ finally:
+ del exc_info

Out of interest, whence the del?

=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py 2011-08-18 22:44:58 +0000
+++ oops_wsgi/tests/test_middleware.py 2011-08-22 07:04:24 +0000

@@ -52,7 +53,21 @@
         environ['HTTP_HOST'] = 'example.com'
         environ['PATH_INFO'] = '/demo'
         environ['QUERY_STRING'] = 'param=value'
- def start_response(status, headers):
+ def start_response(status, headers, exc_info=None):
+ if exc_info:
+ # If an exception is raised after we have written (via the app
+ # calling our oops_write) or after we have yielded data, we
+ # must supply exc_info in the call to start_response, per the
+ # wsgi spec. The easiest way to do that is to always supply
+ # exc_info when reporting an exception - that will DTRT if
+ # start_response upstream had already written data. Thus
+ # our test start_response which lives above the oops middleware
+ # captures the fact exc_info was called and all our tests that
+ # expect oopses raised expect the exc_info to be present.
+ self.calls.append(
+ ('start error', status, headers, exc_info[0],
+ exc_info[1].args))

Here too, indentation goes too far. You're not getting tabs, are you?

@@ -191,7 +206,8 @@
              'url': 'http://example.com/demo?param=value',
              'value': u'boom, yo'},
             # Then the middleware responses
- ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+ ('start error', '500 Internal Server Error',
+ [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),

Horrid layout. Your branch did...

review: Approve

[Robert Collins (lifeless)]

All more or less actioned ;)

[Robert Collins (lifeless)]

(mainly more, that was meant to be funny not flippant :()

=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py	2011-08-18 00:45:58 +0000
+++ oops_wsgi/middleware.py	2011-08-22 07:04:24 +0000
@@ -85,8 +85,23 @@
                 state['write'] = start_response(status, headers)
                 write = state['write']
             write(bytes)
-        def oops_start_response(status, headers):
-            state['response'] = (status, headers)
+        def oops_start_response(status, headers, exc_info=None):
+            if exc_info is not None:
+                # The app is explicitly signalling an error (rather than
+                # returning a page describing the error). Capture that and then
+                # forward to the containing element verbatim. In future we might
+                # choose to add the OOPS id to the headers (but we can't fiddle
+                # the body as it is being generated by the contained app.
+                report = config.create(
+                        dict(exc_info=exc_info, url=construct_url(environ)))
+                ids = config.publish(report)
+                try:
+                    state['write'] = start_response(status, headers, exc_info)
+                    return state['write']
+                finally:
+                    del exc_info
+            else:
+                state['response'] = (status, headers)
             return oops_write
         try:
             iterator = iter(app(environ, oops_start_response))
@@ -112,7 +127,8 @@
                 # server figure it out.
                 raise
             start_response('500 Internal Server Error',
-                [('Content-Type', content_type)])
+                [('Content-Type', content_type)], exc_info=exc_info)
+            del exc_info
             if error_render is not None:
                 yield error_render(report)
             else:

=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py	2011-08-18 22:44:58 +0000
+++ oops_wsgi/tests/test_middleware.py	2011-08-22 07:04:24 +0000
@@ -20,6 +20,7 @@
 from doctest import ELLIPSIS
 import gc
 import socket
+import sys
 from textwrap import dedent
 
 from oops import (
@@ -52,7 +53,21 @@
         environ['HTTP_HOST'] = 'example.com'
         environ['PATH_INFO'] = '/demo'
         environ['QUERY_STRING'] = 'param=value'
-        def start_response(status, headers):
+        def start_response(status, headers, exc_info=None):
+            if exc_info:
+                # If an exception is raised after we have written (via the app
+                # calling our oops_write) or after we have yielded data, we
+                # must supply exc_info in the call to start_response, per the
+                # wsgi spec. The easiest way to do that is to always supply
+                # exc_info when reporting an exception - that will DTRT if
+                # start_response upstream had already written data. Thus
+                # our test start_response which lives above the oops middleware
+                # captures the fact exc_info was called and all our tests that
+                # expect oopses raised expect the exc_info to be present.
+                self.calls.append(
+                        ('start error', status, headers, exc_info[0],
+                        exc_info[1].args))
+                return self.calls.append
             if failing_start:
                 raise socket.error(errno.EPIPE, 'Connection closed')
             self.calls.append((status, headers))
@@ -191,7 +206,8 @@
              'url': 'http://example.com/demo?param=value',
              'value': u'boom, yo'},
             # Then the middleware responses
-            ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+            ('start error', '500 Internal Server Error',
+             [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),
             ], self.calls)
         self.assertThat(iterated, DocTestMatches(dedent("""\
             <html>
@@ -219,7 +235,8 @@
              'url': 'http://example.com/demo?param=value',
              'value': u'boom, yo'},
             # Then the middleware responses
-            ('500 Internal Server Error', [('Content-Type', 'text/json')]),
+            ('start error', '500 Internal Server Error',
+             [('Content-Type', 'text/json')], ValueError, ('boom, yo',)),
             ], self.calls)
         self.assertThat(iterated, DocTestMatches(dedent("""\
             {"oopsid" : "..."}"""), ELLIPSIS))
@@ -239,7 +256,8 @@
              'url': 'http://example.com/demo?param=value',
              'value': u'boom, yo'},
             # Then the middleware responses
-            ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+            ('start error', '500 Internal Server Error',
+             [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),
             ], self.calls)
         self.assertEqual(iterated, 'woo')
 
@@ -250,3 +268,20 @@
         self.wrap_and_run(inner, config)
         self.assertEqual('http://example.com/demo?param=value',
                 self.calls[0]['url'])
+
+    def test_start_response_with_just_exc_info_generates_oops(self):
+        def inner(environ, start_response):
+            try:
+                raise ValueError('boom, yo')
+            except ValueError:
+                exc_info = sys.exc_info()
+            start_response('500 FAIL', [], exc_info)
+            yield 'body'
+        config = self.config_for_oopsing()
+        self.wrap_and_run(inner, config)
+        self.assertEqual([{'id': 1,
+            'type': 'ValueError',
+            'url': 'http://example.com/demo?param=value',
+            'value': u'boom, yo'},
+            ('start error', '500 FAIL', [], ValueError, ('boom, yo',))],
+            self.calls)