Return validation errors from patch actions same way serializers and forms do

misago/api/patch.py

@@ -1,7 +1,8 @@
 from __future__ import unicode_literals
 
-from rest_framework.exceptions import ValidationError as ApiValidationError
+from rest_framework import serializers
 from rest_framework.response import Response
+from rest_framework.settings import api_settings
 
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.db import transaction
@@ -18,7 +19,7 @@ class InvalidAction(Exception):
 
 
 HANDLED_EXCEPTIONS = (
-    ApiValidationError,
+    serializers.ValidationError,
     ValidationError,
     InvalidAction,
     PermissionDenied,
@@ -56,11 +57,14 @@ class ApiPatch(object):
 
         try:
             self.validate_actions(request.data)
-            for action in request.data:
-                self.dispatch_action(response, request, target, action)
         except HANDLED_EXCEPTIONS as exception:
-            detail, status = self.get_error_detail_code(exception)
-            return Response({'detail': detail}, status=status)
+            return self.handle_exception(exception)
+
+        for action in request.data:
+            try:
+                self.dispatch_action(response, request, target, action)
+            except HANDLED_EXCEPTIONS as exception:
+                return self.handle_exception(exception, path=action['path'])
 
         return Response(response)
 
@@ -77,12 +81,12 @@ class ApiPatch(object):
                 try:
                     self.dispatch_action(data['patch'], request, target, action)
                 except HANDLED_EXCEPTIONS as exception:
-                    detail, status = self.get_error_detail_code(exception)
-                    data = {
-                        'id': target.pk,
-                        'status': status,
-                        'detail': detail,
-                    }
+                    errors, status = self.get_error_data_status(exception, path=action['path'])
+                    data = {'id': target.pk, 'status': status, }
+                    if status == 400:
+                        data['invalid'] = errors
+                    else:
+                        data.update(errors)
                     break
             result.append(data)
 
@@ -100,7 +104,7 @@ class ApiPatch(object):
 
     def validate_actions(self, actions):
         if not isinstance(actions, list):
-            raise ApiValidationError(_("PATCH request should be a list of operations."))
+            raise InvalidAction(_("PATCH request should be a list of operations."))
 
         reduced_actions = []
         for action in actions:
@@ -134,22 +138,27 @@ class ApiPatch(object):
                 with transaction.atomic():
                     data.update(handler['handler'](request, target, action['value']))
 
-    def handle_exception(self, exception):
-        detail, status = self.get_error_detail_code(exception)
-        return Response({'detail': detail}, status=status)
+    def handle_exception(self, exception, path=None):
+        data, status = self.get_error_data_status(exception, path)
+        return Response(data, status=status)
+
+    def get_error_data_status(self, exception, path=None):
+        if path:
+            # if path is set, we swap comma for underscore
+            # this makes it easier to process errors in frontend
+            path = path.replace('-', '_')
 
-    def get_error_detail_code(self, exception):
         if isinstance(exception, InvalidAction):
-            return six.text_type(exception), 400
+            return {api_settings.NON_FIELD_ERRORS_KEY: [six.text_type(exception)]}, 400
 
-        if isinstance(exception, ApiValidationError):
-            return exception.detail, 400
+        if isinstance(exception, serializers.ValidationError):
+            return {path: exception.detail}, 400
 
         if isinstance(exception, ValidationError):
-            return exception.messages, 400
+            return {path: exception.messages}, 400
 
         if isinstance(exception, PermissionDenied):
-            return six.text_type(exception), 403
+            return {'detail': six.text_type(exception)}, 403
 
         if isinstance(exception, Http404):
-            return 'NOT FOUND', 404
+            return {'detail': 'NOT FOUND'}, 404

misago/api/tests/test_patch_dispatch.py

@@ -39,6 +39,14 @@ class ApiPatchDispatchTests(TestCase):
 
         patch.replace('error', action_error)
 
+        def action_custom_path_error(request, target, value):
+            if value == 'invalid':
+                raise ValidationError("invalid data here!")
+            if value == 'api_invalid':
+                raise ApiValidationError("invalid api data here!")
+
+        patch.replace('path-error', action_custom_path_error)
+
         def action_mutate(request, target, value):
             return {'value': value * 2}
 
@@ -48,7 +56,7 @@ class ApiPatchDispatchTests(TestCase):
         response = patch.dispatch(MockRequest({}), MockObject(13))
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': ["PATCH request should be a list of operations."],
+            'non_field_errors': ["PATCH request should be a list of operations."],
         })
 
         # valid dispatch
@@ -81,7 +89,7 @@ class ApiPatchDispatchTests(TestCase):
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': '"replace" op has to specify path.',
+            'non_field_errors': ['"replace" op has to specify path.'],
         })
 
         # repeated action in dispatch
@@ -102,7 +110,7 @@ class ApiPatchDispatchTests(TestCase):
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': '"replace" op for "mutate" path is repeated.',
+            'non_field_errors': ['"replace" op for "mutate" path is repeated.'],
         })
 
         # op raised validation error
@@ -122,7 +130,26 @@ class ApiPatchDispatchTests(TestCase):
         )
 
         self.assertEqual(response.status_code, 400)
-        self.assertEqual(response.data, {'detail': ["invalid data here!"]})
+        self.assertEqual(response.data, {'error': ["invalid data here!"]})
+
+        # op raised validation error in custom path
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'path-error',
+                    'value': 'invalid',
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {'path_error': ["invalid data here!"]})
 
         # op raised api validation error
         response = patch.dispatch(
@@ -141,7 +168,26 @@ class ApiPatchDispatchTests(TestCase):
         )
 
         self.assertEqual(response.status_code, 400)
-        self.assertEqual(response.data, {'detail': ["invalid api data here!"]})
+        self.assertEqual(response.data, {'error': ["invalid api data here!"]})
+
+        # op raised api validation error in custom path
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'path-error',
+                    'value': 'api_invalid',
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {'path_error': ["invalid api data here!"]})
 
         # action in dispatch raised perm denied
         response = patch.dispatch(

misago/api/tests/test_patch_dispatch_bulk.py

@@ -39,6 +39,14 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         patch.replace('error', action_error)
 
+        def action_custom_path_error(request, target, value):
+            if value == 'invalid':
+                raise ValidationError("invalid data here!")
+            if value == 'api_invalid':
+                raise ApiValidationError("invalid api data here!")
+
+        patch.replace('path-error', action_custom_path_error)
+
         def action_mutate(request, target, value):
             return {'value': value * 2}
 
@@ -66,7 +74,7 @@ class ApiPatchDispatchBulkTests(TestCase):
         response = patch.dispatch_bulk(MockRequest({}), [MockObject(5), MockObject(7)])
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': ["PATCH request should be a list of operations."],
+            'non_field_errors': ["PATCH request should be a list of operations."],
         })
 
         # invalid action in bulk dispatch
@@ -86,7 +94,7 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': '"replace" op has to specify path.',
+            'non_field_errors': ['"replace" op has to specify path.'],
         })
 
         # repeated action in dispatch
@@ -108,7 +116,7 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': '"replace" op for "mutate" path is repeated.',
+            'non_field_errors': ['"replace" op for "mutate" path is repeated.'],
         })
 
         # op raised validation error
@@ -130,8 +138,31 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data, [
-            {'id': '5', 'status': '400', 'detail': ["invalid data here!"]},
-            {'id': '7', 'status': '400', 'detail': ["invalid data here!"]},
+            {'id': '5', 'status': '400', 'invalid': {'error': ["invalid data here!"]}},
+            {'id': '7', 'status': '400', 'invalid': {'error': ["invalid data here!"]}},
+        ])
+
+        # op raised api validation error dict in custom path
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'path-error',
+                    'value': 'api_invalid',
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': '5', 'status': '400', 'invalid': {'path_error': ["invalid api data here!"]}},
+            {'id': '7', 'status': '400', 'invalid': {'path_error': ["invalid api data here!"]}},
         ])
 
         # op raised api validation error
@@ -153,8 +184,31 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data, [
-            {'id': '5', 'status': '400', 'detail': ["invalid api data here!"]},
-            {'id': '7', 'status': '400', 'detail': ["invalid api data here!"]},
+            {'id': '5', 'status': '400', 'invalid': {'error': ["invalid api data here!"]}},
+            {'id': '7', 'status': '400', 'invalid': {'error': ["invalid api data here!"]}},
+        ])
+
+        # op raised api validation error in custom path
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'path-error',
+                    'value': 'api_invalid',
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': '5', 'status': '400', 'invalid': {'path_error': ["invalid api data here!"]}},
+            {'id': '7', 'status': '400', 'invalid': {'path_error': ["invalid api data here!"]}},
         ])
 
         # action in bulk dispatch raised perm denied