From 640d76193f618cd18d49f8ec03471ea24d8dcf63 Mon Sep 17 00:00:00 2001
From: Alexander Bruy <alexander.bruy@gmail.com>
Date: Wed, 11 Oct 2023 14:37:11 +0300
Subject: [PATCH] address review

---
 mergin/client_pull.py      | 38 ++++++++++++++------------------------
 mergin/test/test_client.py | 23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/mergin/client_pull.py b/mergin/client_pull.py
index 068771c..e6164a4 100644
--- a/mergin/client_pull.py
+++ b/mergin/client_pull.py
@@ -15,7 +15,6 @@
 import pprint
 import shutil
 import tempfile
-from pathlib import Path
 
 import concurrent.futures
 
@@ -55,6 +54,7 @@ def __init__(
         self.mp = mp  # MerginProject instance
         self.is_cancelled = False
         self.project_info = project_info  # parsed JSON with project info returned from the server
+        self.failure_log_file = None # log file, copied from the project directory if download fails
 
     def dump(self):
         print("--- JOB ---", self.total_size, "bytes")
@@ -99,6 +99,8 @@ def _cleanup_failed_download(directory, mergin_project=None):
     If a download job fails, there will be the newly created directory left behind with some
     temporary files in it. We want to remove it because a new download would fail because
     the directory already exists.
+
+    Returns path to the client log file or None if log file does not exist.
     """
     # First try to get the Mergin Maps project logger and remove its handlers to allow the log file deletion
     if mergin_project is not None:
@@ -106,23 +108,16 @@ def _cleanup_failed_download(directory, mergin_project=None):
 
     # keep log file as it might contain useful debug info
     log_file = os.path.join(directory, ".mergin", "client-log.txt")
-    dest_file = None
+    dest_path = None
 
     if os.path.exists(log_file):
-        dest_file = os.path.join(tempfile.gettempdir(), os.path.split(log_file)[1])
-        head, tail = os.path.split(os.path.normpath(dest_file))
-        ext = "".join(Path(tail).suffixes)
-        file_name = tail.replace(ext, "")
-
-        i = 0
-        while os.path.exists(dest_file):
-            i += 1
-            dest_file = os.path.join(head, file_name) + f"_{i}{ext}"
-
-        shutil.copyfile(log_file, dest_file)
+        tmp_file = tempfile.NamedTemporaryFile(prefix="mergin-", suffix=".txt", delete=False)
+        tmp_file.close()
+        dest_path = tmp_file.name
+        shutil.copyfile(log_file, dest_path)
 
     shutil.rmtree(directory)
-    return dest_file
+    return dest_path
 
 
 def download_project_async(mc, project_path, directory, project_version=None):
@@ -150,8 +145,7 @@ def download_project_async(mc, project_path, directory, project_version=None):
             project_info = latest_proj_info
 
     except ClientError as e:
-        file_path = _cleanup_failed_download(directory, mp)
-        e.log_file = file_path
+        _cleanup_failed_download(directory, mp)
         raise e
 
     version = project_info["version"] if project_info["version"] else "v0"
@@ -203,10 +197,8 @@ def download_project_is_running(job):
     """
     for future in job.futures:
         if future.done() and future.exception() is not None:
-            file_path = _cleanup_failed_download(job.directory, job.mp)
-            e = future.exception()
-            e.log_file = file_path
-            raise e
+            job.failure_log_file = _cleanup_failed_download(job.directory, job.mp)
+            raise future.exception()
         if future.running():
             return True
     return False
@@ -227,10 +219,8 @@ def download_project_finalize(job):
     # make sure any exceptions from threads are not lost
     for future in job.futures:
         if future.exception() is not None:
-            file_path = _cleanup_failed_download(job.directory, job.mp)
-            e = future.exception()
-            e.log_file = file_path
-            raise e
+            job.failure_log_file = _cleanup_failed_download(job.directory, job.mp)
+            raise future.exception()
 
     job.mp.log.info("--- download finished")
 
diff --git a/mergin/test/test_client.py b/mergin/test/test_client.py
index 8f3d485..2a6c10c 100644
--- a/mergin/test/test_client.py
+++ b/mergin/test/test_client.py
@@ -13,6 +13,7 @@
 from .. import InvalidProject
 from ..client import MerginClient, ClientError, MerginProject, LoginError, decode_token_data, TokenError, ServerType
 from ..client_push import push_project_async, push_project_cancel
+from ..client_pull import download_project_async, download_project_wait, download_project_finalize
 from ..utils import (
     generate_checksum,
     get_versions_with_file_changes,
@@ -1945,3 +1946,25 @@ def test_clean_diff_files(mc):
     diff_files = glob.glob("*-diff-*", root_dir=os.path.split(mp.fpath_meta("inserted_1_A.gpkg"))[0])
 
     assert diff_files == []
+
+def test_download_failure(mc):
+    test_project = "test_download_failure"
+    project = API_USER + "/" + test_project
+    project_dir = os.path.join(TMP_DIR, test_project)
+    download_dir = os.path.join(TMP_DIR, "download", test_project)
+
+    cleanup(mc, project, [project_dir, download_dir])
+    # prepare local project
+    shutil.copytree(TEST_DATA_DIR, project_dir)
+
+    # create remote project
+    mc.create_project_and_push(test_project, directory=project_dir)
+
+    # download project async
+    with pytest.raises(IsADirectoryError):
+        job = download_project_async(mc, project, download_dir)
+        os.makedirs(os.path.join(download_dir, "base.gpkg.0"))
+        download_project_wait(job)
+        download_project_finalize(job)
+
+    assert job.failure_log_file is not None