Make sure all gpkg files and versions are reported

download diffs functionality was split for higher flexibility of usage

Author: varmar05

mergin/client.py

@@ -2,6 +2,7 @@
 import math
 import os
 import json
+import shutil
 import zlib
 import base64
 import urllib.parse
@@ -757,9 +758,8 @@ def download_file(self, project_dir, file_path, output_filename, version=None):
         pull_project_wait(job)
         download_file_finalize(job)
 
-    def get_file_diff(self, project_dir, file_path, output_diff, version_from, version_to, keep_diffs=False):
+    def get_file_diff(self, project_dir, file_path, output_diff, version_from, version_to):
         """ Create concatenated diff for project file diffs between versions version_from and version_to.
-        Downloaded diffs can be kept if needed.
 
         :param project_dir: project local directory
         :type project_dir: String
@@ -771,12 +771,39 @@ def get_file_diff(self, project_dir, file_path, output_diff, version_from, versi
         :type version_from: String
         :param version_to: ending project version tag for getting diff
         :type version_to: String
-        :param keep_diffs: whether to keep downloaded diff files
-        :type keep_diffs: Boolean
         """
-        job = download_diffs_async(self, project_dir, file_path, version_from, version_to)
+        mp = MerginProject(project_dir)
+        project_path = mp.metadata["name"]
+        file_history = self.project_file_history_info(project_path, file_path)
+        versions_to_fetch = get_versions_with_file_changes(
+            self, project_path, file_path, version_from=version_from, version_to=version_to, file_history=file_history
+        )
+        diffs = self.download_file_diffs(project_dir, file_path, versions_to_fetch[1:])
+        # concatenate diffs, if needed
+        output_dir = os.path.dirname(output_diff)
+        if len(diffs) >= 1:
+            os.makedirs(output_dir, exist_ok=True)
+            if len(diffs) > 1:
+                mp.geodiff.concat_changes(diffs, output_diff)
+            elif len(diffs) == 1:
+                shutil.copy(diffs[0], output_diff)
+
+    def download_file_diffs(self, project_dir, file_path, versions):
+        """ Download file diffs for specified versions.
+
+        :param project_dir: project local directory
+        :type project_dir: String
+        :param file_path: relative path of file to download in the project directory
+        :type file_path: String
+        :param versions: list of versions to download diffs for, for example ['v3', 'v5']
+        :type versions: List(String)
+        :returns: list of downloaded diffs (their actual locations on disk)
+        :rtype: List(String)
+        """
+        job = download_diffs_async(self, project_dir, file_path, versions)
         pull_project_wait(job)
-        download_diffs_finalize(job, output_diff, keep_diffs)
+        diffs = download_diffs_finalize(job)
+        return diffs
 
     def has_unfinished_pull(self, directory):
         """

mergin/client_pull.py

@@ -672,7 +672,7 @@ def download_file_finalize(job):
         shutil.rmtree(temp_dir)
 
 
-def download_diffs_async(mc, project_directory, file_path, version_from, version_to):
+def download_diffs_async(mc, project_directory, file_path, versions):
     """
     Starts background download project file diffs for specified versions.
     Returns handle to the pending download.
@@ -681,20 +681,16 @@ def download_diffs_async(mc, project_directory, file_path, version_from, version
         mc (MerginClient): MerginClient instance.
         project_directory (str): local project directory.
         file_path (str): file path relative to Mergin project root.
-        version_from (str): starting project version tag for getting diff, for example 'v3'.
-        version_to (str): ending project version tag for getting diff.
+        versions (list): list of versions to download diffs for, e.g. ['v1', 'v2'].
 
     Returns:
         PullJob/None: a handle for the pending download.
     """
     mp = MerginProject(project_directory)
     project_path = mp.metadata["name"]
     file_history = mc.project_file_history_info(project_path, file_path)
-    versions_to_fetch = get_versions_with_file_changes(
-        mc, project_path, file_path, version_from=version_from, version_to=version_to, file_history=file_history
-    )
     mp.log.info(f"--- version: {mc.user_agent_info()}")
-    mp.log.info(f"--- start download diffs for {file_path} of {project_path}, versions: {[v for v in versions_to_fetch]}")
+    mp.log.info(f"--- start download diffs for {file_path} of {project_path}, versions: {[v for v in versions]}")
 
     try:
         server_info = mc.project_info(project_path)
@@ -709,7 +705,7 @@ def download_diffs_async(mc, project_directory, file_path, version_from, version
     os.makedirs(download_dir, exist_ok=True)
     fetch_files = []
 
-    for version in versions_to_fetch[1:]:
+    for version in versions:
         version_data = file_history["history"][version]
         diff_data = copy.deepcopy(version_data)
         diff_data['version'] = version
@@ -721,7 +717,7 @@ def download_diffs_async(mc, project_directory, file_path, version_from, version
     total_size = 0
     for file in fetch_files:
         items = _download_items(file, download_dir, diff_only=True)
-        dest_file_path = os.path.normpath(os.path.join(download_dir, file["version"] + "-" + os.path.basename(file['diff']['path'])))
+        dest_file_path = os.path.normpath(os.path.join(download_dir, file["version"] + "-" + file['diff']['path']))
         if os.path.exists(dest_file_path):
             continue
         files_to_merge.append(FileToMerge(dest_file_path, items))
@@ -744,8 +740,12 @@ def download_diffs_async(mc, project_directory, file_path, version_from, version
     return job
 
 
-def download_diffs_finalize(job, output_diff, keep_diffs=False):
-    """ To be called after download_diffs_async """
+def download_diffs_finalize(job):
+    """ To be called after download_diffs_async
+
+    Returns:
+        diffs: list of downloaded diffs (their actual locations on disk)
+    """
 
     job.executor.shutdown(wait=True)
 
@@ -757,34 +757,17 @@ def download_diffs_finalize(job, output_diff, keep_diffs=False):
             raise future.exception()
 
     job.mp.log.info("finalizing diffs pull")
+    diffs = []
 
     # merge downloaded chunks
     try:
         for file_to_merge in job.files_to_merge:
             file_to_merge.merge()
+            diffs.append(file_to_merge.dest_file)
     except ClientError as err:
         job.mp.log.error("Error merging chunks of downloaded file: " + str(err))
         job.mp.log.info("--- diffs pull aborted")
         raise
 
     job.mp.log.info("--- diffs pull finished")
-
-    # Collect and finally concatenate diffs, if needed
-    diffs = []
-    for file_to_merge in job.files_to_merge:
-        diffs.append(file_to_merge.dest_file)
-
-    output_dir = os.path.dirname(output_diff)
-    temp_dir = None
-    if len(diffs) >= 1:
-        os.makedirs(output_dir, exist_ok=True)
-        temp_dir = os.path.dirname(diffs[0])
-        if len(diffs) > 1:
-            job.mp.geodiff.concat_changes(diffs, output_diff)
-        elif len(diffs) == 1:
-            shutil.copy(diffs[0], output_diff)
-
-    if not keep_diffs:
-        # remove the diffs download temporary directory
-        if temp_dir is not None:
-            shutil.rmtree(temp_dir)
+    return diffs

mergin/report.py

@@ -5,9 +5,9 @@
 from collections import defaultdict
 from itertools import groupby
 
-from . import ClientError
-from .merginproject import MerginProject, pygeodiff
-from .utils import int_version
+from mergin import ClientError
+from mergin.merginproject import MerginProject, pygeodiff
+from mergin.utils import int_version
 
 
 # inspired by C++ implementation https://github.com/lutraconsulting/geodiff/blob/master/geodiff/src/drivers/sqliteutils.cpp
@@ -123,7 +123,7 @@ def __init__(self, changeset_reader, schema):
             # get geometry index in both gpkg schema and diffs values
             geom_idx = next((index for (index, col) in enumerate(schema_table["columns"]) if col["type"] == "geometry"),
                             None)
-            if not geom_idx:
+            if geom_idx is None:
                 continue
 
             geom_col = schema_table["columns"][geom_idx]["geometry"]
@@ -169,24 +169,25 @@ def create_report(mc, directory, project, since, to, out_dir=tempfile.gettempdir
     headers = ["file", "table", "author", "timestamp", "version", "quantity_type", "quantity"]
     records = []
     info = mc.project_info(project, since=since)
+    num_since = int_version(since)
+    num_to = int_version(to)
     # filter only .gpkg files
     files = [f for f in info["files"] if mp.is_versioned_file(f["path"])]
     for f in files:
         mp.log.debug(f"analyzing {f['path']} ...")
-        diff_file = os.path.join(mp.meta_dir, f["path"] + ".diff")
         try:
             if "history" not in f:
+                mp.log.debug(f"no history field, skip")
                 continue
 
-            # download diffs in desired range, in case versions are not within history,
-            # pass unknown, and it will be determined in download function
-            # it can be different for each file
-            since_ = since if since in f["history"] else None
-            to_ = to if to in f["history"] else None
-            if not (since_ and to_):
-                continue  # no change at all for particular file in desired range
+            # get version list (keys) within range
+            history_keys = list(filter(lambda v: (num_since <= int_version(v) <= num_to), f["history"].keys()))
+            if not history_keys:
+                mp.log.debug(f"no file history within range, skip")
+                continue
 
-            mc.get_file_diff(directory, f["path"], diff_file, since_, to_, True)
+            # download diffs
+            mc.download_file_diffs(directory, f["path"], history_keys)
 
             # download full gpkg in "to" version to analyze its schema to determine which col is geometry
             full_gpkg = os.path.join(mp.meta_dir, ".cache", f["path"])
@@ -201,13 +202,9 @@ def create_report(mc, directory, project, since, to, out_dir=tempfile.gettempdir
                 schema = json.load(sf).get("geodiff_schema")
 
             # add records for every version (diff) and all tables within geopackage
-            for idx in range(int_version(since) + 1, int_version(to) + 1):
-                version = "v" + str(idx)
-                # skip version if there was no diff file
-                if version not in f['history']:
-                    continue
+            for version in history_keys:
                 v_diff_file = os.path.join(mp.meta_dir, '.cache',
-                                           version + "-" + os.path.basename(f['history'][version]['diff']['path']))
+                                           version + "-" + f['history'][version]['diff']['path'])
 
                 version_data = versions_map[version]
                 cr = mp.geodiff.read_changeset(v_diff_file)

mergin/test/test_client.py

@@ -196,7 +196,7 @@ def test_push_pull_changes(mc):
     assert len(project_info['files']) == len(mp.inspect_files())
     project_versions = mc.project_versions(project)
     assert len(project_versions) == 2
-    f_change = next((f for f in project_versions[0]['changes']['updated'] if f['path'] == f_updated), None)
+    f_change = next((f for f in project_versions[-1]['changes']['updated'] if f['path'] == f_updated), None)
     assert 'origin_checksum' not in f_change  # internal client info
 
     # test parallel changes
@@ -1642,8 +1642,3 @@ def test_report(mc):
         assert "v3,update_count,2" in content
         # files not edited are not in reports
         assert "inserted_1_A.gpkg" not in content
-
-    # test some failure, e.g. wrong version range, which would fail on getting version list
-    with pytest.raises(ClientError) as e:
-        create_report(mc, directory, project, to, since, TMP_DIR)
-    assert "The requested URL was not found on the server." in str(e.value)