From e001593d81cd6fd6fc216d5830073ac012e39e16 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 18:32:40 +0000 Subject: [PATCH 1/2] fix(objectql,lint): bind the parent scope for `requiredWhen`, and gate it at build time (#4977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4889 closed the `parent`-scope hole for `readonlyWhen`. `requiredWhen` sits on the same field, is evaluated by the same module, and still had it: nothing bound `parent`, so `requiredWhen: parent.status == 'sent'` faulted, took the fail-open branch, and the write landed with the field empty. Per the maintainer's 2026-08-06 ruling, A + C and deliberately NOT symmetric with #4889: - A: the engine resolves the master-detail header with #4889's own `resolveMasterDetailParent(s)` and passes it to the evaluator on insert, single-id update and bulk update. Evaluation semantics are unchanged — an unevaluable predicate (unresolvable header included) stays fail-OPEN. Option B (422) was explicitly not taken; it is reserved for ADR-0058 D5. - C: `@objectstack/lint`'s parent-scope gate, previously scoped to `readonlyWhen`, now judges `requiredWhen` too. One gate, two consequence clauses: the runtimes fail in opposite directions, so a shared message would prescribe the wrong fix. `previousParent` is added for the ADR-0113 non-regression pre-check, which asks about the STORED row and therefore needs the header that row hung off — binding the landing header there would read a repoint onto a Sent header as a pre-existing violation and let it rest. The object-level `script` / `cross_field` rules sharing this evaluation site do NOT get the root: they are fail-CLOSED since #4649, and binding one there would flip writes they reject today into accepted ones. Pinned by test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .changeset/required-when-parent-scope.md | 43 +++ .../lint/src/validate-expressions.test.ts | 89 ++++- packages/lint/src/validate-expressions.ts | 46 ++- .../src/engine-required-when-parent.test.ts | 330 ++++++++++++++++++ packages/objectql/src/engine.ts | 94 ++++- packages/objectql/src/master-detail.ts | 10 +- .../src/validation/rule-validator.test.ts | 174 +++++++++ .../objectql/src/validation/rule-validator.ts | 132 ++++++- 8 files changed, 889 insertions(+), 29 deletions(-) create mode 100644 .changeset/required-when-parent-scope.md create mode 100644 packages/objectql/src/engine-required-when-parent.test.ts diff --git a/.changeset/required-when-parent-scope.md b/.changeset/required-when-parent-scope.md new file mode 100644 index 0000000000..4655b2f03c --- /dev/null +++ b/.changeset/required-when-parent-scope.md @@ -0,0 +1,43 @@ +--- +'@objectstack/objectql': patch +'@objectstack/lint': patch +--- + +fix(objectql,lint): 服务端为 `requiredWhen` 绑定 parent 作用域,并把构建期硬闸扩到同一格 + +`readonlyWhen` 的 parent 作用域洞在 #4889 已经补上;同一个字段上、由同一个求值器处理的 +`requiredWhen` 隔一个槽位还漏着。detail 对象上声明的 +``requiredWhen: P`parent.status == 'sent'` `` ——「表头一旦 Sent,每一行都必须填写说明」—— +只在内联表格里被求值,服务端从来只绑 `record` / `previous`,谓词直接 fault 走 fail-open +分支,写入带着空字段落库,API 还回 200。 + +注意它与 #4889 是**镜像**而不是同一种故障:`readonlyWhen` fail-open 是**写进了本该冻结的字段**, +`requiredWhen` fail-open 是**收下了本该被拒的记录**。两者都是同一处声明点上的 `declared ≠ enforced` +(PD #10)。 + +本次按维护者 2026-08-06 的裁决落 A + C 两条,**刻意不对称于 #4889**: + +- **A —— 绑作用域,求值语义不动。** 引擎用 #4889 已经建好的 + `resolveMasterDetailParent(s)` 解析主表头行并传入求值器,insert / 单 id update / + bulk update 三个调用点都覆盖。**不可求值仍然 fail-open**(记日志、跳过、放行): + 表头此刻读不到就 422 掉一次本来合法的写入,比 `readonlyWhen` 那边「拒掉一个字段」响得多。 + 这是 issue 的 B 案,明确不做,留给 ADR-0058 D5 下一次复审。 +- **C —— 改在构建期拦。** `@objectstack/lint` 的 parent 作用域闸原本只盖 `readonlyWhen`, + 现在同样判 `requiredWhen`:对象没有恰好一个 `master_detail` 关系时,`parent` 不是元数据 + 陈述过的事实,声明直接判 error。两格共用同一个闸,但**报错文案不同** —— 两边运行时的失败 + 方向相反(`readonlyWhen` fail-closed ⇒ 字段永远写不进;`requiredWhen` fail-open ⇒ 要求 + 永远不生效),文案指错了就等于给了相反的修法。运行时敢保持 fail-open,正是因为这道闸 + 拦住了那条会无声烂掉的声明。 + +同一次改动里补了 ADR-0113 非回归判定在 parent 作用域下的正确输入:「存量行本来就违规吗」问的是 +**写入前**那一行的状态,而它挂的是**旧**表头。改挂(repoint)到另一个主表时,若把落地表头也 +喂给这个前置判定,就会把「移到 Sent 表头之下」读成既有违规而放行 —— 正是本 issue 要堵的那个 +收下动作,只是换了个入口。因此求值器新增 `previousParent`,仅在载荷确实改挂时由引擎解析, +其余情况沿用同一行、不多付一次读。 + +对象级 `script` / `cross_field` 规则共用这个求值调用点,自 #4649 起对不可求值谓词是 +**fail-closed**,本次**没有**给它们绑新根 —— 绑了会把它们今天拒掉的写入翻成接受。这条由 pin +测试钉住(#4972 当初把本改动挡在范围外,就是为了这个爆炸半径)。 + +仓内暂无 app 声明 parent 作用域的 `requiredWhen`(showcase 的 invoice line 用的是行作用域的 +`record.quantity >= 100`),所以这是补潜伏缺口,不改变任何现有 app 的写入行为。 diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 0de43b42ea..b8d77b663a 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -539,16 +539,101 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { })).toHaveLength(0); }); - it('is scoped to `readonlyWhen` — `requiredWhen`/`visibleWhen` verdicts are unchanged', () => { + // #4977 — this pin used to read "scoped to `readonlyWhen`, `requiredWhen` + // verdicts unchanged" over a fixture declaring BOTH. It pinned exactly the + // limb that issue removes, so it is replaced rather than re-spelled: the + // `requiredWhen` half moved to its own cases below, and what survives here + // is the genuinely-unchanged slot, `visibleWhen`, on a fixture that + // declares only that. + it('is still scoped OUT of `visibleWhen` — that verdict is unchanged', () => { expect(parentScopeIssues({ name: 'orphan_line', fields: { - qty: { type: 'number', requiredWhen: "parent.status == 'paid'", visibleWhen: "parent.status == 'paid'" }, + qty: { type: 'number', visibleWhen: "parent.status == 'paid'" }, }, })).toHaveLength(0); }); }); + // #4977 — the same gate, extended to the slot the same issue gave a server + // `parent` binding. `requiredWhen` stays FAIL-OPEN at runtime, so this build + // gate is the only thing that stops an unbindable declaration from shipping + // and enforcing nothing forever — which is why the message must name that + // consequence and not `readonlyWhen`'s opposite one. + describe('parent-scoped `requiredWhen` needs a resolvable master (#4977)', () => { + const parentScopeIssues = (obj: Record) => + validateStackExpressions({ objects: [obj] }).filter((i) => /reads `parent`/.test(i.message)); + + it('rejects it on an object that declares NO master_detail relationship', () => { + const issues = parentScopeIssues({ + name: 'orphan_line', + fields: { + inv: { type: 'lookup', reference: 'inv' }, // a lookup is not a master + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + }, + }); + expect(issues).toHaveLength(1); + expect(issues[0]!.severity).toBe('error'); + expect(issues[0]!.where).toMatch(/field 'description' requiredWhen/); + expect(issues[0]!.message).toMatch(/declares no `master_detail` relationships/); + // The CONSEQUENCE clause is what separates this from its `readonlyWhen` + // twin: fail-open there, fail-closed here, opposite fixes. + expect(issues[0]!.message).toMatch(/the requirement would never be enforced/); + expect(issues[0]!.message).not.toMatch(/locked on every write/); + }); + + it('rejects it when TWO masters leave "the parent" unstated', () => { + const issues = parentScopeIssues({ + name: 'junction', + fields: { + left: { type: 'master_detail', reference: 'a' }, + right: { type: 'master_detail', reference: 'b' }, + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + }, + }); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toMatch(/declares 2 `master_detail` relationships/); + }); + + it('ACCEPTS it on a real detail object — the showcase shape must stay lintable', () => { + expect(parentScopeIssues({ + name: 'showcase_invoice_line', + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice' }, + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + }, + })).toHaveLength(0); + }); + + it('does not fire on a field named `parent_id` or a `parent` string literal', () => { + expect(parentScopeIssues({ + name: 'node', + fields: { + parent_id: { type: 'text' }, + kind: { type: 'text' }, + a: { type: 'text', requiredWhen: "record.parent_id != ''" }, + b: { type: 'text', requiredWhen: "record.kind == 'parent'" }, + }, + })).toHaveLength(0); + }); + + it('reports BOTH slots when one field declares two unbindable predicates', () => { + const issues = parentScopeIssues({ + name: 'orphan_line', + fields: { + qty: { + type: 'number', + readonlyWhen: "parent.status == 'paid'", + requiredWhen: "parent.status == 'sent'", + }, + }, + }); + expect(issues).toHaveLength(2); + expect(issues.map((i) => i.where.replace(/.* field 'qty' /, '')).sort()) + .toEqual(['readonlyWhen', 'requiredWhen']); + }); + }); + it('flags a bare-field sharing-rule condition', () => { const issues = validateStackExpressions({ objects: [{ name: 'crm_account', fields: { region: { type: 'text' } } }], diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 507a7138c7..cd3752db32 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -583,22 +583,48 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // at build time, so it is decided here rather than discovered as an // unwritable field in production — PD #12, declared rather than guessed. // - // Scoped to `readonlyWhen` on purpose: it is the one field predicate the - // server enforces as a write-path LOCK, so it is the one whose unbindable - // scope changes what lands in the database. `requiredWhen` / - // `visibleWhen` keep their existing verdicts untouched. - const roWhenSource = celSourceOf(f.readonlyWhen); - if (masters !== 1 && roWhenSource && readsParentRoot(roWhenSource)) { + // [#4977] Extended to `requiredWhen`, which the server enforces from the + // same declaration site through the same evaluator and which gained its + // own `parent` binding in the same issue. The two slots ask ONE question — + // "is `parent` a fact this object's metadata states?" — so they share one + // gate rather than growing a second copy of `masterDetailCount` + + // `readsParentRoot`. + // + // What they do NOT share is the CONSEQUENCE, and the message has to name + // the right one or it prescribes the wrong fix (the same reason #4811's + // null-guard gate passes its outcome in explicitly instead of inferring + // it). The two runtimes fail in OPPOSITE directions on an unbindable + // `parent`: `readonlyWhen` fails CLOSED (#4889 — an unbound scope root + // resolves to LOCKED, so the field becomes unwritable forever), while + // `requiredWhen` stays fail-OPEN (#4977's ruling deliberately did not copy + // the carve-out), so the requirement silently enforces NOTHING. This gate + // is why fail-open is affordable there: the declaration that would rot + // unnoticed at runtime cannot ship in the first place. + // + // `conditionalRequired` (retired alias) and `visibleWhen` (no + // server-enforced `parent` binding of its own) keep their verdicts + // untouched. + // Destructured in the loop head on purpose: both predicate slots stay + // LITERAL member reads (`f.readonlyWhen` / `f.requiredWhen`) rather than a + // computed `f[key]`, so the #5017 meta-test's source scan still sees every + // key this rule reads and can still check it against `FieldSchema`. An + // indexed read here would have disarmed that scan silently. + for (const [slot, raw, consequence] of [ + ['readonlyWhen', f.readonlyWhen, `the field would be locked on every write`], + ['requiredWhen', f.requiredWhen, `the requirement would never be enforced — the predicate faults, the server logs and skips it, and the field stays optional in the database`], + ] as const) { + const source = celSourceOf(raw); + if (masters === 1 || !source || !readsParentRoot(source)) continue; issues.push({ - where: `object '${objectName}' · field '${fname}' readonlyWhen`, + where: `object '${objectName}' · field '${fname}' ${slot}`, message: - `\`readonlyWhen\` reads \`parent\`, but object '${objectName}' declares ` + + `\`${slot}\` reads \`parent\`, but object '${objectName}' declares ` + `${masters === 0 ? 'no' : `${masters}`} \`master_detail\` relationship${masters === 1 ? '' : 's'} — ` + - `so the server has no header record to bind as \`parent\` and the field would be locked on every write. ` + + `so the server has no header record to bind as \`parent\` and ${consequence}. ` + (masters === 0 ? `Declare the owning relationship as \`Field.masterDetail('')\`, or rewrite the predicate against \`record\`.` : `\`parent\` needs exactly one master; name the header explicitly through \`record.\` state instead, or model the extra relationship as a \`lookup\`.`), - source: roWhenSource, + source, severity: 'error', }); } diff --git a/packages/objectql/src/engine-required-when-parent.test.ts b/packages/objectql/src/engine-required-when-parent.test.ts new file mode 100644 index 0000000000..a99177a576 --- /dev/null +++ b/packages/objectql/src/engine-required-when-parent.test.ts @@ -0,0 +1,330 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4977 — PARENT-scoped `requiredWhen` is a SERVER guarantee. +// +// The mirror of #4889, one slot over on the same field. `requiredWhen: +// parent.status == 'sent'` on a detail object — "once the header invoice is +// Sent, every line must carry a description" — was evaluated by the inline grid +// and was a NO-OP on the server: nothing bound `parent`, the predicate faulted, +// the fail-open branch skipped the rule, and the write landed with the field +// empty. +// +// Note the failure mode is the MIRROR of #4889's, not the same one: a +// `readonlyWhen` failing open WROTE a field that should have been frozen; a +// `requiredWhen` failing open ACCEPTS a record that should have been rejected. +// +// The maintainer ruled A+C (2026-08-06): bind the scope, keep the evaluation +// semantics FAIL-OPEN, and catch the unbindable declaration at build time +// instead (`@objectstack/lint`). Option B — 422 on an unresolvable header — was +// explicitly NOT taken, so the "header cannot be read" case below asserts the +// write is ACCEPTED. That is the deliberate asymmetry with #4889's fail-CLOSED +// twin, and it is pinned here so nobody "fixes" one into the other by accident. +// +// Driven end-to-end through the real engine + a real driver, not through the +// evaluator in isolation (PD #10: a `case` label is not enforcement — check the +// CALL SITE). The driver fixture is #4889's, unchanged. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const checkOp = (value: any, cond: any): boolean => { + if (cond === null || typeof cond !== 'object' || Array.isArray(cond) || cond instanceof Date) { + return value === cond; + } + return Object.entries(cond).every(([op, target]: [string, any]) => { + switch (op) { + case '$eq': return value === target; + case '$ne': return value !== target; + case '$in': return Array.isArray(target) && target.includes(value); + default: return true; + } + }); + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => { + if (k === '$and') return (v as any[]).every((w) => matches(row, w)); + if (k === '$or') return (v as any[]).some((w) => matches(row, w)); + if (k === '$not') return !matches(row, v); + return checkOp(row?.[k], v); + }); + }; + let n = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + + beforeEach(async () => { + engine = new ObjectQL(); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ + name: 'showcase_invoice', + fields: { + invoice_number: { type: 'text' }, + status: { type: 'select', options: [{ value: 'draft' }, { value: 'sent' }, { value: 'paid' }] }, + // The RECORD-scoped contrast on the header itself + // (`invoice.object.ts` L153) — worked all along, must keep working. + paid_on: { type: 'date', requiredWhen: "record.status == 'paid'" }, + }, + } as any); + engine.registry.registerObject({ + name: 'showcase_invoice_line', + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + // THE SUBJECT: the issue's own example — "once the header is Sent, + // every line must carry a description". + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + // The ROW-scoped contrast the showcase actually ships today + // (`invoice.object.ts` L212). + note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, + quantity: { type: 'number' }, + }, + } as any); + + 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' }); + }); + + const line = (id: string) => storeFor('showcase_invoice_line').get(id); + const lines = () => [...storeFor('showcase_invoice_line').values()]; + + // ── parent scope HIT ────────────────────────────────────────────────────── + + it('THE GAP: rejects an INSERT that leaves the field empty under a Sent header', async () => { + await expect( + engine.insert('showcase_invoice_line', { invoice: 'INV-SENT', quantity: 1 }), + ).rejects.toThrow(/description/i); + // Nothing was written — the rejection is before the driver, not after. + expect(lines()).toHaveLength(0); + }); + + it('rejects an UPDATE that nulls the field while the header is Sent', async () => { + storeFor('showcase_invoice_line').set('l1', { id: 'l1', invoice: 'INV-SENT', description: 'seat', quantity: 1 }); + await expect( + engine.update('showcase_invoice_line', { id: 'l1', description: '' }), + ).rejects.toThrow(/description/i); + expect(line('l1')).toMatchObject({ description: 'seat' }); + }); + + // ── parent scope MISS ───────────────────────────────────────────────────── + + it('does NOT require the field when the header is still Draft (no false positives)', async () => { + const row = await engine.insert('showcase_invoice_line', { invoice: 'INV-DRAFT', quantity: 1 }); + expect(row).toMatchObject({ invoice: 'INV-DRAFT' }); + await engine.update('showcase_invoice_line', { id: (row as any).id, quantity: 2 }); + expect(line((row as any).id)).toMatchObject({ quantity: 2 }); + }); + + it('accepts the write once the field IS supplied under a Sent header', async () => { + const row = await engine.insert('showcase_invoice_line', { invoice: 'INV-SENT', description: 'seat', quantity: 1 }); + expect(row).toMatchObject({ description: 'seat' }); + }); + + // ── parent MISSING — fail-OPEN (the deliberate asymmetry with #4889) ────── + + it('is FAIL-OPEN when the header cannot be resolved (option B was NOT taken)', async () => { + // A stored row whose header is gone — #4889's own orphan fixture, and the + // only spelling that reaches this branch (a DANGLING FK in an insert + // payload is refused earlier by the #4441 reference guard, so the header + // can only go missing under a row that already exists). + // + // #4889's `readonlyWhen` twin treats an unbound `parent` as LOCKED. The + // ruling on #4977 explicitly declined the symmetric answer (reject the + // write), so here the requirement is skipped and the write lands — even + // though `description` is empty and the predicate, could it have been + // evaluated, might well have said it is required. + storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', description: '', quantity: 1 }); + await engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 }); + expect(line('orphan')).toMatchObject({ quantity: 9, description: '' }); + }); + + it('names the unbound ROOT in the skip diagnostic (fail-open, but not silent)', async () => { + 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]), + }); + storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', description: '', quantity: 1 }); + await engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9 }); + expect(warns.some((w) => /requiredWhen for 'description' reads 'parent'/.test(w))).toBe(true); + expect(warns.some((w) => /NOT enforced/.test(w))).toBe(true); + }); + + // ── repoint ─────────────────────────────────────────────────────────────── + + it('judges a REPOINT against the master the write lands on, not the one it leaves', async () => { + // A line that legitimately has no description under a DRAFT header. Moving + // it onto the SENT invoice turns the requirement on, and the write that + // does the moving is the one that must be rejected — the ADR-0113 + // pre-check must not read this as a pre-existing violation that may rest. + storeFor('showcase_invoice_line').set('l2', { id: 'l2', invoice: 'INV-DRAFT', description: '', quantity: 1 }); + await expect( + engine.update('showcase_invoice_line', { id: 'l2', invoice: 'INV-SENT' }), + ).rejects.toThrow(/description/i); + expect(line('l2')).toMatchObject({ invoice: 'INV-DRAFT' }); + }); + + it('lets a legacy row rest: an unrelated edit under an already-Sent header is not blocked (ADR-0113)', async () => { + // Stored state ALREADY violates (header sent, description empty). ADR-0113 + // non-regression: a write that leaves the violation in place is allowed — + // enforcement tightens for new violations, it does not brick deployed rows. + storeFor('showcase_invoice_line').set('l3', { id: 'l3', invoice: 'INV-SENT', description: '', quantity: 1 }); + await engine.update('showcase_invoice_line', { id: 'l3', quantity: 7 }); + expect(line('l3')).toMatchObject({ quantity: 7, description: '' }); + }); + + // ── bulk ────────────────────────────────────────────────────────────────── + + it('enforces the requirement on the BULK path too, per matched row (#3106 shape)', async () => { + storeFor('showcase_invoice_line').set('b_sent', { id: 'b_sent', invoice: 'INV-SENT', description: 'seat', quantity: 1 }); + storeFor('showcase_invoice_line').set('b_draft', { id: 'b_draft', invoice: 'INV-DRAFT', description: 'seat', quantity: 1 }); + // One payload, N priors: nulling the description violates for the row whose + // header is Sent, so the WHOLE batch is rejected before anything is written. + await expect( + engine.update('showcase_invoice_line', { description: '' }, { where: { quantity: 1 }, multi: true } as any), + ).rejects.toThrow(/description/i); + expect(line('b_sent')).toMatchObject({ description: 'seat' }); + expect(line('b_draft')).toMatchObject({ description: 'seat' }); + }); + + it('leaves a bulk edit that touches only Draft-header rows alone', async () => { + storeFor('showcase_invoice_line').set('b_draft', { id: 'b_draft', invoice: 'INV-DRAFT', description: 'seat', quantity: 1 }); + await engine.update('showcase_invoice_line', { description: '' }, { where: { invoice: 'INV-DRAFT' }, multi: true } as any); + expect(line('b_draft')).toMatchObject({ description: '' }); + }); + + // ── contrasts that must not move ────────────────────────────────────────── + + it('CONTRAST: the ROW-scoped requiredWhen on the same object still works', async () => { + await expect( + engine.insert('showcase_invoice_line', { invoice: 'INV-DRAFT', quantity: 500 }), + ).rejects.toThrow(/note/i); + const row = await engine.insert('showcase_invoice_line', { invoice: 'INV-DRAFT', quantity: 500, note: 'bulk order' }); + expect(row).toMatchObject({ note: 'bulk order' }); + }); + + it('CONTRAST: the record-scoped requiredWhen on the HEADER object still works', async () => { + await expect( + engine.update('showcase_invoice', { id: 'INV-DRAFT', status: 'paid' }), + ).rejects.toThrow(/paid_on/i); + }); + + // ── blast radius: the object-level rules at the SAME evaluation site ────── + + it('does NOT bind `parent` for object-level rules — they stay fail-CLOSED (#4649)', async () => { + // The reason #4889 left this change out of PR #4972: `script` / + // `cross_field` rules share this evaluation call and have REJECTED an + // unevaluable predicate since #4649. Binding a new root for them would flip + // writes they refuse today into accepted ones. The binding is scoped to the + // field-level `requiredWhen` block precisely so this verdict does not move. + engine.registry.registerObject({ + name: 'scoped_line', + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + }, + validations: [{ + type: 'script', + name: 'no_parent_root_here', + message: 'object-level rules do not get a parent binding', + condition: "parent.status == 'sent'", + }], + } as any); + // The header resolves fine (the field predicate below would evaluate), yet + // the object-level rule still faults on `parent` and still fails CLOSED. + await expect( + engine.insert('scoped_line', { invoice: 'INV-DRAFT', description: 'seat' }), + ).rejects.toThrow(); + }); + + // ── cost ───────────────────────────────────────────────────────────────── + + it('batch-reads the headers ONCE per insert, and not at all without a parent-scoped requiredWhen', async () => { + // The header resolution goes through `find` (batched, #4889's + // `resolveMasterDetailParents`), so that is what is counted here — the + // `findOne`s on the same object belong to the #4441 reference guard and are + // not this change's cost. + const reads: string[] = []; + const original = (engine as any).find.bind(engine); + (engine as any).find = async (name: string, q: any, o?: any) => { + reads.push(name); + return original(name, q, o); + }; + // Two rows under the SAME master, and two more under another: N rows, M + // masters, ONE query. + await engine.insert('showcase_invoice_line', [ + { invoice: 'INV-SENT', description: 'a', quantity: 1 }, + { invoice: 'INV-SENT', description: 'b', quantity: 1 }, + { invoice: 'INV-DRAFT', quantity: 1 }, + ] as any); + expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(1); + + // An object with only record-scoped requirements pays nothing extra. + engine.registry.registerObject({ + name: 'plain_line', + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, + quantity: { type: 'number' }, + }, + } as any); + reads.length = 0; + await engine.insert('plain_line', { invoice: 'INV-SENT', quantity: 1 }); + expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(0); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 125f0b1e5b..d18c8e8328 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -110,7 +110,7 @@ import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spe import { bindHooksToEngine } from './hook-binder.js'; import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, buildFieldError, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField } from './validation/record-validator.js'; import type { AdmittedValueShapeViolation, AdmittedValueShapeViolationSink } from './validation/record-validator.js'; -import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, stripReadonlyFields, stripRuntimeOwnedFields } from './validation/rule-validator.js'; +import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, hasParentScopedRequiredWhen, stripReadonlyFields, stripRuntimeOwnedFields } from './validation/rule-validator.js'; import { resolveMasterDetailRelation } from './master-detail.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; import { @@ -3087,6 +3087,12 @@ export class ObjectQL implements IObjectQLEngine { * `stripReadonlyWhenFieldsMulti`. A bulk update of N details under M masters * costs ONE extra query, not N — the same "read the match set once" discipline * the #3106 prior-row fetch follows. + * + * [#4977] Serves three callers now, unchanged: the bulk `readonlyWhen` strip, + * the bulk `requiredWhen` evaluation, and the INSERT path — where `data` is + * 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. */ private async resolveMasterDetailParents( schema: any, @@ -5447,12 +5453,22 @@ export class ObjectQL implements IObjectQLEngine { } } } + // [#4977] A `parent`-scoped `requiredWhen` ("once the header is Sent, + // every line must carry a description") is a SERVER guarantee, and the + // insert is where a line is first written empty — so unlike the + // `readonlyWhen` strip, which is an update-path concept, this binding + // has to exist here too. Gated on the schema actually declaring such a + // predicate, so an object with only `record`-scoped requirements pays + // nothing; batched, so N rows under M masters cost ONE header read. + const insertParentForRow = hasParentScopedRequiredWhen(schemaForValidation as any) + ? await this.resolveMasterDetailParents(schemaForValidation, null, rows) + : undefined; for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; try { normalizeMultiValueFields(schemaForValidation, rows[i]); validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx, onAdmittedValueShapeViolation }); - evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx }); + evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx, parent: insertParentForRow?.(rows[i]) }); await this.assertReferencesResolve( schemaForValidation, rows[i], suppliedPerRow[i], opCtx.context, msgCtx, ); @@ -5932,9 +5948,35 @@ export class ObjectQL implements IObjectQLEngine { // is a pure function of what we hand it — resolve here, gated on // the payload actually touching such a predicate so a detail // object with only `record`-scoped locks pays no extra read. - const roWhenParent = hasParentScopedReadonlyWhenInPayload(updateSchema as any, preRoWhen) + // + // [#4977] `requiredWhen` reads the same root at the same write, + // so the two slots share ONE resolution rather than each buying + // a header read — and, more importantly, so a single write can + // never judge its lock and its requirement against two different + // headers. Payload-FK-first for both (#4889's rule: a repoint is + // judged against the master it lands on). + const schemaHasParentRequiredWhen = hasParentScopedRequiredWhen(updateSchema as any); + const wantsParentBinding = + hasParentScopedReadonlyWhenInPayload(updateSchema as any, preRoWhen) || + schemaHasParentRequiredWhen; + const roWhenParent = wantsParentBinding ? await this.resolveMasterDetailParent(updateSchema, preRoWhen, priorRecord) : undefined; + // [#4977] The ADR-0113 non-regression pre-check asks whether the + // STORED row already violated, so for a REPOINT it must read the + // header the row hung off BEFORE the write — not the one it is + // landing on. Resolved only when the payload actually moves the + // detail to another master; otherwise the two are the same row + // and `evaluateValidationRules` reuses `parent` for both. + const mdRel = schemaHasParentRequiredWhen ? resolveMasterDetailRelation(updateSchema as any) : null; + const priorMasterId = mdRel ? masterIdOf(mdRel.fk, null, priorRecord) : undefined; + const repointsMaster = + mdRel != null && + priorMasterId != null && + masterIdOf(mdRel.fk, preRoWhen, priorRecord) !== priorMasterId; + const roWhenPreviousParent = repointsMaster + ? await this.resolveMasterDetailParent(updateSchema, null, priorRecord) + : undefined; hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger, roWhenParent) as any; reportDroppedFields(preRoWhen, hookContext.input.data as Record, 'readonly_when'); // [#2948] Enforce STATIC `readonly` on the write path for @@ -5955,7 +5997,7 @@ export class ObjectQL implements IObjectQLEngine { // "you sent a read-only field" should not depend on whether some // other field also failed a business rule. assertNoStrictDrops(); - evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); + evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx, parent: roWhenParent, previousParent: roWhenPreviousParent }); // [#4441] A repoint is as capable of dangling as an initial link. await this.assertReferencesResolve( updateSchema, hookContext.input.data as Record, @@ -6019,16 +6061,35 @@ export class ObjectQL implements IObjectQLEngine { // locked in any target row is fail-safe-dropped for all (narrow // `where` to reach the unlocked rows). Symmetric with the // single-id `stripReadonlyWhenFields`; INSERT stays exempt. - if (payloadHasReadonlyWhen) { - const preRoWhenMulti = hookContext.input.data as Record; - // [#4889] N matched rows can hang off N different masters, so - // the `parent` binding is per row here. Batch-read the - // distinct headers ONCE (the same shape as the single-id - // resolution, one query instead of one per row) and hand the - // strip a lookup. - const parentForRow = hasParentScopedReadonlyWhenInPayload(updateSchema as any, preRoWhenMulti) + // + // [#4889] N matched rows can hang off N different masters, so the + // `parent` binding is per row here. Batch-read the distinct + // headers ONCE (the same shape as the single-id resolution, one + // query instead of one per row) and hand out a lookup. + // + // [#4977] Hoisted out of the `readonlyWhen` block because the + // per-row `evaluateValidationRules` below needs the same lookup + // for `requiredWhen` — including on a batch whose payload writes + // no `readonlyWhen` field at all. One resolution, both consumers, + // so a bulk write cannot judge its lock and its requirement + // against different headers. + const preRoWhenMulti = hookContext.input.data as Record; + const schemaHasParentRequiredWhenMulti = hasParentScopedRequiredWhen(updateSchema as any); + const parentForRow = + hasParentScopedReadonlyWhenInPayload(updateSchema as any, preRoWhenMulti) || + schemaHasParentRequiredWhenMulti ? await this.resolveMasterDetailParents(updateSchema, preRoWhenMulti, priorRows) : undefined; + // [#4977] Pre-check headers for the ADR-0113 non-regression test, + // resolved only when the payload REPOINTS the matched rows at + // another master (see the single-id branch for why the stored + // row's own header is the one that question needs). + const mdRelMulti = schemaHasParentRequiredWhenMulti ? resolveMasterDetailRelation(updateSchema as any) : null; + const previousParentForRow = + mdRelMulti != null && masterIdOf(mdRelMulti.fk, preRoWhenMulti, undefined) != null + ? await this.resolveMasterDetailParents(updateSchema, null, priorRows) + : undefined; + if (payloadHasReadonlyWhen) { hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow) as any; reportDroppedFields(preRoWhenMulti, hookContext.input.data as Record, 'readonly_when'); } @@ -6059,7 +6120,7 @@ export class ObjectQL implements IObjectQLEngine { if (rulesNeedRows) { for (const row of priorRows ?? []) { try { - evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: row, logger: this.logger, currentUser: bulkEvalUser, skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); + evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: row, logger: this.logger, currentUser: bulkEvalUser, skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx, parent: parentForRow?.(row), previousParent: previousParentForRow?.(row) }); } catch (err) { if (err instanceof ValidationError && row?.id != null) { throw new ValidationError(err.fields.map((f) => ({ ...f, message: `${f.message} (record ${String(row.id)})` }))); @@ -6068,6 +6129,13 @@ export class ObjectQL implements IObjectQLEngine { } } } else { + // [#4977] No `parent` here, and it is not a hole: this branch + // is unreachable for an object that has a `requiredWhen` at + // all. `needsPriorRecord` is TRUE as soon as any field + // declares one (`fieldsNeedPrior`), so `rulesNeedRows` sends + // every such object down the per-row branch above, where the + // binding is supplied. This branch only ever runs for the + // rule families that never read a header. evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: null, logger: this.logger, currentUser: bulkEvalUser, skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); } // [#4441] The bulk call site too — a guard wired into single-id diff --git a/packages/objectql/src/master-detail.ts b/packages/objectql/src/master-detail.ts index 911d4365bc..af1e7c3f8b 100644 --- a/packages/objectql/src/master-detail.ts +++ b/packages/objectql/src/master-detail.ts @@ -26,7 +26,15 @@ * a data-integrity lock depend on field ordering — PD #12's "declared, not * guessed". Such an object simply has no `parent` binding, which the build-time * gate in `@objectstack/lint` (`validate-expressions`) rejects at authoring - * time and the write path treats as LOCKED rather than allowed. + * time — for `readonlyWhen` since #4889 and for `requiredWhen` since #4977. + * + * The two runtimes then part ways on the unbindable case, deliberately: an + * unbound `parent` leaves a `readonlyWhen` field LOCKED (#4889 — refusing to + * wave a declared lock through), while a `requiredWhen` stays fail-OPEN + * (#4977 — a 422 on a write whose header is merely unreadable was ruled too + * loud, and left to the next review of ADR-0058 D5). That asymmetry is why the + * build-time gate covers BOTH slots: it is the only thing standing between an + * unbindable `requiredWhen` and a requirement that enforces nothing in silence. */ /** The child→master link: the FK field on the detail, and the master object. */ diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 84865f867e..389fd77acd 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -9,6 +9,7 @@ import { stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, + hasParentScopedRequiredWhen, stripReadonlyFields, stripRuntimeOwnedFields, isRuntimeOwnedField, @@ -278,6 +279,179 @@ describe('hasParentScopedReadonlyWhenInPayload (#4889 gate)', () => { }); }); +// #4977 — PARENT-scoped `requiredWhen`, the mirror of the block above on the +// same field. `readonlyWhen` failing open WROTE a field that should have been +// frozen; `requiredWhen` failing open ACCEPTS a record that should have been +// rejected. Ruling (2026-08-06): bind the scope, keep fail-OPEN, gate the +// unbindable declaration at build time (option B — 422 — was NOT taken). +const sentLineFields = { + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + // THE SUBJECT. + description: { type: 'text', requiredWhen: "parent.status == 'sent'" }, + // Row-scoped — unaffected by the parent binding, and here to prove it. + note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, + quantity: { type: 'number' }, + }, +}; + +/** Collect the ValidationError field codes, or `null` when the write is accepted. */ +function violations( + schema: unknown, + data: Record, + mode: 'insert' | 'update', + opts: Record = {}, +): string[] | null { + try { + evaluateValidationRules(schema as never, data, mode, opts as never); + return null; + } catch (err) { + if (err instanceof ValidationError) return err.fields.map((f) => f.field); + throw err; + } +} + +describe('parent-scoped requiredWhen (#4977)', () => { + it('REQUIRES the field when the master-detail header says so', () => { + expect(violations(sentLineFields, { invoice: 'inv1', quantity: 1 }, 'insert', { + parent: { id: 'inv1', status: 'sent' }, + })).toEqual(['description']); + }); + + it('does NOT require it when the header does not match', () => { + expect(violations(sentLineFields, { invoice: 'inv1', quantity: 1 }, 'insert', { + parent: { id: 'inv1', status: 'draft' }, + })).toBeNull(); + }); + + it('accepts the write once the field is supplied', () => { + expect(violations(sentLineFields, { invoice: 'inv1', description: 'seat' }, 'insert', { + parent: { id: 'inv1', status: 'sent' }, + })).toBeNull(); + }); + + it('is FAIL-OPEN when `parent` could not be bound — the #4889 asymmetry', () => { + // The twin above resolves an unbound root to LOCKED. Here the ruling + // deliberately kept the historical fail-open exit: the requirement is + // skipped and the write is ACCEPTED. Do not "restore symmetry" — that is + // option B, reserved for the next review of ADR-0058 D5. + const warnings: string[] = []; + expect(violations(sentLineFields, { invoice: 'inv1', quantity: 1 }, 'insert', { + logger: { warn: (m: string) => warnings.push(m) }, + })).toBeNull(); + expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('NOT enforced'))).toBe(true); + }); + + it('keeps the plain fail-open message for a predicate that is simply broken', () => { + // Not an unbound root — `record` IS bound, the key under it is undeclared. + const warnings: string[] = []; + expect(violations( + { fields: { amount: { type: 'currency', requiredWhen: "record.no_such_field == 'x'" } } }, + { amount: 1 }, + 'insert', + { logger: { warn: (m: string) => warnings.push(m) } }, + )).toBeNull(); + expect(warnings.some((w) => w.includes('failed to evaluate — skipped'))).toBe(true); + expect(warnings.some((w) => w.includes("reads 'parent'"))).toBe(false); + }); + + it('leaves the ROW-scoped requiredWhen on the same object working unchanged', () => { + expect(violations(sentLineFields, { invoice: 'inv1', quantity: 500 }, 'insert', { + parent: { id: 'inv1', status: 'draft' }, + })).toEqual(['note']); + }); + + // ── ADR-0113 non-regression, with a parent in scope ────────────────────── + + it('lets a legacy row rest: the header ALREADY required it and it was already empty', () => { + expect(violations(sentLineFields, { quantity: 7 }, 'update', { + previous: { id: 'l1', invoice: 'inv1', description: '', quantity: 1 }, + parent: { id: 'inv1', status: 'sent' }, + })).toBeNull(); + }); + + it('rejects a write that NULLS the field while the header requires it', () => { + expect(violations(sentLineFields, { description: '' }, 'update', { + previous: { id: 'l1', invoice: 'inv1', description: 'seat', quantity: 1 }, + parent: { id: 'inv1', status: 'sent' }, + })).toEqual(['description']); + }); + + it('judges a REPOINT by the header the write LANDS on, not the one it leaves', () => { + // The reason `previousParent` exists. The row complied under its draft + // header; the write moves it under a sent one. Binding the LANDING header + // to the pre-check too would read this as a pre-existing violation and let + // it rest — the acceptance hole this issue exists to close, one case in. + expect(violations(sentLineFields, { invoice: 'sent_inv' }, 'update', { + previous: { id: 'l1', invoice: 'draft_inv', description: '', quantity: 1 }, + parent: { id: 'sent_inv', status: 'sent' }, + previousParent: { id: 'draft_inv', status: 'draft' }, + })).toEqual(['description']); + }); + + it('`previousParent` defaults to `parent` when the write does not repoint', () => { + // No repoint ⇒ the engine does not pay a second header read, and the + // legacy-row exemption must still apply. + expect(violations(sentLineFields, { quantity: 7 }, 'update', { + previous: { id: 'l1', invoice: 'inv1', description: '', quantity: 1 }, + parent: { id: 'inv1', status: 'sent' }, + })).toBeNull(); + }); + + // ── blast radius: object-level rules at the SAME evaluation site ───────── + + it('does NOT bind `parent` for object-level rules — still fail-CLOSED (#4649)', () => { + const withScriptRule = { + fields: { ...sentLineFields.fields }, + validations: [{ + type: 'script', + name: 'parent_in_object_rule', + message: 'nope', + condition: "parent.status == 'sent'", + }], + }; + // A parent IS supplied, and the field predicate below evaluates with it — + // yet the object-level rule still faults on `parent` and still REJECTS. + expect(() => evaluateValidationRules( + withScriptRule as never, + { invoice: 'inv1', description: 'seat' }, + 'insert', + { parent: { id: 'inv1', status: 'sent' } } as never, + )).toThrow(/could not be evaluated/); + }); +}); + +describe('hasParentScopedRequiredWhen (#4977 gate)', () => { + it('is TRUE when a field declares a parent-scoped requiredWhen', () => { + expect(hasParentScopedRequiredWhen(sentLineFields)).toBe(true); + }); + + it('is FALSE for record-scoped requiredWhen only (no needless header read)', () => { + expect(hasParentScopedRequiredWhen(invoiceFields)).toBe(false); + }); + + it('is FALSE for a parent-scoped READONLYWhen — that is the other gate', () => { + expect(hasParentScopedRequiredWhen(invoiceLineFields)).toBe(false); + }); + + it('does not mistake a field NAMED parent_id, or a string literal, for the binding', () => { + expect(hasParentScopedRequiredWhen({ + fields: { + a: { type: 'text', requiredWhen: "record.parent_id != ''" }, + b: { type: 'text', requiredWhen: "record.kind == 'parent'" }, + }, + })).toBe(false); + }); + + it('is NOT payload-filtered, unlike its readonlyWhen twin', () => { + // The whole failure `requiredWhen` catches is a field the payload OMITS, so + // filtering by `name in data` would skip exactly the writes it is for. + // (The twin takes a payload argument; this one deliberately does not.) + expect(hasParentScopedRequiredWhen(sentLineFields)).toBe(true); + expect(hasParentScopedReadonlyWhenInPayload(invoiceLineFields, { description: 'x' })).toBe(false); + }); +}); + // #2948 — static `readonly:true` write enforcement (caller-supplied only). const stampedFields = { fields: { diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index b5883035ea..4ac163d69f 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -114,6 +114,42 @@ * the grounds that it reads inconsistent with D5's table — the table is what * was amended, and ADR-0057 D10 (server enforces, client is courtesy) is why. * + * ## `requiredWhen`: the SCOPE is bound, the SEMANTICS are not changed (#4977) + * + * `requiredWhen` sits on the same field as `readonlyWhen`, is evaluated by this + * same module, and had the same hole one slot over: nothing bound `parent`, so + * ``requiredWhen: P`parent.status == 'sent'` `` — "once the header is Sent every + * line must carry a description" — evaluated in the inline grid and was a no-op + * on the server. The write was accepted with the field empty. + * + * Note this is the MIRROR of #4889's failure mode, not the same one: a + * `readonlyWhen` failing open WROTE a field that should have been frozen; a + * `requiredWhen` failing open ACCEPTS a record that should have been rejected. + * Both are `declared ≠ enforced` (PD #10) on one declaration site. + * + * The maintainer's ruling (2026-08-06) is deliberately narrower than #4889's: + * + * - **Bind the scope.** The engine resolves the master-detail header with the + * same `resolveMasterDetailParent(s)` helpers #4889 added and passes it as + * {@link EvaluateRulesOptions.parent}; the requirement is now enforced where + * it is documented to be enforced, on insert, single-id update and bulk. + * - **Do NOT copy the fail-CLOSED carve-out.** An unevaluable predicate — an + * unresolvable header included — stays fail-OPEN: logged, skipped, the write + * proceeds. Rejecting it (a 422 on a write whose header is merely unreadable + * at that moment) is a louder failure than the `readonlyWhen` case, where the + * cost of the conservative answer is one refused field. That is option B of + * #4977 and it was explicitly NOT taken; it is reserved for the next review + * of ADR-0058 D5. + * - **Catch it at BUILD time instead.** `@objectstack/lint`'s + * `validate-expressions` rejects a `parent`-scoped `requiredWhen` on an object + * that declares no single `master_detail`, so the unbindable declaration — + * the one a runtime fail-open would silently swallow forever — never ships. + * + * Bound for the FIELD predicate only. The object-level `script` / `cross_field` + * rules evaluated further down this same function do NOT get the root: they are + * fail-CLOSED since #4649, so binding a new root there would flip writes they + * reject today into accepted ones. Pinned by test. + * * One consequence worth knowing before writing a predicate: because a declared * field is now always present, `has(record.)` is uniformly TRUE * (a materialised `null` is a present key holding null — this is CEL's rule, @@ -280,6 +316,38 @@ export interface EvaluateRulesOptions { * and fail-open (see {@link evaluateOptionVisibility}). */ currentUser?: { id?: string; roles?: string[]; organizationId?: string | null; [k: string]: unknown } | null; + /** + * [#4977] The master-detail header this write's field `requiredWhen` + * predicates read as `parent` — the SAME binding, resolved by the SAME engine + * helpers, that #4889 gave `readonlyWhen`. Only the engine owns a driver, so + * it resolves the header and hands it over; `undefined` leaves `parent` + * unbound, which is what a non-detail object (or a payload whose predicates + * never name `parent`) passes. + * + * Scoped to the field-level `requiredWhen` block on purpose. It is NOT bound + * for the object-level `script` / `cross_field` / `conditional` rules that + * share this call: those have been fail-CLOSED since #4649, so introducing a + * root there would flip writes those rules reject today into accepted ones — + * the blast radius #4977's issue body called out and the reason the change was + * kept out of #4972. Pinned by test. + */ + parent?: ParentBinding; + /** + * [#4977] The header the record hung off BEFORE this write, for the ADR-0113 + * non-regression pre-check only ("did the stored state already violate?"). + * + * It differs from {@link parent} in exactly one situation: a write that + * REPOINTS the detail at another master. `parent` is payload-FK-first (the + * master the write lands on, #4889's rule) and decides the merged verdict; + * the pre-check asks a question about the STORED row, which hung off the + * stored FK's master. Binding the landing header there would read a repoint + * onto a header that turns the requirement ON as a pre-existing violation and + * let it rest — the acceptance hole this issue exists to close, one case in. + * + * Defaults to {@link parent} when omitted: with no repoint the two headers are + * the same row, and the engine does not pay a second read to prove it. + */ + previousParent?: ParentBinding; /** * When true, `state_machine` rules are skipped entirely — both the * `initialStates` entry-point check on insert (#3165) and the transition @@ -453,6 +521,39 @@ export function hasParentScopedReadonlyWhenInPayload( return false; } +/** + * True when at least one field declares a `requiredWhen` predicate that reads + * the `parent` root (#4977) — the gate the engine uses to decide whether to + * resolve the master-detail header for {@link evaluateValidationRules}, so an + * object with no parent-scoped requirement pays no extra read. + * + * The `readonlyWhen` twin ({@link hasParentScopedReadonlyWhenInPayload}) filters + * by the payload; this one deliberately does NOT, and the asymmetry is the point + * rather than an oversight. `readonlyWhen` judges an incoming CHANGE, so a field + * the payload never touches cannot be locked out of anything. `requiredWhen` + * judges the MERGED record: the whole failure it exists to catch is a field left + * empty — i.e. absent from the payload — while the predicate says it must be + * filled. Filtering by `name in data` here would skip exactly the writes the + * rule is for. + * + * Same AST-based root reader as its twin ({@link readsParentRoot} over + * `collectCelRootIdentifiers`), so a field named `parent_id` or the string + * literal `'parent'` cannot be mistaken for the binding, and build-time lint, + * the `readonlyWhen` gate and this gate can never disagree about what "reads + * `parent`" means. + */ +export function hasParentScopedRequiredWhen( + objectSchema: { fields?: Record } | undefined | null, +): boolean { + const fields = objectSchema?.fields; + if (!fields) return false; + for (const def of Object.values(fields)) { + if (!def?.requiredWhen) continue; + if (readsParentRoot(def.requiredWhen)) return true; + } + return false; +} + /** Parsed-root memo — metadata predicates are a small, fixed set of sources. */ const parentRootCache = new Map(); @@ -1113,12 +1214,37 @@ export function evaluateValidationRules( // generalized from "always true"; it is what makes tightening a // conditional contract on a deployed object safe (#3929's objection). if (hasFieldRules && fields) { + // [#4977] `parent` — the master-detail header — bound for the field-level + // `requiredWhen` predicates exactly as #4889 binds it for `readonlyWhen`, + // and bound ONLY when the engine actually resolved one. An absent binding + // is left absent rather than bound to `null`: `null.status` would fault as + // `No such key` and lose the "which root was unbound" diagnostic below. + const parentScope = opts.parent != null ? { extra: { parent: opts.parent } } : {}; + // The pre-check's header (see `EvaluateRulesOptions.previousParent`). Same + // row as `parent` unless this write repoints the detail at another master. + const prevParent = opts.previousParent !== undefined ? opts.previousParent : opts.parent; + const prevParentScope = prevParent != null ? { extra: { parent: prevParent } } : {}; for (const [name, def] of Object.entries(fields)) { const pred = def?.requiredWhen; if (!pred) continue; - const res = ExpressionEngine.evaluate(toExpression(pred), { record: merged, previous }); + const res = ExpressionEngine.evaluate(toExpression(pred), { record: merged, previous, ...parentScope }); if (!res.ok) { - opts.logger?.warn?.(`requiredWhen for '${name}' failed to evaluate — skipped`); + // Fail-OPEN, unchanged (#4977 ruling: bind the scope, keep the + // evaluation semantics). An unevaluable `requiredWhen` — including one + // whose `parent` the engine could not resolve — is logged and skipped, + // NOT turned into a rejection: that is option B, deliberately not taken + // here and left to the next review of ADR-0058 D5. All that changes is + // the diagnostic: an unbound ROOT is named, because "the header could + // not be read" and "the author typo'd a key" are different faults with + // different remedies and only one line of signal to tell them apart. + const unbound = unknownVariableOf(res.error); + opts.logger?.warn?.( + unbound + ? `requiredWhen for '${name}' reads '${unbound}', which is not bound for this operation — ` + + `skipped (the requirement is NOT enforced for this write). ` + + `A 'parent'-scoped predicate needs the object to declare exactly one master_detail relationship.` + : `requiredWhen for '${name}' failed to evaluate — skipped`, + ); continue; } if (res.value === true && isMissing(merged[name])) { @@ -1127,7 +1253,7 @@ export function evaluateValidationRules( // record, treat the pre state as COMPLIANT — enforcement stays on // unless a legacy violation is proven. if (mode === 'update' && previous) { - const pre = ExpressionEngine.evaluate(toExpression(pred), { record: previous, previous }); + const pre = ExpressionEngine.evaluate(toExpression(pred), { record: previous, previous, ...prevParentScope }); const preViolated = pre.ok && pre.value === true && isMissing(previous[name]); if (preViolated) continue; // legacy rows rest } From 8890f309410114f9eb698c832af847472694718a Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Fri, 7 Aug 2026 20:08:21 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(objectql):=20=E7=BB=99=20engine-requir?= =?UTF-8?q?ed-when-parent=20=E7=9A=84=20registerObject=20=E8=A1=A5?= =?UTF-8?q?=E4=B8=8A=20packageId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 新增的 4 处 `engine.registry.registerObject({...} as any)` 只传了 1 个实参, 而签名是 `registerObject(schema, packageId, namespace?, ownership?, priority?)` —— `packageId` 是必填。tsc 在 test 层因此报 4 条 TS2554 (`Expected 2-5 arguments, but got 1`,行 115/125/275/318),把 `@objectstack/objectql` 的 TEST_DEBT 实测值顶到棘轮记录值之上。 按包内既有写法补 `'test-package'`(与 query-expression-conformance.test.ts、 save-meta-response-conformance.test.ts 一致),不引入宽容 helper、不改运行时语义。 实测:objectql test 层 raw tsc 由 355 降到 351,与 origin/main 基线逐行相同 (comm 差集为空),即本 PR 对该账目的净贡献为 0,较记录值 355 留出 4 的余量。 该文件 14 条测试全绿,包内 142 文件 / 2366 条全绿。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- packages/objectql/src/engine-required-when-parent.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/objectql/src/engine-required-when-parent.test.ts b/packages/objectql/src/engine-required-when-parent.test.ts index a99177a576..225d26b3ad 100644 --- a/packages/objectql/src/engine-required-when-parent.test.ts +++ b/packages/objectql/src/engine-required-when-parent.test.ts @@ -121,7 +121,7 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { // (`invoice.object.ts` L153) — worked all along, must keep working. paid_on: { type: 'date', requiredWhen: "record.status == 'paid'" }, }, - } as any); + } as any, 'test-package'); engine.registry.registerObject({ name: 'showcase_invoice_line', fields: { @@ -134,7 +134,7 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, quantity: { type: 'number' }, }, - } as any); + } 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' }); @@ -284,7 +284,7 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { message: 'object-level rules do not get a parent binding', condition: "parent.status == 'sent'", }], - } as any); + } as any, 'test-package'); // The header resolves fine (the field predicate below would evaluate), yet // the object-level rule still faults on `parent` and still fails CLOSED. await expect( @@ -322,7 +322,7 @@ describe('parent-scoped requiredWhen is enforced server-side (#4977)', () => { note: { type: 'text', requiredWhen: 'record.quantity >= 100' }, quantity: { type: 'number' }, }, - } as any); + } as any, 'test-package'); reads.length = 0; await engine.insert('plain_line', { invoice: 'INV-SENT', quantity: 1 }); expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(0);