From 7347e6caa384aa6c839a8d2ebc5c3ff4b446fe4e Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 7 Oct 2026 10:26:52 -0400 Subject: [PATCH 1/6] OpenConceptLab/ocl_issues#2864 | Name export downloads with full repo coordinates The export GET signs its redirect URL with a Content-Disposition, so every client saves the file as [owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip, e.g. orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip. lastUpdated comes from the storage key, which is unchanged. Co-Authored-By: Claude Opus 5.5 --- core/common/mixins.py | 3 +- core/common/models.py | 22 ++++++++ core/integration_tests/tests_collections.py | 15 ++++-- core/integration_tests/tests_sources.py | 15 ++++-- core/repos/tests.py | 58 ++++++++++++++++++++- core/services/storages/cloud/aws.py | 8 ++- core/services/storages/cloud/azure.py | 3 +- core/services/storages/cloud/core.py | 4 +- core/services/storages/cloud/minio.py | 9 +++- core/services/storages/cloud/tests.py | 32 +++++++++++- 10 files changed, 153 insertions(+), 16 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 3feb5291e..8daf2f2b2 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -1058,7 +1058,8 @@ def get(self, request, *args, **kwargs): # pylint: disable=unused-argument if version.has_export(): export_path = version.get_export_path() - export_url = get_export_service().url_for(export_path) + export_url = get_export_service().url_for( + export_path, filename=version.get_export_download_name(export_path)) if export_url: return redirect(export_url) logger.error( diff --git a/core/common/models.py b/core/common/models.py index bdbd6a45b..9cf474be3 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1,4 +1,5 @@ import logging +import re import time from celery.result import AsyncResult @@ -1400,6 +1401,27 @@ def get_export_path(self): service = get_export_service() return service.get_last_key_from_path(self.get_version_export_path(suffix=None)) or self.version_export_path + def get_export_download_name(self, export_path): + """ + Name a client saves the export under, set on the signed URL: + [owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip + lastUpdated is read from the storage key, so the name matches the export's content. + """ + owner = self.parent + parts = [ + f'{owner.get_url_kwarg()}s', owner.mnemonic, + f'{self.get_resource_url_kwarg()}s', self.mnemonic, + self.version, + ] + expansion = get(self, 'expansion.mnemonic') + if expansion: + parts.append(expansion) + last_update = export_path.split('/')[-1].removesuffix('.zip').split('.')[-1] + if not re.fullmatch(r'\d{4}-\d{2}-\d{2}_\d{6}', last_update): + last_update = self.last_child_update.strftime('%Y-%m-%d_%H%M%S') + parts.append(last_update) + return '_'.join(parts) + '.zip' + def has_export(self): service = get_export_service() if self.is_head: diff --git a/core/integration_tests/tests_collections.py b/core/integration_tests/tests_collections.py index a8e7d6f8e..71e9fcdae 100644 --- a/core/integration_tests/tests_collections.py +++ b/core/integration_tests/tests_collections.py @@ -2700,7 +2700,10 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, self.assertEqual(response['Location'], 'https://signed.example/coll-v1.zip') s3_has_path_mock.assert_called_once_with("users/username/username_coll_v1.") s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_coll_v1.") - s3_url_for_mock.assert_called_once_with(f'users/username/username_coll_v1.{self.v1_updated_at}.zip') + s3_url_for_mock.assert_called_once_with( + f'users/username/username_coll_v1.{self.v1_updated_at}.zip', + filename=f'users_username_collections_coll_v1_{self.v1_updated_at}.zip' + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -2717,7 +2720,10 @@ def test_get_302_head(self, s3_exists_mock, s3_url_for_mock): self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/coll-head.zip') s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_collections_coll_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -2734,7 +2740,10 @@ def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_ self.assertEqual(response.status_code, 500) self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'}) s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_collections_coll_HEAD_{self.HEAD_updated_at}.zip" + ) def test_get_405(self): random_user = UserProfileFactory() diff --git a/core/integration_tests/tests_sources.py b/core/integration_tests/tests_sources.py index b7343468b..feb6ad5d8 100644 --- a/core/integration_tests/tests_sources.py +++ b/core/integration_tests/tests_sources.py @@ -1169,7 +1169,10 @@ def test_get_302_head(self, s3_exists_mock, s3_url_for_mock): self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/head.zip') s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_sources_source1_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.get_last_key_from_path') @@ -1189,7 +1192,10 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, self.assertEqual(response['Location'], 'https://signed.example/v1.zip') s3_has_path_mock.assert_called_once_with("users/username/username_source1_v1.") s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_source1_v1.") - s3_url_for_mock.assert_called_once_with(f'users/username/username_source1_v1.{self.v1_updated_at}.zip') + s3_url_for_mock.assert_called_once_with( + f'users/username/username_source1_v1.{self.v1_updated_at}.zip', + filename=f'users_username_sources_source1_v1_{self.v1_updated_at}.zip' + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') @@ -1206,7 +1212,10 @@ def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_ self.assertEqual(response.status_code, 500) self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'}) s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") - s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip") + s3_url_for_mock.assert_called_once_with( + f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip", + filename=f"users_username_sources_source1_HEAD_{self.HEAD_updated_at}.zip" + ) @patch('core.sources.models.Source.is_exporting', new_callable=PropertyMock) @patch('core.services.storages.cloud.aws.S3.exists') diff --git a/core/repos/tests.py b/core/repos/tests.py index 3a222fbc9..7d3211c10 100644 --- a/core/repos/tests.py +++ b/core/repos/tests.py @@ -7,7 +7,7 @@ from core.collections.documents import CollectionDocument from core.collections.models import Collection -from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory +from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory, ExpansionFactory from core.common.constants import HEAD from core.common.tests import OCLAPITestCase, OCLTestCase from core.orgs.tests.factories import OrganizationFactory @@ -115,6 +115,62 @@ def test_get_external_export_path_normalizes_invalid_filename_characters(self): ) +class RepoExportDownloadNameTest(OCLTestCase): + def test_org_source_version(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + export_path = source_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual(export_path, 'orgs/PIH/PIH_PIH_v1.8.24.2026-09-30_123456.zip') + self.assertEqual( + source_v1.get_export_download_name(export_path), 'orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + def test_user_collection_version_with_expansion(self): + collection_v1 = UserCollectionFactory( + user=UserProfileFactory(username='jdoe'), mnemonic='Concepts', version='1.0') + expansion = ExpansionFactory(collection_version=collection_v1, mnemonic='autoexpand-1.0') + collection_v1.expansion_uri = expansion.uri + collection_v1.save() + export_path = collection_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual( + collection_v1.get_export_download_name(export_path), + 'users_jdoe_collections_Concepts_1.0_autoexpand-1.0_2026-09-30_123456.zip' + ) + + def test_source_head(self): + source = OrganizationSourceFactory(organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH') + last_update = source.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertEqual( + source.get_export_download_name(source.version_export_path), + f'orgs_PIH_sources_PIH_HEAD_{last_update}.zip' + ) + + def test_collection_head_with_expansion(self): + collection = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIHEMR_Concepts') + expansion = ExpansionFactory(collection_version=collection, mnemonic='autoexpand-HEAD') + collection.expansion_uri = expansion.uri + collection.save() + last_update = collection.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertEqual( + collection.get_export_download_name(collection.version_export_path), + f'orgs_PIH_collections_PIHEMR_Concepts_HEAD_autoexpand-HEAD_{last_update}.zip' + ) + + def test_key_without_timestamp_uses_last_child_update(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + last_update = source_v1.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertEqual( + source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip'), + f'orgs_PIH_sources_PIH_1.8.24_{last_update}.zip' + ) + + class UserOrganizationRepoListViewTest(OCLAPITestCase): def test_get(self): CollectionDocument._index.delete() # pylint: disable=protected-access diff --git a/core/services/storages/cloud/aws.py b/core/services/storages/cloud/aws.py index b89a35364..c7b332865 100644 --- a/core/services/storages/cloud/aws.py +++ b/core/services/storages/cloud/aws.py @@ -5,6 +5,7 @@ from botocore.exceptions import ClientError, NoCredentialsError, WaiterError from django.conf import settings from django.core.files.base import ContentFile +from django.utils.http import content_disposition_header from pydash import get from core.services.storages.cloud.core import CloudStorageServiceInterface @@ -64,8 +65,11 @@ def upload_base64( # pylint: disable=too-many-arguments,inconsistent-return-sta return file_name_with_ext - def url_for(self, file_path): - return self._generate_signed_url(self.GET, file_path) if file_path else None + def url_for(self, file_path, filename=None): + if not file_path: + return None + metadata = {'ResponseContentDisposition': content_disposition_header(True, filename)} if filename else None + return self._generate_signed_url(self.GET, file_path, metadata) def public_url_for(self, file_path): url = f"http://{settings.AWS_STORAGE_BUCKET_NAME}.s3.amazonaws.com/{file_path}" diff --git a/core/services/storages/cloud/azure.py b/core/services/storages/cloud/azure.py index 524df1d4e..f60792e17 100644 --- a/core/services/storages/cloud/azure.py +++ b/core/services/storages/cloud/azure.py @@ -19,7 +19,8 @@ def __init__(self): def public_url_for(self, file_path): return f"https://{self.account_name}.blob.core.windows.net/{self.container_name}/{file_path}" - def url_for(self, file_path): + def url_for(self, file_path, filename=None): # pylint: disable=unused-argument + # an unsigned blob URL can't carry a download name, so filename is ignored return self.public_url_for(file_path) def exists(self, key): diff --git a/core/services/storages/cloud/core.py b/core/services/storages/cloud/core.py index 22dd0edc1..c467a497e 100644 --- a/core/services/storages/cloud/core.py +++ b/core/services/storages/cloud/core.py @@ -21,9 +21,9 @@ def upload_base64(self, doc_base64, file_name, append_extension=True, public_rea Uploads base64 file content to file_name """ - def url_for(self, file_path): + def url_for(self, file_path, filename=None): """ - Returns signed url for file_path + Returns signed url for file_path; filename, if given, is the download name the response carries """ def public_url_for(self, file_path): diff --git a/core/services/storages/cloud/minio.py b/core/services/storages/cloud/minio.py index 6a278874e..f41c75e0a 100644 --- a/core/services/storages/cloud/minio.py +++ b/core/services/storages/cloud/minio.py @@ -2,6 +2,7 @@ import mimetypes from io import BytesIO +from django.utils.http import content_disposition_header from minio import Minio, S3Error from minio.deleteobjects import DeleteObject from pydash import get @@ -85,15 +86,19 @@ def upload_base64(self, doc_base64, file_name, append_extension=True, public_rea except S3Error as e: raise Exception(f"Could not upload base64 file {file_name_with_ext} to MinIO. Error: {e}") from e # pylint: disable=broad-exception-raised - def url_for(self, file_path): + def url_for(self, file_path, filename=None): """ Generates a presigned URL for the given file using the external client so the signature is computed with the publicly accessible host (MINIO_EXTERNAL_ENDPOINT). Falls back to the internal client when no external endpoint is configured. + filename, if given, is the download name the response carries. """ + response_headers = { + 'response-content-disposition': content_disposition_header(True, filename) + } if filename else None try: return self._presign_client.get_presigned_url( - method='GET', bucket_name=self.bucket_name, object_name=file_path + method='GET', bucket_name=self.bucket_name, object_name=file_path, response_headers=response_headers ) if file_path else None except S3Error as e: raise Exception(f"Could not generate presigned URL for file {file_path}. Error: {e}") from e # pylint: disable=broad-exception-raised diff --git a/core/services/storages/cloud/tests.py b/core/services/storages/cloud/tests.py index 255a12218..9af1297c6 100644 --- a/core/services/storages/cloud/tests.py +++ b/core/services/storages/cloud/tests.py @@ -189,6 +189,20 @@ def test_url_for(self): 'X-Amz-Expires=' in _url ) + @mock_aws + def test_url_for_with_filename(self): + _conn = boto3.resource('s3', region_name='us-east-1') + _conn.create_bucket(Bucket='oclapi2-dev') + + _url = S3().url_for('some/path', filename='orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + self.assertIn( + 'response-content-disposition=attachment%3B%20filename%3D%22' + 'orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip%22', + _url + ) + self.assertIn('&X-Amz-Signature=', _url) + def test_public_url_for(self): self.assertEqual( S3().public_url_for('some/path').replace('https://', 'http://'), @@ -451,7 +465,23 @@ def test_url_for(self, mock_minio_client): # Assert that the URL is generated correctly self.assertEqual(url, "http://mock_url") mock_client.get_presigned_url.assert_called_once_with(method='GET', bucket_name="bucket", - object_name=file_key) + object_name=file_key, response_headers=None) + + @patch("core.services.storages.cloud.minio.Minio") + def test_url_for_with_filename(self, mock_minio_client): + mock_client = mock_minio_client.return_value + mock_client.get_presigned_url = MagicMock(return_value="http://mock_url") + + url = MinIO().url_for('test-file.zip', filename='orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') + + self.assertEqual(url, "http://mock_url") + mock_client.get_presigned_url.assert_called_once_with( + method='GET', bucket_name="bucket", object_name='test-file.zip', + response_headers={ + 'response-content-disposition': + 'attachment; filename="orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip"' + } + ) @patch("core.services.storages.cloud.minio.Minio", Mock()) def test_public_url_for(self): From b7a4ac7a04f1a90e4b7b0b0ff5a470d836d258e8 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 7 Oct 2026 10:37:20 -0400 Subject: [PATCH 2/6] OpenConceptLab/ocl_issues#2864 | Export name: timestamp only from the key, filename-safe characters - lastUpdated is read only from the storage key (current or legacy 14-digit form). A key without one gets no name, so the download keeps its storage name rather than a timestamp guessed from the live repo. - Characters other than letters, digits and . _ - @ become '-', so names stay ASCII and parse in TBv2, curl -OJ and on Windows (56 prod versions have spaces, ':', '&', ',', '[]' or non-ASCII). Addresses Codex pass 1 on oclapi2#929. Co-Authored-By: Claude Opus 5.5 --- core/common/models.py | 20 +++++++++++++++----- core/repos/tests.py | 23 +++++++++++++++++++---- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/core/common/models.py b/core/common/models.py index 9cf474be3..f95fe1bd6 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1,6 +1,7 @@ import logging import re import time +from datetime import datetime from celery.result import AsyncResult from celery_once import AlreadyQueued @@ -1405,8 +1406,20 @@ def get_export_download_name(self, export_path): """ Name a client saves the export under, set on the signed URL: [owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip - lastUpdated is read from the storage key, so the name matches the export's content. + lastUpdated is read from the storage key, so the name matches the export's content. A key without one + gets no name (None), and the download keeps its storage name. Characters that aren't safe in a filename + become '-'. """ + key_timestamp = export_path.split('/')[-1].removesuffix('.zip').split('.')[-1] + for key_format in ('%Y-%m-%d_%H%M%S', '%Y%m%d%H%M%S'): + try: + last_update = datetime.strptime(key_timestamp, key_format).strftime('%Y-%m-%d_%H%M%S') + break + except ValueError: + continue + else: + return None + owner = self.parent parts = [ f'{owner.get_url_kwarg()}s', owner.mnemonic, @@ -1416,11 +1429,8 @@ def get_export_download_name(self, export_path): expansion = get(self, 'expansion.mnemonic') if expansion: parts.append(expansion) - last_update = export_path.split('/')[-1].removesuffix('.zip').split('.')[-1] - if not re.fullmatch(r'\d{4}-\d{2}-\d{2}_\d{6}', last_update): - last_update = self.last_child_update.strftime('%Y-%m-%d_%H%M%S') parts.append(last_update) - return '_'.join(parts) + '.zip' + return '_'.join(re.sub(r'[^A-Za-z0-9._@-]+', '-', part) for part in parts) + '.zip' def has_export(self): service = get_export_service() diff --git a/core/repos/tests.py b/core/repos/tests.py index 7d3211c10..efd8e6800 100644 --- a/core/repos/tests.py +++ b/core/repos/tests.py @@ -160,14 +160,29 @@ def test_collection_head_with_expansion(self): f'orgs_PIH_collections_PIHEMR_Concepts_HEAD_autoexpand-HEAD_{last_update}.zip' ) - def test_key_without_timestamp_uses_last_child_update(self): + def test_legacy_key_timestamp(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='CIEL'), mnemonic='CIEL', version='v2016-08-22') + + self.assertEqual( + source_v1.get_export_download_name('orgs/CIEL/CIEL_CIEL_v2016-08-22.20150516122820.zip'), + 'orgs_CIEL_sources_CIEL_v2016-08-22_2015-05-16_122820.zip' + ) + + def test_key_without_timestamp_gets_no_name(self): source_v1 = OrganizationSourceFactory( organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') - last_update = source_v1.last_child_update.strftime('%Y-%m-%d_%H%M%S') + + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip')) + + def test_unsafe_version_characters_become_hyphens(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.0 beta: [draft] "x";100%') + export_path = source_v1.get_version_export_path(suffix='2026-09-30_123456.zip') self.assertEqual( - source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip'), - f'orgs_PIH_sources_PIH_1.8.24_{last_update}.zip' + source_v1.get_export_download_name(export_path), + 'orgs_PIH_sources_PIH_1.0-beta-draft-x-100-_2026-09-30_123456.zip' ) From a99559bf8f15c5d0ef8e583a81159772ba64e5b9 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Wed, 7 Oct 2026 10:46:40 -0400 Subject: [PATCH 3/6] OpenConceptLab/ocl_issues#2864 | Export name: read the timestamp after the key's known prefix A date-like version (1.20260930123456) in a key without a timestamp was read as the timestamp. Addresses Codex pass 2 on oclapi2#929. Co-Authored-By: Claude Opus 5.5 --- core/common/models.py | 3 ++- core/repos/tests.py | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/core/common/models.py b/core/common/models.py index f95fe1bd6..70edd903d 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1410,7 +1410,8 @@ def get_export_download_name(self, export_path): gets no name (None), and the download keeps its storage name. Characters that aren't safe in a filename become '-'. """ - key_timestamp = export_path.split('/')[-1].removesuffix('.zip').split('.')[-1] + prefix = self.get_version_export_path(suffix=None) + key_timestamp = export_path[len(prefix):].removesuffix('.zip') if export_path.startswith(prefix) else '' for key_format in ('%Y-%m-%d_%H%M%S', '%Y%m%d%H%M%S'): try: last_update = datetime.strptime(key_timestamp, key_format).strftime('%Y-%m-%d_%H%M%S') diff --git a/core/repos/tests.py b/core/repos/tests.py index efd8e6800..941415a50 100644 --- a/core/repos/tests.py +++ b/core/repos/tests.py @@ -175,6 +175,13 @@ def test_key_without_timestamp_gets_no_name(self): self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip')) + def test_date_like_version_is_not_read_as_the_timestamp(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.20260930123456') + + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.zip')) + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.1.zip')) + def test_unsafe_version_characters_become_hyphens(self): source_v1 = OrganizationSourceFactory( organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.0 beta: [draft] "x";100%') From b2402fe89fedbe4811c6b0110b55b7cc249f9e8c Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Fri, 9 Oct 2026 15:18:32 -0400 Subject: [PATCH 4/6] OpenConceptLab/ocl_issues#2864 | Export name: one regex for the key timestamp, name parts from the uri From the review on OpenConceptLab/oclapi2#929: - One regex reads the key's timestamp, in the current or the legacy form. It replaces the strptime loop and the datetime import. - The owner, repo and expansion parts come from expansion_uri or uri, and the version from the model. This drops the second expansion query; the prefix still needs one. Two changes to the suggestion: - The regex stays anchored to the version's key prefix. get_export_path() lists keys by prefix, so 1.8.24's lookup can return 1.8.24.1's export, and reading the key's last '.' part would name that file 1.8.24. Test: test_key_of_another_version_gets_no_name. - parts[4:6] replaces the uri's version, and an expansion uri's 'expansions' segment, with the model's version. replace('/expansions/', '/') would also drop an owner or repo named 'expansions'. Test: test_mnemonics_named_expansions. The anchor also leaves a timestamp-less key for a 14-digit version unnamed, so test_date_like_version keeps that case and adds a real key timestamp. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EF3UX7Xk4JwPkZ7yjU29X5 --- core/common/models.py | 36 ++++++++++++++---------------------- core/repos/tests.py | 26 +++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/core/common/models.py b/core/common/models.py index 70edd903d..ea96027d7 100644 --- a/core/common/models.py +++ b/core/common/models.py @@ -1,7 +1,6 @@ import logging import re import time -from datetime import datetime from celery.result import AsyncResult from celery_once import AlreadyQueued @@ -1407,30 +1406,23 @@ def get_export_download_name(self, export_path): Name a client saves the export under, set on the signed URL: [owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip lastUpdated is read from the storage key, so the name matches the export's content. A key without one - gets no name (None), and the download keeps its storage name. Characters that aren't safe in a filename - become '-'. + right after this version's prefix gets no name (None), and the download keeps its storage name. Characters + that aren't safe in a filename become '-'. """ - prefix = self.get_version_export_path(suffix=None) - key_timestamp = export_path[len(prefix):].removesuffix('.zip') if export_path.startswith(prefix) else '' - for key_format in ('%Y-%m-%d_%H%M%S', '%Y%m%d%H%M%S'): - try: - last_update = datetime.strptime(key_timestamp, key_format).strftime('%Y-%m-%d_%H%M%S') - break - except ValueError: - continue - else: + # current (2026-09-30_123456) or legacy (20260930123456) key timestamp, anchored to the prefix because a key + # listed under it can be another version's (1.8.24.1's under 1.8.24's) + match = re.fullmatch( + re.escape(self.get_version_export_path(suffix=None)) + r'(\d{4})-?(\d{2})-?(\d{2})_?(\d{6})\.zip', + export_path) + if not match: return None - owner = self.parent - parts = [ - f'{owner.get_url_kwarg()}s', owner.mnemonic, - f'{self.get_resource_url_kwarg()}s', self.mnemonic, - self.version, - ] - expansion = get(self, 'expansion.mnemonic') - if expansion: - parts.append(expansion) - parts.append(last_update) + parts = (get(self, 'expansion_uri') or self.uri).strip('/').split('/') + # the uri percent-encodes the version, HEAD's uri has none and an expansion uri follows it with 'expansions', + # so the version comes from the model + parts[4:6] = [self.version] + year, month, day, time_of_day = match.groups() + parts.append(f'{year}-{month}-{day}_{time_of_day}') return '_'.join(re.sub(r'[^A-Za-z0-9._@-]+', '-', part) for part in parts) + '.zip' def has_export(self): diff --git a/core/repos/tests.py b/core/repos/tests.py index 941415a50..98b25d498 100644 --- a/core/repos/tests.py +++ b/core/repos/tests.py @@ -175,13 +175,37 @@ def test_key_without_timestamp_gets_no_name(self): self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.zip')) - def test_date_like_version_is_not_read_as_the_timestamp(self): + def test_date_like_version(self): source_v1 = OrganizationSourceFactory( organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.20260930123456') + self.assertEqual( + source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.2026-10-01_090000.zip'), + 'orgs_PIH_sources_PIH_1.20260930123456_2026-10-01_090000.zip' + ) self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.zip')) self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.20260930123456.1.zip')) + def test_key_of_another_version_gets_no_name(self): + source_v1 = OrganizationSourceFactory( + organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.8.24') + + # get_export_path() lists keys by prefix, so 1.8.24's lookup can return 1.8.24.1's export + self.assertIsNone(source_v1.get_export_download_name('orgs/PIH/PIH_PIH_v1.8.24.1.2026-10-01_090000.zip')) + + def test_mnemonics_named_expansions(self): + collection_v1 = OrganizationCollectionFactory( + organization=OrganizationFactory(mnemonic='expansions'), mnemonic='expansions', version='1.0') + expansion = ExpansionFactory(collection_version=collection_v1, mnemonic='expansions') + collection_v1.expansion_uri = expansion.uri + collection_v1.save() + export_path = collection_v1.get_version_export_path(suffix='2026-09-30_123456.zip') + + self.assertEqual( + collection_v1.get_export_download_name(export_path), + 'orgs_expansions_collections_expansions_1.0_expansions_2026-09-30_123456.zip' + ) + def test_unsafe_version_characters_become_hyphens(self): source_v1 = OrganizationSourceFactory( organization=OrganizationFactory(mnemonic='PIH'), mnemonic='PIH', version='1.0 beta: [draft] "x";100%') From cbd06ecc9bb10e3d92bc6cffee827231a05b26a0 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Fri, 9 Oct 2026 15:18:32 -0400 Subject: [PATCH 5/6] OpenConceptLab/ocl_issues#2864 | Export GET: log the storage key with the download name The download name no longer matches the storage key. The GET now logs both on one line, so a downloaded file can be traced back to its object (review on OpenConceptLab/oclapi2#929). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EF3UX7Xk4JwPkZ7yjU29X5 --- core/common/mixins.py | 5 +++-- core/integration_tests/tests_sources.py | 16 +++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/core/common/mixins.py b/core/common/mixins.py index 8daf2f2b2..4b18afa10 100644 --- a/core/common/mixins.py +++ b/core/common/mixins.py @@ -1058,8 +1058,9 @@ def get(self, request, *args, **kwargs): # pylint: disable=unused-argument if version.has_export(): export_path = version.get_export_path() - export_url = get_export_service().url_for( - export_path, filename=version.get_export_download_name(export_path)) + download_name = version.get_export_download_name(export_path) + logger.info('Export %s downloads as %s', export_path, download_name) + export_url = get_export_service().url_for(export_path, filename=download_name) if export_url: return redirect(export_url) logger.error( diff --git a/core/integration_tests/tests_sources.py b/core/integration_tests/tests_sources.py index feb6ad5d8..82b21a13f 100644 --- a/core/integration_tests/tests_sources.py +++ b/core/integration_tests/tests_sources.py @@ -1182,11 +1182,12 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_has_path_mock.return_value = True s3_get_last_key_from_path_mock.return_value = f'users/username/username_source1_v1.{self.v1_updated_at}.zip' - response = self.client.get( - self.source_v1.uri + 'export/', - HTTP_AUTHORIZATION='Token ' + self.token, - format='json' - ) + with self.assertLogs('oclapi', level='INFO') as logs: + response = self.client.get( + self.source_v1.uri + 'export/', + HTTP_AUTHORIZATION='Token ' + self.token, + format='json' + ) self.assertEqual(response.status_code, 302) self.assertEqual(response['Location'], 'https://signed.example/v1.zip') @@ -1196,6 +1197,11 @@ def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, f'users/username/username_source1_v1.{self.v1_updated_at}.zip', filename=f'users_username_sources_source1_v1_{self.v1_updated_at}.zip' ) + self.assertIn( + f'INFO:oclapi:Export users/username/username_source1_v1.{self.v1_updated_at}.zip downloads as ' + f'users_username_sources_source1_v1_{self.v1_updated_at}.zip', + logs.output + ) @patch('core.services.storages.cloud.aws.S3.url_for') @patch('core.services.storages.cloud.aws.S3.exists') From 85085629fb87a99343e724935d1e11e8b5c2a463 Mon Sep 17 00:00:00 2001 From: Jonathan Payne Date: Fri, 9 Oct 2026 15:38:44 -0400 Subject: [PATCH 6/6] OpenConceptLab/ocl_issues#2864 | Export URL: simplification-review cuts - S3.url_for keeps master's one-line `if file_path else None` guard. - test_url_for_with_filename drops a bucket it never used: presigning a URL doesn't touch the bucket. From the simplification review on OpenConceptLab/oclapi2#929. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01EF3UX7Xk4JwPkZ7yjU29X5 --- core/services/storages/cloud/aws.py | 4 +--- core/services/storages/cloud/tests.py | 3 --- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/core/services/storages/cloud/aws.py b/core/services/storages/cloud/aws.py index c7b332865..2b0f592e4 100644 --- a/core/services/storages/cloud/aws.py +++ b/core/services/storages/cloud/aws.py @@ -66,10 +66,8 @@ def upload_base64( # pylint: disable=too-many-arguments,inconsistent-return-sta return file_name_with_ext def url_for(self, file_path, filename=None): - if not file_path: - return None metadata = {'ResponseContentDisposition': content_disposition_header(True, filename)} if filename else None - return self._generate_signed_url(self.GET, file_path, metadata) + return self._generate_signed_url(self.GET, file_path, metadata) if file_path else None def public_url_for(self, file_path): url = f"http://{settings.AWS_STORAGE_BUCKET_NAME}.s3.amazonaws.com/{file_path}" diff --git a/core/services/storages/cloud/tests.py b/core/services/storages/cloud/tests.py index 9af1297c6..2228229db 100644 --- a/core/services/storages/cloud/tests.py +++ b/core/services/storages/cloud/tests.py @@ -191,9 +191,6 @@ def test_url_for(self): @mock_aws def test_url_for_with_filename(self): - _conn = boto3.resource('s3', region_name='us-east-1') - _conn.create_bucket(Bucket='oclapi2-dev') - _url = S3().url_for('some/path', filename='orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip') self.assertIn(