diff --git a/.changeset/readonly-when-total-record.md b/.changeset/readonly-when-total-record.md new file mode 100644 index 0000000000..a01b154c4a --- /dev/null +++ b/.changeset/readonly-when-total-record.md @@ -0,0 +1,38 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): 字段 `readonlyWhen` 在服务端看到的记录改为「对象声明的全量形状」(#4953) + +`materializeDeclaredFields`(#1871 / #4649)此前只接在两个求值接缝上: +`evaluateValidationRules`(对象级校验规则、字段 `requiredWhen`、option +`visibleWhen`)与生命周期 hook 的 `condition`。**字段 `readonlyWhen` 不在其中** —— +写入路径上的 `stripReadonlyWhenFields` / `stripReadonlyWhenFieldsMulti` 直接把 +`{ ...previous, ...data }` 交给 CEL 求值。 + +后果是同一个字段上的两条谓词对「记录是什么」给出相反答案:``requiredWhen: +P`record.approved_at == null` `` 是一条可用的守卫,而写在同一字段上的 +``readonlyWhen: P`record.approved_at == null` `` 只要驱动没把 `approved_at` +这一列回读出来就会 fault;**而 `readonlyWhen` fault 是 fail-open**,于是作者声明 +为冻结的字段被照常写入。某次写入是否被拦,取决于驱动回读了哪些列 —— 作者既看不见 +也控制不了的存储细节。 + +本次把这两个 strip 的 `record` 与 `previous` 两个根都过 `materializeDeclaredFields`, +按维护者 2026-08-06 裁决(#4953)统一**服务端**接缝。 + +**这是一次可见的行为变化,方向如下:** + +- 稀疏行上原本 fault→放行的谓词现在正常求值,谓词为真则改动被剥离(即恢复本应生效的 + 只读约束)。`record.x == null` / `!= null` / `previous.x == null` 都属此类。 +- 相应地,`has(record.<已声明字段>)` 在全量绑定下恒为 `true`(物化出的 `null` 是一个 + 「存在且值为 null」的键,这是 CEL 自身的规则),`!has(record.<已声明字段>)` 恒为 + `false`。因此以 `readonlyWhen: !has(record.x)` 表达「x 为空时冻结」的写法**不再锁住 + 字段** —— 它原本也不是一条保证(在回读全部列的驱动上它从来不锁),现在它变成确定的 + `false`。要表达「为空时冻结」请改写为 `record.x == null`(即 `@objectstack/lint` + null-guard 闸门一直建议的写法)。 + +未改动的部分:`readonlyWhen` 的 fail-open 策略本身;#4889 的 `parent` 未绑定 ⇒ +**LOCKED** 判定(`parent` 是另一个对象的行,不做物化);对象级 `script` / +`cross_field` 自 #4649 起的 fail-closed;INSERT 仍不走 `readonlyWhen` 剥离。 +未读到前序行时(引擎未取或行已不存在)**不做**物化 —— 那样不是补齐缺失值,而是 +凭空捏造一个与库中行相矛盾的值。 diff --git a/packages/objectql/src/declared-fields.ts b/packages/objectql/src/declared-fields.ts index 7a97356357..bf16e9fe82 100644 --- a/packages/objectql/src/declared-fields.ts +++ b/packages/objectql/src/declared-fields.ts @@ -3,15 +3,28 @@ /** * Make a record TOTAL over an object's DECLARED fields. * - * Shared by the two places that evaluate a CEL expression against "the - * record": object-level validation predicates - * (`validation/rule-validator.ts`, #1871 / #4649) and declarative hook - * `condition`s (`hook-wrappers.ts`, #4770). They used to disagree — a - * predicate saw a total record while a hook condition saw only the fields the - * current write happened to carry — which is precisely the drift this module - * exists to prevent: an author cannot be expected to know that the same - * `record.done == true` means two different things depending on which surface - * reads it. + * Shared by the SERVER-side places that evaluate a CEL expression against "the + * record": object-level validation predicates + field `requiredWhen` + + * option `visibleWhen` (`validation/rule-validator.ts`, #1871 / #4649), + * declarative hook `condition`s (`hook-wrappers.ts`, #4770), and the field + * `readonlyWhen` strips on the write path (`validation/rule-validator.ts` + * `readonlyWhenBindings`, #4953). They used to disagree — a predicate saw a + * total record while a hook condition saw only the fields the current write + * happened to carry — which is precisely the drift this module exists to + * prevent: an author cannot be expected to know that the same `record.done == + * true` means two different things depending on which surface reads it. + * + * Two bindings are still sparse, and the difference between them matters: + * + * - The flow trigger record (`packages/triggers/trigger-record-change`) is a + * server seam the same ruling puts on this list; it is simply not wired yet + * (services lane, #4953 item 1's other half). Do not read its absence as a + * decision. + * - objectui's action `visible` / `disabled` binds whatever record the client + * already fetched. That one is a DECISION (#4953 item 2): making it total + * would mean every REST read padding out all declared columns, so it stays + * sparse and is documented as sparse — an author on that surface guards with + * `has()`, not `!= null`. * * CEL is strict about missing keys: `record.x` on a record that does not carry * the key `x` aborts the whole expression with `No such key`, which is NOT the diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 389fd77acd..edaf88cc7d 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -452,6 +452,195 @@ describe('hasParentScopedRequiredWhen (#4977 gate)', () => { }); }); +// #4953 — the record a `readonlyWhen` predicate sees is TOTAL over the object's +// DECLARED fields, like the two seams that were materialised in #4649/#4770. +// Before this, `stripReadonlyWhenFields` merged `{...previous, ...data}` raw, so +// a predicate reading a declared column the DRIVER did not echo back faulted — +// and a faulting `readonlyWhen` fails open, i.e. WROTE the field the author +// declared frozen. Which columns come back is a storage property no author can +// see, so the same declaration was enforced or not depending on the driver. +const sparseLockFields = { + fields: { + notes: { type: 'text' }, + approved_at: { type: 'datetime' }, + // "while nothing has been approved, the amount is frozen" — the `== null` + // spelling #4649 made the supported one (and the null-guard gate prescribes). + amount: { type: 'currency', readonlyWhen: 'record.approved_at == null' }, + }, +}; + +/** A prior row from a driver that stores only the columns a write touched. */ +const sparsePrior = () => ({ id: 'r1', amount: 100 }); +/** The same row from a driver that returns every declared column. */ +const totalPrior = (approvedAt: unknown) => ({ id: 'r1', amount: 100, notes: null, approved_at: approvedAt }); + +describe('readonlyWhen binds a TOTAL record (#4953)', () => { + it('evaluates `record. == null` on a SPARSE prior instead of faulting through', () => { + // THE bug. Pre-#4953: `No such key: approved_at` ⇒ fail-open ⇒ amount written. + const warnings: string[] = []; + const out = stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, sparsePrior(), { + warn: (m: string) => warnings.push(m), + } as never); + expect(out).toEqual({}); + expect(warnings.some((w) => w.includes('failed to evaluate'))).toBe(false); + expect(warnings.some((w) => w.includes('is read-only (readonlyWhen)'))).toBe(true); + }); + + it('still KEEPS the change when the materialised value makes the predicate FALSE', () => { + // Materialising is not "lock everything": the row HAS an approval date, so + // the lock is off and the legitimate edit lands. + expect( + stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, { id: 'r1', amount: 100, approved_at: '2026-01-01' }), + ).toEqual({ amount: 999 }); + }); + + it('reads the same verdict on a sparse prior as on a total one (the point)', () => { + // One declaration, two drivers, one answer. This equality is the guarantee; + // before #4953 the left side kept the change and the right side stripped it. + const sparse = stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, sparsePrior()); + const total = stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, totalPrior(null)); + expect(sparse).toEqual(total); + expect(sparse).toEqual({}); + }); + + it('materialises the `previous` root too, not just `record`', () => { + const schema = { fields: { ...sparseLockFields.fields, amount: { type: 'currency', readonlyWhen: 'previous.approved_at == null' } } }; + expect(stripReadonlyWhenFields(schema, { amount: 999 }, sparsePrior())).toEqual({}); + expect(stripReadonlyWhenFields(schema, { amount: 999 }, { id: 'r1', amount: 100, approved_at: '2026-01-01' })).toEqual({ amount: 999 }); + }); + + it('applies on the BULK path identically — one payload, N sparse rows', () => { + // A bulk write must not judge the same predicate by a different record + // shape than a single-id write does. + expect(stripReadonlyWhenFieldsMulti(sparseLockFields, { amount: 999 }, [sparsePrior()])).toEqual({}); + // ≥1 locked row still drops it for the batch; no locked row still writes. + expect(stripReadonlyWhenFieldsMulti(sparseLockFields, { amount: 999 }, [ + { id: 'r1', amount: 1, approved_at: '2026-01-01' }, + sparsePrior(), + ])).toEqual({}); + expect(stripReadonlyWhenFieldsMulti(sparseLockFields, { amount: 999 }, [ + { id: 'r1', amount: 1, approved_at: '2026-01-01' }, + { id: 'r2', amount: 2, approved_at: '2026-02-02' }, + ])).toEqual({ amount: 999 }); + }); + + it('does NOT materialise when the prior row is not in hand (no fabrication)', () => { + // `declared-fields.ts`'s standing rule: without the persisted state, + // defaulting a declared field to null would FABRICATE a value that + // contradicts the stored row. So this case keeps the historical fault → + // fail-open exit, and the engine avoids it by fetching the prior row + // whenever the object declares a readonlyWhen field (`needsPriorRecord`). + const warnings: string[] = []; + const out = stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, null, { + warn: (m: string) => warnings.push(m), + } as never); + expect(out).toEqual({ amount: 999 }); + expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + }); + + it('never mutates the caller\'s prior record (it is the engine\'s hookContext.previous)', () => { + const prior = sparsePrior(); + stripReadonlyWhenFields(sparseLockFields, { amount: 999 }, prior); + expect('approved_at' in prior).toBe(false); + expect('notes' in prior).toBe(false); + const rows = [sparsePrior()]; + stripReadonlyWhenFieldsMulti(sparseLockFields, { amount: 999 }, rows); + expect('approved_at' in rows[0]!).toBe(false); + }); + + it('leaves the fail-open branch ALIVE — an ordering comparison still faults over a total record', () => { + // `null < null` is `no such overload`, so materialising does not make every + // predicate evaluable. This is exactly why the null-guard gate exists. + const warnings: string[] = []; + const out = stripReadonlyWhenFields( + { fields: { ...sparseLockFields.fields, amount: { type: 'currency', readonlyWhen: 'record.notes < record.approved_at' } } }, + { amount: 999 }, + sparsePrior(), + { warn: (m: string) => warnings.push(m) } as never, + ); + expect(out).toEqual({ amount: 999 }); + expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + }); + + it('keeps fail-OPEN for an UNDECLARED key — materialising covers declared fields only', () => { + // The #4649 line, unmoved: a typo must stay unevaluable so it is reported, + // not silently read as null. + const warnings: string[] = []; + expect(stripReadonlyWhenFields( + { fields: { amount: { type: 'currency', readonlyWhen: 'record.stauts == null' } } }, + { amount: 999 }, + { id: 'r1', amount: 100 }, + { warn: (m: string) => warnings.push(m) } as never, + )).toEqual({ amount: 999 }); + expect(warnings.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + }); + + // ── the consequence that moves the OTHER way, pinned rather than discovered ── + it('`has(record.)` is uniformly TRUE — so it locks even on a sparse prior', () => { + // CEL's own rule: a materialised `null` is a PRESENT key holding null. + // `has()` therefore guards against an UNDECLARED key, not an empty value. + expect(stripReadonlyWhenFields( + { fields: { ...sparseLockFields.fields, amount: { type: 'currency', readonlyWhen: 'has(record.approved_at)' } } }, + { amount: 999 }, + sparsePrior(), + )).toEqual({}); + }); + + it('`!has(record.)` is uniformly FALSE — a lock spelled that way STOPS locking', () => { + // The one verdict this change moves toward "allowed": pre-#4953 the sparse + // binding made `!has(...)` true and the field was stripped. It was never a + // guarantee — on a driver returning all columns the same declaration never + // locked anything — so the flip replaces a storage-dependent verdict with a + // deterministic one, and the deterministic answer is FALSE. An author who + // means "while the field is empty" writes `== null` (the spelling + // @objectstack/lint's null-guard gate prescribes). + expect(stripReadonlyWhenFields( + { fields: { ...sparseLockFields.fields, amount: { type: 'currency', readonlyWhen: '!has(record.approved_at)' } } }, + { amount: 999 }, + sparsePrior(), + )).toEqual({ amount: 999 }); + }); + + // ── blast radius: nothing else at this write gate moves ───────────────── + it('does not touch the object-level rules — `script` / `cross_field` stay fail-CLOSED (#4649)', () => { + const withRules = { + fields: { ...sparseLockFields.fields }, + validations: [ + { type: 'script', name: 'typo_rule', message: 'nope', condition: 'record.stauts == null' }, + ], + }; + expect(() => evaluateValidationRules(withRules as never, { amount: 1 }, 'update', { + previous: { id: 'r1', amount: 100 }, + } as never)).toThrow(/could not be evaluated/); + const crossField = { + fields: { ...sparseLockFields.fields }, + validations: [ + { type: 'cross_field', name: 'typo_cross', message: 'nope', condition: 'record.stauts == null' }, + ], + }; + expect(() => evaluateValidationRules(crossField as never, { amount: 1 }, 'update', { + previous: { id: 'r1', amount: 100 }, + } as never)).toThrow(/could not be evaluated/); + }); + + it('does not disturb the #4889 parent binding: unbound root still LOCKS, parent stays unmaterialised', () => { + // `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[] = []; + expect(stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, { id: 'l1', invoice: 'inv1' }, { + 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. + const warnings2: string[] = []; + expect(stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, { id: 'l1', invoice: 'inv1' }, { + warn: (m: string) => warnings2.push(m), + } as never, { id: 'inv1' })).toEqual({ quantity: 9999 }); + expect(warnings2.some((w) => w.includes('failed to evaluate — change allowed through'))).toBe(true); + }); +}); + // #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 4ac163d69f..623904cb24 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -158,6 +158,23 @@ * `record.x == null`. Pinned by test so the app-side `has(...)` idiom cannot be * broken silently from under it. * + * ## `readonlyWhen` sees a TOTAL record too (#4953) + * + * The paragraph above ("Deliberately NOT changed here") is about the fail-open + * POLICY, and that policy is still what the field-level predicates use. What + * changed in #4953 is the other half — what the predicate is evaluated + * AGAINST. `materializeDeclaredFields` was wired into two seams and not the + * third: the strip functions on the write path merged `{ ...previous, ...data }` + * and evaluated it raw, so a `readonlyWhen` faulted (and, failing open, WROTE + * the field it was declared to freeze) on exactly the rows whose driver did not + * echo back the column its predicate reads — while `requiredWhen`, on the same + * field, in this same file, read a total record and worked. The maintainer's + * ruling (2026-08-06) unifies the SERVER seams: totality is a platform + * guarantee wherever the server evaluates, and the cross-process bindings + * (objectui action `visible`/`disabled`) stay sparse and are documented as + * such. See {@link readonlyWhenBindings} for the exact bindings, the + * ground-truth rule they obey, and the verdicts that move in BOTH directions. + * * ## Prior-record plumbing * * `state_machine` and the field-spanning predicates are meaningful only with @@ -389,6 +406,88 @@ export function needsPriorRecord( */ export type ParentBinding = Record | null | undefined; +/** + * The two CEL roots a field `readonlyWhen` predicate reads — `record` (the + * prior row overlaid with the PATCH) and `previous` — made TOTAL over the + * object's DECLARED fields (#4953). + * + * ## Why this seam had to join the other two + * + * `materializeDeclaredFields` existed since #1871/#4649 and was wired into + * exactly two evaluation seams: {@link evaluateValidationRules} (object rules, + * field `requiredWhen`, option `visibleWhen`) and the declarative hook + * `condition`s in `hook-wrappers.ts`. `readonlyWhen` — declared on the SAME + * field as `requiredWhen`, evaluated by THIS module, one function away — merged + * `{ ...previous, ...data }` and evaluated it raw. So one field's two + * predicates disagreed about what "the record" contains: ``requiredWhen: + * P`record.b != null` `` was a working guard while ``readonlyWhen: + * P`record.b != null` `` on the same field faulted whenever the driver did not + * return `b` — and a faulting `readonlyWhen` is fail-OPEN, so the field the + * author declared frozen was written. Whether it was written depended on which + * columns a driver happened to echo back, which is not something an author can + * see or control (#4953; maintainer ruling 2026-08-06: the SERVER seams are + * unified, the cross-process ones are deferred). + * + * The `parent` root is deliberately NOT materialised here. #4889 owns that + * binding's semantics — an ABSENT `parent` is the signal that makes the + * unbound-root branch of {@link isReadonlyWhenLocked} reachable, and the header + * is a row of a DIFFERENT object whose declared fields this function does not + * have. + * + * ## Consequences, both directions (measured, not asserted) + * + * A total record makes a predicate that used to fault evaluate for real, so + * verdicts move — the point of the change, and in both directions: + * + * - ``record.b == null`` / ``record.b != null`` / ``previous.b == null`` on a + * row the driver returned without `b`: fault → fail-open → the change was + * WRITTEN. Now they evaluate, and a TRUE predicate strips the change. This + * is enforcement being restored, and it is the direction the ruling asked + * for. + * - ``has(record.b)`` becomes uniformly TRUE and ``!has(record.b)`` uniformly + * FALSE for a DECLARED field, because a materialised `null` is a present key + * holding null (CEL's own rule). A ``readonlyWhen: !has(record.b)`` that + * used to lock the field therefore stops locking it. That is the same + * consequence #4649 documented for the validation seam and + * `declared-fields.ts` states as the contract — `has()` guards against an + * UNDECLARED key, not against an empty value; test emptiness with + * `!= null`. Pinned by test in both spellings so this is a recorded + * consequence rather than a discovery. + * + * Ordering comparisons still fault over a total record (`null < null` is `no + * such overload`), so the fail-open branch is not dead — the very reason + * `@objectstack/lint`'s null-guard gate exists. + * + * ## Only materialise when the persisted state is IN HAND + * + * Same rule {@link evaluateValidationRules} applies with its `groundTruth` + * flag, and the reason `declared-fields.ts` states: defaulting a declared field + * to `null` when the prior row was NOT read does not materialise an absent + * value, it FABRICATES one that contradicts the stored row. `previous` absent + * (never fetched, or a single-id update whose row is gone) therefore leaves + * both bindings exactly as they were. The engine reads the prior row whenever + * the object declares a `readonlyWhen` field ({@link needsPriorRecord} → + * `fieldsNeedPrior`), so the materialised branch is the normal one. INSERT is + * exempt from the strip entirely (`engine.ts`), so unlike the validation seam + * there is no insert case to answer here. + */ +function readonlyWhenBindings( + data: Record, + prior: Record | undefined | null, + fields: Record, +): { merged: Record; previous: Record | undefined } { + const previous = prior ?? undefined; + const merged: Record = { ...(previous ?? {}), ...data }; + if (!previous) return { merged, previous }; + return { + merged: materializeDeclaredFields(merged, fields), + // COPIED before materialising: the caller's object is the engine's + // `hookContext.previous`, which after-hooks observe — it must not gain + // materialised nulls (the same copy `evaluateValidationRules` makes). + previous: materializeDeclaredFields({ ...previous }, fields), + }; +} + /** * Strip fields whose `readonlyWhen` CEL predicate is TRUE for the (merged) * record from an UPDATE payload — the field is locked, so an incoming change is @@ -405,6 +504,9 @@ export type ParentBinding = Record | null | undefined; * * A predicate that faults is fail-open (the change is allowed through) EXCEPT * when the fault is an unbound scope root — see {@link isReadonlyWhenLocked}. + * + * The `record` / `previous` bindings are TOTAL over the object's declared + * fields (#4953) — see {@link readonlyWhenBindings}. */ export function stripReadonlyWhenFields( objectSchema: { fields?: Record } | undefined | null, @@ -415,11 +517,11 @@ export function stripReadonlyWhenFields( ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; - const merged = { ...(previous ?? {}), ...data }; + const view = readonlyWhenBindings(data, previous, fields); let result = data; for (const [name, def] of Object.entries(fields)) { if (!def?.readonlyWhen || !(name in data)) continue; - if (isReadonlyWhenLocked(def, merged, previous ?? undefined, name, logger, parent)) { + if (isReadonlyWhenLocked(def, view.merged, view.previous, name, logger, parent)) { if (result === data) result = { ...data }; delete (result as Record)[name]; logger?.warn?.(`Field '${name}' is read-only (readonlyWhen) — ignoring incoming change`); @@ -614,6 +716,14 @@ export function hasReadonlyWhenInPayload( * binding absent for that row, which {@link isReadonlyWhenLocked} reads as * LOCKED for a predicate that needs it. * + * Each matched row's `record` / `previous` bindings are made TOTAL over the + * declared fields (#4953, {@link readonlyWhenBindings}) exactly as on the + * single-id path — a bulk write must not judge the same predicate by a + * different record shape than a one-row write does. The views are built ONCE + * per row (they do not depend on which field is being judged) and only when a + * `readonlyWhen` field is actually in the payload, so a batch that touches none + * still pays nothing. + * * Returns the same object when nothing is stripped, else a shallow copy with the * locked keys removed. */ @@ -627,17 +737,23 @@ export function stripReadonlyWhenFieldsMulti( const fields = objectSchema?.fields; if (!fields || !data) return data; const rows = priorRows ?? []; + // Built lazily: a payload writing no `readonlyWhen` field never reaches the + // `.some()` below, and then no row view is materialised at all. + let views: Array> | null = null; + const rowViews = () => (views ??= rows.map((row) => readonlyWhenBindings(data, row, fields))); let result = data; for (const [name, def] of Object.entries(fields)) { if (!def?.readonlyWhen || !(name in data)) continue; - const lockedInSomeRow = rows.some((row) => + const lockedInSomeRow = rowViews().some((view, i) => isReadonlyWhenLocked( def, - { ...(row ?? {}), ...data }, - row ?? undefined, + view.merged, + view.previous, name, logger, - parentForRow?.(row ?? undefined), + // Resolved per (field, row) exactly as before — the header lookup is + // the caller's, and its call pattern is not this change's business. + parentForRow?.(rows[i] ?? undefined), ), ); if (lockedInSomeRow) {