OpenConceptLab/ocl_issues#2864 | Name export downloads with full repo coordinates

[paynejd]

Closes OpenConceptLab/ocl_issues#2864

The API part of OpenConceptLab/ocl_issues#2864. The other parts are TBv3 (OpenConceptLab/oclweb3 5fa6141a), OpenConceptLab/ocl-cli#13 and OpenConceptLab/ocl-docs#68.

What

GET …/export/ now signs its redirect URL with a Content-Disposition, so every client saves the export under one name:

[owner-type]_[owner]_[repo-type]_[repo]_[repo-version]_[expansion]_[lastUpdated].zip

Export	Name
Source version	orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip
Collection version with its expansion	orgs_PIH_collections_PIHEMR_Concepts_1.0_autoexpand-1.0_2026-09-30_123456.zip
Source HEAD	orgs_PIH_sources_PIH_HEAD_2026-09-30_123456.zip

How

• get_export_download_name(export_path) on the repo version model builds the name in one place. lastUpdated is read from the storage key, so the name matches the export's content. A key without a timestamp right after the version's key prefix gets no name, and the download keeps its storage name.
• Characters other than letters, digits and . _ - @ become -, so every name is ASCII and needs no quoting.
• url_for(file_path, filename=None) on the storage services: S3 passes ResponseContentDisposition, MinIO passes response-content-disposition. Azure returns an unsigned public URL, which can't carry a name, so it ignores filename.
• Storage keys don't change, so existing exports stay findable and nothing needs re-exporting.

Tests

• RepoExportDownloadNameTest: an org source version, a user collection version with its expansion, a source HEAD, a collection HEAD with its expansion, and a key without a timestamp.
• S3 and MinIO url_for with a filename. The source and collection export view tests now assert the filename passed to url_for.
• CI's Pylint and Tests pass.

🤖 Generated with Claude Code

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.160.1, commit 7347e6c)

• [P2] Timestamp fallback can mislabel cached content — core/common/models.py:1420. When key parsing fails, the filename uses the live model’s last_child_update. For the timestamp-less key explicitly supported by the new test, I reproduced the same cached ZIP receiving different timestamps after changing model state. That violates the content-matching requirement. Legacy handling needs a trustworthy export timestamp rather than a live-model guess.

• [P2] Quoted versions produce truncated filenames in TBv2 — aws.py:71, also minio.py:97. Django correctly escapes quotes, but TBv2’s existing parser cannot consume that representation. With version 1 "draft", it truncates the filename before the timestamp and .zip; 1;draft also truncates. Version 100% throws URIError when read from the response header, or falls back to the old storage-key filename when read from the redirect URL. These cases need coordinated encoding/parser handling and compatibility tests.

• [P2] Unicode filenames are invisible to curl’s filename handling — aws.py:71, also minio.py:97. For version révision, Django emits only filename*=utf-8''…, with no filename= fallback. Curl explicitly does not support filename*, so -OJ cannot use the advertised name. Signing the header does not resolve that compatibility gap. curl documentation

The added tests miss these client-parsing cases; MinIO signing is mocked. Read-only reproductions confirmed ordinary HEAD/released names, dotted versions, expansions, spaces, and valid key timestamps. Storage keys remain unchanged, and I found no signature-call or existing url_for caller breakage.

No files changed. Container tests were unavailable because Docker access was denied.

Overall verdict: request changes — three P2 findings; no P0/P1 findings.

[paynejd]

Replies to Codex pass 1. All three are fixed in b7a4ac7:

1. Timestamp fallback (fixed): lastUpdated now comes only from the storage key, in the current YYYY-MM-DD_HHMMSS form or the legacy 14-digit form (20150516122820). A key with neither gets no name, so the download keeps its storage name instead of a timestamp taken from the live repo. Tests: test_legacy_key_timestamp, test_key_without_timestamp_gets_no_name.
2. Quotes, ; and % in versions (fixed): characters other than letters, digits, ., _, - and @ now become -, so the name needs no quoting. No prod version has ", ; or %, but 56 of about 29k have spaces, :, &, ,, [] or a non-ASCII character. Mnemonics and expansion mnemonics already fit the safe set. Test: test_unsafe_version_characters_become_hyphens.
3. Non-ASCII and curl (fixed by 2): names are now always ASCII, so Django sends a plain filename="…", which curl -OJ reads.

[paynejd]

Codex adversarial review, pass 2 (codex-cli 0.160.1, commit b7a4ac7)

• [P2] Timestamp-less dotted versions can still invent lastUpdated — core/common/models.py:1413. For version 1.20260930123456, the timestamp-less key orgs/PIH/PIH_PIH_v1.20260930123456.zip produces a filename ending _2026-09-30_123456.zip. That date belongs to the version identifier, not the export. I reproduced this directly. Validate the timestamp suffix separately from the complete repository/version/expansion prefix. The existing timestamp-less test misses date-like version components.

• [P2] Sanitization violates the exact stored-version requirement — core/common/models.py:1433. The compatibility fix changes 1.0 beta to 1.0-beta and révision to r-vision. These become indistinguishable from different, valid versions with those literal names when timestamps match. I reproduced both collisions. This fixes client parsing, but silently changes version identity; it needs reconciliation with the agreed naming contract.

The earlier quote, semicolon, percent-sign and Unicode parsing failures are resolved by normalization. Ten in-memory cases passed through Django 5.2.17 and the TBv2 header/redirect parsers. Ordinary HEAD, released, expansion and legacy-timestamp cases behaved correctly. Storage keys remain unchanged, and existing single-argument url_for calls remain compatible. The presigning arguments match botocore 1.36.10 and MinIO 7.2.9.

Reviewed git diff origin/master...HEAD; git diff --check passed. No files modified. No API container was running, so the Django suite and live storage downloads were not tested.

Overall verdict: request changes — two P2 findings; no P0/P1 findings.

[paynejd]

Replies to Codex pass 2:

1. Date-like versions read as the timestamp (fixed in a99559b): the timestamp is now whatever follows the key's known prefix (get_version_export_path(suffix=None)), not the text after the last dot. So 1.20260930123456 in a key without a timestamp gets no name. This also stops a key for version 1.8.24.1 being read when the version is 1.8.24. Test: test_date_like_version_is_not_read_as_the_timestamp.
2. Replacing characters changes the version as written (kept as is): this is deliberate. It gives every client one name that is plain ASCII and needs no quoting, which pass 1 asked for. The name is for people; the JSON inside the zip carries the exact version and canonical URL. Two names collide only when two versions of the same repo differ just in unsafe characters and share a lastUpdated to the second. The ocl-docs page documents the rule. I've asked the ticket owner to confirm it, since the ticket said "exactly as stored".

[paynejd]

Codex adversarial review, pass 3 (codex-cli 0.160.1, commit a99559b)

No remaining or new [P0]–[P3] findings in git diff origin/master...HEAD. Prior fixes verified; deliberate decisions excluded.

336 in-memory checks passed. Database-backed tests and live S3/MinIO downloads were not run. No files modified.

Overall verdict: approve.

[paynejd]

Quick note:

• Sunny already fixed the bug in TBv3 and TBv2 already reads the export filename straight from the API
• Related PRs already merged:
  • ocl-cli#13 now reads the export filename straight from the API
  • ocl-docs#68 updated to match the new behavior
• this PR to update oclapi2 to use the new file naming convention is the final step, and all client apps will read the filename straight from the api

[snyaggarwal]

Suggestions below simplify get_export_download_name. Behaviour is the same except for one edge case, noted at the end. These are non-functional changes that remove some redundancy:

1. Second prefix call. export_path was built from get_version_export_path(), and the method calls get_version_export_path(suffix=None) again just to slice the timestamp back out. The timestamp is the last .-separated part of the key, so one regex reads it in both the current (2026-09-30_123456) and legacy (20260930123456) forms. That replaces the strptime loop.
2. Name parts. The owner/repo/version/expansion parts can come straight from the URI: expansion_uri (minus expansions/) or self.uri. This also drops the self.expansion DB query that ran on every download. The version segment still comes from self.version, because version URIs are percent-encoded (1.0 beta: → 1.0%20beta%253A) and a HEAD source/collection URI has no version segment.

Two more suggestions are needed for this to apply cleanly. One removes the now-unused datetime import. The other updates test_date_like_version_is_not_read_as_the_timestamp. Without the prefix, a legacy key with no timestamp whose version ends in exactly 14 digits (PIH_PIH_v1.20260930123456.zip) would read the version as the timestamp. That case seems unrealistic enough to accept. The updated test covers a date-like version with a real key timestamp instead.

Something to consider: with this PR, the downloaded file name (orgs_PIH_sources_PIH_1.8.24_2026-09-30_123456.zip) no longer matches the S3 key (orgs/PIH/PIH_PIH_v1.8.24.2026-09-30_123456.zip). Before, the file someone had downloaded mapped directly to the stored object, and that helped a lot when debugging export issues. Now someone has to translate between the two formats to find the object. It isn't a blocker, but worth keeping in mind. Logging the key with the download name, or keeping the names closer to the key format, would close that gap.

[snyaggarwal]

Read the timestamp straight from the key and build the name from expansion_uri/uri. This drops the second get_version_export_path call and the expansion query.

Suggested change

	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:
	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)
	return '_'.join(re.sub(r'[^A-Za-z0-9._@-]+', '-', part) for part in parts) + '.zip'
	# current (2026-09-30_123456) or legacy (20260930123456) key timestamp
	match = re.fullmatch(
	r'(\d{4})-?(\d{2})-?(\d{2})_?(\d{6})', export_path.removesuffix('.zip').rsplit('.', 1)[-1])
	if not match:
	return None
	parts = (get(self, 'expansion_uri') or self.uri).replace('/expansions/', '/').strip('/').split('/')
	# uri percent-encodes the version and HEAD's uri has none, so take it from the model
	parts[4:5] = [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'

[snyaggarwal]

datetime is unused after the suggestion on get_export_download_name.

Suggested change

from datetime import datetime

[snyaggarwal]

Matches the simplified parsing. A key with no timestamp and a 14-digit version is accepted as an edge case. This covers a date-like version with a real key timestamp instead.

Suggested change

	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_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.1.zip'))