Merge remote-tracking branch 'origin/master' into push-v2-integration

Author: MarcelGeo

.github/workflows/autotests.yml

@@ -10,7 +10,7 @@ env:
 concurrency:
   group: ci-${{github.ref}}-autotests
   cancel-in-progress: true
-  
+
 jobs:
   tests:
     runs-on: ubuntu-latest
@@ -19,7 +19,7 @@ jobs:
 
       - uses: actions/setup-python@v2
         with:
-          python-version: '3.8'
+          python-version: "3.8"
 
       - name: Install python package dependencies
         run: |

mergin/client.py

@@ -392,14 +392,18 @@ def server_type(self):
         if not self._server_type:
             try:
                 config = self.server_config()
-                if config["server_type"] == "ce":
+                stype = config.get("server_type")
+                if stype == "ce":
                     self._server_type = ServerType.CE
-                elif config["server_type"] == "ee":
+                elif stype == "ee":
                     self._server_type = ServerType.EE
-                elif config["server_type"] == "saas":
+                elif stype == "saas":
                     self._server_type = ServerType.SAAS
-            except (ClientError, KeyError):
-                self._server_type = ServerType.OLD
+            except ClientError as e:
+                if getattr(e, "http_error", None) == 404:
+                    self._server_type = ServerType.OLD
+                else:
+                    raise
 
         return self._server_type
 

mergin/client_pull.py

@@ -22,7 +22,7 @@
 
 from .common import CHUNK_SIZE, ClientError
 from .merginproject import MerginProject
-from .utils import save_to_file
+from .utils import cleanup_tmp_dir, save_to_file
 
 
 # status = download_project_async(...)
@@ -250,7 +250,7 @@ def download_project_finalize(job):
     # final update of project metadata
     job.mp.update_metadata(job.project_info)
 
-    job.tmp_dir.cleanup()
+    cleanup_tmp_dir(job.mp, job.tmp_dir)
 
 
 def download_project_cancel(job):
@@ -263,6 +263,7 @@ def download_project_cancel(job):
     job.is_cancelled = True
     job.executor.shutdown(wait=True)
     job.mp.log.info("--- download cancelled")
+    cleanup_tmp_dir(job.mp, job.tmp_dir)
 
 
 class UpdateTask:
@@ -550,6 +551,7 @@ def pull_project_cancel(job):
     job.is_cancelled = True
     job.executor.shutdown(wait=True)
     job.mp.log.info("--- pull cancelled")
+    cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
 
 
 class FileToMerge:
@@ -637,6 +639,7 @@ def pull_project_finalize(job: PullJob):
     except Exception as e:
         job.mp.log.error("Failed to apply pull changes: " + str(e))
         job.mp.log.info("--- pull aborted")
+        cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
         raise ClientError("Failed to apply pull changes: " + str(e))
 
     job.mp.update_metadata(job.project_info)
@@ -646,7 +649,7 @@ def pull_project_finalize(job: PullJob):
     else:
         job.mp.log.info("--- pull finished -- at version " + job.mp.version())
 
-    job.tmp_dir.cleanup()  # delete our temporary dir and all its content
+    cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
     return conflicts
 
 

mergin/client_push.py

@@ -32,9 +32,10 @@
     MAX_UPLOAD_MEDIA_SIZE,
     ClientError,
 )
-from .merginproject import MerginProject
+from .merginproject import MerginProject, pygeodiff
 from .editor import filter_changes
-from .utils import get_data_checksum
+from .utils import get_data_checksum, cleanup_tmp_dir
+
 
 POST_JSON_HEADERS = {"Content-Type": "application/json"}
 
@@ -381,6 +382,7 @@ def push_project_finalize(job: UploadJob):
             f"Transferred size ({job.transferred_size}) and expected total size ({job.total_size}) do not match!"
         )
         job.mp.log.error("--- push finish failed! " + error_msg)
+        cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
         raise ClientError("Upload error: " + error_msg)
 
     if server_features.get("v2_push_enabled"):
@@ -410,12 +412,29 @@ def push_project_finalize(job: UploadJob):
             resp = job.mc.post("/v1/project/push/finish/%s" % job.transaction_id)
             job.server_resp = json.load(resp)
         except ClientError as err:
-            # Log additional metadata on server error 502 or 504
-            if hasattr(err, "http_error") and err.http_error in (502, 504):
+            # Log additional metadata on server error 502 or 504 (extended logging only)
+            http_code = getattr(err, "http_error", None)
+            if http_code in (502, 504):
                 job.mp.log.error(
-                    f"Push failed with HTTP error {err.http_error}. "
+                    f"Push failed with HTTP error {http_code}. "
                     f"Upload details: {len(job.upload_queue_items)} file chunks, total size {job.total_size} bytes."
                 )
+                job.mp.log.error("Files:")
+                for f in job.changes.get("added", []) + job.changes.get("updated", []):
+                    path = f.get("path", "<unknown>")
+                    size = f.get("size", "?")
+                    if "diff" in f:
+                        diff_info = f.get("diff", {})
+                        diff_size = diff_info.get("size", "?")
+                        # best-effort: number of geodiff changes (if available)
+                        changes_cnt = _geodiff_changes_count(job.mp, diff_info.get("path"))
+                        if changes_cnt is None:
+                            job.mp.log.error(f"  - {path}, size={size}, diff_size={diff_size}, changes=n/a")
+                        else:
+                            job.mp.log.error(f"  - {path}, size={size}, diff_size={diff_size}, changes={changes_cnt}")
+                    else:
+                        job.mp.log.error(f"  - {path}, size={size}")
+
             # server returns various error messages with filename or something generic
             # it would be better if it returned list of failed files (and reasons) whenever possible
             job.mp.log.error("--- push finish failed! " + str(err))
@@ -428,6 +447,7 @@ def push_project_finalize(job: UploadJob):
                 job.mp.log.info("cancel response: " + resp_cancel.msg)
             except ClientError as err2:
                 job.mp.log.info("cancel response: " + str(err2))
+            cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
             raise err
 
     job.mp.update_metadata(job.server_resp)
@@ -436,16 +456,35 @@ def push_project_finalize(job: UploadJob):
     except Exception as e:
         job.mp.log.error("Failed to apply push changes: " + str(e))
         job.mp.log.info("--- push aborted")
+        cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
         raise ClientError("Failed to apply push changes: " + str(e))
 
-    job.tmp_dir.cleanup()  # delete our temporary dir and all its content
+    cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
     job.mc.upload_chunks_cache.clear()  # clear the upload chunks cache
-
     remove_diff_files(job)
 
     job.mp.log.info("--- push finished - new project version " + job.server_resp["version"])
 
 
+def _geodiff_changes_count(mp: MerginProject, diff_rel_path: str):
+    """
+    Best-effort: return number of changes in the .gpkg diff (int) or None.
+    Never raises – diagnostics/logging must not fail.
+    """
+
+    diff_abs = mp.fpath_meta(diff_rel_path)
+    try:
+        return pygeodiff.GeoDiff().changes_count(diff_abs)
+    except (
+        pygeodiff.GeoDiffLibError,
+        pygeodiff.GeoDiffLibConflictError,
+        pygeodiff.GeoDiffLibUnsupportedChangeError,
+        pygeodiff.GeoDiffLibVersionError,
+        FileNotFoundError,
+    ):
+        return None
+
+
 def push_project_cancel(job: UploadJob):
     """
     To be called (from main thread) to cancel a job that has uploads in progress.
@@ -466,7 +505,9 @@ def push_project_cancel(job: UploadJob):
         job.server_resp = resp_cancel.msg
     except ClientError as err:
         job.mp.log.error("--- push cancelling failed! " + str(err))
+        cleanup_tmp_dir(job.mp, job.tmp_dir)
         raise err
+    cleanup_tmp_dir(job.mp, job.tmp_dir)  # delete our temporary dir and all its content
     job.mp.log.info("--- push cancel response: " + str(job.server_resp))
 
 

mergin/test/test_client.py

@@ -13,6 +13,8 @@
 import glob
 from unittest.mock import patch, Mock
 
+from unittest.mock import patch, Mock
+
 from .. import InvalidProject
 from ..client import (
     MerginClient,
@@ -2328,6 +2330,7 @@ def test_clean_diff_files(mc):
     shutil.copy(mp.fpath("inserted_1_A.gpkg"), mp.fpath(f_updated))
     mc.push_project(project_dir)
 
+    # Get the directory path
     directory = os.path.split(mp.fpath_meta("inserted_1_A.gpkg"))[0]
     diff_files = [f for f in os.listdir(directory) if "-diff-" in f]
 
@@ -3235,3 +3238,19 @@ def test_push_file_limits(mc):
     with patch("mergin.local_changes.MAX_UPLOAD_MEDIA_SIZE", 1):
         with pytest.raises(ClientError, match=f"Some files exceeded maximum upload size. Files: test.txt."):
             mc.push_project(project_dir)
+
+
+def test_server_type(mc):
+    """Test mc.server_type() method"""
+    # success
+    assert mc.server_type() == ServerType.SAAS
+    mc._server_type = None
+    with patch("mergin.client.MerginClient.get") as mock_client_get:
+        # 404
+        mock_client_get.side_effect = ClientError(detail="Not found", http_error=404)
+        assert mc.server_type() == ServerType.OLD
+        mc._server_type = None
+        # 503
+        mock_client_get.side_effect = ClientError(detail="Service unavailable", http_error=503)
+        with pytest.raises(ClientError, match="Service unavailable"):
+            mc.server_type()

mergin/test/test_push_logging.py

@@ -0,0 +1,98 @@
+from types import SimpleNamespace
+from pathlib import Path
+import logging
+import tempfile
+from unittest.mock import MagicMock
+import pytest
+
+from pygeodiff import GeoDiff
+from mergin.client_push import push_project_finalize, UploadJob
+from mergin.common import ClientError
+from mergin.merginproject import MerginProject
+from mergin.client import MerginClient
+
+
+@pytest.mark.parametrize("status_code", [502, 504])
+def test_push_finalize_logs_on_5xx_real_diff(caplog, status_code, tmp_path):
+    # test data
+    data_dir = Path(__file__).resolve().parent / "test_data"
+    base = data_dir / "base.gpkg"
+    modified = data_dir / "inserted_1_A.gpkg"
+    assert base.exists() and modified.exists()
+
+    # real MerginProject in temp dir
+    proj_dir = tmp_path / "proj"
+    meta_dir = proj_dir / ".mergin"
+    meta_dir.mkdir(parents=True)
+    mp = MerginProject(str(proj_dir))
+
+    # route MP logs into pytest caplog
+    logger = logging.getLogger("mergin_test")
+    logger.setLevel(logging.DEBUG)
+    logger.propagate = True
+    caplog.set_level(logging.ERROR, logger="mergin_test")
+    mp.log = logger
+
+    # generate a real diff into .mergin/
+    diff_path = meta_dir / "base_to_inserted_1_A.diff"
+    GeoDiff().create_changeset(str(base), str(modified), str(diff_path))
+    diff_rel = diff_path.name
+    diff_size = diff_path.stat().st_size
+    file_size = modified.stat().st_size
+
+    # mock MerginClient: only patch post(); simulate 5xx on finish
+    tx = "tx-1"
+
+    def mc_post(url, *args, **kwargs):
+        if url.endswith(f"/v1/project/push/finish/{tx}"):
+            err = ClientError("Gateway error")
+            setattr(err, "http_error", status_code)  # emulate HTTP code on the exception
+            raise err
+        if url.endswith(f"/v1/project/push/cancel/{tx}"):
+            return SimpleNamespace(msg="cancelled")
+        return SimpleNamespace(msg="ok")
+
+    mc = MagicMock(spec=MerginClient)
+    mc.post.side_effect = mc_post
+
+    tmp_dir = tempfile.TemporaryDirectory(prefix="python-api-client-test-")
+
+    # build a real UploadJob that references the diff/file sizes
+    job = UploadJob(
+        project_path="u/p",
+        changes={
+            "added": [],
+            "updated": [
+                {
+                    "path": modified.name,
+                    "size": file_size,
+                    "diff": {"path": diff_rel, "size": diff_size},
+                    "chunks": [1],
+                }
+            ],
+            "removed": [],
+        },
+        transaction_id=tx,
+        mp=mp,
+        mc=mc,
+        tmp_dir=tmp_dir,
+    )
+
+    job.total_size = 1234
+    job.transferred_size = 1234
+    job.upload_queue_items = [1]
+    job.executor = SimpleNamespace(shutdown=lambda wait=True: None)
+    job.futures = [SimpleNamespace(done=lambda: True, exception=lambda: None, running=lambda: False)]
+    job.server_resp = {"version": "n/a"}
+
+    with pytest.raises(ClientError):
+        push_project_finalize(job)
+
+    text = caplog.text
+    assert f"Push failed with HTTP error {status_code}" in text
+    assert "Upload details:" in text
+    assert "Files:" in text
+    assert modified.name in text
+    assert f"size={file_size}" in text
+    assert f"diff_size={diff_size}" in text
+    assert ("changes=" in text) or ("changes=n/a" in text)

mergin/utils.py

@@ -6,6 +6,7 @@
 import sqlite3
 from datetime import datetime
 from pathlib import Path
+import tempfile
 from .common import ClientError
 from typing import ByteString
 
@@ -202,7 +203,7 @@ def conflicted_copy_file_name(path, user, version):
     head, tail = os.path.split(os.path.normpath(path))
     ext = "".join(Path(tail).suffixes)
     file_name = tail.replace(ext, "")
-    # in case of QGIS project files we have to add "~" (tilde) to suffix
+    # in case of QGIS project files we TemporaryDirectoryhave to add "~" (tilde) to suffix
     # to avoid having several QGIS project files inside Mergin Maps project.
     # See https://github.com/lutraconsulting/qgis-mergin-plugin/issues/382
     # for more details
@@ -307,3 +308,17 @@ def get_data_checksum(data: ByteString) -> str:
     checksum = hashlib.sha1()
     checksum.update(data)
     return checksum.hexdigest()
+
+
+def cleanup_tmp_dir(mp, tmp_dir: tempfile.TemporaryDirectory):
+    """
+    Remove temporary from tempfile.TemporaryDirectory instance
+    This is needed beacause ignore_clanup_errors is not accepted under < Python 3.10
+    """
+
+    try:
+        tmp_dir.cleanup()
+    except PermissionError:
+        mp.log.warning(f"Permission error during tmp dir cleanup: {tmp_dir.name}")
+    except Exception as e:
+        mp.log.error(f"Error during tmp dir cleanup: {tmp_dir.name}: {e}")

scripts/update_version.py

@@ -4,7 +4,7 @@
 
 
 def replace_in_file(filepath, regex, sub):
-    with open(filepath, 'r') as f:
+    with open(filepath, "r") as f:
         content = f.read()
 
     content_new = re.sub(regex, sub, content, flags=re.M)
@@ -15,14 +15,14 @@ def replace_in_file(filepath, regex, sub):
 
 dir_path = os.path.dirname(os.path.realpath(__file__))
 parser = argparse.ArgumentParser()
-parser.add_argument('--version', help='version to replace')
+parser.add_argument("--version", help="version to replace")
 args = parser.parse_args()
 ver = args.version
 print("using version " + ver)
 
 about_file = os.path.join(dir_path, os.pardir, "mergin", "version.py")
 print("patching " + about_file)
-replace_in_file(about_file, "__version__\s=\s\".*", "__version__ = \"" + ver + "\"")
+replace_in_file(about_file, '__version__\s=\s".*', '__version__ = "' + ver + '"')
 
 setup_file = os.path.join(dir_path, os.pardir, "setup.py")
 print("patching " + setup_file)