From 9ea03d86233532016ccb1febf20e77ebf39a8b7e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 17:33:23 +0000 Subject: [PATCH] fix(objectql,metadata-core): refuse a by-id update whose non-scalar where.id predicate stands beside the payload id (#11230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update(obj, { id: 'rec_1', title: 'x' }, { where: { id: { $in: ['a','b'] } }, multi: true }) used to write exactly one row — rec_1 — with no diagnostic: the payload id outranked `where` and `multi` alike, so the declared row SET and the declared bulk intent were BOTH discarded, and rec_1 need not even have been a member of the set. The last silent member of the dropped-declaration family (#5748 payload operator-objects, #11009 extra where keys, #11142 unequal scalar where.id). Closing it reverses the REMAINING half of the #5748-pinned verdict 'a SCALAR data.id still outranks where and multi', which the maintainer ruling on #11230 (2026-08-23) authorizes. resolveEngineUpdateDispatch now rejects the shape with a message naming the payload id and the KIND of predicate the caller wrote, carrying the SAME ADR-0112 envelope #11142 declares — UPDATE_ID_MISMATCH + status 400. One ledger member for one defect class, two messages: the two shapes share a remedy (drop one of the two row-address spellings), so a second registered code would buy no consumer a branch it could act on, while growing the closed `code` vocabulary every AI-authored consumer must switch over (ADR-0112 D3/D4). No new error code is registered; the ledger's existing entry is widened in place. Pin-reversal discipline: both pins asserting the old verdict are INVERTED in place, never deleted and never relaxed — 'a SCALAR data.id still outranks where and multi' (its first expectation was exactly this shape) and 'a NON-SCALAR where.id keeps its #5748 verdict', the boundary #11142 deliberately left standing. The boundaries that survive gain their own pins: falsy scalar where.id, a `where` declaring no id, and the no-payload-id ladder. Fixes #11230 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y --- .../where-id-predicate-conflict-refusal.md | 18 ++ .../src/engine-update-dispatch.ts | 195 ++++++++++++++-- packages/metadata-protocol/src/protocol.ts | 19 +- .../src/protocol.update-path-id-wins.test.ts | 18 +- .../src/engine-update-dispatch.test.ts | 208 +++++++++++++++++- .../objectql/src/engine-update-dispatch.ts | 10 +- packages/objectql/src/engine.ts | 5 +- .../spec/src/api/error-code-ledger.zod.ts | 16 +- 8 files changed, 437 insertions(+), 52 deletions(-) create mode 100644 .changeset/where-id-predicate-conflict-refusal.md diff --git a/.changeset/where-id-predicate-conflict-refusal.md b/.changeset/where-id-predicate-conflict-refusal.md new file mode 100644 index 0000000000..130c9ce5ac --- /dev/null +++ b/.changeset/where-id-predicate-conflict-refusal.md @@ -0,0 +1,18 @@ +--- +"@objectstack/metadata-core": minor +"@objectstack/objectql": minor +--- + +**BREAKING (accept-set tightening)**: a by-id `update` whose bound truthy scalar payload `data.id` stands beside a DECLARED but non-scalar `options.where.id` — `{ $in: [...] }`, an array, `null` — is now refused loudly (`UPDATE_ID_MISMATCH`, HTTP 400) instead of silently binding the payload row and discarding both the id predicate and any declared `multi: true` (#11230). + +`update(obj, { id: 'rec_1', title: 'x' }, { where: { id: { $in: ['a', 'b'] } }, multi: true })` used to write exactly one row — `rec_1` — with no diagnostic: the payload id outranked `where` and `multi` alike (#5748), so the declared row SET and the declared bulk intent were both dropped, and `rec_1` need not even have been a member of the set. This was the LAST silent member of the dropped-declaration family (#5748 payload operator-objects, #11009 extra `where` keys, #11142 unequal scalar `where.id`); closing it reverses the remaining half of the #5748-pinned verdict `a SCALAR data.id still outranks where and multi`, which the maintainer ruling on #11230 (2026-08-23) authorizes. + +What changes, per call shape (`resolveEngineUpdateDispatch`, so every pinned test double inherits the same verdict): + +- A truthy scalar `data.id` beside a **non-scalar** `where.id` — an operator object, an array, `null`, or an explicitly-`undefined` `id` key — now **throws** `UPDATE_ID_MISMATCH` with `status: 400`, naming the payload id and the KIND of predicate the caller wrote. `multi: true` does not rescue the call (the payload id outranks `multi` per #5748, so the contradiction stands). Previously the write landed on the payload row with both declarations silently ignored. +- Boundaries that do **not** move: a **falsy** scalar `where.id` (`0`, `''`) is a scalar and keeps its #11142 verdict (by-id); a `where` that declares **no** `id` key at all (`{}`, or no `where`) is untouched; and with **no** scalar payload id the ladder is exactly as #5748 left it (`multi` when declared, otherwise `reject`) — the refusal lives only on the payload-sourced by-id arm. +- The refusal shares the #11142 error code deliberately — one ADR-0112 ledger member for one defect class, two messages. No new code is registered. + +A caller hitting the new refusal declared a row address and a row-set predicate in one call and meant one of them; each fix is a one-line edit at the call site: drop `id` from the payload to write EVERY row the predicate selects (`update(object, fields, { where: { id: { $in: [...] } }, multi: true })`), or drop `where.id` to write the single row the payload names (`update(object, { id, ...fields })`). The refusal text names both. Measured before shipping: **no in-repo call site constructs the pair** — every production `where.id` predicate (the outbox sweeps) carries a payload with no `id` — so the in-repo blast radius is nil; an external SDK caller can still write it, and today that silently drops both declarations. + + diff --git a/packages/metadata-core/src/engine-update-dispatch.ts b/packages/metadata-core/src/engine-update-dispatch.ts index 384067f187..2a10f964b0 100644 --- a/packages/metadata-core/src/engine-update-dispatch.ts +++ b/packages/metadata-core/src/engine-update-dispatch.ts @@ -87,9 +87,30 @@ * never identifies a row anywhere on this ladder (point 3 below), so there * is no second row address to disagree with. * - A NON-scalar `where.id` (`{ $in: [...] }`, an array, `null`) beside a - * scalar payload id keeps its #5748 verdict (`by-id`, payload wins) — that - * pin was not reversed, and widening over it is a separate decision, not a - * rider here (#6435's own boundary, one shape over). + * scalar payload id kept its #5748 verdict here (`by-id`, payload wins) — + * #11142 deliberately did not reach it. #11230 does; see the next clause. + * + * **[#11230] A third overriding clause, on the same PAYLOAD-sourced by-id + * arm:** a `where.id` that is DECLARED but is not a scalar id at all + * (`{ $in: [...] }`, an array, `null`) beside a bound scalar payload id is + * refused too — the same `UPDATE_ID_MISMATCH` / 400 envelope, because it is + * the same defect one step further out. The by-id path binds one primary key + * and evaluates no predicate, so the declared row-SET was discarded with no + * diagnostic, and a declared `multi: true` went with it: the caller asked for + * a bulk write over `{ $in: [...] }` and got one silent single-row write. + * This reverses the REMAINING half of the #5748 pin (`a SCALAR data.id still + * outranks where and multi`) — maintainer ruling on #11230, 2026-08-23 — and + * closes the last silent arm of the #5748 / #11009 / #11142 family. The + * boundaries that do NOT move: + * + * - A FALSY scalar `where.id` (`0`, `''`) IS a scalar, so it is not this + * clause's shape at all and keeps its #11142 verdict (`by-id`, nothing to + * conflict with — point 3 below). + * - A `where` with no `id` key (`{}`, no `where` at all, or a `where` that is + * not an object) declares no id predicate and is untouched. + * - With NO scalar id in the payload, a non-scalar `where.id` never reached + * this arm in the first place: it falls down the ladder to `multi`, then + * `reject`, exactly as #5748 left it. * * Three things about that list are load-bearing and easy to get wrong when * copying it by hand — which is the whole argument for importing it instead: @@ -104,11 +125,13 @@ * 2. **`data.id` outranks `where.id`, but only when it IS an id.** The payload * is read first, so a scalar `data.id` still wins over `where` and over an * explicit `multi: true`; `update(o, { id: 'rec_1', … }, { multi: true })` - * is one by-id write, unchanged. Since #11142, "wins over `where`" no - * longer includes silently overriding a truthy scalar `where.id` that - * names a DIFFERENT row — that call is refused (see the clause above); - * what stays is precedence, not the silent drop. What a non-scalar - * `data.id` no longer does + * is one by-id write, unchanged. Since #11142 and #11230, "wins over + * `where`" no longer includes silently overriding a DECLARED `where.id` + * that is not the bound id — a truthy scalar naming a different row + * (#11142) or a non-scalar predicate over a row set (#11230). Both are + * refused (see the two clauses above); what stays is precedence over a + * `where` that declares no id and over `multi`, never a silent drop. What + * a non-scalar `data.id` no longer does * is *outrank* anything: it is not an id, so the decision falls through to * `where.id`, then to `multi`, then to `reject` — exactly the ladder a * non-scalar `where.id` falls down. Until objectstack#5748 the payload half @@ -152,17 +175,27 @@ import { export const ENGINE_UPDATE_REJECT_MESSAGE = 'Update requires an ID or options.multi=true'; /** - * [#11142] The `error.code` of the conflicting-id refusal: a by-id update - * whose truthy scalar `options.where.id` names a different row than the bound - * payload `data.id`. Registered in the spec's `ERROR_CODE_LEDGER` (ADR-0112) - * under `@objectstack/objectql`, the production thrower; travels with - * {@link ENGINE_UPDATE_ID_CONFLICT_STATUS} on the thrown error's own property - * bag (the `recordNotFoundError` convention), so the REST boundary's - * status/code passthrough answers 400 instead of a sanitised 500. + * [#11142/#11230] The `error.code` of the conflicting-id refusals: a by-id + * update whose bound payload `data.id` stands beside an `options.where.id` + * that is not that same id — a truthy scalar naming a DIFFERENT row (#11142), + * or a non-scalar predicate over a row SET (#11230). Registered in the spec's + * `ERROR_CODE_LEDGER` (ADR-0112) under `@objectstack/objectql`, the production + * thrower; travels with {@link ENGINE_UPDATE_ID_CONFLICT_STATUS} on the thrown + * error's own property bag (the `recordNotFoundError` convention), so the REST + * boundary's status/code passthrough answers 400 instead of a sanitised 500. + * + * **One code, two messages, deliberately (#11230).** The two shapes are one + * defect class — the call declares the row address twice and the two + * declarations cannot both hold — and the caller's remedy is the same act in + * both: drop one of the two spellings. A second ledger code would buy a + * consumer no branch it could act on differently, while adding a member to the + * closed `error.code` vocabulary every AI-authored consumer must switch over + * (ADR-0112 D3/D4: registration is deliberate, and the catalog stays small). + * The two messages carry the difference, which is where ADR-0112 puts it. */ export const ENGINE_UPDATE_ID_CONFLICT_CODE = 'UPDATE_ID_MISMATCH'; -/** [#11142] The HTTP status the conflicting-id refusal declares: a caller error, 400. */ +/** [#11142/#11230] The HTTP status the conflicting-id refusals declare: a caller error, 400. */ export const ENGINE_UPDATE_ID_CONFLICT_STATUS = 400; /** What `ObjectQLEngine.update` will do with a given `(data, options)` pair. */ @@ -173,10 +206,11 @@ export type EngineUpdateDispatch = | { readonly kind: 'multi' } /** * Neither — the engine throws `ENGINE_UPDATE_REJECT_MESSAGE`, the #11009 - * unhonoured-predicate message, or (#11142) the conflicting-id message. + * unhonoured-predicate message, or one of the two conflicting-id messages + * (#11142 unequal scalar, #11230 non-scalar predicate). * * `code`/`status` are present only when the refusal declares an ADR-0112 - * envelope of its own (today: the #11142 conflict, + * envelope of its own (today: the #11142 / #11230 conflicting-id refusals, * {@link ENGINE_UPDATE_ID_CONFLICT_CODE} / 400). The #5748 / #11009 * refusals deliberately stay undecorated — adding an envelope to them is a * wire-contract change this module must not make by side effect. Throwers @@ -236,10 +270,33 @@ function asScalarId(value: unknown): string | number | bigint | undefined { export function scalarUpdateId( options?: EngineUpdateDispatchInput | null, ): string | number | bigint | undefined { + const declared = declaredWhereId(options); + return declared.present ? asScalarId(declared.value) : undefined; +} + +/** + * [#11230] Did the caller DECLARE `options.where.id`, and with what raw value? + * + * {@link scalarUpdateId} collapses "no `id` key at all" and "an `id` key + * holding something that is not a primary key" into the same `undefined`, + * because for *picking a row* they are the same answer. The #11230 refusal + * needs them apart: `{ where: {} }` declares nothing, while + * `{ where: { id: { $in: [...] } } }` declares a predicate the by-id path + * would silently discard — and the refusal message has to quote it. + * + * Private on purpose, and `scalarUpdateId` is re-expressed on top of it rather + * than duplicating the lookup: the module header's point is that one rule gets + * one definition, and a second public spelling of "what is in `where.id`" is + * how that stops being true. + */ +function declaredWhereId( + options?: EngineUpdateDispatchInput | null, +): { readonly present: boolean; readonly value: unknown } { const where = options?.where; - if (!where || typeof where !== 'object') return undefined; - if (!('id' in (where as Record))) return undefined; - return asScalarId((where as Record).id); + if (!where || typeof where !== 'object') return { present: false, value: undefined }; + const record = where as Record; + if (!('id' in record)) return { present: false, value: undefined }; + return { present: true, value: record.id }; } /** @@ -280,6 +337,64 @@ export function engineUpdateIdConflictMessage( ); } +/** + * [#11230] How a DECLARED but non-scalar `where.id` is named inside its + * refusal message: by kind, never by dumping the value. + * + * The reader needs to recognise what they wrote (`{ $in: [...] }`, an array, + * `null`), not to read it back — and a caller's predicate can be arbitrarily + * large or cyclic, so `JSON.stringify` here would trade a diagnostic for a + * second failure mode. Operator objects name their keys, which is the half a + * reader actually scans for. + */ +function spellDeclaredWhereId(value: unknown): string { + if (value === null) return 'null'; + if (value === undefined) return 'undefined'; + if (Array.isArray(value)) { + return `an array of ${value.length} value${value.length === 1 ? '' : 's'}`; + } + if (typeof value === 'object') { + const keys = Object.keys(value as Record); + return keys.length > 0 + ? `an operator object (${keys.map((k) => `'${k}'`).join(', ')})` + : 'an operator object with no keys'; + } + return `a ${typeof value}`; +} + +/** + * [#11230] The message a by-id update is refused with when its bound scalar + * payload `data.id` stands beside a DECLARED `options.where.id` that is not a + * primary key at all — an operator object, an array, `null`. + * + * The last silent member of the #5748 / #11009 / #11142 dropped-declaration + * family, and the widest of them: the by-id path discarded the declared row + * SET *and* any declared `multi: true`, so a caller who asked to update every + * row matching `{ $in: [...] }` got one silent single-row write to a row the + * predicate may not even select. Refusing it reverses the REMAINING half of + * the #5748 pin (`a SCALAR data.id still outranks where and multi`) — + * maintainer ruling on #11230, 2026-08-23. + * + * Shares {@link ENGINE_UPDATE_ID_CONFLICT_CODE} with the #11142 message rather + * than registering a second ledger code: same defect class, same remedy (drop + * one of the two row-address spellings), and the difference between them is + * exactly what a message is for. See that constant's note. + */ +export function engineUpdateIdPredicateConflictMessage( + payloadId: string | number | bigint, + whereId: unknown, +): string { + return ( + `Update binds the payload id ${spellScalarId(payloadId)} as the row address, but options.where.id ` + + `declares ${spellDeclaredWhereId(whereId)} — a predicate over a SET of rows, not one primary key. ` + + `The by-id path binds ONLY that one id and never evaluates a predicate, so the write would land on ` + + `${spellScalarId(payloadId)} with the where.id predicate — and any declared multi:true — silently ` + + `ignored (#11230). To write EVERY row the predicate selects, drop id from the payload and declare ` + + `the predicate path: update(object, fields, { where: { id: }, multi: true }). To write ` + + `the single row ${spellScalarId(payloadId)}, drop where.id: update(object, { id, ...fields }).` + ); +} + /** * The one spelling of "throw a `reject` verdict" (#11142) — used by * {@link assertEngineUpdateDispatch} and by `ObjectQL.update` itself, so a @@ -380,6 +495,29 @@ export function resolveEngineUpdateDispatch( status: ENGINE_UPDATE_ID_CONFLICT_STATUS, }; } + // [#11230] The same defect one step further out, and the last silent + // arm of this family. `where.id` is DECLARED but is not a primary key + // at all — `{ $in: [...] }`, an array, `null` — so it is a predicate + // over a SET of rows, and the by-id path evaluates no predicate. The + // call used to dispatch `by-id` on the payload id and discard BOTH the + // row set and any declared `multi: true` with no diagnostic: the + // #5748-pinned verdict `a SCALAR data.id still outranks where and + // multi`, whose REMAINING half the maintainer ruling on #11230 + // (2026-08-23) reverses. `conflictingWhereId === undefined` means the + // scalar test above found no id; `declared.present` separates the two + // reasons it could not — an absent `id` key (nothing was declared, + // by-id stands) from a declared non-scalar (refused here). + if (conflictingWhereId === undefined) { + const declared = declaredWhereId(options); + if (declared.present) { + return { + kind: 'reject', + message: engineUpdateIdPredicateConflictMessage(payloadId, declared.value), + code: ENGINE_UPDATE_ID_CONFLICT_CODE, + status: ENGINE_UPDATE_ID_CONFLICT_STATUS, + }; + } + } } return { kind: 'by-id', id }; } @@ -478,6 +616,21 @@ export const ENGINE_UPDATE_DISPATCH_CASES: readonly EngineUpdateDispatchCase[] = // `multi` (#5748), so the call is still a by-id write carrying a where.id it // can never honour. { what: 'a SCALAR data.id beside a DIFFERENT scalar where.id and multi:true — still refused, the payload id outranks multi (#11142)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: 'rec_2' }, multi: true }, expect: 'reject' }, + // [#11230] The OTHER half of the reversed #5748 pin, and the last silent arm + // of this family: a DECLARED but NON-SCALAR `where.id` beside a scalar + // payload id. It never had a row of its own in this case-set — the assertion + // that pinned it lived in `engine-update-dispatch.test.ts` ("a SCALAR + // data.id still outranks where and multi", first expectation) and is flipped + // there. It gets rows HERE now, so every double bound to this case-set + // inherits the refusal rather than each one re-deriving it. + { what: 'a SCALAR data.id beside an $in where.id and multi:true — refused; the row SET and the declared bulk intent were BOTH silently dropped (#11230 reverses the remaining half of the #5748 pin)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'reject' }, + { what: 'a SCALAR data.id beside an $in where.id, no multi — refused (#11230)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: { $in: ['a', 'b'] } } }, expect: 'reject' }, + { what: 'a SCALAR data.id beside an ARRAY where.id — refused (#11230)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: ['a', 'b'] } }, expect: 'reject' }, + { what: 'a SCALAR data.id beside a NULL where.id — refused (#11230)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: null } }, expect: 'reject' }, + // [#11230] The boundary that does NOT move: a FALSY scalar `where.id` IS a + // scalar, so it is not this refusal's shape at all and keeps the #11142 + // verdict. Pinned so the reversal cannot creep over the truthiness rule. + { what: 'a SCALAR data.id beside a FALSY scalar where.id (0) — still by-id, a falsy id identifies no row (the #11142 boundary, untouched by #11230)', data: { id: 'rec_1', title: 'x' }, options: { where: { id: 0 } }, expect: 'by-id', expectId: 'rec_1' }, // ── The payload's scalar test (objectstack#5748). A non-scalar `data.id` // names no row, so it stops shadowing everything under it: the decision // falls through to `where.id`, then `multi`, then `reject`. Before #5748 diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 31f4462a08..9f6df21733 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -9458,10 +9458,21 @@ export class ObjectStackProtocolImplementation implements // the path `:id` — and nothing in the payload gets to move it. // // The engine's dispatch reads the PAYLOAD first: a truthy scalar `data.id` - // outranks `options.where.id` (`engine-update-dispatch.ts`, case *"a SCALAR - // data.id still wins over a scalar where.id"* — `expectId: 'rec_1'`). That - // rule is correct and deliberate for a caller who hands ObjectQL a payload - // and nothing else (#5748 / PR #5919, ruling A); it is a HOLE here, because + // outranks `options.where.id` (`engine-update-dispatch.ts`, #5748's + // payload-first ladder). That rule is correct and deliberate for a caller + // who hands ObjectQL a payload and nothing else (#5748 / PR #5919, ruling + // A); it is a HOLE here, because + // + // ⚠️ [#11142/#11230] The case this paragraph used to cite by name — *"a + // SCALAR data.id still wins over a scalar where.id"*, `expectId: 'rec_1'` + // — no longer exists in `ENGINE_UPDATE_DISPATCH_CASES`. A `where.id` that + // DISAGREES with a bound payload id is refused at dispatch now + // (`UPDATE_ID_MISMATCH`, 400), whether it is a different scalar (#11142) + // or a non-scalar predicate over a row set (#11230). Nothing below + // changes: the fold makes the two ids EQUAL, the one spelling both + // rulings deliberately keep honoured. What changes is the pre-fix + // behaviour described here — it is a loud refusal now, not a silent + // cross-row write. // this caller has already named the row twice — in the URL and in `where` — // and the three gates around this line all judge THAT row: // diff --git a/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts b/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts index 1437bfb501..e4b208059d 100644 --- a/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts +++ b/packages/metadata-protocol/src/protocol.update-path-id-wins.test.ts @@ -30,10 +30,20 @@ * * ## What is deliberately NOT re-litigated here * - * The engine's payload-first dispatch (`a SCALAR data.id still wins over a - * scalar where.id`, `ENGINE_UPDATE_DISPATCH_CASES`) is correct and merged - * (#5748 / PR #5919) for a caller who hands ObjectQL a payload and nothing - * else, and #6435 / PR #6475's by-id payload strip is likewise untouched — + * The engine's payload-first dispatch (#5748's ladder in + * `ENGINE_UPDATE_DISPATCH_CASES`) is correct and merged (#5748 / PR #5919) for + * a caller who hands ObjectQL a payload and nothing else, and #6435 / PR + * #6475's by-id payload strip is likewise untouched — + * + * ⚠️ [#11142/#11230] That ladder no longer includes the case this paragraph + * used to quote by name, `a SCALAR data.id still wins over a scalar where.id`: + * a `where.id` that disagrees with a bound payload id is refused at dispatch + * (`UPDATE_ID_MISMATCH`, 400) — a different scalar since #11142, a non-scalar + * predicate since #11230. This ingress folds the path id INTO the payload, so + * every call it makes carries two ids that agree, which is the spelling both + * rulings keep honoured; the fake below inherits the refusals for free because + * it asks the producer rather than mirroring it. + * * both are driven against the REAL engine in `packages/objectql`. The fake * below therefore does not re-implement either: its `update` asks the * producer's own `assertEngineUpdateDispatch` which row a call binds and obeys diff --git a/packages/objectql/src/engine-update-dispatch.test.ts b/packages/objectql/src/engine-update-dispatch.test.ts index 58a3eefdf0..4b9cad4ed9 100644 --- a/packages/objectql/src/engine-update-dispatch.test.ts +++ b/packages/objectql/src/engine-update-dispatch.test.ts @@ -28,6 +28,7 @@ import { scalarUpdateId, engineByIdUnhonouredPredicateMessage, engineUpdateIdConflictMessage, + engineUpdateIdPredicateConflictMessage, unhonouredByIdPredicateKeys, } from './engine-update-dispatch.js'; @@ -195,17 +196,42 @@ describe('engine update dispatch — the shared predicate IS the engine (#5480)' // payload. It is pinned here rather than left to the reader, because it // is exactly what a hand-copied guard gets wrong in the OTHER direction — // too strict, and the double then refuses a call the producer accepts. - it('a SCALAR data.id still outranks where and multi (the common legal spelling, untouched by #5748)', () => { + // + // [#11230] THE PIN THE #11230 RULING FLIPPED, name and all. It read + // + // it('a SCALAR data.id still outranks where and multi (the common legal + // spelling, untouched by #5748)', …) + // expect(resolveEngineUpdateDispatch({ id: 'rec_1' }, + // { where: { id: { $in: ['a'] } }, multi: true })) + // .toEqual({ kind: 'by-id', id: 'rec_1' }); + // + // — the REMAINING half of the #5748 verdict after #11142 took the unequal + // scalar half, and the last silent arm of this family: the `$in` row set + // AND the declared `multi: true` were both discarded with no diagnostic. + // The maintainer ruling on #11230 (2026-08-23) reverses it. The assertion + // is INVERTED in place, never deleted and never relaxed, so the reversal + // is visible exactly where the old verdict stood. + it('a SCALAR data.id outranks a `where` that declares NO id, and outranks multi — but no longer outranks a DECLARED where.id (#11142/#11230)', () => { + // FLIPPED (#11230): this line asserted `{ kind: 'by-id', id: 'rec_1' }`. expect(resolveEngineUpdateDispatch({ id: 'rec_1' }, { where: { id: { $in: ['a'] } }, multi: true })) - .toEqual({ kind: 'by-id', id: 'rec_1' }); + .toEqual({ + kind: 'reject', + message: engineUpdateIdPredicateConflictMessage('rec_1', { $in: ['a'] }), + code: ENGINE_UPDATE_ID_CONFLICT_CODE, + status: ENGINE_UPDATE_ID_CONFLICT_STATUS, + }); // [#11142] `{ id: 'rec_1' }` beside `{ where: { id: 'rec_2' } }` used to // sit here as a by-id assertion; the UNEQUAL scalar pair is now refused // (the reversed #5748 pin — see the #11142 describe below). The EQUAL // pair stays the honoured spelling: expect(resolveEngineUpdateDispatch({ id: 'rec_1' }, { where: { id: 'rec_1' } })) .toEqual({ kind: 'by-id', id: 'rec_1' }); + // What a scalar payload id STILL outranks, untouched by either reversal: + // an explicit `multi: true` beside a `where` that declares no id. expect(resolveEngineUpdateDispatch({ id: 42, title: 'x' }, { multi: true })) .toEqual({ kind: 'by-id', id: 42 }); + expect(resolveEngineUpdateDispatch({ id: 42, title: 'x' }, { where: {}, multi: true })) + .toEqual({ kind: 'by-id', id: 42 }); }); // ── #5748, both halves of the flip. FROM: `data.id` was taken verbatim @@ -340,8 +366,9 @@ describe('engine update dispatch — the shared predicate IS the engine (#5480)' // DIFFERENT row than the bound payload id is refused, never silently // dropped. Maintainer ruling 2026-08-23 — the UNEQUAL shape only; the // equal-ids spelling (REST folds the path id into the payload) stays -// honoured, and the falsy / non-scalar `where.id` boundaries keep their -// pre-existing verdicts (truthiness rule; un-reversed #5748 pin). +// honoured. #11142 also left the FALSY and NON-SCALAR `where.id` boundaries +// at their pre-existing verdicts; only the falsy one is still there — +// #11230 reversed the non-scalar half (see the [#11230] describe below). describe('[#11142] a conflicting scalar where.id beside the payload id is refused', () => { const data = { id: 'rec_1', title: 'x' }; const options = { where: { id: 'rec_2' } }; @@ -426,14 +453,20 @@ describe('[#11142] a conflicting scalar where.id beside the payload id is refuse .toEqual({ kind: 'by-id', id: 'rec_1' }); }); - it('a NON-SCALAR where.id keeps its #5748 verdict — the payload id wins (out of the #11142 ruled scope)', () => { - // The un-reversed neighbour pin, restated from the ruling's own boundary: - // widening the refusal over operator-object / array / null `where.id` - // beside a scalar payload id is a separate decision, not a rider here. - expect(resolveEngineUpdateDispatch({ id: 'rec_1', title: 'x' }, { where: { id: { $in: ['a'] } }, multi: true })) - .toEqual({ kind: 'by-id', id: 'rec_1' }); - expect(resolveEngineUpdateDispatch({ id: 'rec_1', title: 'x' }, { where: { id: null } })) - .toEqual({ kind: 'by-id', id: 'rec_1' }); + it('a NON-SCALAR where.id is REFUSED TOO since #11230 — the boundary #11142 left standing did not survive its own ruling', () => { + // FLIPPED (#11230, maintainer ruling 2026-08-23). This pin read "a + // NON-SCALAR where.id keeps its #5748 verdict — the payload id wins (out + // of the #11142 ruled scope)" and asserted `{ kind: 'by-id', id: 'rec_1' }` + // for both shapes below, restating #11142's own boundary: "widening the + // refusal over operator-object / array / null `where.id` beside a scalar + // payload id is a separate decision, not a rider here." #11230 IS that + // separate decision, and it went the other way. Inverted in place, not + // deleted; the full envelope and the boundaries that DID survive are + // pinned in the [#11230] describe below. + expect(resolveEngineUpdateDispatch({ id: 'rec_1', title: 'x' }, { where: { id: { $in: ['a'] } }, multi: true }).kind) + .toBe('reject'); + expect(resolveEngineUpdateDispatch({ id: 'rec_1', title: 'x' }, { where: { id: null } }).kind) + .toBe('reject'); }); it('the #11009 refusal keeps PRIORITY on a where that also carries extra keys (its message names them, unchanged)', () => { @@ -444,3 +477,154 @@ describe('[#11142] a conflicting scalar where.id beside the payload id is refuse .toEqual({ kind: 'reject', message: engineByIdUnhonouredPredicateMessage('Update', ['tenant']) }); }); }); + +// ── [#11230] The OTHER reversed half of the #5748 pin, and the last silent arm +// of the #5748 / #11009 / #11142 family: a DECLARED but non-scalar +// `where.id` — an operator object, an array, `null` — beside a bound scalar +// payload id. The by-id path evaluates no predicate, so the declared row SET +// and any declared `multi: true` were BOTH discarded with no diagnostic. +// Maintainer ruling 2026-08-23, on the same terms as #11142 and sharing its +// error code: one defect class, one remedy, two messages. +describe('[#11230] a DECLARED non-scalar where.id beside the payload id is refused', () => { + const data = { id: 'rec_1', title: 'x' }; + const options = { where: { id: { $in: ['a', 'b'] } }, multi: true }; + const message = engineUpdateIdPredicateConflictMessage('rec_1', { $in: ['a', 'b'] }); + + it('the predicate refuses with the declared message, code and status', () => { + expect(resolveEngineUpdateDispatch(data, options)).toEqual({ + kind: 'reject', + message, + code: ENGINE_UPDATE_ID_CONFLICT_CODE, + status: ENGINE_UPDATE_ID_CONFLICT_STATUS, + }); + // Deliberately the SAME ADR-0112 envelope #11142 declares — see the + // constant's own note in `engine-update-dispatch.ts`. Pinned by value here + // too, so a future split into a second ledger code is a deliberate act. + expect(ENGINE_UPDATE_ID_CONFLICT_CODE).toBe('UPDATE_ID_MISMATCH'); + expect(ENGINE_UPDATE_ID_CONFLICT_STATUS).toBe(400); + }); + + it('assertEngineUpdateDispatch throws it carrying code + status (every pinned fake inherits this)', () => { + let caught: any; + try { + assertEngineUpdateDispatch(data, options); + } catch (e) { + caught = e; + } + expect(caught, 'expected the non-scalar predicate refusal, but the call resolved').toBeDefined(); + expect(caught.code).toBe(ENGINE_UPDATE_ID_CONFLICT_CODE); + expect(caught.status).toBe(ENGINE_UPDATE_ID_CONFLICT_STATUS); + expect(caught.message).toBe(message); + }); + + it('the REAL engine throws the same envelope and NOTHING reaches the driver', async () => { + const { engine, calls } = await makeEngine(); + let caught: any; + try { + await engine.update('task', data as any, options as any); + } catch (e) { + caught = e; + } + expect(caught, 'expected the non-scalar predicate refusal, but the call resolved').toBeDefined(); + expect(caught.code).toBe(ENGINE_UPDATE_ID_CONFLICT_CODE); + expect(caught.status).toBe(ENGINE_UPDATE_ID_CONFLICT_STATUS); + expect(caught.message).toBe(message); + // The entire point: the pre-#11230 behaviour wrote rec_1 — one row, by id + // — with the `$in` row set and the declared `multi: true` both ignored. + expect(calls).toEqual([]); + }); + + // ⚠️ CONTROL for the three pins above. A refusal assertion passes trivially + // if the fixture never constructs the contradictory pair (a typo in + // `options`, a `where` the engine never reads). These two calls are the + // SAME call minus one half of the contradiction, and each must still + // reach the driver — so the refusals above are the branch firing, not the + // fixture missing it. + it('CONTROL — the same call without the payload id still succeeds, as the bulk write it declared', async () => { + const observed = await observeEngine({ title: 'x' }, options); + expect(observed.kind).toBe('multi'); + }); + + it('CONTROL — the same call without the where.id predicate still succeeds, as a by-id write on the payload id', async () => { + const observed = await observeEngine(data, { multi: true }); + expect(observed.kind).toBe('by-id'); + expect(observed.boundId).toBe('rec_1'); + }); + + it('every non-scalar spelling is refused, and the message names the kind the caller wrote', () => { + const operator = resolveEngineUpdateDispatch(data, { where: { id: { $in: ['a'] } } }); + expect(operator.kind).toBe('reject'); + expect((operator as { message?: string }).message).toContain("an operator object ('$in')"); + + const array = resolveEngineUpdateDispatch(data, { where: { id: ['a', 'b'] } }); + expect(array.kind).toBe('reject'); + expect((array as { message?: string }).message).toBe(engineUpdateIdPredicateConflictMessage('rec_1', ['a', 'b'])); + expect((array as { message?: string }).message).toContain('an array of 2 values'); + + const nul = resolveEngineUpdateDispatch(data, { where: { id: null } }); + expect(nul.kind).toBe('reject'); + expect((nul as { message?: string }).message).toContain('declares null'); + + // An explicitly-`undefined` `id` key is a DECLARED key (`'id' in where`), + // and this repo already reads an explicitly-undefined `where` key as a + // real declaration one clause over: `{ where: { tenant: undefined } }` + // beside a payload id is a #11009 refusal today. Same answer here, so the + // two clauses cannot disagree about what "declared" means. + const undef = resolveEngineUpdateDispatch(data, { where: { id: undefined } }); + expect(undef.kind).toBe('reject'); + expect((undef as { message?: string }).message).toContain('declares undefined'); + }); + + it('the message prescribes BOTH call-site fixes, and says the bulk intent was dropped too', () => { + expect(message).toContain('multi:true'); + expect(message).toContain('drop id from the payload'); + expect(message).toContain('drop where.id'); + expect(message).toContain('#11230'); + }); + + it('a FALSY scalar where.id is a SCALAR, so #11230 does not reach it — the #11142 boundary is untouched', () => { + // The truthiness rule (module header point 3): `0` / `''` are scalars that + // identify no row, so there is neither a second row address (#11142) nor a + // predicate over a set (#11230). Pinned so the reversal cannot creep. + expect(resolveEngineUpdateDispatch(data, { where: { id: 0 } })) + .toEqual({ kind: 'by-id', id: 'rec_1' }); + expect(resolveEngineUpdateDispatch(data, { where: { id: '' } })) + .toEqual({ kind: 'by-id', id: 'rec_1' }); + }); + + it('a where that DECLARES no id at all is untouched — the refusal is about a declaration, not about `where`', () => { + expect(resolveEngineUpdateDispatch(data, { where: {} })).toEqual({ kind: 'by-id', id: 'rec_1' }); + expect(resolveEngineUpdateDispatch(data, {})).toEqual({ kind: 'by-id', id: 'rec_1' }); + expect(resolveEngineUpdateDispatch(data, undefined)).toEqual({ kind: 'by-id', id: 'rec_1' }); + expect(resolveEngineUpdateDispatch(data, { where: 'not-an-object' as unknown })).toEqual({ kind: 'by-id', id: 'rec_1' }); + }); + + it('with NO scalar payload id the ladder is exactly as #5748 left it — a non-scalar where.id never reached this arm', () => { + // The refusal lives on the PAYLOAD-sourced by-id arm only. Without a + // payload id these calls fall down the ordinary ladder, and #11230 changes + // neither verdict. + expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: { $in: ['a'] } }, multi: true })) + .toEqual({ kind: 'multi' }); + expect(resolveEngineUpdateDispatch({ title: 'x' }, { where: { id: { $in: ['a'] } } })) + .toEqual({ kind: 'reject', message: ENGINE_UPDATE_REJECT_MESSAGE }); + // …and a NON-scalar payload id is not an id either, so it does not open + // this arm: the decision still falls through to `multi` (#5748). + expect(resolveEngineUpdateDispatch({ id: { $in: ['x'] }, title: 'x' }, { where: { id: { $in: ['a'] } }, multi: true })) + .toEqual({ kind: 'multi' }); + }); + + it('the #11009 refusal keeps PRIORITY when `where` also carries extra keys (its message names them, unchanged)', () => { + // Both defects at once. The unhonoured-keys check runs first and is + // byte-identical to before this change — no existing #11009 pin moves. + expect(resolveEngineUpdateDispatch(data, { where: { id: { $in: ['a'] }, tenant: 't1' } })) + .toEqual({ kind: 'reject', message: engineByIdUnhonouredPredicateMessage('Update', ['tenant']) }); + }); + + it('the #11142 message is NOT reused for this shape — the two refusals stay distinguishable', () => { + // One code, two messages (ADR-0112: the code classifies, the message + // locates). A future edit that collapses them would tell a caller who + // wrote `{ $in: [...] }` that they named "a DIFFERENT row". + expect(message).not.toBe(engineUpdateIdConflictMessage('rec_1', 'rec_2')); + expect(message).not.toContain('names a DIFFERENT row'); + }); +}); diff --git a/packages/objectql/src/engine-update-dispatch.ts b/packages/objectql/src/engine-update-dispatch.ts index d8b33df40e..0be94b3e8f 100644 --- a/packages/objectql/src/engine-update-dispatch.ts +++ b/packages/objectql/src/engine-update-dispatch.ts @@ -26,13 +26,15 @@ export { // once, here, so `objectql` callers can quote the refusal verbatim. engineByIdUnhonouredPredicateMessage, unhonouredByIdPredicateKeys, - // [#11142] The conflicting-id refusal (a truthy scalar `where.id` naming a - // DIFFERENT row than the bound payload id): its message composer, its - // declared ADR-0112 code/status, and the one reject thrower the real engine - // and every pinned fake share. + // [#11142/#11230] The two conflicting-id refusals — a truthy scalar + // `where.id` naming a DIFFERENT row than the bound payload id (#11142), and + // a DECLARED but non-scalar `where.id` predicate beside it (#11230) — with + // their message composers, the ADR-0112 code/status both declare, and the + // one reject thrower the real engine and every pinned fake share. ENGINE_UPDATE_ID_CONFLICT_CODE, ENGINE_UPDATE_ID_CONFLICT_STATUS, engineUpdateIdConflictMessage, + engineUpdateIdPredicateConflictMessage, engineUpdateDispatchRejectError, } from '@objectstack/metadata-core'; diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index bf1f6d0d07..b29ca596ff 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -9868,8 +9868,9 @@ export class ObjectQL implements IObjectQLEngine { // - A TRUTHY SCALAR `data.id` is left exactly as it is. There // the payload's `id` IS the bound key (it outranks `where` — // same case-set; since #11142 a truthy scalar `where.id` - // naming a DIFFERENT row is refused at dispatch and never - // reaches this branch), so the write is `SET id = 'rec_1' + // naming a DIFFERENT row, and since #11230 a DECLARED + // non-scalar `where.id` predicate, are refused at dispatch + // and never reach this branch), so the write is `SET id = 'rec_1' // WHERE id = 'rec_1'`: a same-value no-op, redundant rather // than damaging, and long-standing behaviour. Widening the // strip to cover it is a separate decision, not a rider diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index bb9d5b6193..181b53598c 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -479,11 +479,17 @@ export const ERROR_CODE_LEDGER = { // been written (`TransactionUnsupportedError`, `transaction-errors.ts`; // ADR-0119 D1/D4 fail-closed posture). Same #8087-gate family. 'ERR_TRANSACTION_UNSUPPORTED', - // [#11142] a by-id update carried a truthy scalar `options.where.id` naming - // a DIFFERENT row than the bound payload `data.id` — a condition that can - // never hold, refused 400 at dispatch instead of silently writing the - // payload row (the reversed #5748 pin; equal ids — the REST path-id fold — - // stay honoured). Stamped by `@objectstack/metadata-core`'s + // [#11142/#11230] a by-id update carried an `options.where.id` that is not + // the bound payload `data.id` — a truthy scalar naming a DIFFERENT row + // (#11142), or a non-scalar predicate over a row SET (#11230, which also + // swallowed a declared `multi: true`). Either way a condition the by-id + // path can never evaluate, refused 400 at dispatch instead of silently + // writing the payload row (the two halves of the reversed #5748 pin; equal + // ids — the REST path-id fold — stay honoured). ONE code for both shapes, + // deliberately: same defect class, same caller remedy (drop one of the two + // row-address spellings), and the difference rides the message, which is + // where D3/D4 put it rather than growing the closed `code` vocabulary. + // Stamped by `@objectstack/metadata-core`'s // `engineUpdateDispatchRejectError`, thrown in production by // `ObjectQL.update` (`engine.ts`), hence registered here. Not a // VALIDATION_ERROR synonym: the payload parses fine — the two row