Repository navigation
fix(presigned-url): no downloadUrl for a file that is not uploaded - #1971
theothersideofgod wants to merge 2 commits into
Conversation
A files row is written before its presigned PUT runs, so a `requested` row may name an object that never arrives, and a `rejected` row names bytes the confirm job refused. Neither should hand a reader a GET URL. Today both get a signed URL. The storage fixture gains the generator's `app_buckets_physical_name` reader, which downloadUrl calls; no existing server test selected downloadUrl, so the fixture never needed it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
downloadUrl signed a GET for every files row with a key and a provisioned bucket, whatever its status. A rejected file's URL served the rejected bytes, and a requested row's URL pointed at an object that may never exist. The plan now reads `status` and returns null unless the row is live per `isLiveFileRow` (uploaded or processed), the same rule upload dedup already uses. The check runs before any database or S3 work, and null matches the existing answer for a bucket with no physical name. The field description says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
The presigned-URL resolver now combines the file's
Reviewed commit: 221bc04 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR gates presigned download-URL issuance on the file row's status column in the PostGraphile storage plugin, and updates the server-test seed and integration tests accordingly.
Key findings
- 🟡
statuscolumn assumed on every@storageFilestable — download-url-field.ts:98
| @@ -102,12 +105,14 @@ export function createDownloadUrlPlugin( | |||
| isPublic: $isPublic, | |||
| filename: $filename, | |||
| bucketId: $bucketId, | |||
| status: $status, | |||
| withPgClient: $withPgClient, | |||
| pgSettings: $pgSettings, | |||
| }); | |||
|
|
|||
| return lambda($combined, async ({ key, isPublic, filename, bucketId, withPgClient, pgSettings }: any) => { | |||
| return lambda($combined, async ({ key, isPublic, filename, bucketId, status, withPgClient, pgSettings }: any) => { | |||
| if (!key) return null; | |||
| if (!isLiveFileRow({ status })) return null; | |||
There was a problem hiding this comment.
🟡 bug · medium
status column assumed on every @storageFiles table
The downloadUrl plan adds $parent.get('status') and gates on isLiveFileRow({ status }), but the field is attached to every codec carrying the @storageFiles smart tag, not only tables with a lifecycle column (download-url-field.ts:98). A tagged table whose schema lacks status fails at plan time (grafast cannot get a nonexistent attribute), turning the graceful null promised in the field description into a broken query. Additionally, isLiveFileRow matches only the exact strings uploaded/processed, so a tagged table with a differently-valued lifecycle column would silently return null for every row.
📋 Prompt for AI Agents
In graphile/graphile-presigned-url-plugin/src/download-url-field.ts around lines 68-98, before adding the downloadUrl field, verify the codec actually has a status attribute (e.g. if (!pgCodec.attributes?.status) return fields; alongside the existing tags?.storageFiles check, or log a warning naming the codec when the tag is present but status is missing). If you keep the field for tables without status, treat an absent status as live so isLiveFileRow gating applies only where the schema declares the column. This prevents a plan-time error or a permanently-null downloadUrl on @storageFiles-tagged tables that do not carry the lifecycle status column.
There was a problem hiding this comment.
Checked both parts.
Missing status column: not reachable. @storageFiles is set in one place, constructive-db storage_module.sql:1038-1041, and the same generator always creates status (:768-777, is_required := true). The one commit that replaced status with a processed boolean (4b9078bf847) landed on main together with the commit that restored it as the file_status enum (e8d4e51d885), both in constructive-db #996, so no main version tagged a table without status. Every @storageFiles table on a local cluster (11) has status of type file_status.
Different lifecycle values: real for pre-#996 tenants. Files tables provisioned before 2026-05-04 have status text with pending/ready/processing/failed; their ready files would get null. Dedup already treats them the same way since #1952. Added an "Existing tenants (R7)" section to the PR description with a question for the owner rather than adding a compatibility mapping here.
There was a problem hiding this comment.
Thanks for the detailed analysis. The two-part response is helpful, but neither part addresses the original finding in the diff itself.
Missing status column: Your argument is that the tag is always co-generated with the column in constructive-db, so the case cannot arise in practice. That is a valid operational argument, but the plugin code still has no guard — if the tag is ever applied manually or by a different generator, the plan-time error will occur with no warning. The finding asked for a codec-level check (e.g. if (!pgCodec.attributes?.status) return fields;) or at minimum a warning, and the diff does not show that added.
Different lifecycle values (pre-#996 tenants): You confirm this is a real issue — ready files on older tenants will return null from downloadUrl. Deferring to a PR description question does not fix the silent data-correctness bug that the finding flagged.
The diff adds $parent.get('status') and isLiveFileRow gating unconditionally with no codec attribute guard and no compatibility mapping for legacy status values. The finding is not resolved.
|
Nitpick Pass nullable |
Why
File lifecycle invariant S2 (constructive-db
docs/architecture/file-upload-and-processing.md§6,38ef654d5e0): "Onlyuploaded/processedrows are live: dedup, lists,collect_storage_usage,downloadUrl, and the file-ref projection guard." #1952 made dedup read liveness fromstatus(LIVE_FILE_STATUSES/isLiveFileRowinfile-lifecycle.ts), butdownloadUrlstill hands out a URL whatever the status. Arejectedfile (its bytes disagreed with the declared type) still downloads with GET 200, and arequestedfile whose PUT never ran gets a URL that answers 404NoSuchKey.Change
The plan reads
statusand returnsnullunlessisLiveFileRow({ status }), before any database read or signing.nullis what the field already returns when the bucket was never provisioned.Behavior changes
downloadUrlforrequestedandrejectedrows. Was: a URL (404 for requested, the rejected bytes for rejected). Now:null. Why: onlyuploaded/processedrows stand for bytes a reader can GET.requesteduntil the confirm job runs (confirm_upload_delay, 30 seconds by default). Was: a URL that usually worked once the PUT finished. Now:nulluntil confirm moves it touploaded.fail_jobs_for_worker.sql:76-77), so itsdownloadUrlcan staynullfor minutes.sdk/constructive-sdk/schemas/storage.graphql,sdk/constructive-{sdk,react,cli}/src/storage/orm/input-types.ts) change on the next regen; the type is unchanged.Existing tenants (R7)
Files tables provisioned before constructive-db #996 (merged 2026-05-04) have
status textwith the valuespending/ready/processing/failed(generator atd202622175d,storage_module.sql:387-393), not thefile_statusenum. After this PR theirreadyfiles getdownloadUrl: null, because onlyuploaded/processedcount as live. Dedup already treats those rows the same way since #1952 (sameisLiveFileRow). No backfill or compatibility mapping here.Question for the owner: are there tenants in production with the pre-#996 files table? If so, should
ready/processingcount as live, or will those tenants be rebuilt?Needs a follow-up in constructive-db
When constructive-db picks up this plugin, these integration tests upload and PUT a file, then assert a non-null
downloadUrlwithout confirming the upload first, so they will fail (read from the code, not run):application/app/__tests__/storage/app-storage.integration.test.ts:865,:916, andentity-storage-security.integration.test.ts:566,:579,:640,:739,:1361. They need to confirm the row first, asconfirmUploadedAsLanealready does inapp-storage.integration.test.ts:400. The e2estorage-pipeline.spec.ts:661-665already expects nodownloadUrlon a rejected row.Before / after
Base: origin/main
f1f197863.cd graphql/server-test && STORAGE_ACCESS_KEY_ID=… STORAGE_SECRET_ACCESS_KEY=… pnpm exec jest __tests__/download-url-status.integration.test.tsTests: 2 failed, 2 passed, 4 total.returns no download URL for a requested fileand... for a rejected filefail: both receive signed URLs.Tests: 4 passed, 4 total. Plugin suite83 passed; the four server-test files using the changed fixture,63 passed.tsc --noEmitandeslint srcexit 0.Cluster end to end (local kind cluster, GraphQL image = an older deployed pin plus this fix, swapped in and back): before, the rejected file's
downloadUrlanswered GET 200 with the rejected bytes and the requested file's answered 404; after, bothnull; processed and uploaded files kept their URLs (GET 200, same content).The
simple-seed-storagefixture gainsapp_buckets_physical_name(bucket_id), copying the generator's reader (constructive-dbstorage_module.sql:1721-1750); no test querieddownloadUrlbefore, so the fixture never needed it.Rules touched: S2, R7.
🤖 Generated with Claude Code