diff --git a/jsonpatch.py b/jsonpatch.py index a627ed4..8e67b4c 100644 --- a/jsonpatch.py +++ b/jsonpatch.py @@ -658,37 +658,30 @@ 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 _move_key(self, value): + """ A key of value, which is equal for values that are equal in JSON, + so that e.g. [1] is not moved where [true] is added """ 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) + # Serialized like in _differing_runs + return self.dumps(_sorted_members(value)) + except (TypeError, ValueError): + # Values that cannot be serialized are only moved where they are + # added unchanged + return id(value) - except TypeError: - self.index_storage2[st].append((typed_key, index)) + def store_index(self, value, index, st): + storage = self.index_storage[st] + storage.setdefault(self._move_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._move_key(value)) + if stored: + return stored.pop() def insert(self, op): root = self.__root @@ -860,29 +853,42 @@ def _differing_runs(self, src, dst): common_end += 1 src_end, dst_end = len(src) - common_end, len(dst) - common_end + # How many operations replacing src[i1:i2] by dst[j1:j2] takes + def cost(i1, i2, j1, j2): + return abs((i2 - i1) - (j2 - j1)) + sum( + _positional_cost(src[i], dst[j]) + for i, j in zip(range(i1, i2), range(j1, j2)) + if src_keys[i] != dst_keys[j]) + + by_position = (start, src_end, start, dst_end) + position_cost = cost(*by_position) + # Matching up all items can take time proportional to the product of # the list lengths. For longer lists SequenceMatcher does not match # up items that are frequent in them, unless next to other matches exact = (src_end - start) * (dst_end - start) <= _EXACT_MATCH_LIMIT + if not exact: + # It still compares each item with the equal items of the other + # list that are not frequent, which takes quadratic time for long + # lists of repeated items. Comparing by position takes quadratic + # time in its operations as well, as the diff turns them into + # moves, so matching up may only take about as long + counts = collections.Counter(dst_keys[start:dst_end]) + comparisons = sum(counts[key] for key in src_keys[start:src_end]) + if comparisons > max(_EXACT_MATCH_LIMIT, position_cost ** 2): + return [by_position] + matcher = SequenceMatcher(None, src_keys[start:src_end], dst_keys[start:dst_end], autojunk=not exact) runs = [(start + i1, start + i2, start + j1, start + j2) for tag, i1, i2, j1, j2 in matcher.get_opcodes() if tag != 'equal'] - # How many operations replacing src[i1:i2] by dst[j1:j2] takes - def cost(i1, i2, j1, j2): - return abs((i2 - i1) - (j2 - j1)) + sum( - _positional_cost(src[i], dst[j]) - for i, j in zip(range(i1, i2), range(j1, j2)) - if src_keys[i] != dst_keys[j]) - # Items can be equal by chance, and matching them up can take more # operations than comparing by position: for [2, 3, 5] and [3, 3, 4] # it removes 2, replaces 5 and adds 4 instead of replacing 2 and 5. # It can also pair up arrays or objects that differ in more members. - by_position = (start, src_end, start, dst_end) - if sum(cost(*run) for run in runs) > cost(*by_position): + if sum(cost(*run) for run in runs) > position_cost: return [by_position] return runs 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 ba31215..8744e2d 100755 --- a/tests.py +++ b/tests.py @@ -8,8 +8,10 @@ import unittest import jsonpatch import jsonpointer +import random import sys from types import MappingProxyType +from unittest import mock class ApplyPatchTestCase(unittest.TestCase): @@ -481,9 +483,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)) @@ -640,6 +640,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], [1, value]), + ({'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 = [ @@ -1003,6 +1034,30 @@ def test_issue_78_items_not_serializable(self): self.assertEqual(jsonpatch.make_patch(src, dst).patch, [{'op': 'replace', 'path': '/0/b', 'value': 1}]) + def test_long_list_of_repeated_items_is_compared_by_position(self): + """ Matching up would compare each item with about 133 equal items, + while only two items differ by position """ + src = [i % 150 for i in range(20000)] + dst = [-1] + src[1:-1] + [-2] + with mock.patch('jsonpatch.SequenceMatcher', + side_effect=AssertionError('matched up')): + patch = jsonpatch.make_patch(src, dst) + self.assertEqual(len(patch.patch), 2) + self.assertEqual(patch.apply(src), dst) + + def test_long_list_of_repeated_items_is_matched_up(self): + """ Comparing by position would change the items between the changes, + as they are shifted """ + rng = random.Random(0) + src = [rng.randrange(150) for _ in range(9000)] + dst = src[:1000] + [-1] + src[1000:5000] + src[5001:8000] + [-2] + \ + src[8000:] + self.assertPatch(src, dst, [ + {'op': 'add', 'path': '/1000', 'value': -1}, + {'op': 'remove', 'path': '/5001'}, + {'op': 'add', 'path': '/8000', 'value': -2}, + ]) + class ListTests(unittest.TestCase):