Skip to content

Commit aa25a81

Browse files
baozhoutaoclaude
andauthored
fix(client)!: DeleteDataResult 声明它自称的 schema —— success,不是 deleted (#5638) (#5657)
* fix(client)!: `DeleteDataResult` 声明它自称的 schema —— `success`,不是 `deleted` (#5638) `DeleteDataResult` 顶着 `Spec: DeleteDataResponseSchema` 的注释,却把该 schema 的 `success` 声明成了 `deleted`。两个 delete 面(`client.data.delete` 与 project 作用域下的同名方法)都是 `unwrapResponse` / `_unwrap` 纯直通,这个接口 是对服务端响应体的一句声明、而非改写,所以声明必须是 schema 的那一句。 `deleted` 从未被任何 schema 声明、也从未被 `/data/:object/:id` 的任何服务端路径 返回:#5581 / PR #5641 之后 protocol 与 ObjectQL 兜底两条路径同形返回 `{object, id, success}`。因此 `r.deleted` 编译通过、运行时恒 `undefined` —— 改名是揭错,不是破坏在用行为。不留 deprecated 双键(消费端同时认两种拼写正是 contract-first 禁止的形状)。 - 新增 `data-delete-result-shape.test.ts`:类型层钉 `DeleteDataResult` 与 spec `DeleteDataResponse` 双向可赋值,加两个面的直通形状 + 「不合成 `success`」反钉。 - `client.hono.test.ts` 补上这套 live server 套件一直缺的 delete 用例:真实 HTTP DELETE,读 `deleted.success`,并逐字断言键集(`z.object` 会剥未知键, 单靠 parse 证不了没有残留的 `deleted`)。 - `data-service.mdx` 的 delete 返回值由「success marker / deleted ID payload」 写实为 `{ object, id, success }`。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh * fix(cli): `os data delete` 的 `deleted` 输出键读的是幻影键,改读 spec 的 `success` (#5638) CI 抓到的漏网消费方 —— 我的读取方测绘按派单枚举的路径(client 自身测试、 client-react、examples、docs)扫,没有覆盖 `packages/cli`,而 `packages/cli/src/commands/data/delete.ts:65/68` 正是 `DeleteDataResult` 在 仓内唯一的具名字段读取方,`@objectstack/cli#build` 因此红。 该处 `deleted: result.deleted` 恒为 `undefined`,`JSON.stringify` 会把 undefined 丢掉 —— 也就是说这个命令声明了很久的 `deleted` 键,在 `os data delete --format json` 的任何一次运行里**都没有出现过**。现改读 `result.success`。 输出键名保持 `deleted` 不变:它是本命令自己的输出键,不是 protocol 的键; 而同一个 payload 顶层的 `success` 表达的是另一件事(CLI 信封的「命令完成」)。 把两者拼成同一个名字正是 #5641 点过的 `body.success` / `body.data.success` 混淆风险。改名属输出契约决定,已在 PR 里交维护者裁定。 与 #5644 无交集:那单整单落在 `packages/cli/src/commands/doctor.ts`。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016FNvXhtSdnEGEfLEsMmvxh --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 214f67c commit aa25a81

6 files changed

Lines changed: 268 additions & 5 deletions

File tree

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
---
2+
"@objectstack/client": major
3+
"@objectstack/cli": patch
4+
---
5+
6+
fix(client)!: `DeleteDataResult` declares the schema it names — `success`, not `deleted` (#5638)
7+
8+
`DeleteDataResult` — the return type of `client.data.delete()` and of the
9+
project-scoped `client.project(id).data.delete()` — carried the comment
10+
`Spec: DeleteDataResponseSchema` above a declaration that contradicted it:
11+
12+
```ts
13+
/** Spec: DeleteDataResponseSchema */
14+
export interface DeleteDataResult {
15+
object: string;
16+
id: string;
17+
deleted: boolean; // ← the schema declares `success`
18+
}
19+
```
20+
21+
`DeleteDataResponseSchema` (`packages/spec/src/api/protocol.zod.ts`) declares
22+
`{ object, id, success }`. `deleted` has never been declared by any schema, and
23+
no server path has ever returned it on `/data/:object/:id`.
24+
25+
**The old key was never readable at runtime — this rename reveals a defect, it
26+
does not break working code.** Both `delete` surfaces are pure `unwrapResponse`
27+
/ `_unwrap` passthroughs: the SDK returns the server's body untouched, so this
28+
interface is a *claim* about the wire, never a rewrite of it. The claim was
29+
false in the one direction that matters — the compiler endorsed the wrong
30+
spelling:
31+
32+
```ts
33+
const r = await client.data.delete('task', id);
34+
if (r.deleted) { … } // compiled; `undefined` at runtime; branch never taken
35+
if (r.success) { … } // rejected by the compiler; correct on the wire
36+
```
37+
38+
## What to change
39+
40+
`r.deleted` → `r.success`. That is the whole migration. Nothing about the
41+
request, the route, the status codes or the error shapes changes, and no server
42+
needs upgrading: the value you are now allowed to read is the one that was
43+
already arriving.
44+
45+
⛔ **Do not write `r.success ?? r.deleted`.** There is one producer shape, and a
46+
consumer that accepts two spellings is the shape contract-first exists to
47+
prevent — the same ruling #5581 applied on the producer side. No deprecated
48+
`deleted?: boolean` transition key ships for the same reason; a transition
49+
period is for keys that *worked*, and this one never did.
50+
51+
## Why the type was wrong on every deployment, not just some
52+
53+
The protocol path (`deleteData`) has always answered `success`. #5581 / PR
54+
#5641 brought the ObjectQL fallback — the path a slim assembly without
55+
`MetadataPlugin` takes — to the same shape. So before that fix the declaration
56+
was wrong on ordinary deployments and accidentally right on slim ones; after
57+
it, both paths answer `{ object, id, success }` and the declaration was simply
58+
wrong everywhere. The consumer-side correction had to follow the producer, not
59+
lead it.
60+
61+
## `os data delete` was reading the phantom key too
62+
63+
`packages/cli/src/commands/data/delete.ts` built its `--format json` / `--format
64+
yaml` payload with `deleted: result.deleted`. That evaluated to `undefined`, and
65+
`JSON.stringify` drops undefined values — so the `deleted` key the command has
66+
always declared **never appeared in a single run**. It now carries
67+
`result.success`, the server's own verdict.
68+
69+
Observable change: `os data delete --format json` gains `deleted: true` (YAML
70+
likewise) on a successful delete. The key name stays `deleted` deliberately —
71+
it is the CLI's output key, not the protocol's, and the payload's top-level
72+
`success` already means something different (the CLI envelope's "the command
73+
completed"). Conflating the two is the hazard #5641 called out when it noted
74+
that `body.success` and `body.data.success` are different facts. Scripts
75+
reading `.deleted` from this command were reading `undefined` before and get a
76+
boolean now; nothing that worked stops working.
77+
78+
## Downstream
79+
80+
`objectui`'s `ObjectStackDataSource.delete()` is a live victim of the old
81+
declaration — it guards `emitMutation` on `result.deleted`, so the delete
82+
mutation event has never fired against a real server and the method returns
83+
`undefined` where it declares `boolean`. Its own suite stayed green because the
84+
fixture mocks `{ deleted: true }`, a body no server produces. Filed as
85+
objectstack-ai/objectui#3412, which is blocked on this package publishing —
86+
its fix is a type unblock, not a behaviour change, since `success` is already
87+
what arrives.
88+
89+
## Pins
90+
91+
`packages/client/src/data-delete-result-shape.test.ts` asserts mutual
92+
assignability between `DeleteDataResult` and the spec's `DeleteDataResponse`,
93+
so a rename on either side (or a re-added optional `deleted`) fails
94+
`check:test-typecheck`. `client.hono.test.ts` gains the delete case this live
95+
server suite never had: a real DELETE over HTTP whose body is read as
96+
`deleted.success` and whose key set is asserted literally — `z.object` strips
97+
unknown keys, so a passing parse alone cannot prove no stray `deleted` rode
98+
along.

‎content/docs/kernel/runtime-services/data-service.mdx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ services.data.delete(object: string, id: string): Promise<DeleteDataResult>
3030
- `get`: single record payload
3131
- `find`: list payload + pagination metadata
3232
- `create`/`update`: mutated record payload
33-
- `delete`: success marker / deleted ID payload
33+
- `delete`: `{ object, id, success }` — the spec's `DeleteDataResponse`. The
34+
flag is `success`, not `deleted` (#5638)
3435
3536
## Typical Errors
3637

‎packages/cli/src/commands/data/delete.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,15 +57,24 @@ export default class DataDelete extends Command {
5757
// Delete the record
5858
const result = await client.data.delete(args.object, args.id);
5959

60+
// [#5638] `deleted` is THIS COMMAND's output key; its value is the
61+
// protocol's `DeleteDataResponse.success`. Two different booleans live
62+
// in this payload and must not be conflated: the top-level `success` is
63+
// the CLI envelope's "the command completed" flag (this branch is only
64+
// reached when it did), while the server's flag is its statement that
65+
// the deletion happened. Until now this read was `result.deleted` — a
66+
// key no server has ever returned — so it evaluated to `undefined` and
67+
// `JSON.stringify` dropped it: the documented key was simply absent
68+
// from every `os data delete --format json` run.
6069
if (flags.format === 'json') {
6170
await emitJson({
6271
success: true,
6372
object: result.object,
6473
id: result.id,
65-
deleted: result.deleted,
74+
deleted: result.success,
6675
});
6776
} else if (flags.format === 'yaml') {
68-
await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.deleted }, 'yaml');
77+
await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.success }, 'yaml');
6978
} else {
7079
printSuccess(`Record deleted: ${result.id}`);
7180
}

‎packages/client/src/client.hono.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,4 +200,37 @@ describe('ObjectStackClient (with Hono Server)', () => {
200200
expect(resultsResponse.records.length).toBeGreaterThan(0);
201201
expect(resultsResponse.records[0].name).toBe('Hono User');
202202
});
203+
204+
// [#5638] The one method whose declared return shape this suite never
205+
// exercised — and the one that was wrong. `DeleteDataResult` claimed
206+
// `deleted: boolean`; the schema it names declares `success`. This is the
207+
// runtime half of the pin: a REAL delete, over HTTP, against the server
208+
// this package's consumers talk to, read through the declared type.
209+
//
210+
// Nothing in this test is mocked into the answer: the DELETE route calls
211+
// `protocol.deleteData` (rest-server.ts), NOT the broker shim above, so the
212+
// body asserted here is the server's own.
213+
it('should delete data via hono, answering the SPEC\'s `success` body', async () => {
214+
const client = new ObjectStackClient({ baseUrl });
215+
await client.connect();
216+
217+
const created = await client.data.create('customer', {
218+
name: 'Doomed User',
219+
email: 'doomed@example.com',
220+
});
221+
222+
// Spec: DeleteDataResponse = { object, id, success }
223+
const deleted = await client.data.delete('customer', created.id);
224+
expect(deleted.success).toBe(true);
225+
expect(deleted.object).toBe('customer');
226+
expect(deleted.id).toBe(created.id);
227+
// The undeclared key must not ride along (a passing schema parse would
228+
// strip it silently, so assert the key set itself).
229+
expect(Object.keys(deleted).sort()).toEqual(['id', 'object', 'success']);
230+
231+
// And the delete actually happened — a success flag nobody cross-checks
232+
// is the cheapest thing in the world to keep green.
233+
const remaining = await client.data.find('customer', { where: { id: created.id } });
234+
expect(remaining.records.length).toBe(0);
235+
});
203236
});
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/**
2+
* [#5638] `DeleteDataResult` is a CLAIM about the server's delete body, and it
3+
* has to be the claim its own comment makes: `Spec: DeleteDataResponseSchema`.
4+
*
5+
* It declared `{ object, id, deleted: boolean }` while
6+
* `DeleteDataResponseSchema` declares `{ object, id, success: boolean }`. Both
7+
* `delete` surfaces (`client.data.delete` and the project-scoped
8+
* `client.project(id).data.delete`) are pure `unwrapResponse` / `_unwrap`
9+
* passthroughs — no runtime rewriting anywhere — so a consumer writing
10+
* `r.deleted` compiled fine and read `undefined` against every server path
11+
* that exists (#5581 / PR #5641 brought the ObjectQL fallback to `success`
12+
* too, so both producer paths now answer the same shape).
13+
*
14+
* WHICH LAYER ACTUALLY MOVES, and why that is not the usual one: the fix is a
15+
* type declaration, and types are erased before vitest ever runs. Reverting
16+
* `success` back to `deleted` therefore leaves every runtime assertion below
17+
* GREEN — they assert on a body the mock/server produced, which the client
18+
* never touched either way. The assertion that goes red on revert is the
19+
* type-level one (`SpecAndSdkAgree`), plus the `r.success` reads, and both are
20+
* enforced by `pnpm --filter @objectstack/client typecheck` → `check:test-typecheck`
21+
* (#5449/#5546), which compiles this file with the package's full strictness
22+
* and no debt-ledger entry. The runtime assertions below are shape pins: they
23+
* pin that the passthrough stays a passthrough, which is what makes the type
24+
* the only thing standing between a consumer and the wire.
25+
*/
26+
import { describe, it, expect, vi } from 'vitest';
27+
import { DeleteDataResponseSchema, type DeleteDataResponse } from '@objectstack/spec/api';
28+
import { ObjectStackClient, type DeleteDataResult } from './index';
29+
30+
/** Mutual assignability — `A extends B` alone would pass on a strict subset. */
31+
type Exact<A, B> = [A] extends [B] ? ([B] extends [A] ? true : false) : false;
32+
33+
/**
34+
* The pin. `DeleteDataResult` must be structurally the spec type, field for
35+
* field: rename either side, add a key to one, or make one optional, and this
36+
* stops being `true` — a type error at `check:test-typecheck`, not a runtime
37+
* failure. (This is deliberately an equality, not `extends`: the old `deleted`
38+
* declaration was neither a subset nor a superset of the schema, and a
39+
* one-directional check would still have caught it — but a future
40+
* `success: boolean; deleted?: boolean` "transition" shape would slip past
41+
* one, and that shape is exactly what #5638's triage ruled out.)
42+
*/
43+
const SpecAndSdkAgree: Exact<DeleteDataResult, DeleteDataResponse> = true;
44+
45+
/** Helper: a client whose fetch answers `body` with `status`. */
46+
function createMockClient(body: any, status = 200) {
47+
const fetchMock = vi.fn().mockResolvedValue({
48+
ok: status >= 200 && status < 300,
49+
status,
50+
statusText: status === 200 ? 'OK' : 'Error',
51+
json: async () => body,
52+
headers: new Headers(),
53+
});
54+
const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock });
55+
return { client, fetchMock };
56+
}
57+
58+
/** The body every server path answers a successful delete with (#5581). */
59+
const SPEC_BODY = { object: 'task', id: 't1', success: true };
60+
61+
describe('[#5638] DeleteDataResult === DeleteDataResponseSchema', () => {
62+
it('declares the schema\'s shape, field for field (type-level)', () => {
63+
// Reading the binding is what keeps `noUnusedLocals` satisfied; the
64+
// assertion that matters already happened in tsc, above.
65+
expect(SpecAndSdkAgree).toBe(true);
66+
});
67+
68+
it('the schema still declares `success` and has never declared `deleted`', () => {
69+
const parsed = DeleteDataResponseSchema.safeParse(SPEC_BODY);
70+
expect(parsed.success).toBe(true);
71+
expect(DeleteDataResponseSchema.safeParse({ object: 'task', id: 't1', deleted: true }).success)
72+
.toBe(false);
73+
});
74+
75+
for (const [surface, del] of [
76+
['client.data.delete', (c: ObjectStackClient) => c.data.delete('task', 't1')],
77+
['client.project(id).data.delete', (c: ObjectStackClient) => c.project('proj-xyz').data.delete('task', 't1')],
78+
] as const) {
79+
describe(surface, () => {
80+
it('hands back the spec body, and `success` is reachable through the declared type', async () => {
81+
const { client } = createMockClient(SPEC_BODY);
82+
const r: DeleteDataResult = await del(client);
83+
84+
// The typed read. Under the pre-#5638 declaration this line is
85+
// TS2339 (`success` does not exist) — and the line a consumer WOULD
86+
// have written, `r.deleted`, compiled and evaluated to `undefined`.
87+
expect(r.success).toBe(true);
88+
expect(r.object).toBe('task');
89+
expect(r.id).toBe('t1');
90+
91+
// `z.object` strips unknown keys, so a passing `safeParse` cannot by
92+
// itself prove no stray `deleted` came along (the #5641 caveat).
93+
// Assert the key set literally as well.
94+
expect(DeleteDataResponseSchema.safeParse(r).success).toBe(true);
95+
expect(Object.keys(r).sort()).toEqual(['id', 'object', 'success']);
96+
expect('deleted' in (r as object)).toBe(false);
97+
});
98+
99+
it('does not manufacture `success` — an off-spec body passes through verbatim', async () => {
100+
// Contract-first: the fix belongs in the type (and, for the server,
101+
// in #5581), NOT in a tolerant consumer that reads
102+
// `body.success ?? body.deleted`. If someone ever adds that alias,
103+
// this test is where it shows up.
104+
const { client } = createMockClient({ object: 'task', id: 't1', deleted: true });
105+
const r = await del(client) as unknown as Record<string, unknown>;
106+
107+
expect(r.success).toBeUndefined();
108+
expect(r.deleted).toBe(true);
109+
});
110+
});
111+
}
112+
});

‎packages/client/src/index.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,11 +232,21 @@ export interface UpdateDataResult<T = any> {
232232
droppedFields?: DroppedFieldsEvent[];
233233
}
234234

235-
/** Spec: DeleteDataResponseSchema */
235+
/**
236+
* Spec: DeleteDataResponseSchema
237+
*
238+
* [#5638] The success flag is `success`, matching the schema this comment
239+
* names (`packages/spec/src/api/protocol.zod.ts`). It was declared `deleted`
240+
* — a key no schema has ever declared and no server path has ever returned on
241+
* `/data/:object/:id`, so `r.deleted` compiled and read `undefined` at
242+
* runtime. Both `delete` surfaces below are pure `unwrapResponse` / `_unwrap`
243+
* passthroughs: this interface is a claim about the server's body, never a
244+
* rewrite of it, so the claim has to be the schema's.
245+
*/
236246
export interface DeleteDataResult {
237247
object: string;
238248
id: string;
239-
deleted: boolean;
249+
success: boolean;
240250
}
241251

242252
export interface StandardError {

0 commit comments

Comments
 (0)