Skip to content

Commit cf4ea8c

Browse files
committed
fix(objectql): hook 条件对完整 record 求值 —— stored ⊕ payload,与 #4649 同源 (#4770)
A declarative hook `condition` was evaluated against `ctx.input.data` — the fields the current write happens to carry. `ctx.previous` sat behind it, unreachable, and the two were never merged, so a condition could only reference a field the update touched. Anything else aborted the CEL expression with `No such key`, which the gate swallowed into `false` plus one WARN line. For a guard-style hook that reads as "let it through"; for an audit-style hook it reads as "do not record it". `showcase_audit_task_completion` (`record.done == true`) therefore did NOT run on the most ordinary updates there are — change the status, change the assignee — and the only trace was ten warn lines per showcase boot. The record a condition reads is now built exactly the way a validation predicate's is (#1871 / #4649): stored ⊕ payload, made total over the object's DECLARED fields, `null` when a declared key is in neither. `materializeDeclaredFields` moves out of `validation/rule-validator.ts` into `declared-fields.ts` and is shared by both paths, because `record.done == true` must not mean two different things depending on which surface evaluates it. Declared-only is the load-bearing half: a typo'd or undeclared key stays unevaluable and is still reported. Materialisation runs only when the persisted state is in hand — an insert, or an update whose prior row was fetched — so a predicate bulk update keeps its payload rather than gaining nulls that contradict N stored rows. `ctx.ql.getObject()` is an in-memory registry read; no code path fetches a record it did not already load. Out of scope, deliberately unchanged: what happens when a condition is STILL unevaluable after merging (warn + treat as false). Its failure direction is opposite for guard and audit hooks and is tracked separately. `has(...)` is not a null guard once the record is total — the showcase's over-budget condition is rewritten with `!= null`, and the hook docs that prescribed `has(record.x)` are corrected with it. Evidence: `pnpm dev -- --fresh` on this branch logs 0 `condition evaluation failed` lines; the same boot with the fix reverted logs exactly the 10 the issue reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ
1 parent 9f601e8 commit cf4ea8c

8 files changed

Lines changed: 600 additions & 51 deletions

File tree

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
---
2+
'@objectstack/objectql': minor
3+
---
4+
5+
**A declarative hook `condition` is now evaluated against the RECORD — the stored row overlaid with this write's payload — not against the update payload alone (#4770).**
6+
7+
⚠️ **Behaviour change — read this before upgrading.** The condition gate used to evaluate
8+
against `ctx.input.data`: only the fields the current write happened to carry.
9+
`ctx.previous` sat behind it, unreachable, and the two were never merged. So a condition
10+
could reference only a field the update *happened* to touch; referencing anything else
11+
aborted the CEL expression with `No such key` — which the gate swallowed into `false`,
12+
leaving one WARN line as the sole trace.
13+
14+
For a guard-style hook that reads as "let it through"; for an audit-style hook it reads as
15+
"do not record it". `condition: "record.done == true"` on an audit hook therefore did NOT
16+
run on the most ordinary updates there are — change the status, change the assignee —
17+
because `done` was not in the payload.
18+
19+
The record a condition reads is now built the same way a validation predicate's is
20+
(#1871 / #4649, via one shared helper so the two cannot drift):
21+
22+
- **stored ⊕ payload** — the prior record overlaid with this write's data, so a condition
23+
may reference any field of the record, not just the changed ones. The payload still
24+
wins for the fields it carries.
25+
- **total over the object's DECLARED fields**`null` for a declared field present in
26+
neither, so a driver that stores only the columns it wrote no longer decides whether an
27+
expression is evaluable.
28+
- **declared fields only** — an undeclared or typo'd key (`record.stauts`) stays
29+
unevaluable and is still reported, exactly as before.
30+
31+
Materialisation happens only when the persisted state is actually in hand — an insert, or
32+
an update whose prior row was fetched. A predicate (`multi: true`) bulk update carries no
33+
prior row, so its payload is left as it is rather than gaining `null`s that would
34+
contradict the stored rows. No code path fetches a record it did not already load.
35+
36+
**What you may see after upgrading**
37+
38+
- **Conditions that never fired start firing.** A hook gated on a field the payload rarely
39+
carried was silently skipped; it now evaluates. This is the declaration finally being
40+
honoured, but expect hooks to run on writes where they previously did not.
41+
- **A condition is now about the record's STATE, not about this write's diff.**
42+
`record.done == true` fires on every update of a task that *is* done, not only on the
43+
update that set it. A condition cannot express a transition today — the CEL scope binds
44+
`record` only.
45+
- **Conditions guarded with `has(...)` need `!= null`.** `has(x)` asks whether the key is
46+
**present**, and a declared field holding `null` is present — so
47+
`has(a) && has(b) && a > b` still faults on `null > null`. Same lesson as #4649:
48+
49+
```diff
50+
- condition: 'has(record.spent) && has(record.budget) && record.spent > record.budget'
51+
+ condition: 'record.spent != null && record.budget != null && record.spent > record.budget'
52+
```
53+
54+
`has()` remains correct for asking whether an **undeclared** key exists.
55+
56+
**Unchanged, deliberately:** what happens when a condition is *still* unevaluable after
57+
merging — it is logged at WARN and treated as `false`, as before. Whether that fallback
58+
should differ by hook category (a guard fails open, an audit fails silent) is a separate
59+
decision, tracked on its own issue.

examples/app-showcase/src/data/hooks/index.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,15 @@ export const WarnOverBudgetHook = {
6363
label: 'Warn On Over-Budget Project',
6464
object: 'showcase_project',
6565
events: ['afterUpdate'] as LifecycleEvent[],
66-
// Guard with has(): an afterUpdate fired by a partial write (e.g. the
67-
// task-rollup that only touches task_count) carries a record WITHOUT
68-
// spent/budget, and CEL throws "No such key" on a bare `record.spent`.
69-
// has() is the missing-key-safe macro — the hook simply skips those.
70-
condition: "has(record.spent) && has(record.budget) && record.spent > record.budget",
66+
// Guard with `!= null`, NOT with `has()` (#4770, same lesson as #4649). A
67+
// condition is evaluated against the STORED record overlaid with this
68+
// write's payload, made total over the object's declared fields — so a
69+
// partial write (the task-rollup that only touches task_count) still sees
70+
// spent/budget, and `has(record.spent)` is uniformly TRUE for a declared
71+
// field, including one holding null. Only `!= null` actually keeps
72+
// `null > null` — which CEL has no overload for — from aborting the
73+
// expression.
74+
condition: "record.spent != null && record.budget != null && record.spent > record.budget",
7175
body: {
7276
language: 'js' as const,
7377
source: "var r = ctx.result || ctx.input || {}; ctx.log.warn('project over budget: ' + (r.name || r.id || 'unknown') + ' (' + r.spent + ' / ' + r.budget + ')');",
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Make a record TOTAL over an object's DECLARED fields.
5+
*
6+
* Shared by the two places that evaluate a CEL expression against "the
7+
* record": object-level validation predicates
8+
* (`validation/rule-validator.ts`, #1871 / #4649) and declarative hook
9+
* `condition`s (`hook-wrappers.ts`, #4770). They used to disagree — a
10+
* predicate saw a total record while a hook condition saw only the fields the
11+
* current write happened to carry — which is precisely the drift this module
12+
* exists to prevent: an author cannot be expected to know that the same
13+
* `record.done == true` means two different things depending on which surface
14+
* reads it.
15+
*
16+
* CEL is strict about missing keys: `record.x` on a record that does not carry
17+
* the key `x` aborts the whole expression with `No such key`, which is NOT the
18+
* same as reading `null`. Whether a key is carried is a property of the DRIVER
19+
* (a driver that stores only written columns returns a record missing every
20+
* column the write never touched), not of the data — so without this an
21+
* expression's evaluability depends on storage internals the author cannot
22+
* see.
23+
*
24+
* Scope is deliberately the object's **declared fields only**. Materialising
25+
* every key an expression happens to name would paper over author typos: a
26+
* `record.stauts` must stay unevaluable so it is reported (fail-closed for
27+
* validation, #4649) rather than silently read as `null` and quietly answered
28+
* "no violation" / "condition false".
29+
*
30+
* `undefined` counts as absent (not just a missing key): CEL treats an own key
31+
* holding `undefined` exactly as it treats no key at all.
32+
*
33+
* ## Only ever call this when the record's persisted state is IN HAND
34+
*
35+
* On insert there is nothing to know — absence genuinely means "no value". On
36+
* update it is knowable only when the prior row was actually fetched. Without
37+
* it, defaulting a declared field to `null` would not be materialising an
38+
* absent value, it would be FABRICATING one that contradicts the stored row.
39+
* Callers decide; this function only applies the rule.
40+
*
41+
* ## Consequence worth knowing before writing an expression
42+
*
43+
* Because a declared field is always present afterwards, `has(record.<declared
44+
* field>)` is uniformly TRUE (a materialised `null` is a present key holding
45+
* null — CEL's own rule). `has()` therefore guards against an UNDECLARED key,
46+
* not against an empty value; test emptiness with `record.x != null`.
47+
*/
48+
export function materializeDeclaredFields<T extends Record<string, unknown>>(
49+
record: T,
50+
fields: Record<string, unknown> | undefined | null,
51+
): T {
52+
if (!fields || typeof fields !== 'object') return record;
53+
const target = record as Record<string, unknown>;
54+
for (const name of Object.keys(fields)) {
55+
if (target[name] === undefined) target[name] = null;
56+
}
57+
return record;
58+
}

0 commit comments

Comments
 (0)