Skip to content

Commit 493215f

Browse files
angela-tarantulastefankoeglCopilot
authored
DiffBuilder: fix reindexing of operations involving nested paths (#175)
* create new test files * 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. * Added new tests * removed old test files (tests.py already updated) * 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. * new test to track progress towards proper escape handling * _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. * style: variable/method names * Potential fix for pull request finding 'Make prefix reindexing regression test deterministic' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Stefan Kögl <stefan@skoegl.net> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent d70a4d6 commit 493215f

2 files changed

Lines changed: 134 additions & 45 deletions

File tree

‎jsonpatch.py‎

Lines changed: 82 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -199,17 +199,29 @@ def path(self):
199199

200200
@property
201201
def key(self):
202-
try:
203-
return int(self.pointer.parts[-1])
204-
except ValueError:
205-
return self.pointer.parts[-1]
202+
return self.get_part(-1)
206203

207204
@key.setter
208205
def key(self, value):
209-
self.pointer.parts[-1] = str(value)
206+
self.set_part(-1, value)
207+
208+
def get_part(self, index):
209+
try:
210+
return int(self.pointer.parts[index])
211+
except ValueError:
212+
return self.pointer.parts[index]
213+
214+
def set_part(self, index, value):
215+
self.pointer.parts[index] = str(value)
210216
self.location = self.pointer.path
211217
self.operation['path'] = self.location
212218

219+
def _increment_part(self, index):
220+
self.set_part(index, self.get_part(index) + 1)
221+
222+
def _decrement_part(self, index):
223+
self.set_part(index, self.get_part(index) - 1)
224+
213225

214226
class RemoveOperation(PatchOperation):
215227
"""Removes an object property or an array element."""
@@ -228,18 +240,20 @@ def apply(self, obj):
228240

229241
return obj
230242

231-
def _on_undo_remove(self, path, key):
232-
if self.path == path:
233-
if self.key >= key:
234-
self.key += 1
243+
def _on_undo_remove(self, sub_parts, key):
244+
if _is_prefix(sub_parts, self.pointer.parts):
245+
affected_index = len(sub_parts)
246+
if self.get_part(affected_index) >= key:
247+
self._increment_part(affected_index)
235248
else:
236249
key -= 1
237250
return key
238251

239-
def _on_undo_add(self, path, key):
240-
if self.path == path:
241-
if self.key > key:
242-
self.key -= 1
252+
def _on_undo_add(self, sub_parts, key):
253+
if _is_prefix(sub_parts, self.pointer.parts):
254+
affected_index = len(sub_parts)
255+
if self.get_part(affected_index) > key:
256+
self._decrement_part(affected_index)
243257
else:
244258
key -= 1
245259
return key
@@ -283,18 +297,20 @@ def apply(self, obj):
283297
raise JsonPatchConflict("unable to fully resolve json pointer {0}, part {1}".format(self.location, part))
284298
return obj
285299

286-
def _on_undo_remove(self, path, key):
287-
if self.path == path:
288-
if self.key > key:
289-
self.key += 1
300+
def _on_undo_remove(self, sub_parts, key):
301+
if _is_prefix(sub_parts, self.pointer.parts):
302+
affected_index = len(sub_parts)
303+
if self.get_part(affected_index) > key:
304+
self._increment_part(affected_index)
290305
else:
291306
key += 1
292307
return key
293308

294-
def _on_undo_add(self, path, key):
295-
if self.path == path:
296-
if self.key > key:
297-
self.key -= 1
309+
def _on_undo_add(self, sub_parts, key):
310+
if _is_prefix(sub_parts, self.pointer.parts):
311+
affected_index = len(sub_parts)
312+
if self.get_part(affected_index) > key:
313+
self._decrement_part(affected_index)
298314
else:
299315
key += 1
300316
return key
@@ -335,10 +351,10 @@ def apply(self, obj):
335351
subobj[part] = value
336352
return obj
337353

338-
def _on_undo_remove(self, path, key):
354+
def _on_undo_remove(self, sub_parts, key):
339355
return key
340356

341-
def _on_undo_add(self, path, key):
357+
def _on_undo_add(self, sub_parts, key):
342358
return key
343359

344360

@@ -389,40 +405,58 @@ def from_path(self):
389405

390406
@property
391407
def from_key(self):
392-
from_ptr = self.pointer_cls(self.operation['from'])
393-
try:
394-
return int(from_ptr.parts[-1])
395-
except TypeError:
396-
return from_ptr.parts[-1]
408+
return self.get_from_part(-1)
397409

398410
@from_key.setter
399411
def from_key(self, value):
412+
self.set_from_part(-1, value)
413+
414+
def get_from_part(self, index):
415+
from_ptr = self.pointer_cls(self.operation['from'])
416+
try:
417+
return int(from_ptr.parts[index])
418+
except ValueError:
419+
return from_ptr.parts[index]
420+
421+
def set_from_part(self, index, value):
400422
from_ptr = self.pointer_cls(self.operation['from'])
401-
from_ptr.parts[-1] = str(value)
423+
from_ptr.parts[index] = str(value)
402424
self.operation['from'] = from_ptr.path
403425

404-
def _on_undo_remove(self, path, key):
405-
if self.from_path == path:
406-
if self.from_key >= key:
407-
self.from_key += 1
426+
def _increment_from_part(self, index):
427+
self.set_from_part(index, self.get_from_part(index) + 1)
428+
429+
def _decrement_from_part(self, index):
430+
self.set_from_part(index, self.get_from_part(index) - 1)
431+
432+
def _on_undo_remove(self, sub_parts, key):
433+
from_ptr = self.pointer_cls(self.operation['from'])
434+
if _is_prefix(sub_parts, from_ptr.parts):
435+
affected_index = len(sub_parts)
436+
if self.get_from_part(affected_index) >= key:
437+
self._increment_from_part(affected_index)
408438
else:
409439
key -= 1
410-
if self.path == path:
411-
if self.key > key:
412-
self.key += 1
440+
if _is_prefix(sub_parts, self.pointer.parts):
441+
affected_index = len(sub_parts)
442+
if self.get_part(affected_index) > key:
443+
self._increment_part(affected_index)
413444
else:
414445
key += 1
415446
return key
416447

417-
def _on_undo_add(self, path, key):
418-
if self.from_path == path:
419-
if self.from_key > key:
420-
self.from_key -= 1
448+
def _on_undo_add(self, sub_parts, key):
449+
from_ptr = self.pointer_cls(self.operation['from'])
450+
if _is_prefix(sub_parts, from_ptr.parts):
451+
affected_index = len(sub_parts)
452+
if self.get_from_part(affected_index) > key:
453+
self._decrement_from_part(affected_index)
421454
else:
422455
key -= 1
423-
if self.path == path:
424-
if self.key > key:
425-
self.key -= 1
456+
if _is_prefix(sub_parts, self.pointer.parts):
457+
affected_index = len(sub_parts)
458+
if self.get_part(affected_index) > key:
459+
self._decrement_part(affected_index)
426460
else:
427461
key += 1
428462
return key
@@ -778,7 +812,7 @@ def _item_added(self, path, key, item):
778812
op = index[2]
779813
if type(op.key) == int and type(key) == int:
780814
for v in self.iter_from(index):
781-
op.key = v._on_undo_remove(op.path, op.key)
815+
op.key = v._on_undo_remove(op.pointer.parts[:-1], op.key)
782816

783817
self.remove(index)
784818
if op.location != _path_join(path, key):
@@ -813,7 +847,7 @@ def _item_removed(self, path, key, item):
813847
added_item = op.pointer.to_last(self.dst_doc)[0]
814848
if type(added_item) == list:
815849
for v in self.iter_from(index):
816-
op.key = v._on_undo_add(op.path, op.key)
850+
op.key = v._on_undo_add(op.pointer.parts[:-1], op.key)
817851

818852
self.remove(index)
819853
if new_op.location != op.location:
@@ -908,3 +942,6 @@ def _path_join(path, key):
908942
return path
909943

910944
return path + '/' + str(key).replace('~', '~0').replace('/', '~1')
945+
946+
def _is_prefix(sub_parts, parts):
947+
return sub_parts == parts[:len(sub_parts)]

‎tests.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,58 @@ def test_issue120(self):
627627
res = jsonpatch.apply_patch(src, patch)
628628
self.assertEqual(res, dst)
629629

630+
def test_issue_138(self):
631+
"""
632+
The _on_undo methods should update its operation's path if it is
633+
affected by the removal of a prior operation.
634+
"""
635+
old = [
636+
{"x": ["a", {"y": ["b"]}], "z": "a"},
637+
{"x": ["c", {"d": ["d"]}], "z": "c"},
638+
{},
639+
]
640+
new = [
641+
{"x": ["c", {"y": ["d"]}], "z": "c"},
642+
{},
643+
]
644+
patch = jsonpatch.make_patch(old, new)
645+
result = jsonpatch.apply_patch(old, patch)
646+
self.assertEqual(result, new)
647+
648+
def test_issue_138b(self):
649+
"""Additionally tests escaping special characters."""
650+
old = {"/":
651+
[
652+
{"x": ["a", {"y": ["b"]}], "z": "a"},
653+
{"x": ["c", {"d": ["d"]}], "z": "c"},
654+
{},
655+
]
656+
}
657+
new = {"/":
658+
[
659+
{"x": ["c", {"y": ["d"]}], "z": "c"},
660+
{},
661+
]
662+
}
663+
patch = jsonpatch.make_patch(old, new)
664+
result = jsonpatch.apply_patch(old, patch)
665+
self.assertEqual(result, new)
666+
667+
operation = jsonpatch.RemoveOperation({
668+
'op': 'remove', 'path': '/~1/0/x/1/y/0',
669+
})
670+
operation._on_undo_add(['/', '0', 'x'], 0)
671+
self.assertEqual(operation.location, '/~1/0/x/0/y/0')
672+
673+
674+
def test_issue_124(self):
675+
"""Similar to issue 138, but for different operations."""
676+
old = ['a', 'b', ['d', 'e'], 'f']
677+
new = ['a', 'd', ['e', 'g']]
678+
patch = jsonpatch.make_patch(old, new)
679+
result = jsonpatch.apply_patch(old, patch)
680+
self.assertEqual(result, new)
681+
630682
def test_custom_types_diff(self):
631683
old = {'value': decimal.Decimal('1.0')}
632684
new = {'value': decimal.Decimal('1.00')}

0 commit comments

Comments
 (0)