launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04746
[Merge] lp:~lifeless/python-oops-wsgi/foru1 into lp:python-oops-wsgi
Robert Collins has proposed merging lp:~lifeless/python-oops-wsgi/foru1 into lp:python-oops-wsgi.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #829166 in python-oops-wsgi: "capture environ"
https://bugs.launchpad.net/python-oops-wsgi/+bug/829166
Bug #829171 in python-oops-wsgi: "support oopsing on observed status codes"
https://bugs.launchpad.net/python-oops-wsgi/+bug/829171
Bug #831748 in python-oops-wsgi: "Expose the OOPS ID when the WSGI app produces its own error page"
https://bugs.launchpad.net/python-oops-wsgi/+bug/831748
Bug #832477 in python-oops-wsgi: "Provide a simple way to capture oops info from deep in the request"
https://bugs.launchpad.net/python-oops-wsgi/+bug/832477
For more details, see:
https://code.launchpad.net/~lifeless/python-oops-wsgi/foru1/+merge/72649
Another (perhaps final!) tweak for u1: allow injecting things into the report from deep down the stack.
--
https://code.launchpad.net/~lifeless/python-oops-wsgi/foru1/+merge/72649
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-wsgi/foru1 into lp:python-oops-wsgi.
=== modified file 'oops_wsgi/__init__.py'
--- oops_wsgi/__init__.py 2011-08-24 00:56:00 +0000
+++ oops_wsgi/__init__.py 2011-08-24 03:48:22 +0000
@@ -76,6 +76,18 @@
to gather stats on the number of 404's occuring without doing log processing).
>>> app = oops_wsgi.make_app(app, config, oops_on_status=['404'])
+
+The oops middleware injects two variables into the WSGI environ to make it easy
+for cooperating code to report additional data.
+
+The `oops.report` variable is a dict which is copied into the report. See the
+`oops` package documentation for documentation on what should be present in an
+oops report. This requires the update_report hook to be installed (which
+`install_hooks` will do for you).
+
+The `oops.context` variable is a dict used for generating the report - keys and
+values added to that can be used in the `config.on_create` hooks to populate
+custom data without needing to resort to global variables.
"""
=== modified file 'oops_wsgi/hooks.py'
--- oops_wsgi/hooks.py 2011-08-24 01:21:57 +0000
+++ oops_wsgi/hooks.py 2011-08-24 03:48:22 +0000
@@ -20,6 +20,7 @@
'copy_environ',
'hide_cookie',
'install_hooks',
+ 'update_report',
]
_wsgi_standard_env_keys = set([
@@ -79,5 +80,9 @@
def install_hooks(config):
"""Install the default wsgi hooks into config."""
config.on_create.extend([copy_environ, hide_cookie])
-
-
+ config.on_create.insert(0, update_report)
+
+
+def update_report(report, context):
+ """Copy the oops.report contents from the wsgi environment to report."""
+ report.update(context.get('wsgi_environ', {}).get('oops.report', {}))
=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py 2011-08-24 01:56:49 +0000
+++ oops_wsgi/middleware.py 2011-08-24 03:48:22 +0000
@@ -81,7 +81,15 @@
* socket errors and GeneratorExit errors are passed through without
* being forward to the oops system.
"""
+ environ['oops.report'] = {}
+ environ['oops.context'] = {}
state = {}
+ def make_context(exc_info=None):
+ context = dict(url=construct_url(environ), wsgi_environ=environ)
+ context.update(environ.get('oops.context', {}))
+ if exc_info is not None:
+ context['exc_info'] = exc_info
+ return context
def oops_write(bytes):
write = state.get('write')
if write is None:
@@ -97,9 +105,7 @@
# 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),
- wsgi_environ=environ))
+ report = config.create(make_context(exc_info=exc_info))
ids = config.publish(report)
try:
if ids:
@@ -113,9 +119,7 @@
if oops_on_status:
for sniff_status in oops_on_status:
if status.startswith(sniff_status):
- report = config.create(
- dict(url=construct_url(environ),
- wsgi_environ=environ))
+ report = config.create(make_context())
report['HTTP_STATUS'] = status.split(' ')[0]
config.publish(report)
state['response'] = (status, headers)
@@ -134,9 +138,7 @@
raise
except Exception:
exc_info = sys.exc_info()
- report = config.create(
- dict(exc_info=exc_info, url=construct_url(environ),
- wsgi_environ=environ))
+ report = config.create(make_context(exc_info=exc_info))
ids = config.publish(report)
if not ids or 'write' in state:
# No OOPS generated, no oops publisher, or we have already
=== modified file 'oops_wsgi/tests/test_hooks.py'
--- oops_wsgi/tests/test_hooks.py 2011-08-24 01:43:38 +0000
+++ oops_wsgi/tests/test_hooks.py 2011-08-24 03:48:22 +0000
@@ -26,6 +26,7 @@
from oops_wsgi.hooks import (
copy_environ,
hide_cookie,
+ update_report,
)
@@ -40,6 +41,9 @@
config.on_create.index(copy_environ),
LessThan(config.on_create.index(hide_cookie)),
)
+ # update report wants to be at the start - its closer to a template
+ # than anything.
+ self.assertEqual(config.on_create[0], update_report)
class TestHooks(TestCase):
@@ -94,3 +98,16 @@
}.items())
expected_report = {'req_vars': expected_vars}
self.assertEqual(expected_report, report)
+
+ def test_update_report_no_wsgi_report(self):
+ report = {}
+ update_report(report, {})
+ update_report(report, {'wsgi_environ': {}})
+ self.assertEqual({}, report)
+
+ def test_update_report_copies_wsgi_report_variables(self):
+ report = {}
+ update_report(
+ report,
+ {'wsgi_environ': {'oops.report': {'foo':'bar'}}})
+ self.assertEqual({'foo': 'bar'}, report)
=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py 2011-08-24 01:56:49 +0000
+++ oops_wsgi/tests/test_middleware.py 2011-08-24 03:48:22 +0000
@@ -82,7 +82,7 @@
super(TestMakeApp, self).setUp()
self.calls = []
- def make_environ(self):
+ def make_outer_environ(self):
environ = {}
# Shove enough stuff in to let url reconstruction work:
environ['wsgi.url_scheme'] = 'http'
@@ -91,12 +91,18 @@
environ['QUERY_STRING'] = 'param=value'
return environ
+ def make_inner_environ(self, context={}):
+ environ = self.make_outer_environ()
+ environ['oops.report'] = {}
+ environ['oops.context'] = context
+ return environ
+
def wrap_and_run(self, inner, config, failing_write=False,
failing_start=False, failing_server_write=False, params=None):
if not params:
params = {}
app = make_app(inner, config, **params)
- environ = self.make_environ()
+ environ = self.make_outer_environ()
def start_response(status, headers, exc_info=None):
if exc_info:
# If an exception is raised after we have written (via the app
@@ -343,13 +349,18 @@
ValueError, ('boom, yo',))),
]))
- def test_error_in_app_context_includes_wsgi_environ(self):
- # When the app blows up we attach the environment.
+ def test_error_in_app_context_sets_oops_context(self):
+ # When the app blows up we attach the environment and the oops.context
+ # wsgi variable injects straight into the context.
def inner(environ, start_response):
+ environ['oops.context']['foo'] = 'bar'
raise ValueError('boom, yo')
config = self.config_for_oopsing(capture_create=True)
self.wrap_and_run(inner, config)
- self.assertEqual(self.make_environ(), self.calls[0]['wsgi_environ'])
+ self.assertEqual(
+ self.make_inner_environ({'foo':'bar'}),
+ self.calls[0]['wsgi_environ'])
+ self.assertEqual('bar', self.calls[0]['foo'])
def test_start_response_with_just_exc_info_generates_oops(self):
def inner(environ, start_response):
@@ -378,9 +389,12 @@
Equals(expected_start_response),
]))
- def test_start_response_exc_info_context_includes_wsgi_environ(self):
- # When the app handles its own error we still attach the environment.
+ def test_start_response_exc_info_includes_environ_and_context(self):
+ # When the app handles its own error we still attach the environment
+ # and the wsgi.context wsgi variable.
def inner(environ, start_response):
+ # Set a custom variable for the context
+ environ['oops.context']['foo'] = 'bar'
try:
raise ValueError('boom, yo')
except ValueError:
@@ -390,7 +404,10 @@
yield 'body'
config = self.config_for_oopsing(capture_create=True)
self.wrap_and_run(inner, config)
- self.assertEqual(self.make_environ(), self.calls[0]['wsgi_environ'])
+ self.assertEqual(
+ self.make_inner_environ({'foo':'bar'}),
+ self.calls[0]['wsgi_environ'])
+ self.assertEqual('bar', self.calls[0]['foo'])
def test_sniff_404_not_published_does_not_error(self):
def inner(environ, start_response):
@@ -420,11 +437,25 @@
Equals(expected_start_response),
]))
- def test_sniff_oops_context_includes_wsgi_environ(self):
+ def test_sniff_oops_context_includes_wsgi_environ_and_context(self):
def inner(environ, start_response):
+ environ['oops.context']['foo'] = 'bar'
start_response('404 MISSING', [])
yield 'pretty 404'
config = self.config_for_oopsing(capture_create=True)
self.wrap_and_run(
inner, config, params=dict(oops_on_status=['404']))
- self.assertEqual(self.make_environ(), self.calls[0]['wsgi_environ'])
+ self.assertEqual(
+ self.make_inner_environ({'foo':'bar'}),
+ self.calls[0]['wsgi_environ'])
+ self.assertEqual('bar', self.calls[0]['foo'])
+
+ def test_inner_environ_has_oops_report_and_oops_context_variables(self):
+ def inner(environ, start_response):
+ self.assertEqual({}, environ['oops.report'])
+ self.assertEqual({}, environ['oops.context'])
+ start_response('200 OK', [])
+ yield 'success'
+ config = self.config_for_oopsing()
+ body = self.wrap_and_run(inner, config)
+ self.assertEqual('success', body)
