From 4840a48baba0bdef9e684f3080858d988628b01c Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Wed, 8 Apr 2026 16:15:10 +0200 Subject: [PATCH 1/2] Fix condition for creating diff checkpoints Enfore rule that checkpoint can not contain basefile in its range. When creating diff chain / history, always lookup with version higher than basefile. This prevents confusion when it is worth creating a diff checkpoint and ensures we only create merged diffs which can actually be applied. As a consequence, gpkg will never have checkpoints starting with version 1. --- server/mergin/sync/models.py | 14 ++++++++++---- server/mergin/tests/test_file_restore.py | 11 +++++++++-- server/mergin/tests/test_project_controller.py | 12 ++++++++---- server/mergin/tests/test_public_api_v2.py | 14 ++++++++------ 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/server/mergin/sync/models.py b/server/mergin/sync/models.py index 1b99735b..f1c6cbfd 100644 --- a/server/mergin/sync/models.py +++ b/server/mergin/sync/models.py @@ -777,7 +777,10 @@ def diffs_chain( return None, [] diffs = [] - checkpoints = Checkpoint.get_checkpoints(basefile.project_version_name, version) + # get all checkpoints with diffs after basefile which should be applied + checkpoints = Checkpoint.get_checkpoints( + basefile.project_version_name + 1, version + ) expected_diffs = ( FileDiff.query.filter_by( basefile_id=basefile.id, @@ -922,6 +925,10 @@ def can_create_checkpoint(file_path_id: int, checkpoint: Checkpoint) -> bool: if not basefile: return False + # do not create checkpoint if basefile is present in the range as it does not have valid use case + if basefile.project_version_name >= checkpoint.start: + return False + file_was_deleted = ( FileHistory.query.filter_by(file_path_id=file_path_id) .filter( @@ -1017,9 +1024,8 @@ def construct_checkpoint(self) -> bool: ) for item in cached_items: - # basefile is a start of the diff chain, item cannot cross over it, - # but merged diffs can start with basefile version containing changes since then - if item.start < basefile.project_version_name: + # basefile is a start of the diff chain, checkpoint must start after basefile version + if basefile.project_version_name >= item.start: continue # find diff in table and on disk diff --git a/server/mergin/tests/test_file_restore.py b/server/mergin/tests/test_file_restore.py index 8025d7de..953328cd 100644 --- a/server/mergin/tests/test_file_restore.py +++ b/server/mergin/tests/test_file_restore.py @@ -252,7 +252,8 @@ def test_version_file_restore(diff_project): test_file = os.path.join(diff_project.storage.project_dir, "v30", "test.gpkg") os.rename(test_file, test_file + "_backup") diff_project.storage.restore_versioned_file("test.gpkg", 30) - checkpoints = Checkpoint.get_checkpoints(9, 30) + # only count diffs from v11-v30 where they were present + checkpoints = Checkpoint.get_checkpoints(11, 30) assert os.path.exists(test_file) assert gpkgs_are_equal(test_file, test_file + "_backup") assert FileDiff.query.filter_by(file_path_id=file_path_id).filter( @@ -261,6 +262,9 @@ def test_version_file_restore(diff_project): ) ).count() == len(checkpoints) + # checkpoint v9-v12 which contains basefile should not be created + assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(1, 3)) is False + # let's create new project with basefile at v1 (which can be start of multiple checkpoints) working_dir = os.path.join(TMP_DIR, "restore_from_diffs") basefile = os.path.join(working_dir, "base.gpkg") @@ -284,10 +288,13 @@ def test_version_file_restore(diff_project): ) ) + # checkpoint v1-v16 which contains basefile should not be created + assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(2, 1)) is False + test_file = os.path.join(project.storage.project_dir, "v17", "base.gpkg") os.rename(test_file, test_file + "_backup") project.storage.restore_versioned_file("base.gpkg", 17) - checkpoints = Checkpoint.get_checkpoints(1, 17) + checkpoints = Checkpoint.get_checkpoints(2, 17) assert os.path.exists(test_file) assert gpkgs_are_equal(test_file, test_file + "_backup") assert FileDiff.query.filter_by(file_path_id=file_path_id).filter( diff --git a/server/mergin/tests/test_project_controller.py b/server/mergin/tests/test_project_controller.py index ad04ad3c..25e0e055 100644 --- a/server/mergin/tests/test_project_controller.py +++ b/server/mergin/tests/test_project_controller.py @@ -40,7 +40,7 @@ ) from ..sync.files import files_changes_from_upload from ..sync.schemas import ProjectListSchema -from ..sync.utils import generate_checksum, is_versioned_file +from ..sync.utils import Checkpoint, generate_checksum, is_versioned_file from ..auth.models import User, UserProfile from . import ( @@ -1915,11 +1915,15 @@ def test_file_diffs_chain(diff_project): assert basefile.version.name == 5 assert len(diffs) == 2 - # nothing happened in v8 (=v7) but we have now merged diff in chain v5-v8 + # nothing happened in v8 (=v7) basefile, diffs = FileHistory.diffs_chain(file_id, 8) assert basefile.version.name == 5 - assert len(diffs) == 1 - assert diffs[0].rank == 1 and diffs[0].version == 8 + assert len(diffs) == 2 + assert diffs[0].rank == 0 and diffs[0].version == 6 + assert diffs[1].rank == 0 and diffs[1].version == 7 + + # now merged diff in chain v5-v8 cannot be created as it contains basefile + assert FileDiff.can_create_checkpoint(file_id, Checkpoint(1, 2)) is False # file was removed in v9 basefile, diffs = FileHistory.diffs_chain(file_id, 9) diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py index 46b22cbc..279b330d 100644 --- a/server/mergin/tests/test_public_api_v2.py +++ b/server/mergin/tests/test_public_api_v2.py @@ -303,8 +303,10 @@ def test_create_diff_checkpoint(diff_project): assert file_diff and os.path.exists(file_diff.abs_path) basefile, diffs = FileHistory.diffs_chain(file_path_id, 32) + # count checkpoints since we introduced diffs at v11 + checkpoints = Checkpoint.get_checkpoints(11, 32) assert basefile.project_version_name == 9 - assert len(diffs) == 3 + assert len(diffs) == len(checkpoints) # also a lower diff rank was created lower_diff = FileDiff.query.filter_by(version=24, rank=1).first() assert os.path.exists(lower_diff.abs_path) @@ -320,8 +322,8 @@ def test_create_diff_checkpoint(diff_project): basefile, diffs = FileHistory.diffs_chain(file_path_id, 20) assert basefile.project_version_name == 9 - # individual diff v12 + (v13-v16) + (v17-v20) as the last one - assert len(diffs) == 3 + # individual diffs v11, v12 + (v13-v16) + (v17-v20) as the last one + assert len(diffs) == 4 assert diffs[-1] == diff # repeat - nothing to do @@ -397,9 +399,9 @@ def test_can_create_checkpoint(diff_project): # for zero rank diffs we can always create a checkpoint (but that should already exist) assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(0, 4)) is True - # there are diffs in both ranges, v1-v4 and v5-v8 - assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(1, 1)) is True - assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(1, 2)) is True + # there are diffs in both ranges, v1-v4 and v5-v8 but both contain basefile + assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(1, 1)) is False + assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(1, 2)) is False # higher ranks cannot be created as file was removed at v9 assert FileDiff.can_create_checkpoint(file_path_id, Checkpoint(2, 1)) is False From 32cb307fc0e0f126be436238c5cf3f1842c9d5d1 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Thu, 9 Apr 2026 12:01:03 +0200 Subject: [PATCH 2/2] Add more asserts for file_diffs chain tests --- server/mergin/tests/test_public_api_v2.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py index 279b330d..7a87b1d0 100644 --- a/server/mergin/tests/test_public_api_v2.py +++ b/server/mergin/tests/test_public_api_v2.py @@ -324,7 +324,10 @@ def test_create_diff_checkpoint(diff_project): assert basefile.project_version_name == 9 # individual diffs v11, v12 + (v13-v16) + (v17-v20) as the last one assert len(diffs) == 4 - assert diffs[-1] == diff + assert diffs[0].version == 11 and diffs[0].rank == 0 + assert diffs[1].version == 12 and diffs[1].rank == 0 + assert diffs[2].version == 16 and diffs[2].rank == 1 + assert diffs[3].version == 20 and diffs[3].rank == 1 # repeat - nothing to do mtime = os.path.getmtime(diff.abs_path)