diff --git a/.changeset/client-delete-result-success.md b/.changeset/client-delete-result-success.md new file mode 100644 index 0000000000..296523c501 --- /dev/null +++ b/.changeset/client-delete-result-success.md @@ -0,0 +1,98 @@ +--- +"@objectstack/client": major +"@objectstack/cli": patch +--- + +fix(client)!: `DeleteDataResult` declares the schema it names — `success`, not `deleted` (#5638) + +`DeleteDataResult` — the return type of `client.data.delete()` and of the +project-scoped `client.project(id).data.delete()` — carried the comment +`Spec: DeleteDataResponseSchema` above a declaration that contradicted it: + +```ts +/** Spec: DeleteDataResponseSchema */ +export interface DeleteDataResult { + object: string; + id: string; + deleted: boolean; // ← the schema declares `success` +} +``` + +`DeleteDataResponseSchema` (`packages/spec/src/api/protocol.zod.ts`) declares +`{ object, id, success }`. `deleted` has never been declared by any schema, and +no server path has ever returned it on `/data/:object/:id`. + +**The old key was never readable at runtime — this rename reveals a defect, it +does not break working code.** Both `delete` surfaces are pure `unwrapResponse` +/ `_unwrap` passthroughs: the SDK returns the server's body untouched, so this +interface is a *claim* about the wire, never a rewrite of it. The claim was +false in the one direction that matters — the compiler endorsed the wrong +spelling: + +```ts +const r = await client.data.delete('task', id); +if (r.deleted) { … } // compiled; `undefined` at runtime; branch never taken +if (r.success) { … } // rejected by the compiler; correct on the wire +``` + +## What to change + +`r.deleted` → `r.success`. That is the whole migration. Nothing about the +request, the route, the status codes or the error shapes changes, and no server +needs upgrading: the value you are now allowed to read is the one that was +already arriving. + +⛔ **Do not write `r.success ?? r.deleted`.** There is one producer shape, and a +consumer that accepts two spellings is the shape contract-first exists to +prevent — the same ruling #5581 applied on the producer side. No deprecated +`deleted?: boolean` transition key ships for the same reason; a transition +period is for keys that *worked*, and this one never did. + +## Why the type was wrong on every deployment, not just some + +The protocol path (`deleteData`) has always answered `success`. #5581 / PR +#5641 brought the ObjectQL fallback — the path a slim assembly without +`MetadataPlugin` takes — to the same shape. So before that fix the declaration +was wrong on ordinary deployments and accidentally right on slim ones; after +it, both paths answer `{ object, id, success }` and the declaration was simply +wrong everywhere. The consumer-side correction had to follow the producer, not +lead it. + +## `os data delete` was reading the phantom key too + +`packages/cli/src/commands/data/delete.ts` built its `--format json` / `--format +yaml` payload with `deleted: result.deleted`. That evaluated to `undefined`, and +`JSON.stringify` drops undefined values — so the `deleted` key the command has +always declared **never appeared in a single run**. It now carries +`result.success`, the server's own verdict. + +Observable change: `os data delete --format json` gains `deleted: true` (YAML +likewise) on a successful delete. The key name stays `deleted` deliberately — +it is the CLI's output key, not the protocol's, and the payload's top-level +`success` already means something different (the CLI envelope's "the command +completed"). Conflating the two is the hazard #5641 called out when it noted +that `body.success` and `body.data.success` are different facts. Scripts +reading `.deleted` from this command were reading `undefined` before and get a +boolean now; nothing that worked stops working. + +## Downstream + +`objectui`'s `ObjectStackDataSource.delete()` is a live victim of the old +declaration — it guards `emitMutation` on `result.deleted`, so the delete +mutation event has never fired against a real server and the method returns +`undefined` where it declares `boolean`. Its own suite stayed green because the +fixture mocks `{ deleted: true }`, a body no server produces. Filed as +objectstack-ai/objectui#3412, which is blocked on this package publishing — +its fix is a type unblock, not a behaviour change, since `success` is already +what arrives. + +## Pins + +`packages/client/src/data-delete-result-shape.test.ts` asserts mutual +assignability between `DeleteDataResult` and the spec's `DeleteDataResponse`, +so a rename on either side (or a re-added optional `deleted`) fails +`check:test-typecheck`. `client.hono.test.ts` gains the delete case this live +server suite never had: a real DELETE over HTTP whose body is read as +`deleted.success` and whose key set is asserted literally — `z.object` strips +unknown keys, so a passing parse alone cannot prove no stray `deleted` rode +along. diff --git a/content/docs/kernel/runtime-services/data-service.mdx b/content/docs/kernel/runtime-services/data-service.mdx index 93a6e8d2b5..fe212b04e7 100644 --- a/content/docs/kernel/runtime-services/data-service.mdx +++ b/content/docs/kernel/runtime-services/data-service.mdx @@ -30,7 +30,8 @@ services.data.delete(object: string, id: string): Promise - `get`: single record payload - `find`: list payload + pagination metadata - `create`/`update`: mutated record payload -- `delete`: success marker / deleted ID payload +- `delete`: `{ object, id, success }` — the spec's `DeleteDataResponse`. The + flag is `success`, not `deleted` (#5638) ## Typical Errors diff --git a/packages/cli/src/commands/data/delete.ts b/packages/cli/src/commands/data/delete.ts index 15a94fedde..8a0086653f 100644 --- a/packages/cli/src/commands/data/delete.ts +++ b/packages/cli/src/commands/data/delete.ts @@ -57,15 +57,24 @@ export default class DataDelete extends Command { // Delete the record const result = await client.data.delete(args.object, args.id); + // [#5638] `deleted` is THIS COMMAND's output key; its value is the + // protocol's `DeleteDataResponse.success`. Two different booleans live + // in this payload and must not be conflated: the top-level `success` is + // the CLI envelope's "the command completed" flag (this branch is only + // reached when it did), while the server's flag is its statement that + // the deletion happened. Until now this read was `result.deleted` — a + // key no server has ever returned — so it evaluated to `undefined` and + // `JSON.stringify` dropped it: the documented key was simply absent + // from every `os data delete --format json` run. if (flags.format === 'json') { await emitJson({ success: true, object: result.object, id: result.id, - deleted: result.deleted, + deleted: result.success, }); } else if (flags.format === 'yaml') { - await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.deleted }, 'yaml'); + await formatOutput({ success: true, object: result.object, id: result.id, deleted: result.success }, 'yaml'); } else { printSuccess(`Record deleted: ${result.id}`); } diff --git a/packages/client/src/client.hono.test.ts b/packages/client/src/client.hono.test.ts index ab267c82a7..f85abf7f89 100644 --- a/packages/client/src/client.hono.test.ts +++ b/packages/client/src/client.hono.test.ts @@ -200,4 +200,37 @@ describe('ObjectStackClient (with Hono Server)', () => { expect(resultsResponse.records.length).toBeGreaterThan(0); expect(resultsResponse.records[0].name).toBe('Hono User'); }); + + // [#5638] The one method whose declared return shape this suite never + // exercised — and the one that was wrong. `DeleteDataResult` claimed + // `deleted: boolean`; the schema it names declares `success`. This is the + // runtime half of the pin: a REAL delete, over HTTP, against the server + // this package's consumers talk to, read through the declared type. + // + // Nothing in this test is mocked into the answer: the DELETE route calls + // `protocol.deleteData` (rest-server.ts), NOT the broker shim above, so the + // body asserted here is the server's own. + it('should delete data via hono, answering the SPEC\'s `success` body', async () => { + const client = new ObjectStackClient({ baseUrl }); + await client.connect(); + + const created = await client.data.create('customer', { + name: 'Doomed User', + email: 'doomed@example.com', + }); + + // Spec: DeleteDataResponse = { object, id, success } + const deleted = await client.data.delete('customer', created.id); + expect(deleted.success).toBe(true); + expect(deleted.object).toBe('customer'); + expect(deleted.id).toBe(created.id); + // The undeclared key must not ride along (a passing schema parse would + // strip it silently, so assert the key set itself). + expect(Object.keys(deleted).sort()).toEqual(['id', 'object', 'success']); + + // And the delete actually happened — a success flag nobody cross-checks + // is the cheapest thing in the world to keep green. + const remaining = await client.data.find('customer', { where: { id: created.id } }); + expect(remaining.records.length).toBe(0); + }); }); diff --git a/packages/client/src/data-delete-result-shape.test.ts b/packages/client/src/data-delete-result-shape.test.ts new file mode 100644 index 0000000000..525db91d69 --- /dev/null +++ b/packages/client/src/data-delete-result-shape.test.ts @@ -0,0 +1,112 @@ +/** + * [#5638] `DeleteDataResult` is a CLAIM about the server's delete body, and it + * has to be the claim its own comment makes: `Spec: DeleteDataResponseSchema`. + * + * It declared `{ object, id, deleted: boolean }` while + * `DeleteDataResponseSchema` declares `{ object, id, success: boolean }`. Both + * `delete` surfaces (`client.data.delete` and the project-scoped + * `client.project(id).data.delete`) are pure `unwrapResponse` / `_unwrap` + * passthroughs — no runtime rewriting anywhere — so a consumer writing + * `r.deleted` compiled fine and read `undefined` against every server path + * that exists (#5581 / PR #5641 brought the ObjectQL fallback to `success` + * too, so both producer paths now answer the same shape). + * + * WHICH LAYER ACTUALLY MOVES, and why that is not the usual one: the fix is a + * type declaration, and types are erased before vitest ever runs. Reverting + * `success` back to `deleted` therefore leaves every runtime assertion below + * GREEN — they assert on a body the mock/server produced, which the client + * never touched either way. The assertion that goes red on revert is the + * type-level one (`SpecAndSdkAgree`), plus the `r.success` reads, and both are + * enforced by `pnpm --filter @objectstack/client typecheck` → `check:test-typecheck` + * (#5449/#5546), which compiles this file with the package's full strictness + * and no debt-ledger entry. The runtime assertions below are shape pins: they + * pin that the passthrough stays a passthrough, which is what makes the type + * the only thing standing between a consumer and the wire. + */ +import { describe, it, expect, vi } from 'vitest'; +import { DeleteDataResponseSchema, type DeleteDataResponse } from '@objectstack/spec/api'; +import { ObjectStackClient, type DeleteDataResult } from './index'; + +/** Mutual assignability — `A extends B` alone would pass on a strict subset. */ +type Exact = [A] extends [B] ? ([B] extends [A] ? true : false) : false; + +/** + * The pin. `DeleteDataResult` must be structurally the spec type, field for + * field: rename either side, add a key to one, or make one optional, and this + * stops being `true` — a type error at `check:test-typecheck`, not a runtime + * failure. (This is deliberately an equality, not `extends`: the old `deleted` + * declaration was neither a subset nor a superset of the schema, and a + * one-directional check would still have caught it — but a future + * `success: boolean; deleted?: boolean` "transition" shape would slip past + * one, and that shape is exactly what #5638's triage ruled out.) + */ +const SpecAndSdkAgree: Exact = true; + +/** Helper: a client whose fetch answers `body` with `status`. */ +function createMockClient(body: any, status = 200) { + const fetchMock = vi.fn().mockResolvedValue({ + ok: status >= 200 && status < 300, + status, + statusText: status === 200 ? 'OK' : 'Error', + json: async () => body, + headers: new Headers(), + }); + const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock }); + return { client, fetchMock }; +} + +/** The body every server path answers a successful delete with (#5581). */ +const SPEC_BODY = { object: 'task', id: 't1', success: true }; + +describe('[#5638] DeleteDataResult === DeleteDataResponseSchema', () => { + it('declares the schema\'s shape, field for field (type-level)', () => { + // Reading the binding is what keeps `noUnusedLocals` satisfied; the + // assertion that matters already happened in tsc, above. + expect(SpecAndSdkAgree).toBe(true); + }); + + it('the schema still declares `success` and has never declared `deleted`', () => { + const parsed = DeleteDataResponseSchema.safeParse(SPEC_BODY); + expect(parsed.success).toBe(true); + expect(DeleteDataResponseSchema.safeParse({ object: 'task', id: 't1', deleted: true }).success) + .toBe(false); + }); + + for (const [surface, del] of [ + ['client.data.delete', (c: ObjectStackClient) => c.data.delete('task', 't1')], + ['client.project(id).data.delete', (c: ObjectStackClient) => c.project('proj-xyz').data.delete('task', 't1')], + ] as const) { + describe(surface, () => { + it('hands back the spec body, and `success` is reachable through the declared type', async () => { + const { client } = createMockClient(SPEC_BODY); + const r: DeleteDataResult = await del(client); + + // The typed read. Under the pre-#5638 declaration this line is + // TS2339 (`success` does not exist) — and the line a consumer WOULD + // have written, `r.deleted`, compiled and evaluated to `undefined`. + expect(r.success).toBe(true); + expect(r.object).toBe('task'); + expect(r.id).toBe('t1'); + + // `z.object` strips unknown keys, so a passing `safeParse` cannot by + // itself prove no stray `deleted` came along (the #5641 caveat). + // Assert the key set literally as well. + expect(DeleteDataResponseSchema.safeParse(r).success).toBe(true); + expect(Object.keys(r).sort()).toEqual(['id', 'object', 'success']); + expect('deleted' in (r as object)).toBe(false); + }); + + it('does not manufacture `success` — an off-spec body passes through verbatim', async () => { + // Contract-first: the fix belongs in the type (and, for the server, + // in #5581), NOT in a tolerant consumer that reads + // `body.success ?? body.deleted`. If someone ever adds that alias, + // this test is where it shows up. + const { client } = createMockClient({ object: 'task', id: 't1', deleted: true }); + const r = await del(client) as unknown as Record; + + expect(r.success).toBeUndefined(); + expect(r.deleted).toBe(true); + }); + }); + } +}); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 579a8ed4e3..cbfc750e6e 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -232,11 +232,21 @@ export interface UpdateDataResult { droppedFields?: DroppedFieldsEvent[]; } -/** Spec: DeleteDataResponseSchema */ +/** + * Spec: DeleteDataResponseSchema + * + * [#5638] The success flag is `success`, matching the schema this comment + * names (`packages/spec/src/api/protocol.zod.ts`). It was declared `deleted` + * — a key no schema has ever declared and no server path has ever returned on + * `/data/:object/:id`, so `r.deleted` compiled and read `undefined` at + * runtime. Both `delete` surfaces below are pure `unwrapResponse` / `_unwrap` + * passthroughs: this interface is a claim about the server's body, never a + * rewrite of it, so the claim has to be the schema's. + */ export interface DeleteDataResult { object: string; id: string; - deleted: boolean; + success: boolean; } export interface StandardError {