Repository navigation
Commit 3bd221d
Fixes #19853
Clause-②: no
## What a caller could do before, and what happens now
**Before.** On a master-detail child whose parent field is `readonly:
true`, a caller with edit rights could change a field locked by a
`parent`-scoped `readonlyWhen` by naming a different, unlocked parent in
the same update. The engine resolved `parent` from the FK the payload
named, judged the lock against that header, and only afterwards stripped
the read-only FK — so the row stayed under the locked parent with the
locked field rewritten. The same happened when the FK carried its own
`readonlyWhen` lock that kept the row where it was, and on bulk (`multi:
true`) updates for every matched row.
**Now.** The engine settles whether the update really moves the row
BEFORE it judges any `parent`-scoped lock, and judges every lock (and
`requiredWhen`, which shares the binding) against the parent the row is
stored under afterwards. A legitimate move — FK writable, or an
`isSystem` / `preserveAudit` write the static strip exempts — is still
judged against the parent it moves to.
## A1 — reproduction on `origin/main` 2bbb462, engine untouched
Real `ObjectQL` engine, in-memory driver (the suite's own), the card's
shape: `inv_line.amount` has `readonlyWhen: "parent.status == 'paid'"`,
`inv_line.invoice` is `master_detail` with `readonly: true`, `l1`
(amount 100) under paid `inv_a`, `inv_b` open.
- `update(l1, { amount: 999, invoice: 'inv_b' })` committed and stored
`{ invoice: 'inv_a', amount: 999 }` — reproduced.
- Control `update(l1, { amount: 555 })` — locked, amount stayed 100.
- Same run, the other arms: FK's own record-scoped lock → stored
`amount: 999` under `inv_a`; FK's own parent-scoped lock → same; bulk
stripped repoint → same per row; reverse direction (naming the PAID
invoice for a line under an open one) → the amount was dropped
(over-lock); `requiredWhen: "parent.status == 'paid'"` → a note-only
edit of an open line naming the paid invoice was refused `po_ref is
required`, and clearing `po_ref` on a paid line by naming the open one
was accepted.
## A2 — every strip that can move the FK on this path, from the code
Order on both update branches, after the before-phase hooks: primary-key
strip (`id` only) → `readonlyWhen` strip → static `readonly` strip
(skipped for `isSystem`) → `assertNoStrictDrops` →
`evaluateValidationRules` → reference check → driver.
- **`readonlyWhen` strip** — can take the FK when the FK carries its own
lock. Ran AFTER `parent` was resolved from the named FK.
- **Static `readonly` strip** — takes a non-system caller's FK when it
is `readonly: true` (unless `preserveAudit` keeps it, or a hook wrote
it). Ran after the `readonlyWhen` strip.
- Not strips: field-level security refuses a forbidden FK outright
(plugin-security step 2.5 throws `PermissionDeniedError`); the #16344
hide pass withholds static-readonly values from hooks and restores them
before any engine-owned read (net zero); `beforeUpdate` hooks,
`normalizeMultiValueFields` and `validateRecord` all run before `parent`
is resolved, so their effect is already in the payload the resolution
reads; the primary-key strip touches `id` only.
## The fix — `packages/objectql/src/engine.ts`
`settleMasterDetailLanding` (one private method, used by the by-id and
the bulk branch) runs before the `readonlyWhen` strip:
1. the FK's own `readonlyWhen` lock is judged ALONE (the strip's
`supplied` subset holds only the FK) against the header the FK names —
#4889's verdict for the FK itself, byte-identical to before; the strip
that judges the other fields is then handed a `supplied` without the FK,
so that verdict is never re-asked against a different header;
2. the static strip's verdict on the FK is asked of the same
`stripReadonlyFields`, silently, with the same `supplied`,
`hookWrittenKeys` and `preserveAudit`; its `isSystem` gate is read once
per branch into a const that both the settlement and the real strip
consume (keeps the system-context census at 110 sites, and the two
cannot disagree);
3. `parent` is resolved from the view the write stores — the payload, or
the payload without the FK — so `masterIdOf` falls through to the prior
row's FK when the repoint does not land.
The `requiredWhen` non-regression pre-check's "does this write repoint"
is asked of the same stored view. The header read keeps `opCtx.context`
(the #19837 tenant scope) exactly as it was.
## A3 — what moves
- `parent` for every `parent`-scoped `readonlyWhen` and `requiredWhen`
is the header the row is stored under (the fix).
- `onFieldsDropped` for the card's write: `[{amount, readonly_when},
{invoice, readonly}]` (was `[{invoice, readonly}]`); a
`strictReadonlyWrites` refusal names `['amount', 'invoice']` with both
drops (was `['invoice']`).
- Over-lock removed: naming a locked parent beside a stripped FK no
longer drops a field of a row that stays under an open parent.
- `requiredWhen` moves with the shared binding, both directions (see the
declaration note below).
- Header reads: a stripped repoint reads the header the row keeps
instead of the named one (same count, pinned); the `requiredWhen`
pre-check no longer buys a second read for a repoint that does not land;
a `parent`-scoped lock on the FK ITSELF that refuses the landing costs
two reads (named, then kept) where it cost one.
- Warn-line order only: when the FK's own lock fires, its line now
prints before the other fields' lines. Report order, strict `fields`
order and message text are unchanged.
- Unmoved: the FK's own verdict; strip order; `isSystem` /
`preserveAudit` behaviour; every write whose payload does not name the
FK.
## A4 — the legitimate repoint
With `invoice` writable (`inv_line_free`): paid → open `update(f1, {
invoice: 'inv_b', amount: 999 })` is judged against `inv_b` and commits
both; open → paid `update(f2, { invoice: 'inv_a', amount: 999 })` is
judged against `inv_a` and drops the amount while the move lands. In
both the judged header is the one the row is stored under because the FK
lands: no strip takes it.
## Tests
New `describe` block in
`packages/objectql/src/engine-readonly-when-parent.test.ts` (16 cases):
the card, the control, the report, the strict envelope (`code`
`ERR_READONLY_FIELD_REJECTED`, `name`, `fields`, `drops`; the engine
error carries no HTTP `status` — that mapping is downstream and not
measured here), the reverse over-lock, the one-header read, `isSystem`,
`preserveAudit`, both legitimate repoint directions, the FK's own
record- and parent-scoped locks, both `requiredWhen` directions (refusal
asserted on `code` `VALIDATION_FAILED` + `fields`), and bulk in both
directions.
Measured on `0a538af9d7`:
- `pnpm --filter @objectstack/objectql exec vitest run --project local
--maxWorkers=2` → 305 files / 5105 tests passed (exit 0).
- `pnpm --filter @objectstack/objectql test:repo` → 1 file / 5 tests
passed; `pnpm --filter @objectstack/objectql typecheck` → exit 0,
`check:test-typecheck: OK`.
- Ablation (committed fix; `engine.ts` restored to its `2bbb462335` blob
`ffcda81540` and verified on disk, `settleMasterDetailLanding` count 0):
the parent suite ran 11 failed / 27 passed — every new pin except the
five whose behaviour is unchanged (control, `isSystem`, `preserveAudit`,
both legitimate repoints). Restored with `git checkout HEAD --`, blob
`624fc1e38a` equals `HEAD:packages/objectql/src/engine.ts`, `git diff
HEAD` empty, rerun 38/38 passed. Script carried an EXIT/INT/TERM trap.
## Gates on `0a538af9d7`
- `node scripts/pm/dispatch-gates.mjs --commands` → 64 commands; all run
with exit codes captured before any pipe; `--ran` → `64 derived
famil(ies) accounted for — 64 run, 0 NOT-MEASURED`, exit 0.
- `check:dual-build-cjs-loads` and `check:type-check-debt` first
answered PREREQUISITE NOT MET (no workspace `dist`); after `turbo run
build` over `./packages/*` and `./packages/*/*` (72/72 tasks), both exit
0 (`type-check-debt` needed an objectql rebuild after the ablation
restore touched `engine.ts`'s mtime).
- `node scripts/check-issue-citations.mjs` (live, the verdict CI blocks
on) → exit 0, 13 citations resolve.
- `node scripts/check-system-context-census.mjs` → exit 0 (110 sites).
It was red on the first commit (`d4496752a1`) for a new `isSystem` read
inside the helper; the second commit hoists the one read per branch
instead.
- `pnpm check:query-options-erasure` → exit 0.
- Lint, narrowed and proven: eslint's own config
(`ESLint.calculateConfigForFile` / `isPathIgnored`) puts `engine.ts` and
the test file in the linted population and ignores the changeset;
`eslint --no-inline-config --format json` over the three paths → 3
results, 0 errors, 0 warnings on the two linted files; the config
enables no type-aware linting and no cross-file rule (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
Textually disjoint: this diff does not touch the
`evaluateValidationRules` lines, the import line, the region after
`resolveMasterDetailParents` or `system-context.mdx`. Driver-free bare
probe (`git clone --bare --shared`, no merge driver registered):
`merge-tree --write-tree` of `0a538af9d7` against `3b9c5f2fca` → exit 0;
against `origin/main` `b940f32a56` → exit 0. The merged tree carries
both changes.
## Acceptance notes
- **Sibling on the `record` root, same defect class, not fixed here
(listed for the seat to file).** A record-scoped `readonlyWhen` that
reads a static-`readonly` field is judged against the caller's forged
value of that field, which the static strip then removes. Measured on
`0a538af9d7`: `status` `readonly: true`, `amount` `readonlyWhen:
"record.status == 'closed'"`, row closed; `update(t1, { amount: 555 })`
stays 100, `update(t1, { status: 'open', amount: 999 })` stores `{
status: 'closed', amount: 999 }`. Judging the conditional strip over the
post-static view would close both roots at once, but it moves reason
attribution for fields declaring both locks and the strict message text,
so it is a separate decision.
- #4889's landing rule still decides the FK's OWN `parent`-scoped lock:
`invoice: { readonlyWhen: "parent.status == 'paid'" }` judges a move off
a paid invoice against the invoice it moves to. Documented behaviour,
unchanged here; noted, not filed.
## Declaration note for the contract review
The claim declares no widening; copied above as given. Measured movement
in `update()`'s refuse/accept answer, only when a payload names an FK
that a strip then takes back out and the object has a `parent`-scoped
`requiredWhen`: a write refused `VALIDATION_FAILED` against the named
header now commits (the row stays under a header that does not require
the field), and a write that cleared a required field under a header
that does require it is now refused. Every other movement is a strip
(the write still commits) or a strict refusal that was already a
refusal. Whether that requirement movement counts as an accept-set
change is the review's call.
**Seat's disposition (after the contract review):** `Clause-②: no`
stands, and `patch` is right. The accept-direction movement removes a
refusal the published text already negated.
`content/docs/data-modeling/fields.mdx` ("Locking a detail from its
master: `parent`") says: "`parent` = the record on the other end of this
object's master_detail field", and "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)". A row whose repoint never
lands points at, and lands on, its stored master, so refusing it against
the named header contradicted that text. The refuse-direction movement
is a restored guarantee (a requirement bypass now refused). Neither
moves an authorable key, an export or an error code.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01TEhopqrWQYBycZzyJHpAZr)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
1 parent e9eb224 commit 3bd221d
3 files changed
Lines changed: 450 additions & 11 deletions
File tree
- .changeset
- packages/objectql/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
402 | 402 | | |
403 | 403 | | |
404 | 404 | | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
| 534 | + | |
| 535 | + | |
| 536 | + | |
| 537 | + | |
| 538 | + | |
| 539 | + | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
| 583 | + | |
| 584 | + | |
| 585 | + | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
| 593 | + | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
| 602 | + | |
| 603 | + | |
| 604 | + | |
| 605 | + | |
| 606 | + | |
| 607 | + | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
| 611 | + | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
| 621 | + | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
| 636 | + | |
| 637 | + | |
| 638 | + | |
| 639 | + | |
| 640 | + | |
0 commit comments