Reject moving any location into its own children, fixes #214

jsonpatch.py

@@ -338,7 +338,18 @@ def apply(self, obj):
             raise InvalidJsonPatch(
                 "The operation does not contain a 'from' member")
 
+        # 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)
+
+        # Moving the whole document onto itself is a no-op
+        if part is None:
+            return obj
+
         try:
             value = subobj[part]
         except (KeyError, IndexError) as ex:
@@ -348,10 +359,6 @@ def apply(self, obj):
         if self.pointer == from_ptr:
             return obj
 
-        if isinstance(subobj, MutableMapping) and \
-                self.pointer.contains(from_ptr):
-            raise JsonPatchConflict('Cannot move values into their own children')
-
         obj = RemoveOperation({
             'op': 'remove',
             'path': self.operation['from']

property_tests.py

@@ -335,7 +335,7 @@ def test_does_not_modify_patch(self, case):
 
     @unittest.expectedFailure
     @given(docs_with_patches())
-    # 'move' from the whole document crashes for array roots
+    # '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'}]))
@@ -406,8 +406,7 @@ def test_copy_whole_document(self, doc):
         assert_json_equal(result, expected)
 
     # RFC 6902, 4.4: a location cannot be moved into one of its children;
-    # only enforced if the location is an object member
-    @unittest.expectedFailure
+    # was only enforced if the location is an object member, #214
     @given(moves_into_own_child())
     @example(case=([[], []], '/0', '/0/0'))
     def test_move_into_own_child_fails(self, case):

tests.py

@@ -201,10 +201,16 @@ def test_move_array_item(self):
         self.assertEqual(res, {'foo': ['all', 'cows', 'eat', 'grass']})
 
     def test_move_array_item_into_other_item(self):
-        obj = [{"foo": []}, {"bar": []}]
-        patch = [{"op": "move", "from": "/0", "path": "/0/bar/0"}]
-        res = jsonpatch.apply_patch(obj, patch)
-        self.assertEqual(res, [{'bar': [{"foo": []}]}])
+        # https://github.com/stefankoegl/python-json-patch/issues/214
+        # "from" is a proper prefix of "path", which RFC 6902, 4.4 forbids,
+        # even though "path" is inside the other item once "from" is removed
+        for obj, path in [([{"foo": []}, {"bar": []}], "/0/bar/0"),
+                          ([[], []], "/0/0")]:
+            saved = copy.deepcopy(obj)
+            patch = [{"op": "move", "from": "/0", "path": path}]
+            self.assertRaises(jsonpatch.JsonPatchConflict,
+                              jsonpatch.apply_patch, obj, patch)
+            self.assertEqual(obj, saved)
 
     def test_copy_object_keyerror(self):
         obj = {'foo': {'bar': 'baz'},
@@ -1170,6 +1176,22 @@ def test_move_into_child(self):
         patch_obj = [ { "op": "move", "from": "/foo", "path": "/foo/bar" } ]
         self.assertRaises(jsonpatch.JsonPatchException, jsonpatch.apply_patch, src, patch_obj)
 
+    def test_move_whole_document_into_own_child(self):
+        for src, path in [([], '/-'), ([1], '/0'), ({}, '/a'), ({'a': {}}, '/a/b')]:
+            patch_obj = [ { "op": "move", "from": "", "path": path } ]
+            self.assertRaises(jsonpatch.JsonPatchConflict, jsonpatch.apply_patch, src, patch_obj)
+
+    def test_move_whole_document_onto_itself(self):
+        for src in [[1], {'a': 1}]:
+            res = jsonpatch.apply_patch(src, [ { "op": "move", "from": "", "path": "" } ])
+            self.assertEqual(res, src)
+
+    def test_move_onto_itself_must_exist(self):
+        src = {'foo': [1]}
+        for path in ['/bar', '/foo/1']:
+            patch_obj = [ { "op": "move", "from": path, "path": path } ]
+            self.assertRaises(jsonpatch.JsonPatchConflict, jsonpatch.apply_patch, src, patch_obj)
+
     def test_replace_oob(self):
         src = {"foo": [1, 2]}
         patch_obj = [ { "op": "replace", "path": "/foo/10", "value": 10} ]