Skip to content

Commit 326f5de

Browse files
fix(objectql): a TRUE readonlyWhen no longer strips hook-derived values — the conditional strip judges API-boundary callers only (#9107) (#9321)
* fix(objectql): readonlyWhen strips caller-supplied values only (#9107) The conditional `readonlyWhen` strip ran after the before-phase hooks and was keyed on `name in data` over the POST-hook payload, so a value a `beforeUpdate` hook derived was judged exactly like a key the caller forged — and, unlike the static `readonly` strip beside it, it carried no `isSystem` exemption either. A field locked by a TRUE predicate therefore had no server-side write path at all: the derived-field pattern and a conditional form lock could not coexist on one field, and the failure was silent behind an HTTP 200. Give the conditional strip the exact key discipline #5591 gave the static one: judge a key only while it is still an own property of the engine-entry caller snapshot AND still holds that caller's value by `Object.is`. Both branches, by-id and multi-row, off the same snapshot. The API-boundary lock is unchanged. To reach the exempt side of either test a value must differ from what arrived at engine entry, which only server code can arrange: a client that echoes the locked key back is stripped as before, and if a hook overwrites it the value that persists is the hook's. `isSystem` is still NOT an exemption here — a state lock any system-context write could bypass would not be a state lock. Docs and the strict-mode refusal wording now say "every API-boundary caller" and name the hook as the sanctioned derived-field write path. * test(objectql): pass the required packageId so the new pin adds no TEST_DEBT; add changeset (#9107) --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent df04324 commit 326f5de

10 files changed

Lines changed: 672 additions & 15 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@objectstack/objectql": minor
3+
---
4+
5+
fix(objectql): a TRUE `readonlyWhen` no longer strips hook-derived values — the conditional strip judges only API-boundary callers (#9107)
6+
7+
`stripReadonlyWhenFields` runs AFTER the before-phase hooks and was keyed on
8+
`name in data` over the POST-hook payload, so a value a `beforeUpdate` hook
9+
computed was judged exactly like a key the caller forged. Unlike the static
10+
`readonly` strip immediately beside it, it carried no `isSystem` exemption
11+
either — so a field locked by a TRUE predicate had **no server-side write path
12+
at all**: a hook derived it and the strip deleted it; a cron or plugin wrote it
13+
with `{ context: { isSystem: true } }` and the strip deleted that too.
14+
15+
Net effect before this change: the derived-field pattern (hook-computed column)
16+
and a conditional form lock could not coexist on one field. An author wanting
17+
"visible on the form but locked" **and** "recomputed by a hook" had no
18+
spec-compliant spelling, and the failure was silent behind an HTTP 200.
19+
20+
Measured downstream (steedos-labs/os-project-titanwind-ehr#1446):
21+
`equipment.next_maintenance_date` is hook-derived (last maintenance date + cycle
22+
days) and declared with an always-true `readonlyWhen` to render
23+
visible-but-locked. After a maintenance sign-off the recompute never landed, and
24+
a scheduler keyed on that date regenerated the same maintenance plan on every
25+
scan — a user-visible duplicate-plans loop, diagnosed only by reading the
26+
engine's strip order in the dist bundle.
27+
28+
The conditional strip now carries the exact key discipline #5591 gave the static
29+
one, on **both** branches (by-id and multi-row) off one engine-entry snapshot: a
30+
key is judged only while it is still an own property of the caller's payload as
31+
it arrived at engine entry AND still holds that caller's value by `Object.is`. A
32+
key a hook added, or overwrote, is a server value and survives.
33+
34+
**The API-boundary lock is unchanged, and a caller cannot launder a write
35+
through the hook phase.** To reach the exempt side of either test a value must
36+
differ from what arrived at engine entry — which only server code can arrange. A
37+
client that echoes the locked key back is stripped exactly as before; if a hook
38+
overwrites that key, what persists is the **hook's** value, never the client's.
39+
`isSystem` is still deliberately NOT an exemption for `readonlyWhen`: a state
40+
lock that any system-context write could bypass would not be a state lock (the
41+
frozen paid-invoice-lines case depends on it).
42+
43+
What moves for callers:
44+
45+
- A `beforeUpdate` hook may now write a field locked by a TRUE `readonlyWhen`.
46+
This is the sanctioned channel for a conditionally-locked derived field.
47+
- `onFieldsDropped` no longer reports such a key under `readonly_when` — it is
48+
written, not dropped, so reporting it would make the observability seam lie.
49+
- `strictReadonlyWrites` no longer refuses a write whose only `readonlyWhen`
50+
"drop" was a hook's own value; a caller-supplied locked field is still refused.
51+
- The `ERR_READONLY_FIELD_REJECTED` refusal message's `readonlyWhen` remedy
52+
clause now reads "every **API-boundary** caller, isSystem included" and names
53+
the hook path. The error `code` is unchanged; a pin on the exact message text
54+
moves with it.
55+
56+
If an app relied on the strip discarding a hook's own write to a locked field,
57+
that write now lands — remove the hook assignment, or narrow the predicate.

‎content/docs/data-modeling/fields.mdx‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -375,6 +375,42 @@ protocol 17 (#3855): authoring it is rejected with an error naming the
375375
replacement, not silently stripped, and `os migrate meta --from 16` rewrites
376376
existing sources automatically.
377377

378+
#### Who the lock applies to — and the derived-field write path
379+
380+
A TRUE `readonlyWhen` predicate locks the field for **every API-boundary
381+
caller**. Unlike static `readonly`, passing `{ context: { isSystem: true } }`
382+
does **not** exempt it: a state lock any system-context write could bypass would
383+
not be a state lock, and the frozen-paid-invoice case depends on it.
384+
385+
What the lock does **not** cover is the server's own trusted code. A value a
386+
`beforeUpdate` hook derives — or writes over a key the caller also sent — is a
387+
server value, not a caller write, and is never stripped. That is what makes
388+
"visible on the form but locked, and recomputed by a hook" a spelling the
389+
platform supports on one field:
390+
391+
```typescript
392+
// The field renders locked, and only the hook may move it.
393+
next_maintenance_date: Field.date({
394+
label: 'Next Maintenance',
395+
readonlyWhen: P`true`,
396+
}),
397+
```
398+
399+
```typescript
400+
// The hook is the write path.
401+
beforeUpdate: (ctx) => {
402+
if ('period_days' in ctx.input.data) {
403+
ctx.input.data.next_maintenance_date = addDays(today, ctx.input.data.period_days);
404+
}
405+
},
406+
```
407+
408+
A client cannot use this to launder a write. The engine snapshots the caller's
409+
payload **at entry**, before any middleware or hook runs, and judges a key only
410+
while it still holds that value — so echoing the locked key back never exempts
411+
it, and if a hook overwrites it, the value that persists is the hook's. Either
412+
way the caller's value does not land.
413+
378414
#### Locking a detail from its master: `parent`
379415

380416
On a **detail** object — one that declares a `master_detail` relationship — a

‎content/docs/kernel/contracts/data-engine.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ The strips these two options cover are the engine's legal ones:
306306
| Strip | `reason` | Verbs | Writers it skips |
307307
|:---|:---|:---|:---|
308308
| Static `readonly: true` (#2948) | `readonly` | `update` | `isSystem` |
309-
| A TRUE `readonlyWhen` predicate (#3042) | `readonly_when` | `update` | none — every caller, `isSystem` included |
309+
| A TRUE `readonlyWhen` predicate (#3042) | `readonly_when` | `update` | none at the API boundary — every caller, `isSystem` included; a value a `beforeUpdate` hook derived or overwrote is not a caller write and is never stripped (#9107) |
310310
| Implicitly-readonly runtime-owned type (#5503 — `RUNTIME_OWNED_FIELD_TYPES`, today `autonumber`) | `readonly` | `insert` **and** `update` | `isSystem`, `preserveAudit` (#3493) |
311311
| Primary-key strip of a payload `id` the update dispatch already ruled is not an identifier (#6437) | `primary_key` | `update` | none |
312312

‎packages/objectql/src/engine-dropped-fields-primary-key.test.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -291,10 +291,18 @@ describe('#6437 — the refusal message is composed from `drops`, not from the c
291291
expect(err.message).toContain('SCALAR id');
292292
});
293293

294-
it('the READ-ONLY-only message is byte-identical to the pre-#6437 text', async () => {
295-
// The compatibility half. Every caller and pin written against #5126's /
296-
// #5503's wording must be untouched by a change that only teaches the
297-
// error about a class those payloads do not carry.
294+
it('the READ-ONLY-only message is unmoved by a NEW REASON CLASS', async () => {
295+
// The compatibility half, and read it for what it actually asserts: adding
296+
// a `reason` (#6437's whole change) must not disturb the wording payloads
297+
// that carry no such class see. That property still holds.
298+
//
299+
// [#9107] What it never claimed is that the sentence is frozen for all
300+
// time. The remedy clause moved ONCE, deliberately, when the maintainer
301+
// ruled the conditional strip judges only API-boundary callers: the old
302+
// text told a server author its hook-derived write was locked out, which
303+
// stopped being true. A remedy sentence that has gone false is the one
304+
// thing a refusal message may not keep — so the pin moves WITH the ruling,
305+
// and stays a byte-exact pin so the next unintended drift is still caught.
298306
const { err } = await refuse({ id: 'rec_1', settled_total: 99 }, {});
299307
expect(err.message).toBe(
300308
`Update on 'task' was REFUSED: 1 caller-supplied field(s) ` +
@@ -304,7 +312,9 @@ describe('#6437 — the refusal message is composed from `drops`, not from the c
304312
`server-side code that legitimately writes read-only columns, pass ` +
305313
`{ context: { isSystem: true } } (this exempts statically 'readonly' fields, but NOT ` +
306314
`fields locked by a TRUE 'readonlyWhen' predicate — those stay locked for every ` +
307-
`caller). To let the strip happen and merely observe it, drop ` +
315+
`API-boundary caller, isSystem included. A value DERIVED by a beforeUpdate hook is ` +
316+
`not a caller write and is never stripped — that is the sanctioned write path for a ` +
317+
`conditionally-locked derived field). To let the strip happen and merely observe it, drop ` +
308318
`strictReadonlyWrites and pass options.onFieldsDropped instead (#3407).`,
309319
);
310320
});

0 commit comments

Comments
 (0)