Code review comment for lp:~lifeless/python-oops-wsgi/extraction

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 didn't start it, so I won't insist that you fix it. But take my review with a grain of salt since I can't really make sense of this in reasonable time.

@@ -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)

In this case though, please do fix the layout! So far the best solution I've found is to move the expected dict into a variable. Not great, but better than any of the alternatives I'm aware of.

Jeroen