launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/reusable-error-middleware into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/reusable-error-middleware/+merge/94167

Extract the generic bit (well, basically all of it) of the APIErrorsMiddleware so that I can reuse it smoothly in the metadata server.  This is to serve another branch I already have working with a prototype of these changes.

Moved the existing tests over to the generic part.  Documented the regex.  Added test coverage to ensure that the thing ignores errors it's not meant to handle (which we should have done all along).  Created new test case specifically for its application in the API.  Factored out low-level test setup so test methods can be shorter.  Where appropriate, checked (response.status_code, response.content) in one assertion to get more helpful failures.  Removed duplicated check that ValidationError produces BAD_REQUEST in the test that verifies its exception dict.  More verbose test names.
-- 
https://code.launchpad.net/~jtv/maas/reusable-error-middleware/+merge/94167
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/reusable-error-middleware into lp:maas.

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2012-02-16 11:35:58 +0000
+++ src/maasserver/middleware.py	2012-02-22 14:03:54 +0000
@@ -11,6 +11,8 @@
 __metaclass__ = type
 __all__ = [
     "AccessMiddleware",
+    "APIErrorsMiddleware",
+    "ExceptionMiddleware",
     ]
 
 import json
@@ -68,33 +70,41 @@
                 return None
 
 
-class APIErrorsMiddleware:
-    """This middleware_ converts exceptions raised in execution of an API
-    method into proper API errors (like "404 Not Found" errors or
-    "400 Bad Request" errors).
+class ExceptionMiddleware:
+    """Convert exceptions into appropriate HttpResponse responses.
+
+    For example, a MaasAPINotFound exception will result in a 404 response
+    to the client.  Validation errors become "bad request"
+
+    Subclass this for each sub-tree of the http path tree that needs
+    exceptions handled in this way, and provide a `path_regex`.
 
     .. middleware: https://docs.djangoproject.com
        /en/dev/topics/http/middleware/
 
-    - Convert MaasAPIException instances into the corresponding error
-      (see maasserver.exceptions).
-    - Convert ValidationError instances into Bad Request error.
+    :ivar path_regex: A regular expression matching any path that needs
+        its exceptions handled.
     """
+
+    path_regex = None
+
     def __init__(self):
-        self.api_regexp = re.compile(settings.API_URL_REGEXP)
+        self.path_matcher = re.compile(self.path_regex)
 
     def process_exception(self, request, exception):
-        if not self.api_regexp.match(request.path):
-            # The exception was *not* raised in an API call.
+        """Called by django: process an exception."""
+        if not self.path_matcher.match(request.path):
+            # Not a path we're handling exceptions for.
             return None
 
+        encoding = b'utf-8'
         if isinstance(exception, MaasAPIException):
             # The exception is a MaasAPIException: exception.api_error
             # will give us the proper error type.
             return HttpResponse(
-                content=unicode(exception).encode('utf-8'),
+                content=unicode(exception).encode(encoding),
                 status=exception.api_error,
-                mimetype="text/plain; charset=utf-8")
+                mimetype=b"text/plain; charset=%s" % encoding)
         elif isinstance(exception, ValidationError):
             if hasattr(exception, 'message_dict'):
                 # Complex validation error with multiple fields:
@@ -105,14 +115,20 @@
             else:
                 # Simple validation error: return the error message.
                 return HttpResponseBadRequest(
-                    unicode(''.join(exception.messages)).encode('utf-8'),
-                    mimetype="text/plain; charset=utf-8")
+                    unicode(''.join(exception.messages)).encode(encoding),
+                    mimetype=b"text/plain; charset=%s" % encoding)
         else:
             # Do not handle the exception, this will result in a
             # "Internal Server Error" response.
             return None
 
 
+class APIErrorsMiddleware(ExceptionMiddleware):
+    """Report exceptions from API requests as HTTP error responses."""
+
+    path_regex = settings.API_URL_REGEXP
+
+
 class ConsoleExceptionMiddleware:
     def process_exception(self, request, exception):
         import traceback

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2012-02-13 12:08:50 +0000
+++ src/maasserver/tests/test_middleware.py	2012-02-22 14:03:54 +0000
@@ -16,65 +16,116 @@
 
 from django.core.exceptions import ValidationError
 from django.test.client import RequestFactory
-from maasserver.exceptions import MaasAPIException
-from maasserver.middleware import APIErrorsMiddleware
-from maasserver.testing import TestCase
-
-
-class APIErrorsMiddlewareTest(TestCase):
-
-    def get_fake_api_request(self):
-        rf = RequestFactory()
-        return rf.get('/api/hello/')
-
-    def test_process_UnknownException(self):
+from maasserver.exceptions import (
+    MaasAPIException,
+    MaasAPINotFound,
+    )
+from maasserver.middleware import (
+    APIErrorsMiddleware,
+    ExceptionMiddleware,
+    )
+from maasserver.testing import (
+    factory,
+    TestCase,
+    )
+
+
+def fake_request(base_path):
+    """Create a fake request.
+
+    :param base_path: The base path to make the request to.
+    """
+    rf = RequestFactory()
+    return rf.get('%s/hello/' % base_path)
+
+
+class ExceptionMiddlewareTest(TestCase):
+
+    def make_base_path(self):
+        """Return a path to handle exceptions for."""
+        return "/%s" % factory.getRandomString()
+
+    def make_middleware(self, base_path):
+        """Create an ExceptionMiddleware for base_path."""
+        class TestingExceptionMiddleware(ExceptionMiddleware):
+            path_regex = base_path
+
+        return TestingExceptionMiddleware()
+
+    def process_exception(self, exception):
+        """Run a given exception through a fake ExceptionMiddleware.
+
+        :param exception: The exception to simulate.
+        :type exception: Exception
+        :return: The response as returned by the ExceptionMiddleware.
+        :rtype: HttpResponse or None.
+        """
+        base_path = self.make_base_path()
+        middleware = self.make_middleware(base_path)
+        request = fake_request(base_path)
+        return middleware.process_exception(request, exception)
+
+    def test_ignores_paths_outside_path_regex(self):
+        middleware = self.make_middleware(self.make_base_path())
+        request = fake_request(self.make_base_path())
+        exception = MaasAPINotFound("Huh?")
+        self.assertIsNone(middleware.process_exception(request, exception))
+
+    def test_ignores_unknown_exception(self):
         # An unknown exception is not processed by the middleware
         # (returns None).
-        middleware = APIErrorsMiddleware()
-        exception = ValueError("Error occurred!")
-        fake_request = self.get_fake_api_request()
-        response = middleware.process_exception(fake_request, exception)
-        self.assertIsNone(response)
-
-    def test_process_MaasAPIException(self):
-        middleware = APIErrorsMiddleware()
-
+        self.assertIsNone(
+            self.process_exception(ValueError("Error occurred!")))
+
+    def test_reports_MaasAPIException_with_appropriate_api_error(self):
         class MyException(MaasAPIException):
             api_error = httplib.UNAUTHORIZED
+
         exception = MyException("Error occurred!")
-        fake_request = self.get_fake_api_request()
-        response = middleware.process_exception(fake_request, exception)
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
-        self.assertEqual("Error occurred!", response.content)
-
-    def test_process_MaasAPIException_unicode(self):
-        middleware = APIErrorsMiddleware()
-
+        response = self.process_exception(exception)
+        self.assertEqual(
+            (httplib.UNAUTHORIZED, "Error occurred!"),
+            (response.status_code, response.content))
+
+    def test_renders_MaasAPIException_as_unicode(self):
         class MyException(MaasAPIException):
             api_error = httplib.UNAUTHORIZED
-        exception = MyException("Error %s" % unichr(233))
-        fake_request = self.get_fake_api_request()
-        response = middleware.process_exception(fake_request, exception)
-        self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
-        self.assertEqual(
-            "Error %s" % unichr(233),
-            response.content.decode('utf8'))
-
-    def test_process_ValidationError_message_dict(self):
-        middleware = APIErrorsMiddleware()
+
+        error_message = "Error %s" % unichr(233)
+        response = self.process_exception(MyException(error_message))
+        self.assertEqual(
+            (httplib.UNAUTHORIZED, error_message),
+            (response.status_code, response.content.decode('utf-8')))
+
+    def test_reports_ValidationError_as_Bad_Request(self):
+        response = self.process_exception(ValidationError("Validation Error"))
+        self.assertEqual(
+            (httplib.BAD_REQUEST, "Validation Error"),
+            (response.status_code, response.content))
+
+    def test_returns_ValidationError_message_dict_as_json(self):
         exception = ValidationError("Error")
-        setattr(exception, 'message_dict', {'hostname': 'invalid'})
-        fake_request = self.get_fake_api_request()
-        response = middleware.process_exception(fake_request, exception)
-        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+        exception_dict = {'hostname': 'invalid'}
+        setattr(exception, 'message_dict', exception_dict)
+        response = self.process_exception(exception)
+        self.assertEqual(exception_dict, json.loads(response.content))
+        self.assertIn('application/json', response['Content-Type'])
+
+
+class APIErrorsMiddlewareTest(TestCase):
+
+    def test_handles_error_on_API(self):
+        middleware = APIErrorsMiddleware()
+        non_api_request = fake_request("/api/hello")
+        exception = MaasAPINotFound("Have you looked under the couch?")
+        response = middleware.process_exception(non_api_request, exception)
         self.assertEqual(
-            {'hostname': 'invalid'},
-            json.loads(response.content))
+            (httplib.NOT_FOUND, "Have you looked under the couch?"),
+            (response.status_code, response.content))
 
-    def test_process_ValidationError(self):
+    def test_ignores_error_outside_API(self):
         middleware = APIErrorsMiddleware()
-        exception = ValidationError("Validation Error")
-        fake_request = self.get_fake_api_request()
-        response = middleware.process_exception(fake_request, exception)
-        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
-        self.assertEqual("Validation Error", response.content)
+        non_api_request = fake_request("/middleware/api/hello")
+        exception = MaasAPINotFound("Have you looked under the couch?")
+        self.assertIsNone(
+            middleware.process_exception(non_api_request, exception))