Refactor JSON Patch OpRemove and OpMove

CHANGELOG.md

@@ -5,6 +5,7 @@
 **Fixes**
 
 - JSON Patch `move` and `copy` operations now use `add` semantics for their target locations. Previously it was not possible to move or copy to the end of an array with the special `-` index, and we would silently append to arrays given an index greater than the array's length. We now raise a `JSONPatchError` if the target index is greater than the number of elements in the array.
+- Fixed JSONPatch `move` operations with immutable Mapping or Sequence source objects. Previously we silently ignored the internal `remove` op if the source was immutable,making the operation a copy. Now we raise a `JSONPatchError`.
 
 # Version 2.2.1
 

jsonpath/__about__.py

@@ -1,4 +1,4 @@
 # SPDX-FileCopyrightText: 2023-present James Prior <jamesgr.prior@gmail.com>
 #
 # SPDX-License-Identifier: MIT
-__version__ = "2.2.1"
+__version__ = "2.2.2"

jsonpath/patch.py

@@ -152,22 +152,8 @@ def apply(
         self, data: Union[MutableSequence[object], MutableMapping[str, object]]
     ) -> Union[MutableSequence[object], MutableMapping[str, object]]:
         """Apply this patch operation to _data_."""
-        parent, obj = self.path.resolve_parent(data)
-        if parent is None:
-            raise JSONPatchError("can't remove root")
-
-        if isinstance(parent, MutableSequence):
-            if obj is UNDEFINED:
-                raise JSONPatchError("can't remove nonexistent item")
-            del parent[int(self.path.parts[-1])]
-        elif isinstance(parent, MutableMapping):
-            if obj is UNDEFINED:
-                raise JSONPatchError("can't remove nonexistent property")
-            del parent[str(self.path.parts[-1])]
-        else:
-            raise JSONPatchError(
-                f"unexpected operation on {parent.__class__.__name__!r}"
-            )
+        # Note that it's not possible to remove the root value.
+        _remove(data=data, path=self.path, op=self.name)
         return data
 
     def asdict(self) -> Dict[str, object]:
@@ -231,17 +217,8 @@ def apply(
         if self.dest.is_relative_to(self.source):
             raise JSONPatchError("can't move object to one of its own children")
 
-        source_parent, source_obj = self.source.resolve_parent(data)
-
-        if source_obj is UNDEFINED:
-            raise JSONPatchError("source object does not exist")
-
-        if isinstance(source_parent, MutableSequence):
-            del source_parent[int(self.source.parts[-1])]
-        if isinstance(source_parent, MutableMapping):
-            del source_parent[str(self.source.parts[-1])]
-
-        return _add(path=self.dest, value=source_obj, data=data)
+        obj = _remove(path=self.source, data=data, op=self.name)
+        return _add(path=self.dest, value=obj, data=data)
 
     def asdict(self) -> Dict[str, object]:
         """Return a dictionary representation of this operation."""
@@ -788,7 +765,7 @@ def patched(
 
 
 def _add(*, data: _DataT, path: JSONPointer, value: object) -> _DataT:
-    """Add _value_ to _data_ at _path_.
+    """Add _value_ to _data_ at _path_ and return modified _data_.
 
     This is semantically the `add` operation, used by `OpAdd`, `OpMove` and
     `OpCopy`.
@@ -816,5 +793,32 @@ def _add(*, data: _DataT, path: JSONPointer, value: object) -> _DataT:
     elif isinstance(parent, MutableMapping):
         parent[str(target)] = value
     else:
-        raise JSONPatchError(f"unexpected operation on {parent.__class__.__name__!r}")
+        raise JSONPatchError(
+            f"unexpected operation on {parent.__class__.__name__!r}",
+        )
     return data
+
+
+def _remove(*, data: _DataT, path: JSONPointer, op: str) -> object:
+    """Remove value at _path_ from _data_ and return the removed value.
+
+    This is semantically the `remove` operation used by `OpRemove` and
+    `OpMove`.
+    """
+    parent, obj = path.resolve_parent(data)
+    if parent is None:
+        raise JSONPatchError(f"can't {op} root")
+
+    if isinstance(parent, MutableSequence):
+        if obj is UNDEFINED:
+            raise JSONPatchError(f"can't {op} nonexistent item")
+        del parent[int(path.parts[-1])]
+    elif isinstance(parent, MutableMapping):
+        if obj is UNDEFINED:
+            raise JSONPatchError(f"can't {op} nonexistent property")
+        del parent[str(path.parts[-1])]
+    else:
+        raise JSONPatchError(
+            f"unexpected operation on {parent.__class__.__name__!r}",
+        )
+    return obj

tests/test_json_patch.py

@@ -6,6 +6,7 @@
 from collections.abc import Mapping
 from contextlib import suppress
 from io import StringIO
+from types import MappingProxyType
 from typing import Any
 from typing import Dict
 from typing import Iterator
@@ -117,7 +118,7 @@ def test_move_to_child() -> None:
 
 def test_move_nonexistent_value() -> None:
     with pytest.raises(
-        JSONPatchError, match=re.escape("source object does not exist (move:0)")
+        JSONPatchError, match=re.escape("can't move nonexistent property (move:0)")
     ):
         JSONPatch().move(from_="/foo/bar", path="/bar").apply({"foo": {"baz": 1}})
 
@@ -141,6 +142,24 @@ def test_move_to_immutable_mapping() -> None:
         patch.apply({"foo": {"bar": "hello"}, "baz": MockMapping()})
 
 
+def test_move_from_immutable_mapping() -> None:
+    patch = JSONPatch().move(from_="/foo/bar", path="/baz/bar")
+    with pytest.raises(
+        JSONPatchError,
+        match=re.escape("unexpected operation on 'mappingproxy' (move:0)"),
+    ):
+        patch.apply({"foo": MappingProxyType({"bar": "hello"}), "baz": {}})
+
+
+def test_move_from_immutable_sequence() -> None:
+    patch = JSONPatch().move(from_="/foo/0", path="/baz/0")
+    with pytest.raises(
+        JSONPatchError,
+        match=re.escape("unexpected operation on 'tuple' (move:0)"),
+    ):
+        patch.apply({"foo": ("hello",), "baz": []})
+
+
 def test_copy_nonexistent_value() -> None:
     with pytest.raises(
         JSONPatchError, match=re.escape("source object does not exist (copy:0)")