Merge lp:~gmb/maas/fix-bug-1394746 into lp:~maas-committers/maas/trunk

[Graham Binns]

Add a MAASAPIValidationError, which is a nice way of putting Django validation errors into something that behaves in a MAASException-like way.

All the callsites that used ValidationError in maasserver.api.* have been updated to use the new exception class.

[Gavin Panella (allenap)]

Looks good, but make_http_response() needs a test or two.

review: Approve

[Graham Binns (gmb)]

On 27 November 2014 at 11:17, Gavin Panella <email address hidden>
wrote:

> Looks good, but make_http_response() needs a test or two.
>

Aw, darn, really? (Yes)

=== modified file 'src/maasserver/api/boot_resources.py'
--- src/maasserver/api/boot_resources.py	2014-09-04 20:01:15 +0000
+++ src/maasserver/api/boot_resources.py	2014-11-27 12:10:49 +0000
@@ -21,7 +21,6 @@
 import httplib
 import os
 
-from django.core.exceptions import ValidationError
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
@@ -40,6 +39,7 @@
 from maasserver.exceptions import (
     MAASAPIBadRequest,
     MAASAPIForbidden,
+    MAASAPIValidationError,
     )
 from maasserver.forms import (
     BootResourceForm,
@@ -205,7 +205,7 @@
         else:
             form = BootResourceNoContentForm(data=data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         resource = form.save()
 
         # If an upload contained the full file, then we can have the clusters

=== modified file 'src/maasserver/api/boot_source_selections.py'
--- src/maasserver/api/boot_source_selections.py	2014-10-23 15:26:37 +0000
+++ src/maasserver/api/boot_source_selections.py	2014-11-27 12:10:49 +0000
@@ -18,9 +18,9 @@
     ]
 
 
-from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import OperationsHandler
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import BootSourceSelectionForm
 from maasserver.models import (
     BootSource,
@@ -72,7 +72,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, boot_source_id, id):
         """Delete a specific boot source."""
@@ -166,7 +166,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
 
 class BootSourceSelectionsBackwardHandler(BootSourceSelectionsHandler):

=== modified file 'src/maasserver/api/boot_sources.py'
--- src/maasserver/api/boot_sources.py	2014-09-04 14:51:02 +0000
+++ src/maasserver/api/boot_sources.py	2014-11-27 12:10:49 +0000
@@ -20,11 +20,11 @@
 from base64 import b64encode
 import httplib
 
-from django.core.exceptions import ValidationError
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import OperationsHandler
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import BootSourceForm
 from maasserver.models import BootSource
 from piston.emitters import JSONEmitter
@@ -94,7 +94,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, id):
         """Delete a specific boot source."""
@@ -176,7 +176,7 @@
                 stream, mimetype='application/json; charset=utf-8',
                 status=httplib.CREATED)
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
 
 class BootSourcesBackwardHandler(BootSourcesHandler):

=== modified file 'src/maasserver/api/license_keys.py'
--- src/maasserver/api/license_keys.py	2014-08-16 16:47:28 +0000
+++ src/maasserver/api/license_keys.py	2014-11-27 12:10:49 +0000
@@ -18,9 +18,9 @@
     ]
 
 
-from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import OperationsHandler
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import LicenseKeyForm
 from maasserver.models import LicenseKey
 from maasserver.utils.orm import get_one
@@ -52,7 +52,7 @@
             data = {}
         form = LicenseKeyForm(data=data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.save()
 
     @classmethod
@@ -89,7 +89,7 @@
             data = {}
         form = LicenseKeyForm(instance=license_key, data=data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.save()
 
     def delete(self, request, osystem, distro_series):

=== modified file 'src/maasserver/api/maas.py'
--- src/maasserver/api/maas.py	2014-08-18 12:39:48 +0000
+++ src/maasserver/api/maas.py	2014-11-27 12:10:49 +0000
@@ -17,7 +17,6 @@
     ]
 
 
-from django.core.exceptions import ValidationError
 from django.http import HttpResponse
 from formencode import validators
 from maasserver.api.support import (
@@ -25,6 +24,7 @@
     OperationsHandler,
     )
 from maasserver.api.utils import get_mandatory_param
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms_settings import (
     get_config_doc,
     get_config_form,
@@ -56,7 +56,7 @@
         value = get_mandatory_param(request.data, 'value')
         form = get_config_form(name, {name: value})
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         form.save()
         return rc.ALL_OK
 

=== modified file 'src/maasserver/api/networks.py'
--- src/maasserver/api/networks.py	2014-09-17 03:14:08 +0000
+++ src/maasserver/api/networks.py	2014-11-27 12:10:49 +0000
@@ -18,7 +18,6 @@
     ]
 
 
-from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     admin_method,
@@ -26,6 +25,7 @@
     OperationsHandler,
     )
 from maasserver.enum import NODE_PERMISSION
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import (
     NetworkConnectMACsForm,
     NetworkDisconnectMACsForm,
@@ -77,7 +77,7 @@
             instance=network, data=request.data,
             delete_macs_if_not_present=False)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.save()
 
     @admin_method
@@ -107,7 +107,7 @@
         network = get_object_or_404(Network, name=name)
         form = NetworkConnectMACsForm(network=network, data=request.data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         form.save()
 
     @admin_method
@@ -123,7 +123,7 @@
         network = get_object_or_404(Network, name=name)
         form = NetworkDisconnectMACsForm(network=network, data=request.data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         form.save()
 
     @operation(idempotent=True)
@@ -165,7 +165,7 @@
         """
         form = NetworksListingForm(data=request.GET)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.filter_networks(Network.objects.all())
 
     @admin_method
@@ -186,7 +186,7 @@
         """
         form = NetworkForm(request.data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.save()
 
     @classmethod

=== modified file 'src/maasserver/api/node_group_interfaces.py'
--- src/maasserver/api/node_group_interfaces.py	2014-11-24 18:23:26 +0000
+++ src/maasserver/api/node_group_interfaces.py	2014-11-27 12:10:49 +0000
@@ -18,13 +18,13 @@
     ]
 
 
-from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.node_groups import check_nodegroup_access
 from maasserver.api.support import (
     operation,
     OperationsHandler,
     )
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import NodeGroupInterfaceForm
 from maasserver.models import (
     NodeGroup,
@@ -107,7 +107,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls, nodegroup=None):
@@ -189,7 +189,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, uuid, name):
         """Delete a specific NodeGroupInterface.

=== modified file 'src/maasserver/api/node_groups.py'
--- src/maasserver/api/node_groups.py	2014-11-24 17:55:55 +0000
+++ src/maasserver/api/node_groups.py	2014-11-27 12:10:49 +0000
@@ -21,10 +21,7 @@
 import httplib
 
 import bson
-from django.core.exceptions import (
-    PermissionDenied,
-    ValidationError,
-    )
+from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from formencode import validators
@@ -43,7 +40,10 @@
 from maasserver.clusterrpc.power_parameters import (
     get_all_power_types_from_clusters,
     )
-from maasserver.exceptions import Unauthorized
+from maasserver.exceptions import (
+    MAASAPIValidationError,
+    Unauthorized,
+    )
 from maasserver.forms import (
     DownloadProgressForm,
     NodeGroupEdit,
@@ -212,7 +212,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @admin_method
     @operation(idempotent=False)
@@ -356,7 +356,7 @@
 
         form = DownloadProgressForm(data=request.data, instance=download)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         form.save()
 
         return HttpResponse(status=httplib.OK)

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2014-11-24 18:23:26 +0000
+++ src/maasserver/api/nodes.py	2014-11-27 12:10:49 +0000
@@ -22,10 +22,7 @@
 import bson
 import crochet
 from django.conf import settings
-from django.core.exceptions import (
-    PermissionDenied,
-    ValidationError,
-    )
+from django.core.exceptions import PermissionDenied
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from maasserver import locks
@@ -50,6 +47,7 @@
     )
 from maasserver.exceptions import (
     MAASAPIBadRequest,
+    MAASAPIValidationError,
     NodesNotAvailable,
     NodeStateViolation,
     PowerProblem,
@@ -246,7 +244,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, system_id):
         """Delete a specific Node.
@@ -341,7 +339,7 @@
             if form.is_valid():
                 form.save()
             else:
-                raise ValidationError(form.errors)
+                raise MAASAPIValidationError(form.errors)
 
         try:
             node.start(request.user, user_data=user_data)
@@ -396,7 +394,7 @@
             node = form.save(allow_redirect=False)
             return node
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @operation(idempotent=True)
     def details(self, request, system_id):
@@ -622,7 +620,7 @@
     if given_arch and '/' in given_arch:
         if given_subarch:
             # Architecture with a '/' and a subarchitecture: error.
-            raise ValidationError('Subarchitecture cannot be specified twice.')
+            raise MAASAPIValidationError('Subarchitecture cannot be specified twice.')
         # Architecture with a '/' in it: use normally.
     elif given_arch:
         if given_subarch:
@@ -644,7 +642,7 @@
             # We insist on this to protect command-line API users who
             # are manually enlisting nodes.  You can't use the origin's
             # IP address to indicate in which nodegroup the new node belongs.
-            raise ValidationError(
+            raise MAASAPIValidationError(
                 "'autodetect_nodegroup' must be specified if 'nodegroup' "
                 "parameter missing")
         nodegroup = find_nodegroup(request)
@@ -660,7 +658,7 @@
         maaslog.info("%s: Enlisted new node", node.hostname)
         return node
     else:
-        raise ValidationError(form.errors)
+        raise MAASAPIValidationError(form.errors)
 
 
 class AnonNodesHandler(AnonymousOperationsHandler):
@@ -940,7 +938,7 @@
             invalid_macs = [
                 mac for mac in match_macs if MAC_RE.match(mac) is None]
             if len(invalid_macs) != 0:
-                raise ValidationError(
+                raise MAASAPIValidationError(
                     "Invalid MAC address(es): %s" % ", ".join(invalid_macs))
 
         # Fetch nodes and apply filters.
@@ -1040,7 +1038,7 @@
             request.user.username, request.data)
 
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
         # This lock prevents a node we've picked as available from
         # becoming unavailable before our transaction commits.
@@ -1085,7 +1083,7 @@
         }
         form = BulkNodeActionForm(request.user, data=data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         form.save()
 
     @admin_method

=== modified file 'src/maasserver/api/ssh_keys.py'
--- src/maasserver/api/ssh_keys.py	2014-11-20 23:47:49 +0000
+++ src/maasserver/api/ssh_keys.py	2014-11-27 12:10:49 +0000
@@ -19,13 +19,13 @@
 
 import httplib
 
-from django.core.exceptions import ValidationError
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     operation,
     OperationsHandler,
     )
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import SSHKeyForm
 from maasserver.models import SSHKey
 from piston.emitters import JSONEmitter
@@ -64,7 +64,7 @@
                 stream, mimetype='application/json; charset=utf-8',
                 status=httplib.CREATED)
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls, *args, **kwargs):

=== modified file 'src/maasserver/api/ssl_keys.py'
--- src/maasserver/api/ssl_keys.py	2014-11-21 21:23:39 +0000
+++ src/maasserver/api/ssl_keys.py	2014-11-27 12:10:49 +0000
@@ -19,13 +19,13 @@
 
 import httplib
 
-from django.core.exceptions import ValidationError
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     operation,
     OperationsHandler,
     )
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import SSLKeyForm
 from maasserver.models import SSLKey
 from piston.emitters import JSONEmitter
@@ -64,7 +64,7 @@
                 stream, mimetype='application/json; charset=utf-8',
                 status=httplib.CREATED)
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls, *args, **kwargs):

=== modified file 'src/maasserver/api/tags.py'
--- src/maasserver/api/tags.py	2014-11-21 21:23:39 +0000
+++ src/maasserver/api/tags.py	2014-11-27 12:10:49 +0000
@@ -20,10 +20,7 @@
 
 import httplib
 
-from django.core.exceptions import (
-    PermissionDenied,
-    ValidationError,
-    )
+from django.core.exceptions import PermissionDenied
 from django.db.utils import DatabaseError
 from django.http import HttpResponse
 from maasserver.api.node_groups import check_nodegroup_access
@@ -33,6 +30,7 @@
     )
 from maasserver.api.utils import get_list_from_dict_or_multidict
 from maasserver.enum import NODE_PERMISSION
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import TagForm
 from maasserver.models import (
     Node,
@@ -89,10 +87,10 @@
                 new_tag.save()
                 form.save_m2m()
             except DatabaseError as e:
-                raise ValidationError(e)
+                raise MAASAPIValidationError(e)
             return new_tag
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, name):
         """Delete a specific Tag.
@@ -223,7 +221,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @operation(idempotent=True)
     def list(self, request):

=== modified file 'src/maasserver/api/utils.py'
--- src/maasserver/api/utils.py	2014-08-16 05:43:33 +0000
+++ src/maasserver/api/utils.py	2014-11-27 12:10:49 +0000
@@ -23,10 +23,12 @@
     'get_overridden_query_dict',
     ]
 
-from django.core.exceptions import ValidationError
 from django.http import QueryDict
 from formencode.validators import Invalid
-from maasserver.exceptions import Unauthorized
+from maasserver.exceptions import (
+    MAASAPIValidationError,
+    Unauthorized,
+    )
 from piston.models import Token
 
 
@@ -69,12 +71,12 @@
     """
     value = data.get(key, None)
     if value is None:
-        raise ValidationError("No provided %s!" % key)
+        raise MAASAPIValidationError("No provided %s!" % key)
     if validator is not None:
         try:
             return validator.to_python(value)
         except Invalid as e:
-            raise ValidationError("Invalid %s: %s" % (key, e.msg))
+            raise MAASAPIValidationError("Invalid %s: %s" % (key, e.msg))
     else:
         return value
 
@@ -103,7 +105,7 @@
         try:
             return validator.to_python(value)
         except Invalid as e:
-            raise ValidationError("Invalid %s: %s" % (key, e.msg))
+            raise MAASAPIValidationError("Invalid %s: %s" % (key, e.msg))
     else:
         return value
 

=== modified file 'src/maasserver/api/zones.py'
--- src/maasserver/api/zones.py	2014-11-24 17:55:55 +0000
+++ src/maasserver/api/zones.py	2014-11-27 12:10:49 +0000
@@ -17,12 +17,12 @@
     'ZonesHandler',
     ]
 
-from django.core.exceptions import ValidationError
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import (
     admin_method,
     OperationsHandler,
     )
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import ZoneForm
 from maasserver.models import Zone
 from maasserver.utils.orm import get_one
@@ -63,7 +63,7 @@
         zone = get_object_or_404(Zone, name=name)
         form = ZoneForm(instance=zone, data=request.data)
         if not form.is_valid():
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
         return form.save()
 
     @admin_method
@@ -110,7 +110,7 @@
         if form.is_valid():
             return form.save()
         else:
-            raise ValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     def read(self, request):
         """List zones.

=== modified file 'src/maasserver/exceptions.py'
--- src/maasserver/exceptions.py	2014-11-25 10:18:03 +0000
+++ src/maasserver/exceptions.py	2014-11-27 12:10:49 +0000
@@ -31,10 +31,12 @@
 
 import httplib
 
+from django.core.exceptions import ValidationError
 from django.http import (
     HttpResponse,
     HttpResponseRedirect,
     )
+import simplejson as json
 
 
 class MAASException(Exception):
@@ -78,6 +80,26 @@
     api_error = httplib.FORBIDDEN
 
 
+class MAASAPIValidationError(MAASAPIBadRequest, ValidationError):
+    """A validation error raised during a MAAS API request."""
+
+    def make_http_response(self):
+        """Create an :class:`HttpResponse` representing this exception."""
+        mimetype = b"application/json"
+        if hasattr(self, 'error_dict'):
+            messages = json.dumps(self.message_dict)
+        elif len(self.messages) == 1:
+            messages = self.messages[0]
+            mimetype = b"text/plain"
+        else:
+            messages = json.dumps(self.messages)
+
+        encoding = b'utf-8'
+        return HttpResponse(
+            status=self.api_error, content=messages,
+            mimetype=b"%s; charset=%s" % (mimetype, encoding))
+
+
 class Unauthorized(MAASAPIException):
     """HTTP error 401: Unauthorized.  Login required."""
     api_error = httplib.UNAUTHORIZED

=== modified file 'src/maasserver/tests/test_exceptions.py'
--- src/maasserver/tests/test_exceptions.py	2014-07-18 17:05:57 +0000
+++ src/maasserver/tests/test_exceptions.py	2014-11-27 12:10:49 +0000
@@ -18,11 +18,14 @@
 
 from maasserver.exceptions import (
     MAASAPIBadRequest,
+    MAASAPIValidationError,
     Redirect,
     )
 from maasserver.testing import extract_redirect
 from maastesting.factory import factory
 from maastesting.testcase import MAASTestCase
+import simplejson as json
+from testtools.matchers import Equals
 
 
 class TestExceptions(MAASTestCase):
@@ -40,3 +43,57 @@
         exception = Redirect(target)
         response = exception.make_http_response()
         self.assertEqual(target, extract_redirect(response))
+
+
+class TestMAASAPIValidationError(MAASTestCase):
+    """Tests for the `MAASAPIValidationError` exception class."""
+
+    def test_returns_http_response(self):
+        error = factory.make_string()
+        exception = MAASAPIValidationError(error)
+        response = exception.make_http_response()
+        self.assertEqual(
+            (httplib.BAD_REQUEST, error),
+            (response.status_code, response.content))
+
+    def test_returns_textual_response_if_message_is_a_string(self):
+        error = factory.make_string()
+        exception = MAASAPIValidationError(error)
+        response = exception.make_http_response()
+        self.assertEqual(
+            "text/plain; charset=utf-8", response.get("Content-Type"))
+
+    def test_returns_json_response_if_message_is_a_list(self):
+        errors = [
+            factory.make_string(),
+            factory.make_string(),
+            ]
+        exception = MAASAPIValidationError(errors)
+        response = exception.make_http_response()
+        self.expectThat(
+            response.get("Content-Type"),
+            Equals("application/json; charset=utf-8"))
+        self.expectThat(response.content, Equals(json.dumps(errors)))
+
+    def test_if_message_is_single_item_list_returns_only_first_message(self):
+        errors = [
+            factory.make_string(),
+            ]
+        exception = MAASAPIValidationError(errors)
+        response = exception.make_http_response()
+        self.expectThat(
+            response.get("Content-Type"),
+            Equals("text/plain; charset=utf-8"))
+        self.expectThat(response.content, Equals(errors[0]))
+
+    def test_returns_json_response_if_message_is_a_dict(self):
+        errors = {
+            'error_1': [factory.make_string()],
+            'error_2': [factory.make_string()],
+            }
+        exception = MAASAPIValidationError(errors)
+        response = exception.make_http_response()
+        self.expectThat(
+            response.get("Content-Type"),
+            Equals("application/json; charset=utf-8"))
+        self.expectThat(response.content, Equals(json.dumps(errors)))