diff --git a/.changeset/17620-action-engine-delete-nullish-id.md b/.changeset/17620-action-engine-delete-nullish-id.md new file mode 100644 index 0000000000..f02e242349 --- /dev/null +++ b/.changeset/17620-action-engine-delete-nullish-id.md @@ -0,0 +1,39 @@ +--- +'@objectstack/runtime': patch +--- + +`ActionEngineFacade.delete` refuses a nullish id instead of silently skipping it + +**Who this is for: untyped hosts.** A JS host, or a `registerAction` handler +whose context slot is still `(ctx: any)`, can hand `ctx.engine.delete()` a +nullish id — `delete('todo_task', null)`, or an array with a hole in it. Until +now the arm dropped that element on the floor: nothing refused it, nothing +warned, and the call **resolved as though the row had been deleted**. A silent +no-op on a destructive verb is the one failure an untyped caller has no way to +detect, which is why it is worth a line in your changelog rather than a shrug. + +**What changes.** Every id now reaches the engine as written, and the engine's +own delete-dispatch predicate refuses a `where.id` that is not a truthy scalar: +the call rejects with `Delete requires an ID or options.multi=true` where it +used to resolve in silence. In the array form the refusal stops the loop where +the declared member doc already said a failure stops it — ids before the +nullish element are deleted, ids after it are untouched. + +**If a host was leaning on the old behaviour**, filter before you call: + +```js +const ids = candidates.filter((id) => id != null); +if (ids.length > 0) await ctx.engine.delete('todo_task', ids); +// `delete nothing` is the EMPTY ARRAY (it resolves, deleting nothing) — +// never a null id. An empty array is contract; a nullish id never was. +``` + +⛔ **No declaration moves, and this is not a correction of the `string | string[]` +widening that shipped just before it.** That declaration is accurate: it takes a +single id or an array of them, and under it **no typed caller could ever reach +the skipped branch** — the accept set it publishes has never admitted nullish. +The array form, its per-row semantics, its ordering and its empty-array case are +all unchanged and pinned as controls. What moves is only the runtime's +undeclared tolerance for a value three separate statements already excluded: the +published type, the member's own doc comment, and the spec-side pin that reads +«"delete nothing" is the EMPTY ARRAY, never a null id». diff --git a/packages/runtime/src/action-engine-facade-nullish-id.test.ts b/packages/runtime/src/action-engine-facade-nullish-id.test.ts new file mode 100644 index 0000000000..dce5d2cf8a --- /dev/null +++ b/packages/runtime/src/action-engine-facade-nullish-id.test.ts @@ -0,0 +1,154 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17620] `ActionEngineFacade.delete` no longer swallows a NULLISH element. + * + * ## What was here, and who could reach it + * + * The `delete` arm of {@link buildActionEngineFacade} normalises its argument + * to a list and issues one `ql.delete` per id. It used to open that loop with + * `if (id != null)`, so a nullish element was **silently skipped**: nothing + * refused it, nothing warned, and the call resolved as though the deletion had + * happened — a silent no-op on a destructive verb, which is the one failure a + * caller cannot detect. + * + * The declared type is `string | string[]` (#15117), so **no typed caller ever + * reached the guard** — the population is UNTYPED hosts: a JS host, or a + * `registerAction` handler whose slot is still `(ctx: any)`. That is also why + * this file is not a correction of #15117 / PR #17608: that card's contract + * sentence is true, and this arm's declared behaviour is unchanged by the + * removal. + * + * ## Why removing the guard is enough to make it loud + * + * Every id now reaches `ql.delete(object, { where: { id }, context })` as + * written, and the engine's own dispatch predicate answers that call: a + * `where.id` that is not a TRUTHY SCALAR is neither `by-id` nor (absent + * `multi`) a bulk intent, so `ObjectQL.delete` throws + * {@link ENGINE_DELETE_REJECT_MESSAGE}. The refusal is the producer's, not a + * second copy of it — which is why the double below opens with + * {@link assertEngineDeleteDispatch} rather than a hand-rolled id check: a + * double looser than the engine would keep this file green against a facade + * that still swallowed the value. + * + * ⚠️ The pin is the exported MESSAGE CONSTANT, compared exactly. This refusal + * is a plain `Error` — it carries no ADR-0112 `code`/`status` — so a bare + * `toThrow()` here would stay green against any unnamed `Error` at all, which + * is precisely what an unfixed arm would have to produce to be believed. + * + * @see packages/runtime/src/action-execution.ts — `buildActionEngineFacade`. + * @see packages/spec/src/ui/action-params.test.ts — the declaration's own pin, + * whose `@ts-expect-error` reads «"delete nothing" is the EMPTY ARRAY, + * never a null id». + */ + +import { describe, it, expect } from 'vitest'; +import { ENGINE_DELETE_REJECT_MESSAGE, assertEngineDeleteDispatch } from '@objectstack/metadata-core'; +import { buildActionEngineFacade } from './action-execution.js'; + +const deps: any = { resolveService: () => undefined, getObjectQL: async () => undefined }; + +/** + * An engine double whose `delete` is bound to the REAL engine's dispatch + * contract: one call to the producer's own predicate, never a mirrored `if`. + * Everything it accepts, a running server accepts; everything it refuses, a + * running server refuses (`scripts/check-engine-double-contract.mjs`). + */ +function makeEngine() { + const deleted: Array<{ object: string; id: unknown; context: unknown }> = []; + const ql: any = { + deleted, + async insert(_object: string, data: Record) { + return { id: (data as Record)?.id ?? 'rec_new' }; + }, + async find(_object: string, _options?: Record) { + return []; + }, + async count(_object: string, _options?: Record) { + return 0; + }, + async delete(object: string, options?: Record) { + assertEngineDeleteDispatch(options); + const where = (options as { where?: Record } | undefined)?.where; + deleted.push({ object, id: where?.id, context: (options as { context?: unknown } | undefined)?.context }); + return { ok: true }; + }, + }; + return ql; +} + +/** Drive the arm and hand back whatever it rejected with, or `undefined`. */ +async function rejection(run: Promise): Promise { + return run.then(() => undefined, (e: unknown) => e); +} + +describe('#17620 — ActionEngineFacade.delete refuses a nullish id', () => { + it('refuses a nullish ELEMENT of the array form instead of skipping it', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + const err = await rejection(engine.delete('crm_case', [null])); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + // …and it is loud INSTEAD of deleting, not as well as: nothing landed. + expect(ql.deleted).toEqual([]); + }); + + it('refuses a nullish SINGLE id (the non-array spelling) the same way', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + for (const nullish of [null, undefined]) { + const err = await rejection(engine.delete('crm_case', nullish)); + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + } + expect(ql.deleted).toEqual([]); + }); + + it('stops AT the nullish element — ids before it are deleted, ids after it untouched', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + const err = await rejection(engine.delete('crm_case', ['case_1', null, 'case_3'])); + + expect((err as Error).message).toBe(ENGINE_DELETE_REJECT_MESSAGE); + // The declared partial-progress shape, unchanged: "a failure part-way + // through leaves the ids before it deleted and the ids after it + // untouched" (`ActionEngineFacade.delete`'s member doc). + expect(ql.deleted.map((d: { id: unknown }) => d.id)).toEqual(['case_1']); + }); +}); + +describe('#17620 — controls: the declared contract is untouched', () => { + it('a well-formed single id still deletes', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1', tenantId: 'org_acme' }); + + await expect(engine.delete('crm_case', 'case_1')).resolves.toBeUndefined(); + + expect(ql.deleted).toHaveLength(1); + expect(ql.deleted[0]).toMatchObject({ object: 'crm_case', id: 'case_1' }); + // the elevated caller envelope still rides every call (#3914) + expect(ql.deleted[0].context).toMatchObject({ isSystem: true, userId: 'u1', tenantId: 'org_acme' }); + }); + + it('the declared ARRAY form still deletes every id, in order, one call each', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + await expect(engine.delete('crm_case', ['case_1', 'case_2', 'case_3'])).resolves.toBeUndefined(); + + expect(ql.deleted.map((d: { id: unknown }) => d.id)).toEqual(['case_1', 'case_2', 'case_3']); + }); + + it('an empty array still deletes nothing and resolves', async () => { + const ql = makeEngine(); + const engine = buildActionEngineFacade(deps, ql, { userId: 'u1' }); + + await expect(engine.delete('crm_case', [])).resolves.toBeUndefined(); + + expect(ql.deleted).toEqual([]); + }); +}); diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index f812c06158..1bd873a79a 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1468,10 +1468,25 @@ export function buildActionEngineFacade(_deps: ActionExecutionDeps, ql: any, ec? }, // Both spellings are DECLARED contract (#15117), not a tolerance: the // spec's `ActionEngineFacade.delete` takes `string | string[]`. + // + // [#17620] And there is no third, undeclared one. This loop used to + // open with `if (id != null)`, so a NULLISH element was silently + // skipped and the call resolved as though the deletion had happened — + // a silent no-op on a destructive verb. The declared type excludes + // nullish, so no typed caller ever reached it; the population was + // UNTYPED hosts (a JS host, a `registerAction` handler whose slot is + // still `(ctx: any)`), which is exactly the population that cannot see + // the loss. Every id now goes to `ql.delete` as written, where the + // engine's own dispatch predicate refuses a `where.id` that is not a + // truthy scalar (`ENGINE_DELETE_REJECT_MESSAGE`) — the loud answer the + // declaration already implied. Removing the guard declares nothing new: + // it pulls the runtime back onto the contract that is already on the + // record, here, in the spec member doc, and in the `never a null id` + // pin at `packages/spec/src/ui/action-params.test.ts`. async delete(object: string, idOrIds: string | string[]): Promise { const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds]; for (const id of ids) { - if (id != null) await ql.delete(object, { where: { id }, context }); + await ql.delete(object, { where: { id }, context }); } }, async find(object: string, query: Record): Promise>> { diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index b70be27081..b888922a21 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -3356,6 +3356,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/runtime/src/action-engine-facade-nullish-id.test.ts", + "verb": "delete", + "pinned": 1 + }, { "file": "packages/runtime/src/action-execution-calldata-batch-retired.test.ts", "verb": "findOne",