diff --git a/.changeset/15117-action-engine-delete-id-array.md b/.changeset/15117-action-engine-delete-id-array.md new file mode 100644 index 0000000000..1e504d9e4e --- /dev/null +++ b/.changeset/15117-action-engine-delete-id-array.md @@ -0,0 +1,32 @@ +--- +'@objectstack/spec': minor +--- + +fix(spec): `ActionEngineFacade.delete` declares the id ARRAY the runtime has always accepted, and says which convention is the contract (#15117) + +`delete(object, id: string)` declared one id. The runtime facade +(`buildActionEngineFacade` in `packages/runtime`) has accepted `string | string[]` +all along — normalising the argument and issuing one `ql.delete` per id — and +described that in a comment as a tolerance two handler suites happened to cause. +The declaration was simply behind the behaviour, and the one first-party suite on +the array form could only reach it by hand-rolling a private copy of the +interface (a copy that had already drifted on `find`). + +The slot is now `delete(object: string, idOrIds: string | string[])`, and the +member's doc comment states the contract instead of leaving it to be inferred +from a runtime comment two packages away: + +- **Both spellings are contract.** One row is `delete(object, id)`; a set is + `delete(object, ids)` — a handler holding a list does not have to unroll it + into a loop to stay on the contract. +- **The array form is a convenience over the same per-row path** — not a bulk or + atomic delete. There is no transaction around the set: a failure part-way + leaves the ids before it deleted. An empty array deletes nothing and resolves. + +Nothing is removed and nothing narrows: every existing single-id call still +type-checks, and no runtime behaviour changes — this release makes the published +type describe what was already being served. That makes it non-breaking, not a +patch: widening a published parameter is a purely additive widening of a public +surface, which takes at least `minor` whatever the commit type says. Handler authors who copied the +facade into a local context type to reach the array form can delete the copy and +annotate with `ActionHandlerContext` / `ActionHandler` from `@objectstack/spec/ui`. diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index 9e35e501a1..4bac025a2d 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -137,15 +137,39 @@ For logic that belongs in real source files, point `target` at a handler name and register it in your config's `onEnable` lifecycle hook: ```typescript title="src/actions/task.handlers.ts" -export async function completeTask(ctx: ActionContext): Promise { - const { record, engine } = ctx; // ctx = { record, user, engine, params } +import type { ActionHandlerContext } from '@objectstack/spec/ui'; + +export async function completeTask(ctx: ActionHandlerContext): Promise { + const { record, engine } = ctx; // ctx = { record, params, user, session, engine } await engine.update('todo_task', record.id as string, { status: 'completed', - completed_date: new Date().toISOString(), }); } ``` +Annotate `ctx` with the **published** `ActionHandlerContext`, never a local copy +of it: a hand-rolled context interface drifts from the contract the moment either +side moves, and the drift is silent because the copy is what your handler is +checked against. (The contract's own docblock asks for `ActionHandler`; that is a +function type, so it fits an arrow-function handler assigned to a `const`, while +a function *declaration* like the one above annotates its parameter instead.) + +Write only the fields the action owns — and read that literally, because a +handler's `ctx.engine` runs **elevated** (`isSystem`). The read-only strip that +protects a server-owned column from an ordinary caller is gated on *not* being +a system write, so it does not run on a handler's write at all. **What a +handler names, lands.** A server-owned field is therefore more dangerous here +than in a form PUT, not less. + +`completed_date` is the example. It is `readonly` on `todo_task` — the server +owns it, and the object's `beforeUpdate` hook stamps it on the completion +**transition**. Adding `completed_date: new Date().toISOString()` to the write +above looks harmless, and on that transition it is: the hook overwrites it. But +a completion write that is *not* a transition — re-completing a task that is +already `completed` — is not stamped, so nothing overwrites the handler's value +and nothing strips it either, and the action silently replaces the real +completion timestamp with "now". That is why the snippet sends `status` alone. + **`ctx.engine.find(object, filter)` takes a filter, not a query.** The second argument is the `where` half only — `{ status: 'completed' }`, operators @@ -161,6 +185,24 @@ refuses a primitive or a mistyped `$and` / `$or` / `$not` but still admits double must honour it too. + +**`ctx.engine.delete(object, id)` takes one id — or an array of them.** Both +spellings are contract: `ActionEngineFacade` in `@objectstack/spec/ui` declares +the parameter `string | string[]`, so a handler that already holds a list does +not have to unroll it into a loop to stay on the contract. + +```typescript +await ctx.engine.delete('todo_task', record.id as string); // one row +await ctx.engine.delete('todo_task', ['tsk_0001', 'tsk_0002']); // a set +``` + +The array form is a **convenience over the same per-row path** — not a bulk and +not an atomic delete. The runtime issues one delete per id, in order, with no +transaction around the set: a failure part-way through leaves the ids before it +deleted and the ids after it untouched, and the rejection you see is the one that +stopped it. An empty array deletes nothing and resolves. + + ```typescript title="objectstack.config.ts" export const onEnable = async (ctx: { ql: { registerAction: (...args: unknown[]) => void } }) => { ctx.ql.registerAction('todo_task', 'completeTask', completeTask); diff --git a/examples/app-todo/src/actions/task.handlers.ts b/examples/app-todo/src/actions/task.handlers.ts index ea100b6ddf..4cca90c010 100644 --- a/examples/app-todo/src/actions/task.handlers.ts +++ b/examples/app-todo/src/actions/task.handlers.ts @@ -14,22 +14,30 @@ * ``` */ -// ─── Handler Context (simplified for example purposes) ────────────── -interface ActionContext { - /** The record being acted upon */ - record: Record; - /** Current authenticated user */ - user: { id: string; name: string }; - /** Data engine for CRUD operations */ - engine: { - update(object: string, id: string, data: Record): Promise; - insert(object: string, data: Record): Promise<{ id: string }>; - find(object: string, query: Record): Promise>>; - delete(object: string, ids: string[]): Promise; - }; - /** Action parameters (from user input / params) */ - params?: Record; -} +import type { ActionHandlerContext } from '@objectstack/spec/ui'; + +// ─── Handler Context ───────────────────────────────────── +// +// `ActionHandlerContext` (`@objectstack/spec/ui`) is the PUBLISHED contract for +// what an action body reads as `ctx` — `record`, `params`, `user`, `session` and +// the trusted `engine` facade. +// +// What the contract asks for in its own words is `ActionHandler`. That is a +// FUNCTION type, and the handlers below are function DECLARATIONS, which cannot +// carry one; rebinding them as `const cloneTask: ActionHandler = ...` would also +// erase their return types, because `ActionHandler` returns `unknown`. Annotating +// the ctx parameter with `ActionHandlerContext` — the type `ActionHandler` is +// defined in terms of — is the same contract, reached the way this file is +// written. +// +// This file used to declare a local simplified copy of that context instead, +// and the copy drifted: its `find` still took an ObjectQL-shaped `query` bag +// long after the contract had settled on a FILTER (#14175). It existed because +// `ActionEngineFacade.delete` was declared as a single id while the runtime had +// always accepted an id array too, so `deleteCompletedTasks` below could not be +// written against the published type at all. The declaration now says +// `string | string[]` (#15117), so the copy is gone and this example +// type-checks against exactly the types a real app gets. /** * Mark a single task as complete. @@ -41,7 +49,7 @@ interface ActionContext { * `beforeUpdate` leg of `src/objects/task.hook.ts`, which runs on the * transition and whose write the strip lets through. */ -export async function completeTask(ctx: ActionContext): Promise { +export async function completeTask(ctx: ActionHandlerContext): Promise { const { record, engine } = ctx; await engine.update('todo_task', record.id as string, { status: 'completed', @@ -49,7 +57,7 @@ export async function completeTask(ctx: ActionContext): Promise { } /** Mark a task as in-progress */ -export async function startTask(ctx: ActionContext): Promise { +export async function startTask(ctx: ActionHandlerContext): Promise { const { record, engine } = ctx; await engine.update('todo_task', record.id as string, { status: 'in_progress', @@ -57,7 +65,7 @@ export async function startTask(ctx: ActionContext): Promise { } /** Clone a task (duplicate with reset status) */ -export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> { +export async function cloneTask(ctx: ActionHandlerContext): Promise<{ id: string }> { const { record, engine } = ctx; const { id, created_at, updated_at, completed_date, ...fields } = record as Record; return engine.insert('todo_task', { @@ -68,7 +76,7 @@ export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> { } /** Mark all selected tasks as complete (bulk) — same `status`-only rule as {@link completeTask} (#7036) */ -export async function massCompleteTasks(ctx: ActionContext): Promise { +export async function massCompleteTasks(ctx: ActionHandlerContext): Promise { const { params, engine } = ctx; const ids = (params?.selectedIds ?? []) as string[]; for (const id of ids) { @@ -79,7 +87,7 @@ export async function massCompleteTasks(ctx: ActionContext): Promise { } /** Delete all completed tasks */ -export async function deleteCompletedTasks(ctx: ActionContext): Promise { +export async function deleteCompletedTasks(ctx: ActionHandlerContext): Promise { const { engine } = ctx; const completed = await engine.find('todo_task', { status: 'completed' }); const ids = completed.map((r) => r.id as string); @@ -89,7 +97,7 @@ export async function deleteCompletedTasks(ctx: ActionContext): Promise { } /** Defer a task by updating its due date (params collected by the action dialog) */ -export async function deferTask(ctx: ActionContext): Promise { +export async function deferTask(ctx: ActionHandlerContext): Promise { const { record, engine, params } = ctx; await engine.update('todo_task', record.id as string, { due_date: params?.new_due_date ? String(params.new_due_date) : null, @@ -99,7 +107,7 @@ export async function deferTask(ctx: ActionContext): Promise { } /** Set a reminder on a task (params collected by the action dialog) */ -export async function setReminder(ctx: ActionContext): Promise { +export async function setReminder(ctx: ActionHandlerContext): Promise { const { record, engine, params } = ctx; await engine.update('todo_task', record.id as string, { reminder_date: params?.reminder_date ? String(params.reminder_date) : null, @@ -108,7 +116,7 @@ export async function setReminder(ctx: ActionContext): Promise { } /** Export tasks to CSV format */ -export async function exportTasksToCSV(ctx: ActionContext): Promise { +export async function exportTasksToCSV(ctx: ActionHandlerContext): Promise { const { engine } = ctx; const tasks = await engine.find('todo_task', {}); const header = 'subject,status,priority,category,due_date'; diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 109403b7d9..f812c06158 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1466,8 +1466,8 @@ export function buildActionEngineFacade(_deps: ActionExecutionDeps, ql: any, ec? async update(object: string, id: string, data: Record): Promise { await ql.update(object, data, { where: { id }, context }); }, - // Tolerant of both the single-id and array conventions handler suites - // use (CRM handlers pass one id; todo handlers pass an id array). + // Both spellings are DECLARED contract (#15117), not a tolerance: the + // spec's `ActionEngineFacade.delete` takes `string | string[]`. async delete(object: string, idOrIds: string | string[]): Promise { const ids = Array.isArray(idOrIds) ? idOrIds : [idOrIds]; for (const id of ids) { diff --git a/packages/spec/src/ui/action-params.test.ts b/packages/spec/src/ui/action-params.test.ts index c524b3261f..8181e4c704 100644 --- a/packages/spec/src/ui/action-params.test.ts +++ b/packages/spec/src/ui/action-params.test.ts @@ -468,3 +468,58 @@ describe('#14175 — ActionEngineFacade.find takes a FILTER (the `where` half), expect('where' in envelope && 'where' in nested).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// #15117 — `ActionEngineFacade.delete` takes ONE id, or an ARRAY of ids +// --------------------------------------------------------------------------- + +// The declared slot, read off the interface — not a retyped copy of it, so a +// re-narrowing back to the single-id `string` this card retired fails HERE, +// rather than in the first handler that hands the facade a list. +type DeleteIds = Parameters[1]; + +// The type-level pin (the tsc channel, `tsc -p tsconfig.test.json`). `Eq` is +// the strict mutual-assignability test, so the pre-#15117 declaration +// (`string` alone) does not satisfy it — neither does a slot widened all the +// way to `unknown`. Exported for the same reason the sibling pins are. +export type DeleteIdsAcceptsOneOrMany = Assert< Eq< DeleteIds, string | string[] > >; + +describe('#15117 — ActionEngineFacade.delete accepts one id or an array, both as contract', () => { + it('types the second parameter as `string | string[]` (the tsc channel)', () => { + // The value-level half of `DeleteIdsAcceptsOneOrMany` above: literals + // annotated with the slot type, so the runtime run exercises the same + // declaration the type pin reads. + const one: DeleteIds = 'tsk_0001'; + const many: DeleteIds = ['tsk_0001', 'tsk_0002']; + + expect([typeof one, Array.isArray(many)]).toEqual(['string', true]); + }); + + it('positive control — both handler conventions compile, and so does the empty set', () => { + // The single-id convention (the CRM handler suites). + const singleId: DeleteIds = 'tsk_0001'; + // The array convention (`examples/app-todo`'s `deleteCompletedTasks`), + // which before this card was reachable only through a hand-rolled copy of + // `ActionEngineFacade` — the workaround the widening retired. + const idArray: DeleteIds = ['tsk_0001', 'tsk_0002', 'tsk_0003']; + // An empty list deletes nothing and resolves; the member doc says so. + const empty: DeleteIds = []; + + expect([singleId, idArray, empty].length).toBe(3); + }); + + it('refuses at compile time what neither convention admits', () => { + // Each `@ts-expect-error` is itself checked: if the slot ever ADMITS one + // of these, the directive goes unused and `tsc -p tsconfig.test.json` reds. + // @ts-expect-error — an id is a string; a number is not an id under either convention. + const numericId: DeleteIds = 42; + // @ts-expect-error — the array form is an array of ids, not of numbers. + const numericIds: DeleteIds = [1, 2]; + // @ts-expect-error — one level of array, never a nested one. + const nestedIds: DeleteIds = [['tsk_0001']]; + // @ts-expect-error — "delete nothing" is the EMPTY ARRAY, never a null id. + const nullId: DeleteIds = null; + + expect([numericId, numericIds, nestedIds, nullId]).toHaveLength(4); + }); +}); diff --git a/packages/spec/src/ui/action-params.zod.ts b/packages/spec/src/ui/action-params.zod.ts index dd75f9a948..fccd803f35 100644 --- a/packages/spec/src/ui/action-params.zod.ts +++ b/packages/spec/src/ui/action-params.zod.ts @@ -231,15 +231,49 @@ export function validateActionParams( * context-less, RLS/FLS-bypassing by design (#2849); the boundary is enforced * at invoke time (`ai.exposed` + the ADR-0066 D4 capability gate), not here. * - * `find` is the one member whose argument shape the signature alone never - * settled: it takes a bare FILTER — the `where` half of a query — and never - * an ObjectQL query envelope; read its doc comment before writing a handler - * or a test double against it (#14175). + * Two members carry an argument contract the signature alone does not settle, + * and both state it on the member: `find` takes a bare FILTER — the `where` + * half of a query — and never an ObjectQL query envelope (#14175); `delete` + * accepts a single id OR an array of them, both as declared contract, served + * one row at a time (#15117). Read those doc comments before writing a handler + * or a test double against either. */ export interface ActionEngineFacade { insert(object: string, data: Record): Promise<{ id: string }>; update(object: string, id: string, data: Record): Promise; - delete(object: string, id: string): Promise; + /** + * Delete rows of `object` by id. + * + * ## Which convention is the contract: BOTH — one id, or an array of them + * + * `idOrIds` takes a SINGLE id or an ARRAY of ids, and both spellings are + * contract, not a runtime tolerance a handler author has to discover by + * reading `packages/runtime`. Deleting one row is `delete(object, id)`; + * deleting a set is `delete(object, ids)` — a handler that already holds a + * list does NOT have to unroll it into a loop to stay on the contract. + * + * What the array form is, exactly: a convenience over the SAME per-row path, + * never a bulk or atomic delete. The `delete` arm of + * `packages/runtime/src/action-execution.ts#buildActionEngineFacade` + * normalises the argument to a list and issues one `ql.delete` per id, in + * order, under the caller's execution context. There is no transaction + * around the set: a failure part-way through leaves the ids before it + * deleted and the ids after it untouched, and the rejection a caller sees is + * the one that stopped it. An empty array deletes nothing and resolves. + * + * Until #15117 this slot declared `id: string` while the runtime had been + * accepting both forms all along, described in a comment there as an + * accident of the two handler suites that happened to use them. It was + * neither: it is this declaration. The one first-party suite on the array + * form (`examples/app-todo/src/actions/task.handlers.ts`) could only reach + * it by hand-rolling a copy of this interface — the same hand-written-fake + * pattern #14175 found on `find`, one member over — and that copy drifted, + * exactly as a copy does. Widening the declaration is what retired it. + * + * Both accepted forms, and the argument types that stay refused, are pinned + * in `action-params.test.ts`. + */ + delete(object: string, idOrIds: string | string[]): Promise; /** * Read the rows of `object` that match `filter`. *