Raise documented exceptions for malformed patches and "from": "/-"

jsonpatch.py

@@ -36,7 +36,7 @@
 import copy
 import functools
 import json
-from collections.abc import MutableMapping, MutableSequence, Sequence
+from collections.abc import Mapping, MutableMapping, MutableSequence, Sequence
 from difflib import SequenceMatcher
 from types import MappingProxyType
 
@@ -185,6 +185,23 @@ def __init__(self, operation, pointer_cls=JsonPointer):
 
         self.operation = operation
 
+    def _from_pointer(self):
+        """Builds the pointer of the 'from' member like __init__ builds the
+        one of 'path'."""
+        try:
+            from_ptr = self.operation['from']
+        except KeyError:
+            raise InvalidJsonPatch(
+                "The operation does not contain a 'from' member")
+
+        if isinstance(from_ptr, self.pointer_cls):
+            return from_ptr
+
+        try:
+            return self.pointer_cls(from_ptr)
+        except TypeError:
+            raise InvalidJsonPatch("Invalid 'from'")
+
     def apply(self, obj):
         """Abstract method that applies a patch operation to the specified object."""
         raise NotImplementedError('should implement the patch operation.')
@@ -334,22 +351,15 @@ class MoveOperation(PatchOperation):
     """Moves an object property or an array element to a new location."""
 
     def apply(self, obj):
-        try:
-            if isinstance(self.operation['from'], self.pointer_cls):
-                from_ptr = self.operation['from']
-            else:
-                from_ptr = self.pointer_cls(self.operation['from'])
-        except KeyError:
-            raise InvalidJsonPatch(
-                "The operation does not contain a 'from' member")
+        from_ptr = self._from_pointer()
 
         # Checked before anything is resolved, as removing an array element
         # shifts its siblings, so the target would resolve to a different
         # location. This also covers moving the whole document ('from' is "").
         if self.pointer != from_ptr and self.pointer.contains(from_ptr):
             raise JsonPatchConflict('Cannot move values into their own children')
 
-        subobj, part = _to_last(from_ptr, obj)
+        subobj, part = _to_existing_last(from_ptr, obj)
 
         # Moving the whole document onto itself is a no-op
         if part is None:
@@ -434,13 +444,9 @@ class CopyOperation(PatchOperation):
     """ Copies an object property or an array element to a new location """
 
     def apply(self, obj):
-        try:
-            from_ptr = self.pointer_cls(self.operation['from'])
-        except KeyError:
-            raise InvalidJsonPatch(
-                "The operation does not contain a 'from' member")
+        from_ptr = self._from_pointer()
 
-        subobj, part = _to_last(from_ptr, obj)
+        subobj, part = _to_existing_last(from_ptr, obj)
         try:
             value = copy.deepcopy(subobj if part is None else subobj[part])
         except (KeyError, IndexError) as ex:
@@ -649,6 +655,9 @@ def apply(self, obj, in_place=False):
         return obj
 
     def _get_operation(self, operation):
+        if not isinstance(operation, Mapping):
+            raise InvalidJsonPatch("Operation must be an object")
+
         if 'op' not in operation:
             raise InvalidJsonPatch("Operation does not contain 'op' member")
 
@@ -1089,3 +1098,16 @@ def _to_last(pointer, doc):
                 part, type(subobj)))
 
     return subobj, part
+
+
+def _to_existing_last(pointer, doc):
+    """Resolve pointer like _to_last, for a location that has to exist.
+
+    The '-' of an array refers to the nonexistent element after its last one.
+    """
+    subobj, part = _to_last(pointer, doc)
+
+    if part == '-' and isinstance(subobj, Sequence):
+        raise JsonPointerException("invalid array index '{0}'".format(part))
+
+    return subobj, part

property_tests.py

@@ -336,8 +336,6 @@ def test_does_not_modify_patch(self, case):
     @given(docs_with_patches())
     # 'move' from the whole document crashed for array roots, #214
     @example(case=([], [{'op': 'move', 'from': '', 'path': '/-'}]))
-    # 'copy' or 'move' from the '-' of an array crashes
-    @example(case=([0], [{'op': 'copy', 'from': '/-', 'path': '/0'}]))
     # operations on the whole document crash if it is a scalar
     @example(case=(None, [{'op': 'remove', 'path': ''}]))
     def test_raises_only_documented_exceptions(self, case):
@@ -347,7 +345,6 @@ def test_raises_only_documented_exceptions(self, case):
         except (jsonpatch.JsonPatchException, JsonPointerException):
             pass
 
-    @unittest.expectedFailure
     @given(st.lists(st.one_of(
         json_docs,
         st.dictionaries(st.sampled_from(['op', 'path', 'from', 'value']),
@@ -357,9 +354,9 @@ def test_raises_only_documented_exceptions(self, case):
             optional={'path': json_docs, 'from': json_docs,
                       'value': json_docs}),
     )))
-    # patch elements that are not objects
+    # patch elements that are not objects raised TypeError, #215
     @example(patch=[0])
-    # 'from' that is not a string
+    # 'from' that is not a string raised TypeError, #215
     @example(patch=[{'op': 'copy', 'from': 0, 'path': '/a'}])
     def test_malformed_patch_raises_documented_exceptions(self, patch):
         try:

tests.py

@@ -89,6 +89,20 @@ def test_remove_invalid_item(self):
         with self.assertRaises(jsonpointer.JsonPointerException):
             jsonpatch.apply_patch(obj, [{'op': 'remove', 'path': '/foo/-'}])
 
+    def test_move_copy_from_end_of_list(self):
+        # '-' refers to the nonexistent element after the last one, #215
+        obj = {'foo': ['bar', 'qux', 'baz']}
+        for op in ('move', 'copy'):
+            with self.assertRaises(jsonpointer.JsonPointerException):
+                jsonpatch.apply_patch(obj, [{'op': op, 'from': '/foo/-', 'path': '/foo/0'}])
+
+    def test_move_copy_from_dash_object_key(self):
+        # '-' is an ordinary key in objects
+        obj = {'-': 1}
+        res = jsonpatch.apply_patch(obj, [{'op': 'copy', 'from': '/-', 'path': '/a'}])
+        self.assertEqual(res, {'-': 1, 'a': 1})
+        res = jsonpatch.apply_patch(obj, [{'op': 'move', 'from': '/-', 'path': '/a'}])
+        self.assertEqual(res, {'a': 1})
 
     def test_replace_object_key(self):
         obj = {'foo': 'bar', 'baz': 'qux'}
@@ -1208,6 +1222,20 @@ def test_invalid_op(self):
         patch_obj = [ { "op": "invalid", "path": "/child", "value": { "grandchild": { } } } ]
         self.assertRaises(jsonpatch.JsonPatchException, jsonpatch.apply_patch, src, patch_obj)
 
+    def test_operation_not_an_object(self):
+        # each element of a patch has to be an object, #215
+        src = {"foo": "bar"}
+        for operation in [0, None, True, [], ["op", "add"]]:
+            self.assertRaises(jsonpatch.InvalidJsonPatch, jsonpatch.apply_patch, src, [operation])
+
+    def test_invalid_from(self):
+        # "from" has to be a JSON pointer, like "path", #215
+        src = {"foo": "bar"}
+        for op in ("move", "copy"):
+            for from_ in [0, None, ["foo"], {"foo": "bar"}]:
+                patch_obj = [ { "op": op, "from": from_, "path": "/baz" } ]
+                self.assertRaises(jsonpatch.InvalidJsonPatch, jsonpatch.apply_patch, src, patch_obj)
+
 
 class ConflictTests(unittest.TestCase):
 
@@ -1339,6 +1367,12 @@ def test_create_with_pointer(self):
         expected = {'bar': 'bar', 'baz': [42]}
         self.assertEqual(result, expected)
 
+    def test_copy_from_pointer(self):
+        patch = jsonpatch.JsonPatch([
+            {'op': 'copy', 'from': jsonpointer.JsonPointer('/foo'), 'path': '/bar'},
+        ])
+        self.assertEqual(patch.apply({'foo': 1}), {'foo': 1, 'bar': 1})
+
 
 class JsonPatchCreationTest(unittest.TestCase):