diff --git a/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts index 4bf4195e46..b2055717b5 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts @@ -30,7 +30,6 @@ function storageConfig( hasPathShares: false, hasVersioning: false, hasContentHash: false, - hasConfirmUpload: false, maxBulkFiles: 100, maxBulkTotalSize: 1000, ...overrides, diff --git a/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts index d4399151e1..66be6a0e6c 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts @@ -35,7 +35,6 @@ function storageConfig(overrides: Partial = {}): StorageMod hasPathShares: false, hasVersioning: false, hasContentHash: false, - hasConfirmUpload: false, ...overrides, } as unknown as StorageModuleConfig; } @@ -60,7 +59,7 @@ function fakeTx(existingHash: string, deletable = true) { async query(opts: { text: string; values: unknown[] }) { queries.push(opts); if (/SELECT id, content_hash/.test(opts.text)) { - return { rows: [{ id: OLD_FILE_ID, content_hash: existingHash }] }; + return { rows: [{ id: OLD_FILE_ID, content_hash: existingHash, status: 'uploaded' }] }; } if (/^DELETE FROM storage_public\.app_files/.test(opts.text)) { return { rows: deletable ? [{ id: OLD_FILE_ID }] : [] }; diff --git a/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts index d027513109..63f1d2fafc 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts @@ -1,12 +1,11 @@ /** * A dedup hit must name bytes that exist. * - * The files row is written before the presigned PUT, so a module without the - * confirm-upload lifecycle holds `requested` rows whose PUT may never have run. - * Reporting such a row as `deduplicated: true` hands the caller a reference to an - * object that is not in S3, and leaves no way to ever upload it. Without a - * confirm job to settle `status`, the object itself is the evidence: the row is - * live only if its key resolves in S3. + * The files row is written before the presigned PUT, so a `requested` row may + * have no object behind it. Reporting such a row as `deduplicated: true` hands + * the caller a reference to an object that is not in S3, and leaves no way to + * ever upload it. The confirm-upload job settles `status` once it has inspected + * the object, so the row's status — never an S3 HEAD — decides whether it is live. * * The database is faked at the query boundary and S3 at the client's `send`. */ @@ -48,7 +47,6 @@ function storageConfig(overrides: Partial = {}): StorageMod hasPathShares: false, hasVersioning: false, hasContentHash: false, - hasConfirmUpload: false, ...overrides, } as unknown as StorageModuleConfig; } @@ -64,30 +62,9 @@ const bucket = { allow_custom_keys: true, } as unknown as BucketConfig; -const notFound = Object.assign(new Error('NotFound'), { name: 'NotFound', $metadata: { httpStatusCode: 404 } }); - -function s3Answering(objectExists: boolean): S3Config { - return { - client: { - send: jest.fn(async (command: unknown) => { - if (command instanceof HeadObjectCommand && !objectExists) throw notFound; - return {}; - }), - }, - bucket: 'site-bucket', - region: 'us-east-1', - } as unknown as S3Config; -} - -/** An S3 whose HEAD resolves only the keys given. */ -function s3Holding(keys: string[]): S3Config { +function fakeS3(): S3Config { return { - client: { - send: jest.fn(async (command: unknown) => { - if (command instanceof HeadObjectCommand && !keys.includes(command.input.Key as string)) throw notFound; - return {}; - }), - }, + client: { send: jest.fn(async () => ({})) }, bucket: 'site-bucket', region: 'us-east-1', } as unknown as S3Config; @@ -96,20 +73,19 @@ function s3Holding(keys: string[]): S3Config { const heads = (s3: S3Config) => (s3.client.send as jest.Mock).mock.calls .map(([command]) => command) - .filter((command) => command instanceof HeadObjectCommand) - .map((command: HeadObjectCommand) => command.input.Key); + .filter((command) => command instanceof HeadObjectCommand); const options: PresignedUrlPluginOptions = { credentials: { accessKeyId: 'test', secretAccessKey: 'test' } }; -/** A tx whose one existing row is a `requested` row at `rowKey` carrying HASH. */ -function fakeTx(rowKey: string) { +/** A tx whose one existing row sits at `rowKey` carrying HASH in `status`. */ +function fakeTx(rowKey: string, status: string) { const queries: Array<{ text: string; values: unknown[] }> = []; const txClient = { async query(opts: { text: string; values: unknown[] }) { queries.push(opts); if (/record_file\(/.test(opts.text)) return { rows: [{ id: NEW_FILE_ID }] }; if (/^SELECT id/.test(opts.text)) { - return { rows: [{ id: OLD_FILE_ID, key: rowKey, content_hash: HASH, status: 'requested' }] }; + return { rows: [{ id: OLD_FILE_ID, key: rowKey, content_hash: HASH, status }] }; } if (/^DELETE FROM storage_public\.app_files/.test(opts.text)) return { rows: [{ id: OLD_FILE_ID }] }; throw new Error(`unexpected query: ${opts.text}`); @@ -130,62 +106,64 @@ const upload = (txClient: unknown, s3: S3Config, key?: string) => describe.each([ ['content-addressed key', undefined, HASH], ['custom key', SITE_KEY, SITE_KEY], -])('presigned retry of a never-completed upload (%s, no confirm-upload)', (_label, customKey, rowKey) => { - it('issues a fresh upload URL when the row has no object behind it', async () => { - const s3 = s3Answering(false); - const { txClient, queries } = fakeTx(rowKey); - - const result = await upload(txClient, s3, customKey); - - expect(result).toMatchObject({ - fileId: NEW_FILE_ID, - deduplicated: false, - uploadUrl: 'https://s3.example.com/put', - }); - expect(heads(s3)).toEqual([rowKey]); - const del = queries.find((q) => /^DELETE/.test(q.text)); - expect(del?.values).toEqual([OLD_FILE_ID]); - }); +])('presigned retry of a never-completed upload (%s)', (_label, customKey, rowKey) => { + it.each(['requested', 'rejected', 'expired'])( + 'issues a fresh upload URL when the row is %s, without probing S3', + async (status) => { + const s3 = fakeS3(); + const { txClient, queries } = fakeTx(rowKey, status); + + const result = await upload(txClient, s3, customKey); + + expect(result).toMatchObject({ + fileId: NEW_FILE_ID, + deduplicated: false, + uploadUrl: 'https://s3.example.com/put', + }); + expect(heads(s3)).toEqual([]); + const del = queries.find((q) => /^DELETE/.test(q.text)); + expect(del?.values).toEqual([OLD_FILE_ID]); + }, + ); - it('deduplicates when the row\'s object is in S3', async () => { - const s3 = s3Answering(true); - const { txClient, queries } = fakeTx(rowKey); + it.each(['uploaded', 'processed'])('deduplicates when the row is %s', async (status) => { + const s3 = fakeS3(); + const { txClient, queries } = fakeTx(rowKey, status); const result = await upload(txClient, s3, customKey); expect(result).toMatchObject({ fileId: OLD_FILE_ID, deduplicated: true, uploadUrl: null }); - expect(heads(s3)).toEqual([rowKey]); + expect(heads(s3)).toEqual([]); expect(queries).toHaveLength(1); + expect(queries[0].text).toContain('status'); }); }); -describe('content-addressed upload whose hash matches a custom-key row (no confirm-upload)', () => { +describe('content-addressed upload whose hash matches a custom-key row', () => { // Custom-key rows carry content_hash too, so the hash probe can match a row - // whose bytes live at its own key, not at the hash. That row is live. - it('deduplicates against the row and leaves it in place', async () => { - const s3 = s3Holding([SITE_KEY]); - const { txClient, queries } = fakeTx(SITE_KEY); + // whose bytes live at its own key, not at the hash. + it('deduplicates against a live row and leaves it in place', async () => { + const s3 = fakeS3(); + const { txClient, queries } = fakeTx(SITE_KEY, 'uploaded'); const result = await upload(txClient, s3); expect(result).toMatchObject({ fileId: OLD_FILE_ID, deduplicated: true, uploadUrl: null, key: SITE_KEY }); - expect(heads(s3)).toEqual([SITE_KEY]); expect(queries.some((q) => /^DELETE/.test(q.text))).toBe(false); }); - it('restarts the upload when that row has no object at its own key', async () => { - const s3 = s3Holding([]); - const { txClient, queries } = fakeTx(SITE_KEY); + it('restarts the upload when that row is still requested', async () => { + const s3 = fakeS3(); + const { txClient, queries } = fakeTx(SITE_KEY, 'requested'); const result = await upload(txClient, s3); expect(result).toMatchObject({ fileId: NEW_FILE_ID, deduplicated: false, key: HASH }); - expect(heads(s3)).toEqual([SITE_KEY]); expect(queries.find((q) => /^DELETE/.test(q.text))?.values).toEqual([OLD_FILE_ID]); }); }); -describe('streamed upload landing on a never-completed row (no confirm-upload)', () => { +describe('streamed upload landing on a never-completed row', () => { const staged = { stagingKey: '.staging/tmp-1', contentHash: HASH, @@ -194,14 +172,14 @@ describe('streamed upload landing on a never-completed row (no confirm-upload)', filename: SITE_KEY, }; - function db({ deleteAffects = true } = {}) { + function db({ status = 'requested', deleteAffects = true } = {}) { const queries: Array<{ text: string; values?: unknown[] }> = []; const client = { async query(opts: { text: string; values?: unknown[] }) { queries.push(opts); if (/set_config/.test(opts.text)) return { rows: [] }; if (/SELECT id, key, mime_type/.test(opts.text)) { - return { rows: [{ id: OLD_FILE_ID, key: HASH, mime_type: 'text/html', size: 16, filename: null as string | null, status: 'requested' }] }; + return { rows: [{ id: OLD_FILE_ID, key: HASH, mime_type: 'text/html', size: 16, filename: null as string | null, status }] }; } if (/^DELETE FROM storage_public\.app_files/.test(opts.text)) { return { rows: deleteAffects ? [{ id: OLD_FILE_ID }] : [] }; @@ -223,8 +201,8 @@ describe('streamed upload landing on a never-completed row (no confirm-upload)', binding: null, }); - it('promotes the staged bytes as a fresh file instead of absorbing them into the empty row', async () => { - const s3 = s3Answering(false); + it('promotes the staged bytes as a fresh file instead of absorbing them into the requested row', async () => { + const s3 = fakeS3(); const fake = db(); const { projection, deduplicated } = await finalizeStagedUpload({ @@ -233,12 +211,12 @@ describe('streamed upload landing on a never-completed row (no confirm-upload)', expect(deduplicated).toBe(false); expect(projection.id).toBe(NEW_FILE_ID); - expect(heads(s3)).toEqual([HASH]); + expect(heads(s3)).toEqual([]); expect(fake.queries.find((q) => /^DELETE/.test(q.text))?.values).toEqual([OLD_FILE_ID]); }); - it('fails instead of promoting when the caller cannot delete the empty row', async () => { - const s3 = s3Answering(false); + it('fails instead of promoting when the caller cannot delete the requested row', async () => { + const s3 = fakeS3(); const fake = db({ deleteAffects: false }); await expect( @@ -247,13 +225,13 @@ describe('streamed upload landing on a never-completed row (no confirm-upload)', expect(fake.queries.some((q) => /record_file/.test(q.text))).toBe(false); const sent = (s3.client.send as jest.Mock).mock.calls.map(([command]) => command); - expect(sent.map((command) => command.constructor.name)).toEqual(['HeadObjectCommand', 'DeleteObjectCommand']); - expect(sent[1].input.Key).toBe(staged.stagingKey); + expect(sent.map((command) => command.constructor.name)).toEqual(['DeleteObjectCommand']); + expect(sent[0].input.Key).toBe(staged.stagingKey); }); - it('deduplicates against a row whose object is in S3', async () => { - const s3 = s3Answering(true); - const fake = db(); + it('deduplicates against an uploaded row', async () => { + const s3 = fakeS3(); + const fake = db({ status: 'uploaded' }); const { projection, deduplicated } = await finalizeStagedUpload({ target: target(s3), withPgClient: fake.withPgClient, pgSettings: null, staged, diff --git a/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts index fd0d012c31..4ab9403265 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts @@ -524,7 +524,7 @@ describe('finalizeStagedUpload', () => { SET_CONFIG, { match: /SELECT id, key, mime_type/, - rows: () => [{ id: FILE_ID, key: staged.contentHash, mime_type: 'image/png', size: 16, filename: 'original.png' }], + rows: () => [{ id: FILE_ID, key: staged.contentHash, mime_type: 'image/png', size: 16, filename: 'original.png', status: 'uploaded' }], }, ]); @@ -536,20 +536,15 @@ describe('finalizeStagedUpload', () => { expect(projection.id).toBe(FILE_ID); expect(projection.filename).toBe('original.png'); expect(db.queries.some((q) => /INSERT/.test(q.text))).toBe(false); - // The existing row's object is confirmed, then only the staged object is - // deleted; nothing is copied. + // The row's status is the evidence the bytes exist, so only the staged + // object is deleted; nothing is copied or probed in S3. const sent = (s3.client.send as jest.Mock).mock.calls.map(([command]) => command.constructor.name); - expect(sent).toEqual(['HeadObjectCommand', 'DeleteObjectCommand']); + expect(sent).toEqual(['DeleteObjectCommand']); }); - // With the confirm-upload lifecycle, a row is a claim on bytes rather than - // proof of them, so only a confirmed row may absorb an upload. - describe('with the confirm-upload lifecycle', () => { - const lifecycleTarget = { - ...target, - storageConfig: { ...storageConfig(), hasConfirmUpload: true } as StorageModuleConfig, - }; - + // A row is a claim on bytes rather than proof of them, so only a confirmed + // row may absorb an upload. + describe('row liveness', () => { const existingRow = (status: string) => ({ id: FILE_ID, key: staged.contentHash, @@ -567,7 +562,7 @@ describe('finalizeStagedUpload', () => { ]); const { projection, deduplicated } = await finalizeStagedUpload({ - target: lifecycleTarget, withPgClient: db.withPgClient, pgSettings: null, staged, + target, withPgClient: db.withPgClient, pgSettings: null, staged, }); expect(deduplicated).toBe(true); @@ -588,7 +583,7 @@ describe('finalizeStagedUpload', () => { ]); const { projection, deduplicated } = await finalizeStagedUpload({ - target: lifecycleTarget, withPgClient: db.withPgClient, pgSettings: null, staged, + target, withPgClient: db.withPgClient, pgSettings: null, staged, }); expect(deduplicated).toBe(false); @@ -601,28 +596,16 @@ describe('finalizeStagedUpload', () => { }, ); - it('asks for the status column only when the module has one', async () => { + it('reads the status column alongside the row', async () => { const { finalizeStagedUpload } = await import('../src/managed-upload'); - const withLifecycle = fakeDb([ + const db = fakeDb([ SET_CONFIG, { match: /SELECT id, key, mime_type/, rows: () => [existingRow('uploaded')] }, ]); await finalizeStagedUpload({ - target: lifecycleTarget, withPgClient: withLifecycle.withPgClient, pgSettings: null, staged, - }); - expect(withLifecycle.queries.find((q) => /SELECT id, key/.test(q.text))!.text).toContain('status'); - - const without = fakeDb([ - SET_CONFIG, - { - match: /SELECT id, key, mime_type/, - rows: () => [{ id: FILE_ID, key: staged.contentHash, mime_type: 'image/png', size: 16, filename: null }], - }, - ]); - await finalizeStagedUpload({ - target, withPgClient: without.withPgClient, pgSettings: null, staged, + target, withPgClient: db.withPgClient, pgSettings: null, staged, }); - expect(without.queries.find((q) => /SELECT id, key/.test(q.text))!.text).not.toContain('status'); + expect(db.queries.find((q) => /SELECT id, key/.test(q.text))!.text).toContain('status'); }); }); diff --git a/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts index d15ab309c7..937cf66797 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts @@ -30,7 +30,6 @@ function storageConfig( hasPathShares: false, hasVersioning: false, hasContentHash: false, - hasConfirmUpload: false, maxBulkFiles: 100, maxBulkTotalSize: 1000, ...overrides, diff --git a/graphile/graphile-presigned-url-plugin/src/file-lifecycle.ts b/graphile/graphile-presigned-url-plugin/src/file-lifecycle.ts index d2e54fc403..33f11dd392 100644 --- a/graphile/graphile-presigned-url-plugin/src/file-lifecycle.ts +++ b/graphile/graphile-presigned-url-plugin/src/file-lifecycle.ts @@ -1,6 +1,3 @@ -import { headObject } from './s3-signer'; -import type { S3Config, StorageModuleConfig } from './types'; - /** * The statuses in which a files row stands for bytes a reader can actually GET. * A `requested` row is a claim, not an object — its presigned PUT may never have @@ -8,28 +5,13 @@ import type { S3Config, StorageModuleConfig } from './types'; */ export const LIVE_FILE_STATUSES = ['uploaded', 'processed']; -/** - * The `status` column for splicing into a select list, when the module runs the - * confirm-upload lifecycle that settles it. - */ -export function statusSelectFragment(storageConfig: StorageModuleConfig): string { - return storageConfig.hasConfirmUpload ? ', status' : ''; -} - /** * Whether an existing row may be handed back as a dedup hit: only once the bytes * it names are in S3. * - * The row is written before the PUT. With confirm-upload, the confirm job moves - * it out of `requested` only after inspecting the object, so its status is the - * answer. Without it nothing ever moves the row, so `status` stays `requested` - * whether or not the PUT ran, and the object at `key` is the only evidence. + * The row is written before the PUT; the confirm-upload job moves it out of + * `requested` only after inspecting the object, so its status is the answer. */ -export async function isLiveFileRow( - storageConfig: StorageModuleConfig, - s3: S3Config, - row: { key: string; status?: string } -): Promise { - if (storageConfig.hasConfirmUpload) return LIVE_FILE_STATUSES.includes(row.status as string); - return (await headObject(s3, row.key)) !== null; +export function isLiveFileRow(row: { status?: string | null }): boolean { + return LIVE_FILE_STATUSES.includes(row.status as string); } diff --git a/graphile/graphile-presigned-url-plugin/src/managed-upload.ts b/graphile/graphile-presigned-url-plugin/src/managed-upload.ts index 251545a9ef..424367d456 100644 --- a/graphile/graphile-presigned-url-plugin/src/managed-upload.ts +++ b/graphile/graphile-presigned-url-plugin/src/managed-upload.ts @@ -22,12 +22,12 @@ import { Logger } from '@pgpmjs/logger'; import { type WithPgClient, withRequestPgClient } from 'graphile-plugin-utils'; import { resolveDefaultBucket } from './default-bucket'; -import { isLiveFileRow, statusSelectFragment } from './file-lifecycle'; +import { isLiveFileRow } from './file-lifecycle'; +import { fileNotReplaceable } from './file-not-replaceable'; import { type FileRefFieldBinding, getFileRefFieldBinding } from './file-ref-registry'; import { assertBucketReconciled, resolveS3ForDatabase } from './physical-bucket'; import { copyS3Object, deleteS3Object } from './s3-signer'; import { recordManagedFile } from './storage-file-recorder'; -import { fileNotReplaceable } from './file-not-replaceable'; import { getBucketConfig, loadAllStorageModules } from './storage-module-cache'; import type { BucketConfig, FileProjection, PresignedUrlPluginOptions, S3Config, StorageModuleConfig } from './types'; @@ -308,7 +308,7 @@ export async function finalizeStagedUpload(args: { const existing = await withRequestPgClient(withPgClient, pgSettings, async (pgClient) => { const result = await pgClient.query({ - text: `SELECT id, key, mime_type, size, filename${statusSelectFragment(storageConfig)} + text: `SELECT id, key, mime_type, size, filename, status FROM ${storageConfig.filesQualifiedName} WHERE content_hash = $1 AND bucket_id = $2 LIMIT 1`, @@ -321,7 +321,7 @@ export async function finalizeStagedUpload(args: { mime_type: string; size: number; filename: string | null; - status?: string; + status: string; } | undefined; }); @@ -332,7 +332,7 @@ export async function finalizeStagedUpload(args: { // final key is the content hash and (bucket_id, key) is unique. The GC job the // delete enqueues re-takes the reference count when it runs, by which point // the replacement row exists, so it no-ops. - if (existing && !(await isLiveFileRow(storageConfig, s3, existing))) { + if (existing && !isLiveFileRow(existing)) { log.info( `Restarting upload of hash ${staged.contentHash}: file ${existing.id} has no bytes behind it` ); diff --git a/graphile/graphile-presigned-url-plugin/src/plugin.ts b/graphile/graphile-presigned-url-plugin/src/plugin.ts index 4109cd5916..fa1573ce69 100644 --- a/graphile/graphile-presigned-url-plugin/src/plugin.ts +++ b/graphile/graphile-presigned-url-plugin/src/plugin.ts @@ -30,7 +30,7 @@ import { checkTypeAgreement } from 'mime-bytes'; import { validateCustomKey } from './custom-key'; import { resolveDefaultBucket } from './default-bucket'; -import { isLiveFileRow, statusSelectFragment } from './file-lifecycle'; +import { isLiveFileRow } from './file-lifecycle'; import { fileNotReplaceable } from './file-not-replaceable'; import { buildFileProjection, type FileProjection } from './managed-upload'; import { assertBucketReconciled, resolveS3ForDatabase } from './physical-bucket'; @@ -685,12 +685,11 @@ export async function processSingleFile( // hash and a second row would collide on (bucket_id, key). The GC job the // delete enqueues re-takes the reference count when it runs (≥5s later), by // which point the replacement row exists, so it no-ops. - const statusColumn = statusSelectFragment(storageConfig); let staleFileId: string | null = null; if (isCustomKey) { const existingResult = await txClient.query({ - text: `SELECT id, content_hash${statusColumn} + text: `SELECT id, content_hash, status FROM ${storageConfig.filesQualifiedName} WHERE key = $1 AND bucket_id = $2 @@ -702,7 +701,7 @@ export async function processSingleFile( if (existingResult.rows.length > 0) { const existing = existingResult.rows[0]; if (existing.content_hash === contentHash) { - if (await isLiveFileRow(storageConfig, s3ForDb, { key: s3Key, status: existing.status })) { + if (isLiveFileRow(existing)) { log.info(`Dedup hit (custom key): file ${existing.id} for key ${s3Key}`); return { uploadUrl: null as string | null, @@ -737,7 +736,7 @@ export async function processSingleFile( } } else { const dedupResult = await txClient.query({ - text: `SELECT id, key${statusColumn} + text: `SELECT id, key, status FROM ${storageConfig.filesQualifiedName} WHERE content_hash = $1 AND bucket_id = $2 @@ -749,7 +748,7 @@ export async function processSingleFile( const existingFile = dedupResult.rows[0]; // A custom-key row can carry this hash, so the bytes are at the row's own key. const existingKey = existingFile.key as string; - if (await isLiveFileRow(storageConfig, s3ForDb, { key: existingKey, status: existingFile.status })) { + if (isLiveFileRow(existingFile)) { log.info(`Dedup hit: file ${existingFile.id} for hash ${contentHash}`); return { diff --git a/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts b/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts index e02356d19a..a18a2b8d20 100644 --- a/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts +++ b/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts @@ -102,7 +102,6 @@ interface StorageModuleRow { has_path_shares: boolean; has_versioning: boolean; has_content_hash: boolean; - has_confirm_upload: boolean; entity_schema: string | null; entity_table: string | null; } @@ -144,7 +143,6 @@ function buildConfig(row: StorageModuleRow): StorageModuleConfig { hasPathShares: row.has_path_shares ?? false, hasVersioning: row.has_versioning ?? false, hasContentHash: row.has_content_hash ?? false, - hasConfirmUpload: row.has_confirm_upload ?? false, maxBulkFiles: row.max_bulk_files ?? DEFAULT_MAX_BULK_FILES, maxBulkTotalSize: row.max_bulk_total_size ?? DEFAULT_MAX_BULK_TOTAL_SIZE, }; diff --git a/graphile/graphile-presigned-url-plugin/src/types.ts b/graphile/graphile-presigned-url-plugin/src/types.ts index 25a545210f..d6c3c11bb8 100644 --- a/graphile/graphile-presigned-url-plugin/src/types.ts +++ b/graphile/graphile-presigned-url-plugin/src/types.ts @@ -85,12 +85,6 @@ export interface StorageModuleConfig { cacheTtlSeconds: number; /** Whether this storage module uses ltree path + path shares (determines if path column exists on files) */ hasPathShares: boolean; - /** - * Whether the files table carries the confirm-upload lifecycle (`status`, - * `promoted_at`). Only then can a row be told apart from the bytes it claims: - * without it every row is treated as live, because there is nothing to read. - */ - hasConfirmUpload: boolean; /** Whether the files table carries the versioning chain. */ hasVersioning: boolean; /** diff --git a/graphile/graphile-settings/__tests__/upload-resolver.test.ts b/graphile/graphile-settings/__tests__/upload-resolver.test.ts index e0822084f9..240d52457f 100644 --- a/graphile/graphile-settings/__tests__/upload-resolver.test.ts +++ b/graphile/graphile-settings/__tests__/upload-resolver.test.ts @@ -46,7 +46,6 @@ function storageModuleRow(): Record { max_bulk_total_size: null, has_path_shares: false, has_versioning: false, - has_confirm_upload: false, entity_schema: null, entity_table: null, }; @@ -88,7 +87,7 @@ function fakeContext(opts: { isPublic?: boolean; existingFile?: boolean; failIns { match: /SELECT id, key, mime_type/, rows: () => opts.existingFile - ? [{ id: FILE_ID, key: 'existing-key', mime_type: 'image/png', size: 16, filename: 'first.png' }] + ? [{ id: FILE_ID, key: 'existing-key', mime_type: 'image/png', size: 16, filename: 'first.png', status: 'uploaded' }] : [], }, { diff --git a/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql b/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql index 19bfc9e5be..aca3d4b149 100644 --- a/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql +++ b/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql @@ -40,6 +40,7 @@ CREATE TABLE "tess-storage-public".files ( filename text, owner_id uuid, is_public boolean NOT NULL DEFAULT false, + status text NOT NULL DEFAULT 'requested', previous_version_id uuid REFERENCES "tess-storage-public".files(id), created_at timestamptz DEFAULT now(), updated_at timestamptz DEFAULT now(), diff --git a/graphql/server-test/__fixtures__/seed/simple-seed-storage/schema.sql b/graphql/server-test/__fixtures__/seed/simple-seed-storage/schema.sql index 8114eb3f33..7fd0c4b92e 100644 --- a/graphql/server-test/__fixtures__/seed/simple-seed-storage/schema.sql +++ b/graphql/server-test/__fixtures__/seed/simple-seed-storage/schema.sql @@ -54,6 +54,7 @@ BEGIN filename text, owner_id uuid, is_public boolean NOT NULL DEFAULT false, + status text NOT NULL DEFAULT ''requested'', previous_version_id uuid REFERENCES %I.app_files(id), created_at timestamptz DEFAULT now(), updated_at timestamptz DEFAULT now(), diff --git a/graphql/server-test/__tests__/upload.integration.test.ts b/graphql/server-test/__tests__/upload.integration.test.ts index b9d581572b..69ddd697da 100644 --- a/graphql/server-test/__tests__/upload.integration.test.ts +++ b/graphql/server-test/__tests__/upload.integration.test.ts @@ -280,6 +280,18 @@ function expectSuccess(res: supertest.Response): Record { describe('Integration tests (uploads, tenant isolation, RLS)', () => { let request: supertest.Agent; let pg: PgTestClient; + + /** + * Stand-in for the storage:confirm_upload worker, which this fixture does not + * run: a row is written as `requested` and only becomes a dedup hit once the + * confirm job has inspected the object and moved it to `uploaded`. + */ + const confirmUploaded = async (contentHash: string): Promise => { + await pg.query( + `UPDATE "${aliceSchemas[0]}".app_files SET status = 'uploaded' WHERE content_hash = $1`, + [contentHash] + ); + }; let teardown: () => Promise; const s3Client = createS3Client({ provider: 'minio', @@ -415,6 +427,7 @@ describe('Integration tests (uploads, tenant isolation, RLS)', () => { it('should accept a PUT to the presigned URL', async () => { const putRes = await putToPresignedUrl(uploadUrl, fileContent, contentType); expect(putRes.ok).toBe(true); + await confirmUploaded(contentHash); }); }); @@ -487,9 +500,10 @@ describe('Integration tests (uploads, tenant isolation, RLS)', () => { expect(payload.fileId).toBeTruthy(); }); - // The row is written before the PUT, and this module has no confirm-upload - // job to settle its status, so only the object itself proves the bytes - // arrived. A retry after a PUT that never ran must get a URL again. + // The row is written before the PUT as `requested`; only the confirm job + // (stood in for here by confirmUploaded) moves it to `uploaded`, and only + // an uploaded row is a dedup hit. A retry after a PUT that never ran must + // get a URL again. it('issues a fresh upload URL when the earlier PUT never ran', async () => { const fileContent = `Never PUT: the client walked away (${crypto.randomUUID()}).`; const contentHash = await hashContent(fileContent); @@ -512,6 +526,7 @@ describe('Integration tests (uploads, tenant isolation, RLS)', () => { const putRes = await putToPresignedUrl(retry.uploadUrl, fileContent, 'text/plain'); expect(putRes.ok).toBe(true); + await confirmUploaded(contentHash); const afterPut = expectSuccess(await postGraphQL({ query: UPLOAD_APP_FILE, variables: { input } })).uploadAppFile; expect(afterPut.deduplicated).toBe(true); diff --git a/packages/node-type-registry/src/blueprint-types.generated.ts b/packages/node-type-registry/src/blueprint-types.generated.ts index ff5a878666..0d98554f2c 100644 --- a/packages/node-type-registry/src/blueprint-types.generated.ts +++ b/packages/node-type-registry/src/blueprint-types.generated.ts @@ -1944,9 +1944,7 @@ export interface BlueprintStorageConfig { default_max_file_size?: number; /** CORS allowed origins for the storage module. */ allowed_origins?: string[]; - /** Enable deferred upload confirmation via HeadObject. When true, creates SECURITY DEFINER status transition functions (confirm_uploaded, mark_processed) and an AFTER INSERT trigger that enqueues a storage:confirm_upload job. The job verifies the file exists in S3 before transitioning status from requested to uploaded. Defaults to false. */ - has_confirm_upload?: boolean; - /** Delay before the first upload confirmation attempt (PostgreSQL interval string, e.g. "30 seconds"). Only used when has_confirm_upload is true. Defaults to "30 seconds". */ + /** Delay before the first upload confirmation attempt (PostgreSQL interval string, e.g. "30 seconds"). Defaults to "30 seconds". */ confirm_upload_delay?: string; /** Provision a temporary staging bucket (type temp, tagged default-temp) that stages for the private default bucket, so uploads land there and are promoted into their destination on confirmation. Setting staging_ttl implies this. Defaults to false. */ staging?: boolean; diff --git a/packages/node-type-registry/src/codegen/generate-types.ts b/packages/node-type-registry/src/codegen/generate-types.ts index 693a1d45e3..2e5f9f61d7 100644 --- a/packages/node-type-registry/src/codegen/generate-types.ts +++ b/packages/node-type-registry/src/codegen/generate-types.ts @@ -930,13 +930,9 @@ function buildBlueprintStorageConfig(): t.ExportNamedDeclaration { ), 'CORS allowed origins for the storage module.' ), - addJSDoc( - optionalProp('has_confirm_upload', t.tsBooleanKeyword()), - 'Enable deferred upload confirmation via HeadObject. When true, creates SECURITY DEFINER status transition functions (confirm_uploaded, mark_processed) and an AFTER INSERT trigger that enqueues a storage:confirm_upload job. The job verifies the file exists in S3 before transitioning status from requested to uploaded. Defaults to false.' - ), addJSDoc( optionalProp('confirm_upload_delay', t.tsStringKeyword()), - 'Delay before the first upload confirmation attempt (PostgreSQL interval string, e.g. "30 seconds"). Only used when has_confirm_upload is true. Defaults to "30 seconds".' + 'Delay before the first upload confirmation attempt (PostgreSQL interval string, e.g. "30 seconds"). Defaults to "30 seconds".' ), addJSDoc( optionalProp('staging', t.tsBooleanKeyword()), diff --git a/packages/node-type-registry/src/module-presets/types.ts b/packages/node-type-registry/src/module-presets/types.ts index f43e243b75..c3507153a6 100644 --- a/packages/node-type-registry/src/module-presets/types.ts +++ b/packages/node-type-registry/src/module-presets/types.ts @@ -87,7 +87,6 @@ export type StorageModuleOptions = BaseModuleOptions & { has_content_hash?: boolean; has_custom_keys?: boolean; has_audit_log?: boolean; - has_confirm_upload?: boolean; /** Postgres interval literal, e.g. '30 seconds'. */ confirm_upload_delay?: string; /** Provision a temp staging bucket that stages for the private default. */ diff --git a/packages/node-type-registry/src/process/extraction.ts b/packages/node-type-registry/src/process/extraction.ts index 1969aeb4de..a8b1da54ef 100644 --- a/packages/node-type-registry/src/process/extraction.ts +++ b/packages/node-type-registry/src/process/extraction.ts @@ -4,8 +4,8 @@ import type { NodeTypeDefinition } from '../types'; /** * File extraction processing node. * - * Composes a JobTrigger that fires when a file transitions to status = 'uploaded' - * (or on INSERT if confirm_upload is not enabled). The trigger enqueues a + * Composes a JobTrigger that fires when a file transitions to status = 'uploaded'. + * The trigger enqueues a * text-extraction job that converts the file contents (PDF, DOCX, HTML, etc.) * into plain text or markdown, storing the result in configurable output fields. * diff --git a/packages/node-type-registry/src/process/image-versions.ts b/packages/node-type-registry/src/process/image-versions.ts index c9e153f275..a8707b0635 100644 --- a/packages/node-type-registry/src/process/image-versions.ts +++ b/packages/node-type-registry/src/process/image-versions.ts @@ -5,7 +5,7 @@ import type { NodeTypeDefinition } from '../types'; * Image version processing node. * * Composes a JobTrigger that fires when an image file transitions to - * status = 'uploaded' (or on INSERT if confirm_upload is not enabled). + * status = 'uploaded'. * The trigger enqueues an image-processing job that generates resized, * cropped, or reformatted variants of the source image. *