diff --git a/jsonpatch.py b/jsonpatch.py index 5383aec..93d780c 100644 --- a/jsonpatch.py +++ b/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'] diff --git a/property_tests.py b/property_tests.py index 9e92ebf..0c64b97 100644 --- a/property_tests.py +++ b/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): diff --git a/tests.py b/tests.py index af42fbc..31499ee 100755 --- a/tests.py +++ b/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} ]