fix(service-storage): stop handing out _local/file/:key, a URL nothing mounts (#3641) - #3744
Merged
Merged
Conversation
…ing mounts (#3641) Three call sites built `${basePath}/_local/file/<key>`. No registrar has ever mounted it, so anyone who followed one got a 404. The tranche-3 storage ledger (#3636) recorded the URL as deliberately ABSENT and filed this; now nothing builds it either. Each site fixed according to what it could honestly do: - LocalStorageAdapter.getPresignedUpload() omits `downloadUrl` (optional on the descriptor). It CANNOT construct the real capability URL — that is keyed by sys_file.id and an adapter only ever sees the storage key. Nothing read the field, which is how it survived: the presigned-upload route builds its own downloadUrl and ignores this one, and all three real readers of `desc.downloadUrl` take it from getPresignedDownload, whose URL IS mounted. - GET /files/:fileId/url and GET /files/:fileId answer 501 NOT_IMPLEMENTED when the adapter has neither getPresignedDownload nor getSignedUrl, instead of returning/redirecting to the unmounted URL. The caller learns the adapter is the limitation rather than chasing a broken link. Behaviour change is confined to adapters implementing neither capability; LocalStorageAdapter and the S3 adapter both implement getPresignedDownload, so no shipped path changes. A 200/302 pointing at a 404 becomes a 501 that says why. Two conformance cases added, mutation-checked: restoring either dead URL fails them. service-storage: 198 passed (17 files), up from 196. Note on typecheck: this package has no `typecheck` script and CI's TypeScript gate runs `--filter @objectstack/spec` only. A bare `tsc --noEmit` here reports 39 pre-existing errors, 7 of them in test files this change never touches (the repo-wide `.js`-extension style under node16 resolution). Not touched, and not claimed as clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 01:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3641.
Three call sites built
${basePath}/_local/file/<key>. No registrar has ever mounted it, so anyone following one got a 404. The tranche-3 storage ledger (#3636) recorded the URL as deliberately absent and filed this; now nothing builds it either.Each site, fixed according to what it could honestly do
LocalStorageAdapter.getPresignedUpload()— omitsdownloadUrl(optional on the descriptor) rather than emitting a fake one. It cannot construct the real capability URL: that is keyed bysys_file.id, and an adapter only ever sees the storage key.Nothing read the field, which is exactly how it survived. The presigned-upload route builds its own
downloadUrl(${basePath}/files/:fileId/url) and ignores this one, and all three real readers ofdesc.downloadUrltake it fromgetPresignedDownload— whose URL is mounted (_local/raw/<token>). A write-only field pointing at nothing.GET /files/:fileId/urlandGET /files/:fileId— answer 501NOT_IMPLEMENTEDwhen the adapter has neithergetPresignedDownloadnorgetSignedUrl, instead of returning (or 302-ing to) the unmounted URL. The caller learns the adapter is the limitation rather than chasing a broken link.Blast radius
Confined to adapters implementing neither capability.
LocalStorageAdapterand the S3 adapter both implementgetPresignedDownload, so no shipped path changes. For the remaining case, a 200/302 pointing at a 404 becomes a 501 that says why — strictly more informative, and the sameNOT_IMPLEMENTEDcode the raw routes already use for unsupported adapters.Tests
@objectstack/service-storage— 198 passed (17 files), up from 196: two new conformance cases for the 501 branches. Mutation-checked — restoring either dead URL fails them.On typecheck, precisely
This package has no
typecheckscript, and CI's TypeScript gate runspnpm --filter @objectstack/spec exec tsc --noEmit— spec only. A baretsc --noEmithere reports 39 pre-existing errors, 7 of them in test files this change never touches (the repo-wide.js-extension style undernode16resolution).I am neither fixing those (unrelated, 39 files of churn) nor claiming this package typechecks clean. Flagging it because I had been reporting "typecheck clean" on packages that do have the script, and it would be easy to read that as a repo-wide property. It isn't.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K35y3ovfWtCkBHYFCqUfAt
Generated by Claude Code