From 7f92a3ffb413c0116d16e59540d166cad2446491 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 21:36:36 +0000 Subject: [PATCH 1/2] Align list items before diffing them (#83) make_patch compared list items by index, so inserting or removing an item made every item after it look changed. Move detection undid some of that for scalars, but lists of objects or arrays were rebuilt field by field: inserting one object at the front of a list of 2000 gave 4003 operations instead of one 'add'. Lists are now aligned before their items are compared. A common prefix and suffix are skipped, and difflib.SequenceMatcher aligns the rest. SequenceMatcher ignores items that occur often in long lists, so its alignment is only used if it changes fewer items than comparing them by index. Items are matched by a key that follows the existing notion of equality: leaves are compared by their JSON dump, so 1 and true stay different, and the key order of objects does not matter. This re-enables test_should_just_add_new_item_not_rebuild_all_list, which was disabled when the DiffBuilder was introduced. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R62pG28eF1nRucQY4KsxoJ --- jsonpatch.py | 120 ++++++++++++++++++++++++++++++++++++++++----------- tests.py | 56 ++++++++++++++++++++++-- 2 files changed, 147 insertions(+), 29 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index ea8be34..dabf99d 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -34,6 +34,7 @@ import collections import copy +import difflib import functools import json from collections.abc import MutableMapping, MutableSequence, Sequence @@ -826,37 +827,58 @@ def _compare_dicts(self, path, src, dst): for key in intersection: self._compare_values(path, str(key), src[key], dst[key]) - def _compare_lists(self, path, src, dst): - len_src, len_dst = len(src), len(dst) - max_len = max(len_src, len_dst) - min_len = min(len_src, len_dst) - for key in range(max_len): - if key < min_len: - old, new = src[key], dst[key] - if isinstance(old, MutableMapping) and \ - isinstance(new, MutableMapping): - self._compare_dicts(_path_join(path, key), old, new) - - elif isinstance(old, MutableSequence) and \ - isinstance(new, MutableSequence): - self._compare_lists(_path_join(path, key), old, new) - - # To ensure we catch changes to JSON, we can't rely on a - # simple old == new, because it would not recognize the - # difference between 1 and True, among other things. - elif self.dumps(old) == self.dumps(new): - continue + def _item_key(self, item): + """ A hashable key of item, which is the same for items between which + the diff finds no changes """ + if isinstance(item, MutableMapping): + return frozenset((str(key), self._item_key(value)) + for key, value in item.items()) - else: - self._item_removed(path, key, old) - self._item_added(path, key, new) + if isinstance(item, MutableSequence): + return tuple(self._item_key(value) for value in item) - elif len_src > len_dst: - self._item_removed(path, len_dst, src[key]) + return self.dumps(item) - else: + def _compare_lists(self, path, src, dst): + # Items are aligned first, so that inserting or removing one does not + # make all items after it look changed + ids = {} + src_ids = [ids.setdefault(self._item_key(item), len(ids)) + for item in src] + dst_ids = [ids.setdefault(self._item_key(item), len(ids)) + for item in dst] + + for i1, i2, j1, j2 in _changed_blocks(src_ids, dst_ids): + # src[:i1] has been changed to dst[:j1] already, so src[i1] is + # at index j1 now + common = min(i2 - i1, j2 - j1) + for offset in range(common): + if src_ids[i1 + offset] != dst_ids[j1 + offset]: + self._compare_items(path, j1 + offset, src[i1 + offset], + dst[j1 + offset]) + + for item in src[i1 + common:i2]: + self._item_removed(path, j1 + common, item) + + for key in range(j1 + common, j2): self._item_added(path, key, dst[key]) + def _compare_items(self, path, key, old, new): + if isinstance(old, MutableMapping) and \ + isinstance(new, MutableMapping): + self._compare_dicts(_path_join(path, key), old, new) + + elif isinstance(old, MutableSequence) and \ + isinstance(new, MutableSequence): + self._compare_lists(_path_join(path, key), old, new) + + # To ensure we catch changes to JSON, we can't rely on a simple + # old == new, because it would not recognize the difference between + # 1 and True, among other things. + elif self.dumps(old) != self.dumps(new): + self._item_removed(path, key, old) + self._item_added(path, key, new) + def _compare_values(self, path, key, src, dst): if isinstance(src, MutableMapping) and \ isinstance(dst, MutableMapping): @@ -950,6 +972,52 @@ def _item_after(location, parts, inserted): return location +def _changed_blocks(src, dst): + """ Aligns the sequences src and dst, and returns the blocks + (i1, i2, j1, j2) in which src[i1:i2] has to change into dst[j1:j2] """ + min_len = min(len(src), len(dst)) + start = 0 + while start < min_len and src[start] == dst[start]: + start += 1 + + end = 0 + while end < min_len - start and src[-1 - end] == dst[-1 - end]: + end += 1 + + src_end, dst_end = len(src) - end, len(dst) - end + # comparing the items at the same index + positional = [(start, src_end, start, dst_end)] + if start in (src_end, dst_end): + # items are only inserted or only removed + return positional + + matcher = difflib.SequenceMatcher(None, src[start:src_end], + dst[start:dst_end]) + aligned = [(start + i1, start + i2, start + j1, start + j2) + for tag, i1, i2, j1, j2 in matcher.get_opcodes() + if tag != 'equal'] + + # SequenceMatcher ignores items that occur often in long sequences, so + # its alignment can change more items than comparing them by index + if _changed_items(src, dst, aligned) < \ + _changed_items(src, dst, positional): + return aligned + + return positional + + +def _changed_items(src, dst, blocks): + """ How many items are inserted, removed or replaced in blocks """ + count = 0 + for i1, i2, j1, j2 in blocks: + common = min(i2 - i1, j2 - j1) + count += max(i2 - i1, j2 - j1) - common + count += sum(src[i1 + offset] != dst[j1 + offset] + for offset in range(common)) + + return count + + def _to_last(pointer, doc): """Resolve pointer like JsonPointer.to_last, without indexing into strings. diff --git a/tests.py b/tests.py index d107957..f955d23 100755 --- a/tests.py +++ b/tests.py @@ -481,9 +481,7 @@ def test_add_nested(self): } self.assertEqual(expected, res) - # TODO: this test is currently disabled, as the optimized patch is - # not ideal - def _test_should_just_add_new_item_not_rebuild_all_list(self): + def test_should_just_add_new_item_not_rebuild_all_list(self): src = {'foo': [1, 2, 3]} dst = {'foo': [3, 1, 2, 3]} patch = list(jsonpatch.make_patch(src, dst)) @@ -902,6 +900,58 @@ def test_minimal_patch(self): self.assertEqual(patch.patch, exp) + def test_insert_into_list_of_objects(self): + """ Inserting an object does not change the ones after it, see #83 """ + src = {'items': [{'id': 1, 'name': 'a'}, {'id': 2, 'name': 'b'}]} + dst = {'items': [{'id': 0, 'name': 'z'}, {'id': 1, 'name': 'a'}, + {'id': 2, 'name': 'b'}]} + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'add', 'path': '/items/0', + 'value': {'id': 0, 'name': 'z'}}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + + def test_remove_from_list_of_objects(self): + src = [{'id': 1}, {'id': 2}, {'id': 3}, {'id': 4}] + dst = [{'id': 1}, {'id': 3}] + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'remove', 'path': '/1'}, {'op': 'remove', 'path': '/2'}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + + def test_insert_and_remove_in_list(self): + src = ['a', 'b', 'c', 'd', 'e'] + dst = ['x', 'a', 'b', 'd', 'e'] + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'add', 'path': '/0', 'value': 'x'}, + {'op': 'remove', 'path': '/3'}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + + def test_insert_into_long_list_of_repeated_values(self): + src = [i % 2 == 0 for i in range(500)] + dst = [False] + src + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'add', 'path': '/0', 'value': False}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + + def test_list_items_are_compared_by_index_if_aligning_is_worse(self): + # aligning on the 1 would move all the 0s + src = [0] * 300 + [1] + dst = [1] + [0] * 300 + patch = jsonpatch.make_patch(src, dst) + self.assertLessEqual(len(patch.patch), 2) + self.assertEqual(patch.apply(src), dst) + + def test_list_alignment_ignores_key_order(self): + src = [{'a': 1, 'b': 2}] + dst = [0, {'b': 2, 'a': 1}] + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'add', 'path': '/0', 'value': 0}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + class ListTests(unittest.TestCase): From 50d107a616ece623129d6c0a4792028746f611d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 13:35:59 +0000 Subject: [PATCH 2/2] Match moves by JSON equality and bound the list alignment work Move detection stored values by (value, type(value)) and compared unhashable ones with ==, so [1] and [true] were considered equal and one was moved where the other belongs. Aligning lists made this reachable from more inputs, e.g. [[1], [2], [3]] -> [[2], [3], [true]]. Values are now stored by the same JSON-aware key that aligns list items (#180). Values that dumps cannot serialize get a key equal only to themselves, so they can still be added, removed and moved as before. SequenceMatcher compares each item with the equal items of the other list, which took 22 seconds for a list of 100,000 repeated numbers whose first and last items were replaced. Aligning is now skipped if that takes more comparisons than comparing by index would take steps, which is quadratic in the number of items changed by index, as the diff turns them into moves, with a floor for lists that differ in few items. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R62pG28eF1nRucQY4KsxoJ --- jsonpatch.py | 55 +++++++++++++++++++++++++---------------------- property_tests.py | 14 +++++------- tests.py | 53 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 35 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index dabf99d..4371e6e 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -46,6 +46,10 @@ _ST_ADD = 0 _ST_REMOVE = 1 +# How many comparisons of equal items make_patch may spend on aligning the +# items of two arrays, if they differ in only a few items by index +_MAX_ALIGNMENT_COMPARISONS = 10 ** 5 + # Will be parsed by setup.py to determine package metadata __author__ = 'Stefan Kögl ' @@ -651,37 +655,21 @@ def __init__(self, src_doc, dst_doc, dumps=json.dumps, pointer_cls=JsonPointer): self.dumps = dumps self.pointer_cls = pointer_cls self.index_storage = [{}, {}] - self.index_storage2 = [[], []] self.__root = root = [] self.src_doc = src_doc self.dst_doc = dst_doc root[:] = [root, root, None] + # Values are stored by _item_key, so that only values which are equal in + # JSON are moved instead of removed and added (e.g. not [1] and [true]) def store_index(self, value, index, st): - typed_key = (value, type(value)) - try: - storage = self.index_storage[st] - stored = storage.get(typed_key) - if stored is None: - storage[typed_key] = [index] - else: - storage[typed_key].append(index) - - except TypeError: - self.index_storage2[st].append((typed_key, index)) + storage = self.index_storage[st] + storage.setdefault(self._item_key(value), []).append(index) def take_index(self, value, st): - typed_key = (value, type(value)) - try: - stored = self.index_storage[st].get(typed_key) - if stored: - return stored.pop() - - except TypeError: - storage = self.index_storage2[st] - for i in range(len(storage)-1, -1, -1): - if storage[i][0] == typed_key: - return storage.pop(i)[1] + stored = self.index_storage[st].get(self._item_key(value)) + if stored: + return stored.pop() def insert(self, op): root = self.__root @@ -837,7 +825,12 @@ def _item_key(self, item): if isinstance(item, MutableSequence): return tuple(self._item_key(value) for value in item) - return self.dumps(item) + try: + return self.dumps(item) + except TypeError: + # dumps cannot compare values it cannot serialize, which can + # still be added, removed or moved as long as they are not changed + return id(item) def _compare_lists(self, path, src, dst): # Items are aligned first, so that inserting or removing one does not @@ -991,6 +984,17 @@ def _changed_blocks(src, dst): # items are only inserted or only removed return positional + # SequenceMatcher compares each item with the equal items of the other + # sequence, which takes quadratic time for long sequences of repeated + # items. Comparing items by index takes quadratic time in the number of + # changed items, as the diff tries to turn them into moves, so aligning is + # only allowed to take about as long + changed = _changed_items(src, dst, positional) + counts = collections.Counter(dst[start:dst_end]) + comparisons = sum(counts[item] for item in src[start:src_end]) + if comparisons > max(_MAX_ALIGNMENT_COMPARISONS, changed ** 2): + return positional + matcher = difflib.SequenceMatcher(None, src[start:src_end], dst[start:dst_end]) aligned = [(start + i1, start + i2, start + j1, start + j2) @@ -999,8 +1003,7 @@ def _changed_blocks(src, dst): # SequenceMatcher ignores items that occur often in long sequences, so # its alignment can change more items than comparing them by index - if _changed_items(src, dst, aligned) < \ - _changed_items(src, dst, positional): + if _changed_items(src, dst, aligned) < changed: return aligned return positional diff --git a/property_tests.py b/property_tests.py index 6517682..87c1848 100644 --- a/property_tests.py +++ b/property_tests.py @@ -163,16 +163,14 @@ def json_values(scalars, keys): def safe_json_values(scalars, keys): """ Documents that avoid the inputs on which make_patch is currently known - to fail (see test_roundtrip): booleans and object keys that are '-' """ - return json_values( - scalars.filter(lambda value: not isinstance(value, bool)), - keys.filter(lambda key: key != '-'), - ) + to fail (see test_roundtrip): object keys that are '-' """ + return json_values(scalars, keys.filter(lambda key: key != '-')) safe_json_docs = safe_json_values(json_scalars, json_keys) -small_safe_json_docs = safe_json_values(st.sampled_from([None, 0, 1, 'a']), - st.sampled_from(['a', 'b', '0', '1'])) +small_safe_json_docs = safe_json_values( + st.sampled_from([None, True, False, 0, 1, 'a']), + st.sampled_from(['a', 'b', '0', '1'])) def pairs_of(*doc_strategies): @@ -288,8 +286,6 @@ def test_diff_of_equal_documents_is_empty(self, doc): @unittest.expectedFailure @given(doc_pairs) - # move detection considers e.g. [1] and [true] equal, #180 - @example(docs=({'a': [1]}, {'b': [True]})) # replace of the object member '-' is rejected @example(docs=({'-': 0}, {'-': 1})) def test_roundtrip(self, docs): diff --git a/tests.py b/tests.py index f955d23..7aa69c7 100755 --- a/tests.py +++ b/tests.py @@ -10,6 +10,7 @@ import jsonpointer import sys from types import MappingProxyType +from unittest import mock class ApplyPatchTestCase(unittest.TestCase): @@ -617,6 +618,37 @@ def test_issue180(self): self.assertIsInstance(res['aaa'][1], bool) self.assertIsInstance(res['aaa'][2], bool) + def test_move_only_values_equal_in_json(self): + """[1] and [true] are equal in Python, so the diff moved one to where + the other belongs""" + cases = [ + ([[1], [2], [3]], [[2], [3], [True]]), + ([{'a': 1}, 2], [2, {'a': True}]), + ({'a': [1]}, {'b': [True]}), + ] + for src, dst in cases: + with self.subTest(src=src, dst=dst): + patch = jsonpatch.make_patch(src, dst) + res = patch.apply(src) + self.assertEqual(json.dumps(res), json.dumps(dst)) + + def test_values_dumps_cannot_serialize(self): + """Such values can be added, removed and moved, as they need not be + compared""" + value = decimal.Decimal('1.5') + cases = [ + ({}, {'a': value}), + ([1], [value, 1]), + ({'a': [value]}, {}), + ({'a': value}, {'b': value}), + ] + for src, dst in cases: + with self.subTest(src=src, dst=dst): + patch = jsonpatch.make_patch(src, dst) + self.assertEqual(patch.apply(src), dst) + self.assertEqual(jsonpatch.make_patch(*cases[-1]).patch, + [{'op': 'move', 'from': '/a', 'path': '/b'}]) + def test_issue119(self): """Make sure it avoids casting numeric str dict key to int""" src = [ @@ -944,6 +976,27 @@ def test_list_items_are_compared_by_index_if_aligning_is_worse(self): self.assertLessEqual(len(patch.patch), 2) self.assertEqual(patch.apply(src), dst) + def test_long_list_of_repeated_items_is_compared_by_index(self): + # aligning would compare each item with about 133 equal items, while + # only two items differ by index + src = [i % 150 for i in range(20000)] + dst = [-1] + src[1:-1] + [-2] + with mock.patch('difflib.SequenceMatcher', + side_effect=AssertionError('aligned')): + patch = jsonpatch.make_patch(src, dst) + self.assertEqual(len(patch.patch), 2) + self.assertEqual(patch.apply(src), dst) + + def test_shifted_long_list_of_repeated_items_is_aligned(self): + # comparing by index would change all 6000 items + src = [i % 150 for i in range(6000)] + dst = [-1] + src[:-1] + patch = jsonpatch.make_patch(src, dst) + exp = [{'op': 'add', 'path': '/0', 'value': -1}, + {'op': 'remove', 'path': '/6000'}] + self.assertEqual(patch.patch, exp) + self.assertEqual(patch.apply(src), dst) + def test_list_alignment_ignores_key_order(self): src = [{'a': 1, 'b': 2}] dst = [0, {'b': 2, 'a': 1}]