diff --git a/.changeset/service-storage-success-envelope.md b/.changeset/service-storage-success-envelope.md new file mode 100644 index 0000000000..5535c4d7e6 --- /dev/null +++ b/.changeset/service-storage-success-envelope.md @@ -0,0 +1,65 @@ +--- +"@objectstack/spec": patch +"@objectstack/service-storage": patch +"@objectstack/client": patch +--- + +fix(service-storage): emit the declared success envelope on all eight routes (#3689) + +#3675 moved the **error** bodies of the autonomously-mounted `/api/v1/storage/*` +routes into the declared `{ success: false, error: { code, message } }` +envelope and deliberately stopped there: unlike the errors, the success bodies +were not an additive fix. They were three shapes, none of them carrying the +`success` flag `BaseResponseSchema` declares and +`ObjectStackClient.unwrapResponse` keys on — + +| Route(s) | Was | Now | +|---|---|---| +| the six upload routes (`/upload/presigned`, `/upload/complete`, `/upload/chunked`, `…/chunk/:i`, `…/complete`, `…/progress`) | `{ data: {…} }` | `{ success: true, data: {…} }` | +| `GET /files/:fileId/url` | `{ url }` | `{ success: true, data: { url } }` | +| `PUT /_local/raw/:token` | `{ ok: true, key }` | `{ success: true, data: { key } }` | + +— while `storage.zod.ts` declared every one of them as +`BaseResponseSchema.extend({ data })`, and `PresignedUrlResponse` and friends +are `z.infer`red from those schemas and published as the SDK's return types. +The declaration said `success: boolean`; the wire said nothing. It broke +nothing only because the storage SDK methods returned `res.json()` raw — +`any`, so TypeScript could not see the gap and nothing relied on the +declaration. That is the posture i18n was in before #3636, right up until +something did rely on it. + +**The payload moved on two routes, and that is the breaking part.** A direct +HTTP caller reading `body.url` from `GET /files/:fileId/url` must now read +`body.data.url`; one reading `body.ok`/`body.key` from the local adapter's +`PUT /_local/raw/:token` loopback must read `body.success`/`body.data.key`. +`ok` is dropped rather than kept beside `success` — it was a second, private +word for the same thing. The six upload routes are additive: callers already +destructure `.data`, and a new sibling key changes nothing. + +Every in-repo consumer was fixed first, so the two repos are not coupled by +merge order: + +- `client.storage.getDownloadUrl()` now reads through `unwrapResponse`, the + SDK's one standard envelope seam — which strips the envelope when present + and returns the body untouched when not, so a client either side of this + server change resolves the same URL. The other storage methods hand back the + whole envelope by design and were already correct. +- The console's two attachment openers (`RecordAttachmentsPanel`, + `ApprovalsInboxPage`) already read `body?.url ?? body?.data?.url`; objectui + gains tests pinning that tolerance as deliberate. + +Two schemas that were missing are now declared — `FileDownloadUrlResponse` and +`RawUploadResponse` — and `getDownloadUrl` joins `StorageApiContracts`, which +it had never been in. That absence is how its shape drifted outside the +envelope unnoticed. The two `_local/raw/:token` routes stay out of the +registry on purpose: they are the local adapter's own presign loopback, +ledgered `server-only` and addressed as an opaque signed URL rather than as an +API. + +`success-envelope.conformance.test.ts` holds the new shape in place the way +`error-envelope.conformance.test.ts` holds the error one: every route is +driven and its body parsed against the **declared schema** it answers to — not +a restatement — the retired shapes are asserted dead, and the module source is +scanned so a new route cannot bypass the `sendOk` helper. As with #3675, the +route ledgers cannot catch this class of drift: they audit which routes exist +and whether the SDK can address them, not what comes back. diff --git a/content/docs/references/api/storage.mdx b/content/docs/references/api/storage.mdx index 7a8dd6516e..d1616db0b1 100644 --- a/content/docs/references/api/storage.mdx +++ b/content/docs/references/api/storage.mdx @@ -20,8 +20,8 @@ rather than proxying bytes through the API server. ## TypeScript Usage ```typescript -import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api'; -import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api'; +import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api'; +import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api'; // Validate data const result = CompleteChunkedUploadRequest.parse(data); @@ -65,6 +65,20 @@ const result = CompleteChunkedUploadRequest.parse(data); | **eTag** | `string` | optional | S3 ETag verification | +--- + +## FileDownloadUrlResponse + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **success** | `boolean` | ✅ | Operation success status | +| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false | +| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata | +| **data** | `{ url: string }` | ✅ | | + + --- ## FileTypeValidation @@ -154,6 +168,20 @@ const result = CompleteChunkedUploadRequest.parse(data); | **data** | `{ uploadUrl: string; downloadUrl?: string; fileId: string; method: Enum<'PUT' \| 'POST'>; … }` | ✅ | | +--- + +## RawUploadResponse + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **success** | `boolean` | ✅ | Operation success status | +| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false | +| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata | +| **data** | `{ key: string }` | ✅ | | + + --- ## UploadChunkRequest diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 6d33585942..14eb60da48 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -2443,11 +2443,23 @@ export class ObjectStackClient { return completeJson; }, + /** + * Resolve a committed file to a short-lived signed download URL. + * + * Read through `unwrapResponse` rather than off the raw body: the route + * answers the declared `{ success: true, data: { url } }` envelope as of + * #3689, and this SDK ships as its own npm package against servers it was + * not built with. `unwrapResponse` strips the envelope when it is there + * and hands back the body untouched when it is not, so a client on either + * side of that server upgrade resolves the same URL. That is the SDK's one + * standard envelope seam — every other enveloped method already goes + * through it — not a fallback grown for this route. + */ getDownloadUrl: async (fileId: string): Promise => { const route = this.getRoute('storage'); const res = await this.fetch(`${this.baseUrl}${route}/files/${fileId}/url`); - const data = await res.json(); - return data.url; + const { url } = await this.unwrapResponse<{ url: string }>(res); + return url; }, /** diff --git a/packages/client/src/storage-wire-dialect.test.ts b/packages/client/src/storage-wire-dialect.test.ts new file mode 100644 index 0000000000..2745c8901d --- /dev/null +++ b/packages/client/src/storage-wire-dialect.test.ts @@ -0,0 +1,138 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Storage wire-dialect proof (#3689) — the SDK's storage methods against the + * envelope `service-storage` actually emits, and against the one it emitted + * before. + * + * The gap this pins: `storage.zod.ts` declared every storage response as + * `BaseResponseSchema.extend({ data })`, `PresignedUrlResponse` and friends are + * `z.infer`red from those schemas and published as these methods' return types + * — and the methods returned `res.json()` raw. `res.json()` is `any`, so the + * declaration could say `success: boolean` while the wire said nothing at all + * and TypeScript would never know. `getDownloadUrl` was the one method that + * read INTO a body, and it read `data.url` off a bare `{ url }`. + * + * #3689 moved the wire onto the declaration. This SDK ships as its own npm + * package, so it meets servers on both sides of that change: the enveloped and + * the bare body are both asserted here, and `unwrapResponse` — the client's one + * standard envelope seam, not a fallback grown for this route — is what spans + * them. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackClient } from './index'; + +function clientReturning(body: any) { + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + status: 200, + statusText: 'OK', + json: async () => body, + headers: new Headers(), + }); + const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock }); + return { client, fetchMock }; +} + +describe('storage.getDownloadUrl reads the signed URL off either dialect (#3689)', () => { + it('resolves from the declared { success: true, data: { url } } envelope', async () => { + const { client, fetchMock } = clientReturning({ + success: true, + data: { url: '/api/v1/storage/_local/raw/eyJrIjoi.c2ln' }, + }); + + await expect(client.storage.getDownloadUrl('f1')).resolves.toBe( + '/api/v1/storage/_local/raw/eyJrIjoi.c2ln', + ); + expect(fetchMock.mock.calls[0][0]).toContain('/api/v1/storage/files/f1/url'); + }); + + it('still resolves from the bare { url } an older server answers', async () => { + // Not a tolerated dialect going forward — a version-skew allowance. The + // client is published separately from the server it talks to, so a build + // predating the #3689 rollout must keep resolving downloads. + const { client } = clientReturning({ url: 'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc' }); + + await expect(client.storage.getDownloadUrl('f1')).resolves.toBe( + 'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc', + ); + }); + + it('resolves an absolute S3-style URL out of the envelope unchanged', async () => { + const { client } = clientReturning({ + success: true, + data: { url: 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc' }, + }); + + await expect(client.storage.getDownloadUrl('f1')).resolves.toBe( + 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc', + ); + }); +}); + +describe('the enveloped storage responses match their declared return types (#3689)', () => { + /** + * These methods hand back the whole envelope by design — their declared + * return types ARE `BaseResponseSchema.extend({ data })`. Before #3689 the + * `success` half of that declaration was fiction. Asserting it here is what + * makes the published type honest, since `res.json()` erases to `any` and + * the compiler cannot. + */ + it('getPresignedUrl carries success alongside data', async () => { + const { client } = clientReturning({ + success: true, + data: { + uploadUrl: '/api/v1/storage/_local/raw/tok', + method: 'PUT', + fileId: 'f1', + expiresIn: 3600, + downloadUrl: '/api/v1/storage/files/f1/url', + }, + }); + + const res = await client.storage.getPresignedUrl({ + filename: 'a.png', + mimeType: 'image/png', + size: 10, + scope: 'user', + }); + expect(res.success).toBe(true); + expect(res.data.fileId).toBe('f1'); + }); + + it('initChunkedUpload carries success alongside data', async () => { + const { client } = clientReturning({ + success: true, + data: { + uploadId: 'up1', + resumeToken: 'tok', + fileId: 'f1', + totalChunks: 1, + chunkSize: 5242880, + expiresAt: '2026-01-01T00:00:00.000Z', + }, + }); + + const res = await client.storage.initChunkedUpload({ + filename: 'big.bin', + mimeType: 'application/octet-stream', + totalSize: 100, + chunkSize: 5242880, + scope: 'user', + }); + expect(res.success).toBe(true); + expect(res.data.uploadId).toBe('up1'); + }); + + it('uploadPart carries success alongside data', async () => { + const { client } = clientReturning({ + success: true, + data: { chunkIndex: 0, eTag: '"abc"', bytesReceived: 100 }, + }); + + const res = await client.storage.uploadPart('up1', 0, 'tok', Buffer.from('x')); + expect(res.success).toBe(true); + expect(res.data.eTag).toBe('"abc"'); + }); +}); diff --git a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts index b3c8dc252b..e88b0a467f 100644 --- a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts @@ -327,10 +327,14 @@ describe('attachments permission matrix (#2755)', () => { expect(denied.status).toBe(403); expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); - // The owner (admin) → 200 with a signed URL. + // The owner (admin) → 200 with a signed URL, in the declared + // `{ success: true, data: { url } }` envelope (#3689 — this route answered + // a bare `{ url }` until then). const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`); expect(owner.status).toBe(200); - expect(((await owner.json()) as any).url).toBeTruthy(); + const ownerBody = (await owner.json()) as any; + expect(ownerBody.success).toBe(true); + expect(ownerBody.data?.url).toBeTruthy(); // Parent-inherited read: a file on the PUBLIC att_case record is // downloadable by any member who can read that record — even a diff --git a/packages/services/service-storage/src/storage-route-ledger.ts b/packages/services/service-storage/src/storage-route-ledger.ts index 61c81ed095..b67420655b 100644 --- a/packages/services/service-storage/src/storage-route-ledger.ts +++ b/packages/services/service-storage/src/storage-route-ledger.ts @@ -79,13 +79,13 @@ export const STORAGE_ROUTE_LEDGER: readonly StorageRouteLedgerEntry[] = [ // ── download ────────────────────────────────────────────────────────────── { route: 'GET /api/v1/storage/files/:fileId/url', family: 'download', disposition: 'sdk', client: 'storage.getDownloadUrl', - note: 'JSON { url } — the authorization-gated signed-URL resolve the dispatcher ledger points at (#3584)' }, + note: 'JSON { success: true, data: { url } } — the authorization-gated signed-URL resolve the dispatcher ledger points at (#3584); the bare { url } it used to answer moved into the declared envelope in #3689' }, { route: 'GET /api/v1/storage/files/:fileId', family: 'download', disposition: 'server-only', note: 'stable 302 to the same signed URL. This is the value objectql stamps into file/image field payloads (engine.ts buildFileValue), followed verbatim by / — a browser URL, not an SDK call. The SDK resolves via the /url sibling above.' }, // ── local-driver loopback (LocalStorageAdapter presign targets) ─────────── { route: 'PUT /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only', - note: 'the uploadUrl LocalStorageAdapter mints for its own presign tokens. storage.upload does PUT it, but opaquely — via fetchImpl on whatever uploadUrl came back, exactly as it would an S3 presigned URL. HMAC-token-authorized, adapter-internal, never a named SDK method.' }, + note: 'the uploadUrl LocalStorageAdapter mints for its own presign tokens. storage.upload does PUT it, but opaquely — via fetchImpl on whatever uploadUrl came back, exactly as it would an S3 presigned URL. HMAC-token-authorized, adapter-internal, never a named SDK method. Answers { success: true, data: { key } }; the { ok: true, key } it used to answer retired in #3689, `ok` being a private second word for `success`.' }, { route: 'GET /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only', note: 'ditto for download: the target of getPresignedDownload/getSignedUrl on the local adapter, handed to the browser as an opaque signed link.' }, ]; diff --git a/packages/services/service-storage/src/storage-routes.test.ts b/packages/services/service-storage/src/storage-routes.test.ts index 408ca9ce66..7c7344f540 100644 --- a/packages/services/service-storage/src/storage-routes.test.ts +++ b/packages/services/service-storage/src/storage-routes.test.ts @@ -241,7 +241,7 @@ describe('Storage REST Routes', () => { const urlRes = createMockRes(); await urlHandler(urlReq, urlRes); expect(urlRes._status).toBe(200); - expect(urlRes._json.url).toContain('/_local/raw/'); + expect(urlRes._json.data.url).toContain('/_local/raw/'); }); it('should 404 for non-committed file', async () => { @@ -308,7 +308,7 @@ describe('Storage REST Routes', () => { await commit(s, { id: 'a3' }); const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a3'); expect(res._status).toBe(200); - expect(res._json.url).toContain('/_local/raw/'); + expect(res._json.data.url).toContain('/_local/raw/'); expect(authorizeFileRead).toHaveBeenCalledOnce(); }); @@ -421,7 +421,11 @@ describe('Storage REST Routes', () => { const putRes = createMockRes(); await putHandler(putReq, putRes); expect(putRes._status).toBe(200); - expect(putRes._json.ok).toBe(true); + // `{ ok: true, key }` until #3689 — `ok` was a private second word for + // the `success` the declared envelope already carries. + expect(putRes._json.success).toBe(true); + expect(putRes._json.ok).toBeUndefined(); + expect(putRes._json.data.key).toBe('rawtest/file.bin'); // Verify file was written const downloaded = await adapter.download('rawtest/file.bin'); diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 10aa5d7459..22f891edc0 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -21,16 +21,41 @@ export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated'; * real message from the dispatcher (#3675). `ObjectStackClient` papers over * the difference by sniffing four shapes; that shim is the consumer-side * symptom Prime Directive #12 says to cure at the producer. - * - * Only the ERROR path is normalized here. The success bodies are still three - * shapes of their own (`{ data }`, bare `{ url }`, `{ ok, key }`, none with - * `success: true`) — a separate, non-additive change, tracked as its own - * issue rather than smuggled into this one. */ function sendError(res: IHttpResponse, status: number, code: string, message: string): void { res.status(status).json({ success: false, error: { code, message } }); } +/** + * Emit a success body in the DECLARED envelope — `BaseResponseSchema` + * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`. + * That is what `storage.zod.ts` has declared for these routes all along, and + * what `ObjectStackClient.unwrapResponse` keys on. + * + * #3675 normalized the error path and deliberately left this one alone, + * because unlike the errors it is not additive. The success bodies were three + * shapes, none carrying `success` (#3689): + * + * - the six upload routes: `{ data: {…} }` — flag missing, payload already in place + * - `GET /files/:fileId/url`: `{ url }` — no envelope at all + * - `PUT /_local/raw/:token`: `{ ok: true, key }` — a second, private word for `success` + * + * so the last two MOVE their payload under `data`. The consumers were taught + * both shapes first — `storage.getDownloadUrl` reads through the SDK's + * standard `unwrapResponse`, the console's two attachment openers already read + * `body?.url ?? body?.data?.url` — so the repos are not coupled by merge + * order, the same way the error path was landed. `ok` is dropped rather than + * kept beside `success`: one envelope, one word for it. + * + * Every success body in this module goes through here, so the shape lives in + * exactly one line. `success-envelope.conformance.test.ts` drives each route, + * parses the result against the real spec schemas, and scans this source so a + * new route cannot quietly reintroduce a bare one. + */ +function sendOk(res: IHttpResponse, data: unknown): void { + res.json({ success: true, data }); +} + /** * Options for the storage route registration helper. */ @@ -230,15 +255,13 @@ export function registerStorageRoutes( uploadUrl = `${basePath}/_local/raw/${fileId}`; } - res.json({ - data: { - uploadUrl, - method, - headers, - fileId, - expiresIn, - downloadUrl: `${basePath}/files/${fileId}/url`, - }, + sendOk(res, { + uploadUrl, + method, + headers, + fileId, + expiresIn, + downloadUrl: `${basePath}/files/${fileId}/url`, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -268,20 +291,18 @@ export function registerStorageRoutes( etag: eTag ?? undefined, }); - res.json({ - data: { - // The opaque sys_file id — the value a file field stores as a - // reference (ADR-0104 D3). Previously omitted, so a caller could not - // learn the id to persist after committing an upload. - fileId: updated!.id ?? fileId, - path: updated!.key, - name: updated!.name, - size: updated!.size ?? 0, - mimeType: updated!.mime_type ?? 'application/octet-stream', - lastModified: updated!.updated_at ?? new Date().toISOString(), - created: updated!.created_at ?? new Date().toISOString(), - etag: updated!.etag, - }, + sendOk(res, { + // The opaque sys_file id — the value a file field stores as a + // reference (ADR-0104 D3). Previously omitted, so a caller could not + // learn the id to persist after committing an upload. + fileId: updated!.id ?? fileId, + path: updated!.key, + name: updated!.name, + size: updated!.size ?? 0, + mimeType: updated!.mime_type ?? 'application/octet-stream', + lastModified: updated!.updated_at ?? new Date().toISOString(), + created: updated!.created_at ?? new Date().toISOString(), + etag: updated!.etag, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -354,15 +375,13 @@ export function registerStorageRoutes( expires_at: expiresAt, }); - res.json({ - data: { - uploadId, - resumeToken, - fileId, - totalChunks, - chunkSize, - expiresAt, - }, + sendOk(res, { + uploadId, + resumeToken, + fileId, + totalChunks, + chunkSize, + expiresAt, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -425,12 +444,10 @@ export function registerStorageRoutes( parts: JSON.stringify(currentParts), }); - res.json({ - data: { - chunkIndex, - eTag, - bytesReceived: data.byteLength, - }, + sendOk(res, { + chunkIndex, + eTag, + bytesReceived: data.byteLength, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -467,14 +484,12 @@ export function registerStorageRoutes( await store.updateFile(session.file_id, { status: 'committed', key: finalKey }); await store.updateSession(uploadId, { status: 'completed' }); - res.json({ - data: { - fileId: session.file_id, - key: finalKey, - size: session.total_size, - mimeType: session.mime_type ?? 'application/octet-stream', - url: `${basePath}/files/${session.file_id}/url`, - }, + sendOk(res, { + fileId: session.file_id, + key: finalKey, + size: session.total_size, + mimeType: session.mime_type ?? 'application/octet-stream', + url: `${basePath}/files/${session.file_id}/url`, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -500,20 +515,18 @@ export function registerStorageRoutes( ? Math.min(100, Math.round((uploadedSize / session.total_size) * 100)) : 0; - res.json({ - data: { - uploadId: session.id, - fileId: session.file_id, - filename: session.filename, - totalSize: session.total_size, - uploadedSize, - totalChunks: session.total_chunks, - uploadedChunks, - percentComplete, - status: session.status, - startedAt: session.started_at, - expiresAt: session.expires_at, - }, + sendOk(res, { + uploadId: session.id, + fileId: session.file_id, + filename: session.filename, + totalSize: session.total_size, + uploadedSize, + totalChunks: session.total_chunks, + uploadedChunks, + percentComplete, + status: session.status, + startedAt: session.started_at, + expiresAt: session.expires_at, }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); @@ -550,7 +563,10 @@ export function registerStorageRoutes( return; } - res.json({ url }); + // `{ success: true, data: { url } }` since #3689 — this route used to + // answer a bare `{ url }`, the one success body on the surface that + // carried no envelope at all. + sendOk(res, { url }); } catch (err: any) { sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } @@ -626,7 +642,13 @@ export function registerStorageRoutes( } await storage.upload(payload.k, data, { contentType: payload.ct }); - res.json({ ok: true, key: payload.k }); + // `{ ok: true, key }` until #3689. `ok` was a second, private word for + // `success` on a route that is mounted under `/api/v1/storage` like any + // other; the envelope now says it once. Nothing reads this body — the + // SDK and the console both PUT here opaquely, exactly as they would an + // S3 presigned URL, and check only the status — so the move is + // observable to conformance tests and to curl, not to a caller. + sendOk(res, { key: payload.k }); } catch (err: any) { const invalidToken = err?.message?.includes('expired') || err?.message?.includes('signature'); sendError( diff --git a/packages/services/service-storage/src/success-envelope.conformance.test.ts b/packages/services/service-storage/src/success-envelope.conformance.test.ts new file mode 100644 index 0000000000..ff53dba204 --- /dev/null +++ b/packages/services/service-storage/src/success-envelope.conformance.test.ts @@ -0,0 +1,326 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Success-envelope conformance for the autonomously-mounted + * `/api/v1/storage/*` routes (#3689) — the success-path twin of the error-path + * fix in #3675 (`error-envelope.conformance.test.ts`), and the same pairing + * i18n went through in #3636 / #3675. + * + * The drift this closes: `storage.zod.ts` declared every one of these routes as + * `BaseResponseSchema.extend({ data })`, and `PresignedUrlResponse` and friends + * are `z.infer`red from those schemas and published as the SDK's return types — + * while the wire carried three shapes, none of them with `success`: + * + * - the six upload routes: `{ data: {…} }` + * - `GET /files/:fileId/url`: `{ url }` + * - `PUT /_local/raw/:token`: `{ ok: true, key }` + * + * It broke nothing only because the SDK's storage methods returned + * `res.json()` raw — `any`, so TypeScript could not see the gap, and nothing + * relied on the declaration. That is exactly the posture i18n was in until + * something did rely on it. + * + * So the assertions here are against the DECLARED schemas, imported from + * `packages/spec` rather than restated: a body that parses is a body the SDK's + * published return type does not lie about. Three directions are covered: + * + * 1. every success-producing branch is DRIVEN and parsed against both + * `BaseResponseSchema` and the specific schema that route declares; + * 2. the two retired shapes are asserted DEAD, so a revert cannot pass + * quietly; + * 3. the module source is scanned, so a NEW route cannot bypass the + * `sendOk` helper and reintroduce a bare body — without this the suite + * would only ever cover the routes that existed the day it was written. + * + * As with the error twin: the route ledgers cannot catch any of this. They + * audit which routes exist and whether the SDK can address them, not what + * comes back. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { BaseResponseSchema } from '@objectstack/spec/api'; +import { + PresignedUrlResponseSchema, + FileUploadResponseSchema, + InitiateChunkedUploadResponseSchema, + UploadChunkResponseSchema, + CompleteChunkedUploadResponseSchema, + UploadProgressSchema, + FileDownloadUrlResponseSchema, + RawUploadResponseSchema, +} from '@objectstack/spec/api'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { LocalStorageAdapter } from './local-storage-adapter'; +import { StorageMetadataStore } from './metadata-store'; +import { registerStorageRoutes } from './storage-routes'; + +const BASE = '/api/v1/storage'; + +interface Captured { + status: number; + body: any; +} + +function mount(storage: any, store: any, routeOpts: any = {}) { + const routes = new Map(); + const http = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: (p: string, h: RouteHandler) => { routes.set(`PUT:${p}`, h); }, + delete: () => {}, + patch: () => {}, + use: () => {}, + listen: async () => {}, + close: async () => {}, + }; + registerStorageRoutes(http as any, storage, store, { basePath: BASE, ...routeOpts }); + return routes; +} + +async function drive( + routes: Map, + method: string, + path: string, + req: Partial = {}, +): Promise { + const handler = routes.get(`${method}:${path}`); + if (!handler) throw new Error(`no handler for ${method} ${path}`); + const captured: Captured = { status: 200, body: undefined }; + const res: any = { + json(data: any) { captured.body = data; }, + send() {}, + status(code: number) { captured.status = code; return res; }, + header() { return res; }, + }; + await handler( + { params: {}, query: {}, body: undefined, headers: {}, method, path, ...req } as IHttpRequest, + res as IHttpResponse, + ); + return captured; +} + +async function tmpAdapter(): Promise { + const rootDir = join(tmpdir(), `os-ok-env-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + return new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); +} + +/** Presign → PUT the bytes → commit, returning the fileId of a live file. */ +async function committedFile( + routes: Map, + adapter: LocalStorageAdapter, +): Promise { + const presigned = await drive(routes, 'POST', `${BASE}/upload/presigned`, { + body: { filename: 'dl.txt', mimeType: 'text/plain', size: 5 }, + }); + const fileId = presigned.body.data.fileId; + await adapter.upload(`user/${fileId}.txt`, Buffer.from('hello')); + await drive(routes, 'POST', `${BASE}/upload/complete`, { body: { fileId } }); + return fileId; +} + +describe('storage success envelope (#3689)', () => { + /** + * One entry per success-producing route. `schema` is the contract that route + * declares in `packages/spec/src/api/storage.zod.ts` — the same object the + * SDK's return type is inferred from, so a body that parses is a body the + * published type describes. + */ + const CASES: Array<{ + name: string; + schema: { safeParse(v: unknown): { success: boolean; error?: unknown } }; + /** Payload keys the route is expected to carry, asserted under `data`. */ + dataKeys: string[]; + run: () => Promise; + }> = [ + { + name: 'POST /upload/presigned', + schema: PresignedUrlResponseSchema, + dataKeys: ['uploadUrl', 'method', 'fileId', 'expiresIn'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/presigned`, { + body: { filename: 'photo.jpg', mimeType: 'image/jpeg', size: 1024, scope: 'user' }, + }); + }, + }, + { + name: 'POST /upload/complete', + schema: FileUploadResponseSchema, + dataKeys: ['fileId', 'path', 'name', 'size', 'mimeType', 'lastModified', 'created'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + const presigned = await drive(routes, 'POST', `${BASE}/upload/presigned`, { + body: { filename: 'test.txt', mimeType: 'text/plain', size: 5 }, + }); + return drive(routes, 'POST', `${BASE}/upload/complete`, { + body: { fileId: presigned.body.data.fileId }, + }); + }, + }, + { + name: 'POST /upload/chunked', + schema: InitiateChunkedUploadResponseSchema, + dataKeys: ['uploadId', 'resumeToken', 'fileId', 'totalChunks', 'chunkSize', 'expiresAt'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/chunked`, { + body: { filename: 'large.bin', mimeType: 'application/octet-stream', totalSize: 100 }, + }); + }, + }, + { + name: 'PUT /upload/chunked/:uploadId/chunk/:chunkIndex', + schema: UploadChunkResponseSchema, + dataKeys: ['chunkIndex', 'eTag', 'bytesReceived'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + const init = await drive(routes, 'POST', `${BASE}/upload/chunked`, { + body: { filename: 'large.bin', mimeType: 'application/octet-stream', totalSize: 100 }, + }); + const { uploadId, resumeToken } = init.body.data; + return drive(routes, 'PUT', `${BASE}/upload/chunked/:uploadId/chunk/:chunkIndex`, { + params: { uploadId, chunkIndex: '0' }, + headers: { 'x-resume-token': resumeToken }, + rawBody: async () => Buffer.from('a'.repeat(100)), + } as any); + }, + }, + { + name: 'POST /upload/chunked/:uploadId/complete', + schema: CompleteChunkedUploadResponseSchema, + dataKeys: ['fileId', 'key', 'size', 'mimeType', 'url'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + const init = await drive(routes, 'POST', `${BASE}/upload/chunked`, { + body: { filename: 'large.bin', mimeType: 'application/octet-stream', totalSize: 100 }, + }); + const { uploadId, resumeToken } = init.body.data; + const chunk = await drive(routes, 'PUT', `${BASE}/upload/chunked/:uploadId/chunk/:chunkIndex`, { + params: { uploadId, chunkIndex: '0' }, + headers: { 'x-resume-token': resumeToken }, + rawBody: async () => Buffer.from('a'.repeat(100)), + } as any); + return drive(routes, 'POST', `${BASE}/upload/chunked/:uploadId/complete`, { + params: { uploadId }, + body: { parts: [{ chunkIndex: 0, eTag: chunk.body.data.eTag }] }, + }); + }, + }, + { + name: 'GET /upload/chunked/:uploadId/progress', + schema: UploadProgressSchema, + dataKeys: ['uploadId', 'fileId', 'filename', 'totalSize', 'uploadedSize', 'percentComplete', 'status'], + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + const init = await drive(routes, 'POST', `${BASE}/upload/chunked`, { + body: { filename: 'progress.bin', mimeType: 'application/octet-stream', totalSize: 200 }, + }); + return drive(routes, 'GET', `${BASE}/upload/chunked/:uploadId/progress`, { + params: { uploadId: init.body.data.uploadId }, + }); + }, + }, + { + // The one that MOVED: a bare `{ url }` before #3689, with no envelope at + // all and no schema in `storage.zod.ts` to answer to. + name: 'GET /files/:fileId/url', + schema: FileDownloadUrlResponseSchema, + dataKeys: ['url'], + run: async () => { + const adapter = await tmpAdapter(); + const routes = mount(adapter, new StorageMetadataStore(null)); + const fileId = await committedFile(routes, adapter); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId } }); + }, + }, + { + // And the other: `{ ok: true, key }`, whose `ok` was a private second + // word for the `success` the envelope already carries. + name: 'PUT /_local/raw/:token', + schema: RawUploadResponseSchema, + dataKeys: ['key'], + run: async () => { + const adapter = await tmpAdapter(); + const routes = mount(adapter, new StorageMetadataStore(null)); + const desc = await adapter.getPresignedUpload!('rawtest/file.bin', 60, { + contentType: 'application/octet-stream', + }); + return drive(routes, 'PUT', `${BASE}/_local/raw/:token`, { + params: { token: desc.uploadUrl.split('/_local/raw/')[1] }, + rawBody: async () => Buffer.from('raw bytes'), + } as any); + }, + }, + ]; + + for (const c of CASES) { + it(`${c.name} answers { success: true, data } and parses as its declared schema`, async () => { + const { status, body } = await c.run(); + expect(status).toBe(200); + + // The shared envelope, imported — not a restatement of it. + const base = BaseResponseSchema.safeParse(body); + expect(base.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + expect(body.success).toBe(true); + expect(body.error).toBeUndefined(); + + // The route's OWN declared contract — the object `packages/spec` infers + // the SDK's return type from. This is the assertion that makes + // `declared === actual` rather than merely `declared ≈ actual`. + const declared = c.schema.safeParse(body); + expect( + declared.success, + `body does not match its declared schema: ${JSON.stringify(declared.error ?? body)}`, + ).toBe(true); + + // The payload really is under `data`, not scattered across the top level. + for (const k of c.dataKeys) { + expect(body.data?.[k], `data.${k} missing from ${c.name}`).toBeDefined(); + } + }); + } + + it('the pre-#3689 shapes are dead — no top-level payload, no second success word', async () => { + for (const c of CASES) { + const { body } = await c.run(); + // `{ url }` and `{ ok: true, key }` lived at the top level. + expect(body.url, `${c.name} still answers a top-level url`).toBeUndefined(); + expect(body.ok, `${c.name} still answers a top-level ok`).toBeUndefined(); + expect(body.key, `${c.name} still answers a top-level key`).toBeUndefined(); + // And `{ data }` alone, without the flag, was the third shape. + expect(typeof body.success, `${c.name} answers no success flag`).toBe('boolean'); + } + }); + + it('routes every success through `sendOk` — no route may reintroduce a bare body', () => { + // Comments stripped first: this module's prose quotes both the old and the + // new shape, and a doc comment is not a code path. + const source = readFileSync(new URL('./storage-routes.ts', import.meta.url), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, ''); + + // Exactly two `.json(` call sites in the module: the one inside `sendOk` + // and the one inside `sendError`. Every route body is built by a helper, so + // this count does not grow with the number of routes — only with a route + // that bypasses them. + const jsonCalls = [...source.matchAll(/\.json\(/g)]; + expect( + jsonCalls, + `expected only sendOk + sendError to call res.json(), found ${jsonCalls.length}`, + ).toHaveLength(2); + + // One builder per envelope half, so each shape lives in one line of this + // package. (`success: false` is the error twin's assertion; both are + // pinned here so moving one without the other fails.) + expect([...source.matchAll(/success:\s*true/g)]).toHaveLength(1); + expect([...source.matchAll(/success:\s*false/g)]).toHaveLength(1); + + // The retired words, gone from the code path rather than merely unused. + expect([...source.matchAll(/\bok:\s*true/g)]).toHaveLength(0); + }); +}); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 1cb5316e24..afc8db5981 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2491,6 +2491,8 @@ "FieldErrorSchema (const)", "FieldMappingEntry (type)", "FieldMappingEntrySchema (const)", + "FileDownloadUrlResponse (type)", + "FileDownloadUrlResponseSchema (const)", "FileTypeValidation (type)", "FileTypeValidationSchema (const)", "FileUploadResponse (type)", @@ -2821,6 +2823,8 @@ "QueryAdapterTargetSchema (const)", "QueryOptimizationConfig (type)", "QueryOptimizationConfigSchema (const)", + "RawUploadResponse (type)", + "RawUploadResponseSchema (const)", "RealtimeConfig (type)", "RealtimeConfigSchema (const)", "RealtimeConnectRequest (type)", diff --git a/packages/spec/json-schema.manifest.json b/packages/spec/json-schema.manifest.json index 241453face..c3ecf84bca 100644 --- a/packages/spec/json-schema.manifest.json +++ b/packages/spec/json-schema.manifest.json @@ -212,6 +212,7 @@ "api/ExportRequest", "api/FieldError", "api/FieldMappingEntry", + "api/FileDownloadUrlResponse", "api/FileTypeValidation", "api/FileUploadResponse", "api/FilterOperator", @@ -377,6 +378,7 @@ "api/QueryAdapterConfig", "api/QueryAdapterTarget", "api/QueryOptimizationConfig", + "api/RawUploadResponse", "api/RealtimeConfig", "api/RealtimeConnectRequest", "api/RealtimeConnectResponse", diff --git a/packages/spec/src/api/storage.test.ts b/packages/spec/src/api/storage.test.ts index 655c749fa2..3697755886 100644 --- a/packages/spec/src/api/storage.test.ts +++ b/packages/spec/src/api/storage.test.ts @@ -12,6 +12,8 @@ import { CompleteChunkedUploadRequestSchema, CompleteChunkedUploadResponseSchema, UploadProgressSchema, + FileDownloadUrlResponseSchema, + RawUploadResponseSchema, StorageApiContracts, } from './storage.zod'; @@ -604,6 +606,64 @@ describe('CompleteChunkedUploadResponseSchema', () => { }); }); +// ========================================== +// Download / raw-loopback responses (#3689) +// +// Both routes existed long before they were declared, and both answered +// outside the shared envelope while undeclared — `{ url }` and +// `{ ok: true, key }`. The schemas below are the contract they now answer to; +// `service-storage`'s success-envelope conformance suite parses the real +// bodies against these exact objects. +// ========================================== + +describe('FileDownloadUrlResponseSchema', () => { + it('should accept an absolute signed URL', () => { + const resp = FileDownloadUrlResponseSchema.parse({ + success: true, + data: { url: 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc' }, + }); + expect(resp.success).toBe(true); + expect(resp.data.url).toContain('X-Amz-Signature'); + }); + + it('should accept the local adapter’s server-relative loopback URL', () => { + const resp = FileDownloadUrlResponseSchema.parse({ + success: true, + data: { url: '/api/v1/storage/_local/raw/eyJrIjoi.c2ln' }, + }); + expect(resp.data.url).toBe('/api/v1/storage/_local/raw/eyJrIjoi.c2ln'); + }); + + it('should reject the bare pre-#3689 shape', () => { + expect(() => + FileDownloadUrlResponseSchema.parse({ url: 'https://cdn.example.com/f1.png' }) + ).toThrow(); + }); + + it('should reject a missing url', () => { + expect(() => FileDownloadUrlResponseSchema.parse({ success: true, data: {} })).toThrow(); + }); +}); + +describe('RawUploadResponseSchema', () => { + it('should accept the written key', () => { + const resp = RawUploadResponseSchema.parse({ + success: true, + data: { key: 'user/f1.png' }, + }); + expect(resp.success).toBe(true); + expect(resp.data.key).toBe('user/f1.png'); + }); + + it('should reject the pre-#3689 `{ ok, key }` shape', () => { + expect(() => RawUploadResponseSchema.parse({ ok: true, key: 'user/f1.png' })).toThrow(); + }); + + it('should reject a missing key', () => { + expect(() => RawUploadResponseSchema.parse({ success: true, data: {} })).toThrow(); + }); +}); + // ========================================== // Storage API Contracts // ========================================== @@ -616,6 +676,9 @@ describe('StorageApiContracts', () => { expect(StorageApiContracts.uploadChunk).toBeDefined(); expect(StorageApiContracts.completeChunkedUpload).toBeDefined(); expect(StorageApiContracts.getUploadProgress).toBeDefined(); + // #3689: the download resolve is an SDK-addressed route + // (`storage.getDownloadUrl`) that had been missing from the registry. + expect(StorageApiContracts.getDownloadUrl).toBeDefined(); }); it('should have correct HTTP methods', () => { @@ -625,6 +688,7 @@ describe('StorageApiContracts', () => { expect(StorageApiContracts.uploadChunk.method).toBe('PUT'); expect(StorageApiContracts.completeChunkedUpload.method).toBe('POST'); expect(StorageApiContracts.getUploadProgress.method).toBe('GET'); + expect(StorageApiContracts.getDownloadUrl.method).toBe('GET'); }); it('should have valid paths', () => { @@ -633,6 +697,7 @@ describe('StorageApiContracts', () => { expect(StorageApiContracts.uploadChunk.path).toContain('/chunk/:chunkIndex'); expect(StorageApiContracts.completeChunkedUpload.path).toContain('/complete'); expect(StorageApiContracts.getUploadProgress.path).toContain('/progress'); + expect(StorageApiContracts.getDownloadUrl.path).toContain('/storage/files/:fileId/url'); }); it('should have input and output schemas for each endpoint', () => { @@ -645,5 +710,6 @@ describe('StorageApiContracts', () => { expect(StorageApiContracts.completeChunkedUpload.input).toBeDefined(); expect(StorageApiContracts.completeChunkedUpload.output).toBeDefined(); expect(StorageApiContracts.getUploadProgress.output).toBeDefined(); + expect(StorageApiContracts.getDownloadUrl.output).toBeDefined(); }); }); diff --git a/packages/spec/src/api/storage.zod.ts b/packages/spec/src/api/storage.zod.ts index a05a289a42..84c02f74f7 100644 --- a/packages/spec/src/api/storage.zod.ts +++ b/packages/spec/src/api/storage.zod.ts @@ -49,10 +49,49 @@ export const FileUploadResponseSchema = lazySchema(() => BaseResponseSchema.exte data: FileMetadataSchema.describe('Uploaded file metadata'), })); +/** + * Download URL Response + * + * `GET /api/v1/storage/files/:fileId/url` — resolves a committed file to a + * short-lived signed URL (absolute for S3/GCS, server-relative for the local + * adapter's `_local/raw` loopback). + * + * Declared here as of #3689. The route always existed and was always ledgered + * `disposition: 'sdk'` (`storage.getDownloadUrl`), but it had no schema — so + * it answered a bare `{ url }` with nothing to answer to, the only success + * body on this surface outside the envelope. Both are now fixed together: + * the shape is declared, and the route emits it. + */ +export const FileDownloadUrlResponseSchema = lazySchema(() => BaseResponseSchema.extend({ + data: z.object({ + url: z.string().describe('Short-lived signed download URL; may be server-relative'), + }), +})); + +/** + * Raw Upload Response + * + * `PUT /api/v1/storage/_local/raw/:token` — the loopback target + * `LocalStorageAdapter` mints for its own presign tokens, standing in for the + * S3 presigned PUT a cloud adapter would hand out. Callers PUT to it opaquely + * and read only the status, so the body exists for conformance and for curl. + * + * Declared as of #3689, which also retired the `{ ok: true, key }` shape it + * used to answer: `ok` was a second word for the `success` the envelope + * already carries. + */ +export const RawUploadResponseSchema = lazySchema(() => BaseResponseSchema.extend({ + data: z.object({ + key: z.string().describe('Storage key the bytes were written to'), + }), +})); + export type GetPresignedUrlRequest = z.infer; export type CompleteUploadRequest = z.infer; export type PresignedUrlResponse = z.infer; export type FileUploadResponse = z.infer; +export type FileDownloadUrlResponse = z.infer; +export type RawUploadResponse = z.infer; // ========================================== // Chunked / Resumable Upload Protocol @@ -239,4 +278,17 @@ export const StorageApiContracts = { path: '/api/v1/storage/upload/chunked/:uploadId/progress', output: UploadProgressSchema, }, + // The download resolve. An SDK-addressed route (`storage.getDownloadUrl`) + // that had been missing from this registry, which is how its response shape + // went undeclared long enough to drift outside the envelope (#3689). + // + // The two `_local/raw/:token` routes stay out on purpose: they are the local + // adapter's own presign loopback, ledgered `server-only`, addressed as an + // opaque signed URL rather than as an API. `RawUploadResponseSchema` above + // declares what the PUT answers without promoting it to a client contract. + getDownloadUrl: { + method: 'GET' as const, + path: '/api/v1/storage/files/:fileId/url', + output: FileDownloadUrlResponseSchema, + }, };