Skip to content

Commit ae0c90c

Browse files
fix(objectql): judge each readonlyWhen lock against the row the update stores, not a value another lock drops (#19911) (#19923)
Fixes #19911 Clause-②: no ## What a caller could do before, and what happens now **Before.** When one field's `readonlyWhen` read a field that carries its own `readonlyWhen`, a caller with edit rights could change a locked field by sending a new value for the other field in the same update. The conditional strip judged every lock against one `record` view built before any lock was judged. That view still held every value the strip then dropped. With `status` locked by `previous.status == 'closed'` and `amount` by `record.status == 'closed'`, `update(c1, { status: 'open', amount: 999 })` on a closed row stored `{ status: 'closed', amount: 999 }`. The same happened on bulk (`multi: true`) updates, for `isSystem` callers (a `readonlyWhen` lock binds them), and for a master-detail FK's own lock judged inside the PR #19877 settlement: the line moved to another invoice although its own lock held on the row it kept. The mechanism also over-locked in reverse: a dropped value that WOULD lock another field locked it, though the row never took that value. **Now.** Each lock is judged with its own incoming value and every OTHER dropped key reverted to the stored value. No field is written while its lock is TRUE on the row the update stores. The strip then releases, in one step, every dropped key that is unlocked on that row, and keeps the result only when every dropped key is locked and every kept key is unlocked on the row it stores. When that step does not settle the set (locks that read each other in a cycle, or a cascade where releasing one key moves another's verdict, #19927), the larger fail-safe drop set stands. This applies on all four conditional call sites (by-id and bulk strips, and the by-id and bulk FK judgements of the settlement). ## A1: reproduction on `origin/main` `a34c27cbe5`, before any edit Real `ObjectQL` engine, the PR #19905 suite's in-memory driver shape. Object `case_x`: `status` `readonlyWhen: "previous.status == 'closed'"`, `amount` `readonlyWhen: "record.status == 'closed'"`, rows `c1`, `c2` closed with `amount: 100`. - By id: `update(c1, { status: 'open', amount: 999 })` stored `{ status: 'closed', amount: 999 }`, event `[{ status, readonly_when }]`. Control `update(c1, { amount: 555 })` stayed 100. **Reproduced.** - Bulk (`where: { note: 'k' }, multi: true`): both rows stored `amount: 999`. Control stayed 100. **Reproduced.** ## A2: the candidates, measured All five variants (the base and four candidates) were run on the same 20 files: the nine suites PR #19905's body names (its own `engine-readonly-when-stored-view` included) and every other objectql test file that mentions `readonlyWhen` (674 tests). **Every variant passes 674/674**, so the existing pins do not tell the candidates apart. They were compared on the card's shape and on a 27-row probe of every movement class instead. The expected rows were checked against a brute-force enumeration of every drop set that agrees with the stored row. | shape (exact drop sets by enumeration) | base | (i) monotone fixpoint | **(iv) fixpoint + exact release (chosen)** | (ii) judge against the stored row | (iii) refuse when locks interact | |:---|:---|:---|:---|:---|:---| | the card, by id and bulk (`{status, amount}`) | amount 999 **under-lock** | 100 | 100 | 100 | refused | | legitimate reopen: `status` unlocked, amount edited (`{}`) | both land | both land | both land | amount dropped **over-lock** | both land | | close + edit on an open row (`{amount}`) | amount dropped | dropped | dropped | amount 999 lands on a closed row **under-lock** | dropped | | REVERSE: frozen `status`, `closed` + amount (`{status}`) | amount dropped **over-lock** | dropped **over-lock** | amount lands | lands | refused | | lock reading its own field, open row over the cap | dropped | dropped | dropped | 5000 lands **under-lock** | dropped | | three-lock chain (`{c,b}`) | `{c,a}` | `{c,b,a}` over-locks `a` | `{c,b}` | `{c,b}` | refused | | four-lock chain (`{c,z,y}`) | `{c}` | `{c,x,z,y}` over-locks `x` | `{c,z,y}` | `{c,z,y}` | refused | | two-lock cycle (NONE) | `{a}` | `{a,b}` | `{a,b}` fail-safe | `{b}` | refused | | FK's own `record` lock reads a stage its own lock keeps | FK lands, amount 999 | FK stays, 100 | FK stays, 100 | FK stays, 100 | refused | - **(i) monotone fixpoint.** Never opens a lock, and its first pass is the old single pass. It **over-locks**: a key dropped in an early pass whose own lock is FALSE on the final row. Measured on a two-field shape (REVERSE, where the base over-locks too, so (i) keeps that defect) and on the three- and four-lock chains (the four-lock `x` is a new over-lock). - **(ii) judge every lock against the stored row.** It reverts every judged key, a key's own value included. It over-locks the legitimate reopen, and it **opens locks the base holds**: close + edit writes the amount onto a now-closed row, and a lock that reads its own field lets 5000 past a 1000 cap. It also reads `record` as `previous` for those fields. Rejected. - **(iii) refuse.** Moves accept to refuse on every interacting shape. The documented behaviour is to *ignore* a locked write, not refuse it (quoted below). Rejected. Measured in its narrowest form, refusing only when a drop moves another lock's verdict; a syntactic "reads a dropped field" rule would refuse a superset. - **(iv) chosen.** (i), then ONE release: every dropped key that is unlocked on (i)'s row is released at once. The result is kept only if it is exact (every dropped key locked, every kept key unlocked, on the row it stores). Otherwise (i)'s answer stands. It never opens a lock: no field is written while its lock is TRUE on the row the update stores. It reaches the unique exact drop set on every measured shape except the three-lock cascade (#19927): there, one release step does not settle the set (releasing `x` moves `y`'s verdict), so the fixpoint's larger drop set `{c, x, y}` stands instead of `{c, y}`, and a field whose own lock is FALSE on the stored row is still dropped, as on base. The cycle has no exact set at all and takes the same fail-safe answer. It changes no documented behaviour: every movement below brings the stored row into line with the documented rule. It keeps every existing pin, as all candidates do. ## The fix - `packages/objectql/src/validation/rule-validator.ts` - `settleReadonlyWhenDrops`: the fixpoint and the exact release. Each key is judged with its own incoming value (a lock reading its own field judges the write) and the other drops reverted. Views are memoised per drop set. - `stripReadonlyWhenFields` and `stripReadonlyWhenFieldsMulti` now route through it, with no new arguments at existing call sites. Bulk means "locked in at least one matched row" for every evaluation. - Warnings come from each key's deciding evaluation and are logged once, in declaration order. - `ReadonlyWhenStripOptions.only`: the strip judges every key but takes, and speaks about, only that key, and only when it is taken. - `readonlyWhenFkJudgementReadsParent`: the settlement's question "does judging the FK's lock need the header it names?". - The `parent`-root reader now caches roots per source, so it answers for `record` too. - Neither the options type nor the strips are exported from the package (`index.ts` and `core.ts` export none of them), so no published surface moves. - `packages/objectql/src/engine.ts`, `settleMasterDetailLanding` and its two `judgeFkLock` closures - The FK's own lock is judged with every caller-supplied lock (`only: fk`) instead of alone. This is how a value another lock drops is reverted before the FK's lock reads it. - The named header is read when the FK's lock reads `parent` (as before), or reads `record` while another payload key's lock reads `parent`. - A landing FK now stays in `supplied`, so the strip that judges the rest re-judges it on the same landing and settles the same drop set. An FK that does not land keeps PR #19877's rule: its verdict is final and never re-asked against the header the row keeps. - One new import line, placed away from the rule-validator import line PR #19728 edits. ## Every movement (base `a34c27cbe5` to head `d01b912f3d`; patch round 1 changed no code, so `611a2fc561` answers the same) | write | before | now | |:---|:---|:---| | the card, by id and bulk | `amount: 999`; event `[status]` | `amount: 100`; one event `[status, amount]` `readonly_when` | | the card under `strictReadonlyWrites` | refused, `fields: ['status']` | refused, `fields: ['status', 'amount']`, `drops` one `readonly_when` event (a refusal before and now) | | the card, `isSystem` / `preserveAudit` / a hook echoing the caller's `status` | `amount: 999` | `amount: 100` | | REVERSE by id and bulk (frozen `status`) | amount dropped; event `[status, amount]` | `amount: 999` lands; event `[status]` | | REVERSE under `strictReadonlyWrites` | refused, `['status', 'amount']` | refused, `['status']` | | own-field lock beside a dropped reopen | `amount: 500` on a closed row | `amount: 100` | | three-lock chain | stored `{c: L, b: x, a: 1}` | stored `{c: L, b: y, a: 2}` | | two-lock cycle | `{a}` dropped | `{a, b}` dropped (no exact set exists) | | `requiredWhen` requiring `note` while the amount is above 500, on the card | refused `VALIDATION_FAILED` | commits, amount 100 | | `requiredWhen` requiring `note` while the amount is below 500, the card clearing `note` | commits `{closed, 999, null}` | refused `VALIDATION_FAILED`, nothing lands | | settlement, FK lock reads a stage its own lock keeps, by id and bulk | line moves to `inv_b`, amount 999 | stays under `inv_a`, amount 100; event `[stage, invoice, amount]` | | settlement, stays to moves: the FK's own `record` lock reads a value that is itself locked under the header the update names (`invoice` locked by `record.amount == 'big'`, `amount` by `parent.status == 'paid'`; line under open `inv_b`, the update names paid `inv_a` with `amount: 'big'`), by id and bulk | line stays under `inv_b` and stores `amount: 'big'`; event `[invoice]`; header reads `[inv_b]` | line moves to `inv_a` and keeps `amount: 'small'`; event `[amount]`; header reads `[inv_a]`, then the repoint's reference check on `inv_a`. Both rows agree with their locks | | the same write under `strictReadonlyWrites` | refused, `fields: ['invoice']` | refused, `fields: ['amount']`, nothing lands | | three-lock cascade (`c` locked by `previous.c == 'L'`, `x` by `record.c == 'open'`, `y` by `record.x == 'xv'`; row `c: 'L'`; the update sets all three), by id and bulk | drops `{c, x, y}` | drops `{c, x, y}`, unchanged; the exact set is `{c, y}` (#19927) | | same FK lock, no `parent`-scoped lock (plain strip) | FK moves | FK stays | | PR #19877's `inv_line_moored` shape (stage not in the payload) | header reads `[inv_a]` | `[inv_b, inv_a]`: one extra read, same row, same event | **Unmoved (pinned):** both controls; a hook-written `status` and a hook overwriting the caller's `status` (both land and are read); a hook-written `amount`; the legitimate reopen by id, bulk and strict (nothing dropped); close + edit; an own-field lock over and under its cap; an FK lock reading only `previous` (one header read); a landing FK's fail-open fault warning, said once. PR #19905's 18 pins and PR #19877's 38 (its whole file) are green. **Warn lines:** a newly dropped key gains its drop line and a released key loses it. A key re-judged in a later pass speaks from its deciding evaluation, once. A landing FK's fault warnings now print with the rest, in declaration order, instead of first. When the FK stands on its own lock but the static strip takes it, its conditional fault warning is no longer printed. ## A3: all four call sites By-id strip, bulk strip, by-id `judgeFkLock` and bulk `judgeFkLock` all run the same settlement; no site is exempt. The FK's verdict is consistent when it lands: the re-judge is the same computation over the same header. When the FK's lock reads neither `record` nor `parent`, its verdict does not depend on the view. PR #19905's `stored` view is the base every view is built from, so a value the static strip takes is never read either. ## Tests New `packages/objectql/src/engine-readonly-when-interdependent-locks.test.ts`, 35 cases (32 in round 1, plus three stays-to-moves pins by id, bulk and under `strictReadonlyWrites` in patch round 1): the card by id and bulk with both controls, the report and warn lines, the strict envelope (`code` `ERR_READONLY_FIELD_REJECTED`, `name`, `fields`, `drops`; the engine error carries no HTTP `status`, which is mapped downstream and not measured here), `isSystem`, `preserveAudit`, four hook shapes, the legitimate reopen (by id; bulk plus strict), close + edit, REVERSE by id / bulk / strict, the own-field lock (two cases), the chain, the cycle, both `requiredWhen` directions (refusal asserted on `code` `VALIDATION_FAILED` + `fields`), the settlement by id and bulk with its header reads, the plain-strip FK, the moored read count, the `previous`-only FK, the landing FK's single warning, and two unit cases for `only`. On `d01b912f3d` (patch round 1): - `pnpm --filter @objectstack/objectql exec vitest run --project local --maxWorkers=2`: 307 files / 5158 tests passed, exit 0. - `pnpm --filter @objectstack/objectql typecheck`: exit 0, `check:test-typecheck: OK`. The new file is in the `tsconfig.test.json` program (`--listFiles`: 1) with 0 diagnostics. `pnpm --filter @objectstack/objectql test:repo`: 5 passed. - **Ablation** (fix committed; `engine.ts` and `rule-validator.ts` restored to their `a34c27cbe5` blobs `e973ab50ac` / `2b3002b8e1` with `git restore --source`, tree only; on-disk hashes verified and markers `settleReadonlyWhenDrops` / `readonlyWhenFkJudgementReadsParent` counted 0 / 0). The three suites ran **23 failed / 68 passed**: every new pin failed except the 12 whose behaviour is unchanged, the three stays-to-moves pins included, and PR #19905's 18 and PR #19877's 38 stayed green. The subject is imported from source (`./engine.js`), so no `dist` sits on the path. Restored with `git checkout HEAD --`: blobs `c9b1cfc19e` / `f3934b80f6` equal `HEAD`, `git diff HEAD` empty, rerun 91/91 passed. The script carried an `EXIT` / `INT` / `TERM` trap. ## Gates on `611a2fc561` Patch round 1 (`d01b912f3d`) changed docblocks, the changeset and three pins over the same four paths. It re-ran the objectql tests and typecheck, `node scripts/check-issue-citations.mjs` (23 citations resolve, #19927 included), `node scripts/check-changeset-no-major.mjs --base origin/main`, `pnpm check:nul-bytes` and the narrowed eslint run: exit 0 each. The full derivation below ran on `611a2fc561`. - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`: 65 commands, each run with its exit code captured before any pipe. `--ran`: `65 derived famil(ies) accounted for — 65 run, 0 NOT-MEASURED (a DERIVED zero …)`, exit 0. `check:dual-build-cjs-loads`, `check:lean-entry-closure` and `check:type-check-debt` first answered PREREQUISITE NOT MET (exit 3, no workspace `dist`). After `pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*' --concurrency=2` (72/72 tasks) all three exit 0, and the record carries the reruns. - `node scripts/check-issue-citations.mjs` (live, the verdict CI blocks on): exit 0, 22 citations resolve. - `node scripts/check-system-context-census.mjs`: exit 0 (no new `isSystem` read). `pnpm check:query-options-erasure`: exit 0. - The three roster gates the derivation marks as keeping a roster under this diff's directories: `node scripts/check-changeset-fixed.mjs`, `pnpm check:authz-resolver`, `pnpm check:filter-alias-parity`, exit 0 each. - Lint, narrowed and proven. eslint's own config (`ESLint.isPathIgnored` / `calculateConfigForFile`) ignores the changeset and lints the three TypeScript files with `typescript-eslint/parser` and no `parserOptions.project` / `projectService`. `eslint --no-inline-config --format json` over them gives 3 results, 0 errors, 0 warnings. The config enables no type-aware linting, and its only file reads are two baselines this diff does not touch, so this diff cannot move a verdict on any untouched file. `pnpm lint` itself is CI's. ## Neighbour PR #19728 This PR stays textually disjoint from it and does not wait on it. Driver-free bare probe (`git clone --bare --shared`, no `merge.*` driver registered): `merge-tree --write-tree` of `d01b912f3d` (and before it `611a2fc561`) against its head `3b9c5f2fca` exits 0, and against `origin/main` `beac798026` exits 0. The merged tree carries both changes (`readonlyWhenFkJudgementReadsParent` 3 in `engine.ts`, `settleReadonlyWhenDrops` 6 in `rule-validator.ts`, #19728's `RelatedRecordBinding` 2 and 5). ## Acceptance notes - **Cycle residue.** When locks read each other in a cycle, no drop set can make every dropped key locked AND every kept key unlocked on the stored row, and no rule can promise both halves. This one keeps the fail-safe half (a lock that cannot be settled is not waived), as the bulk strip's "locked in at least one row" rule already does. - **Cost.** A write where nothing locks runs one pass, as before. Each drop adds one pass over the standing keys and one re-check of the dropped keys; a full check pass runs only when something is over-locked. The settlement reads one extra header in the moored shape. Noted, not measured. - **In-tree usage.** The tree has no case of one `readonlyWhen` reading another `readonlyWhen` field. Examples and platform objects were scanned: the showcase `invoice` locks read `status` / `parent.status`, and neither carries a lock. The card's shape is the natural "a closed case stays closed" plus "a closed case's amount is frozen". - User docs are untouched and still true after the fix; see the declaration note. ## Declaration note for the contract review The claim declares no widening; copied above as given. The fix moves a refuse/accept answer only through the validation rules that run on the stripped payload, and moves strip verdicts toward the documented rule: - **Accept direction.** A write refused `VALIDATION_FAILED` only because an amount the lock should have held raised a requirement now commits without it. The REVERSE shape's amount, which the base silently dropped, now lands. Both remove a verdict the published text already negated. `content/docs/data-modeling/fields.mdx` ("Conditional Logic") says `readonlyWhen` is a "CEL predicate; field is read-only when `TRUE`", and "The server enforces `requiredWhen` on submit and ignores writes to fields whose `readonlyWhen` predicate is `TRUE`". ("Who the lock applies to") says "A TRUE `readonlyWhen` predicate locks the field for **every API-boundary caller**". `content/docs/data-modeling/formulas.mdx` binds `record` to "the row being evaluated". On the row the update stores, the REVERSE amount's predicate is FALSE and the card's is TRUE. - **Refuse direction.** A write that cleared a field the kept amount requires is now refused: a restored guarantee. - `strictReadonlyWrites`: every shape that moves carries a conditional drop, so it was a refusal before and still is; only `fields` / `drops` move, and they grow on the card and shrink on REVERSE. Every other movement is a strip (the write still commits). No authorable key, export or error code moves. --- _Generated by [Claude Code](https://claude.ai/code/session_01TEhopqrWQYBycZzyJHpAZr)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent beac798 commit ae0c90c

4 files changed

Lines changed: 975 additions & 68 deletions

File tree

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): a value one `readonlyWhen` lock drops can no longer unlock another `readonlyWhen` lock (#19911)
6+
7+
**What a caller could do before.** When one field's `readonlyWhen` read a field
8+
that carries its own `readonlyWhen`, a caller with edit rights could change a
9+
locked field by sending a new value for the other one in the same update. With
10+
`status: { readonlyWhen: "previous.status == 'closed'" }` and `amount: {
11+
readonlyWhen: "record.status == 'closed'" }`, `update(c1, { status: 'open',
12+
amount: 999 })` on a CLOSED row committed `amount = 999`: `status` was dropped
13+
by its own lock, but `amount` was judged against the dropped `'open'`, so the
14+
row stayed closed with its frozen amount rewritten. The same happened on bulk
15+
(`multi: true`) updates for every matched row, and for a master-detail field's
16+
own `readonlyWhen` lock that reads such a field — the row moved to another
17+
header although its lock held on the row it kept. `isSystem` callers were
18+
affected too (a `readonlyWhen` lock binds them).
19+
20+
**What happens now.** A value one `readonlyWhen` lock drops can no longer
21+
unlock another: no field is written while its `readonlyWhen` is TRUE on the row
22+
the update stores. The locks are judged together, again with each dropped
23+
value put back to the row's stored one, until no further field locks; then a
24+
field that was held only by a value that was later put back is released, if
25+
the result agrees with the stored row. In the example above `amount` is
26+
dropped as locked, exactly as `update(c1, { amount: 999 })` on its own always
27+
was. Values a `beforeUpdate` hook wrote are still stored and read as before.
28+
29+
**What else you may see move:**
30+
31+
- The reverse: a value that WOULD lock another field no longer locks it when
32+
its own lock drops it. With `status` frozen by `previous.frozen == true`,
33+
`update(r, { status: 'closed', amount: 999 })` now stores the amount (the row
34+
stays open, so its amount is unlocked); before, the amount was dropped too.
35+
- `onFieldsDropped` reports every dropped field in the one `readonly_when`
36+
event, and a `strictReadonlyWrites` refusal names the fields the update would
37+
have dropped; it was a refusal before and still is.
38+
- `requiredWhen` and validation rules run on the stripped update, so they see
39+
the amount the row keeps: a requirement only the let-through amount raised
40+
no longer refuses the write, and clearing a field the kept amount requires is
41+
now refused (`VALIDATION_FAILED`).
42+
- The release is one step, not a search. When it does not settle the drops,
43+
every lock involved holds and the field is dropped, never written — so a
44+
field whose own lock is FALSE on the stored row can still be dropped. That
45+
happens when locks read each other in a cycle (no set of drops agrees with
46+
the stored row), and in a cascade where releasing one field changes another's
47+
verdict: with `c` locked by `previous.c == 'L'`, `x` by `record.c == 'open'`
48+
and `y` by `record.x == 'xv'`, `update(r, { c: 'open', x: 'xv', y: 'yv' })`
49+
on a row with `c: 'L'` drops all three, although `x` is unlocked on the
50+
stored row. That was dropped before this change too; it is tracked as
51+
#19927.
52+
- A master-detail repoint that the field's own `record`-scoped lock used to
53+
hold can now land. Its lock is judged together with the other locks on the
54+
header the update names, so when the value that lock reads is itself locked
55+
under that header, the value is dropped, the repoint lands, and the edit is
56+
dropped under the header the row lands on. With `invoice: { readonlyWhen:
57+
"record.amount == 'big'" }` and `amount: { readonlyWhen: "parent.status ==
58+
'paid'" }`, `update(line, { invoice: 'inv_a', amount: 'big' })` on a line
59+
under an open invoice, naming a paid one, used to keep the line where it was
60+
and store `amount: 'big'` (`onFieldsDropped` reported `invoice`); it now
61+
moves the line onto the paid invoice and keeps its old amount
62+
(`onFieldsDropped` reports `amount`), by id and on bulk updates. A
63+
`strictReadonlyWrites` refusal of that write now names `amount` instead of
64+
`invoice`. Both outcomes agree with the locks on the row they store.
65+
- A master-detail field whose own lock reads `record`, on an object where
66+
another field in the update has a `parent`-scoped lock, now reads the named
67+
header before deciding whether the row moves: one more header read when it
68+
does not.

0 commit comments

Comments
 (0)