First pass on api.patch improv: don't allow repeated actions.

misago/api/patch.py

@@ -52,32 +52,25 @@ class ApiPatch(object):
         })
 
     def dispatch(self, request, target):
-        if not isinstance(request.data, list):
-            return Response({
-                'detail': _("PATCH request should be a list of operations."),
-            }, status=400)
-
         response = {'id': target.pk}
-        for action in request.data:
-            try:
-                self.validate_action(action)
+
+        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)
+        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):
-        result = []
-
-        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)
+        try:
+            self.validate_actions(request.data['ops'])
+        except HANDLED_EXCEPTIONS as exception:
+            return self.handle_exception(exception)
 
+        result = []
         for target in targets:
             data = {'id': target.pk, 'status': 200, 'patch': {}}
             for action in request.data['ops']:
@@ -105,6 +98,20 @@ class ApiPatch(object):
         # success and error handles
         return Response(result)
 
+    def validate_actions(self, actions):
+        if not isinstance(actions, list):
+            raise ApiValidationError(_("PATCH request should be a list of operations."))
+
+        reduced_actions = []
+        for action in actions:
+            self.validate_action(action)
+
+            reduced_action = self.reduce_action(action)
+            if reduced_action in reduced_actions:
+                raise InvalidAction(
+                    _('"%(op)s" op for "%(path)s" path is repeated.') % reduced_action)
+            reduced_actions.append(reduced_action)
+
     def validate_action(self, action):
         if not action.get('op'):
             raise InvalidAction(_('"op" parameter must be defined.'))
@@ -118,12 +125,19 @@ class ApiPatch(object):
         if 'value' not in action:
             raise InvalidAction(_('"%s" op has to specify value.') % action.get('op'))
 
+    def reduce_action(self, action):
+        return {'op': action['op'], 'path': action['path']}
+
     def dispatch_action(self, data, request, target, action):
         for handler in self._actions:
             if action['op'] == handler['op'] and action['path'] == handler['path']:
                 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 get_error_detail_code(self, exception):
         if isinstance(exception, InvalidAction):
             return six.text_type(exception), 400

misago/api/tests/test_patch.py

@@ -54,8 +54,8 @@ class ApiPatchTests(TestCase):
         self.assertEqual(patch._actions[0]['path'], 'test-replace')
         self.assertEqual(patch._actions[0]['handler'], mock_function)
 
-    def test_validate_action(self):
-        """validate_action method validates action dict"""
+    def test_validate_actions(self):
+        """validate_actions method validates action dict"""
         patch = ApiPatch()
 
         VALID_ACTIONS = [
@@ -77,47 +77,56 @@ class ApiPatchTests(TestCase):
         ]
 
         for action in VALID_ACTIONS:
-            patch.validate_action(action)
+            patch.validate_actions([action])
 
         # undefined op
         UNSUPPORTED_ACTIONS = ({}, {'op': ''}, {'no': 'op'}, )
 
         for action in UNSUPPORTED_ACTIONS:
             try:
-                patch.validate_action(action)
+                patch.validate_actions([action])
             except InvalidAction as e:
                 self.assertEqual(e.args[0], '"op" parameter must be defined.')
 
         # unsupported op
         try:
-            patch.validate_action({'op': 'nope'})
+            patch.validate_actions([{'op': 'nope'}])
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"nope" op is unsupported.')
+            self.assertEqual(e.args[0], '"nope" op is unsupported.')
 
         # op lacking patch
         try:
-            patch.validate_action({'op': 'add'})
+            patch.validate_actions([{'op': 'add'}])
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify path.')
+            self.assertEqual(e.args[0], '"add" op has to specify path.')
 
         # op lacking value
         try:
-            patch.validate_action({
+            patch.validate_actions([{
                 'op': 'add',
                 'path': 'yolo',
-            })
+            }])
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify value.')
+            self.assertEqual(e.args[0], '"add" op has to specify value.')
 
-        # empty value is allowed
+        # empty value is forbidden
         try:
-            patch.validate_action({
+            patch.validate_actions([{
                 'op': 'add',
                 'path': 'yolo',
                 'value': '',
-            })
+            }])
         except InvalidAction as e:
-            self.assertEqual(e.args[0], u'"add" op has to specify value.')
+            self.assertEqual(e.args[0], '"add" op has to specify value.')
+
+        # duplicated actions are forbidden
+        try:
+            patch.validate_actions([
+                {'op': 'add', 'path': 'like', 'value': True},
+                {'op': 'add', 'path': 'like', 'value': False},
+            ])
+        except InvalidAction as e:
+            self.assertEqual(e.args[0], '"add" op for "like" path is repeated.')
 
     def test_dispatch_action(self):
         """dispatch_action calls specified actions"""

misago/api/tests/test_patch_dispatch.py

@@ -45,9 +45,11 @@ class ApiPatchDispatchTests(TestCase):
         patch.replace('mutate', action_mutate)
 
         # dispatch requires list as an argument
-        response = patch.dispatch(MockRequest({}), {})
+        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."})
+        self.assertEqual(response.data, {
+            'detail': ["PATCH request should be a list of operations."],
+        })
 
         # valid dispatch
         response = patch.dispatch(
@@ -55,23 +57,13 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     '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})
+        self.assertEqual(response.data, {'value': 12, 'id': 13})
 
         # invalid action in dispatch
         response = patch.dispatch(
@@ -79,26 +71,39 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
+                    'value': 6,
                 },
                 {
                     'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
                 },
+            ]), MockObject(13)
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {
+            'detail': '"replace" op has to specify path.',
+        })
+
+        # repeated action in dispatch
+        response = patch.dispatch(
+            MockRequest([
                 {
                     'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
                 },
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 7,
+                    'value': 12,
                 },
             ]), MockObject(13)
         )
 
         self.assertEqual(response.status_code, 400)
-        self.assertEqual(response.data, {'detail': '"replace" op has to specify path.'})
+        self.assertEqual(response.data, {
+            'detail': '"replace" op for "mutate" path is repeated.',
+        })
 
         # op raised validation error
         response = patch.dispatch(
@@ -106,11 +111,6 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
@@ -118,11 +118,6 @@ class ApiPatchDispatchTests(TestCase):
                     'path': 'error',
                     'value': 'invalid',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]), MockObject(13)
         )
 
@@ -135,11 +130,6 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
@@ -147,11 +137,6 @@ class ApiPatchDispatchTests(TestCase):
                     'path': 'error',
                     'value': 'api_invalid',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]), MockObject(13)
         )
 
@@ -164,20 +149,10 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
                     'op': 'replace',
-                    'path': 'mutate',
-                    'value': 9,
-                },
-                {
-                    'op': 'replace',
                     'path': 'error',
                     'value': 'perm',
                 },
@@ -193,30 +168,20 @@ class ApiPatchDispatchTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
+                    'value': 6,
                 },
                 {
                     '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
+        # action in dispatch raised 404 with message but didn't expose it
         response = patch.dispatch(
             MockRequest([
                 {
@@ -229,16 +194,6 @@ class ApiPatchDispatchTests(TestCase):
                     'path': 'error',
                     'value': '404_reason',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]), MockObject(13)
         )
 

misago/api/tests/test_patch_dispatch_bulk.py

@@ -50,48 +50,57 @@ class ApiPatchDispatchBulkTests(TestCase):
                 {
                     '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', 'status': '200', 'patch': {'value': 14}},
-            {'id': '7', 'status': '200', 'patch': {'value': 14}},
+            {'id': '5', 'status': '200', 'patch': {'value': 12}},
+            {'id': '7', 'status': '200', 'patch': {'value': 12}},
         ])
 
+        # dispatch requires list as an argument
+        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."],
+        })
+
         # invalid action in bulk dispatch
         response = patch.dispatch_bulk(
             MockRequest([
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
+                    'value': 6,
                 },
                 {
                     'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
                 },
+            ]),
+            [MockObject(5), MockObject(7)],
+        )
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, {
+            'detail': '"replace" op has to specify path.',
+        })
+
+        # repeated action in dispatch
+        response = patch.dispatch_bulk(
+            MockRequest([
                 {
                     'op': 'replace',
+                    'path': 'mutate',
+                    'value': 6,
                 },
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 7,
+                    'value': 12,
                 },
             ]),
             [MockObject(5), MockObject(7)],
@@ -99,7 +108,7 @@ class ApiPatchDispatchBulkTests(TestCase):
 
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.data, {
-            'detail': '"replace" op has to specify path.',
+            'detail': '"replace" op for "mutate" path is repeated.',
         })
 
         # op raised validation error
@@ -108,11 +117,6 @@ class ApiPatchDispatchBulkTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
@@ -120,11 +124,6 @@ class ApiPatchDispatchBulkTests(TestCase):
                     'path': 'error',
                     'value': 'invalid',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]),
             [MockObject(5), MockObject(7)],
         )
@@ -141,11 +140,6 @@ class ApiPatchDispatchBulkTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
@@ -153,11 +147,6 @@ class ApiPatchDispatchBulkTests(TestCase):
                     'path': 'error',
                     'value': 'api_invalid',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]),
             [MockObject(5), MockObject(7)],
         )
@@ -174,20 +163,10 @@ class ApiPatchDispatchBulkTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
                     'value': 6,
                 },
                 {
                     'op': 'replace',
-                    'path': 'mutate',
-                    'value': 9,
-                },
-                {
-                    'op': 'replace',
                     'path': 'error',
                     'value': 'perm',
                 },
@@ -207,23 +186,13 @@ class ApiPatchDispatchBulkTests(TestCase):
                 {
                     'op': 'replace',
                     'path': 'mutate',
-                    'value': 2,
+                    'value': 6,
                 },
                 {
                     'op': 'replace',
                     'path': 'error',
                     'value': '404',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]),
             [MockObject(5), MockObject(7)],
         )
@@ -234,7 +203,7 @@ class ApiPatchDispatchBulkTests(TestCase):
             {'id': '7', 'status': '404', 'detail': 'NOT FOUND'},
         ])
 
-        # action in bulk dispatch raises 404 and hides the message
+        # action in dispatch raised 404 with message but didn't expose it
         response = patch.dispatch_bulk(
             MockRequest([
                 {
@@ -247,16 +216,6 @@ class ApiPatchDispatchBulkTests(TestCase):
                     'path': 'error',
                     'value': '404_reason',
                 },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 6,
-                },
-                {
-                    'op': 'replace',
-                    'path': 'mutate',
-                    'value': 7,
-                },
             ]),
             [MockObject(5), MockObject(7)],
         )