launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04745
[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
For more details, see:
https://code.launchpad.net/~lifeless/python-oops-wsgi/foru1/+merge/72640
This fixes limitations in python-oops-wsgi preventing Ubuntu one from using it.
--
https://code.launchpad.net/~lifeless/python-oops-wsgi/foru1/+merge/72640
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 'README'
--- README 2011-08-17 09:43:07 +0000
+++ README 2011-08-24 01:25:17 +0000
@@ -53,6 +53,13 @@
Note that you will probably want at least one publisher, or your reports will
be discarded.
+* Add in wsgi specific hooks to the config::
+
+ >>> oops_wsgi.install_hooks(config)
+
+This is a convenience function - you are welcome to pick and choose the creation
+or filter hooks you want from oops_wsgi.hooks.
+
* Create your wsgi app as normal, and then wrap it::
>>> app = oops_wsgi.make_app(app, config)
@@ -83,7 +90,7 @@
>>> json_template='{"oopsid" : "%(id)s"}'
>>> app = oops_wsgi.make_app(app, config, error_template=json_template)
-More coming soon.
+For more information see pydoc oops_wsgi.
Installation
============
=== modified file 'oops_wsgi/__init__.py'
--- oops_wsgi/__init__.py 2011-08-22 07:07:16 +0000
+++ oops_wsgi/__init__.py 2011-08-24 01:25:17 +0000
@@ -30,6 +30,13 @@
Note that you will probably want at least one publisher, or your reports will
be discarded.
+* Add in wsgi specific hooks to the config::
+
+ >>> oops_wsgi.install_hooks(config)
+
+This is a convenience function - you are welcome to pick and choose the creation
+or filter hooks you want from oops_wsgi.hooks.
+
* Create your wsgi app as normal, and then wrap it::
>>> app = oops_wsgi.make_app(app, config)
@@ -59,6 +66,16 @@
>>> json_template='{"oopsid" : "%(id)s"}'
>>> app = oops_wsgi.make_app(app, config, error_template=json_template)
+
+If the wrapped app errors by sending exc_info to start_response, that will be
+used to create an OOPS report, and the id added to the headers under the
+X-Oops-Id header. This is also present when an OOPS is triggered by catching an
+exception in the wrapped app (as long as the body hasn't started).
+
+You can request that reports be created when a given status code is used (e.g.
+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'])
"""
@@ -73,10 +90,12 @@
# established at this point, and setup.py will use a version of next-$(revno).
# If the releaselevel is 'final', then the tarball will be major.minor.micro.
# Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 2, 'beta', 0)
+__version__ = (0, 0, 3, 'beta', 0)
__all__ = [
+ 'install_hooks',
'make_app'
]
from oops_wsgi.middleware import make_app
+from oops_wsgi.hooks import install_hooks
=== added file 'oops_wsgi/hooks.py'
--- oops_wsgi/hooks.py 1970-01-01 00:00:00 +0000
+++ oops_wsgi/hooks.py 2011-08-24 01:25:17 +0000
@@ -0,0 +1,83 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""oops creation and filtering hooks for working with WSGI."""
+
+__all__ = [
+ 'copy_environ',
+ 'hide_cookie',
+ 'install_hooks',
+ ]
+
+_wsgi_standard_env_keys = set([
+ 'REQUEST_METHOD',
+ 'SCRIPT_NAME',
+ 'PATH_INFO',
+ 'QUERY_STRING',
+ 'CONTENT_TYPE',
+ 'CONTENT_LENGTH',
+ 'SERVER_NAME',
+ 'SERVER_PORT',
+ 'SERVER_PROTOCOL',
+ 'wsgi.version',
+ 'wsgi.url_scheme',
+ ])
+
+
+def copy_environ(report, context):
+ """Copy useful variables from the wsgi environment if it is present.
+
+ This should be in the context as 'wsgi_environ'.
+
+ e.g.
+ report = config.create(context=dict(wsgi_environ=environ))
+ """
+ environ = context.get('wsgi_environ', {})
+ if 'req_vars' not in report:
+ report['req_vars'] = []
+ req_vars = report['req_vars']
+ for key, value in sorted(environ.items()):
+ if (key in _wsgi_standard_env_keys or
+ key.startswith('HTTP_')):
+ req_vars.append((key, value))
+
+
+def hide_cookie(report, context):
+ """If there is an HTTP_COOKIE entry in the report, hide its value.
+
+ The entry is looked for either as a top level key or in the req_vars list.
+
+ The COOKIE header is often used to carry session tokens and thus permits
+ folk analyzing crash reports to log in as an arbitrary user (e.g. your
+ sysadmin users).
+ """
+ if 'HTTP_COOKIE' in report:
+ report['HTTP_COOKIE'] = '<hidden>'
+ if 'req_vars' not in report:
+ return
+ new_vars = []
+ for key, value in report['req_vars']:
+ if key == 'HTTP_COOKIE':
+ value = '<hidden>'
+ new_vars.append((key, value))
+ report['req_vars'][:] = new_vars
+
+
+def install_hooks(config):
+ """Install the default wsgi hooks into config."""
+ config.on_create.extend([copy_environ, hide_cookie])
+
+
=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py 2011-08-22 06:58:07 +0000
+++ oops_wsgi/middleware.py 2011-08-24 01:25:17 +0000
@@ -40,7 +40,7 @@
def make_app(app, config, template=default_error_template,
- content_type='text/html', error_render=None):
+ content_type='text/html', error_render=None, oops_on_status=None):
"""Construct a middleware around app that will forward errors via config.
Any errors encountered by the app will be forwarded to config and an error
@@ -64,6 +64,11 @@
:param error_render: Optional custom renderer for presenting error reports
to clients. Should be a callable taking the report as its only
parameter.
+ :param oops_on_status: Optional list of HTTP status codes that should
+ generate OOPSes. OOPSes triggered by sniffing these codes will not
+ interfere with the response being sent. For instance, if you do
+ not expect any 404's from your application, you might set
+ oops_on_status=['404'].
:return: A WSGI app.
"""
def oops_middleware(environ, start_response):
@@ -96,11 +101,21 @@
dict(exc_info=exc_info, url=construct_url(environ)))
ids = config.publish(report)
try:
+ if ids:
+ headers = list(headers)
+ headers.append(('X-Oops-Id', str(report['id'])))
state['write'] = start_response(status, headers, exc_info)
return state['write']
finally:
del exc_info
else:
+ if oops_on_status:
+ for sniff_status in oops_on_status:
+ if status.startswith(sniff_status):
+ report = config.create(
+ dict(url=construct_url(environ)))
+ report['HTTP_STATUS'] = status.split(' ')[0]
+ config.publish(report)
state['response'] = (status, headers)
return oops_write
try:
@@ -126,8 +141,10 @@
# replace the content with a clean error, so let the wsgi
# server figure it out.
raise
- start_response('500 Internal Server Error',
- [('Content-Type', content_type)], exc_info=exc_info)
+ headers = [('Content-Type', content_type)]
+ headers.append(('X-Oops-Id', str(report['id'])))
+ start_response(
+ '500 Internal Server Error', headers, exc_info=exc_info)
del exc_info
if error_render is not None:
yield error_render(report)
=== modified file 'oops_wsgi/tests/__init__.py'
--- oops_wsgi/tests/__init__.py 2011-08-17 07:27:56 +0000
+++ oops_wsgi/tests/__init__.py 2011-08-24 01:25:17 +0000
@@ -21,6 +21,7 @@
def test_suite():
test_mod_names = [
+ 'hooks',
'middleware',
]
return TestLoader().loadTestsFromNames(
=== added file 'oops_wsgi/tests/test_hooks.py'
--- oops_wsgi/tests/test_hooks.py 1970-01-01 00:00:00 +0000
+++ oops_wsgi/tests/test_hooks.py 2011-08-24 01:25:17 +0000
@@ -0,0 +1,96 @@
+# Copyright (c) 2010, 2011, Canonical Ltd
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the various hooks included in oops-wsgi."""
+
+from StringIO import StringIO
+
+from oops import Config
+from testtools import TestCase
+from testtools.matchers import LessThan
+
+from oops_wsgi import install_hooks
+from oops_wsgi.hooks import (
+ copy_environ,
+ hide_cookie,
+ )
+
+
+class TestInstallHooks(TestCase):
+
+ def test_install_hooks_installs_defaults(self):
+ config = Config()
+ install_hooks(config)
+ self.assertIn(hide_cookie, config.on_create)
+ self.assertIn(copy_environ, config.on_create)
+ self.assertThat(
+ config.on_create.index(copy_environ),
+ LessThan(config.on_create.index(hide_cookie)),
+ )
+
+\
+class TestHooks(TestCase):
+
+ def test_hide_cookie_no_cookie(self):
+ report = {}
+ hide_cookie(report, {})
+ self.assertEqual({}, report)
+
+ def test_hide_cookie_cookie_present_top_level(self):
+ report = {'HTTP_COOKIE': 'foo'}
+ hide_cookie(report, {})
+ self.assertEqual({'HTTP_COOKIE': '<hidden>'}, report)
+
+ def test_hide_cookie_cookie_present_req_vars(self):
+ report = {'req_vars': [('HTTP_COOKIE', 'foo')]}
+ hide_cookie(report, {})
+ self.assertEqual({'req_vars': [('HTTP_COOKIE', '<hidden>')]}, report)
+
+ def test_copy_environ_copied_variables(self):
+ environ = {
+ 'REQUEST_METHOD': 'GET',
+ 'SCRIPT_NAME': '',
+ 'PATH_INFO': '/foo',
+ 'QUERY_STRING': 'bar=quux',
+ 'CONTENT_TYPE': 'multipart/x-form-encoding',
+ 'CONTENT_LENGTH': '12',
+ 'SERVER_NAME': 'example.com',
+ 'SERVER_PORT': '80',
+ 'SERVER_PROTOCOL': 'HTTP/1.0',
+ 'HTTP_COOKIE': 'zaphod',
+ 'wsgi.version': (1, 0),
+ 'wsgi.url_scheme': 'https',
+ 'wsgi.input': StringIO(),
+ }
+ context = dict(wsgi_environ=environ)
+ report = {}
+ copy_environ(report, context)
+ expected_vars = sorted({
+ 'REQUEST_METHOD': 'GET',
+ 'SCRIPT_NAME': '',
+ 'PATH_INFO': '/foo',
+ 'QUERY_STRING': 'bar=quux',
+ 'CONTENT_TYPE': 'multipart/x-form-encoding',
+ 'CONTENT_LENGTH': '12',
+ 'SERVER_NAME': 'example.com',
+ 'SERVER_PORT': '80',
+ 'SERVER_PROTOCOL': 'HTTP/1.0',
+ 'HTTP_COOKIE': 'zaphod',
+ 'wsgi.version': (1, 0),
+ 'wsgi.url_scheme': 'https',
+ }.items())
+ expected_report = {'req_vars': expected_vars}
+ self.assertEqual(expected_report, report)
=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py 2011-08-22 07:40:30 +0000
+++ oops_wsgi/tests/test_middleware.py 2011-08-24 01:25:17 +0000
@@ -30,12 +30,52 @@
from testtools import TestCase
from testtools.matchers import (
DocTestMatches,
+ Equals,
+ MatchesListwise,
+ Mismatch,
+ MismatchesAll,
raises,
)
from oops_wsgi import make_app
+class MatchesOOPS:
+ """Matches an OOPS checking some keys and ignoring the rest."""
+
+ def __init__(self, checkkeys):
+ """Create a MatchesOOPS.
+
+ :param checkkeys: A dict describing the keys to check. For each key in
+ the dict the report must either have a matching key with the same value
+ or a matching key whose value is matched by the matcher given.
+ e.g. MatchesOOPS(
+ dict(id=2, req_vars=MatchesSetwise(Equals(("foo", "bar")))))
+ will check the id is 2, that req_vars is equivalent to [("foo", "bar")]
+ and will ignore all other keys in the report.
+ """
+ self.checkkeys = checkkeys
+
+ def match(self, report):
+ sentinel = object()
+ mismatches = []
+ for key, value in self.checkkeys.items():
+ if key not in report:
+ mismatches.append(Mismatch("Report has no key '%s'" % key))
+ if getattr(value, 'match', sentinel) is sentinel:
+ matcher = Equals(value)
+ else:
+ matcher = value
+ mismatch = matcher.match(report[key])
+ if mismatch is not None:
+ mismatches.append(mismatch)
+ if mismatches:
+ return MismatchesAll(mismatches)
+
+ def __str__(self):
+ return "MatchesOOPS(%s)" % self.checkkeys
+
+
class TestMakeApp(TestCase):
def setUp(self):
@@ -179,15 +219,32 @@
def config_for_oopsing(self):
config = Config()
- # datestamps are hell on tests
- config.on_create.remove(createhooks.attach_date)
- # so are file paths
- def remove_tb_text(report, context):
- report.pop('tb_text')
- config.on_create.append(remove_tb_text)
config.publishers.append(self.publish_to_calls)
return config
+ def test_oops_start_reponse_adds_x_oops_id_header(self):
+ # When the oops middleware generates the error page itself, it includes
+ # an x-oops-id header (vs adding one when we decorate a start_response
+ # including exc_info from the wrapped app.
+ def inner(environ, start_response):
+ raise ValueError('booyah')
+ config = self.config_for_oopsing()
+ self.wrap_and_run(inner, config)
+ headers = [
+ ('Content-Type', 'text/html'),
+ ('X-Oops-Id', '1')
+ ]
+ expected_start_response = \
+ ('start error', '500 Internal Server Error', headers, ValueError,
+ ('booyah',))
+ self.assertThat(self.calls, MatchesListwise([
+ # The oops is logged:
+ MatchesOOPS({'value': 'booyah'}),
+ # And the containing start_response was called with our custom
+ # headers.
+ Equals(expected_start_response),
+ ]))
+
def test_start_response_500_if_fails_after_start_before_body(self):
# oops middle ware needs to buffer the start_response call in case the
# wrapped app blows up before it starts streaming data - otherwise the
@@ -199,16 +256,16 @@
yield ''
config = self.config_for_oopsing()
iterated = self.wrap_and_run(inner, config)
- self.assertEqual([
+ self.assertThat(self.calls, MatchesListwise([
# First the oops is generated
- {'id': 1,
- 'type': 'ValueError',
- 'url': 'http://example.com/demo?param=value',
- 'value': u'boom, yo'},
+ MatchesOOPS({'value': 'boom, yo'}),
# Then the middleware responses
- ('start error', '500 Internal Server Error',
- [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),
- ], self.calls)
+ Equals((
+ 'start error',
+ '500 Internal Server Error',
+ [('Content-Type', 'text/html'), ('X-Oops-Id', '1')],
+ ValueError, ('boom, yo',))),
+ ]))
self.assertThat(iterated, DocTestMatches(dedent("""\
<html>
<head><title>Oops! - ...</title></head>
@@ -228,16 +285,16 @@
iterated = self.wrap_and_run(inner, config,
params=dict(content_type='text/json',
template='{"oopsid" : "%(id)s"}'))
- self.assertEqual([
+ self.assertThat(self.calls, MatchesListwise([
# First the oops is generated
- {'id': 1,
- 'type': 'ValueError',
- 'url': 'http://example.com/demo?param=value',
- 'value': u'boom, yo'},
+ MatchesOOPS({'value': 'boom, yo'}),
# Then the middleware responses
- ('start error', '500 Internal Server Error',
- [('Content-Type', 'text/json')], ValueError, ('boom, yo',)),
- ], self.calls)
+ Equals((
+ 'start error',
+ '500 Internal Server Error',
+ [('Content-Type', 'text/json'), ('X-Oops-Id', '1')],
+ ValueError, ('boom, yo',))),
+ ]))
self.assertThat(iterated, DocTestMatches(dedent("""\
{"oopsid" : "..."}"""), ELLIPSIS))
@@ -249,16 +306,16 @@
return "woo"
iterated = self.wrap_and_run(inner, config,
params=dict(error_render=error_render))
- self.assertEqual([
+ self.assertThat(self.calls, MatchesListwise([
# First the oops is generated
- {'id': 1,
- 'type': 'ValueError',
- 'url': 'http://example.com/demo?param=value',
- 'value': u'boom, yo'},
+ MatchesOOPS({'value': 'boom, yo'}),
# Then the middleware responses
- ('start error', '500 Internal Server Error',
- [('Content-Type', 'text/html')], ValueError, ('boom, yo',)),
- ], self.calls)
+ Equals((
+ 'start error',
+ '500 Internal Server Error',
+ [('Content-Type', 'text/html'), ('X-Oops-Id', '1')],
+ ValueError, ('boom, yo',))),
+ ]))
self.assertEqual(iterated, 'woo')
def test_oops_url_in_context(self):
@@ -266,8 +323,16 @@
raise ValueError('boom, yo')
config = self.config_for_oopsing()
self.wrap_and_run(inner, config)
- self.assertEqual('http://example.com/demo?param=value',
- self.calls[0]['url'])
+ self.assertThat(self.calls, MatchesListwise([
+ # First the oops is generated - with a url.
+ MatchesOOPS({'url': 'http://example.com/demo?param=value'}),
+ # Then the middleware responses
+ Equals((
+ 'start error',
+ '500 Internal Server Error',
+ [('Content-Type', 'text/html'), ('X-Oops-Id', '1')],
+ ValueError, ('boom, yo',))),
+ ]))
def test_start_response_with_just_exc_info_generates_oops(self):
def inner(environ, start_response):
@@ -275,16 +340,51 @@
raise ValueError('boom, yo')
except ValueError:
exc_info = sys.exc_info()
- start_response('500 FAIL', [], exc_info)
+ start_response(
+ '500 FAIL', [('content-type', 'text/plain')], exc_info)
yield 'body'
config = self.config_for_oopsing()
- self.wrap_and_run(inner, config)
- expected_report = {
- 'id': 1,
- 'type': 'ValueError',
- 'url': 'http://example.com/demo?param=value',
- 'value': u'boom, yo'}
+ contents = self.wrap_and_run(inner, config)
+ # The body from the wrapped app is preserved.
+ self.assertEqual('body', contents)
+ # The header though have an X-Oops-Id header added:
+ headers = [
+ ('content-type', 'text/plain'),
+ ('X-Oops-Id', '1')
+ ]
expected_start_response = \
- ('start error', '500 FAIL', [], ValueError, ('boom, yo',))
- self.assertEqual([expected_report, expected_start_response],
- self.calls)
+ ('start error', '500 FAIL', headers, ValueError, ('boom, yo',))
+ self.assertThat(self.calls, MatchesListwise([
+ # The oops is logged:
+ MatchesOOPS({'value': 'boom, yo'}),
+ # And we have forwarded on the wrapped apps start_response call.
+ Equals(expected_start_response),
+ ]))
+
+ def test_sniff_404_not_published_does_not_error(self):
+ def inner(environ, start_response):
+ start_response('404 MISSING', [])
+ yield 'pretty 404'
+ contents = self.wrap_and_run(
+ inner, Config(), params=dict(oops_on_status=['404']))
+ self.assertEqual('pretty 404', contents)
+ expected_start_response = ('404 MISSING', [])
+ self.assertEqual([expected_start_response], self.calls)
+
+ def test_sniff_404_published_does_not_error(self):
+ # Sniffing of unexpected pages to gather oops data doesn't alter the
+ # wrapped apps page. It also does not add an X-OOPS-ID header because
+ # client side scripts often look for that to signal errors.
+ def inner(environ, start_response):
+ start_response('404 MISSING', [])
+ yield 'pretty 404'
+ config = self.config_for_oopsing()
+ contents = self.wrap_and_run(
+ inner, config, params=dict(oops_on_status=['404']))
+ self.assertEqual('pretty 404', contents)
+ expected_start_response = ('404 MISSING', [])
+ self.assertThat(self.calls, MatchesListwise([
+ # An OOPS is logged with the observed response code.
+ MatchesOOPS({'HTTP_STATUS': '404'}),
+ Equals(expected_start_response),
+ ]))
=== modified file 'setup.py'
--- setup.py 2011-08-22 07:07:16 +0000
+++ setup.py 2011-08-24 01:25:17 +0000
@@ -23,7 +23,7 @@
os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
setup(name="oops_wsgi",
- version="0.0.2",
+ version="0.0.3",
description=\
"OOPS wsgi middleware.",
long_description=description,
