diff --git a/mergin/client_push.py b/mergin/client_push.py index d52ff5a9..1e492082 100644 --- a/mergin/client_push.py +++ b/mergin/client_push.py @@ -15,6 +15,13 @@ import tempfile import concurrent.futures import os +from pygeodiff import ( + GeoDiff, + GeoDiffLibError, + GeoDiffLibConflictError, + GeoDiffLibUnsupportedChangeError, + GeoDiffLibVersionError, +) from .common import UPLOAD_CHUNK_SIZE, ClientError from .merginproject import MerginProject @@ -268,14 +275,31 @@ def push_project_finalize(job): 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", "") + 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 + # it would be better if it returned list of failed files (and reasons) whenever possible job.mp.log.error("--- push finish failed! " + str(err)) # if push finish fails, the transaction is not killed, so we @@ -303,6 +327,25 @@ def push_project_finalize(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 GeoDiff().changes_count(diff_abs) + except ( + GeoDiffLibError, + GeoDiffLibConflictError, + GeoDiffLibUnsupportedChangeError, + GeoDiffLibVersionError, + FileNotFoundError, + ): + return None + + def push_project_cancel(job): """ To be called (from main thread) to cancel a job that has uploads in progress. diff --git a/mergin/test/test_push_logging.py b/mergin/test/test_push_logging.py new file mode 100644 index 00000000..f2920f6a --- /dev/null +++ b/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) diff --git a/scripts/update_version.py b/scripts/update_version.py index 184d6a8a..20206593 100644 --- a/scripts/update_version.py +++ b/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) diff --git a/setup.py b/setup.py index 371014e9..2b4df73e 100644 --- a/setup.py +++ b/setup.py @@ -4,35 +4,31 @@ from setuptools import setup, find_packages setup( - name='mergin-client', - version='0.10.5', - url='https://github.com/MerginMaps/python-api-client', - license='MIT', - author='Lutra Consulting Ltd.', - author_email='info@merginmaps.com', - description='Mergin Maps utils and client', - long_description='Mergin Maps utils and client', - + name="mergin-client", + version="0.10.5", + url="https://github.com/MerginMaps/python-api-client", + license="MIT", + author="Lutra Consulting Ltd.", + author_email="info@merginmaps.com", + description="Mergin Maps utils and client", + long_description="Mergin Maps utils and client", packages=find_packages(), - - platforms='any', + platforms="any", install_requires=[ - 'python-dateutil==2.8.2', - 'pygeodiff==2.0.4', - 'pytz==2022.1', - 'click==8.1.3', + "python-dateutil==2.8.2", + "pygeodiff==2.0.4", + "pytz==2022.1", + "click==8.1.3", ], - entry_points={ - 'console_scripts': ['mergin=mergin.cli:cli'], + "console_scripts": ["mergin=mergin.cli:cli"], }, - classifiers=[ - 'Development Status :: 5 - Production/Stable', - 'Intended Audience :: Developers', - 'License :: OSI Approved :: MIT License', - 'Operating System :: OS Independent', - 'Programming Language :: Python :: 3' + "Development Status :: 5 - Production/Stable", + "Intended Audience :: Developers", + "License :: OSI Approved :: MIT License", + "Operating System :: OS Independent", + "Programming Language :: Python :: 3", ], - package_data={'mergin': ['cert.pem']} + package_data={"mergin": ["cert.pem"]}, )