Skip to content

Commit 6633337

Browse files
os-zhuangclaude
andauthored
fix(service-storage): emit the declared success envelope on all eight routes (#3689) (#3837)
The other half of #3675. That PR 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 carrying the `success` flag `BaseResponseSchema` declares and `ObjectStackClient.unwrapResponse` keys on — the six upload routes { 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.inferred 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 the compiler 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. The payload MOVES on the last two, which is why this was filed separately rather than smuggled into #3675. Every in-repo consumer was fixed first, so the 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 — and the console's two attachment openers already read both `url` shapes (objectui follow-up pins that). `ok` is dropped rather than kept beside `success`: it was a second, private word for the same thing. 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 `_local/raw/:token` pair stays out of the registry on purpose: it is 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 its error twin holds the other: 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 single `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. Claude-Session: https://claude.ai/code/session_01RmnSQC8KxxsoZQe3oDEHjG Co-authored-by: Claude <noreply@anthropic.com>
1 parent 14252d3 commit 6633337

13 files changed

Lines changed: 801 additions & 78 deletions
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/service-storage": patch
4+
"@objectstack/client": patch
5+
---
6+
7+
fix(service-storage): emit the declared success envelope on all eight routes (#3689)
8+
9+
#3675 moved the **error** bodies of the autonomously-mounted `/api/v1/storage/*`
10+
routes into the declared `{ success: false, error: { code, message } }`
11+
envelope and deliberately stopped there: unlike the errors, the success bodies
12+
were not an additive fix. They were three shapes, none of them carrying the
13+
`success` flag `BaseResponseSchema` declares and
14+
`ObjectStackClient.unwrapResponse` keys on —
15+
16+
| Route(s) | Was | Now |
17+
|---|---|---|
18+
| the six upload routes (`/upload/presigned`, `/upload/complete`, `/upload/chunked`, `…/chunk/:i`, `…/complete`, `…/progress`) | `{ data: {…} }` | `{ success: true, data: {…} }` |
19+
| `GET /files/:fileId/url` | `{ url }` | `{ success: true, data: { url } }` |
20+
| `PUT /_local/raw/:token` | `{ ok: true, key }` | `{ success: true, data: { key } }` |
21+
22+
— while `storage.zod.ts` declared every one of them as
23+
`BaseResponseSchema.extend({ data })`, and `PresignedUrlResponse` and friends
24+
are `z.infer`red from those schemas and published as the SDK's return types.
25+
The declaration said `success: boolean`; the wire said nothing. It broke
26+
nothing only because the storage SDK methods returned `res.json()` raw —
27+
`any`, so TypeScript could not see the gap and nothing relied on the
28+
declaration. That is the posture i18n was in before #3636, right up until
29+
something did rely on it.
30+
31+
**The payload moved on two routes, and that is the breaking part.** A direct
32+
HTTP caller reading `body.url` from `GET /files/:fileId/url` must now read
33+
`body.data.url`; one reading `body.ok`/`body.key` from the local adapter's
34+
`PUT /_local/raw/:token` loopback must read `body.success`/`body.data.key`.
35+
`ok` is dropped rather than kept beside `success` — it was a second, private
36+
word for the same thing. The six upload routes are additive: callers already
37+
destructure `.data`, and a new sibling key changes nothing.
38+
39+
Every in-repo consumer was fixed first, so the two repos are not coupled by
40+
merge order:
41+
42+
- `client.storage.getDownloadUrl()` now reads through `unwrapResponse`, the
43+
SDK's one standard envelope seam — which strips the envelope when present
44+
and returns the body untouched when not, so a client either side of this
45+
server change resolves the same URL. The other storage methods hand back the
46+
whole envelope by design and were already correct.
47+
- The console's two attachment openers (`RecordAttachmentsPanel`,
48+
`ApprovalsInboxPage`) already read `body?.url ?? body?.data?.url`; objectui
49+
gains tests pinning that tolerance as deliberate.
50+
51+
Two schemas that were missing are now declared — `FileDownloadUrlResponse` and
52+
`RawUploadResponse` — and `getDownloadUrl` joins `StorageApiContracts`, which
53+
it had never been in. That absence is how its shape drifted outside the
54+
envelope unnoticed. The two `_local/raw/:token` routes stay out of the
55+
registry on purpose: they are the local adapter's own presign loopback,
56+
ledgered `server-only` and addressed as an opaque signed URL rather than as an
57+
API.
58+
59+
`success-envelope.conformance.test.ts` holds the new shape in place the way
60+
`error-envelope.conformance.test.ts` holds the error one: every route is
61+
driven and its body parsed against the **declared schema** it answers to — not
62+
a restatement — the retired shapes are asserted dead, and the module source is
63+
scanned so a new route cannot bypass the `sendOk` helper. As with #3675, the
64+
route ledgers cannot catch this class of drift: they audit which routes exist
65+
and whether the SDK can address them, not what comes back.

content/docs/references/api/storage.mdx

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ rather than proxying bytes through the API server.
2020
## TypeScript Usage
2121

2222
```typescript
23-
import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
24-
import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
23+
import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
24+
import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
2525

2626
// Validate data
2727
const result = CompleteChunkedUploadRequest.parse(data);
@@ -65,6 +65,20 @@ const result = CompleteChunkedUploadRequest.parse(data);
6565
| **eTag** | `string` | optional | S3 ETag verification |
6666

6767

68+
---
69+
70+
## FileDownloadUrlResponse
71+
72+
### Properties
73+
74+
| Property | Type | Required | Description |
75+
| :--- | :--- | :--- | :--- |
76+
| **success** | `boolean` || Operation success status |
77+
| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false |
78+
| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata |
79+
| **data** | `{ url: string }` || |
80+
81+
6882
---
6983

7084
## FileTypeValidation
@@ -154,6 +168,20 @@ const result = CompleteChunkedUploadRequest.parse(data);
154168
| **data** | `{ uploadUrl: string; downloadUrl?: string; fileId: string; method: Enum<'PUT' \| 'POST'>; … }` || |
155169

156170

171+
---
172+
173+
## RawUploadResponse
174+
175+
### Properties
176+
177+
| Property | Type | Required | Description |
178+
| :--- | :--- | :--- | :--- |
179+
| **success** | `boolean` || Operation success status |
180+
| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false |
181+
| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata |
182+
| **data** | `{ key: string }` || |
183+
184+
157185
---
158186

159187
## UploadChunkRequest

packages/client/src/index.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2443,11 +2443,23 @@ export class ObjectStackClient {
24432443
return completeJson;
24442444
},
24452445

2446+
/**
2447+
* Resolve a committed file to a short-lived signed download URL.
2448+
*
2449+
* Read through `unwrapResponse` rather than off the raw body: the route
2450+
* answers the declared `{ success: true, data: { url } }` envelope as of
2451+
* #3689, and this SDK ships as its own npm package against servers it was
2452+
* not built with. `unwrapResponse` strips the envelope when it is there
2453+
* and hands back the body untouched when it is not, so a client on either
2454+
* side of that server upgrade resolves the same URL. That is the SDK's one
2455+
* standard envelope seam — every other enveloped method already goes
2456+
* through it — not a fallback grown for this route.
2457+
*/
24462458
getDownloadUrl: async (fileId: string): Promise<string> => {
24472459
const route = this.getRoute('storage');
24482460
const res = await this.fetch(`${this.baseUrl}${route}/files/${fileId}/url`);
2449-
const data = await res.json();
2450-
return data.url;
2461+
const { url } = await this.unwrapResponse<{ url: string }>(res);
2462+
return url;
24512463
},
24522464

24532465
/**
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Storage wire-dialect proof (#3689) — the SDK's storage methods against the
5+
* envelope `service-storage` actually emits, and against the one it emitted
6+
* before.
7+
*
8+
* The gap this pins: `storage.zod.ts` declared every storage response as
9+
* `BaseResponseSchema.extend({ data })`, `PresignedUrlResponse` and friends are
10+
* `z.infer`red from those schemas and published as these methods' return types
11+
* — and the methods returned `res.json()` raw. `res.json()` is `any`, so the
12+
* declaration could say `success: boolean` while the wire said nothing at all
13+
* and TypeScript would never know. `getDownloadUrl` was the one method that
14+
* read INTO a body, and it read `data.url` off a bare `{ url }`.
15+
*
16+
* #3689 moved the wire onto the declaration. This SDK ships as its own npm
17+
* package, so it meets servers on both sides of that change: the enveloped and
18+
* the bare body are both asserted here, and `unwrapResponse` — the client's one
19+
* standard envelope seam, not a fallback grown for this route — is what spans
20+
* them.
21+
*/
22+
23+
import { describe, it, expect, vi } from 'vitest';
24+
import { ObjectStackClient } from './index';
25+
26+
function clientReturning(body: any) {
27+
const fetchMock = vi.fn().mockResolvedValue({
28+
ok: true,
29+
status: 200,
30+
statusText: 'OK',
31+
json: async () => body,
32+
headers: new Headers(),
33+
});
34+
const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock });
35+
return { client, fetchMock };
36+
}
37+
38+
describe('storage.getDownloadUrl reads the signed URL off either dialect (#3689)', () => {
39+
it('resolves from the declared { success: true, data: { url } } envelope', async () => {
40+
const { client, fetchMock } = clientReturning({
41+
success: true,
42+
data: { url: '/api/v1/storage/_local/raw/eyJrIjoi.c2ln' },
43+
});
44+
45+
await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
46+
'/api/v1/storage/_local/raw/eyJrIjoi.c2ln',
47+
);
48+
expect(fetchMock.mock.calls[0][0]).toContain('/api/v1/storage/files/f1/url');
49+
});
50+
51+
it('still resolves from the bare { url } an older server answers', async () => {
52+
// Not a tolerated dialect going forward — a version-skew allowance. The
53+
// client is published separately from the server it talks to, so a build
54+
// predating the #3689 rollout must keep resolving downloads.
55+
const { client } = clientReturning({ url: 'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc' });
56+
57+
await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
58+
'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc',
59+
);
60+
});
61+
62+
it('resolves an absolute S3-style URL out of the envelope unchanged', async () => {
63+
const { client } = clientReturning({
64+
success: true,
65+
data: { url: 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc' },
66+
});
67+
68+
await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
69+
'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc',
70+
);
71+
});
72+
});
73+
74+
describe('the enveloped storage responses match their declared return types (#3689)', () => {
75+
/**
76+
* These methods hand back the whole envelope by design — their declared
77+
* return types ARE `BaseResponseSchema.extend({ data })`. Before #3689 the
78+
* `success` half of that declaration was fiction. Asserting it here is what
79+
* makes the published type honest, since `res.json()` erases to `any` and
80+
* the compiler cannot.
81+
*/
82+
it('getPresignedUrl carries success alongside data', async () => {
83+
const { client } = clientReturning({
84+
success: true,
85+
data: {
86+
uploadUrl: '/api/v1/storage/_local/raw/tok',
87+
method: 'PUT',
88+
fileId: 'f1',
89+
expiresIn: 3600,
90+
downloadUrl: '/api/v1/storage/files/f1/url',
91+
},
92+
});
93+
94+
const res = await client.storage.getPresignedUrl({
95+
filename: 'a.png',
96+
mimeType: 'image/png',
97+
size: 10,
98+
scope: 'user',
99+
});
100+
expect(res.success).toBe(true);
101+
expect(res.data.fileId).toBe('f1');
102+
});
103+
104+
it('initChunkedUpload carries success alongside data', async () => {
105+
const { client } = clientReturning({
106+
success: true,
107+
data: {
108+
uploadId: 'up1',
109+
resumeToken: 'tok',
110+
fileId: 'f1',
111+
totalChunks: 1,
112+
chunkSize: 5242880,
113+
expiresAt: '2026-01-01T00:00:00.000Z',
114+
},
115+
});
116+
117+
const res = await client.storage.initChunkedUpload({
118+
filename: 'big.bin',
119+
mimeType: 'application/octet-stream',
120+
totalSize: 100,
121+
chunkSize: 5242880,
122+
scope: 'user',
123+
});
124+
expect(res.success).toBe(true);
125+
expect(res.data.uploadId).toBe('up1');
126+
});
127+
128+
it('uploadPart carries success alongside data', async () => {
129+
const { client } = clientReturning({
130+
success: true,
131+
data: { chunkIndex: 0, eTag: '"abc"', bytesReceived: 100 },
132+
});
133+
134+
const res = await client.storage.uploadPart('up1', 0, 'tok', Buffer.from('x'));
135+
expect(res.success).toBe(true);
136+
expect(res.data.eTag).toBe('"abc"');
137+
});
138+
});

packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -327,10 +327,14 @@ describe('attachments permission matrix (#2755)', () => {
327327
expect(denied.status).toBe(403);
328328
expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
329329

330-
// The owner (admin) → 200 with a signed URL.
330+
// The owner (admin) → 200 with a signed URL, in the declared
331+
// `{ success: true, data: { url } }` envelope (#3689 — this route answered
332+
// a bare `{ url }` until then).
331333
const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`);
332334
expect(owner.status).toBe(200);
333-
expect(((await owner.json()) as any).url).toBeTruthy();
335+
const ownerBody = (await owner.json()) as any;
336+
expect(ownerBody.success).toBe(true);
337+
expect(ownerBody.data?.url).toBeTruthy();
334338

335339
// Parent-inherited read: a file on the PUBLIC att_case record is
336340
// downloadable by any member who can read that record — even a

packages/services/service-storage/src/storage-route-ledger.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,13 @@ export const STORAGE_ROUTE_LEDGER: readonly StorageRouteLedgerEntry[] = [
7979

8080
// ── download ──────────────────────────────────────────────────────────────
8181
{ route: 'GET /api/v1/storage/files/:fileId/url', family: 'download', disposition: 'sdk', client: 'storage.getDownloadUrl',
82-
note: 'JSON { url } — the authorization-gated signed-URL resolve the dispatcher ledger points at (#3584)' },
82+
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' },
8383
{ route: 'GET /api/v1/storage/files/:fileId', family: 'download', disposition: 'server-only',
8484
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 <img src>/<a href> — a browser URL, not an SDK call. The SDK resolves via the /url sibling above.' },
8585

8686
// ── local-driver loopback (LocalStorageAdapter presign targets) ───────────
8787
{ route: 'PUT /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only',
88-
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.' },
88+
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`.' },
8989
{ route: 'GET /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only',
9090
note: 'ditto for download: the target of getPresignedDownload/getSignedUrl on the local adapter, handed to the browser as an opaque signed link.' },
9191
];

packages/services/service-storage/src/storage-routes.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ describe('Storage REST Routes', () => {
241241
const urlRes = createMockRes();
242242
await urlHandler(urlReq, urlRes);
243243
expect(urlRes._status).toBe(200);
244-
expect(urlRes._json.url).toContain('/_local/raw/');
244+
expect(urlRes._json.data.url).toContain('/_local/raw/');
245245
});
246246

247247
it('should 404 for non-committed file', async () => {
@@ -308,7 +308,7 @@ describe('Storage REST Routes', () => {
308308
await commit(s, { id: 'a3' });
309309
const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a3');
310310
expect(res._status).toBe(200);
311-
expect(res._json.url).toContain('/_local/raw/');
311+
expect(res._json.data.url).toContain('/_local/raw/');
312312
expect(authorizeFileRead).toHaveBeenCalledOnce();
313313
});
314314

@@ -421,7 +421,11 @@ describe('Storage REST Routes', () => {
421421
const putRes = createMockRes();
422422
await putHandler(putReq, putRes);
423423
expect(putRes._status).toBe(200);
424-
expect(putRes._json.ok).toBe(true);
424+
// `{ ok: true, key }` until #3689 — `ok` was a private second word for
425+
// the `success` the declared envelope already carries.
426+
expect(putRes._json.success).toBe(true);
427+
expect(putRes._json.ok).toBeUndefined();
428+
expect(putRes._json.data.key).toBe('rawtest/file.bin');
425429

426430
// Verify file was written
427431
const downloaded = await adapter.download('rawtest/file.bin');

0 commit comments

Comments
 (0)