Repository navigation
OpenConceptLab/ocl_issues#2864 | Export API: new download names, and GET redirects rather than streams - #68
Conversation
…GET redirects rather than streams Documents the export filename the API now sets, [:ownerType]_[:owner]_[:repoType]_[:repo]_[:repoVersion]_[:expansion]_[:lastUpdated].zip, and describes GET/HEAD as a 302 to a signed download URL. Also corrects the HEAD-version notes: repo admins can export HEAD. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 1 (codex-cli 0.160.1, commit c353d3d)
Reviewed against oclapi2#929 (core/... paths below are in oclapi2).
Ran git diff origin/main...HEAD. No files modified. Reviewed documentation commit c353d3d against oclapi2 commit 7347e6c, including the requested retained sections.
-
[P2] Processing and missing exports are conflated — exportapi.md:4. GET/HEAD returns
208whenis_exportingis true;204means no export exists. The “three possible results” list at line 49 also omits208and the explicit500when signing fails. Seecore/common/mixins.py:1056–1075. -
[P2]
is_processing=Falsedoes not establish export availability — exportapi.md:4. Calling this method “still correct” overstates its guarantee: export tasks remove their processing marker infinally, including after failure (core/common/tasks.py:126–137,154–170). Availability requires checking the export endpoint. -
[P2] Not every collection export includes an expansion — exportapi.md:23. Collections can have no default expansion, and
write_export_file()still produces an export with empty concept/mapping arrays. The filename helper then omits the expansion segment. Seecore/collections/models.py:359–364,core/common/utils.py:217–228, andcore/common/models.py:1416–1418. -
[P2] Timestamp provenance is incompletely described — exportapi.md:25. The timestamp normally comes from the selected cached storage key, not the repository’s current last change.
get_export_path()can select an older cached export. Also, empty repositories useupdated_atthroughlast_child_update, despite having no concept/mapping change. Seecore/common/models.py:841–846,1398–1422. -
[P2] The filename timestamp cannot be passed directly to
updatedSince— exportapi.md:35. The newly specified2026-09-30_123456format is rejected byfrom_string_to_date()and becomesNone, removing the date filter. Document conversion to2026-09-30T12:34:56Z, consistent with subscriptions.md. Confirmed with an isolated parser probe; seecore/common/utils.py:157–169andcore/common/mixins.py:713–720. -
[P2] HEAD-version permissions and rejection codes are oversimplified — exportapi.md:58, repeated at line 107. The gate permits staff, superusers, and
is_admin_for(version); that helper includes the owning user or any member of the owning organization. Authentication and object permissions run first, so “anyone else” can receive401/403, rather than universally405. Seecore/common/mixins.py:1030–1054, the export views’ permission classes, andcore/users/models.py:175–180. -
[P2] HEAD polling needs an explicit instruction to disable redirects — exportapi.md:59. Clients must inspect the API’s initial
302. Automatically following it checks the storage response instead, and both storage adapters generate a URL signed for GET, including when Django dispatches HEAD throughget(). Seeaws.py:68–72andminio.py:89–102. -
[P2] POST’s response contract remains incorrect — exportapi.md:121. Active exporting returns
208;409is the separateAlreadyQueuedcase. An existing export returns303with aURLheader, or204withnoRedirect=true, never the documented200. Also qualify line 102’s “no action” promise:force=truerequests regeneration. Seecore/common/mixins.py:1085–1100andcore/sources/views.py:618–627. -
[P2] DELETE incorrectly prohibits HEAD exports — exportapi.md:139. Authorized users can delete HEAD exports under the same HEAD permission gate; the handler returns
204when present or404when absent. Seecore/common/mixins.py:1102–1118. The response examples should also say204 No Contentand404 Not Found. -
[P2] Subscriptions still specifies the wrong download protocol and archive format — subscriptions.md:22, repeated at lines 26, 49–51 and 258. GET supplies a
Locationredirect, not anexportURL/exportUrlheader, and the download is ZIP, not compressed tar. Following these instructions would prevent clients from locating or unpacking the export. Seecore/common/mixins.py:1064andcore/common/utils.py:359–360. -
[P3] Signed URLs are described as single-use without implementation support — subscriptions.md:26. The adapters implement expiring URLs, with no consumption or invalidation after a download. Recommend obtaining a fresh URL without implying single-use semantics.
The reference checkout acquired external, uncommitted changes during review. These add two further mismatches if those changes are included in oclapi2#929:
-
[P2] Version IDs are no longer reproduced exactly — exportapi.md:24. The updated helper replaces characters outside
[A-Za-z0-9._@-]with-; an isolated probe converts1.0 betato1.0-beta. -
[P2] The filename header is no longer unconditional — exportapi.md:60. The updated helper returns
Nonefor keys without a recognized timestamp. Both adapters then omit the disposition override, so neither the advertised filename nor its query parameter is guaranteed.
Overall verdict: Changes requested. The 302 redirect correction and standard filename examples are sound, but the remaining contract errors would mislead API and subscription clients.
…source, HEAD permissions From Codex pass 1 on ocl-docs#68: 208 vs 204 and 500 on GET; don't follow the redirect when polling with HEAD; expansion only when a collection has a default one; lastUpdated comes from the cached export and needs converting for updatedSince; unsafe version characters become '-'; HEAD exports are for staff and the repository's owner on GET, POST and DELETE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replies to Codex pass 1. Fixed in b1391aa, except where noted:
Not fixed here (these were already wrong before this PR and are outside the naming change in OpenConceptLab/ocl_issues#2864; proposed as a follow-up ticket):
|
…superusers, empty-version timestamp Addresses Codex pass 2 on ocl-docs#68. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 2 (codex-cli 0.160.1, commit b1391aa)
Reviewed against oclapi2#929 (models.py, mixins.py, aws.py, minio.py are in oclapi2).
Reviewed origin/main...HEAD at b1391aa against API commit b7a4ac7a, excluding the later uncommitted API edits. No files modified.
-
[P2] Filename headers are not guaranteed for legacy caches — exportapi.md:63
When the key has no parseable timestamp,get_export_download_name()returnsNone; both storage backends then omit the disposition override entirely (models.py:1421,aws.py:71,minio.py:96–102). Consequently, neither the header nor the signed URL parameter promised here is guaranteed. Line 27 should describe “no filename override” rather than guarantee a saved storage filename; the client determines the fallback. -
[P2] “Other users get 405” overlooks earlier permission checks — exportapi.md:61, also lines 110 and 142
get_object()checks object permissions before reaching the HEAD gate (mixins.py:1044). An authenticated outsider requesting a private repository receives403, not405; unauthenticated requests fail authentication first. Also, the gate explicitly permitsis_superuser, independently of staff status or ownership (mixins.py:1032). Qualify405as applying to authenticated users with view access who fail the HEAD-specific gate, and include superusers. -
[P3] Timestamp description omits empty-repository fallback — exportapi.md:25
last_child_updatefalls back to the repository version’supdated_at, thentimezone.now(), when both child timestamps are absent (models.py:842–847). This occurs for empty repositories and collections without an expansion. The timestamp therefore does not always represent a concept or mapping change; document that fallback.
The redirect, GET availability statuses, expansion condition, sanitization, and ISO timestamp conversion fixes otherwise match the reference code.
Overall verdict: Needs changes — two P2 findings and one P3 clarification remain.
|
Replies to Codex pass 2. All three are fixed in d96fa68:
|
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 3 (codex-cli 0.160.1, commit d96fa68)
No remaining or new [P0]–[P3] findings.
Verified the pass 1/2 fixes against the specified oclapi2 implementation and checked subscriptions consistency, respecting your exclusions. No files modified.
Overall verdict: Approve.
Part of OpenConceptLab/ocl_issues#2864 (ocl-docs piece). Merge once OpenConceptLab/oclapi2#929 is deployed, so the page matches what the API returns.
Changes to
oclapi/apireference/exportapi.md[:repositoryId]_[:repoVersion].[:lastUpdated].zipwith the name the API now sets,[:ownerType]_[:owner]_[:repoType]_[:repo]_[:repoVersion]_[:expansion]_[:lastUpdated].zip, with rules and examples.302 Foundto a signed download URL. It never streamed the zip. The filename is in the download'sContent-Dispositionheader and in the signed URL'sresponse-content-disposition.other/subscriptions.mdneeds no change. It tells clients to readlastUpdatedfrom the filename, and every name still ends with it.🤖 Generated with Claude Code