Merge pull request #607 from MerginMaps/fix_diff_checkpoints

Fix condition for creating diff checkpoints

Author: MarcelGeo

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

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(

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)

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,9 +322,12 @@ 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
-    assert diffs[-1] == diff
+    # individual diffs v11, v12 + (v13-v16) + (v17-v20) as the last one
+    assert len(diffs) == 4
+    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)
@@ -397,9 +402,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