launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/bug-979858-pserv-errors into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #979858 in MAAS: "pserv errors are not caught when raised from non-api requests."
  https://bugs.launchpad.net/maas/+bug/979858

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-979858-pserv-errors/+merge/101756

This branch adds a new middleware that will catch MAASExceptions errors raised when servicing POST requests.  This middleware will then add a message (using Django's messaging framework) with the error string and redirect to the same page (using GET).  The main goal for this is to fix the error 500 that we get when the provisioning server raises exceptions (when using the node action buttons or when editing a node).  When this happens in an API request (when adding a node for instance), the APIErrorsMiddleware takes care of transforming the error into a proper error response.  This is the non-api conterpart.

The middleware only handles POST requests because:
a) I would not want to do in an infinite redirect loop if a GET request was to raise an MAASException
b) I think it's fine to assume that most of the heavy work is done on POST request (GET requests should not modify data).
c) it fixes the problem at hand (MAASExceptions raised by the provisioning server)

= Pre-imp =

I talked with Gavin (briefly I admin) about my idea to use a middleware for this.
-- 
https://code.launchpad.net/~rvb/maas/bug-979858-pserv-errors/+merge/101756
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-979858-pserv-errors into lp:maas.

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-04-12 04:16:23 +0000
+++ src/maas/settings.py	2012-04-12 15:29:22 +0000
@@ -208,6 +208,10 @@
 MIDDLEWARE_CLASSES = (
     'django.middleware.common.CommonMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',
+    # ErrorsMiddleware catches all the exceptions and issues
+    # a redirect.  Specialize error handling middlewares
+    # (like APIErrorsMiddleware) should be placed after it.
+    'maasserver.middleware.ErrorsMiddleware',
     'maasserver.middleware.APIErrorsMiddleware',
     'metadataserver.middleware.MetadataErrorsMiddleware',
     'django.middleware.transaction.TransactionMiddleware',

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2012-04-02 14:45:28 +0000
+++ src/maasserver/middleware.py	2012-04-12 15:29:22 +0000
@@ -12,6 +12,7 @@
 __all__ = [
     "AccessMiddleware",
     "APIErrorsMiddleware",
+    "ErrorsMiddleware",
     "ExceptionMiddleware",
     ]
 
@@ -25,6 +26,7 @@
 import re
 
 from django.conf import settings
+from django.contrib import messages
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
@@ -37,7 +39,10 @@
     HttpResponseRedirect,
     )
 from django.utils.http import urlquote_plus
-from maasserver.exceptions import MAASAPIException
+from maasserver.exceptions import (
+    MAASAPIException,
+    MAASException,
+    )
 
 
 def get_relative_path(path):
@@ -163,6 +168,20 @@
     path_regex = settings.API_URL_REGEXP
 
 
+class ErrorsMiddleware:
+    """Handle MAASException exceptions in POST requests: add a message
+    with the error string and redirect to the same page (using GET).
+    """
+
+    def process_exception(self, request, exception):
+        if request.method == 'POST' and isinstance(exception, MAASException):
+            messages.error(request, unicode(exception))
+            return HttpResponseRedirect(request.path)
+        else:
+            # Not a MAASException or not a POST request: do not handle it.
+            return None
+
+
 class ExceptionLoggerMiddleware:
 
     def process_exception(self, request, exception):

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2012-04-02 14:58:26 +0000
+++ src/maasserver/tests/test_middleware.py	2012-04-12 15:29:22 +0000
@@ -16,6 +16,7 @@
 import logging
 from tempfile import NamedTemporaryFile
 
+from django.contrib.messages import constants
 from django.core.exceptions import (
     PermissionDenied,
     ValidationError,
@@ -24,23 +25,44 @@
 from maasserver.exceptions import (
     MAASAPIException,
     MAASAPINotFound,
+    MAASException,
     )
 from maasserver.middleware import (
     APIErrorsMiddleware,
+    ErrorsMiddleware,
     ExceptionLoggerMiddleware,
     ExceptionMiddleware,
     )
 from maasserver.testing.factory import factory
-from maasserver.testing.testcase import TestCase
-
-
-def fake_request(base_path):
+from maasserver.testing.testcase import (
+    LoggedInTestCase,
+    TestCase,
+    )
+
+
+class Messages:
+    """A class to record messages published by Django messaging
+    framework.
+    """
+
+    messages = []
+
+    def add(self, level, message, extras):
+        self.messages.append((level, message, extras))
+
+
+def fake_request(path, method='GET'):
     """Create a fake request.
 
-    :param base_path: The base path to make the request to.
+    :param path: The path to make the request to.
+    :param method: The method to use for the reques
+        ('GET' or 'POST').
     """
     rf = RequestFactory()
-    return rf.get('%s/hello/' % base_path)
+    request = rf.get(path)
+    request.method = method
+    request._messages = Messages()
+    return request
 
 
 class ExceptionMiddlewareTest(TestCase):
@@ -164,3 +186,35 @@
                 fake_request('/middleware/api/hello'),
                 ValueError(error_text))
             self.assertIn(error_text, open(logfile.name).read())
+
+
+class ErrorsMiddlewareTest(LoggedInTestCase):
+
+    def test_error_middleware_ignores_get_requests(self):
+        request = fake_request(factory.getRandomString(), 'GET')
+        exception = MAASException()
+        middleware = ErrorsMiddleware()
+        response = middleware.process_exception(request, exception)
+        self.assertIsNone(response)
+
+    def test_error_middleware_ignores_non_maasexceptions(self):
+        request = fake_request(factory.getRandomString(), 'GET')
+        exception = ValueError()
+        middleware = ErrorsMiddleware()
+        response = middleware.process_exception(request, exception)
+        self.assertIsNone(response)
+
+    def test_error_middleware_handles_maasexception(self):
+        url = factory.getRandomString()
+        request = fake_request(url, 'POST')
+        error_message = factory.getRandomString()
+        exception = MAASException(error_message)
+        middleware = ErrorsMiddleware()
+        response = middleware.process_exception(request, exception)
+        # The response is a redirect.
+        self.assertEqual(
+            (httplib.FOUND, dict(response.items())['Location']),
+            (response.status_code, url))
+        # An error message has been published.
+        self.assertEqual(
+            [(constants.ERROR, error_message, '')], request._messages.messages)

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-12 14:23:32 +0000
+++ src/maasserver/tests/test_views.py	2012-04-12 15:29:22 +0000
@@ -15,6 +15,7 @@
 import httplib
 import os
 import urllib2
+from urlparse import urlparse
 from xmlrpclib import Fault
 
 from django.conf import settings
@@ -29,7 +30,10 @@
     views,
     )
 from maasserver.components import register_persistent_error
-from maasserver.exceptions import NoRabbit
+from maasserver.exceptions import (
+    MAASException,
+    NoRabbit,
+    )
 from maasserver.forms import NodeActionForm
 from maasserver.models import (
     Config,
@@ -57,6 +61,7 @@
 from maasserver.views import (
     get_longpoll_context,
     get_yui_location,
+    NodeEdit,
     proxy_to_longpoll,
     )
 from maastesting.rabbit import uses_rabbit_fixture
@@ -754,6 +759,43 @@
             [message.message for message in response.context['messages']])
 
 
+class MAASExceptionHandledInView(LoggedInTestCase):
+
+    def test_raised_MAASException_redirects(self):
+        # When a MAASException is raised in a POST request, the response
+        # is a redirect to the same page.
+
+        # Patch NodeEdit to error on post.
+        def post(self, request, *args, **kwargs):
+            raise MAASException()
+        self.patch(NodeEdit, 'post', post)
+        node = factory.make_node(owner=self.logged_in_user)
+        node_edit_link = reverse('node-edit', args=[node.system_id])
+        response = self.client.post(node_edit_link, {})
+        redirect_url = urlparse(dict(response.items())['Location']).path
+        self.assertEqual(
+            (httplib.FOUND, redirect_url),
+            (response.status_code, node_edit_link))
+
+    def test_raised_MAASException_publishes_message(self):
+        # When a MAASException is raised in a POST request, a message is
+        # published with the error message.
+        error_message = factory.getRandomString()
+
+        # Patch NodeEdit to error on post.
+        def post(self, request, *args, **kwargs):
+            raise MAASException(error_message)
+        self.patch(NodeEdit, 'post', post)
+        node = factory.make_node(owner=self.logged_in_user)
+        node_edit_link = reverse('node-edit', args=[node.system_id])
+        self.client.post(node_edit_link, {})
+        # Manually perform the redirect: i.e. get the same page.
+        response = self.client.get(node_edit_link, {})
+        self.assertEqual(
+            [error_message],
+            [message.message for message in response.context['messages']])
+
+
 class AdminNodeViewsTest(AdminLoggedInTestCase):
 
     def test_admin_can_edit_nodes(self):