From feff399d56d2755583d82c42b7e8359f170f3460 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sat, 11 Oct 2025 13:58:15 -0400 Subject: [PATCH 1/8] create new test files --- issue1.py | 16 ++++++++++++++++ issue2.py | 9 +++++++++ 2 files changed, 25 insertions(+) create mode 100644 issue1.py create mode 100644 issue2.py diff --git a/issue1.py b/issue1.py new file mode 100644 index 0000000..69418e0 --- /dev/null +++ b/issue1.py @@ -0,0 +1,16 @@ +from jsonpatch import apply_patch, make_patch + +old = [ + {"x": ["a", {"y": ["b"]}], "z": "a"}, + {"x": ["c", {"d": ["d"]}], "z": "c"}, + {}, +] + +new = [ + {"x": ["c", {"y": ["d"]}], "z": "c"}, + {}, +] + +patch = make_patch(old, new) + +assert apply_patch(old, patch) == new diff --git a/issue2.py b/issue2.py new file mode 100644 index 0000000..7a99a8c --- /dev/null +++ b/issue2.py @@ -0,0 +1,9 @@ +from jsonpatch import apply_patch, make_patch + +old = ['a', 'b', ['d', 'e'], 'f'] + +new = ['a', 'd', ['e', 'g']] + +patch = make_patch(old, new) + +assert apply_patch(old, patch) == new From 471806d21587da7b17ba75946e92dfe8ef9b465b Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sat, 11 Oct 2025 14:02:50 -0400 Subject: [PATCH 2/8] fix on_undo logic When a RemoveOperation or AddOperation is removed, subsequent operations must have their loactions adjusted whenever they depended on deleted operation. Previously, the logic only tested self.path == path, which fails to account for edge cases where self.path.startswith(path) but self.path != path. --- jsonpatch.py | 102 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 67 insertions(+), 35 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index d3fc26d..f045525 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -223,17 +223,29 @@ def path(self): @property def key(self): - try: - return int(self.pointer.parts[-1]) - except ValueError: - return self.pointer.parts[-1] + return self.get_part(-1) @key.setter def key(self, value): - self.pointer.parts[-1] = str(value) + self.set_part(-1, value) + + def get_part(self, index): + try: + return int(self.pointer.parts[index]) + except ValueError: + return self.pointer.parts[index] + + def set_part(self, index, value): + self.pointer.parts[index] = str(value) self.location = self.pointer.path self.operation['path'] = self.location + def _increment_part(self, index): + self.set_part(index, self.get_part(index) + 1) + + def _decrement_part(self, index): + self.set_part(index, self.get_part(index) - 1) + class RemoveOperation(PatchOperation): """Removes an object property or an array element.""" @@ -253,17 +265,19 @@ def apply(self, obj): return obj def _on_undo_remove(self, path, key): - if self.path == path: - if self.key >= key: - self.key += 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) >= key: + self._increment_part(part_index) else: key -= 1 return key def _on_undo_add(self, path, key): - if self.path == path: - if self.key > key: - self.key -= 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) > key: + self._decrement_part(part_index) else: key -= 1 return key @@ -305,17 +319,19 @@ def apply(self, obj): return obj def _on_undo_remove(self, path, key): - if self.path == path: - if self.key > key: - self.key += 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) > key: + self._increment_part(part_index) else: key += 1 return key def _on_undo_add(self, path, key): - if self.path == path: - if self.key > key: - self.key -= 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) > key: + self._decrement_part(part_index) else: key += 1 return key @@ -410,40 +426,56 @@ def from_path(self): @property def from_key(self): - from_ptr = self.pointer_cls(self.operation['from']) - try: - return int(from_ptr.parts[-1]) - except TypeError: - return from_ptr.parts[-1] + return self.get_from_part(-1) @from_key.setter def from_key(self, value): + self.set_from_part(-1, value) + + def get_from_part(self, index): from_ptr = self.pointer_cls(self.operation['from']) - from_ptr.parts[-1] = str(value) + try: + return int(from_ptr.parts[index]) + except ValueError: + return from_ptr.parts[index] + + def set_from_part(self, index, value): + from_ptr = self.pointer_cls(self.operation['from']) + from_ptr.parts[index] = str(value) self.operation['from'] = from_ptr.path + def _increment_from_part(self, index): + self.set_from_part(index, self.get_from_part(index) + 1) + + def _decrement_from_part(self, index): + self.set_from_part(index, self.get_from_part(index) - 1) + def _on_undo_remove(self, path, key): - if self.from_path == path: - if self.from_key >= key: - self.from_key += 1 + if self.from_path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_from_part(part_index) >= key: + self._increment_from_part(part_index) else: key -= 1 - if self.path == path: - if self.key > key: - self.key += 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) > key: + self._increment_part(part_index) else: key += 1 return key def _on_undo_add(self, path, key): - if self.from_path == path: - if self.from_key > key: - self.from_key -= 1 + if self.from_path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_from_part(part_index) > key: + self._decrement_from_part(part_index) else: key -= 1 - if self.path == path: - if self.key > key: - self.key -= 1 + if self.path.startswith(path): + part_index = len(path.split("/")) if path else 0 + if self.get_part(part_index) > key: + self._decrement_part(part_index) else: key += 1 return key From ab50e6d70fe8978e1f5b5242ab9097f9d411a835 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sat, 11 Oct 2025 17:05:09 -0400 Subject: [PATCH 3/8] Added new tests --- tests.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests.py b/tests.py index d9eea92..7fffbec 100755 --- a/tests.py +++ b/tests.py @@ -566,6 +566,32 @@ def test_issue120(self): res = jsonpatch.apply_patch(src, patch) self.assertEqual(res, dst) + def test_issue_138(self): + """ + The _on_undo methods should update its operation's path if it is + affected by the removal of a prior operation. + """ + old = [ + {"x": ["a", {"y": ["b"]}], "z": "a"}, + {"x": ["c", {"d": ["d"]}], "z": "c"}, + {}, + ] + new = [ + {"x": ["c", {"y": ["d"]}], "z": "c"}, + {}, + ] + patch = jsonpatch.make_patch(old, new) + result = jsonpatch.apply_patch(old, patch) + self.assertEqual(result, new) + + def test_issue_124(self): + """Similar to issue 138, but for different operations.""" + old = ['a', 'b', ['d', 'e'], 'f'] + new = ['a', 'd', ['e', 'g']] + patch = jsonpatch.make_patch(old, new) + result = jsonpatch.apply_patch(old, patch) + self.assertEqual(result, new) + def test_custom_types_diff(self): old = {'value': decimal.Decimal('1.0')} new = {'value': decimal.Decimal('1.00')} From 23d8ad98f7b6fa990adf10068f86c52abc0aa2d5 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sat, 11 Oct 2025 17:05:55 -0400 Subject: [PATCH 4/8] removed old test files (tests.py already updated) --- issue1.py | 16 ---------------- issue2.py | 9 --------- 2 files changed, 25 deletions(-) delete mode 100644 issue1.py delete mode 100644 issue2.py diff --git a/issue1.py b/issue1.py deleted file mode 100644 index 69418e0..0000000 --- a/issue1.py +++ /dev/null @@ -1,16 +0,0 @@ -from jsonpatch import apply_patch, make_patch - -old = [ - {"x": ["a", {"y": ["b"]}], "z": "a"}, - {"x": ["c", {"d": ["d"]}], "z": "c"}, - {}, -] - -new = [ - {"x": ["c", {"y": ["d"]}], "z": "c"}, - {}, -] - -patch = make_patch(old, new) - -assert apply_patch(old, patch) == new diff --git a/issue2.py b/issue2.py deleted file mode 100644 index 7a99a8c..0000000 --- a/issue2.py +++ /dev/null @@ -1,9 +0,0 @@ -from jsonpatch import apply_patch, make_patch - -old = ['a', 'b', ['d', 'e'], 'f'] - -new = ['a', 'd', ['e', 'g']] - -patch = make_patch(old, new) - -assert apply_patch(old, patch) == new From f05c0e5e94f9059f4ea610f90391f628ca801197 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sun, 12 Oct 2025 10:29:49 -0400 Subject: [PATCH 5/8] replace startswith with _is_path_prefix Necessary for the case where prefix == 'a/b' and path == 'a/bc/d' in which case it should return False, not True. I also considered using JsonPointer.contains(), but for some reason JsonPointer('/a/b').contains(JsonPointer('/')) returns False, so it's easier to just compare strings. --- jsonpatch.py | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index f045525..c144d30 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -265,8 +265,8 @@ def apply(self, obj): return obj def _on_undo_remove(self, path, key): - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) >= key: self._increment_part(part_index) else: @@ -274,8 +274,8 @@ def _on_undo_remove(self, path, key): return key def _on_undo_add(self, path, key): - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -319,8 +319,8 @@ def apply(self, obj): return obj def _on_undo_remove(self, path, key): - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) > key: self._increment_part(part_index) else: @@ -328,8 +328,8 @@ def _on_undo_remove(self, path, key): return key def _on_undo_add(self, path, key): - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -451,14 +451,14 @@ def _decrement_from_part(self, index): self.set_from_part(index, self.get_from_part(index) - 1) def _on_undo_remove(self, path, key): - if self.from_path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.from_path): + part_index = _path_len(path) if self.get_from_part(part_index) >= key: self._increment_from_part(part_index) else: key -= 1 - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) > key: self._increment_part(part_index) else: @@ -466,14 +466,14 @@ def _on_undo_remove(self, path, key): return key def _on_undo_add(self, path, key): - if self.from_path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.from_path): + part_index = _path_len(path) if self.get_from_part(part_index) > key: self._decrement_from_part(part_index) else: key -= 1 - if self.path.startswith(path): - part_index = len(path.split("/")) if path else 0 + if _is_path_prefix(prefix=path, path=self.path): + part_index = _path_len(path) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -961,3 +961,9 @@ def _path_join(path, key): return path return path + '/' + str(key).replace('~', '~0').replace('/', '~1') + +def _is_path_prefix(prefix, path): + return prefix == path or path.startswith(prefix + "/") or not prefix + +def _path_len(path): + return len(path.split("/")) if path else 0 From 7165029c89a20e76e4fa598f44da3dac484f340f Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sun, 12 Oct 2025 12:24:06 -0400 Subject: [PATCH 6/8] new test to track progress towards proper escape handling --- tests.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests.py b/tests.py index 7fffbec..291267e 100755 --- a/tests.py +++ b/tests.py @@ -584,6 +584,26 @@ def test_issue_138(self): result = jsonpatch.apply_patch(old, patch) self.assertEqual(result, new) + def test_issue_138b(self): + """Similar to issue 138 but additionally tests escaping""" + old = {"/": + [ + {"x": ["a", {"y": ["b"]}], "z": "a"}, + {"x": ["c", {"d": ["d"]}], "z": "c"}, + {}, + ] + } + new = {"/": + [ + {"x": ["c", {"y": ["d"]}], "z": "c"}, + {}, + ] + } + patch = jsonpatch.make_patch(old, new) + result = jsonpatch.apply_patch(old, patch) + self.assertEqual(result, new) + + def test_issue_124(self): """Similar to issue 138, but for different operations.""" old = ['a', 'b', ['d', 'e'], 'f'] From 0b313bb5ab81ee51fed610cc75ce2eea91db1df2 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sun, 12 Oct 2025 17:46:21 -0400 Subject: [PATCH 7/8] _is_prefix should take parts, not string paths Comparing string paths to determine if one path depends on another's removal is prone to error due to special characters '/' and '~'. Comparing parts is the correct approach. --- jsonpatch.py | 57 ++++++++++++++++++++++++++-------------------------- tests.py | 2 +- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index c144d30..74da4f6 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -264,18 +264,18 @@ def apply(self, obj): return obj - def _on_undo_remove(self, path, key): - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + def _on_undo_remove(self, sub_parts, key): + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) >= key: self._increment_part(part_index) else: key -= 1 return key - def _on_undo_add(self, path, key): - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + def _on_undo_add(self, sub_parts, key): + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -318,18 +318,18 @@ def apply(self, obj): raise JsonPatchConflict("unable to fully resolve json pointer {0}, part {1}".format(self.location, part)) return obj - def _on_undo_remove(self, path, key): - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + def _on_undo_remove(self, sub_parts, key): + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) > key: self._increment_part(part_index) else: key += 1 return key - def _on_undo_add(self, path, key): - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + def _on_undo_add(self, sub_parts, key): + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -450,30 +450,32 @@ def _increment_from_part(self, index): def _decrement_from_part(self, index): self.set_from_part(index, self.get_from_part(index) - 1) - def _on_undo_remove(self, path, key): - if _is_path_prefix(prefix=path, path=self.from_path): - part_index = _path_len(path) + def _on_undo_remove(self, sub_parts, key): + from_ptr = self.pointer_cls(self.operation['from']) + if _is_prefix(prefix_parts=sub_parts, parts=from_ptr.parts): + part_index = len(sub_parts) if self.get_from_part(part_index) >= key: self._increment_from_part(part_index) else: key -= 1 - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) > key: self._increment_part(part_index) else: key += 1 return key - def _on_undo_add(self, path, key): - if _is_path_prefix(prefix=path, path=self.from_path): - part_index = _path_len(path) + def _on_undo_add(self, sub_parts, key): + from_ptr = self.pointer_cls(self.operation['from']) + if _is_prefix(prefix_parts=sub_parts, parts=from_ptr.parts): + part_index = len(sub_parts) if self.get_from_part(part_index) > key: self._decrement_from_part(part_index) else: key -= 1 - if _is_path_prefix(prefix=path, path=self.path): - part_index = _path_len(path) + if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): + part_index = len(sub_parts) if self.get_part(part_index) > key: self._decrement_part(part_index) else: @@ -831,7 +833,7 @@ def _item_added(self, path, key, item): op = index[2] if type(op.key) == int and type(key) == int: for v in self.iter_from(index): - op.key = v._on_undo_remove(op.path, op.key) + op.key = v._on_undo_remove(op.pointer.parts[:-1], op.key) self.remove(index) if op.location != _path_join(path, key): @@ -866,7 +868,7 @@ def _item_removed(self, path, key, item): added_item = op.pointer.to_last(self.dst_doc)[0] if type(added_item) == list: for v in self.iter_from(index): - op.key = v._on_undo_add(op.path, op.key) + op.key = v._on_undo_add(op.pointer.parts[:-1], op.key) self.remove(index) if new_op.location != op.location: @@ -962,8 +964,5 @@ def _path_join(path, key): return path + '/' + str(key).replace('~', '~0').replace('/', '~1') -def _is_path_prefix(prefix, path): - return prefix == path or path.startswith(prefix + "/") or not prefix - -def _path_len(path): - return len(path.split("/")) if path else 0 +def _is_prefix(prefix_parts, parts): + return prefix_parts == parts[:len(prefix_parts)] diff --git a/tests.py b/tests.py index 291267e..1bf89f4 100755 --- a/tests.py +++ b/tests.py @@ -585,7 +585,7 @@ def test_issue_138(self): self.assertEqual(result, new) def test_issue_138b(self): - """Similar to issue 138 but additionally tests escaping""" + """Additionally tests escaping special characters.""" old = {"/": [ {"x": ["a", {"y": ["b"]}], "z": "a"}, From 6c7d2cef12dffc2f6df2daaf5f942b92571496d6 Mon Sep 17 00:00:00 2001 From: Angela Liss <59097311+angela-tarantula@users.noreply.github.com> Date: Sun, 12 Oct 2025 18:28:47 -0400 Subject: [PATCH 8/8] style: variable/method names --- jsonpatch.py | 72 ++++++++++++++++++++++++++-------------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index 74da4f6..c7c9671 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -265,19 +265,19 @@ def apply(self, obj): return obj def _on_undo_remove(self, sub_parts, key): - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) >= key: - self._increment_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) >= key: + self._increment_part(affected_index) else: key -= 1 return key def _on_undo_add(self, sub_parts, key): - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) > key: - self._decrement_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) > key: + self._decrement_part(affected_index) else: key -= 1 return key @@ -319,19 +319,19 @@ def apply(self, obj): return obj def _on_undo_remove(self, sub_parts, key): - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) > key: - self._increment_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) > key: + self._increment_part(affected_index) else: key += 1 return key def _on_undo_add(self, sub_parts, key): - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) > key: - self._decrement_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) > key: + self._decrement_part(affected_index) else: key += 1 return key @@ -372,10 +372,10 @@ def apply(self, obj): subobj[part] = value return obj - def _on_undo_remove(self, path, key): + def _on_undo_remove(self, sub_parts, key): return key - def _on_undo_add(self, path, key): + def _on_undo_add(self, sub_parts, key): return key @@ -452,32 +452,32 @@ def _decrement_from_part(self, index): def _on_undo_remove(self, sub_parts, key): from_ptr = self.pointer_cls(self.operation['from']) - if _is_prefix(prefix_parts=sub_parts, parts=from_ptr.parts): - part_index = len(sub_parts) - if self.get_from_part(part_index) >= key: - self._increment_from_part(part_index) + if _is_prefix(sub_parts, from_ptr.parts): + affected_index = len(sub_parts) + if self.get_from_part(affected_index) >= key: + self._increment_from_part(affected_index) else: key -= 1 - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) > key: - self._increment_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) > key: + self._increment_part(affected_index) else: key += 1 return key def _on_undo_add(self, sub_parts, key): from_ptr = self.pointer_cls(self.operation['from']) - if _is_prefix(prefix_parts=sub_parts, parts=from_ptr.parts): - part_index = len(sub_parts) - if self.get_from_part(part_index) > key: - self._decrement_from_part(part_index) + if _is_prefix(sub_parts, from_ptr.parts): + affected_index = len(sub_parts) + if self.get_from_part(affected_index) > key: + self._decrement_from_part(affected_index) else: key -= 1 - if _is_prefix(prefix_parts=sub_parts, parts=self.pointer.parts): - part_index = len(sub_parts) - if self.get_part(part_index) > key: - self._decrement_part(part_index) + if _is_prefix(sub_parts, self.pointer.parts): + affected_index = len(sub_parts) + if self.get_part(affected_index) > key: + self._decrement_part(affected_index) else: key += 1 return key @@ -964,5 +964,5 @@ def _path_join(path, key): return path + '/' + str(key).replace('~', '~0').replace('/', '~1') -def _is_prefix(prefix_parts, parts): - return prefix_parts == parts[:len(prefix_parts)] +def _is_prefix(sub_parts, parts): + return sub_parts == parts[:len(sub_parts)]