From 80086cd29fb8edf397d7429e3833bf1149e19494 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 21:26:50 +0000 Subject: [PATCH 1/3] Only move values that are the same in JSON (#180) make_patch found moved values by looking up (value, type(value)) in a dict, or with == for unhashable values. Python considers e.g. [1] and [True], {'x': 1} and {'x': True}, or 0.0 and -0.0 equal, so a removed value could be moved to where a different one was added, and the patch did not produce the target document. Move detection now keys values the way _compare_values compares them: by their serialization with the diff's dumps, with objects and arrays taken member by member so that the order of object members still does not matter. Values that dumps cannot serialize are added and removed instead of moved, so make_patch still accepts them. The key is computed once per added or removed value, and as all keys are hashable the linear search for unhashable values is gone. The {'a': [1]} -> {'b': [True]} example of test_roundtrip passes now and is dropped; the test still fails for the object member '-'. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn --- jsonpatch.py | 62 ++++++++++++++++++++++++++--------------------- property_tests.py | 2 -- tests.py | 39 +++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 29 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index ea8be34..02661ef 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -650,37 +650,28 @@ 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] - def store_index(self, value, index, st): - typed_key = (value, type(value)) + def index_key(self, value): + """ A key that two values share exactly when comparing them finds no + changes, so that a value is only moved to where it is the same. It is + None for values that cannot be serialized, which are not moved. """ 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)) + return _serialized_key(value, self.dumps) + except (TypeError, ValueError): + return None - 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() + def store_index(self, key, index, st): + if key is not None: + self.index_storage[st].setdefault(key, []).append(index) - 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] + def take_index(self, key, st): + stored = self.index_storage[st].get(key) + if stored: + return stored.pop() def insert(self, op): root = self.__root @@ -769,7 +760,8 @@ def _adjust_following(self, index, removed): def _item_added(self, path, key, item): target = _path_join(path, key) - index = self.take_index(item, _ST_REMOVE) + index_key = self.index_key(item) + index = self.take_index(index_key, _ST_REMOVE) if index is not None: changes, source = self._adjust_following(index, removed=True) # RFC 6902 does not allow moving a value into its own children @@ -783,11 +775,12 @@ def _item_added(self, path, key, item): return new_index = self.insert({'op': 'add', 'path': target, 'value': item}) - self.store_index(item, new_index, _ST_ADD) + self.store_index(index_key, new_index, _ST_ADD) def _item_removed(self, path, key, item): source = _path_join(path, key) - index = self.take_index(item, _ST_ADD) + index_key = self.index_key(item) + index = self.take_index(index_key, _ST_ADD) if index is not None: changes, added = self._adjust_following(index, removed=False) moved_from = _without_item(source, added) @@ -803,7 +796,7 @@ def _item_removed(self, path, key, item): return new_index = self.insert({'op': 'remove', 'path': source}) - self.store_index(item, new_index, _ST_REMOVE) + self.store_index(index_key, new_index, _ST_REMOVE) def _item_replaced(self, path, key, item): self.insert({ @@ -880,6 +873,21 @@ def _compare_values(self, path, key, src, dst): self._item_replaced(path, key, dst) +def _serialized_key(value, dumps): + """ Python considers e.g. 1 and True equal, so values themselves cannot + be the keys that find moved values (#180). Like _compare_values, the key + uses serialized values, but objects and arrays are compared member by + member there, so the order of object members does not matter here. """ + if isinstance(value, MutableMapping): + return frozenset((key, _serialized_key(item, dumps)) + for key, item in value.items()) + + if isinstance(value, MutableSequence): + return tuple(_serialized_key(item, dumps) for item in value) + + return dumps(value) + + # The DiffBuilder keeps locations as tuples of object keys (str) and array # indices (int), so it can tell them apart when it adjusts array indices diff --git a/property_tests.py b/property_tests.py index 6517682..9716fb2 100644 --- a/property_tests.py +++ b/property_tests.py @@ -288,8 +288,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 d107957..21ef647 100755 --- a/tests.py +++ b/tests.py @@ -2,6 +2,7 @@ # -*- coding: utf-8 -*- import copy +import datetime import json import decimal import doctest @@ -619,6 +620,38 @@ def test_issue180(self): self.assertIsInstance(res['aaa'][1], bool) self.assertIsInstance(res['aaa'][2], bool) + def test_issue180_moves(self): + """Values are only moved where they are the same in JSON, even though + in python e.g. [1] == [True]""" + cases = [ + ({'a': [1]}, {'b': [True]}), + ({'a': {'x': 1}}, {'b': {'x': True}}), + ([[1], 0], [0, [True]]), + ({'a': [1]}, {'b': [1.0]}), + ({'a': 0.0}, {'b': -0.0}), + ] + for src, dst in cases: + with self.subTest(src=src, dst=dst): + patch = jsonpatch.make_patch(src, dst) + res = jsonpatch.apply_patch(src, patch) + self.assertEqual(json.dumps(res), json.dumps(dst)) + + def test_issue180_moves_custom_types(self): + """The given dumps decides which values are the same""" + src = {'a': decimal.Decimal('1.0')} + dst = {'b': decimal.Decimal('1.00')} + patch = jsonpatch.JsonPatch.from_diff( + src, dst, dumps=custom_types_dumps) + res = jsonpatch.apply_patch(src, patch) + self.assertEqual(str(res['b']), '1.00') + + def test_values_that_cannot_be_serialized(self): + """Values that dumps rejects are added and removed instead of moved""" + src = {'a': [datetime.date(2020, 1, 1)]} + dst = {'b': [datetime.date(2020, 1, 1)]} + patch = jsonpatch.make_patch(src, dst) + self.assertEqual(jsonpatch.apply_patch(src, patch), dst) + def test_issue119(self): """Make sure it avoids casting numeric str dict key to int""" src = [ @@ -848,6 +881,12 @@ def fn(_src, _dst): fn({'foo': [1, 2, 3]}, {'foo': [3, 2, 1]}) fn([1, 2, 3], [3, 2, 1]) + def test_use_move_regardless_of_member_order(self): + src = {'a': {'x': 1, 'y': [True, {}]}} + dst = {'b': {'y': [True, {}], 'x': 1}} + patch = list(jsonpatch.make_patch(src, dst)) + self.assertEqual(patch, [{'op': 'move', 'from': '/a', 'path': '/b'}]) + def test_success_if_replace_inside_dict(self): src = [{'a': 1, 'foo': {'b': 2, 'd': 5}}] dst = [{'a': 1, 'foo': {'b': 3, 'd': 6}}] From d89b2e48b05882eb606092dc49b89daa60f0ff60 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 13:09:13 +0000 Subject: [PATCH 2/3] Cover booleans in the passing roundtrip property The safe documents left out booleans only because make_patch confused them with numbers (#180). Now that it does not, they are generated again, and the counterexamples of both halves of #180 are pinned on test_roundtrip_of_safe_documents, as random search finds them rarely. The test for values that cannot be serialized now checks that they are removed and added, as applying a move would give the same document. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn --- property_tests.py | 15 ++++++++------- tests.py | 1 + 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/property_tests.py b/property_tests.py index 9716fb2..9e92ebf 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): @@ -294,6 +292,9 @@ def test_roundtrip(self, docs): self.check_roundtrip(*docs) @given(safe_doc_pairs) + # the diff considered e.g. 1 and true equal, #180 + @example(docs=([0], [False])) + @example(docs=({'a': [1]}, {'b': [True]})) def test_roundtrip_of_safe_documents(self, docs): self.check_roundtrip(*docs) diff --git a/tests.py b/tests.py index 21ef647..9a2a29e 100755 --- a/tests.py +++ b/tests.py @@ -650,6 +650,7 @@ def test_values_that_cannot_be_serialized(self): src = {'a': [datetime.date(2020, 1, 1)]} dst = {'b': [datetime.date(2020, 1, 1)]} patch = jsonpatch.make_patch(src, dst) + self.assertEqual([op['op'] for op in patch], ['remove', 'add']) self.assertEqual(jsonpatch.apply_patch(src, patch), dst) def test_issue119(self): From 2ee075f4f544e231785387fd12174dd2a8756d08 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 15:57:01 +0000 Subject: [PATCH 3/3] Serialize member names and catch recursion in move keys The keys that find moved values used member names as they are, so {1: 'v'} and {True: 'v'} shared a key although they serialize as {"1": "v"} and {"true": "v"}. Member names that are not strings are serialized now. Values that contain themselves made building their key recurse until RecursionError, which crashed make_patch. Like other values that cannot be serialized, they are now added and removed instead of moved. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn --- jsonpatch.py | 14 +++++++++----- tests.py | 9 +++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/jsonpatch.py b/jsonpatch.py index 02661ef..9e16b9d 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -656,12 +656,13 @@ def __init__(self, src_doc, dst_doc, dumps=json.dumps, pointer_cls=JsonPointer): root[:] = [root, root, None] def index_key(self, value): - """ A key that two values share exactly when comparing them finds no - changes, so that a value is only moved to where it is the same. It is - None for values that cannot be serialized, which are not moved. """ + """ A key that two values share only if they are the same in JSON, so + that a value is only moved to where it is the same. It is None for + values that cannot be serialized, which are not moved. """ try: return _serialized_key(value, self.dumps) - except (TypeError, ValueError): + # values that contain themselves recurse until RecursionError + except (TypeError, ValueError, RecursionError): return None def store_index(self, key, index, st): @@ -879,7 +880,10 @@ def _serialized_key(value, dumps): uses serialized values, but objects and arrays are compared member by member there, so the order of object members does not matter here. """ if isinstance(value, MutableMapping): - return frozenset((key, _serialized_key(item, dumps)) + # JSON member names are strings: 1 and True become "1" and "true", + # which differ although Python considers 1 and True equal + return frozenset((key if isinstance(key, str) else dumps(key), + _serialized_key(item, dumps)) for key, item in value.items()) if isinstance(value, MutableSequence): diff --git a/tests.py b/tests.py index 9a2a29e..f735c0b 100755 --- a/tests.py +++ b/tests.py @@ -629,6 +629,8 @@ def test_issue180_moves(self): ([[1], 0], [0, [True]]), ({'a': [1]}, {'b': [1.0]}), ({'a': 0.0}, {'b': -0.0}), + # member names that are not strings are serialized, too + ({'a': {1: 'v'}}, {'b': {True: 'v'}}), ] for src, dst in cases: with self.subTest(src=src, dst=dst): @@ -653,6 +655,13 @@ def test_values_that_cannot_be_serialized(self): self.assertEqual([op['op'] for op in patch], ['remove', 'add']) self.assertEqual(jsonpatch.apply_patch(src, patch), dst) + def test_values_that_contain_themselves(self): + """Values that contain themselves cannot be serialized either""" + value = [] + value.append(value) + patch = jsonpatch.make_patch({'a': value}, {'b': value}) + self.assertEqual([op['op'] for op in patch], ['remove', 'add']) + def test_issue119(self): """Make sure it avoids casting numeric str dict key to int""" src = [