Fix patch order for interdependent move operations

Co-authored-by: stefankoegl <[email protected]>

Author: Copilot

jsonpatch.py

@@ -774,24 +774,91 @@ def __iter__(self):
             curr = curr[1]
 
     def execute(self):
+        # First, collect all operations
+        operations = []
         root = self.__root
         curr = root[1]
+        
+        # First pass: collect operations and handle replace optimizations
         while curr is not root:
             if curr[1] is not root:
                 op_first, op_second = curr[2], curr[1][2]
                 if op_first.location == op_second.location and \
                         type(op_first) == RemoveOperation and \
                         type(op_second) == AddOperation:
-                    yield ReplaceOperation({
+                    operations.append(ReplaceOperation({
                         'op': 'replace',
                         'path': op_second.location,
                         'value': op_second.operation['value'],
-                    }, pointer_cls=self.pointer_cls).operation
+                    }, pointer_cls=self.pointer_cls).operation)
                     curr = curr[1][1]
                     continue
-
-            yield curr[2].operation
+            
+            operations.append(curr[2].operation)
             curr = curr[1]
+        
+        # Second pass: identify and sort move operations
+        move_indices = []
+        move_ops = []
+        
+        for i, op in enumerate(operations):
+            if op['op'] == 'move':
+                move_indices.append(i)
+                move_ops.append(op)
+        
+        # Sort move operations based on dependencies
+        if move_ops:
+            # Create dependency graph: if op1's target is op2's source, op2 depends on op1
+            dependencies = {}
+            for i, op1 in enumerate(move_ops):
+                dependencies[i] = []
+                src1 = op1['from']
+                tgt1 = op1['path']
+                
+                for j, op2 in enumerate(move_ops):
+                    if i == j:
+                        continue  # Skip self-comparison
+                    
+                    src2 = op2['from']
+                    
+                    # If op2's source is the same as op1's target, op2 should come after op1
+                    if src2 == tgt1 or src2.startswith(tgt1 + '/'):
+                        dependencies[i].append(j)
+            
+            # Topological sort to determine execution order
+            # We're using a simple approach: if op1 depends on op2, op2 should be executed first
+            sorted_indices = []
+            visited = set()
+            temp_visited = set()
+            
+            def visit(i):
+                if i in temp_visited:
+                    # Cyclic dependency - maintain original order
+                    return
+                if i in visited:
+                    return
+                
+                temp_visited.add(i)
+                
+                for j in dependencies.get(i, []):
+                    visit(j)
+                
+                temp_visited.remove(i)
+                visited.add(i)
+                sorted_indices.append(i)
+            
+            for i in range(len(move_ops)):
+                if i not in visited:
+                    visit(i)
+            
+            # Replace original move operations with sorted ones
+            sorted_move_ops = [move_ops[i] for i in sorted_indices]
+            for idx, op in zip(move_indices, sorted_move_ops):
+                operations[idx] = op
+        
+        # Yield operations
+        for op in operations:
+            yield op
 
     def _item_added(self, path, key, item):
         index = self.take_index(item, _ST_REMOVE)

patch_test.py

@@ -0,0 +1,15 @@
+import jsonpatch
+
+src_obj = {'a': [{'id': [1]}, {'id': [2]}], 'b': [{'id': 5}]}
+tgt_obj = {'a': [{'id': []}, {'id': [1]}], 'b': [{'id': 5, 'newKey': 2}]}
+
+patch = jsonpatch.make_patch(src_obj, tgt_obj)
+print(patch)
+
+# Check normal application of the patch
+tgt_obj_check = jsonpatch.apply_patch(src_obj, patch)
+print('Forward', 'OK' if len(jsonpatch.make_patch(tgt_obj, tgt_obj_check).patch) == 0 else 'ERROR', '->', tgt_obj_check)
+
+# Check reverse application of the patch
+tgt_obj_check = jsonpatch.apply_patch(src_obj, patch.patch[::-1])
+print('Reverse', 'OK' if len(jsonpatch.make_patch(tgt_obj, tgt_obj_check).patch) == 0 else 'ERROR', '->', tgt_obj_check)

tests.py

@@ -455,6 +455,24 @@ def test_arrays_one_element_sequences(self):
         patch = jsonpatch.make_patch(src, dst)
         res = jsonpatch.apply_patch(src, patch)
         self.assertEqual(res, dst)
+        
+    def test_move_operations_order(self):
+        """Test that move operations are properly ordered to ensure correct application"""
+        src_obj = {'a': [{'id': [1]}, {'id': [2]}], 'b': [{'id': 5}]}
+        tgt_obj = {'a': [{'id': []}, {'id': [1]}], 'b': [{'id': 5, 'newKey': 2}]}
+        
+        # Generate patch
+        patch = jsonpatch.make_patch(src_obj, tgt_obj)
+        
+        # Apply patch forward and check result
+        result = jsonpatch.apply_patch(src_obj, patch)
+        self.assertEqual(result, tgt_obj, "Patch application should produce the expected target object")
+        
+        # Apply patch in reverse and verify it fails (to confirm we're testing the right scenario)
+        patch_ops = list(patch)
+        reverse_patch = jsonpatch.JsonPatch(patch_ops[::-1])
+        reverse_result = jsonpatch.apply_patch(src_obj, reverse_patch)
+        self.assertNotEqual(reverse_result, tgt_obj, "Reverse patch should not work - confirms we're testing the right issue")
 
     def test_list_in_dict(self):
         """ Test patch creation with a list within a dict, as reported in #74