From 55cc564163b154141ad9f8c2f7f4517772f77944 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 17:31:28 +0000 Subject: [PATCH] fix(objectql,lint): enforce parent-scoped `readonlyWhen` on the server (#4889) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readonlyWhen: parent.` was enforced only in the client grid. The server-side strip bound `record` and `previous` and had no `parent`, so every parent-scoped predicate faulted, took the fail-open branch, and the write landed — one PATCH rewrote quantity and unit price on a settled invoice's line (HTTP 200, value persisted, grid still drawing the cell locked). ADR-0057 D10 puts enforcement on the server; only the courtesy layer enforced. - Bind `parent`: the engine resolves the master-detail header (payload FK first so a repoint is judged against the master it lands on, else the prior row) and passes it into the strip, on the single-id and bulk update paths. Gated on the payload touching a parent-scoped predicate, decided from the parsed CEL AST; the bulk path batch-reads distinct headers in one query. - An unbindable scope root no longer waives the lock: the field is stripped rather than written. A merely broken predicate (undeclared key, null overload, parse error, throw) keeps the documented fail-open policy, and `requiredWhen` / option `visibleWhen` are untouched. Recorded as an ADR-0058 D5 addendum next to the same narrowing made by #4649 and #4775, and anchored in scripts/adr-anchors.json. - `objectstack compile` now rejects a parent-scoped `readonlyWhen` on an object with no `master_detail` relationship, or two of them, so the runtime branch is a backstop rather than the plan. Tests: rule-validator unit cases, an engine-level regression over a real driver (the issue's INV-1003 shape, single-id + bulk + repoint + fail-closed), the record-scoped contrast, lint cases, and a dogfood proof that replays the issue's PATCH against the real showcase over HTTP. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- ...nt-scoped-readonly-when-server-enforced.md | 47 ++++ content/docs/data-modeling/fields.mdx | 30 +++ .../0058-expression-and-predicate-surface.md | 34 +++ .../0072-reference-scope-and-resolvability.md | 2 +- .../lint/src/validate-expressions.test.ts | 60 +++++ packages/lint/src/validate-expressions.ts | 70 +++++- .../src/engine-readonly-when-parent.test.ts | 222 ++++++++++++++++++ packages/objectql/src/engine.ts | 126 +++++++++- packages/objectql/src/master-detail.ts | 73 ++++++ .../src/validation/rule-validator.test.ts | 129 ++++++++++ .../objectql/src/validation/rule-validator.ts | 155 +++++++++++- .../test/expression-conformance.ledger.ts | 10 +- ...wcase-readonly-when-parent.dogfood.test.ts | 127 ++++++++++ packages/qa/dogfood/vitest.config.ts | 1 + scripts/adr-anchors.json | 5 + 15 files changed, 1077 insertions(+), 14 deletions(-) create mode 100644 .changeset/parent-scoped-readonly-when-server-enforced.md create mode 100644 packages/objectql/src/engine-readonly-when-parent.test.ts create mode 100644 packages/objectql/src/master-detail.ts create mode 100644 packages/qa/dogfood/test/showcase-readonly-when-parent.dogfood.test.ts diff --git a/.changeset/parent-scoped-readonly-when-server-enforced.md b/.changeset/parent-scoped-readonly-when-server-enforced.md new file mode 100644 index 0000000000..6e81781621 --- /dev/null +++ b/.changeset/parent-scoped-readonly-when-server-enforced.md @@ -0,0 +1,47 @@ +--- +"@objectstack/objectql": minor +"@objectstack/lint": minor +--- + +fix(objectql,lint): enforce parent-scoped `readonlyWhen` on the server (#4889) + +`readonlyWhen: P\`parent.status == 'paid'\`` — the documented "once the header +invoice is Paid, its lines are frozen" lock — was enforced **only in the client +grid**. The server-side strip bound `record` and `previous` and had no `parent` +at all, so every parent-scoped predicate faulted, took the fail-open branch, and +the write landed anyway. On the reference app that meant one `PATCH` rewrote the +quantity and unit price of a settled invoice's line: HTTP 200, value persisted, +the grid still drawing the cell read-only. ADR-0057 D10 puts enforcement on the +server and makes the client courtesy; here only the courtesy layer enforced. + +**`parent` is now bound on the write path.** For a detail object — one declaring +exactly one `master_detail` relationship — the engine resolves the master record +and binds it as `parent` before the strip runs, on both the single-id and the +bulk (`multi: true`) update paths. A repointing write is judged against the +master it *lands on*, not the one it leaves. The read is gated on the payload +actually touching a parent-scoped predicate (decided from the parsed CEL AST, so +a field named `parent_id` costs nothing), and the bulk path batch-reads the +distinct headers in one query rather than one per row. + +**An unbindable scope no longer waives the lock.** A `readonlyWhen` that names a +root the operation could not bind now resolves to **locked** — the field is +stripped — instead of "not locked". "The platform could not check this" must not +mean "allowed" on a field the author declared frozen. This is deliberately the +narrowest possible carve-out from the fail-open policy the strip has always had: +a predicate that is merely *broken* on the record (undeclared key, `null` +ordering overload, parse error, engine throw) still fails open exactly as +before, and `requiredWhen` / option `visibleWhen` are untouched. Recorded as an +addendum to ADR-0058's D5 fail-policy matrix, alongside the same narrowing +already made for validation predicates (#4649) and hook conditions (#4775). + +**And the runtime branch is a backstop, not the plan.** `objectstack compile` +now **rejects** a `parent`-scoped `readonlyWhen` on an object that declares no +`master_detail` relationship, or two of them (where the metadata does not say +which one is "the parent" and picking by declaration order would make a +data-integrity lock depend on field ordering). The common authoring mistake is +caught where it is cheap to fix, so it never reaches a runtime that has to judge +it — declared, not guessed. + +No metadata changes are required: an app whose parent-scoped locks were already +correct simply starts having them enforced. If you authored one on an object +with no single master, the build now names it. diff --git a/content/docs/data-modeling/fields.mdx b/content/docs/data-modeling/fields.mdx index fcb936538e..655d6ebf78 100644 --- a/content/docs/data-modeling/fields.mdx +++ b/content/docs/data-modeling/fields.mdx @@ -366,6 +366,36 @@ protocol 17 (#3855): authoring it is rejected with an error naming the replacement, not silently stripped, and `os migrate meta --from 16` rewrites existing sources automatically. +#### Locking a detail from its master: `parent` + +On a **detail** object — one that declares a `master_detail` relationship — a +`readonlyWhen` predicate may read the header record as `parent`. The canonical +case is "once the invoice is Paid, its lines are frozen": + +```typescript +quantity: Field.number({ + label: 'Qty', + required: true, + // `parent` = the record on the other end of this object's master_detail field. + readonlyWhen: P`parent.status == 'paid'`, +}), +``` + +The server binds `parent` on the update path by reading the master the row +points at (a repointing write is judged against the master it lands on), so the +lock holds against a direct API call, not only in the grid. + +Two rules make this predictable: + +- **Exactly one master.** `parent` resolves only when the object declares + exactly one `master_detail` relationship. With none — or with two, where the + metadata does not say which one is "the parent" — `objectstack compile` + rejects the predicate with an error naming the object. +- **An unresolvable `parent` means locked, not open.** If the header cannot be + read at write time, the field is treated as read-only rather than written. A + lock the platform could not evaluate is never waived: that is what makes the + declaration a guarantee instead of a hint. + ## Naming Conventions - Field names use `snake_case`: `first_name`, `annual_revenue`, `is_active` diff --git a/docs/adr/0058-expression-and-predicate-surface.md b/docs/adr/0058-expression-and-predicate-surface.md index 6c02294fb0..ca72bbe558 100644 --- a/docs/adr/0058-expression-and-predicate-surface.md +++ b/docs/adr/0058-expression-and-predicate-surface.md @@ -22,6 +22,40 @@ --- +> **Addendum (2026-08, #4889) — D5's "non-security ⇒ fail soft" line is NARROWED +> at the write-path gates: an UNEVALUABLE gate now fails CLOSED.** +> D5 sorts fail policy by *security-relevance*, and it put validation, hook and +> field predicates on the fail-soft side. Three findings since have shown that +> the sorting axis was one notch off for a specific subset: the predicates that +> **gate a write**. Fail-soft there does not mean "the rule was advisory"; it +> means "the guarantee the author declared did not happen, and nothing told +> anyone". The write returns 200, the client still renders the guard, and only a +> log line disagrees. +> +> Three instances, one direction: +> +> | # | Gate | Old | New | +> | :-- | :-- | :-- | :-- | +> | #4649 | object validation predicate (`script`/`cross_field`/`when`) | skip rule + log | **reject the write**, naming the rule and key | +> | #4775 | declarative hook `condition` | → `false` + log | **abort the operation** (`HookConditionError`) | +> | #4889 | field `readonlyWhen` whose predicate names an **unbound scope root** (e.g. `parent` with no master-detail header resolved) | → "not locked", write lands | **treat the field as LOCKED** (strip it) | +> +> The narrowing is deliberately *not* "every CEL fault is now fail-closed". A +> predicate that is simply BROKEN on this record — an undeclared key, a `null` +> ordering overload, a parse error, an engine throw — keeps D5's fail-soft +> policy at the field-predicate surfaces (`requiredWhen`, option `visibleWhen`, +> and every non-scope `readonlyWhen` fault), because the author has no remedy for +> an engine fault and bricking CRUD over one is the cure being worse. What +> changed is the case where the expression is **well-formed and supported** and +> the site simply could not bind what it names: there, "I could not check" must +> not resolve to "allowed", because the *declaration itself* says otherwise. +> +> Read the Pass-2 evidence table below as a snapshot of 2026-06, not as current +> behaviour, for those three rows. D5's tiering stands everywhere else — formula +> → `null` + log, flow → throw, security predicates fail closed. + +--- + ## TL;DR ObjectStack exposes **~50 authorable declarations** that hold an expression — formulas, visibility/required/readonly predicates, validation rules, hook conditions, flow/edge conditions, sharing-rule conditions, RLS `using`/`check`, action/view/app visibility, notification/ETL/export/sync/connector conditions — and they all funnel through **one authoring primitive** (`ExpressionInputSchema` → `{ dialect: 'cel', source }`, helpers `cel`/`F`/`P`). The authoring surface is already unified and clean. diff --git a/docs/adr/0072-reference-scope-and-resolvability.md b/docs/adr/0072-reference-scope-and-resolvability.md index 9036e28887..502e23adfe 100644 --- a/docs/adr/0072-reference-scope-and-resolvability.md +++ b/docs/adr/0072-reference-scope-and-resolvability.md @@ -53,7 +53,7 @@ for (const [k, v] of Object.entries(context.record)) | Flow / edge / decision | `service-automation/src/engine.ts:946-964` | `record`,`previous`, **flattened record fields**, flow `variables`, node outputs, `$runId`/`$flowName` | bare **and** `record.x` | | Formula field (`Field.expression`) | `objectql/src/engine.ts:119` | `{ now, timezone, user, org, record }` | `record.x` only | | Validation (`script`/`cross_field`/`when`) | `objectql/src/validation/rule-validator.ts:289` | `{ record: {...previous,...patch}, previous }` | `record.x` only | -| Field `visibleWhen`/`requiredWhen`/`readonlyWhen` | `objectql/src/validation/rule-validator.ts:178-190` | merged `record`, `previous`, (`parent` for master-detail) | `record.x` only | +| Field `visibleWhen`/`requiredWhen`/`readonlyWhen` | `objectql/src/validation/rule-validator.ts:178-190` | merged `record`, `previous`, (`parent` for master-detail — server-bound for `readonlyWhen` since #4889; the client grid binds it for all three) | `record.x` only | | Hook lifecycle `condition` | `objectql/src/hook-wrappers.ts:84` | `{ record }` | `record.x` only | | RLS `using`/`check` (compile→filter) | `plugin-security/src/rls-compiler.ts:259` | `current_user.*` (+ pre-resolved membership), record field names | field operands; pushdown subset only | | Sharing-rule `condition` (compile→filter) | `plugin-sharing/src/bootstrap-declared-sharing-rules.ts:61` | record fields only | field operands; pushdown subset only | diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index e637acb5ca..f076428e5b 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -468,6 +468,10 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { objects: [{ name: 'inv_line', fields: { + // #4889 — `parent` is bound from THIS relationship at write time, so + // the fixture declares it: a detail object without one has no header + // for the server to read (see the gate's own cases below). + inv: { type: 'master_detail', reference: 'inv' }, qty: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, note: { type: 'text', requiredWhen: 'record.qty >= 100' }, }, @@ -476,6 +480,62 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(0); }); + // #4889 — `parent`-scoped `readonlyWhen` is enforced by the SERVER binding + // the object's master-detail header. No single master ⇒ no binding ⇒ the + // write path holds the field locked forever. The metadata says so at build + // time, so the build says so. + describe('parent-scoped `readonlyWhen` needs a resolvable master (#4889)', () => { + 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 + qty: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, + }, + }); + expect(issues).toHaveLength(1); + expect(issues[0]!.severity).toBe('error'); + expect(issues[0]!.message).toMatch(/declares no `master_detail` relationships/); + }); + + 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' }, + qty: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, + }, + }); + expect(issues).toHaveLength(1); + expect(issues[0]!.message).toMatch(/declares 2 `master_detail` relationships/); + }); + + it('does not fire on a field literally named `parent_id`, or a `parent` string literal', () => { + expect(parentScopeIssues({ + name: 'node', + fields: { + parent_id: { type: 'text' }, + kind: { type: 'text' }, + a: { type: 'text', readonlyWhen: "record.parent_id != ''" }, + b: { type: 'text', readonlyWhen: "record.kind == 'parent'" }, + }, + })).toHaveLength(0); + }); + + it('is scoped to `readonlyWhen` — `requiredWhen`/`visibleWhen` verdicts are unchanged', () => { + expect(parentScopeIssues({ + name: 'orphan_line', + fields: { + qty: { type: 'number', requiredWhen: "parent.status == 'paid'", visibleWhen: "parent.status == 'paid'" }, + }, + })).toHaveLength(0); + }); + }); + 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 0dcc3f2d68..1253e05ffd 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -23,7 +23,7 @@ * See `validate-null-guards.ts` for the decision procedure and its scope. */ -import { validateExpression } from '@objectstack/formula'; +import { validateExpression, collectCelRootIdentifiers } from '@objectstack/formula'; import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import type { FlowNodeParsed } from '@objectstack/spec/automation'; @@ -147,6 +147,39 @@ function buildNullableFieldIndex(objects: AnyRec[]): Map> { return idx; } +/** + * [#4889] Does this CEL source read the `parent` root — the master-detail + * header the write path binds for a detail object's field predicates? + * + * Decided from the parsed AST (the same `collectCelRootIdentifiers` the runtime + * gate uses, so build and runtime can never disagree about what "reads + * `parent`" means), never a substring scan: a field named `parent_id`, or the + * string literal `'parent'`, is not a reference to the binding. A source that + * does not parse answers `false` — the ordinary syntax pass already reports it, + * and this gate must not report the same defect twice under a worse name. + */ +function readsParentRoot(source: string): boolean { + const roots = collectCelRootIdentifiers(source); + return roots.ok && roots.roots.includes('parent'); +} + +/** + * The number of `master_detail` relationships an object declares — what decides + * whether `parent` is a fact the metadata states (#4889). Exactly one ⇒ the + * write path binds that master as `parent`. Zero ⇒ nothing to bind. Two ⇒ no + * single "the parent", and picking one by declaration order would make a + * data-integrity lock depend on field ordering. + */ +function masterDetailCount(obj: AnyRec): number { + let n = 0; + for (const [, def] of fieldEntries(obj)) { + if (def.type !== 'master_detail') continue; + const ref = def.reference ?? def.referenceTo; + if (typeof ref === 'string' && ref.trim() !== '') n += 1; + } + return n; +} + /** The raw CEL source behind a predicate slot (string or `{ dialect, source }`). */ function celSourceOf(raw: unknown): string | undefined { if (typeof raw === 'string') return raw; @@ -372,6 +405,10 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // with `field.columnName` itself in #2377: the field no longer exists, so there // is no dual-source ambiguity to guard — external column mapping is `external.columnMap`.) + // [#4889] How many masters this object has, for the `parent`-scope gate + // below. Computed once per object, not per field. + const masters = masterDetailCount(obj); + for (const f of fieldList) { // Field-level conditional rules are server-enforced (rule-validator) and // record-scoped — a bare ref silently fails the rule (required/readonly @@ -381,6 +418,37 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const key of ['requiredWhen', 'readonlyWhen', 'conditionalRequired', 'visibleWhen'] as const) { check(`object '${objectName}' · field '${fname}' ${key}`, (f as AnyRec)[key], objectName, 'record'); } + // [#4889] A `parent`-scoped `readonlyWhen` is a SERVER-enforced lock: + // the write path resolves the object's master-detail header and binds + // it as `parent`. That binding exists only when the object declares + // exactly ONE `master_detail` relationship. With none — or with two, + // where the metadata does not say which one "the parent" is — the + // predicate can never evaluate, and the runtime then holds the field + // LOCKED forever (it will not wave a declared lock through just because + // it could not be checked). The metadata already contains everything + // needed to see that 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 as AnyRec).readonlyWhen); + if (masters !== 1 && roWhenSource && readsParentRoot(roWhenSource)) { + issues.push({ + where: `object '${objectName}' · field '${fname}' readonlyWhen`, + message: + `\`readonlyWhen\` 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. ` + + (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, + severity: 'error', + }); + } } if (f && typeof f === 'object' && f.formula) { // formulas are `value` role (any return type), still CEL. They are diff --git a/packages/objectql/src/engine-readonly-when-parent.test.ts b/packages/objectql/src/engine-readonly-when-parent.test.ts new file mode 100644 index 0000000000..7d459c665d --- /dev/null +++ b/packages/objectql/src/engine-readonly-when-parent.test.ts @@ -0,0 +1,222 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4889 — PARENT-scoped `readonlyWhen` is a SERVER guarantee. +// +// `readonlyWhen: parent.status == 'paid'` on `showcase_invoice_line.{quantity, +// unit_price}` reads, in the showcase's own words, "once the header invoice is +// Paid, its lines are frozen". It was enforced only in the client grid: the +// server-side strip bound `record` and `previous` and nothing else, so every +// `parent.*` predicate faulted, took the fail-OPEN branch, and the write landed +// with a 200 while the UI still drew the cell locked. ADR-0057 D10 puts +// enforcement on the SERVER and makes the client courtesy; this suite pins that +// direction end-to-end through the real engine + a real driver, not through the +// strip function in isolation (PD #10: a `case` label is not enforcement — +// check the CALL SITE). +// +// 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. + +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 readonlyWhen is enforced server-side (#4889)', () => { + 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(); + // The showcase's shape, trimmed to what the lock needs + // (`examples/app-showcase/src/data/objects/invoice.object.ts`). + 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 (invoice.object.ts L144). + tax_rate: { type: 'number', readonlyWhen: "record.status == 'paid'" }, + }, + } as any); + engine.registry.registerObject({ + name: 'showcase_invoice_line', + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + // PARENT-scoped (invoice.object.ts L201/L219/L225). + product: { type: 'lookup', reference: 'showcase_product', readonlyWhen: "parent.status == 'paid'" }, + quantity: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, + 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' }, + }, + } 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 }); + 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' }); + }); + + const line = (id: string) => storeFor('showcase_invoice_line').get(id); + const invoice = (id: string) => storeFor('showcase_invoice').get(id); + + 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. + await engine.update('showcase_invoice_line', { id: 'line_paid', quantity: 9999, unit_price: 0.01 }); + expect(line('line_paid')).toMatchObject({ quantity: 6, unit_price: 49.99 }); + }); + + it('reports the strip to the caller as reason `readonly_when` (#3407 observability)', async () => { + const events: any[] = []; + await engine.update( + 'showcase_invoice_line', + { id: 'line_paid', quantity: 9999 }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([ + { object: 'showcase_invoice_line', fields: ['quantity'], reason: 'readonly_when' }, + ]); + }); + + it('leaves an unlocked field on the SAME locked row writable', async () => { + await engine.update('showcase_invoice_line', { id: 'line_paid', quantity: 9999, description: 'renamed' }); + expect(line('line_paid')).toMatchObject({ quantity: 6, description: 'renamed' }); + }); + + it('does NOT lock a line whose header is still draft (no false positives)', async () => { + await engine.update('showcase_invoice_line', { id: 'line_draft', quantity: 42, unit_price: 12.5 }); + expect(line('line_draft')).toMatchObject({ quantity: 42, unit_price: 12.5 }); + }); + + it('judges a REPOINT against the master the write lands on, not the one it leaves', async () => { + // Moving a draft line onto the PAID invoice: the incoming `invoice` value is + // the master whose state decides the lock. + await engine.update('showcase_invoice_line', { id: 'line_draft', invoice: 'INV-1003', quantity: 777 }); + expect(line('line_draft')).toMatchObject({ invoice: 'INV-1003', quantity: 3 }); + }); + + it('holds the field LOCKED when the header cannot be resolved (fail-CLOSED)', async () => { + storeFor('showcase_invoice_line').set('orphan', { id: 'orphan', invoice: 'GONE', quantity: 1 }); + await engine.update('showcase_invoice_line', { id: 'orphan', quantity: 9999 }); + // An unresolvable `parent` is NOT read as "unlocked" — the declared lock is + // not waived just because the platform could not check it. + expect(line('orphan')).toMatchObject({ quantity: 1 }); + }); + + it('enforces the lock on the BULK path too, per matched row (#3042 shape)', async () => { + // One paid line + one draft line in the match set. `readonlyWhen` locked in + // ≥1 matched row ⇒ the field is dropped for the whole batch. + await engine.update('showcase_invoice_line', { quantity: 9999 }, { where: { description: 'seat' }, multi: true } as any); + expect(line('line_paid')).toMatchObject({ quantity: 6 }); + expect(line('line_draft')).toMatchObject({ quantity: 3 }); + }); + + it('leaves a bulk edit that touches only draft-header lines alone', async () => { + await engine.update('showcase_invoice_line', { quantity: 55 }, { where: { invoice: 'INV-1004' }, multi: true } as any); + expect(line('line_draft')).toMatchObject({ quantity: 55 }); + expect(line('line_paid')).toMatchObject({ quantity: 6 }); + }); + + it('CONTRAST: the record-scoped lock on the header still works (and still only when TRUE)', async () => { + await engine.update('showcase_invoice', { id: 'INV-1003', tax_rate: 99 }); + expect(invoice('INV-1003')).toMatchObject({ tax_rate: 8 }); + + await engine.update('showcase_invoice', { id: 'INV-1004', tax_rate: 99 }); + expect(invoice('INV-1004')).toMatchObject({ tax_rate: 99 }); + }); + + it('reads the header ONCE per single-id write, and not at all without a parent-scoped lock', 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_paid', quantity: 9999 }); + expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(1); + + // A payload touching no parent-scoped field pays nothing. + reads.length = 0; + await engine.update('showcase_invoice_line', { id: 'line_paid', description: 'note' }); + expect(reads.filter((r) => r === 'showcase_invoice')).toHaveLength(0); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 8cc23d4073..d55c1100b6 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -84,7 +84,8 @@ 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, stripReadonlyFields } from './validation/rule-validator.js'; +import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, hasParentScopedReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; +import { resolveMasterDetailRelation } from './master-detail.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; import { applyHaving } from './having-filter.js'; import { @@ -633,6 +634,24 @@ function isEmptyReferenceValue(v: unknown): boolean { return false; } +/** + * [#4889] The master id a detail row's `parent` binding resolves against: + * the write's own value for the FK when it carries one (a REPOINT must be + * judged against the master it lands on), else the prior row's. Only a scalar + * id counts — an expanded relation object or an array is not an id this read + * can bind, and guessing one would be worse than leaving `parent` unbound. + */ +function masterIdOf( + fk: string, + data: Record | null | undefined, + row: Record | null | undefined, +): string | number | undefined { + const raw = data && fk in data ? data[fk] : row?.[fk]; + if (typeof raw === 'string') return raw === '' ? undefined : raw; + if (typeof raw === 'number' && Number.isFinite(raw)) return raw; + return undefined; +} + /** * RFC-4122 v4 uuid for the realtime `DataEvent.id` (#4626). * @@ -2395,6 +2414,90 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * [#4889] The master-detail header a detail row's `parent`-scoped + * `readonlyWhen` reads, or `null` when this write cannot resolve one. + * + * `readonlyWhen: parent.status == 'paid'` is a documented **server** + * guarantee (ADR-0057 D10 puts enforcement here; the client grid is + * courtesy), but the strip is a pure function over the payload and the prior + * row — it has no driver and cannot fetch a header. So the engine resolves it + * and passes it in. + * + * The header id comes from the payload first, then the prior row: a write that + * REPOINTS the detail at another master must be judged against the master it + * is landing on, not the one it is leaving. Read as **system**: the lock is a + * data-integrity property of the header's state, not of the caller's + * visibility of it, and the caller's right to touch this detail at all was + * already settled upstream (RLS / `controlled_by_parent`, ADR-0055) before the + * write reached the strip. + * + * `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. + */ + private async resolveMasterDetailParent( + schema: any, + data: Record | null | undefined, + priorRow: Record | null | undefined, + ): Promise | null> { + const rel = resolveMasterDetailRelation(schema); + if (!rel) return null; + const parentId = masterIdOf(rel.fk, data, priorRow); + 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; + } catch (err) { + this.logger?.warn?.('readonlyWhen parent lookup failed — parent stays unbound', { + object: rel.master, id: parentId, error: err, + }); + return null; + } + } + + /** + * Bulk counterpart of {@link resolveMasterDetailParent}: one read for the + * whole matched set, then a per-row lookup for + * `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. + */ + private async resolveMasterDetailParents( + schema: any, + data: Record | null | undefined, + priorRows: ReadonlyArray> | null | undefined, + ): Promise<(row: Record | undefined) => Record | null> { + const unbound = () => null; + const rel = resolveMasterDetailRelation(schema); + if (!rel) return unbound; + const ids = new Set(); + for (const row of priorRows ?? []) { + const id = masterIdOf(rel.fk, data, row); + if (id != null) ids.add(String(id)); + } + if (ids.size === 0) return unbound; + const byId = new Map>(); + try { + const rows = await this.find(rel.master, { + where: { id: { $in: [...ids] } }, + context: { isSystem: true }, + } as any) as Array>; + for (const row of Array.isArray(rows) ? rows : []) { + if (row?.id != null) byId.set(String(row.id), row); + } + } catch (err) { + this.logger?.warn?.('readonlyWhen parent lookup failed — parent stays unbound', { + object: rel.master, error: err, + }); + return unbound; + } + return (row) => { + const id = masterIdOf(rel.fk, data, row); + return id == null ? null : (byId.get(String(id)) ?? null); + }; + } + /** * [#4551] Report stored references that resolve to nothing. **Read-only** — * this issues no writes at all. @@ -4768,7 +4871,16 @@ export class ObjectQL implements IObjectQLEngine { // field is read-only for this record's state, so the incoming // change is ignored (the persisted value is kept). const preRoWhen = hookContext.input.data as Record; - hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, preRoWhen, priorRecord, this.logger) as any; + // [#4889] A `parent`-scoped predicate ("once the header invoice + // is Paid, its lines are frozen") needs the master-detail header + // bound as `parent`. Only the engine can fetch it, so the strip + // 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) + ? await this.resolveMasterDetailParent(updateSchema, preRoWhen, 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 // non-system callers (system writes legitimately set read-only @@ -4828,7 +4940,15 @@ export class ObjectQL implements IObjectQLEngine { // single-id `stripReadonlyWhenFields`; INSERT stays exempt. if (payloadHasReadonlyWhen) { const preRoWhenMulti = hookContext.input.data as Record; - hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger) as any; + // [#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) + ? await this.resolveMasterDetailParents(updateSchema, preRoWhenMulti, priorRows) + : undefined; + hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, preRoWhenMulti, priorRows, this.logger, parentForRow) as any; reportDroppedFields(preRoWhenMulti, hookContext.input.data as Record, 'readonly_when'); } // [#2948] Same static-`readonly` write guard on the bulk path — diff --git a/packages/objectql/src/master-detail.ts b/packages/objectql/src/master-detail.ts new file mode 100644 index 0000000000..911d4365bc --- /dev/null +++ b/packages/objectql/src/master-detail.ts @@ -0,0 +1,73 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Master-detail relation resolution (ADR-0035). + * + * A detail object declares its owner as a `master_detail` field whose + * `reference` names the master object. ADR-0035 makes that relationship the + * single declaration point for the whole master-detail story — inline grids, + * cascade delete, `controlled_by_parent` sharing (ADR-0055), roll-up summaries + * — and it is also what gives a child field's `readonlyWhen` / `requiredWhen` + * predicate its `parent` binding (#1581, ADR-0072's scope table). + * + * This module answers exactly one question — *which field points at the master, + * and which object is it?* — so the write path can resolve the header row + * without re-deriving the relationship at each call site. `plugin-security` + * keeps its own `controlled_by_parent` resolver: that one is deliberately more + * permissive (it falls back to a required `lookup`) because a CBP object with + * no derivable master must deny reads rather than open them, which is a + * different question from "what does `parent` mean in this predicate". + * + * ## Why `parent` is not guessed + * + * The resolution is deliberately narrow: a `master_detail` field, and only when + * the object declares exactly ONE of them. A junction object with two masters + * has no single "the parent", and picking one by declaration order would make + * 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. + */ + +/** The child→master link: the FK field on the detail, and the master object. */ +export interface MasterDetailRelation { + /** Field on the DETAIL object holding the master's id. */ + fk: string; + /** Object name of the MASTER (header) record. */ + master: string; +} + +/** The subset of a field definition this resolution reads. */ +export interface RelationFieldDef { + type?: string; + /** Canonical reference target. `referenceTo` is the stored-row spelling. */ + reference?: string; + referenceTo?: string; +} + +/** The reference target a relation field names, or `undefined`. */ +function referenceOf(def: RelationFieldDef | null | undefined): string | undefined { + const raw = def?.reference ?? def?.referenceTo; + return typeof raw === 'string' && raw.trim() !== '' ? raw : undefined; +} + +/** + * The object's master-detail relation, or `null` when it has none — or when it + * has more than one and "the parent" is therefore not a fact the metadata + * states (see the module doc). + */ +export function resolveMasterDetailRelation( + objectSchema: { fields?: Record } | undefined | null, +): MasterDetailRelation | null { + const fields = objectSchema?.fields; + if (!fields) return null; + let found: MasterDetailRelation | null = null; + for (const [name, def] of Object.entries(fields)) { + if (def?.type !== 'master_detail') continue; + const master = referenceOf(def); + if (!master) continue; + if (found) return null; // ambiguous — two masters, no single `parent` + found = { fk: name, master }; + } + return found; +} diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index c0244aade2..659dc218b0 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -8,6 +8,7 @@ import { stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, + hasParentScopedReadonlyWhenInPayload, stripReadonlyFields, } from './rule-validator.js'; import { ValidationError } from './record-validator.js'; @@ -146,6 +147,134 @@ describe('stripReadonlyWhenFieldsMulti (#3042)', () => { }); }); +// #4889 — PARENT-scoped `readonlyWhen`. The showcase's invoice-line shape: +// `readonlyWhen: parent.status == 'paid'` on a detail whose master is the +// invoice header. Until #4889 the server bound only `record`/`previous`, so +// every one of these predicates faulted and the fail-open branch wrote the +// field anyway — the client grid was the only thing enforcing the lock. +const invoiceLineFields = { + fields: { + invoice: { type: 'master_detail', reference: 'showcase_invoice', required: true }, + // Row-scoped — unaffected by the parent binding, and here to prove it. + description: { type: 'text', requiredWhen: 'record.quantity >= 100' }, + quantity: { type: 'number', readonlyWhen: "parent.status == 'paid'" }, + unit_price: { type: 'currency', readonlyWhen: "parent.status == 'paid'" }, + }, +}; + +describe('parent-scoped readonlyWhen (#4889)', () => { + const line = { id: 'l1', invoice: 'inv1', quantity: 6, unit_price: 49.99 }; + + it('drops the field when the master-detail header locks it', () => { + const out = stripReadonlyWhenFields( + invoiceLineFields, + { quantity: 9999, unit_price: 0.01 }, + line, + undefined, + { id: 'inv1', status: 'paid' }, + ); + expect(out).toEqual({}); + }); + + it('KEEPS the field when the header is not locked', () => { + const out = stripReadonlyWhenFields( + invoiceLineFields, + { quantity: 9999 }, + line, + undefined, + { id: 'inv1', status: 'draft' }, + ); + expect(out).toEqual({ quantity: 9999 }); + }); + + it('treats the field as LOCKED when `parent` could not be bound (fail-CLOSED)', () => { + // The pre-#4889 behaviour, and the bug: no `parent` binding ⇒ the predicate + // faults ⇒ the write used to go through. It must now be dropped. + const warnings: string[] = []; + const out = stripReadonlyWhenFields(invoiceLineFields, { quantity: 9999 }, line, { + warn: (m: string) => warnings.push(m), + } as never); + expect(out).toEqual({}); + expect(warnings.some((w) => w.includes("reads 'parent'") && w.includes('LOCKED'))).toBe(true); + }); + + it('keeps fail-OPEN for a predicate that is simply broken (undeclared key)', () => { + // Not an unbound root — `record` IS bound, the key under it is not declared. + // #4649 deliberately left this fail-open for field predicates; #4889 must + // not have widened itself into that case. + const warnings: string[] = []; + const out = stripReadonlyWhenFields( + { fields: { amount: { type: 'currency', readonlyWhen: "record.no_such_field == 'paid'" } } }, + { amount: 999 }, + { amount: 1 }, + { warn: (m: string) => warnings.push(m) } as never, + ); + expect(out).toEqual({ amount: 999 }); + expect(warnings.some((w) => w.includes('change allowed through'))).toBe(true); + }); + + it('leaves a RECORD-scoped lock on the same object working unchanged', () => { + // The contrast the issue drew: `record.status == 'paid'` was correct all + // along, and stays correct with a parent bound alongside it. + const headerFields = { fields: { status: { type: 'select' }, tax_rate: { type: 'number', readonlyWhen: "record.status == 'paid'" } } }; + expect(stripReadonlyWhenFields(headerFields, { tax_rate: 99 }, { status: 'paid', tax_rate: 8 })).toEqual({}); + expect(stripReadonlyWhenFields(headerFields, { tax_rate: 99 }, { status: 'draft', tax_rate: 8 })).toEqual({ tax_rate: 99 }); + }); + + it('binds a DIFFERENT parent per matched row on the bulk path', () => { + const rows = [ + { id: 'l1', invoice: 'paid_inv', quantity: 1 }, + { id: 'l2', invoice: 'draft_inv', quantity: 2 }, + ]; + const headers: Record> = { + paid_inv: { id: 'paid_inv', status: 'paid' }, + draft_inv: { id: 'draft_inv', status: 'draft' }, + }; + // Locked in ≥1 matched row ⇒ dropped for the whole batch (#3042 rule). + expect( + stripReadonlyWhenFieldsMulti(invoiceLineFields, { quantity: 9999 }, rows, undefined, + (row) => headers[String(row?.invoice)] ?? null), + ).toEqual({}); + // No matched row locked ⇒ a legitimate bulk edit still lands. + expect( + stripReadonlyWhenFieldsMulti(invoiceLineFields, { quantity: 9999 }, [rows[1]!], undefined, + (row) => headers[String(row?.invoice)] ?? null), + ).toEqual({ quantity: 9999 }); + }); + + it('is fail-CLOSED on the bulk path too when a row has no resolvable parent', () => { + const out = stripReadonlyWhenFieldsMulti( + invoiceLineFields, + { quantity: 9999 }, + [{ id: 'l1', invoice: 'gone', quantity: 1 }], + undefined, + () => null, + ); + expect(out).toEqual({}); + }); +}); + +describe('hasParentScopedReadonlyWhenInPayload (#4889 gate)', () => { + it('is TRUE when the payload writes a parent-scoped readonlyWhen field', () => { + expect(hasParentScopedReadonlyWhenInPayload(invoiceLineFields, { quantity: 1 })).toBe(true); + }); + it('is FALSE when the payload only writes fields with no parent-scoped lock', () => { + expect(hasParentScopedReadonlyWhenInPayload(invoiceLineFields, { description: 'x' })).toBe(false); + }); + it('is FALSE for a record-scoped readonlyWhen (no needless header read)', () => { + expect(hasParentScopedReadonlyWhenInPayload(invoiceFields, { amount: 1 })).toBe(false); + }); + it('does not mistake a field NAMED parent_id, or a string literal, for the binding', () => { + const decoys = { + fields: { + a: { type: 'text', readonlyWhen: "record.parent_id != ''" }, + b: { type: 'text', readonlyWhen: "record.kind == 'parent'" }, + }, + }; + expect(hasParentScopedReadonlyWhenInPayload(decoys, { a: 'x', b: 'y' })).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 37f3aa55eb..3ae1b858e0 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -94,6 +94,24 @@ * field-level predicates evaluate far more often anyway, since their fault mode * was the same missing key. * + * ## `readonlyWhen`: the UNBOUND-ROOT case is fail-CLOSED (#4889) + * + * One carve-out was added to the paragraph above, and only one. A + * `readonlyWhen` predicate that faults because it names a scope ROOT this + * operation did not bind — `parent.status == 'paid'` with no master-detail + * header in hand — is not a broken predicate; it is a supported construct the + * evaluation site could not answer, and answering "not locked" writes a field + * the author declared frozen. That single case now resolves to LOCKED. Every + * OTHER `readonlyWhen` fault (undeclared key, null overload, parse error) keeps + * the fail-open policy this section describes, and `requiredWhen` / option + * `visibleWhen` are untouched. See {@link isReadonlyWhenLocked}. + * + * This is a NARROWING of ADR-0058 D5's "non-security predicate ⇒ fail soft" + * line, recorded as an addendum on that ADR alongside the same narrowing #4649 + * and #4775 made at the two neighbouring write gates. Do not widen it back on + * 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. + * * 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, @@ -115,7 +133,7 @@ * evaluator once per matched row — one payload, N priors (#3106). */ -import { ExpressionEngine } from '@objectstack/formula'; +import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { AUDIT_PROVENANCE_FIELDS } from '@objectstack/spec/data'; import Ajv, { type ValidateFunction } from 'ajv'; @@ -130,7 +148,7 @@ import { // that evaluate CEL against "the record" cannot drift apart on what that record // contains — see the module's own doc comment. import { materializeDeclaredFields } from '../declared-fields.js'; -import { describeCelFault } from '../cel-fault.js'; +import { describeCelFault, unknownVariableOf } from '../cel-fault.js'; type Mode = 'insert' | 'update'; @@ -256,18 +274,37 @@ export function needsPriorRecord( return !!(ruleNeeds || fieldsNeedPrior(objectSchema?.fields)); } +/** + * The master-detail header a `parent`-scoped predicate reads (#4889). `null` + * means "this operation could not resolve one" — which is NOT the same as + * "there is none to read": see {@link isReadonlyWhenLocked} for why the two + * resolve to opposite verdicts. + */ +export type ParentBinding = Record | null | undefined; + /** * 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 * ignored (the persisted value is kept) rather than rejected. Returns the same * object when nothing is locked, else a shallow copy with the locked keys - * removed. A broken predicate is fail-open (the change is allowed through). + * removed. + * + * `parent` (#4889) is the master-detail header row, bound for a detail object + * so a `parent.` predicate — the documented "once the header invoice is + * Paid, its lines are frozen" lock — evaluates here and not only in the client + * grid. The engine resolves it (it owns the driver) and passes it in; pass + * `undefined` when the object is not a detail or the payload's predicates never + * name `parent`, and the binding is simply absent. + * + * A predicate that faults is fail-open (the change is allowed through) EXCEPT + * when the fault is an unbound scope root — see {@link isReadonlyWhenLocked}. */ export function stripReadonlyWhenFields( objectSchema: { fields?: Record } | undefined | null, data: Record | undefined | null, previous: Record | undefined | null, logger?: EvaluateRulesOptions['logger'], + parent?: ParentBinding, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; @@ -275,7 +312,7 @@ export function stripReadonlyWhenFields( 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)) { + if (isReadonlyWhenLocked(def, merged, previous ?? undefined, name, logger, parent)) { if (result === data) result = { ...data }; delete (result as Record)[name]; logger?.warn?.(`Field '${name}' is read-only (readonlyWhen) — ignoring incoming change`); @@ -287,10 +324,38 @@ export function stripReadonlyWhenFields( /** * Evaluate one field's `readonlyWhen` predicate against a (merged) record. * TRUE ⇒ the field is locked for that record and the incoming change must be - * dropped. A broken predicate is fail-open (returns `false` — the change is - * allowed through), matching the strip's historical behaviour. Shared by the - * single-id ({@link stripReadonlyWhenFields}) and bulk + * dropped. Shared by the single-id ({@link stripReadonlyWhenFields}) and bulk * ({@link stripReadonlyWhenFieldsMulti}) strips. + * + * ## Two faults, two answers (#4889) + * + * Until #4889 every fault took one exit — WARN and `false`, "not locked" — and + * that single answer had to serve two very different situations: + * + * - **The predicate is broken on this record.** A typo'd key, a `null` + * ordering overload, a parse error. The author has a bug; the field is not + * demonstrably locked; the historical (and deliberate, documented) policy is + * fail-OPEN. Unchanged here — an engine fault the author cannot act on must + * not brick every write to the object. + * + * - **The predicate names a ROOT this operation did not bind.** `parent.status + * == 'paid'` where no master-detail header was resolved. The expression is + * well-formed and spec-sanctioned; nothing about the RECORD says the field + * is unlocked — we simply could not ask. Waving it through inverts the + * guarantee: a field the author declared locked is written, the API answers + * 200, and the client grid still draws the cell as read-only, so the UI and + * the database disagree with nobody told. ADR-0057 D10 puts enforcement on + * the server; a lock that fails open leaves enforcement in the courtesy + * layer. So an unbound root resolves to LOCKED — conservative toward the + * author's declared intent, and the direction #4649 (validation predicates) + * and #4775 (hook conditions) already took for their own unevaluable case. + * + * The two are distinguishable without guessing: cel-js says `Unknown variable: + * ` for the second and `No such key` / an overload / a parse fault for + * the first ({@link unknownVariableOf}). This is a LAST resort, not the plan — + * `@objectstack/lint` rejects a `parent`-scoped `readonlyWhen` on an object with + * no master-detail relation at build time, so the common authoring mistake never + * reaches a runtime this branch has to judge. */ function isReadonlyWhenLocked( def: ConditionalFieldDef, @@ -298,18 +363,77 @@ function isReadonlyWhenLocked( previous: Record | undefined, name: string, logger?: EvaluateRulesOptions['logger'], + parent?: ParentBinding, ): boolean { const res = ExpressionEngine.evaluate(toExpression(def.readonlyWhen!), { record: merged, previous, + // Bound ONLY when resolved. An absent binding is what makes the unbound-root + // fault below reachable, and that fault is the signal — binding `null` here + // would turn it into a `No such key` and re-open the fail-open hole. + ...(parent != null ? { extra: { parent } } : {}), }); if (!res.ok) { + const unbound = unknownVariableOf(res.error); + if (unbound) { + logger?.warn?.( + `readonlyWhen for '${name}' reads '${unbound}', which is not bound for this operation — ` + + `treating the field as LOCKED (the declared lock is not waived because it could not be evaluated). ` + + `A 'parent'-scoped predicate needs the object to declare exactly one master_detail relationship.`, + ); + return true; + } logger?.warn?.(`readonlyWhen for '${name}' failed to evaluate — change allowed through`); return false; } return res.value === true; } +/** + * True when at least one `readonlyWhen` predicate the UPDATE payload touches + * reads the `parent` root (#4889) — the gate the engine uses to decide whether + * to resolve the master-detail header at all, so an object with no + * parent-scoped lock pays nothing. + * + * Decided from the parsed CEL AST ({@link collectCelRootIdentifiers}), not a + * substring scan, so a field literally named `parent_id` or a string constant + * `'parent'` cannot be mistaken for the binding. A predicate that does not + * parse answers `false`: it will fault at evaluation, where the fail-open / + * fail-closed judgment already lives — this gate does not duplicate it. + */ +export function hasParentScopedReadonlyWhenInPayload( + objectSchema: { fields?: Record } | undefined | null, + data: Record | undefined | null, +): boolean { + const fields = objectSchema?.fields; + if (!fields || !data) return false; + for (const [name, def] of Object.entries(fields)) { + if (!def?.readonlyWhen || !(name in data)) continue; + if (readsParentRoot(def.readonlyWhen)) return true; + } + return false; +} + +/** Parsed-root memo — metadata predicates are a small, fixed set of sources. */ +const parentRootCache = new Map(); + +/** Does this predicate's CEL source reference the `parent` root? */ +function readsParentRoot(cond: string | Expression): boolean { + const expr = toExpression(cond); + if (expr.dialect !== 'cel') return false; + const source = typeof expr.source === 'string' ? expr.source : ''; + if (!source) return false; + const cached = parentRootCache.get(source); + if (cached !== undefined) return cached; + const roots = collectCelRootIdentifiers(source); + const answer = roots.ok && roots.roots.includes(PARENT_ROOT); + parentRootCache.set(source, answer); + return answer; +} + +/** The CEL scope root a master-detail header is bound under (`cel-engine.ts`). */ +const PARENT_ROOT = 'parent'; + /** * True when the UPDATE payload writes at least one field that declares a * `readonlyWhen` predicate. A cheap gate the engine uses to decide whether the @@ -343,6 +467,13 @@ export function hasReadonlyWhenInPayload( * predicate is fail-open for that row. INSERT is exempt (update path only), * symmetric with the single-id strip. * + * `parentForRow` (#4889) supplies each matched row's master-detail header, since + * N rows can hang off N different masters — the bulk counterpart of the + * single-id `parent` binding. The engine batch-reads the headers once and hands + * over a lookup; `undefined` (or a resolver returning nothing) leaves the + * binding absent for that row, which {@link isReadonlyWhenLocked} reads as + * LOCKED for a predicate that needs it. + * * Returns the same object when nothing is stripped, else a shallow copy with the * locked keys removed. */ @@ -351,6 +482,7 @@ export function stripReadonlyWhenFieldsMulti( data: Record | undefined | null, priorRows: ReadonlyArray> | undefined | null, logger?: EvaluateRulesOptions['logger'], + parentForRow?: (row: Record | undefined) => ParentBinding, ): Record | undefined | null { const fields = objectSchema?.fields; if (!fields || !data) return data; @@ -359,7 +491,14 @@ export function stripReadonlyWhenFieldsMulti( for (const [name, def] of Object.entries(fields)) { if (!def?.readonlyWhen || !(name in data)) continue; const lockedInSomeRow = rows.some((row) => - isReadonlyWhenLocked(def, { ...(row ?? {}), ...data }, row ?? undefined, name, logger), + isReadonlyWhenLocked( + def, + { ...(row ?? {}), ...data }, + row ?? undefined, + name, + logger, + parentForRow?.(row ?? undefined), + ), ); if (lockedInSomeRow) { if (result === data) result = { ...data }; diff --git a/packages/qa/dogfood/test/expression-conformance.ledger.ts b/packages/qa/dogfood/test/expression-conformance.ledger.ts index d1d42a2e4c..dd128cd5bc 100644 --- a/packages/qa/dogfood/test/expression-conformance.ledger.ts +++ b/packages/qa/dogfood/test/expression-conformance.ledger.ts @@ -94,8 +94,16 @@ export const EXPRESSION_SURFACE: ExprSurface[] = [ { id: 'cel-field-rule', summary: 'field UI rules (requiredWhen / readonlyWhen / visibleWhen)', + // `fail-soft-log` remains the tier for this row: a predicate that is BROKEN + // on the record (undeclared key, null overload, parse fault) is logged and + // skipped, on all three slots. One case is carved out and is not + // representable in the D5 enum — a `readonlyWhen` naming a scope root the + // write path could not bind (`parent` with no master-detail header) is held + // LOCKED rather than waved through (#4889, ADR-0058 addendum). The carve-out + // is inside the same evaluator and the same surface, so it does not split + // into a second row. dialect: 'cel', mode: 'interpret', state: 'enforced', failPolicy: 'fail-soft-log', - enforcement: '@objectstack/formula celEngine (interpret) — console (objectui) + server', + enforcement: '@objectstack/formula celEngine (interpret) — console (objectui) + server (rule-validator `readonlyWhen` strip binds `record`/`previous`, plus the master-detail header as `parent`)', covers: [ 'data/field.zod.ts:requiredWhen', 'data/field.zod.ts:readonlyWhen', diff --git a/packages/qa/dogfood/test/showcase-readonly-when-parent.dogfood.test.ts b/packages/qa/dogfood/test/showcase-readonly-when-parent.dogfood.test.ts new file mode 100644 index 0000000000..f2dcfd878f --- /dev/null +++ b/packages/qa/dogfood/test/showcase-readonly-when-parent.dogfood.test.ts @@ -0,0 +1,127 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4889 — PARENT-scoped `readonlyWhen` over the REAL showcase, over real HTTP. +// +// The showcase declares, on `showcase_invoice_line.{product,quantity,unit_price}`: +// +// readonlyWhen: P`parent.status == 'paid'` +// // "once the header invoice is Paid, its lines are frozen" +// +// It was enforced in the client grid only. The server strip bound `record` and +// `previous` and had no `parent`, so every one of those predicates faulted, the +// fail-OPEN branch let the write through, and a single PATCH rewrote the +// quantity and unit price of a settled invoice's line — HTTP 200, value +// persisted, while the grid still drew the cell locked. ADR-0057 D10 makes the +// SERVER the enforcement point and the client courtesy; here it was inverted. +// +// This is the issue's own repro, run against the shipped metadata rather than a +// hand-built fixture: the unit + engine suites in `@objectstack/objectql` pin +// the mechanism, and this pins that the mechanism is actually wired to the +// surface a caller reaches. +// +// The record-scoped contrast the issue drew — `showcase_invoice.tax_rate` with +// `readonlyWhen: record.status == 'paid'`, correct all along — rides along, so a +// future change cannot repair one by breaking the other. + +import { describe, it, expect, beforeAll } from 'vitest'; +import { type VerifyStack } from '@objectstack/verify'; +import { getSharedShowcase } from './shared-showcase.js'; + +const LINES = '/data/showcase_invoice_line'; +const INVOICES = '/data/showcase_invoice'; +const idOf = (r: any) => r?.id ?? r?.record?.id ?? r?.data?.id ?? r; +const recordOf = (b: any) => b?.record ?? b?.data ?? b; + +describe('showcase: parent-scoped readonlyWhen is server-enforced (#4889)', () => { + let stack: VerifyStack; + let token: string; + let paidInvoiceId: string; + let draftInvoiceId: string; + let paidLineId: string; + let draftLineId: string; + + beforeAll(async () => { + stack = await getSharedShowcase(); + token = await stack.signIn(); // the issue's own principal: admin@objectos.ai + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const ql = await stack.kernel.getServiceAsync('objectql'); + const sys = { context: { isSystem: true } }; + + // Own fixtures (uniquely named — the stack is shared across files). + const account = idOf(await ql.insert('showcase_account', { name: 'Frozen Lines Co #4889', status: 'prospect' }, sys)); + const product = idOf(await ql.insert('showcase_product', { name: 'Frozen Widget #4889', unit_price: 49.99 }, sys)); + + // A SETTLED invoice — the INV-1003 shape from the issue. `issued_on` / + // `paid_on` satisfy the header's own `requiredWhen` rules for `paid`. + paidInvoiceId = idOf(await ql.insert('showcase_invoice', { + name: 'INV-4889-PAID', account, status: 'paid', tax_rate: 8, + issued_on: '2026-01-05', paid_on: '2026-01-20', + }, sys)); + draftInvoiceId = idOf(await ql.insert('showcase_invoice', { + name: 'INV-4889-DRAFT', account, status: 'draft', tax_rate: 8, + }, sys)); + + paidLineId = idOf(await ql.insert('showcase_invoice_line', { + invoice: paidInvoiceId, product, description: 'frozen line', quantity: 6, unit_price: 49.99, + }, sys)); + draftLineId = idOf(await ql.insert('showcase_invoice_line', { + invoice: draftInvoiceId, product, description: 'open line', quantity: 6, unit_price: 49.99, + }, sys)); + expect(paidLineId && draftLineId).toBeTruthy(); + }, 60_000); + + const readLine = async (id: string) => + recordOf(await (await stack.apiAs(token, 'GET', `${LINES}/${id}`)).json()); + const readInvoice = async (id: string) => + recordOf(await (await stack.apiAs(token, 'GET', `${INVOICES}/${id}`)).json()); + + it("THE REGRESSION: a paid invoice's frozen line cannot be rewritten over the API", async () => { + const before = await readLine(paidLineId); + expect(before.quantity, 'fixture precondition').toBe(6); + + // Verbatim from the issue report. + const res = await stack.apiAs(token, 'PATCH', `${LINES}/${paidLineId}`, { + quantity: 9999, unit_price: 0.01, + }); + // The strip is SILENT by contract (a legal drop, not an error) — the + // regression is about what LANDS, not about the status code. + expect(res.status, 'the strip is silent: the request still succeeds').toBe(200); + + const after = await readLine(paidLineId); + expect(after.quantity, 'frozen quantity must survive the PATCH').toBe(6); + expect(after.unit_price, 'frozen unit price must survive the PATCH').toBe(49.99); + }); + + it('tells the caller what it dropped (X-ObjectStack-Dropped-Fields, #3431)', async () => { + const res = await stack.apiAs(token, 'PATCH', `${LINES}/${paidLineId}`, { quantity: 1234 }); + expect(res.status).toBe(200); + expect(res.headers.get('X-ObjectStack-Dropped-Fields') ?? '').toContain('quantity;reason=readonly_when'); + }); + + it('leaves an unlocked field on the same frozen line writable', async () => { + const res = await stack.apiAs(token, 'PATCH', `${LINES}/${paidLineId}`, { + quantity: 4242, description: 'note added after settlement', + }); + expect(res.status).toBe(200); + const after = await readLine(paidLineId); + expect(after.quantity, 'still frozen').toBe(6); + expect(after.description, 'the field the author left open still lands').toBe('note added after settlement'); + }); + + it('does NOT freeze a line whose header is still draft', async () => { + const res = await stack.apiAs(token, 'PATCH', `${LINES}/${draftLineId}`, { quantity: 42 }); + expect(res.status).toBe(200); + expect((await readLine(draftLineId)).quantity, 'an open invoice edits normally').toBe(42); + }); + + it('CONTRAST: the record-scoped lock on the header still behaves as it always did', async () => { + const locked = await stack.apiAs(token, 'PATCH', `${INVOICES}/${paidInvoiceId}`, { tax_rate: 99 }); + expect(locked.status).toBe(200); + expect((await readInvoice(paidInvoiceId)).tax_rate, 'paid ⇒ tax rate frozen').toBe(8); + + const open = await stack.apiAs(token, 'PATCH', `${INVOICES}/${draftInvoiceId}`, { tax_rate: 12 }); + expect(open.status).toBe(200); + expect((await readInvoice(draftInvoiceId)).tax_rate, 'draft ⇒ tax rate editable').toBe(12); + }); +}); diff --git a/packages/qa/dogfood/vitest.config.ts b/packages/qa/dogfood/vitest.config.ts index 29cb4daf98..4627c450c1 100644 --- a/packages/qa/dogfood/vitest.config.ts +++ b/packages/qa/dogfood/vitest.config.ts @@ -28,6 +28,7 @@ const SHARED_SHOWCASE = [ 'test/showcase-permission-zoo.dogfood.test.ts', 'test/showcase-private-owd.dogfood.test.ts', 'test/showcase-public-read-owd.dogfood.test.ts', + 'test/showcase-readonly-when-parent.dogfood.test.ts', 'test/showcase-search.dogfood.test.ts', 'test/showcase-static-readonly.dogfood.test.ts', 'test/two-doors-permission.dogfood.test.ts', diff --git a/scripts/adr-anchors.json b/scripts/adr-anchors.json index 2efe46e27e..4beba4f963 100644 --- a/scripts/adr-anchors.json +++ b/scripts/adr-anchors.json @@ -1,6 +1,11 @@ { "//": "ADR anchors — see scripts/check-adr-anchors.mjs. Each entry pins the ADR ids that MUST stay referenced in a file whose behaviour an accepted ADR decided. Add an entry when an ADR's decision is realized in code that would look arbitrary (or wrong) to someone reading the file alone.", "anchors": [ + { + "file": "packages/objectql/src/validation/rule-validator.ts", + "adrs": ["ADR-0057", "ADR-0058"], + "invariant": "A declared field lock is the SERVER's to enforce (ADR-0057 D10 — the client grid is courtesy). A `readonlyWhen` whose predicate names a scope root the write path could not bind resolves to LOCKED, not to \"not locked\": \"could not check\" must never read as \"allowed\" on a field the author declared frozen. This narrows ADR-0058 D5's fail-soft tier deliberately and only for that case — a merely BROKEN predicate (undeclared key, null overload, parse fault, throw) still fails open, and requiredWhen / option visibleWhen are untouched." + }, { "file": "packages/spec/src/identity/membership-role.ts", "adrs": ["ADR-0090", "ADR-0108"],