From fb9cbaa9b9650e9bde53d46740fb4e1cdb2aa0c0 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Mon, 18 Aug 2025 17:29:38 +0200 Subject: [PATCH 1/4] Add post request to explicitely prepare download archive --- server/mergin/sync/private_api.yaml | 36 +++++++--- server/mergin/sync/private_api_controller.py | 46 ++++++++---- server/mergin/sync/tasks.py | 2 +- .../lib/src/modules/project/projectApi.ts | 4 ++ .../packages/lib/src/modules/project/store.ts | 71 +++++++++++-------- 5 files changed, 105 insertions(+), 54 deletions(-) diff --git a/server/mergin/sync/private_api.yaml b/server/mergin/sync/private_api.yaml index b5fa2c87..1c7aae0b 100644 --- a/server/mergin/sync/private_api.yaml +++ b/server/mergin/sync/private_api.yaml @@ -384,20 +384,20 @@ paths: $ref: "#/components/responses/NotFoundResp" x-openapi-router-controller: mergin.sync.private_api_controller /projects/{id}/download: + parameters: + - $ref: "#/components/parameters/ProjectId" + - name: version + in: query + description: Particular version to download + required: false + schema: + $ref: "#/components/schemas/VersionName" get: tags: - project summary: Download full project description: Download whole project folder as zip file operationId: download_project - parameters: - - $ref: "#/components/parameters/ProjectId" - - name: version - in: query - description: Particular version to download - required: false - schema: - $ref: "#/components/schemas/VersionName" responses: "200": description: Zip file @@ -417,6 +417,26 @@ paths: "404": $ref: "#/components/responses/NotFoundResp" x-openapi-router-controller: mergin.sync.private_api_controller + post: + tags: + - project + summary: Prepare project archive + description: Prepare project zip archive to download + operationId: prepare_archive + responses: + "200": + description: Archive is already prepared + "202": + description: Accepted + "400": + $ref: "#/components/responses/BadStatusResp" + "422": + $ref: "#/components/responses/UnprocessableEntity" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFoundResp" + x-openapi-router-controller: mergin.sync.private_api_controller components: responses: UnauthorizedError: diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index 13f059b9..4429d47c 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -322,7 +322,7 @@ def get_project_access(id: str): def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W0622 """Download whole project folder as zip file in any version - Return zip file if it exists, otherwise trigger background job to create it""" + Return zip file if it exists, otherwise return 202""" project = require_project_by_uuid(id, ProjectPermissions.Read) lookup_version = ( ProjectVersion.from_v_name(version) if version else project.latest_version @@ -352,17 +352,35 @@ def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W06 f"attachment; filename*=UTF-8''{file_name}" ) return resp - # GET request triggers background job if no partial zip or expired one - if request.method == "GET": - temp_zip_path = project_version.zip_path + ".partial" - # create zip if it does not exist yet or has expired - partial_exists = os.path.exists(temp_zip_path) - is_expired = partial_exists and datetime.fromtimestamp( - os.path.getmtime(temp_zip_path), tz=timezone.utc - ) < datetime.now(timezone.utc) - timedelta( - seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] - ) - if not partial_exists or is_expired: - create_project_version_zip.delay(project_version.id) - return "Project zip being prepared, please try again later", 202 + return "Project zip being prepared", 202 + + +def prepare_archive(id: str, version=None): + """Triggers background job to create project archive""" + project = require_project_by_uuid(id, ProjectPermissions.Read) + lookup_version = ( + ProjectVersion.from_v_name(version) if version else project.latest_version + ) + pv = ProjectVersion.query.filter_by( + project_id=project.id, name=lookup_version + ).first_or_404() + + if pv.project_size > current_app.config["MAX_DOWNLOAD_ARCHIVE_SIZE"]: + abort(400) + + if os.path.exists(pv.zip_path): + return "Project archive already ready", 200 + + # trigger job if no recent partial + temp_zip_path = pv.zip_path + ".partial" + partial_exists = os.path.exists(temp_zip_path) + is_expired = partial_exists and datetime.fromtimestamp( + os.path.getmtime(temp_zip_path), tz=timezone.utc + ) < datetime.now(timezone.utc) - timedelta( + seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + ) + if not partial_exists or is_expired: + create_project_version_zip.delay(pv.id) + + return "Project zip being prepared", 202 diff --git a/server/mergin/sync/tasks.py b/server/mergin/sync/tasks.py index ce92171e..770320ea 100644 --- a/server/mergin/sync/tasks.py +++ b/server/mergin/sync/tasks.py @@ -123,7 +123,7 @@ def create_project_version_zip(version_id: int): # partial zip is recent -> another job is likely running return else: - # partial zip is too old -> remove and creating new one + # partial zip is too old -> remove and create new one os.remove(zip_path) os.makedirs(os.path.dirname(zip_path), exist_ok=True) diff --git a/web-app/packages/lib/src/modules/project/projectApi.ts b/web-app/packages/lib/src/modules/project/projectApi.ts index 91b6edbb..657675c8 100644 --- a/web-app/packages/lib/src/modules/project/projectApi.ts +++ b/web-app/packages/lib/src/modules/project/projectApi.ts @@ -311,6 +311,10 @@ export const ProjectApi = { return ProjectModule.httpService.get(url, { responseType: 'blob' }) }, + async prepareArchive(url: string): Promise> { + return ProjectModule.httpService.post(url) + }, + /** Request head of file download */ async getHeadDownloadFile(url: string): Promise> { return ProjectModule.httpService.head(url) diff --git a/web-app/packages/lib/src/modules/project/store.ts b/web-app/packages/lib/src/modules/project/store.ts index 182bdea2..f6603849 100644 --- a/web-app/packages/lib/src/modules/project/store.ts +++ b/web-app/packages/lib/src/modules/project/store.ts @@ -707,6 +707,7 @@ export const useProjectStore = defineStore('projectModule', { }, async downloadArchive(payload: DownloadPayload) { + console.log("You are here") const notificationStore = useNotificationStore() await this.cancelDownloadArchive() this.projectDownloadingVersion = payload.versionId @@ -719,8 +720,12 @@ export const useProjectStore = defineStore('projectModule', { const delays = [...Array(3).fill(1000), ...Array(3).fill(3000), 5000] let retryCount = 0 - const pollDownloadArchive = async () => { - try { + try { + // STEP 1: request archive creation + await ProjectApi.prepareArchive(payload.url) + + // STEP 2: start polling HEAD for readiness + const pollDownloadArchive = async () => { if (retryCount > 125) { notificationStore.warn({ text: exceedMessage, @@ -729,38 +734,42 @@ export const useProjectStore = defineStore('projectModule', { await this.cancelDownloadArchive() return } - - const head = await ProjectApi.getHeadDownloadFile(payload.url) - const polling = head.status == 202 - if (polling) { - const delay = delays[Math.min(retryCount, delays.length - 1)] // Select delay based on retry count - retryCount++ // Increment retry count - downloadArchiveTimeout = setTimeout(async () => { - await pollDownloadArchive() - }, delay) - return - } - - // Use browser download instead of playing around with the blob - FileSaver.saveAs(payload.url) - notificationStore.closeNotification() - this.cancelDownloadArchive() - } catch (e) { - if (axios.isAxiosError(e) && e.response?.status === 400) { - notificationStore.error({ - group: 'download-large-error', - text: '', - life: 6000 - }) - } else { - notificationStore.error({ - text: errorMessage - }) + try { + const head = await ProjectApi.getHeadDownloadFile(payload.url) + const polling = head.status === 202 + if (polling) { + const delay = delays[Math.min(retryCount, delays.length - 1)] // Select delay based on retry count + retryCount++ // Increment retry count + downloadArchiveTimeout = setTimeout(async () => { + await pollDownloadArchive() + }, delay) + return + } + + // Use browser download instead of playing around with the blob + FileSaver.saveAs(payload.url) + notificationStore.closeNotification() + this.cancelDownloadArchive() + } catch (e) { + if (axios.isAxiosError(e) && e.response?.status === 400) { + notificationStore.error({ + group: 'download-large-error', + text: '', + life: 6000 + }) + } else { + notificationStore.error({ + text: errorMessage + }) + } + this.cancelDownloadArchive() } - this.cancelDownloadArchive() } + pollDownloadArchive() + } catch (e) { + notificationStore.error({ text: errorMessage }) + this.cancelDownloadArchive() } - pollDownloadArchive() }, async cancelDownloadArchive() { From 4d923d10e40ed2e5e99053f38c9b44a221037abd Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 19 Aug 2025 13:28:48 +0200 Subject: [PATCH 2/4] Fix tests --- .../mergin/tests/test_private_project_api.py | 154 ++++++++++-------- 1 file changed, 82 insertions(+), 72 deletions(-) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 54575114..1755055a 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -424,27 +424,102 @@ def test_admin_project_list(client): assert len(resp.json["items"]) == 14 -test_download_proj_data = [ - # zips do not exist, version not specified -> call celery task to create zip with latest version +# test_download_proj_data = [ +# # project archive does not exist yet -> 202 +# (0, None, 202), +# # specified version does not exist -> 404 +# (0, "v100", 404), +# # zip is ready to download +# (1, None, 200), +# ] +# +# +# @pytest.mark.parametrize( +# "zipfile,version,exp_resp", test_download_proj_data +# ) +# @patch("mergin.sync.tasks.create_project_version_zip.delay") +def test_download_project( + # mock_create_zip, + client, + # zipfile, + # partial, + # expired, + # version, + # exp_resp, + # exp_call, + diff_project, +): + """Test download endpoint responses and celery task calling""" + resp = client.head( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="", + ) + ) + # zip archive does not exist yet + assert resp.status_code == 202 + project_version = diff_project.get_latest_version() + # pretend archive has been created + zip_path = Path(project_version.zip_path) + if zip_path.parent.exists(): + shutil.rmtree(zip_path.parent, ignore_errors=True) + zip_path.parent.mkdir(parents=True, exist_ok=True) + zip_path.touch() + resp = client.head( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="", + ) + ) + # zip archive is ready -> download can start + assert resp.status_code == 200 + + +def test_large_project_download_fail(client, diff_project): + """Test downloading too large project is refused""" + resp = client.get( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="v1", + ) + ) + assert resp.status_code == 202 + # pretend testing project to be too large by lowering limit + client.application.config["MAX_DOWNLOAD_ARCHIVE_SIZE"] = 10 + resp = client.get( + url_for( + "/app.mergin_sync_private_api_controller_download_project", + id=diff_project.id, + version="v1", + ) + ) + assert resp.status_code == 400 + + +test_prepare_proj_data = [ + # zips do not exist, version not specified -> trigger celery to create zip with latest version (0, 0, 0, None, 202, 1), # expired partial zip exists -> call celery task (0, 1, 1, None, 202, 1), - # valid partial zip exists -> return, do not call celery + # valid partial zip exists -> do not call celery (0, 1, 0, None, 202, 0), # zips do not exist, version specified -> call celery task with specified version (0, 0, 0, "v1", 202, 1), # specified version does not exist -> 404 (0, 0, 0, "v100", 404, 0), - # zip is ready to download + # zip is already prepared to download -> do not call celery (1, 0, 0, None, 200, 0), ] @pytest.mark.parametrize( - "zipfile,partial,expired,version,exp_resp,exp_call", test_download_proj_data + "zipfile,partial,expired,version,exp_resp,exp_call", test_prepare_proj_data ) @patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_download_project( +def test_prepare_archive( mock_create_zip, client, zipfile, @@ -474,7 +549,7 @@ def test_download_project( seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] + 1 ) modify_file_times(temp_zip_path, new_time) - resp = client.get( + resp = client.post( url_for( "/app.mergin_sync_private_api_controller_download_project", id=diff_project.id, @@ -487,68 +562,3 @@ def test_download_project( call_args, _ = mock_create_zip.call_args args = call_args[0] assert args == diff_project.latest_version - - -def test_large_project_download_fail(client, diff_project): - """Test downloading too large project is refused""" - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version="v1", - ) - ) - assert resp.status_code == 202 - # pretend testing project to be too large by lowering limit - client.application.config["MAX_DOWNLOAD_ARCHIVE_SIZE"] = 10 - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - version="v1", - ) - ) - assert resp.status_code == 400 - - -@patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_remove_abandoned_zip(mock_prepare_zip, client, diff_project): - """Test project download removes partial zip which is inactive for some time""" - latest_version = diff_project.get_latest_version() - # pretend an incomplete zip remained - partial_zip_path = latest_version.zip_path + ".partial" - os.makedirs(os.path.dirname(partial_zip_path), exist_ok=True) - os.mknod(partial_zip_path) - assert os.path.exists(partial_zip_path) - # pretend abandoned partial zip by lowering the expiration limit - client.application.config["PARTIAL_ZIP_EXPIRATION"] = 0 - # download should remove it (move to temp folder) and call a celery task which will try to create a correct zip - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert mock_prepare_zip.called - assert resp.status_code == 202 - - -@patch("mergin.sync.tasks.create_project_version_zip.delay") -def test_download_project_request_method(mock_prepare_zip, client, diff_project): - """Test head request does not create a celery job""" - resp = client.head( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert not mock_prepare_zip.called - assert resp.status_code == 202 - resp = client.get( - url_for( - "/app.mergin_sync_private_api_controller_download_project", - id=diff_project.id, - ) - ) - assert mock_prepare_zip.called - assert resp.status_code == 202 From f83bd45af855bbee9a1f79f0ec51e5b661809326 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 19 Aug 2025 13:45:31 +0200 Subject: [PATCH 3/4] cleanup --- .../mergin/tests/test_private_project_api.py | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 1755055a..43d1b6dd 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -424,32 +424,11 @@ def test_admin_project_list(client): assert len(resp.json["items"]) == 14 -# test_download_proj_data = [ -# # project archive does not exist yet -> 202 -# (0, None, 202), -# # specified version does not exist -> 404 -# (0, "v100", 404), -# # zip is ready to download -# (1, None, 200), -# ] -# -# -# @pytest.mark.parametrize( -# "zipfile,version,exp_resp", test_download_proj_data -# ) -# @patch("mergin.sync.tasks.create_project_version_zip.delay") def test_download_project( - # mock_create_zip, client, - # zipfile, - # partial, - # expired, - # version, - # exp_resp, - # exp_call, diff_project, ): - """Test download endpoint responses and celery task calling""" + """Test download endpoint responses""" resp = client.head( url_for( "/app.mergin_sync_private_api_controller_download_project", @@ -530,7 +509,7 @@ def test_prepare_archive( exp_call, diff_project, ): - """Test download endpoint responses and celery task calling""" + """Test prepare archive endpoint responses and celery task calling""" # prepare initial state according to testcase project_version = diff_project.get_latest_version() if zipfile: From 1151319bfe1dad715f749db43fcd1ecc36b0bda1 Mon Sep 17 00:00:00 2001 From: Herman Snevajs Date: Tue, 26 Aug 2025 17:33:53 +0200 Subject: [PATCH 4/4] Update response codes --- server/mergin/sync/private_api.yaml | 12 ++++++---- server/mergin/sync/private_api_controller.py | 12 ++++------ .../mergin/tests/test_private_project_api.py | 24 +++++++++---------- .../packages/lib/src/modules/project/store.ts | 1 - 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/server/mergin/sync/private_api.yaml b/server/mergin/sync/private_api.yaml index 1c7aae0b..48a88933 100644 --- a/server/mergin/sync/private_api.yaml +++ b/server/mergin/sync/private_api.yaml @@ -424,10 +424,10 @@ paths: description: Prepare project zip archive to download operationId: prepare_archive responses: - "200": - description: Archive is already prepared - "202": - description: Accepted + "201": + description: Project archive creation started + "204": + $ref: "#/components/responses/NoContent" "400": $ref: "#/components/responses/BadStatusResp" "422": @@ -456,7 +456,9 @@ components: UnsupportedMediaType: description: Payload format is in an unsupported format. ConflictResp: - description: Request could not be processed becuase of conflict in resources + description: Request could not be processed because of conflict in resources + NoContent: + description: Success. No content returned. parameters: Page: name: page diff --git a/server/mergin/sync/private_api_controller.py b/server/mergin/sync/private_api_controller.py index 4429d47c..dd870aae 100644 --- a/server/mergin/sync/private_api_controller.py +++ b/server/mergin/sync/private_api_controller.py @@ -331,9 +331,6 @@ def download_project(id: str, version=None): # noqa: E501 # pylint: disable=W06 project_id=project.id, name=lookup_version ).first_or_404("Project version does not exist") - if project_version.project_size > current_app.config["MAX_DOWNLOAD_ARCHIVE_SIZE"]: - abort(400) - # check zip is already created if os.path.exists(project_version.zip_path): if current_app.config["USE_X_ACCEL"]: @@ -370,7 +367,7 @@ def prepare_archive(id: str, version=None): abort(400) if os.path.exists(pv.zip_path): - return "Project archive already ready", 200 + return NoContent, 204 # trigger job if no recent partial temp_zip_path = pv.zip_path + ".partial" @@ -380,7 +377,8 @@ def prepare_archive(id: str, version=None): ) < datetime.now(timezone.utc) - timedelta( seconds=current_app.config["PARTIAL_ZIP_EXPIRATION"] ) - if not partial_exists or is_expired: - create_project_version_zip.delay(pv.id) + if partial_exists and not is_expired: + return NoContent, 204 - return "Project zip being prepared", 202 + create_project_version_zip.delay(pv.id) + return "Project zip creation started", 201 diff --git a/server/mergin/tests/test_private_project_api.py b/server/mergin/tests/test_private_project_api.py index 43d1b6dd..5f062ac2 100644 --- a/server/mergin/tests/test_private_project_api.py +++ b/server/mergin/tests/test_private_project_api.py @@ -456,21 +456,21 @@ def test_download_project( assert resp.status_code == 200 -def test_large_project_download_fail(client, diff_project): - """Test downloading too large project is refused""" - resp = client.get( +def test_prepare_large_project_fail(client, diff_project): + """Test asking for too large project is refused""" + resp = client.post( url_for( - "/app.mergin_sync_private_api_controller_download_project", + "/app.mergin_sync_private_api_controller_prepare_archive", id=diff_project.id, version="v1", ) ) - assert resp.status_code == 202 + assert resp.status_code == 201 # pretend testing project to be too large by lowering limit client.application.config["MAX_DOWNLOAD_ARCHIVE_SIZE"] = 10 - resp = client.get( + resp = client.post( url_for( - "/app.mergin_sync_private_api_controller_download_project", + "/app.mergin_sync_private_api_controller_prepare_archive", id=diff_project.id, version="v1", ) @@ -480,17 +480,17 @@ def test_large_project_download_fail(client, diff_project): test_prepare_proj_data = [ # zips do not exist, version not specified -> trigger celery to create zip with latest version - (0, 0, 0, None, 202, 1), + (0, 0, 0, None, 201, 1), # expired partial zip exists -> call celery task - (0, 1, 1, None, 202, 1), + (0, 1, 1, None, 201, 1), # valid partial zip exists -> do not call celery - (0, 1, 0, None, 202, 0), + (0, 1, 0, None, 204, 0), # zips do not exist, version specified -> call celery task with specified version - (0, 0, 0, "v1", 202, 1), + (0, 0, 0, "v1", 201, 1), # specified version does not exist -> 404 (0, 0, 0, "v100", 404, 0), # zip is already prepared to download -> do not call celery - (1, 0, 0, None, 200, 0), + (1, 0, 0, None, 204, 0), ] diff --git a/web-app/packages/lib/src/modules/project/store.ts b/web-app/packages/lib/src/modules/project/store.ts index f6603849..53150334 100644 --- a/web-app/packages/lib/src/modules/project/store.ts +++ b/web-app/packages/lib/src/modules/project/store.ts @@ -707,7 +707,6 @@ export const useProjectStore = defineStore('projectModule', { }, async downloadArchive(payload: DownloadPayload) { - console.log("You are here") const notificationStore = useNotificationStore() await this.cancelDownloadArchive() this.projectDownloadingVersion = payload.versionId