Merge lp:~michael.nelson/ubuntu-recommender/944075-python-oops-wsgi into lp:ubuntu-recommender

[Michael Nelson]

Adds schema config options for for oops_wsgi integration and tests.

Overview
========
This branch just documents and adds tests to confirm that a configuration using oops-wsgi will work (and updates our schema to support it).

Turning this on in production will simply require updating the settings and django.wsgi in our config branch as documented here. But that can't be done until https://portal.admin.canonical.com/50529 is complete (unless we want to simply store the oopses locally, which could be done immediately with a config branch?)

The new dependencies are available in cat (thanks james_w!), so we'll need to ensure they are installed on the machine: python-oops-wsgi, python-oops-dictconfig and python-oops_datedir_repo

I'll create a ca-config/ur-config branch that can land once that's done.

NOTE: lazr.restfulclient and launchpad lib are pulled in via oops_datedir_repo :/ see the XXX below with a bug.

[James Westby (james-w)]

On Wed, 07 Mar 2012 15:15:22 -0000, Michael Nelson <email address hidden> wrote:
Non-text part: multipart/mixed
> Michael Nelson has proposed merging lp:~michael.nelson/ubuntu-recommender/944075-python-oops-wsgi into lp:ubuntu-recommender.
>
> Requested reviews:
> Canonical Consumer Applications Hackers (canonical-ca-hackers)
>
> For more details, see:
> https://code.launchpad.net/~michael.nelson/ubuntu-recommender/944075-python-oops-wsgi/+merge/96333
>
> Overview
> ========
> This branch just documents and adds tests to confirm that a configuration using oops-wsgi will work (and updates our schema to support it).

Hi Michael,

Thanks for working on this, it will be great to have this in use across
all our services.

> Turning this on in production will simply require updating the settings and django.wsgi in our config branch as documented here. But that can't be done until https://portal.admin.canonical.com/50529 is complete (unless we want to simply store the oopses locally, which could be done immediately with a config branch?)
>
> The new dependencies will need to be added via branches, as none of python-oops-wsgi, python-oops-dictconfig or python-oops_datedir_repo are available on lucid.

They are all packaged and available in lucid-cat though :-)
(You can use queries like "ssh people.canonical.com ~jamesw/cat-madison
python-oops-wsgi" to see what is in cat)

I'd be happy to work with you on getting the oops id available to the
500 handler, as it's a very useful thing, and something we will need in
many places.

> + class oops_wsgi(schema.Section):
> + oopses = schema.DictOption(default={
> + 'publishers': [{
> + 'type': 'datedir',
> + 'error_dir': 'oopses',
> + 'instance_id': 'dev',
> + }],
> + })

You could use the OopsOption from python-oops-dictconfig here.

> + def test_traceback_in_api_response(self):
> + # The API response should contain the traceback.

Is the API open for use by the public? Would we want to disclose
tracebacks in that case?

Thanks,

James

[lp:~michael.nelson/ubuntu-recommender/944075-python-oops-wsgi]

58. By Michael Nelson on 2012-03-07

REFACTOR: Use OopsOption.

[James Westby (james-w)]

Hi,

This looks good.

I'd leave out the tracebacks, but I don't feel strongly about it at this point.

Thanks,

James

review: Approve

=== modified file 'django_project/urls.py'
--- django_project/urls.py	2012-02-10 13:22:04 +0000
+++ django_project/urls.py	2012-03-07 16:30:26 +0000
@@ -6,6 +6,8 @@
 admin.autodiscover()
 preflight.autodiscover()
 
+handler500 = 'recommender.views.server_error'
+
 urlpatterns = patterns('',
     # Django's admin site:
     (r'^admin/', include(admin.site.urls)),
 
=== modified file 'requirements.txt'
--- requirements.txt	2012-02-29 00:21:56 +0000
+++ requirements.txt	2012-03-07 16:30:26 +0000
@@ -15,3 +15,7 @@
 south==0.7.3
 django-factory
 psycopg2==2.4.1
+oops-wsgi==0.0.10
+oops-dictconfig==0.0.1
+# XXX 2012-03-07 bug=948857 Currently this pulls in lazr.restfulclient and co.
+oops_datedir_repo==0.0.17
 
=== modified file 'src/recommender/schema.py'
--- src/recommender/schema.py	2012-03-02 14:41:35 +0000
+++ src/recommender/schema.py	2012-03-07 16:30:26 +0000
@@ -27,6 +27,7 @@
 import django
 from configglue import schema
 from django_configglue.schema import schemas
+from oops_dictconfig.configglue_options import OopsOption
 
 DjangoSchema = schemas.get(django.get_version())
 
@@ -93,3 +94,16 @@
 
     class logging(schema.Section):
         webapp_logging_config = schema.StringOption(fatal=True)
+
+    class piston(schema.Section):
+        piston_display_errors = schema.BoolOption(default=False)
+        piston_email_errors = schema.BoolOption(default=False)
+
+    class oops_wsgi(schema.Section):
+        oopses = OopsOption(default={
+            'publishers': [{
+                'type': 'datedir',
+                'error_dir': 'oopses',
+                'instance_id': 'dev',
+                }],
+            })
 
=== added directory 'src/recommender/templates/recommender'
=== added file 'src/recommender/templates/recommender/500_api.txt'
--- src/recommender/templates/recommender/500_api.txt	1970-01-01 00:00:00 +0000
+++ src/recommender/templates/recommender/500_api.txt	2012-03-07 16:30:26 +0000
@@ -0,0 +1,6 @@
+API Error - OOPS ID: {{ oops_id }}
+
+There was an error while processing this request. It has been reported to the developers with the above OOPS ID.
+
+Traceback:
+{{ traceback }}
 
=== modified file 'src/recommender/tests/__init__.py'
--- src/recommender/tests/__init__.py	2012-02-28 10:15:15 +0000
+++ src/recommender/tests/__init__.py	2012-03-07 16:30:26 +0000
@@ -1,9 +1,10 @@
 from .test_api import *
 from .test_auth import *
+from .test_commands import *
 from .test_managers import *
 from .test_models_oauthtoken import *
 from .test_models_data import *
+from .test_oops_wsgi_setup import *
 from .test_pep8 import *
-from .test_commands import *
 from .test_slopeone import *
 from .test_utilities import *
 
=== added file 'src/recommender/tests/test_oops_wsgi_setup.py'
--- src/recommender/tests/test_oops_wsgi_setup.py	1970-01-01 00:00:00 +0000
+++ src/recommender/tests/test_oops_wsgi_setup.py	2012-03-07 16:30:26 +0000
@@ -0,0 +1,162 @@
+#   -*- coding: utf-8 -*-
+#   This file is part of the Ubuntu Recommender
+#   Copyright (C) 2011-2012 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/>.
+
+"""Tests for the oops_wsgi setup."""
+
+from __future__ import absolute_import
+
+__metaclass__ = type
+__all__ = [
+    'OopsWsgiTestCase',
+    ]
+
+import bson
+import os
+from shutil import rmtree
+from tempfile import mkdtemp
+
+from django.core.urlresolvers import reverse
+from django.conf import settings
+from django_factory import TestCase
+from mock import (
+    Mock,
+    patch,
+    )
+import oops_dictconfig
+# This is needed to workaround a bug in Django, see the file it is
+# implemented in for more details.
+from oops_wsgi.django import OOPSWSGIHandler
+from oops_wsgi import (
+    make_app,
+    install_hooks,
+    )
+
+from recommender.tests.helpers import patch_settings
+
+
+def make_oops_app():
+    """Create an oops_wsgi handler."""
+
+    application = OOPSWSGIHandler()
+
+    ###
+    # Wrap the application in the Oops wsgi app to catch unhandled exceptions
+    # and create oops for them.
+    #
+    # First we create the config that defines what to do with the oopses.
+
+    config = oops_dictconfig.config_from_dict(settings.OOPSES)
+    install_hooks(config)
+
+    # Then we wrap the django app in the oops one
+    return make_app(application, config, oops_on_status=['500'])
+
+
+class OopsWsgiTestCase(TestCase):
+
+    def setUp(self):
+        super(OopsWsgiTestCase, self).setUp()
+        self.oops_dir = mkdtemp()
+        self.addCleanup(rmtree, self.oops_dir)
+        self.environ = {
+            'PATH_INFO': '/',
+            'QUERY_STRING': '',
+            'REMOTE_ADDR': '127.0.0.1',
+            'REQUEST_METHOD': 'GET',
+            'SCRIPT_NAME': '',
+            'SERVER_NAME': 'testserver',
+            'SERVER_PORT': '80',
+            'SERVER_PROTOCOL': 'HTTP/1.1',
+            'wsgi.version': (1, 0),
+            'wsgi.url_scheme': 'http',
+            'wsgi.multiprocess': True,
+            'wsgi.multithread': False,
+            'wsgi.run_once': False,
+            'wsgi.input': '',
+        }
+
+    def request_api_exception(self):
+        self.environ['PATH_INFO'] = reverse('api-server-status')
+        read_fn_path = 'recommender.api.handlers.StatusHandler.read'
+        with patch(read_fn_path) as mock_read:
+            mock_read.side_effect = ZeroDivisionError
+            response = self.request_wsgi_response(self.environ)
+        return response
+
+    def request_wsgi_response(self, environ):
+        start_response = Mock()
+        with patch_settings(OOPSES={
+            'publishers': [{
+                'type': 'datedir',
+                'error_dir': self.oops_dir,
+                'instance_id': 'dev',
+                }],
+            }):
+            app = make_oops_app()
+            response = app(environ, start_response).next()
+        return response
+
+    def read_oops_from_disk(self):
+        # Helper to read in the oops written to disk.
+        # There is only one date directory in the oops directory.
+        dated_dirs = os.listdir(self.oops_dir)
+        self.assertEqual(1, len(dated_dirs))
+
+        # There is only one oops.
+        todays_oops_path = dated_dirs[0]
+        oops_list = os.listdir(self.oops_dir + '/' + todays_oops_path)
+        self.assertEqual(1, len(oops_list))
+
+        oops_path = self.oops_dir + '/' + todays_oops_path + '/' + oops_list[0]
+        with open(oops_path, "r") as oops_file:
+            oops_file_contents = oops_file.read()
+        return bson.loads(oops_file_contents)
+
+    def test_traceback_in_api_response(self):
+        # The API response should contain the traceback.
+        response = self.request_api_exception()
+
+        self.assertIn('raise effect', response)
+
+    # XXX 2012-03-07 bug=949013 michaeln Can't access oops-id from django.
+    def disabled_test_oops_id_in_api_response(self):
+        # The API response should contain the oops id.
+        response = self.request_api_exception()
+
+        oops = self.read_oops_from_disk()
+        self.assertIn(oops['id'], response)
+
+    def test_traceback_in_api_oops_on_disk(self):
+        # A request resulting in an api oops writes the traceback to the file.
+        response = self.request_api_exception()
+
+        oops = self.read_oops_from_disk()
+        self.assertIn('tb_text', oops)
+        self.assertEqual('ZeroDivisionError', oops['type'])
+
+    def request_non_api_exception(self):
+        self.environ['PATH_INFO'] = reverse('preflight-overview')
+        view_fn_path = 'preflight.views.render_to_response'
+        with patch(view_fn_path) as mock_render:
+            mock_render.side_effect = ZeroDivisionError
+            response = self.request_wsgi_response(self.environ)
+        return response
+
+    def test_normal_template_for_non_api(self):
+        response = self.request_non_api_exception()
+
+        self.assertIn('Internal Server Error', response)
 
=== modified file 'src/recommender/urls.py'
--- src/recommender/urls.py	2011-12-07 04:01:01 +0000
+++ src/recommender/urls.py	2012-03-07 16:30:26 +0000
@@ -23,6 +23,7 @@
     url,
     )
 
+
 urlpatterns = patterns('',
     url(r'^api/1.0/', include('recommender.api.urls')),
 )
 
=== modified file 'src/recommender/views.py'
--- src/recommender/views.py	2011-12-01 15:32:25 +0000
+++ src/recommender/views.py	2012-03-07 16:30:26 +0000
@@ -1,1 +1,51 @@
-# Create your views here.
+#   -*- coding: utf-8 -*-
+#   This file is part of the Ubuntu Recommender
+#   Copyright (C) 2011-2012 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/>.
+
+"""Views for Ubuntu Recommender."""
+
+import traceback
+
+from django.http import HttpResponseServerError
+from django.template import (
+    Context,
+    loader,
+    )
+from django.views.defaults import server_error as default_server_error
+
+
+def server_error(request):
+    if request.META['PATH_INFO'].startswith('/api'):
+        t = loader.get_template('recommender/500_api.txt')
+        traceback_formatted = 'Traceback not available.'
+        oops_id = 'OOPS ID not available.'
+        oops_context = request.META.get('oops.context')
+        if oops_context:
+            exc_info = oops_context.get('exc_info')
+            if exc_info and len(exc_info) >= 3:
+                tb_object = exc_info[2]
+                if isinstance(tb_object, traceback.types.TracebackType):
+                    traceback_formatted = "\n".join(
+                        traceback.format_tb(exc_info[2]))
+
+        response = HttpResponseServerError(
+            t.render(Context({
+                'traceback': traceback_formatted,
+                'oops_id': oops_id,
+                })))
+        return response
+    else:
+        return default_server_error(request)