#934: ApiPatch utility's error handling revamped

misago/api/patch.py

@@ -1,8 +1,13 @@
+from __future__ import unicode_literals
+
+from rest_framework.exceptions import ValidationError as ApiValidationError
 from rest_framework.response import Response
 
-from django.core.exceptions import PermissionDenied
+from django.core.exceptions import PermissionDenied, ValidationError
 from django.db import transaction
 from django.http import Http404
+from django.utils import six
+from django.utils.translation import gettext as _
 
 
 ALLOWED_OPS = ('add', 'remove', 'replace')
@@ -12,6 +17,15 @@ class InvalidAction(Exception):
     pass
 
 
+HANDLED_EXCEPTIONS = (
+    ApiValidationError,
+    ValidationError,
+    InvalidAction,
+    PermissionDenied,
+    Http404,
+)
+
+
 class ApiPatch(object):
     def __init__(self):
         self._actions = []
@@ -40,77 +54,81 @@ class ApiPatch(object):
     def dispatch(self, request, target):
         if not isinstance(request.data, list):
             return Response({
-                'detail': "PATCH request should be list of operations",
+                'detail': _("PATCH request should be list of operations."),
             }, status=400)
 
-        detail = []
-        is_errored = False
-
-        patch = {'id': target.pk}
+        response = {'id': target.pk}
         for action in request.data:
             try:
                 self.validate_action(action)
-                self.dispatch_action(patch, request, target, action)
-                detail.append('ok')
-            except Http404:
-                is_errored = True
-                detail.append('NOT FOUND')
-                break
-            except (InvalidAction, PermissionDenied) as e:
-                is_errored = True
-                detail.append(e.args[0])
-                break
-
-        patch['detail'] = detail
-        if is_errored:
-            return Response(patch, status=400)
-        else:
-            return Response(patch)
+                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 Response(response)
 
     def dispatch_bulk(self, request, targets):
-        is_errored = False
         result = []
 
-        for target in targets:
-            detail = []
+        for action in request.data['ops']:
+            try:
+                self.validate_action(action)
+            except InvalidAction as exception:
+                detail, status = self.get_error_detail_code(exception)
+                return Response({'detail': detail}, status=status)
 
-            patch = {'id': target.pk}
+        for target in targets:
+            patch = {'id': target.pk, 'status': 200}
             for action in request.data['ops']:
                 try:
-                    self.validate_action(action)
                     self.dispatch_action(patch, request, target, action)
-                except Http404:
-                    is_errored = True
-                    detail.append('NOT FOUND')
-                    break
-                except (InvalidAction, PermissionDenied) as e:
-                    is_errored = True
-                    detail.append(e.args[0])
+                except HANDLED_EXCEPTIONS as exception:
+                    detail, status = self.get_error_detail_code(exception)
+                    patch = {
+                        'id': target.pk,
+                        'detail': detail,
+                        'status': status,
+                    }
                     break
-            if detail:
-                patch['detail'] = detail
             result.append(patch)
 
-        if is_errored:
-            return Response(result, status=400)
-        else:
-            return Response(result)
+        # always returning 200 on op error saves us logic duplication
+        # in the frontend, were we need to do success handling in both
+        # success and error handles
+        return Response(result)
 
     def validate_action(self, action):
         if not action.get('op'):
-            raise InvalidAction(u"undefined op")
+            raise InvalidAction(_('"op" parameter must be defined.'))
 
         if action.get('op') not in ALLOWED_OPS:
-            raise InvalidAction(u'"%s" op is unsupported' % action.get('op'))
+            raise InvalidAction(_('"%s" op is unsupported.') % action.get('op'))
 
         if not action.get('path'):
-            raise InvalidAction(u'"%s" op has to specify path' % action.get('op'))
+            raise InvalidAction(_('"%s" op has to specify path.') % action.get('op'))
 
         if 'value' not in action:
-            raise InvalidAction(u'"%s" op has to specify value' % action.get('op'))
+            raise InvalidAction(_('"%s" op has to specify value.') % action.get('op'))
 
     def dispatch_action(self, patch, request, target, action):
         for handler in self._actions:
             if action['op'] == handler['op'] and action['path'] == handler['path']:
                 with transaction.atomic():
                     patch.update(handler['handler'](request, target, action['value']))
+
+    def get_error_detail_code(self, exception):
+        if isinstance(exception, InvalidAction):
+            return six.text_type(exception), 400
+
+        if isinstance(exception, ApiValidationError):
+            return exception.detail, 400
+
+        if isinstance(exception, ValidationError):
+            return exception.messages, 400
+
+        if isinstance(exception, PermissionDenied):
+            return six.text_type(exception), 403
+
+        if isinstance(exception, Http404):
+            return six.text_type(exception) or "NOT FOUND", 404

misago/api/tests/test_patch.py

@@ -1,15 +1,10 @@
-from django.core.exceptions import PermissionDenied
-from django.http import Http404
+from __future__ import unicode_literals
+
 from django.test import TestCase
 
 from misago.api.patch import ApiPatch, InvalidAction
 
 
-class MockRequest(object):
-    def __init__(self, data=None):
-        self.data = data
-
-
 class MockObject(object):
     def __init__(self, pk):
         self.id = pk
@@ -91,19 +86,19 @@ class ApiPatchTests(TestCase):
             try:
                 patch.validate_action(action)
             except InvalidAction as e:
-                self.assertEqual(e.args[0], u"undefined op")
+                self.assertEqual(e.args[0], '"op" parameter must be defined.')
 
         # unsupported op
         try:
             patch.validate_action({'op': 'nope'})
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"nope" op is unsupported')
+            self.assertEqual(e.args[0], u'"nope" op is unsupported.')
 
         # op lacking patch
         try:
             patch.validate_action({'op': 'add'})
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify path')
+            self.assertEqual(e.args[0], u'"add" op has to specify path.')
 
         # op lacking value
         try:
@@ -112,7 +107,7 @@ class ApiPatchTests(TestCase):
                 'path': 'yolo',
             })
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify value')
+            self.assertEqual(e.args[0], u'"add" op has to specify value.')
 
         # empty value is allowed
         try:
@@ -122,7 +117,7 @@ class ApiPatchTests(TestCase):
                 'value': '',
             })
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify value')
+            self.assertEqual(e.args[0], u'"add" op has to specify value.')
 
     def test_dispatch_action(self):
         """dispatch_action calls specified actions"""
@@ -165,159 +160,3 @@ class ApiPatchTests(TestCase):
         self.assertEqual(patch_dict['id'], 123)
         self.assertEqual(patch_dict['a'], 10)
         self.assertEqual(patch_dict['b'], 50)
-
-    def test_dispatch(self):
-        """dispatch calls actions and returns response"""
-        patch = ApiPatch()
-
-        def action_error(request, target, value):
-            if value == '404':
-                raise Http404()
-            if value == 'perm':
-                raise PermissionDenied("yo ain't doing that!")
-
-        patch.replace('error', action_error)
-
-        def action_mutate(request, target, value):
-            return {'value': value * 2}
-
-        patch.replace('mutate', action_mutate)
-
-        # dispatch requires list as an argument
-        response = patch.dispatch(MockRequest({}), {})
-        self.assertEqual(response.status_code, 400)
-
-        self.assertEqual(response.data['detail'], "PATCH request should be list of operations")
-
-        # valid dispatch
-        response = patch.dispatch(
-            MockRequest([
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
-            ]), MockObject(13)
-        )
-
-        self.assertEqual(response.status_code, 200)
-
-        self.assertEqual(len(response.data['detail']), 3)
-        self.assertEqual(response.data['detail'][0], 'ok')
-        self.assertEqual(response.data['detail'][1], 'ok')
-        self.assertEqual(response.data['detail'][2], 'ok')
-        self.assertEqual(response.data['id'], 13)
-        self.assertEqual(response.data['value'], 14)
-
-        # invalid action in dispatch
-        response = patch.dispatch(
-            MockRequest([
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
-            ]), MockObject(13)
-        )
-
-        self.assertEqual(response.status_code, 400)
-
-        self.assertEqual(len(response.data['detail']), 3)
-        self.assertEqual(response.data['detail'][0], 'ok')
-        self.assertEqual(response.data['detail'][1], 'ok')
-        self.assertEqual(response.data['detail'][2], '"replace" op has to specify path')
-        self.assertEqual(response.data['id'], 13)
-        self.assertEqual(response.data['value'], 12)
-
-        # action in dispatch raised 404
-        response = patch.dispatch(
-            MockRequest([
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'error',
-                    'value': '404',
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
-            ]), MockObject(13)
-        )
-
-        self.assertEqual(response.status_code, 400)
-
-        self.assertEqual(len(response.data['detail']), 2)
-        self.assertEqual(response.data['detail'][0], 'ok')
-        self.assertEqual(response.data['detail'][1], "NOT FOUND")
-        self.assertEqual(response.data['id'], 13)
-        self.assertEqual(response.data['value'], 4)
-
-        # action in dispatch raised perm denied
-        response = patch.dispatch(
-            MockRequest([
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 9,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'error',
-                    'value': 'perm',
-                },
-            ]), MockObject(13)
-        )
-
-        self.assertEqual(response.status_code, 400)
-
-        self.assertEqual(len(response.data['detail']), 4)
-        self.assertEqual(response.data['detail'][0], 'ok')
-        self.assertEqual(response.data['detail'][1], 'ok')
-        self.assertEqual(response.data['detail'][2], 'ok')
-        self.assertEqual(response.data['detail'][3], "yo ain't doing that!")
-        self.assertEqual(response.data['id'], 13)
-        self.assertEqual(response.data['value'], 18)

misago/api/tests/test_patch_dispatch.py

@@ -0,0 +1,246 @@
+from __future__ import unicode_literals
+
+from rest_framework.exceptions import ValidationError as ApiValidationError
+
+from django.core.exceptions import PermissionDenied, ValidationError
+from django.http import Http404
+from django.test import TestCase
+
+from misago.api.patch import ApiPatch
+
+
+class MockRequest(object):
+    def __init__(self, data=None):
+        self.data = data
+
+
+class MockObject(object):
+    def __init__(self, pk):
+        self.id = pk
+        self.pk = pk
+
+
+class ApiPatchDispatchTests(TestCase):
+    def test_dispatch(self):
+        """dispatch calls actions and returns response"""
+        patch = ApiPatch()
+
+        def action_error(request, target, value):
+            if value == '404':
+                raise Http404()
+            if value == '404_reason':
+                raise Http404("something was removed")
+            if value == 'perm':
+                raise PermissionDenied("yo ain't doing that!")
+            if value == 'invalid':
+                raise ValidationError("invalid data here!")
+            if value == 'api_invalid':
+                raise ApiValidationError("invalid api data here!")
+
+        patch.replace('error', action_error)
+
+        def action_mutate(request, target, value):
+            return {'value': value * 2}
+
+        patch.replace('mutate', action_mutate)
+
+        # dispatch requires list as an argument
+        response = patch.dispatch(MockRequest({}), {})
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data['detail'], "PATCH request should be list of operations.")
+
+        # valid dispatch
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, {'value': 14, 'id': 13})
+
+        # invalid action in dispatch
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data['detail'], '"replace" op has to specify path.')
+
+        # op raised validation error
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'invalid',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data['detail'], ["invalid data here!"])
+
+        # op raised api validation error
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'api_invalid',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data['detail'], ["invalid api data here!"])
+
+        # action in dispatch raised perm denied
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 9,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'perm',
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 403)
+        self.assertEqual(response.data['detail'], "yo ain't doing that!")
+
+        # action in dispatch raised 404
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': '404',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.data['detail'], "NOT FOUND")
+
+        # action in dispatch raised 404 with message
+        response = patch.dispatch(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': '404_reason',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 404)
+        self.assertEqual(response.data['detail'], "something was removed")

misago/api/tests/test_patch_dispatch_bulk.py

@@ -0,0 +1,266 @@
+from __future__ import unicode_literals
+
+from rest_framework.exceptions import ValidationError as ApiValidationError
+
+from django.core.exceptions import PermissionDenied, ValidationError
+from django.http import Http404
+from django.test import TestCase
+
+from misago.api.patch import ApiPatch, InvalidAction
+
+
+class MockRequest(object):
+    def __init__(self, ops):
+        self.data = {'ops': ops}
+
+
+class MockObject(object):
+    def __init__(self, pk):
+        self.id = pk
+        self.pk = pk
+
+
+class ApiPatchDispatchBulkTests(TestCase):
+    def test_dispatch_bulk(self):
+        """dispatch_bulk calls actions and returns response"""
+        patch = ApiPatch()
+
+        def action_error(request, target, value):
+            if value == '404':
+                raise Http404()
+            if value == '404_reason':
+                raise Http404("something was removed")
+            if value == 'perm':
+                raise PermissionDenied("yo ain't doing that!")
+            if value == 'invalid':
+                raise ValidationError("invalid data here!")
+            if value == 'api_invalid':
+                raise ApiValidationError("invalid api data here!")
+
+        patch.replace('error', action_error)
+
+        def action_mutate(request, target, value):
+            return {'value': value * 2}
+
+        patch.replace('mutate', action_mutate)
+
+        # valid bulk dispatch
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'value': 14, 'status': 200},
+            {'id': 7, 'value': 14, 'status': 200},
+        ])
+
+        # invalid action in bulk dispatch
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data['detail'], '"replace" op has to specify path.')
+
+        # op raised validation error
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'invalid',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'detail': ["invalid data here!"], 'status': 400},
+            {'id': 7, 'detail': ["invalid data here!"], 'status': 400},
+        ])
+
+        # op raised api validation error
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'api_invalid',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'detail': ["invalid api data here!"], 'status': 400},
+            {'id': 7, 'detail': ["invalid api data here!"], 'status': 400},
+        ])
+
+        # action in bulk dispatch raised perm denied
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 9,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': 'perm',
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'detail': "yo ain't doing that!", 'status': 403},
+            {'id': 7, 'detail': "yo ain't doing that!", 'status': 403},
+        ])
+
+        # action in bulk dispatch raised 404
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': '404',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'detail': "NOT FOUND", 'status': 404},
+            {'id': 7, 'detail': "NOT FOUND", 'status': 404},
+        ])
+
+        # action in bulk dispatch raised 404 with message
+        response = patch.dispatch_bulk(
+            MockRequest([
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 2,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'error',
+                    'value': '404_reason',
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
+                },
+                {
+                    'op': 'replace',
+                    'path': 'mutate',
+                    'value': 7,
+                },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, [
+            {'id': 5, 'detail': "something was removed", 'status': 404},
+            {'id': 7, 'detail': "something was removed", 'status': 404},
+        ])