Skip to content

Commit 8e69fef

Browse files
committed
feat(lint,docs): reject un-guarded nullable comparisons in CEL predicates — has(x) is not a null guard (#4763)
CEL's `has(x)` asks whether the KEY is present. Since #4649 every predicate reads a record that is total over the object's declared fields, so a declared column holding NULL is still present and `has(record.end_date)` is uniformly true. The idiom that reads like a guard — has(record.start_date) && has(record.end_date) && record.end_date < record.start_date — therefore reaches `null < null`, CEL has no overload, and the whole predicate aborts. Before #4761 the abort was swallowed, so a rule of this shape enforced NOTHING on exactly the rows it was written to catch. The fault is fully decidable from the metadata alone, so per PD #12 it belongs at authoring, not at a 400 on production data. New gate (error, no warn-mode escape hatch), in `packages/lint`: - `validate-null-guards.ts` — the decision procedure. Parses the predicate with cel-js and rejects an ordering (`< <= > >=`) or arithmetic (`+ - * / %`, unary `-`) operator applied to an operand that resolves to a declared NULLABLE field (no `required: true`, no `defaultValue`, no default option, not autonumber) and is not dominated by an explicit `!= null` / `== null` / `!isBlank()` test in the same boolean branch. `has()` deliberately does not count. Guard propagation follows `&&` left-to-right, `||` short-circuit, `!` polarity and ternary branches. - Wired into `validateStackExpressions`, already a `gating` authoring rule on `os build` / `os validate` / `os lint` AND the runtime publish gate. - Scope: object validation rules (including predicates nested in a `conditional` rule's `then`/`otherwise`) and lifecycle hook `condition`s — the surfaces CEL actually evaluates over a total record. Sharing rules (compiled to a SQL filter, three-valued, never faults), flattened flow conditions (a bare id may be a flow variable) and `Field.formula` (its own #3306 handling) are deliberately out, not half-covered. - Message names the rule, the operand and the fix, and closes with the sentence lifted verbatim from `unevaluableRuleError` in `rule-validator.ts`, so the publish-time and runtime rejections read identically. `has()` over an UNDECLARED key is untouched — that is its legitimate use. All three example apps pass the new gate unchanged; the pre-#4786 showcase hook shape is pinned as a regression test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ
1 parent f160ba4 commit 8e69fef

10 files changed

Lines changed: 943 additions & 1 deletion

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/lint": minor
3+
---
4+
5+
feat(lint): `has(x)` 不是 null 守卫 —— 发布期直接拒绝未守卫的可空比较 (#4763)
6+
7+
CEL 的 `has(x)` 问的是**键是否存在**。自 #4649 起,谓词读到的记录对对象声明的每个
8+
字段都是**全量**的:一个声明了却存 `NULL` 的列同样"存在",所以
9+
`has(record.end_date)` 对声明字段恒为 `true`,什么也没告诉作者。于是这个读起来
10+
像守卫的写法根本不是守卫:
11+
12+
```text
13+
has(record.start_date) && has(record.end_date) && record.end_date < record.start_date
14+
```
15+
16+
它会走到 `null < null`,CEL 没有对应重载,整个谓词中断。#4761 之前中断被吞掉
17+
(规则跳过,一条 WARN),也就是说**这一形状的规则在任何含 null 值的行上从未生效
18+
**——它写在元数据里、读起来完全正确、却什么都没有强制执行。#4761 把运行时改成
19+
fail-closed 之后,当场就在我们自己的两个示例对象里抓到了它。
20+
21+
运行时拒绝是兜底,不是该学到这件事的地方:作者会在真实数据(很可能是生产数据)
22+
上收到一个 400,离写下规则可能已经过去几个月。而这个错误**仅凭元数据就可判定**
23+
——谓词的 AST 加上对象声明的字段类型,就足以判断某个操作数是否可能为 null。按
24+
AGENTS.md PD #12(在创作期拒绝,不要在消费端容忍),它属于发布闸门。
25+
26+
**新增闸门(error,直接拒绝,没有降级开关)。** `os build` / `os validate` /
27+
`os lint` 与运行时发布闸门共用的 `validateStackExpressions` 现在会拒绝这样的谓词:
28+
**声明为可空**的字段(没有 `required: true`、没有 `defaultValue`、没有默认选项、
29+
不是 autonumber)应用**排序**(`< <= > >=`)或**算术**(`+ - * / %`,含一元 `-`)
30+
运算符,而该操作数没有被同一布尔分支内支配它的 `!= null` / `== null` / `!isBlank()`
31+
显式判空所守卫。`has(x)` **刻意不**计入守卫——这正是本规则存在的理由。错误信息点名
32+
规则、操作数与修法,收尾句逐字取自 `rule-validator.ts``unevaluableRuleError`,
33+
两道闸门措辞完全一致。
34+
35+
覆盖面(有意划定,而不是含糊地覆盖一半):对象**校验规则**(含 `conditional` 规则
36+
`then` / `otherwise` 里嵌套的谓词)与**生命周期 hook 的 `condition`** ——即真正由 CEL
37+
在全量记录上求值、会 fail-closed 的两类面。共享规则条件(下推成 SQL 过滤,`NULL > x`
38+
是三值逻辑,不会 fault)、flow 的扁平作用域条件(裸标识符可能是 flow 变量)与
39+
`Field.formula`(有自己的 #3306 `guard ? value : null` 处理)不在此列。
40+
41+
**未声明**键的 `has()` 完全不受影响——那才是它的正当用途:区分"这次 PATCH 里
42+
根本没提到这个键"与"显式写了 null"。示例应用无需改动即通过新闸门。

content/docs/data-modeling/validation.mdx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ export const Order = ObjectSchema.create({
5959
Condition expressions are **CEL** (evaluated by `@objectstack/formula`), not Salesforce-style formulas. Reference the incoming record via `record.<field>`, use `==`/`!=`, `&&`/`||`, and helpers like `isBlank(x)` and `has(record.field)`. A string condition is accepted as authoring shorthand and normalized to `{ dialect: 'cel', source }` at build time.
6060
</Callout>
6161

62+
<Callout type="warn">
63+
**`has(x)` is not a null guard.** Predicates see a record that is *total* over the object's declared fields, so `has(record.end_date)` is true even when the value is `NULL``has(a) && has(b) && a < b` then reaches `null < null`, CEL has no overload, and the whole rule aborts (the write is rejected fail-closed). Write `record.start_date != null && record.end_date != null && record.end_date < record.start_date` instead. Since #4763 the `has()` form is **rejected at build/publish**: an ordering or arithmetic operator over a declared nullable field needs a real `!= null` guard. `has()` over an *undeclared* key — "was this in the PATCH at all?" — is untouched.
64+
</Callout>
65+
6266
## Common Properties
6367

6468
All validation types share these base properties:

packages/lint/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
"typecheck": "tsc --noEmit"
2626
},
2727
"dependencies": {
28+
"@marcbachmann/cel-js": "^8.0.0",
2829
"@objectstack/formula": "workspace:*",
2930
"@objectstack/sdui-parser": "workspace:*",
3031
"@objectstack/spec": "workspace:*",

packages/lint/src/index.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,16 @@ export type { WidgetBindingFinding, WidgetBindingSeverity } from './validate-wid
2727
export { validateStackExpressions } from './validate-expressions.js';
2828
export type { ExprIssue } from './validate-expressions.js';
2929

30+
// #4763 — `has(x)` reads as a null guard and is not one. The decision procedure
31+
// is exported on its own so other authoring surfaces (cloud graph-lint, the AI
32+
// authoring path) reuse ONE verdict instead of re-deriving it.
33+
export {
34+
findUnguardedNullableOperands,
35+
nullGuardMessage,
36+
NULL_GUARD_HINT,
37+
} from './validate-null-guards.js';
38+
export type { NullGuardFinding, NullGuardOptions } from './validate-null-guards.js';
39+
3040
export { validateListViewMode, LIST_VIEW_FILTERS_IN_VIEWS_MODE } from './validate-list-view-mode.js';
3141

3242
// [ADR-0078] The functional-completeness gate. All judgement lives in the shared

packages/lint/src/validate-expressions.test.ts

Lines changed: 207 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,13 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
341341
close_date: { type: 'date' },
342342
expected: { type: 'formula', formula: 'record.amount * record.probability / 100' },
343343
},
344-
validations: [{ name: 'future', expression: 'record.close_date >= today()' }],
344+
// The `!= null` guard is load-bearing since #4763: `close_date` is a
345+
// declared NULLABLE field, and an un-guarded `>=` over it faults at
346+
// runtime (`null >= timestamp` has no overload) — the null-guard gate
347+
// rejects that shape at authoring now. Soundness (this block's
348+
// subject) and null-guarding are separate verdicts; the predicate has
349+
// to satisfy both to produce zero issues.
350+
validations: [{ name: 'future', expression: 'record.close_date != null && record.close_date >= today()' }],
345351
}],
346352
});
347353
expect(issues).toHaveLength(0);
@@ -758,3 +764,203 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
758764
});
759765
});
760766
});
767+
768+
// ───────────────────────────────────────────────────────────────────────
769+
// #4763 — `has(x)` reads as a null guard and is not one.
770+
//
771+
// Scope note (constraint of the issue, pinned here so it stays a decision):
772+
// this gate walks the AUTHORED METADATA the stack carries — object validation
773+
// rules and lifecycle-hook conditions. It never reads source files, so the
774+
// deliberately-bad fixtures in `packages/objectql/src/validation/rule-*.test.ts`
775+
// (which pin the runtime's fail-closed behaviour and MUST keep the bad shape)
776+
// are structurally out of its reach.
777+
// ───────────────────────────────────────────────────────────────────────
778+
describe('null-guard gate (#4763)', () => {
779+
// Mirrors `showcase_project`: dates and money are declared but nullable;
780+
// `status` carries a default option and `name` is required, so neither can
781+
// be null and neither may ever be flagged.
782+
const project = {
783+
name: 'showcase_project',
784+
fields: {
785+
name: { type: 'text', required: true },
786+
status: { type: 'select', options: [{ value: 'planned', default: true }, { value: 'active' }] },
787+
start_date: { type: 'date' },
788+
end_date: { type: 'date' },
789+
budget: { type: 'currency' },
790+
spent: { type: 'currency', defaultValue: 0 },
791+
},
792+
};
793+
const withRule = (rule: Record<string, unknown>) =>
794+
validateStackExpressions({ objects: [{ ...project, validations: [rule] }] });
795+
796+
it('REJECTS the `has(a) && has(b) && a < b` shape over nullable declared fields', () => {
797+
const issues = withRule({
798+
type: 'script',
799+
name: 'end_after_start',
800+
condition: 'has(record.start_date) && has(record.end_date) && record.end_date < record.start_date',
801+
});
802+
expect(issues.length).toBeGreaterThan(0);
803+
expect(issues.every((i) => (i.severity ?? 'error') === 'error')).toBe(true);
804+
const joined = issues.map((i) => i.message).join('\n');
805+
// names the rule …
806+
expect(joined).toContain("validation rule 'end_after_start'");
807+
// … the operand …
808+
expect(joined).toContain('record.end_date');
809+
expect(joined).toContain('record.start_date');
810+
// … and the fix, in the runtime's own words.
811+
expect(joined).toContain("Guard it with '!= null'");
812+
expect(joined).toContain('has(x)');
813+
expect(issues[0].where).toContain("object 'showcase_project'");
814+
});
815+
816+
it('ACCEPTS the `!= null` form (the fix #4761 landed in the examples)', () => {
817+
expect(
818+
withRule({
819+
type: 'script',
820+
name: 'end_after_start',
821+
condition:
822+
'record.start_date != null && record.end_date != null && record.end_date < record.start_date',
823+
}),
824+
).toHaveLength(0);
825+
});
826+
827+
it('ACCEPTS a guarded arithmetic predicate (showcase `spent_within_budget`)', () => {
828+
expect(
829+
withRule({
830+
type: 'script',
831+
name: 'spent_within_budget',
832+
condition: 'record.budget != null && record.spent != null && record.spent > record.budget * 1.2',
833+
}),
834+
).toHaveLength(0);
835+
});
836+
837+
it('never flags a required field or one with a default (`spent`, `status`, `name`)', () => {
838+
expect(
839+
withRule({ type: 'script', name: 'spend_positive', condition: 'record.spent > 0' }),
840+
).toHaveLength(0);
841+
});
842+
843+
it('reaches the predicates nested in a `conditional` rule’s then/otherwise', () => {
844+
const issues = withRule({
845+
type: 'conditional',
846+
name: 'budget_sanity',
847+
when: "record.status == 'active'",
848+
then: { type: 'script', name: 'over_budget', condition: 'has(record.budget) && record.budget > 1' },
849+
});
850+
expect(issues.length).toBe(1);
851+
expect(issues[0].message).toContain('record.budget');
852+
expect(issues[0].where).toContain("'budget_sanity' then → 'over_budget'");
853+
});
854+
855+
// Negative-case pin: the real `showcase_account` rule pair. Both use `has()`
856+
// — legitimately, to tell "key absent from the PATCH" apart from "explicit
857+
// null" — and both compare with EQUALITY only. They must stay legal; a rule
858+
// that flags them is too broad.
859+
it('leaves `showcase_account.churn_reason_consistency` alone (legitimate `has()`)', () => {
860+
const issues = validateStackExpressions({
861+
objects: [{
862+
name: 'showcase_account',
863+
fields: { status: { type: 'select', options: [{ value: 'churned' }] }, churn_reason: { type: 'text' } },
864+
validations: [{
865+
type: 'conditional',
866+
name: 'churn_reason_consistency',
867+
when: "record.status == 'churned'",
868+
then: {
869+
type: 'script',
870+
name: 'churn_reason_present',
871+
condition: "!has(record.churn_reason) || record.churn_reason == null || record.churn_reason == ''",
872+
},
873+
otherwise: {
874+
type: 'script',
875+
name: 'churn_reason_absent',
876+
condition: "has(record.churn_reason) && record.churn_reason != null && record.churn_reason != ''",
877+
},
878+
}],
879+
}],
880+
});
881+
expect(issues).toHaveLength(0);
882+
});
883+
884+
describe('hook conditions — the third instance the issue named', () => {
885+
const hookStack = (condition: string) => ({
886+
objects: [project],
887+
hooks: [{ name: 'project_budget_alert', object: 'showcase_project', condition }],
888+
});
889+
890+
// Regression pin. `examples/app-showcase/src/data/hooks/index.ts` carried
891+
// `has(record.spent) && has(record.budget) && record.spent > record.budget`
892+
// until #4770/#4786 corrected it. This asserts the bad shape cannot come
893+
// back: it is red today, and would have been red before that fix.
894+
it('REJECTS the pre-#4786 showcase hook shape', () => {
895+
const issues = validateStackExpressions(
896+
hookStack('has(record.spent) && has(record.budget) && record.spent > record.budget'),
897+
);
898+
expect(issues.length).toBeGreaterThan(0);
899+
expect(issues[0].where).toContain("hook 'project_budget_alert'");
900+
expect(issues.map((i) => i.message).join('\n')).toContain('record.budget');
901+
});
902+
903+
it('ACCEPTS the corrected shape now on `main`', () => {
904+
expect(
905+
validateStackExpressions(
906+
hookStack('record.spent != null && record.budget != null && record.spent > record.budget'),
907+
),
908+
).toHaveLength(0);
909+
});
910+
911+
it('applies per target for a multi-object hook', () => {
912+
const issues = validateStackExpressions({
913+
objects: [project, { name: 'other_obj', fields: { budget: { type: 'currency', required: true } } }],
914+
hooks: [{ name: 'multi', object: ['showcase_project', 'other_obj'], condition: 'record.budget > 1' }],
915+
});
916+
// Only the object that declares `budget` nullable is flagged.
917+
expect(issues).toHaveLength(1);
918+
expect(issues[0].where).toContain('showcase_project');
919+
});
920+
});
921+
922+
describe('surfaces deliberately NOT covered', () => {
923+
it('leaves sharing-rule conditions alone (compiled to a SQL filter, never faults)', () => {
924+
expect(
925+
validateStackExpressions({
926+
objects: [project],
927+
sharingRules: [{
928+
name: 'big_budget',
929+
object: 'showcase_project',
930+
condition: "record.status == 'active' && record.budget > 100000",
931+
}],
932+
}),
933+
).toHaveLength(0);
934+
});
935+
936+
it('leaves flattened flow conditions alone (a bare id may be a flow variable)', () => {
937+
expect(
938+
validateStackExpressions({
939+
objects: [project],
940+
flows: [{
941+
name: 'escalate',
942+
nodes: [
943+
{ id: 'start', type: 'start', config: { objectName: 'showcase_project' } },
944+
{ id: 'd', type: 'decision', config: { condition: 'record.budget > 100000' } },
945+
],
946+
edges: [],
947+
}],
948+
}),
949+
).toHaveLength(0);
950+
});
951+
952+
it('leaves `Field.formula` expressions alone (blessed `guard ? value : null`, #3306)', () => {
953+
expect(
954+
validateStackExpressions({
955+
objects: [{
956+
...project,
957+
fields: {
958+
...project.fields,
959+
remaining: { type: 'formula', formula: 'record.budget - record.spent' },
960+
},
961+
}],
962+
}),
963+
).toHaveLength(0);
964+
});
965+
});
966+
});

0 commit comments

Comments
 (0)