Skip to content

Commit afa6aa5

Browse files
feat(objectql): pin fake engines to ObjectQL.delete's dispatch contract (#4550) (#4948)
#4550 records a failure mode with four instances: a test double LOOSER than the implementation it replaces converts a green suite into no suite at all — silently, and on exactly the paths a double was introduced for, which are the paths that were hard to test, which are usually where the contract is densest. This takes ONE slice of it, the one whose criterion is mechanically decidable: the ObjectQL engine's delete dispatch. `ObjectQL.delete(object, options)` is a total function from an options bag to three verdicts — `by-id` (scalar `where.id`), `multi` (`options.multi`), or a throw — so "is this double looser?" has a yes/no answer that does not require reading the test's intent. The producer's decision now lives in one place. `engine-delete-dispatch.ts` exports `resolveEngineDeleteDispatch` / `assertEngineDeleteDispatch` / `scalarDeleteId` / `ENGINE_DELETE_DISPATCH_CASES`, `ObjectQL.delete` itself reads it, and a fake engine calls it instead of mirroring it. #4434's fix mirrored the guard by hand into one fake; a mirror is a second copy of the contract and drifts the moment either side is edited — and the scalar test is the half a mirror drops (`where: { id: { $in: [...] } }` looks like an id and is a multi-row predicate). A double that imports the decision cannot be looser than the decision. `scripts/check-engine-double-contract.mjs` (wired into lint.yml's ESLint job, `--self-test` first per the repo's 10 other gates) finds all 39 fake ObjectQL engines by AST — separating them from the 39 DRIVER doubles, whose `delete` takes a scalar id and is a different contract — and requires the pinned call. 9 are converted here (objectql's five sys_metadata fakes, plugin-sharing's four, including the two that hand-mirrored); the other 30 sit in a measured, shrink-only baseline that reconciles in both directions. Proof it discriminates, not just that it is green: 1. `git show ba5ff2f^:…/sharing-rule.test.ts` — the pre-#4434 fake — restored: the gate goes red naming that file and line. 2. With the fake pinned and the pre-#4434 `deleteRule` restored, the PRE-EXISTING test `deleteRule drops rule + all its grants` fails with `Delete requires an ID or options.multi=true` — the same error the running server answered 500 with. Had this gate existed, #4434 could not have shipped. Verified: objectql 111 files / 1757 tests, plugin-sharing 11 / 243, both typecheck clean, ESLint clean. @objectstack/spec ran 295/295 files green with the dispatch guard installed in every engine double, which is why its type-conformance witness is EXEMPT rather than DEBT. Refs #4550, #4434 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ Co-authored-by: Claude <noreply@anthropic.com>
1 parent d5eae53 commit afa6aa5

18 files changed

Lines changed: 1234 additions & 36 deletions
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
feat(objectql): export the delete-dispatch contract so test doubles can be pinned to it (#4550)
6+
7+
A test double that is **looser** than the implementation it replaces converts a
8+
green suite into no suite at all — silently, and on exactly the paths a double
9+
was introduced for, which are the paths that were hard to test, which are
10+
usually where the contract is densest. #4434 is the worked example:
11+
`DELETE /api/v1/sharing/rules/:idOrName` answered 500 for every rule and both
12+
address forms it advertises, from the day it was written, while
13+
`deleteRule drops rule + all its grants` asserted success against it the whole
14+
time — against a fake engine whose `delete` accepted the one call shape
15+
`ObjectQL.delete` refuses.
16+
17+
`ObjectQL.delete`'s dispatch decision now lives in one exported place instead of
18+
being re-derived by every fake:
19+
20+
```ts
21+
import { assertEngineDeleteDispatch } from '@objectstack/objectql';
22+
23+
async delete(object: string, options?: any) {
24+
assertEngineDeleteDispatch(options); // refuses what a real server refuses
25+
26+
}
27+
```
28+
29+
New exports, all pure and side-effect free:
30+
31+
- `resolveEngineDeleteDispatch(options)``{ kind: 'by-id', id }` |
32+
`{ kind: 'multi' }` | `{ kind: 'reject', message }` — what the engine will do
33+
with this call, without doing it.
34+
- `assertEngineDeleteDispatch(options)` — throws exactly what the engine throws
35+
on `reject`, returns the dispatch otherwise. This is the line a fake engine's
36+
`delete` opens with.
37+
- `scalarDeleteId(options)` — the SCALAR `where.id` or `undefined`. The half a
38+
hand-written mirror drops: `where: { id: { $in: [...] } }` looks like an id
39+
and is a multi-row predicate, so the engine rejects it without `multi`.
40+
- `ENGINE_DELETE_REJECT_MESSAGE`, `ENGINE_DELETE_DISPATCH_CASES` — the message
41+
and the shared conformance case-set, the same role
42+
`packages/spec/src/data/*-conformance.ts` plays for drivers.
43+
44+
`ObjectQL.delete` itself reads `resolveEngineDeleteDispatch`, so a double that
45+
imports it cannot be looser than the engine, ever — that is the property, and
46+
it is the one a hand-mirrored `if` can only have until somebody edits one side.
47+
No runtime behaviour changes: the same three verdicts, over the same inputs,
48+
proved case-by-case against the real engine in
49+
`engine-delete-dispatch.test.ts`.
50+
51+
Repo-side, `pnpm check:engine-double-contract` (wired into `lint.yml`) finds all
52+
39 fake ObjectQL engines in the repo, holds new ones to this predicate, and
53+
keeps the 30 not yet converted in a measured, shrink-only baseline.

.github/workflows/lint.yml

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,23 @@ jobs:
289289
- name: Published-files whitelist guard
290290
run: pnpm check:published-files
291291

292+
# Engine test-double contract gate (#4550, from #4434). A test double
293+
# LOOSER than the implementation it replaces turns a green suite into no
294+
# suite at all, silently, on exactly the paths a double was introduced
295+
# for. #4434 is the worked example: DELETE /sharing/rules/:idOrName
296+
# answered 500 for every rule and both address forms from the day it was
297+
# written, while `deleteRule drops rule + all its grants` asserted success
298+
# against a fake engine that accepted the one call shape ObjectQL.delete
299+
# refuses. This holds every fake ObjectQL engine's `delete` to the real
300+
# dispatch predicate — imported from @objectstack/objectql, not
301+
# hand-mirrored, so it cannot drift — with the pre-existing fakes in a
302+
# shrink-only, measured baseline. Static AST only, so it needs no build
303+
# and belongs in this job. Runs its own --self-test first: the detector
304+
# can be broken while every fake is fine, and a scan that quietly stops
305+
# matching would report OK while reading nothing (#4868's family).
306+
- name: Engine test-double contract gate
307+
run: pnpm check:engine-double-contract
308+
292309
typecheck:
293310
name: TypeScript Type Check
294311
runs-on: ubuntu-latest

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@
5454
"check:node-version": "node scripts/check-node-version.mjs",
5555
"check:published-files": "node scripts/check-published-files.mjs --self-test && node scripts/check-published-files.mjs",
5656
"check:type-check-coverage": "node scripts/check-type-check-coverage.mjs --self-test && node scripts/check-type-check-coverage.mjs",
57-
"check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs"
57+
"check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs",
58+
"check:engine-double-contract": "node scripts/check-engine-double-contract.mjs --self-test && node scripts/check-engine-double-contract.mjs"
5859
},
5960
"keywords": [
6061
"objectstack",
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// objectstack#4550 — the shared delete-dispatch predicate must be the REAL
4+
// engine's answer, not a second opinion that happens to agree today.
5+
//
6+
// A shared predicate that drifted from `ObjectQL.delete` would be worse than
7+
// no predicate at all: every fake engine pinned to it would be confidently,
8+
// uniformly wrong, and the gate over them would report success (route-ownership
9+
// rule 3 — prefer failing to falling back). So this file does not test the
10+
// predicate against a table of expectations written next to it. It drives the
11+
// **real engine** with a recording driver over `ENGINE_DELETE_DISPATCH_CASES`
12+
// and asserts the engine's observed behaviour equals the predicate's verdict,
13+
// case by case.
14+
//
15+
// If someone changes the dispatch rule in `engine.ts` without changing
16+
// `engine-delete-dispatch.ts`, this goes red here — the one place where both
17+
// halves are in the room together.
18+
19+
import { describe, it, expect } from 'vitest';
20+
import { ObjectQL } from './engine.js';
21+
import {
22+
ENGINE_DELETE_DISPATCH_CASES,
23+
ENGINE_DELETE_REJECT_MESSAGE,
24+
resolveEngineDeleteDispatch,
25+
assertEngineDeleteDispatch,
26+
scalarDeleteId,
27+
} from './engine-delete-dispatch.js';
28+
29+
/** Records which driver entry point the engine chose, if any. */
30+
function makeRecordingDriver() {
31+
const calls: Array<{ fn: 'delete' | 'deleteMany'; arg: unknown }> = [];
32+
const driver: any = {
33+
name: 'recording',
34+
version: '0.0.0',
35+
supports: {},
36+
async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; },
37+
async find() { return []; },
38+
async findOne() { return null; },
39+
async create(_o: string, data: Record<string, unknown>) { return { id: 'r1', ...data }; },
40+
async update(_o: string, id: string, data: Record<string, unknown>) { return { id, ...data }; },
41+
async delete(_o: string, id: string) { calls.push({ fn: 'delete', arg: id }); return true; },
42+
async deleteMany(_o: string, ast: unknown) { calls.push({ fn: 'deleteMany', arg: ast }); return 0; },
43+
async count() { return 0; },
44+
async bulkCreate() { return []; }, async bulkUpdate() { return []; }, async bulkDelete() {},
45+
async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; },
46+
async commit() {}, async rollback() {},
47+
};
48+
return { driver, calls };
49+
}
50+
51+
async function makeEngine() {
52+
const engine = new ObjectQL();
53+
const { driver, calls } = makeRecordingDriver();
54+
engine.registerDriver(driver, true);
55+
await engine.init();
56+
engine.registry.registerObject({ name: 'task', fields: { title: { type: 'text' } } } as any);
57+
return { engine, calls };
58+
}
59+
60+
/** What the real engine actually did with this options bag. */
61+
async function observeEngine(options: unknown): Promise<'by-id' | 'multi' | 'reject'> {
62+
const { engine, calls } = await makeEngine();
63+
try {
64+
await engine.delete('task', options as any);
65+
} catch (e) {
66+
if ((e as Error).message === ENGINE_DELETE_REJECT_MESSAGE) return 'reject';
67+
throw e;
68+
}
69+
if (calls.length !== 1) {
70+
throw new Error(`expected exactly one driver call, saw ${JSON.stringify(calls)}`);
71+
}
72+
return calls[0].fn === 'delete' ? 'by-id' : 'multi';
73+
}
74+
75+
describe('engine delete dispatch — the shared predicate IS the engine (#4550)', () => {
76+
it('has cases on both sides of the guard (an empty or one-sided set proves nothing)', () => {
77+
const kinds = new Set(ENGINE_DELETE_DISPATCH_CASES.map((c) => c.expect));
78+
expect(kinds).toEqual(new Set(['by-id', 'multi', 'reject']));
79+
expect(ENGINE_DELETE_DISPATCH_CASES.filter((c) => c.expect === 'reject').length).toBeGreaterThan(3);
80+
});
81+
82+
for (const c of ENGINE_DELETE_DISPATCH_CASES) {
83+
it(`real engine agrees with the predicate: ${c.what}${c.expect}`, async () => {
84+
expect(resolveEngineDeleteDispatch(c.options).kind, 'predicate').toBe(c.expect);
85+
expect(await observeEngine(c.options), 'real ObjectQL.delete').toBe(c.expect);
86+
});
87+
}
88+
89+
it('rejects with the exact message a fake must reproduce', () => {
90+
expect(() => assertEngineDeleteDispatch({ where: { rule_id: 'r1' } }))
91+
.toThrow(ENGINE_DELETE_REJECT_MESSAGE);
92+
// …and returns the dispatch (never `reject`) when the call is legal.
93+
expect(assertEngineDeleteDispatch({ where: { id: 'a' } })).toEqual({ kind: 'by-id', id: 'a' });
94+
expect(assertEngineDeleteDispatch({ multi: true })).toEqual({ kind: 'multi' });
95+
});
96+
97+
it('scalarDeleteId treats operator objects and arrays as predicates, not ids', () => {
98+
expect(scalarDeleteId({ where: { id: 'a' } })).toBe('a');
99+
expect(scalarDeleteId({ where: { id: 7 } })).toBe(7);
100+
expect(scalarDeleteId({ where: { id: { $in: ['a'] } } })).toBeUndefined();
101+
expect(scalarDeleteId({ where: { id: ['a'] } })).toBeUndefined();
102+
expect(scalarDeleteId({ where: { id: null } })).toBeUndefined();
103+
expect(scalarDeleteId({ where: {} })).toBeUndefined();
104+
expect(scalarDeleteId(undefined)).toBeUndefined();
105+
});
106+
});
Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The **one** answer to "what does `ObjectQLEngine.delete` do with this call?"
5+
* — extracted so that the engine and every test double that stands in for it
6+
* read the same predicate rather than two hand-written approximations of it
7+
* (objectstack#4550, from objectstack#4434).
8+
*
9+
* ## Why this is a shared module and not four lines inside `engine.ts`
10+
*
11+
* `#4434` shipped green. `DELETE /api/v1/sharing/rules/:idOrName` answered 500
12+
* for **both** address forms the route advertises, for every rule, from the day
13+
* it was written — and `plugin-sharing`'s `deleteRule drops rule + all its
14+
* grants` test asserted success against it the whole time. The route was not
15+
* untested; it was tested against a **fake engine whose `delete` accepted a
16+
* call the real engine refuses**. A predicate-shaped purge of
17+
* `sys_record_share` (no scalar `where.id`, no `options.multi`) is precisely
18+
* the one shape `delete()` throws on, and the fake happily deleted by
19+
* predicate.
20+
*
21+
* The fix for #4434 mirrored the guard into that fake by hand. That closes one
22+
* fake and starts a second copy of the contract — the failure mode this module
23+
* exists to remove. A double that *imports the producer's own decision* cannot
24+
* be looser than the producer, ever, which is the property the gate wants and
25+
* the property a copy can only have until someone edits one side.
26+
*
27+
* Same reasoning as `packages/spec/src/data/*-conformance.ts` for drivers, and
28+
* the same shape as objectstack#4455: **the scan and the validator must answer
29+
* with one predicate.**
30+
*
31+
* ## The contract, normatively
32+
*
33+
* `delete(object, options)` dispatches on exactly one question — *does this
34+
* call identify a single row by primary key?*
35+
*
36+
* - `options.where.id` is a **scalar** (`string` / `number` / `bigint`, not
37+
* `null`) → `by-id`: routes to `driver.delete`, runs cascade-delete and the
38+
* by-id RLS pre-image check.
39+
* - otherwise, `options.multi` is truthy → `multi`: routes to
40+
* `driver.deleteMany` with the middleware-composed AST.
41+
* - otherwise → **`reject`**. The call names neither one row nor a bulk
42+
* intent, and the engine throws rather than guessing.
43+
*
44+
* The scalar test is load-bearing and is the half a hand-written double most
45+
* often drops: `where: { id: { $in: [...] } }` is a *multi-row predicate*, not
46+
* an id. Treating it as an id would bind the operator object literally into
47+
* `driver.delete(object, {$in: […]})` **and** skip both the row-scoping AST
48+
* seeding (#2982) and the by-id pre-image check. So it is `reject` unless the
49+
* caller also said `multi`.
50+
*
51+
* @see ObjectQL.delete in `engine.ts` — the only production caller.
52+
* @see scripts/check-engine-double-contract.mjs — the gate that keeps doubles on it.
53+
*/
54+
55+
/** The message `delete()` throws when a call identifies neither one row nor a bulk intent. */
56+
export const ENGINE_DELETE_REJECT_MESSAGE = 'Delete requires an ID or options.multi=true';
57+
58+
/** What `ObjectQLEngine.delete` will do with a given options bag. */
59+
export type EngineDeleteDispatch =
60+
/** A scalar `where.id` — `driver.delete`, cascade + by-id RLS pre-image. */
61+
| { readonly kind: 'by-id'; readonly id: string | number | bigint }
62+
/** No single id but `options.multi` — `driver.deleteMany` with the composed AST. */
63+
| { readonly kind: 'multi' }
64+
/** Neither — the engine throws `ENGINE_DELETE_REJECT_MESSAGE`. */
65+
| { readonly kind: 'reject'; readonly message: string };
66+
67+
/** The subset of `EngineDeleteOptions` the dispatch decision actually reads. */
68+
export interface EngineDeleteDispatchInput {
69+
readonly where?: unknown;
70+
readonly multi?: unknown;
71+
readonly [k: string]: unknown;
72+
}
73+
74+
/**
75+
* Extract the SCALAR `where.id`, or `undefined` when the call does not name one
76+
* row by primary key.
77+
*
78+
* `null`, `undefined`, arrays, and operator objects (`{ $in: [...] }`,
79+
* `{ $ne: … }`) all yield `undefined` — they are predicates over many rows, not
80+
* a primary key.
81+
*/
82+
export function scalarDeleteId(
83+
options?: EngineDeleteDispatchInput | null,
84+
): string | number | bigint | undefined {
85+
const where = options?.where;
86+
if (!where || typeof where !== 'object') return undefined;
87+
if (!('id' in (where as Record<string, unknown>))) return undefined;
88+
const whereId = (where as Record<string, unknown>).id;
89+
const t = typeof whereId;
90+
if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) {
91+
return whereId as string | number | bigint;
92+
}
93+
return undefined;
94+
}
95+
96+
/**
97+
* Decide what `ObjectQLEngine.delete` does with `options`, without doing it.
98+
*
99+
* Pure and side-effect free, so a test double can call it to *classify* a call
100+
* and then implement `by-id` / `multi` however its fixture stores rows — while
101+
* being bound to the real engine's `reject` surface for free.
102+
*/
103+
export function resolveEngineDeleteDispatch(
104+
options?: EngineDeleteDispatchInput | null,
105+
): EngineDeleteDispatch {
106+
const id = scalarDeleteId(options);
107+
if (id !== undefined) return { kind: 'by-id', id };
108+
if (options?.multi) return { kind: 'multi' };
109+
return { kind: 'reject', message: ENGINE_DELETE_REJECT_MESSAGE };
110+
}
111+
112+
/**
113+
* Throw exactly what `ObjectQLEngine.delete` throws when a call is neither
114+
* `by-id` nor `multi`; return the resolved dispatch otherwise.
115+
*
116+
* This is the line a fake engine's `delete` opens with. One call pins the fake
117+
* to the producer's rejection surface, and — unlike a mirrored `if` — it cannot
118+
* drift when the producer's rule changes.
119+
*
120+
* ```ts
121+
* async delete(object: string, options?: any) {
122+
* assertEngineDeleteDispatch(options); // refuses what a real server refuses
123+
* …
124+
* }
125+
* ```
126+
*/
127+
export function assertEngineDeleteDispatch(
128+
options?: EngineDeleteDispatchInput | null,
129+
): Exclude<EngineDeleteDispatch, { kind: 'reject' }> {
130+
const dispatch = resolveEngineDeleteDispatch(options);
131+
if (dispatch.kind === 'reject') throw new Error(dispatch.message);
132+
return dispatch;
133+
}
134+
135+
/**
136+
* The shared conformance case-set for the delete dispatch — the same role
137+
* `packages/spec/src/data/*-conformance.ts` plays for drivers.
138+
*
139+
* Every case names a call shape and the verdict the **real engine** gives it.
140+
* A double proved against these is proved against the producer, including the
141+
* three shapes that look like an id and are not.
142+
*/
143+
export interface EngineDeleteDispatchCase {
144+
/** What the shape is, in the words a failure message should use. */
145+
readonly what: string;
146+
/** The options bag handed to `delete(object, options)`. */
147+
readonly options: EngineDeleteDispatchInput | undefined;
148+
/** The verdict the engine gives it. */
149+
readonly expect: EngineDeleteDispatch['kind'];
150+
}
151+
152+
export const ENGINE_DELETE_DISPATCH_CASES: readonly EngineDeleteDispatchCase[] = [
153+
{ what: 'scalar string id', options: { where: { id: 'rec_1' } }, expect: 'by-id' },
154+
{ what: 'scalar number id', options: { where: { id: 42 } }, expect: 'by-id' },
155+
{ what: 'scalar id alongside other predicates', options: { where: { id: 'rec_1', tenant: 't1' } }, expect: 'by-id' },
156+
{ what: 'multi with a predicate', options: { where: { rule_id: 'r1' }, multi: true }, expect: 'multi' },
157+
{ what: 'multi with no predicate at all', options: { multi: true }, expect: 'multi' },
158+
{ what: 'multi alongside an $in id set', options: { where: { id: { $in: ['a', 'b'] } }, multi: true }, expect: 'multi' },
159+
// ── The rejects. Everything below is what #4434 shipped against a fake that
160+
// accepted it, and what a running server answers 500 to.
161+
{ what: 'predicate on a non-id column, no multi', options: { where: { rule_id: 'r1' } }, expect: 'reject' },
162+
{ what: '$in over ids, no multi (an operator object is NOT an id)', options: { where: { id: { $in: ['a', 'b'] } } }, expect: 'reject' },
163+
{ what: 'array id, no multi', options: { where: { id: ['a', 'b'] } }, expect: 'reject' },
164+
{ what: 'null id, no multi', options: { where: { id: null } }, expect: 'reject' },
165+
{ what: 'empty where, no multi', options: { where: {} }, expect: 'reject' },
166+
{ what: 'no options at all', options: undefined, expect: 'reject' },
167+
{ what: 'multi explicitly false with a predicate', options: { where: { rule_id: 'r1' }, multi: false }, expect: 'reject' },
168+
];

0 commit comments

Comments
 (0)