diff --git a/server/mergin/sync/models.py b/server/mergin/sync/models.py index 64ce4078..2445d99c 100644 --- a/server/mergin/sync/models.py +++ b/server/mergin/sync/models.py @@ -69,10 +69,12 @@ class ChangeComparisonAction(Enum): """Actions to take when comparing two changes""" REPLACE = "replace" - DELETE = "delete" - UPDATE = "update" - UPDATE_DIFF = "update_diff" + UPDATE_METADATA = "update_metadata" # Update metadata and keep diffs (used for update + update sequence) + REPLACE_DIFFS = "replace_diffs" # Replace diffs but keep metadata (used for update + update sequence when only diffs are changed) EXCLUDE = "exclude" # Return None to exclude the file + FORCE_UPDATE = ( + "force_update" # Force update even if it looks like a delete + create sequence + ) class Project(db.Model): @@ -1089,9 +1091,11 @@ def merge_changes( continue # Compare and merge with previous change for this file - can_delete = path in updating_files + # If file did not exist before this range (first change was CREATE), + # a CREATE+DELETE sequence is transparent — exclude the pair. + exclude_delete = path not in updating_files new_change = ProjectVersionDelta._compare_changes( - result[path], current, can_delete + result[path], current, exclude_delete ) # Update result (or remove if no change is detected) @@ -1106,7 +1110,7 @@ def merge_changes( def _compare_changes( previous: DeltaChangeMerged, new: DeltaChangeMerged, - prevent_delete_change: bool, + exclude_delete: bool, ) -> Optional[DeltaChangeMerged]: """ Compare and merge two changes for the same file. @@ -1114,7 +1118,10 @@ def _compare_changes( Args: previous: Previously accumulated change new: New change to compare - prevent_delete_change: Whether the change can be deleted when resolving create+delete sequences + exclude_delete: If True, a CREATE+DELETE pair is excluded (file was + created and deleted within the same range — no net effect). + If False, the DELETE is kept (file existed before this range + and the client needs to remove it). Returns: Merged change or None if file should be excluded @@ -1122,29 +1129,35 @@ def _compare_changes( # Map change type pairs to actions action_map = { - # create + delete = file is transparent for current changes -> delete it + # CREATE + DELETE: if file didn't exist before (exclude_delete=True), + # the pair cancels out (EXCLUDE). If it did exist, keep the DELETE + # so the client removes its local copy. ( PushChangeType.CREATE, PushChangeType.DELETE, - ): ChangeComparisonAction.DELETE, + ): ( + ChangeComparisonAction.EXCLUDE + if exclude_delete + else ChangeComparisonAction.REPLACE + ), # create + update = create with updated info ( PushChangeType.CREATE, PushChangeType.UPDATE, - ): ChangeComparisonAction.UPDATE, + ): ChangeComparisonAction.UPDATE_METADATA, ( PushChangeType.CREATE, PushChangeType.UPDATE_DIFF, - ): ChangeComparisonAction.UPDATE, + ): ChangeComparisonAction.UPDATE_METADATA, ( PushChangeType.CREATE, PushChangeType.CREATE, - ): ChangeComparisonAction.EXCLUDE, + ): ChangeComparisonAction.REPLACE, # update + update_diff = update with latest info ( PushChangeType.UPDATE, PushChangeType.UPDATE_DIFF, - ): ChangeComparisonAction.UPDATE, + ): ChangeComparisonAction.UPDATE_METADATA, ( PushChangeType.UPDATE, PushChangeType.UPDATE, @@ -1161,7 +1174,7 @@ def _compare_changes( ( PushChangeType.UPDATE_DIFF, PushChangeType.UPDATE_DIFF, - ): ChangeComparisonAction.UPDATE_DIFF, + ): ChangeComparisonAction.REPLACE_DIFFS, ( PushChangeType.UPDATE_DIFF, PushChangeType.UPDATE, @@ -1173,16 +1186,16 @@ def _compare_changes( ( PushChangeType.UPDATE_DIFF, PushChangeType.CREATE, - ): ChangeComparisonAction.EXCLUDE, + ): ChangeComparisonAction.REPLACE, ( PushChangeType.DELETE, PushChangeType.CREATE, - ): ChangeComparisonAction.REPLACE, - # delete + update = invalid sequence + ): ChangeComparisonAction.FORCE_UPDATE, + # delete + update = replace it (used for multicheckpoint ranges when we want to keep file in history even if it was deleted in the middle, so we keep delete but update metadata and diffs) ( PushChangeType.DELETE, PushChangeType.UPDATE, - ): ChangeComparisonAction.EXCLUDE, + ): ChangeComparisonAction.REPLACE, ( PushChangeType.DELETE, PushChangeType.UPDATE_DIFF, @@ -1190,27 +1203,28 @@ def _compare_changes( ( PushChangeType.DELETE, PushChangeType.DELETE, - ): ChangeComparisonAction.EXCLUDE, + ): ChangeComparisonAction.REPLACE, } action = action_map.get((previous.change, new.change)) + result = None if action == ChangeComparisonAction.REPLACE: result = new - elif action == ChangeComparisonAction.DELETE: - # if change is create + delete, we can just remove the change from accumulated changes - # only if this action is allowed (file existed before) - if prevent_delete_change: - result = new + elif action == ChangeComparisonAction.FORCE_UPDATE: + # handle force update case, when previous change was delete and new change is create - just revert to update with new metadata + new.change = PushChangeType.UPDATE + new.diffs = [] + result = new - elif action == ChangeComparisonAction.UPDATE: + elif action == ChangeComparisonAction.UPDATE_METADATA: # handle update case, when previous change was create - just revert to create with new metadata new.change = previous.change new.diffs = [] result = new - elif action == ChangeComparisonAction.UPDATE_DIFF: + elif action == ChangeComparisonAction.REPLACE_DIFFS: new.diffs = (previous.diffs or []) + (new.diffs or []) result = new @@ -1269,8 +1283,12 @@ def create_checkpoint( changes = [] for delta in delta_range: changes.extend(DeltaChangeSchema(many=True).load(delta.changes)) + + # Merge changes for compact storage and FileDiff checkpoint decisions. + merged_changes = cls.merge_changes(changes) + merged_delta_items: List[DeltaChange] = [ - d.to_data_delta() for d in cls.merge_changes(changes) + d.to_data_delta() for d in merged_changes ] # Pre-fetch data for all versioned files to create FileDiff checkpoints where it makes sense diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py index 855b531c..9aabc0aa 100644 --- a/server/mergin/tests/test_public_api_v2.py +++ b/server/mergin/tests/test_public_api_v2.py @@ -25,6 +25,7 @@ from .utils import ( add_user, + create_blank_version, create_project, create_workspace, diffs_are_equal, @@ -493,6 +494,106 @@ def test_delta_merge_changes(): assert merged[0].checksum == delete8.checksum +def test_delta_cross_checkpoint_create_delete_recreate(client): + """ + Setup (rank-1 chunks cover 4 versions each, 4^1=4): + v1–v4: tracked.gpkg does NOT exist (client at v4 has never seen it) + v5–v8: tracked.gpkg is CREATED at v5 → rank-1 stores [CREATE(v5)] + v9–v12: tracked.gpkg is DELETED at v9, RE-CREATED at v10 + → rank-1 stores [CREATE(v10)] after pre-merging DELETE+CREATE + + get_delta_changes(4, 12) then loads [CREATE(v5), CREATE(v10)] and the outer + merge_changes hits CREATE+CREATE → EXCLUDE → returns []. + + Expected: [CREATE(v10)] so the client downloads the re-created file. + """ + project = Project.query.filter_by( + workspace_id=test_workspace_id, name=test_project + ).first() + + tracked_gpkg = os.path.join(TMP_DIR, "tracked.gpkg") + shutil.copy(os.path.join(test_project_dir, "base.gpkg"), tracked_gpkg) + + # advance to v4 with blank versions (project starts at v1) + create_blank_version(project) # v2 + create_blank_version(project) # v3 + create_blank_version(project) # v4 + assert project.latest_version == 4 + + # rank-1 chunk [v5–v8]: file is born at v5 + push_change(project, "added", "tracked.gpkg", TMP_DIR) # v5 + create_blank_version(project) # v6 + create_blank_version(project) # v7 + create_blank_version(project) # v8 + assert project.latest_version == 8 + + # rank-1 chunk [v9–v12]: file is deleted then re-created in the same chunk + push_change(project, "removed", "tracked.gpkg", TMP_DIR) # v9 + push_change(project, "added", "tracked.gpkg", TMP_DIR) # v10 + create_blank_version(project) # v11 + create_blank_version(project) # v12 + assert project.latest_version == 12 + + # client at v4 never had tracked.gpkg; it was created at v5, deleted at v9, re-created at v10 + delta = project.get_delta_changes(4, 12) + + assert delta is not None + tracked = next((d for d in delta if d.path == "tracked.gpkg"), None) + # DELETE(v9)+CREATE(v10) was collapsed to CREATE inside checkpoint(v9-v12) + # then CREATE(v5)+CREATE(v10) hit CREATE+CREATE→EXCLUDE in the outer merge + assert tracked is not None + assert tracked.change == PushChangeType.CREATE + assert tracked.version == 10 + + +def test_delta_cross_checkpoint_recreate_then_delete(client): + """ + Setup: + v1–v4: base.gpkg exists (client at v4 has the file, exclude_delete should be False) + v5–v8: base.gpkg DELETED at v5, RE-CREATED at v6 + → rank-1 stores [CREATE(v6)] after pre-merging DELETE+CREATE + v9–v12: base.gpkg DELETED at v9 (permanently gone) + → rank-1 stores [DELETE(v9)] + + get_delta_changes(4, 12) loads [CREATE(v6), DELETE(v9)]. The stored CREATE + from chunk A did not set updating_files, so exclude_delete=True and + CREATE+DELETE → EXCLUDE → returns []. + + Expected: [DELETE] so the client removes the stale local copy of base.gpkg. + """ + project = Project.query.filter_by( + workspace_id=test_workspace_id, name=test_project + ).first() + + # advance to v4; base.gpkg is present throughout (client at v4 has it) + create_blank_version(project) # v2 + create_blank_version(project) # v3 + create_blank_version(project) # v4 + assert project.latest_version == 4 + + # rank-1 chunk [v5–v8]: delete then immediately re-create in the same chunk + push_change(project, "removed", "base.gpkg", TMP_DIR) # v5 + push_change(project, "added", "base.gpkg", test_project_dir) # v6 + create_blank_version(project) # v7 + create_blank_version(project) # v8 + assert project.latest_version == 8 + + # rank-1 chunk [v9–v12]: file is permanently deleted + push_change(project, "removed", "base.gpkg", TMP_DIR) # v9 + create_blank_version(project) # v10 + create_blank_version(project) # v11 + create_blank_version(project) # v12 + assert project.latest_version == 12 + + # client at v4 has base.gpkg; server deleted it at v9 + delta = project.get_delta_changes(4, 12) + + assert delta is not None + base_gpkg = next((d for d in delta if d.path == "base.gpkg"), None) + assert base_gpkg is not None + assert base_gpkg.change == PushChangeType.DELETE + + def test_project_version_delta_changes(client, diff_project: Project): """Test that get_delta_changes and its schema work as expected""" latest_version = diff_project.get_latest_version() @@ -524,7 +625,7 @@ def test_project_version_delta_changes(client, diff_project: Project): # delete + create version delta = diff_project.get_delta_changes(1, 3) assert len(delta) == 1 - assert delta[0].change == PushChangeType.CREATE + assert delta[0].change == PushChangeType.UPDATE # file was created in v3 assert delta[0].version == 3 assert delta[0].checksum == deltas[3].changes[0]["checksum"] @@ -532,7 +633,7 @@ def test_project_version_delta_changes(client, diff_project: Project): # get_delta with update diff delta = diff_project.get_delta_changes(1, 4) assert len(delta) == 1 - assert delta[0].change == PushChangeType.CREATE + assert delta[0].change == PushChangeType.UPDATE assert ProjectVersionDelta.query.filter_by(rank=1).count() == 0 # create rank 1 checkpoint for v4