launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04643
[Merge] lp:~lifeless/python-oops-wsgi/extraction into lp:python-oops-wsgi
Robert Collins has proposed merging lp:~lifeless/python-oops-wsgi/extraction into lp:python-oops-wsgi.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/python-oops-wsgi/extraction/+merge/71839
Extract and refactor with an eye to dead-simple the launchpad-loggerhead wsgi middleware. The tests in LP provided the primary guidance for things that could go wrong.
--
https://code.launchpad.net/~lifeless/python-oops-wsgi/extraction/+merge/71839
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-wsgi/extraction into lp:python-oops-wsgi.
=== modified file 'README'
--- README 2011-08-17 06:36:18 +0000
+++ README 2011-08-17 09:53:30 +0000
@@ -27,6 +27,8 @@
* oops (http://pypi.python.org/pypi/oops)
+* paste
+
Testing Dependencies
====================
@@ -70,11 +72,16 @@
client and the exception will propogate up the wsgi app stack.
You can customise the error page if you supply a helper that accepts (environ,
-oopsid) and returns HTML to be sent to the client.
-
- >>> def myerror_html(environ, oops_id):
- ... return '<html><body><h1>OOPS! %s</h1></body></html>'
- >>> app = oops_wsgi.make_app(app, config, myerror_html)
+report) and returns HTML to be sent to the client.
+
+ >>> def myerror_html(environ, report):
+ ... return '<html><body><h1>OOPS! %s</h1></body></html>' % report['id']
+ >>> app = oops_wsgi.make_app(app, config, error_render=myerror_html)
+
+Or you can supply a string template to be formatted with the report.
+
+ >>> json_template='{"oopsid" : "%(id)s"}'
+ >>> app = oops_wsgi.make_app(app, config, error_template=json_template)
More coming soon.
=== modified file 'buildout.cfg'
--- buildout.cfg 2011-08-17 06:36:18 +0000
+++ buildout.cfg 2011-08-17 09:53:30 +0000
@@ -34,5 +34,6 @@
eggs = oops-wsgi [test]
include-site-packages = true
allowed-eggs-from-site-packages =
+ paste
subunit
interpreter = py
=== modified file 'oops_wsgi/__init__.py'
--- oops_wsgi/__init__.py 2011-08-17 06:36:18 +0000
+++ oops_wsgi/__init__.py 2011-08-17 09:53:30 +0000
@@ -49,11 +49,16 @@
client and the exception will propogate up the wsgi app stack.
You can customise the error page if you supply a helper that accepts (environ,
-oopsid) and returns HTML to be sent to the client.
-
- >>> def myerror_html(environ, oops_id):
- ... return '<html><body><h1>OOPS! %s</h1></body></html>'
- >>> app = oops_wsgi.make_app(app, config, myerror_html)
+report) and returns HTML to be sent to the client.
+
+ >>> def myerror_html(environ, report):
+ ... return '<html><body><h1>OOPS! %s</h1></body></html>' % report['id']
+ >>> app = oops_wsgi.make_app(app, config, error_render=myerror_html)
+
+Or you can supply a string template to be formatted with the report.
+
+ >>> json_template='{"oopsid" : "%(id)s"}'
+ >>> app = oops_wsgi.make_app(app, config, error_template=json_template)
"""
@@ -74,3 +79,4 @@
'make_app'
]
+from oops_wsgi.middleware import make_app
=== added file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py 1970-01-01 00:00:00 +0000
+++ oops_wsgi/middleware.py 2011-08-17 09:53:30 +0000
@@ -0,0 +1,118 @@
+# 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).
+
+"""WSGI middleware to integrate with an oops.Config."""
+
+__metaclass__ = type
+
+import socket
+import sys
+
+__all__ = [
+ 'make_app',
+ ]
+
+
+default_error_template='''<html>
+<head><title>Oops! - %(id)s</title></head>
+<body>
+<h1>Oops!</h1>
+<p>Something broke while generating the page.
+Please try again in a few minutes, and if the problem persists file
+a bug or contact customer support. PLease quote OOPS-ID
+<strong>%(id)s</strong>
+</p></body></html>'''
+
+
+def make_app(app, config, template=default_error_template,
+ content_type='text/html', error_render=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
+ page shown.
+
+ If the body of a reply has already started the error will be forwarded to
+ config and also re-raised.
+
+ If there are no publishers, or an error is filtered, the error will be
+ re-raised rather than an error page shown. This permits containing
+ middleware to show custom errors (for 404's, for instance), perhaps even
+ for just some occurences of the issue.
+
+ :param app: A WSGI app.
+ :param config: An oops.Config.
+ :param template: Optional string template to use when reporting the oops to
+ the client. If not supplied a default template is used (unless an
+ error_render function has been supplied).
+ :param content_type: The content type for error pages. Defaults to
+ text/html.
+ :param error_render: Optional custom renderer for presenting error reports
+ to clients. Should be a callable taking the report as its only
+ parameter.
+ :return: A WSGI app.
+ """
+ def oops_middleware(environ, start_response):
+ """OOPS inserting middleware.
+
+ This has the following WSGI properties:
+ * start_response is buffered until either write() is called, or the
+ wrapped app starts yielding content.
+ * Exceptions that are ignored by the oops config get re-raised.
+ * socket errors and GeneratorExit errors are passed through without
+ * being forward to the oops system.
+ """
+ state = {}
+ def oops_write(bytes):
+ write = state.get('write')
+ if write is None:
+ status, headers = state.pop('response')
+ # Signal that we have called start_response
+ state['write'] = start_response(status, headers)
+ write = state['write']
+ write(bytes)
+ def oops_start_response(status, headers):
+ state['response'] = (status, headers)
+ return oops_write
+ try:
+ iterator = iter(app(environ, oops_start_response))
+ for bytes in iterator:
+ if 'write' not in state:
+ status, headers = state.pop('response')
+ # Signal that we have called start_response
+ state['write'] = start_response(status, headers)
+ yield bytes
+ except socket.error:
+ raise
+ except GeneratorExit:
+ raise
+ except Exception:
+ exc_info = sys.exc_info()
+ report = config.create(dict(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
+ # transmitted the wrapped apps headers - either way we can't
+ # 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)])
+ if error_render is not None:
+ yield error_render(report)
+ else:
+ yield template % report
+
+ return oops_middleware
=== modified file 'oops_wsgi/tests/__init__.py'
--- oops_wsgi/tests/__init__.py 2011-08-17 06:36:18 +0000
+++ oops_wsgi/tests/__init__.py 2011-08-17 09:53:30 +0000
@@ -21,6 +21,7 @@
def test_suite():
test_mod_names = [
+ 'middleware',
]
return TestLoader().loadTestsFromNames(
['oops_wsgi.tests.test_' + name for name in test_mod_names])
=== added file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py 1970-01-01 00:00:00 +0000
+++ oops_wsgi/tests/test_middleware.py 2011-08-17 09:53:30 +0000
@@ -0,0 +1,235 @@
+# 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 middleware."""
+
+import errno
+from doctest import ELLIPSIS
+import gc
+import socket
+from textwrap import dedent
+
+from oops import (
+ Config,
+ createhooks,
+ )
+from testtools import TestCase
+from testtools.matchers import (
+ DocTestMatches,
+ raises,
+ )
+
+from oops_wsgi import make_app
+
+
+class TestMakeApp(TestCase):
+
+ def setUp(self):
+ super(TestMakeApp, self).setUp()
+ self.calls = []
+
+ 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)
+ def start_response(status, headers):
+ if failing_start:
+ raise socket.error(errno.EPIPE, 'Connection closed')
+ self.calls.append((status, headers))
+ if failing_write:
+ def fail(bytes):
+ raise socket.error(errno.EPIPE, 'Connection closed')
+ return fail
+ else:
+ return self.calls.append
+ if not failing_server_write:
+ return ''.join(list(app({}, start_response)))
+ else:
+ iterator = iter(app({}, start_response))
+ # get one item from it, which is enough to ensure we've activated
+ # all the frames.
+ step = iterator.next()
+ # the client pipe is closed or something - we discard the iterator
+ del iterator
+ gc.collect()
+ return step
+
+ def test_make_app_returns_app(self):
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ self.calls.append('inner')
+ yield ''
+ self.wrap_and_run(inner, Config())
+ self.assertEqual([
+ # the start_reponse gets buffered.
+ 'inner',
+ ('200 OK', [('Content-Type', 'text/html')]),
+ ], self.calls)
+
+ def test_unpublished_exception_raises(self):
+ def inner(environ, start_response):
+ raise ValueError('foo')
+ yield ''
+ self.assertThat(lambda:self.wrap_and_run(inner, Config()),
+ raises(ValueError('foo')))
+
+ def test_filtered_exception_raises(self):
+ def inner(environ, start_response):
+ raise ValueError('foo')
+ yield ''
+ config = Config()
+ config.filters.append(lambda report: True)
+ self.assertThat(lambda:self.wrap_and_run(inner, Config()),
+ raises(ValueError('foo')))
+
+ def publish_to_calls(self, report):
+ self.calls.append(report)
+ report['id'] = len(self.calls)
+ return report['id']
+
+ def test_write_with_socket_exceptions_raise_no_oops(self):
+ # When the wrapped app uses the 'write' function (which is
+ # deprecated..., socket errors will raise within the oops middleware
+ # but should be ignored.
+ def inner(environ, start_response):
+ write = start_response('200 OK', [('Content-Type', 'text/html')])
+ write('')
+ config = Config()
+ config.publishers.append(self.publish_to_calls)
+ self.assertThat(lambda:self.wrap_and_run(inner, config,
+ failing_write=True), raises(socket.error))
+ self.assertEqual([
+ ('200 OK', [('Content-Type', 'text/html')]),
+ ], self.calls)
+
+ def test_GeneratorExit_while_iterating_raise_no_oops(self):
+ # if the wgsi server encounters a dropped socket, any iterators will be
+ # interrupted with GeneratorExit, and this is normal - its not caught
+ # or put into the OOPS system.
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ while True:
+ yield ''
+ config = Config()
+ config.publishers.append(self.publish_to_calls)
+ self.assertEqual('', self.wrap_and_run(inner, config,
+ failing_server_write=True))
+ self.assertEqual([
+ ('200 OK', [('Content-Type', 'text/html')]),
+ ], self.calls)
+
+ def test_socket_error_from_start_response_raise_no_oops(self):
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ yield ''
+ config = Config()
+ config.publishers.append(self.publish_to_calls)
+ self.assertThat(lambda:self.wrap_and_run(inner, config,
+ failing_start=True), raises(socket.error))
+ self.assertEqual([], self.calls)
+
+ def test_start_response_not_called_if_fails_after_start_no_oops(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
+ # response cannot be replaced. If the exception is one that would not
+ # oops (e.g. no publishers), start_response is never called.
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ raise ValueError('boom, yo')
+ yield ''
+ config = Config()
+ self.assertThat(lambda:self.wrap_and_run(inner, config,),
+ raises(ValueError('boom, yo')))
+ self.assertEqual([], self.calls)
+
+ 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_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
+ # response cannot be replaced. If the exception is one that would
+ # oops start_response is only called with 500.
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ raise ValueError('boom, yo')
+ yield ''
+ config = self.config_for_oopsing()
+ iterated = self.wrap_and_run(inner, config)
+ self.assertEqual([
+ # First the oops is generated
+ {'id': 1,
+ 'type': 'ValueError',
+ 'value': u'boom, yo'},
+ # Then the middleware responses
+ ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+ ], self.calls)
+ self.assertThat(iterated, DocTestMatches(dedent("""\
+ <html>
+ <head><title>Oops! - ...</title></head>
+ <body>
+ <h1>Oops!</h1>
+ <p>Something broke while generating the page.
+ Please try again in a few minutes, and if the problem persists file
+ a bug or contact customer support. PLease quote OOPS-ID
+ <strong>...</strong>
+ </p></body></html>
+ """),ELLIPSIS))
+
+ def test_custom_template_content_type(self):
+ def inner(environ, start_response):
+ raise ValueError('boom, yo')
+ config = self.config_for_oopsing()
+ iterated = self.wrap_and_run(inner, config,
+ params=dict(content_type='text/json',
+ template='{"oopsid" : "%(id)s"}'))
+ self.assertEqual([
+ # First the oops is generated
+ {'id': 1,
+ 'type': 'ValueError',
+ 'value': u'boom, yo'},
+ # Then the middleware responses
+ ('500 Internal Server Error', [('Content-Type', 'text/json')]),
+ ], self.calls)
+ self.assertThat(iterated, DocTestMatches(dedent("""\
+ {"oopsid" : "..."}"""), ELLIPSIS))
+
+ def test_custom_renderer(self):
+ def inner(environ, start_response):
+ raise ValueError('boom, yo')
+ config = self.config_for_oopsing()
+ def error_render(report):
+ return "woo"
+ iterated = self.wrap_and_run(inner, config,
+ params=dict(error_render=error_render))
+ self.assertEqual([
+ # First the oops is generated
+ {'id': 1,
+ 'type': 'ValueError',
+ 'value': u'boom, yo'},
+ # Then the middleware responses
+ ('500 Internal Server Error', [('Content-Type', 'text/html')]),
+ ], self.calls)
+ self.assertEqual(iterated, 'woo')
=== modified file 'setup.py'
--- setup.py 2011-08-17 06:36:18 +0000
+++ setup.py 2011-08-17 09:53:30 +0000
@@ -41,6 +41,7 @@
],
install_requires = [
'oops',
+ 'paste',
],
extras_require = dict(
test=[
=== modified file 'versions.cfg'
--- versions.cfg 2011-08-17 06:36:18 +0000
+++ versions.cfg 2011-08-17 09:53:30 +0000
@@ -5,6 +5,7 @@
fixtures = 0.3.6
iso8601 = 0.1.4
oops = 0.0.6
+paste = 1.7.2
pytz = 2010o
setuptools = 0.6c11
testtools = 0.9.11
