From 2b1a08044cccba3fa562d3f0a95f488ca61c397d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:35:31 +0000 Subject: [PATCH] fix(objectql): materialise the master-detail header a `parent` predicate reads (#6457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveMasterDetailParent(s)` handed the driver-read header row through as-is, so a `parent.` predicate faulted with `No such key` on any driver that did not echo back the column it reads. That fault is NOT `Unknown variable: parent` — `parent` IS bound — so it missed #4889's fail-closed carve-out and took the ordinary fail-OPEN exit: a `readonlyWhen` lock was let through and a `requiredWhen` requirement was not enforced. The header is now made TOTAL over the MASTER object's declared fields inside the two resolvers, which are the only place holding both the master's schema and the just-read row. One change serves both consumers; no strip or validator signature moves; the same `materializeDeclaredFields` helper as every other server seam (#1871/#4649/#4953); no extra query. The fail-closed line is preserved exactly: materialisation only ever applies to a header row that EXISTS, so an unresolvable header still leaves `parent` unbound, still faults as `Unknown variable: parent`, and is still read as LOCKED. Both paths pinned, and told apart by fault channel rather than by the write's outcome. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GNUt6cLqqcaLVbiDbin27R --- .changeset/parent-header-materialize.md | 66 +++++++ .../src/engine-readonly-when-parent.test.ts | 182 ++++++++++++++++++ .../src/engine-required-when-parent.test.ts | 153 +++++++++++++++ packages/objectql/src/engine.ts | 91 ++++++++- .../src/validation/rule-validator.test.ts | 24 ++- .../objectql/src/validation/rule-validator.ts | 11 ++ 6 files changed, 522 insertions(+), 5 deletions(-) create mode 100644 .changeset/parent-header-materialize.md diff --git a/.changeset/parent-header-materialize.md b/.changeset/parent-header-materialize.md new file mode 100644 index 0000000000..bc4771d4f8 --- /dev/null +++ b/.changeset/parent-header-materialize.md @@ -0,0 +1,66 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): materialise the master-detail header a `parent`-scoped predicate reads (#6457) + +`readonlyWhen: parent.status == 'paid'` and `requiredWhen: parent.status == 'sent'` +are documented **server** guarantees (#4889 / #4977 bound the `parent` root at the +write path so they are enforced by the engine and not only by the inline grid). +What was still storage-dependent is what that bound header CONTAINS. The engine +resolved it with a plain driver read and passed the row through as-is, so a driver +that returns only the columns it stored handed the predicate a header missing the +very key it reads. + +CEL is strict about missing keys, and the resulting fault is `No such key: status` +— **not** `Unknown variable: parent`, because `parent` IS bound. That misses +#4889's fail-closed carve-out and takes the ordinary fail-OPEN exit, so: + +- a `readonlyWhen` lock was **let through** and the frozen field was written; +- a `requiredWhen` requirement was **not enforced** and the record was accepted + with the field empty. + +Whether a declared lock or requirement enforced anything therefore depended on +which columns the driver happened to echo back — something the author who wrote +the predicate cannot see or control. This is #4953's trap on a different root: +the 2026-08-06 ruling made `record` / `previous` total at every server seam and +deliberately left `parent` out, as an ABSENT `parent` is the fail-closed signal. + +**The header is now made TOTAL over the MASTER object's declared fields** inside +`ObjectQL.resolveMasterDetailParent` and `resolveMasterDetailParents` — the only +place holding both the master's schema and the just-read row, so one change +serves both consumers and no strip/validator signature moves. It reuses the same +`materializeDeclaredFields` helper as every other server seam (#1871/#4649/#4953), +covers the single-id, bulk and insert paths, adds no query (the declared-field +table is a registry lookup, read once per batch), and copies each header before +materialising so the stored row never gains materialised nulls. + +**Verdicts move, in both directions, and only for a header that RESOLVED:** + +| header state | before | now | +|---|---|---| +| carries the key | evaluates per verdict | unchanged | +| resolved, key absent | fault ⇒ fail-OPEN (lock let through / requirement skipped) | evaluates — the key reads `null`, so locks lock and requirements enforce | +| unresolvable (`null`) | `readonlyWhen` LOCKED (#4889) / `requiredWhen` fail-OPEN (#4977) | **unchanged** | + +The bottom row is the one thing this change does not touch. Materialisation is +only ever applied to a header row that exists, so an unresolvable header still +leaves `parent` unbound, still faults as `Unknown variable: parent`, and is still +read as LOCKED for `readonlyWhen` — and still fail-OPEN for `requiredWhen`, the +deliberate #4977 asymmetry. The two cases stay distinguishable by fault channel, +and both are pinned. + +**Consequences worth knowing before writing a `parent`-scoped predicate**, the +same two `declared-fields.ts` states for `record`: + +- `has(parent.)` is now uniformly TRUE — a materialised `null` is + a PRESENT key holding null (CEL's own rule). `has()` guards against an + UNDECLARED key on the header, not against an empty value; test emptiness with + `parent.x != null`. +- Scope is the master's DECLARED fields only. A typo (`parent.stauts`) stays + unevaluable and therefore reportable rather than silently reading as `null`. + +If you have a `parent`-scoped `readonlyWhen` that was quietly failing open on a +sparse-returning driver, it starts locking; a `requiredWhen` in the same position +starts rejecting writes that leave the field empty. That is the declaration being +enforced as written. diff --git a/packages/objectql/src/engine-readonly-when-parent.test.ts b/packages/objectql/src/engine-readonly-when-parent.test.ts index 7d459c665d..80d8042e64 100644 --- a/packages/objectql/src/engine-readonly-when-parent.test.ts +++ b/packages/objectql/src/engine-readonly-when-parent.test.ts @@ -16,6 +16,14 @@ // The record-scoped contrast the issue drew — `showcase_invoice.tax_rate` with // `readonlyWhen: record.status == 'paid'`, which worked all along — is pinned in // the same file so a future change cannot fix one by breaking the other. +// +// #6457 extends this suite rather than starting its own, because what it changes +// is the same binding this file already owns: the header the engine resolves is +// now TOTAL over the MASTER object's declared fields, so a `parent.` +// predicate no longer depends on which columns the driver echoed back. Only the +// MIDDLE row of the issue's verdict table moves (resolved-but-sparse: fail-OPEN +// ⇒ evaluated); the fail-CLOSED line above (unresolvable ⇒ LOCKED) is asserted +// unchanged, on the same fixtures, in the same block. import { describe, it, expect, beforeEach } from 'vitest'; import { ObjectQL } from './engine.js'; @@ -126,18 +134,55 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { unit_price: { type: 'currency', readonlyWhen: "parent.status == 'paid'" }, // No lock at all — a line is still editable in the ways the author left open. description: { type: 'text' }, + // [#6457] The issue's own predicate — a lock that reads a header key the + // driver may or may not have echoed back. TRUE on a header carrying no + // `status`, which is what makes the middle-row flip observable. + locked_until_status: { type: 'text', readonlyWhen: 'parent.status == null' }, + // [#6457] The recorded CONSEQUENCE (#4953's, one root over): a + // materialised `null` is a PRESENT key, so `has()` over a DECLARED + // master field is uniformly TRUE. + has_guard: { type: 'text', readonlyWhen: 'has(parent.status)' }, + // [#6457] The #4649 line, unmoved: materialisation covers the master's + // DECLARED fields only, so an author typo on the header stays + // unevaluable — and therefore fail-OPEN — instead of reading as null. + typo_guard: { type: 'text', readonlyWhen: 'parent.stauts == null' }, }, } as any); storeFor('showcase_invoice').set('INV-1003', { id: 'INV-1003', invoice_number: 'INV-1003', status: 'paid', tax_rate: 8 }); storeFor('showcase_invoice').set('INV-1004', { id: 'INV-1004', invoice_number: 'INV-1004', status: 'draft', tax_rate: 8 }); + // [#6457] A header row the driver returned WITHOUT its `status` column — the + // MIDDLE row of the issue's verdict table. `status` is declared on the + // master; this row simply does not carry the key. + storeFor('showcase_invoice').set('INV-SPARSE', { id: 'INV-SPARSE', invoice_number: 'INV-SPARSE' }); storeFor('showcase_invoice_line').set('line_paid', { id: 'line_paid', invoice: 'INV-1003', quantity: 6, unit_price: 49.99, description: 'seat' }); storeFor('showcase_invoice_line').set('line_draft', { id: 'line_draft', invoice: 'INV-1004', quantity: 3, unit_price: 10, description: 'seat' }); + storeFor('showcase_invoice_line').set('line_sparse', { + id: 'line_sparse', invoice: 'INV-SPARSE', quantity: 1, unit_price: 5, + // A description OUTSIDE the 'seat' match set the bulk tests above use. + description: 'sparse', locked_until_status: 'kept', has_guard: 'kept', typo_guard: 'kept', + }); }); const line = (id: string) => storeFor('showcase_invoice_line').get(id); const invoice = (id: string) => storeFor('showcase_invoice').get(id); + /** Everything the engine warned during one write — the strip's own channel, + * which is how the fail-OPEN exit and the LOCKED exits are told apart. */ + async function warningsDuring(run: () => Promise): Promise { + const warns: string[] = []; + const base = (engine as any).logger; + (engine as any).logger = new Proxy(base, { + get: (t: any, k: string) => (k === 'warn' ? (m: string) => warns.push(String(m)) : t[k]), + }); + try { + await run(); + } finally { + (engine as any).logger = base; + } + return warns; + } + it('THE REGRESSION: a paid invoice\'s frozen line survives the PATCH that used to rewrite it', async () => { // Verbatim from the issue: PATCH {"quantity":9999,"unit_price":0.01} on a // line of the PAID invoice INV-1003 returned 200 and PERSISTED. @@ -219,4 +264,141 @@ describe('parent-scoped readonlyWhen is enforced server-side (#4889)', () => { await engine.update('showcase_invoice_line', { id: 'line_paid', description: 'note' }); expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(0); }); + + // ── #6457 — the resolved header is TOTAL over the MASTER's declared fields ── + // + // The issue's three-row verdict table, pinned end-to-end. Exactly one row + // moves. The other two — including #4889's fail-CLOSED line — are asserted on + // the same fixtures precisely so the move cannot quietly take them with it, + // and they are told apart by FAULT CHANNEL, not by the write's outcome: rows 2 + // and 3 can both end in "the field was not written", and only the warning says + // whether that was an evaluated verdict or a refusal to guess. + + it('ROW 1 (header carries the key): evaluates as it always did, verdict unchanged', async () => { + // INV-1003 carries `status: 'paid'`, so `parent.status == null` is FALSE and + // the field is writable. No fault ⇒ nothing on the fail-open channel. + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'line_paid', locked_until_status: 'written' })); + expect(line('line_paid')).toMatchObject({ locked_until_status: 'written' }); + expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + }); + + it('ROW 2 — THE FIX: a header missing the key now LOCKS instead of failing open', async () => { + // Before #6457 this was `No such key: status` on a BOUND `parent`, so + // `unknownVariableOf` did not match, the ordinary fail-OPEN exit ran, and + // the declared lock was let through. The header is now total over the + // master's declared fields, so `status` reads `null`, the predicate is TRUE, + // and the field is stripped. + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'line_sparse', locked_until_status: 'forged' })); + expect(line('line_sparse')).toMatchObject({ locked_until_status: 'kept' }); + // The verdict came from an EVALUATION: the fail-open exit is not on the + // channel at all… + expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + expect(warns.some((w) => w.includes("Field 'locked_until_status' is read-only (readonlyWhen)"))).toBe(true); + // …and it is NOT #4889's unbound-root exit either — `parent` IS bound here. + // This is the assertion that keeps ROW 2 and ROW 3 distinguishable. + expect(warns.some((w) => w.includes("reads 'parent'"))).toBe(false); + }); + + it('ROW 2: the strip is reported to the caller as `readonly_when` (#3407)', async () => { + const events: any[] = []; + await engine.update( + 'showcase_invoice_line', + { id: 'line_sparse', locked_until_status: 'forged' }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([ + { object: 'showcase_invoice_line', fields: ['locked_until_status'], reason: 'readonly_when' }, + ]); + }); + + it('ROW 3 (unresolvable header): still LOCKED, and still by the UNBOUND-ROOT exit (#4889)', async () => { + // The fail-closed line, unmoved and byte-identical: materialisation is only + // ever applied to a row that EXISTS, so a header that resolves to nothing + // still leaves `parent` unbound and still faults as `Unknown variable`. + storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', locked_until_status: 'kept' }); + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'orphan', locked_until_status: 'forged' })); + expect(line('orphan')).toMatchObject({ locked_until_status: 'kept' }); + expect(warns.some((w) => w.includes("reads 'parent'") && w.includes('LOCKED'))).toBe(true); + }); + + it('a sparse header that answers FALSE allows the change — by a VERDICT, not by a fault', async () => { + // `parent.status == 'paid'` over a header carrying no status now evaluates + // to FALSE. The write lands either way; what changed is why, and the why is + // what every other predicate on that header depends on. + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'line_sparse', quantity: 42 })); + expect(line('line_sparse')).toMatchObject({ quantity: 42 }); + expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(false); + }); + + it('CONSEQUENCE: `has(parent.)` is uniformly TRUE — it locks even on a sparse header', async () => { + // CEL's own rule, the same one #4953 recorded for `record`: a materialised + // `null` is a PRESENT key. `has()` guards against an UNDECLARED key on the + // header, not against an empty value — test emptiness with `!= null`. + await engine.update('showcase_invoice_line', { id: 'line_sparse', has_guard: 'forged' }); + expect(line('line_sparse')).toMatchObject({ has_guard: 'kept' }); + }); + + it('BOUNDARY: an UNDECLARED key on the header stays unevaluable — fail-OPEN (#4649 unmoved)', async () => { + // `parent.stauts` is a typo, not a sparse column. Materialising it would + // paper over the bug; it must stay reportable. + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'line_sparse', typo_guard: 'written' })); + expect(line('line_sparse')).toMatchObject({ typo_guard: 'written' }); + expect(warns.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + }); + + it('does NOT mutate the stored header row — the materialised copy stays local', async () => { + // The in-memory driver hands back the stored object BY REFERENCE, which is + // exactly how a materialisation leaks: the header would silently gain a + // `status: null` column that every later reader (and after-hooks) sees. + await engine.update('showcase_invoice_line', { id: 'line_sparse', locked_until_status: 'forged' }); + expect('status' in invoice('INV-SPARSE')).toBe(false); + expect(invoice('INV-SPARSE')).toEqual({ id: 'INV-SPARSE', invoice_number: 'INV-SPARSE' }); + }); + + it('BULK: the batch path materialises its headers too, per matched row', async () => { + await engine.update( + 'showcase_invoice_line', + { locked_until_status: 'forged' }, + { where: { description: 'sparse' }, multi: true } as any, + ); + expect(line('line_sparse')).toMatchObject({ locked_until_status: 'kept' }); + }); + + it('BULK: a batch under a header that DOES carry the key still writes', async () => { + await engine.update( + 'showcase_invoice_line', + { locked_until_status: 'written' }, + { where: { description: 'seat' }, multi: true } as any, + ); + expect(line('line_paid')).toMatchObject({ locked_until_status: 'written' }); + expect(line('line_draft')).toMatchObject({ locked_until_status: 'written' }); + }); + + it('BULK: an unresolvable header still LOCKS the batch (fail-CLOSED, bulk twin)', async () => { + storeFor('showcase_invoice_line').set('orphan_b', { + id: 'orphan_b', invoice: 'GONE', description: 'orphaned', locked_until_status: 'kept', + }); + await engine.update( + 'showcase_invoice_line', + { locked_until_status: 'forged' }, + { where: { description: 'orphaned' }, multi: true } as any, + ); + expect(line('orphan_b')).toMatchObject({ locked_until_status: 'kept' }); + }); + + it('costs no extra header read — materialisation is a registry lookup, not a query', async () => { + const reads: string[] = []; + const original = (engine as any).findOne.bind(engine); + (engine as any).findOne = async (name: string, q: any, o?: any) => { + reads.push(name); + return original(name, q, o); + }; + await engine.update('showcase_invoice_line', { id: 'line_sparse', locked_until_status: 'forged' }); + expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(1); + }); }); diff --git a/packages/objectql/src/engine-required-when-parent.test.ts b/packages/objectql/src/engine-required-when-parent.test.ts index 225d26b3ad..1ed5ca3cd9 100644 --- a/packages/objectql/src/engine-required-when-parent.test.ts +++ b/packages/objectql/src/engine-required-when-parent.test.ts @@ -133,16 +133,40 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { // (`invoice.object.ts` L212). note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, quantity: { type: 'number' }, + // [#6457] A requirement whose predicate is TRUE exactly when the header + // carries no `status`. On a header row the driver returned without that + // column this used to fault (`No such key: status` on a BOUND `parent`) + // and fail OPEN — the requirement enforced nothing. The mirror of + // #4889's hole: opposite direction, same `declared ≠ enforced`. + reason: { type: 'text', requiredWhen: 'parent.status == null' }, }, } as any, 'test-package'); storeFor('showcase_invoice').set('INV-SENT', { id: 'INV-SENT', invoice_number: 'INV-SENT', status: 'sent' }); storeFor('showcase_invoice').set('INV-DRAFT', { id: 'INV-DRAFT', invoice_number: 'INV-DRAFT', status: 'draft' }); + // [#6457] The MIDDLE row of the issue's table: `status` is declared on the + // master; this header row simply does not carry the key. + storeFor('showcase_invoice').set('INV-SPARSE', { id: 'INV-SPARSE', invoice_number: 'INV-SPARSE' }); }); const line = (id: string) => storeFor('showcase_invoice_line').get(id); const lines = () => [...storeFor('showcase_invoice_line').values()]; + /** The error a write threw, or a failure saying it was accepted. Callers + * assert on `.name` / `.fields` (field + code) — never a bare `toThrow()`. */ + async function rejectionOf(run: () => Promise): Promise { + let thrown: unknown; + let threw = false; + try { + await run(); + } catch (err) { + threw = true; + thrown = err; + } + expect(threw).toBe(true); + return thrown as any; + } + // ── parent scope HIT ────────────────────────────────────────────────────── it('THE GAP: rejects an INSERT that leaves the field empty under a Sent header', async () => { @@ -327,4 +351,133 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { await engine.insert('plain_line', { invoice: 'INV-SENT', quantity: 1 }); expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(0); }); + + // ── #6457 — the header is TOTAL over the MASTER's declared fields ────────── + // + // The `requiredWhen` half of the same change. #4977 bound the scope; what it + // could not fix from here is what the bound header CONTAINS — a driver that + // returns only the columns it stored hands over a header missing the very key + // the predicate reads, and a `requiredWhen` that faults is fail-OPEN, so the + // requirement silently enforces nothing. + // + // Note which line moves and which does not. The middle row (header RESOLVED + // but sparse) becomes evaluable and therefore ENFORCED. The bottom row (header + // UNRESOLVABLE) keeps #4977's deliberate fail-OPEN asymmetry with #4889 — + // option B was not taken here and is not taken here now either. + + /** Every warning the engine emitted during one write. */ + async function warningsDuring(run: () => Promise): Promise { + const warns: string[] = []; + const base = (engine as any).logger; + (engine as any).logger = new Proxy(base, { + get: (t: any, k: string) => (k === 'warn' ? (m: string) => warns.push(String(m)) : t[k]), + }); + try { + await run(); + } catch { + /* the caller asserts the outcome; this helper only collects the channel */ + } finally { + (engine as any).logger = base; + } + return warns; + } + + it('ROW 1 (header carries the key): evaluates as it always did — no false positive', async () => { + // INV-SENT carries `status: 'sent'`, so `parent.status == null` is FALSE and + // `reason` is not required. + const row = await engine.insert('showcase_invoice_line', { invoice: 'INV-SENT', description: 'seat', quantity: 1 }); + expect(row).toMatchObject({ description: 'seat' }); + }); + + it('ROW 2 — THE FIX: an INSERT under a sparse header is now REJECTED (was accepted)', async () => { + const err = await rejectionOf(() => + engine.insert('showcase_invoice_line', { invoice: 'INV-SPARSE', quantity: 1 })); + expect(err.name).toBe('ValidationError'); + expect(err.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + // Rejected BEFORE the driver, not rolled back after it. + expect(lines()).toHaveLength(0); + }); + + it('ROW 2: accepted once the required field IS supplied under the same sparse header', async () => { + const row = await engine.insert('showcase_invoice_line', { invoice: 'INV-SPARSE', reason: 'awaiting status', quantity: 1 }); + expect(row).toMatchObject({ reason: 'awaiting status' }); + }); + + it('ROW 2: an UPDATE that nulls the field under a sparse header is REJECTED', async () => { + storeFor('showcase_invoice_line').set('s1', { id: 's1', invoice: 'INV-SPARSE', reason: 'stated', quantity: 1 }); + const err = await rejectionOf(() => + engine.update('showcase_invoice_line', { id: 's1', reason: '' })); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + expect(line('s1')).toMatchObject({ reason: 'stated' }); + }); + + it('ROW 2: the refusal is an EVALUATED verdict — the unbound-root diagnostic is absent', async () => { + // What tells ROW 2 apart from ROW 3: here `parent` IS bound, so nothing on + // the channel names it as unbound. The write is refused because the + // predicate said so, not because it could not be asked. + const warns = await warningsDuring(() => + engine.insert('showcase_invoice_line', { invoice: 'INV-SPARSE', quantity: 1 })); + expect(warns.some((w) => /requiredWhen for 'reason' reads 'parent'/.test(w))).toBe(false); + expect(warns.some((w) => /requiredWhen for 'reason' failed to evaluate/.test(w))).toBe(false); + }); + + it('ROW 3 (unresolvable header): still FAIL-OPEN, still names the unbound root (#4977 asymmetry)', async () => { + // The line #4977 drew and this issue does NOT move: a header that resolves + // to nothing leaves `parent` unbound, the predicate is skipped, and the + // write lands with the field empty. Option B (422) stays not taken. + storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', reason: '', quantity: 1 }); + const warns = await warningsDuring(() => + engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 })); + expect(line('orphan')).toMatchObject({ quantity: 9, reason: '' }); + expect(warns.some((w) => /requiredWhen for 'reason' reads 'parent'/.test(w) && /NOT enforced/.test(w))).toBe(true); + }); + + it('ADR-0113 still holds on the newly-evaluable predicate: a legacy row may rest', async () => { + // The stored row ALREADY violates (sparse header, `reason` empty). Making + // the predicate evaluable must not brick unrelated edits to rows deployed + // before it started enforcing — enforcement tightens for NEW violations. + storeFor('showcase_invoice_line').set('legacy', { id: 'legacy', invoice: 'INV-SPARSE', reason: '', quantity: 1 }); + await engine.update('showcase_invoice_line', { id: 'legacy', quantity: 7 }); + expect(line('legacy')).toMatchObject({ quantity: 7, reason: '' }); + }); + + it('judges a REPOINT onto a sparse header against the header it lands on', async () => { + // A line with no `reason` under a Sent header (where none is required). + // Moving it under the sparse header turns the requirement ON, and the write + // that does the moving is the one that must be refused — the ADR-0113 + // pre-check must read the STORED row's own header, not the landing one. + storeFor('showcase_invoice_line').set('r1', { id: 'r1', invoice: 'INV-SENT', description: 'seat', reason: '', quantity: 1 }); + const err = await rejectionOf(() => + engine.update('showcase_invoice_line', { id: 'r1', invoice: 'INV-SPARSE' })); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + expect(line('r1')).toMatchObject({ invoice: 'INV-SENT' }); + }); + + it('BULK: the batch path materialises its headers too, per matched row', async () => { + storeFor('showcase_invoice_line').set('b_sparse', { id: 'b_sparse', invoice: 'INV-SPARSE', reason: 'stated', quantity: 3 }); + storeFor('showcase_invoice_line').set('b_sent', { id: 'b_sent', invoice: 'INV-SENT', description: 'seat', quantity: 3 }); + const err = await rejectionOf(() => + engine.update('showcase_invoice_line', { reason: '' }, { where: { quantity: 3 }, multi: true } as any)); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + // Rejected before anything was written — neither matched row moved. + expect(line('b_sparse')).toMatchObject({ reason: 'stated' }); + }); + + it('BULK INSERT: one batched header read still materialises every header', async () => { + // The insert path is the bulk resolver with `data: null` — each row supplies + // its own FK. One row lands under a header carrying `status`, one under the + // sparse header, and only the second is required to carry `reason`. + const err = await rejectionOf(() => + engine.insert('showcase_invoice_line', [ + { invoice: 'INV-SENT', description: 'seat', quantity: 1 }, + { invoice: 'INV-SPARSE', quantity: 1 }, + ] as any)); + expect(err.fields).toContainEqual(expect.objectContaining({ field: 'reason', code: 'required' })); + }); + + it('does NOT mutate the stored header row — the materialised copy stays local', async () => { + await engine.insert('showcase_invoice_line', { invoice: 'INV-SPARSE', reason: 'stated', quantity: 1 }); + expect('status' in storeFor('showcase_invoice').get('INV-SPARSE')).toBe(false); + }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 946bbeae3f..88f4d611c4 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -125,6 +125,10 @@ import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, Validat import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, hasParentScopedRequiredWhen, stripReadonlyFields, stripRuntimeOwnedFields } from './validation/rule-validator.js'; import { resolveMasterDetailRelation } from './master-detail.js'; +// [#6457] The master-detail header a `parent`-scoped predicate reads is made +// total over the MASTER's declared fields before it leaves this engine — the +// same helper every other server seam materialises with (#1871/#4649/#4953). +import { materializeDeclaredFields } from './declared-fields.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; import { resolveEngineDeleteDispatch, @@ -3418,6 +3422,10 @@ export class ObjectQL implements IObjectQLEngine { * `null` on any failure — no relation, no id, header gone, read threw. It is * NOT read as "unlocked": an unresolved binding leaves `parent` unbound, and * `isReadonlyWhenLocked` treats a predicate that needs it as LOCKED. + * + * [#6457] A header that IS resolved is handed over TOTAL over the MASTER + * object's declared fields — see {@link materializeParentHeader} for why that + * had to happen here and nowhere else, and for the verdict table it moves. */ private async resolveMasterDetailParent( schema: any, @@ -3430,7 +3438,9 @@ export class ObjectQL implements IObjectQLEngine { if (parentId == null) return null; try { const row = await this.findOne(rel.master, { where: { id: parentId }, context: { isSystem: true } } as any); - return (row as Record) ?? null; + // `null` stays `null` — the fail-CLOSED signal (#4889) is the ABSENCE of + // the binding, and materialising a row we do not have would destroy it. + return row == null ? null : this.materializeParentHeader(rel.master, row as Record); } catch (err) { this.logger?.warn?.('readonlyWhen parent lookup failed — parent stays unbound', { object: rel.master, id: parentId, error: err, @@ -3451,6 +3461,11 @@ export class ObjectQL implements IObjectQLEngine { * passed as `null` and each inserted row supplies its own FK, so * `masterIdOf(fk, null, row)` reads `row[fk]` and the batch costs one header * read for the whole `insert()` call. + * + * [#6457] Every header this resolves is materialised over the MASTER's + * declared fields, exactly as the single-id twin does — the declared-field + * table is read ONCE for the batch, not per row. A row this map has no entry + * for still answers `null` (unbound, fail-CLOSED for `readonlyWhen`). */ private async resolveMasterDetailParents( schema: any, @@ -3472,8 +3487,13 @@ export class ObjectQL implements IObjectQLEngine { where: { id: { $in: [...ids] } }, context: { isSystem: true }, } as any) as Array>; + // [#6457] One declared-field lookup for the whole batch, then one shallow + // copy per header. A master the registry does not know leaves `fields` + // undefined and every header passes through untouched — see + // {@link materializeParentHeader}. + const masterFields = this.masterDeclaredFields(rel.master); for (const row of Array.isArray(rows) ? rows : []) { - if (row?.id != null) byId.set(String(row.id), row); + if (row?.id != null) byId.set(String(row.id), materializeDeclaredFields({ ...row }, masterFields)); } } catch (err) { this.logger?.warn?.('readonlyWhen parent lookup failed — parent stays unbound', { @@ -3487,6 +3507,73 @@ export class ObjectQL implements IObjectQLEngine { }; } + /** + * [#6457] Make a resolved master-detail header TOTAL over the MASTER + * object's declared fields, so a `parent.` predicate is evaluable + * whatever subset of columns the driver echoed back. + * + * ## The hole this closes + * + * #4953 made the `record` / `previous` roots total at every server seam + * ({@link ./declared-fields.js#materializeDeclaredFields}); the 2026-08-06 + * ruling deliberately left `parent` out, because `parent` is a row of ANOTHER + * object and its ABSENCE is #4889's fail-closed signal. What that left behind + * is the same trap one root over — visible as a THREE-row table, of which + * only the middle row moves here: + * + * | header state | `readonlyWhen: parent.status == null` | before | now | + * |---|---|---|---| + * | carries `status` | evaluates | locks per verdict | unchanged | + * | resolved, no `status` key | `No such key: status` — `parent` IS bound, so `unknownVariableOf` does not match ⇒ ordinary fail-OPEN | **declared lock let through** | evaluates (`status` reads `null`) | + * | unresolvable (`null`) | `Unknown variable: parent` | LOCKED (#4889) | LOCKED — unchanged | + * + * The middle row is the bug: whether a declared lock enforced depended on + * which columns a driver happened to return, which is not something an author + * can see or control. `requiredWhen` (#4977) shares the binding and had the + * mirror of it — fail-open there means the requirement is simply not + * enforced. One materialisation serves both consumers. + * + * ## Why HERE, and not in the strip / the evaluator + * + * `stripReadonlyWhenFields*` and `evaluateValidationRules` are pure functions + * over what they are handed; they hold the DETAIL object's field table and + * have no way to reach the MASTER's — threading a second field table through + * their signatures would put the master's schema in four call sites' hands to + * serve one binding. The engine already holds both the registry and the + * just-read header at these two seams, so the header arrives at those + * functions already total and their signatures do not move. + * + * ## The fail-CLOSED line is preserved EXACTLY (#4889) + * + * This function is only ever reached with a row IN HAND — the persisted-state + * precondition `declared-fields.ts` states. An UNRESOLVABLE header still + * returns `null` from the resolvers above, still leaves `parent` unbound, + * still faults as `Unknown variable: parent`, and is still read as LOCKED. The + * two cases stay distinguishable by construction: absence is decided before + * this function is called, materialisation only ever applies to a row that + * exists. + * + * A master the registry cannot resolve yields no field table, and the header + * passes through unchanged — sparse, i.e. exactly the pre-#6457 behaviour. + * That is the honest answer: without the declared shape we cannot know which + * absent keys are fields and which would be fabrication. + * + * COPIED before materialising, like every other caller: the row is what + * `findOne`/`find` returned and may be observed elsewhere; it must not gain + * materialised nulls behind its reader's back. + */ + private materializeParentHeader(master: string, row: Record): Record { + return materializeDeclaredFields({ ...row }, this.masterDeclaredFields(master)); + } + + /** The MASTER object's declared-field table, or `undefined` when the registry + * does not know it (see {@link materializeParentHeader}). */ + private masterDeclaredFields(master: string): Record | undefined { + const schema = this._registry.getObject(master) as { fields?: Record } | undefined; + const fields = schema?.fields; + return fields && typeof fields === 'object' ? fields : undefined; + } + /** * [#4551] Report stored references that resolve to nothing. **Read-only** — * this issues no writes at all. diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index fe2d020c13..a608a60f67 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -623,7 +623,7 @@ describe('readonlyWhen binds a TOTAL record (#4953)', () => { } as never)).toThrow(/could not be evaluated/); }); - it('does not disturb the #4889 parent binding: unbound root still LOCKS, parent stays unmaterialised', () => { + it('does not disturb the #4889 parent binding: unbound root still LOCKS, and this function still materialises no header', () => { // `parent` is a row of ANOTHER object — this function has no declared-field // list for it — and an ABSENT parent is the signal #4889 depends on. const warnings: string[] = []; @@ -631,8 +631,26 @@ describe('readonlyWhen binds a TOTAL record (#4953)', () => { warn: (m: string) => warnings.push(m), } as never)).toEqual({}); expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('LOCKED'))).toBe(true); - // A parent that IS bound but does not carry the key stays a fault (no - // materialisation of the header): fail-open, the change goes through. + // [#6457 — RE-ANNOTATED, verdict deliberately NOT flipped here.] + // + // A parent that IS bound but does not carry the key is still a fault at + // THIS seam: fail-open, the change goes through. That was the hole #6457 + // closed, and the sentence below is the reason this assertion nonetheless + // stays exactly as PR #6454 wrote it. + // + // #6457's ruling materialises the header INSIDE the engine's + // `resolveMasterDetailParent(s)`, using the MASTER object's declared-field + // table — the one thing this pure function does not and cannot have. So the + // strip's own contract is unchanged (its signature never grew a second field + // table), and a caller that hands it a genuinely sparse header still gets + // the fail-open answer pinned here. What changed is that the ENGINE no + // longer hands it one. + // + // The moved verdict therefore lives where the change lives, and is pinned + // end-to-end against a real driver in `engine-readonly-when-parent.test.ts` + // ("ROW 2 — THE FIX"), with its `requiredWhen` mirror in + // `engine-required-when-parent.test.ts`. Read the two together: this one + // says the strip did not move, that one says the write path did. const warnings2: string[] = []; expect(stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, { id: 'l1', invoice: 'inv1' }, { warn: (m: string) => warnings2.push(m), diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index d4852cf6ec..d7fd06c706 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -434,6 +434,17 @@ export type ParentBinding = Record | null | undefined; * is a row of a DIFFERENT object whose declared fields this function does not * have. * + * [#6457] That last clause is why the header IS materialised — just not here. + * A RESOLVED-but-sparse header reproduced this same trap one root over (the + * fault is `No such key`, not `Unknown variable`, because `parent` IS bound ⇒ + * ordinary fail-OPEN ⇒ the declared lock is let through), and the fix went to + * the only place holding both the master's schema and the just-read header: + * `ObjectQL.resolveMasterDetailParent(s)` (`engine.ts#materializeParentHeader`), + * which serves this seam and the `requiredWhen` one below from one resolution. + * This function's own contract is unchanged — hand it a sparse header and it + * still fails open — and the ABSENT-parent signal above is untouched, because + * materialisation is only ever applied to a header row that EXISTS. + * * ## Consequences, both directions (measured, not asserted) * * A total record makes a predicate that used to fault evaluate for real, so