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

[Robert Collins]

Another (perhaps final!) tweak for u1: allow injecting things into the report from deep down the stack.

[Steve Kowalik (stevenk)]

review: Approve (code)

[James Henstridge (jamesh)]

Looks good.

review: Approve

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