Skip to content

Commit 50185a8

Browse files
os-zhuangclaude
andauthored
fix(objectql): fail closed on unevaluable validation predicates, and make the merged record total (#4649) (#4761)
A script/cross_field/conditional validation whose CEL predicate could not be evaluated was logged at WARN and SKIPPED, so the write went through. The rule stayed declared, listed in the metadata, and enforced nothing — on exactly the records whose shape triggered the fault. For a validation that inverts the guarantee: the rule exists to reject a write. Two halves, neither sufficient alone: 1. The record a predicate reads is TOTAL over the object's declared fields on UPDATE as well as insert — `null` when the key is in neither the payload nor the prior record — and the `previous` binding is materialised the same way. Without this, step 2 would 422 every legitimate predicate on any driver that stores only written columns (the shape hotcrm#630 reported). Materialisation covers DECLARED fields only, so a typo'd key stays unevaluable and reportable. 2. A predicate that still faults REJECTS the write, naming the rule and the offending key. `severity` still governs blocking, so an advisory rule stays advisory. A `conditional` now counts as needing the prior record whenever it declares a `when`: that predicate is evaluated against the merged record, so without the prior state it read a PATCH as though it were the whole record. Fail-closed evaluation immediately found two of our own example rules that had never enforced anything: `has(x)` is TRUE for a declared column holding NULL, so `has(a) && has(b) && a < b` faults on `null < null` on any driver that returns its NULL columns. Both are rewritten with `!= null` guards, and the rejection message now teaches that distinction. Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny Co-authored-by: Claude <noreply@anthropic.com>
1 parent 891d345 commit 50185a8

6 files changed

Lines changed: 712 additions & 41 deletions

File tree

.changeset/tender-donkeys-smoke.md

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
---
2+
'@objectstack/objectql': minor
3+
---
4+
5+
**Validation rules now fail CLOSED when their predicate cannot be evaluated, and the record a predicate reads is total over the object's declared fields (#4649).**
6+
7+
⚠️ **Behaviour change — read this before upgrading.** A `script` / `cross_field` /
8+
`conditional` validation whose CEL predicate faulted used to be logged at WARN and
9+
**skipped**, so the write went through. The rule stayed declared, appeared in the
10+
metadata and in any "what protects this object" listing, and enforced nothing. Two
11+
changes close that, and they are load-bearing together:
12+
13+
1. **The merged record is total on UPDATE, not just on INSERT.** Every field the object
14+
declares is present when the predicate runs — `null` when it is in neither the payload
15+
nor the prior record. Previously `previous` was whatever the driver returned, so on a
16+
driver that stores only written columns a predicate referencing a declared column
17+
aborted with `No such key` and the rule was skipped. The `previous` CEL binding is
18+
materialised the same way. Insert and update now behave identically.
19+
2. **A predicate that still cannot be evaluated rejects the write** with
20+
`VALIDATION_FAILED`, naming the rule and — when the fault is a missing key — the key
21+
the predicate read and how to fix it. A validation exists to reject a write; "the rule
22+
could not be checked" must never resolve to "allowed".
23+
24+
`severity` still governs blocking: an unevaluable `warning` / `info` rule is logged and
25+
does not throw.
26+
27+
**What you may see after upgrading**
28+
29+
- **Rules that were never running start running.** A rule skipped because of a missing key
30+
now evaluates and can reject writes it previously let through. This is not a regression —
31+
it is the declaration finally being enforced — but on an existing deployment it can
32+
surface as new `400 VALIDATION_FAILED` responses on writes that used to succeed. Review
33+
each such rule: it is doing what its author wrote.
34+
- **Predicates guarded with `has(...)` may now reject.** `has(x)` asks whether the key is
35+
**present**, and a declared field holding `null` is present — so
36+
`has(a) && has(b) && a < b` still faults on `null < null`. Such a rule never enforced
37+
anything on rows with a null value (on any driver that returns its NULL columns); the
38+
fault used to be swallowed and is now reported. **Guard with `!= null`, not `has(...)`:**
39+
40+
```diff
41+
- condition: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date'
42+
+ condition: 'record.start_date != null && record.end_date != null && record.end_date < record.start_date'
43+
```
44+
45+
The rejection message says this explicitly, and `error.fields[0].constraint` carries
46+
`{ reason: 'unevaluable', missingKey?, hint?: 'null-comparison' }` for machine handling.
47+
`has()` remains correct for asking whether an **undeclared** key exists.
48+
- **A `conditional` rule now always fetches the prior record on update.** Its `when` is
49+
evaluated against the merged record, so without the prior state it read a PATCH as if it
50+
were the whole record. One extra `findOne` per update on objects that declare one.
51+
52+
**Unchanged, deliberately:** a broken `regex` (`format`), an uncompilable JSON Schema
53+
(`json_schema`), the field-level `requiredWhen` / `readonlyWhen` / option `visibleWhen`
54+
predicates, and a rule that throws all keep their existing fail-open policy.

examples/app-crm/src/objects/opportunity.object.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,11 @@ export const Opportunity = ObjectSchema.create({
109109
label: 'Close Date Must Be Future',
110110
description: 'Prevent back-dating the close_date of an OPEN opportunity. Closed (won/lost) deals legitimately carry a historical close date, so they are exempt.',
111111
fields: ['close_date'],
112-
condition: P`has(record.close_date) && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`,
112+
// `!= null`, not `has(...)` (#4649): `has(x)` is TRUE for a declared
113+
// column holding NULL, so the old guard let `null < now()` fault and the
114+
// rule silently did nothing on every opportunity created without a close
115+
// date.
116+
condition: P`record.close_date != null && record.close_date < now() && record.stage != "closed_won" && record.stage != "closed_lost"`,
113117
message: 'Close Date must be today or a future date.',
114118
events: ['insert'],
115119
},

examples/app-showcase/src/data/objects/project.object.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,12 @@ export const Project = ObjectSchema.create({
106106
label: 'End After Start',
107107
description: 'Target end date must be on or after the start date.',
108108
fields: ['start_date', 'end_date'],
109-
condition: P`has(record.start_date) && has(record.end_date) && record.end_date < record.start_date`,
109+
// Guarded with `!= null`, NOT `has(...)` (#4649). `has(x)` asks whether
110+
// the key is PRESENT — a declared column holding NULL is present, so
111+
// `has(a) && has(b) && a < b` still faults on `null < null` and the rule
112+
// enforced nothing on any project missing a date. It read as a guard and
113+
// was not one.
114+
condition: P`record.start_date != null && record.end_date != null && record.end_date < record.start_date`,
110115
message: 'Target End Date must be on or after the Start Date.',
111116
},
112117
{

0 commit comments

Comments
 (0)