launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06340
[Merge] lp:~rvb/maas/maas-api-exceptions into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/maas-api-exceptions into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/maas-api-exceptions/+merge/92683
This branch introduces a new exceptions.py module and a new middleware. The exceptions in there have an api_error that defines the API error type that should be returned when an exception is raised in an API call. The middleware is responsible for turning the exceptions raised into the proper API error. This simplifies the code a great deal (cf. the number of lines removed by this branch) because all the exception handling is done by the middleware class. The pattern to handle a new exception in the API is now much more simple: simply add a new exception with the proper api_error in exceptions.py and raise that from the API code. If you have to deal with Django-raised exceptions, simply extend the APIErrorsMiddleware to cope with that exception (have a look at how it's done for validation errors).
--
https://code.launchpad.net/~rvb/maas/maas-api-exceptions/+merge/92683
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/maas-api-exceptions into lp:maas.
=== modified file 'src/maas/development.py'
--- src/maas/development.py 2012-02-09 17:09:23 +0000
+++ src/maas/development.py 2012-02-12 18:35:22 +0000
@@ -136,6 +136,7 @@
'django.contrib.messages.middleware.MessageMiddleware',
'debug_toolbar.middleware.DebugToolbarMiddleware',
'maasserver.middleware.AccessMiddleware',
+ 'maasserver.middleware.APIErrorsMiddleware',
)
ROOT_URLCONF = 'maas.urls'
=== modified file 'src/maas/settings.py'
--- src/maas/settings.py 2012-02-10 11:30:24 +0000
+++ src/maas/settings.py 2012-02-12 18:35:22 +0000
@@ -35,6 +35,10 @@
API_URL_REGEXP = '^/api/'
+# We handle exceptions ourselves (in
+# maasserver.middleware.APIErrorsMiddleware)
+PISTON_DISPLAY_ERRORS = False
+
DEBUG = False
TEMPLATE_DEBUG = DEBUG
YUI_DEBUG = DEBUG
@@ -151,6 +155,7 @@
'django.contrib.auth.middleware.AuthenticationMiddleware',
'django.contrib.messages.middleware.MessageMiddleware',
'maasserver.middleware.AccessMiddleware',
+ 'maasserver.middleware.APIErrorsMiddleware',
)
ROOT_URLCONF = 'maas.urls'
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-02-10 16:50:56 +0000
+++ src/maasserver/api.py 2012-02-12 18:35:22 +0000
@@ -16,13 +16,9 @@
"NodeMacsHandler",
]
-from functools import wraps
import types
-from django.core.exceptions import (
- PermissionDenied,
- ValidationError,
- )
+from django.core.exceptions import ValidationError
from django.http import HttpResponseBadRequest
from django.shortcuts import (
get_object_or_404,
@@ -59,34 +55,6 @@
return rc.FORBIDDEN
-def validate_and_save(obj):
- try:
- obj.full_clean()
- obj.save()
- return obj
- except ValidationError, e:
- return HttpResponseBadRequest(
- e.message_dict, content_type='application/json')
-
-
-def validate_mac_address(mac_address):
- try:
- validate_mac(mac_address)
- return True, None
- except ValidationError:
- return False, HttpResponseBadRequest('Invalid MAC Address.')
-
-
-def perm_denied_handler(view_func):
- def _decorator(request, *args, **kwargs):
- try:
- response = view_func(request, *args, **kwargs)
- return response
- except PermissionDenied:
- return rc.FORBIDDEN
- return wraps(view_func)(_decorator)
-
-
dispatch_methods = {
'GET': 'read',
'POST': 'create',
@@ -216,22 +184,20 @@
model = Node
fields = ('system_id', 'hostname', ('macaddress_set', ('mac_address',)))
- @perm_denied_handler
def read(self, request, system_id):
"""Read a specific Node."""
return Node.objects.get_visible_node_or_404(
system_id=system_id, user=request.user)
- @perm_denied_handler
def update(self, request, system_id):
"""Update a specific Node."""
node = Node.objects.get_visible_node_or_404(
system_id=system_id, user=request.user)
for key, value in request.data.items():
setattr(node, key, value)
- return validate_and_save(node)
+ node.full_clean()
+ return node.save()
- @perm_denied_handler
def delete(self, request, system_id):
"""Delete a specific Node."""
node = Node.objects.get_visible_node_or_404(
@@ -290,7 +256,6 @@
"""
allowed_methods = ('GET', 'POST',)
- @perm_denied_handler
def read(self, request, system_id):
"""Read all MAC Addresses related to a Node."""
node = Node.objects.get_visible_node_or_404(
@@ -319,25 +284,18 @@
fields = ('mac_address',)
model = MACAddress
- @perm_denied_handler
def read(self, request, system_id, mac_address):
"""Read a MAC Address related to a Node."""
node = Node.objects.get_visible_node_or_404(
user=request.user, system_id=system_id)
- valid, response = validate_mac_address(mac_address)
- if not valid:
- return response
+ validate_mac(mac_address)
return get_object_or_404(
MACAddress, node=node, mac_address=mac_address)
- @perm_denied_handler
def delete(self, request, system_id, mac_address):
"""Delete a specific MAC Address for the specified Node."""
- valid, response = validate_mac_address(mac_address)
- if not valid:
- return response
-
+ validate_mac(mac_address)
node = Node.objects.get_visible_node_or_404(
user=request.user, system_id=system_id)
=== added file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/exceptions.py 2012-02-12 18:35:22 +0000
@@ -0,0 +1,32 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Exceptions."""
+
+from __future__ import (
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ "MaasException",
+ "PermissionDenied",
+ ]
+
+
+from piston.utils import rc
+
+
+class MaasException(Exception):
+ """Base class for Maas' exceptions.
+
+ :ivar api_error: The HTTP code that should be returned when this error
+ is raised in the API (defaults to 500: "Internal Server Error").
+
+ """
+ api_error = rc.INTERNAL_ERROR
+
+
+class PermissionDenied(MaasException):
+ api_error = rc.FORBIDDEN
=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py 2012-02-07 18:14:14 +0000
+++ src/maasserver/middleware.py 2012-02-12 18:35:22 +0000
@@ -13,12 +13,18 @@
"AccessMiddleware",
]
+import json
import re
from django.conf import settings
+from django.core.exceptions import ValidationError
from django.core.urlresolvers import reverse
-from django.http import HttpResponseRedirect
+from django.http import (
+ HttpResponseBadRequest,
+ HttpResponseRedirect,
+ )
from django.utils.http import urlquote_plus
+from maasserver.exceptions import MaasException
class AccessMiddleware(object):
@@ -59,3 +65,33 @@
self.login_url, urlquote_plus(request.path)))
else:
return None
+
+
+class APIErrorsMiddleware(object):
+ """Convert exceptions raised in the API into a proper API error.
+
+ - Convert MaasException instances into the corresponding error.
+ - Convert ValidationError into Bad Request.
+ """
+ def __init__(self):
+ self.api_regexp = re.compile(settings.API_URL_REGEXP)
+
+ def process_exception(self, request, exception):
+ if self.api_regexp.match(request.path):
+ # The exception was raised in an API call.
+ if isinstance(exception, MaasException):
+ # The exception is a MaasException: exception.api_error
+ # will give us the proper error type.
+ response = exception.api_error
+ response.write(str(exception))
+ return response
+ if isinstance(exception, ValidationError):
+ if hasattr(exception, 'message_dict'):
+ # Complex validation error with multiple fields:
+ # return a json version of the message_dict.
+ return HttpResponseBadRequest(
+ json.dumps(exception.message_dict),
+ content_type='application/json')
+ else:
+ # Simple validation error: return the error message.
+ return HttpResponseBadRequest(str(exception))
=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py 2012-02-10 15:39:22 +0000
+++ src/maasserver/models.py 2012-02-12 18:35:22 +0000
@@ -23,11 +23,11 @@
from django.contrib import admin
from django.contrib.auth.backends import ModelBackend
from django.contrib.auth.models import User
-from django.core.exceptions import PermissionDenied
from django.core.files.base import ContentFile
from django.db import models
from django.db.models.signals import post_save
from django.shortcuts import get_object_or_404
+from maasserver.exceptions import PermissionDenied
from maasserver.macaddress import MACAddressField
from piston.models import (
Consumer,
@@ -166,14 +166,11 @@
:param user: The user that should be used in the permission check.
:type user: django.contrib.auth.models.User
:raises: django.http.Http404_,
- django.core.exceptions.PermissionDenied_.
+ maasserver.exceptions.PermissionDenied_.
.. _django.http.Http404: https://
docs.djangoproject.com/en/dev/topics/http/views/
#the-http404-exception
- .. _django.core.exceptions.PermissionDenied: https://
- docs.djangoproject.com/en/dev/ref/exceptions/
- #django.core.exceptions.PermissionDenied
"""
node = get_object_or_404(Node, system_id=system_id)
if user.has_perm('access', node):
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-02-10 17:10:25 +0000
+++ src/maasserver/tests/test_api.py 2012-02-12 18:35:22 +0000
@@ -179,8 +179,14 @@
response = self.client.put(
'/api/nodes/%s/' % node.system_id,
{'hostname': 'too long' * 100})
+ parsed_result = json.loads(response.content)
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+ self.assertEqual(
+ {u'hostname':
+ [u'Ensure this value has at most 255 characters '
+ '(it has 800).']},
+ parsed_result)
def test_PUT_refuses_to_update_invisible_node(self):
# The request to update a single node is denied if the node isn't
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-02-10 12:44:20 +0000
+++ src/maasserver/tests/test_models.py 2012-02-12 18:35:22 +0000
@@ -15,10 +15,8 @@
import shutil
from django.conf import settings
-from django.core.exceptions import (
- PermissionDenied,
- ValidationError,
- )
+from django.core.exceptions import ValidationError
+from maasserver.exceptions import PermissionDenied
from maasserver.models import (
GENERIC_CONSUMER,
MACAddress,
