Improve list diffing with sequence alignment to avoid cascading changes

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

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):

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):