diff --git a/.changeset/25267-cel-has-indexed-receiver.md b/.changeset/25267-cel-has-indexed-receiver.md new file mode 100644 index 00000000000..726aa5fa32c --- /dev/null +++ b/.changeset/25267-cel-has-indexed-receiver.md @@ -0,0 +1,15 @@ +--- +'@objectstack/formula': minor +--- + +fix(formula): `has()` over a selection whose receiver is an index or a call result evaluates as a presence test, as the CEL spec defines it + +Clause-②: yes (widening) +Metadata-protocol: added + +The CEL spec defines `has(e.f)` for any expression `e`. The engine used to refuse every such argument whose selection chain was not rooted in a plain name — `has(data.environments[0].storage.usedMb)`, `has(record.items[0].name)`, `has(dyn(x).y)` — with `has() invalid argument`, at evaluation and at `compile` / `validateExpression` alike. A page or view `visibleWhen`, whose publish gate checks syntax but not types, published such a predicate and then failed open in the console. + +- **What now works.** The call is a presence test on the last field: `true` when the receiver has the key, `false` when it does not, and a fault (`No such key`) when a step before the last one is absent, exactly like `has(record.a.b)`. `evaluate`, `compile`, `validateExpression`, `firstUndeclaredReference` and `inferCelType` all give that one answer. +- **What widens.** Object, flow, action, sharing-rule and hook predicates that `validateExpression` used to refuse for this argument now pass. They evaluate as written. +- **What is still refused.** An argument the spec itself refuses: a bare name (`has(status)`) or an index as the last step (`has(record.items[0])`). Test a key with `'key' in m`, or a list position with `size(list) > n`. +- **Nothing to migrate.** A predicate already written in the `'key' in e` form keeps working and answers the same. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 021ae582c7b..f4f10ad31d5 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -404,6 +404,9 @@ let recordScopeEnv: Environment | undefined; * faults `has() invalid argument`. `@objectstack/lint`'s * `validate-visibility-predicates` masks `has(…)` spans at its own call site * (#16118) and that mask stays load-bearing; no other consumer has one. + * A select whose receiver is an index or a call (`has(a[0].b)`) is NOT in + * this class: the spec admits it, and {@link rewriteIndexedHas} lowers it + * before this check runs (#25267). * * ⇒ A `null` from this helper is still "nothing was reported", never "the * source is clean". ⛔ Do not close the remaining classes by widening the regex @@ -423,7 +426,9 @@ export function firstUndeclaredReference( const env = knownFields.length === 0 ? (recordScopeEnv ??= buildScopedEnv([])) : buildScopedEnv(knownFields); - const result = env.parse(source).check?.() as + // #25267 — judged in the form the engine checks, so an indexed `has()` + // receiver is not a first error that masks every reference behind it. + const result = env.parse(rewriteIndexedHas(source)).check?.() as | { valid: boolean; error?: { message?: string } } | undefined; if (result && result.valid === false) { @@ -862,7 +867,8 @@ export function inferCelType(source: string, knownFields: readonly string[] = [] const env = knownFields.length === 0 ? (recordScopeEnv ??= buildScopedEnv([])) : buildScopedEnv(knownFields); - const result = env.parse(source).check?.() as + // #25267 — typed in the form the engine checks (an indexed `has()` is `bool`). + const result = env.parse(rewriteIndexedHas(source)).check?.() as | { valid?: boolean; type?: unknown } | undefined; if (!result || result.valid === false) return null; @@ -1066,7 +1072,7 @@ export function firstTypeMismatch( // unifier *before* the check reaches an inner date-arith overload; rewrite it // first (same pass the runtime uses) so the real fault surfaces (#3306). const env = buildTypedEnv(fieldCelTypes, scope); - const result = env.parse(rewriteNullableTernary(source)).check?.() as + const result = env.parse(rewriteNullableTernary(rewriteIndexedHas(source))).check?.() as | { valid?: boolean; error?: { message?: string } } | undefined; if (!result || result.valid !== false) return null; @@ -1463,6 +1469,111 @@ function rememberNullableRewrite(source: string, rewritten: string): void { nullableTernaryCache.set(source, rewritten); } +/** + * #25267 — make `has()` over a selection whose receiver is not a plain name + * (`has(data.environments[0].storage.usedMb)`, `has(lookup(x).y)`) type-check + * and evaluate, as the CEL spec defines it: `has(e.f)` is a presence test on + * field `f` of whatever `e` evaluates to, and `e` may be any expression. + * + * cel-js 8.0.0's `has()` macro walks the selection chain back to its root and + * refuses the argument (`has() invalid argument`) unless that root is an + * identifier — so an index or a call anywhere in the chain faulted every time + * it was checked or run, while a page's `visibleWhen` (whose publish gate + * parses but does not type-check) published clean and failed open. + * + * The call is lowered to `e.?f.hasValue()`. That is the CEL spec's own + * definition of optional field selection (`e.?f` is `optional.of(e.f)` when + * `has(e.f)`, else `optional.none()`), so the two are the same test, and cel-js + * evaluates the receiver `e` exactly as `has()` does: an absent key on the way + * to the last selection still faults (`No such key`), only the last one answers + * `false`. `cel-has-indexed-receiver.test.ts` pins the two forms against each + * other over every value kind. + * + * The rewrite is: + * - **narrow** — it fires ONLY on a `has()` whose argument is a `.` selection + * rooted in something other than an identifier. `has(record.a.b)` is left to + * cel-js, and so is every argument the spec itself refuses: a bare name + * (`has(status)`) or an index as the LAST step (`has(a[0])`) still faults + * `has() invalid argument`; + * - **applied where cel-js type-checks** — `evaluate`, `compile` and the + * checker-backed helpers ({@link firstUndeclaredReference}, + * {@link inferCelType}, the overload-fault probe), which are the only places + * the macro's refusal can arise. The parse-only front end + * ({@link parseCelToAst}) keeps the authored `has(…)` call, which parses + * already, so an AST consumer that keys on a `has()` call still finds it; + * - **idempotent** — the lowered form carries no `has()` call to lower again. + * + * Returns the (possibly rewritten) source; only reserializes when a rewrite + * actually happened. Memoized; a parse fault returns the source unchanged. + */ +export function rewriteIndexedHas(source: string): string { + if (typeof source !== 'string' || !source.trim()) return source; + const cached = indexedHasCache.get(source); + if (cached !== undefined) return cached; + // Cheap gate: a rewrite needs a `has(` call. + if (!HAS_CALL_RE.test(source)) { + rememberIndexedHasRewrite(source, source); + return source; + } + let ast: unknown; + try { + ast = (recordScopeEnv ??= buildScopedEnv([])).parse(source).ast; + } catch { + rememberIndexedHasRewrite(source, source); + return source; + } + let changed = false; + // Children first, so a `has()` nested inside a lowered receiver is lowered too. + const lower = (value: unknown): unknown => { + if (Array.isArray(value)) { + for (let i = 0; i < value.length; i++) value[i] = lower(value[i]); + return value; + } + if (!isCelNode(value)) return value; + if (Array.isArray(value.args)) lower(value.args); + const selection = indexedHasSelection(value); + if (!selection) return value; + changed = true; + return { op: 'rcall', args: ['hasValue', { op: '.?', args: selection.args }, []] }; + }; + const lowered = lower(ast); + const out = changed ? serialize(lowered as Parameters[0]) : source; + rememberIndexedHasRewrite(source, out); + return out; +} + +/** A `has(` call, spaced or not, not the tail of a longer name or a member call. */ +const HAS_CALL_RE = /(^|[^\w$.])has\s*\(/; + +/** + * The argument of `node` when it is a `has()` call {@link rewriteIndexedHas} + * lowers — a `.` selection whose chain of `.` / `.?` steps ends in something + * other than an identifier — or `null`. + */ +function indexedHasSelection(node: CelNode): CelNode | null { + if (node.op !== 'call' || !Array.isArray(node.args) || node.args[0] !== 'has') return null; + const callArgs: unknown = node.args[1]; + if (!Array.isArray(callArgs) || callArgs.length !== 1) return null; + const selection: unknown = callArgs[0]; + if (!isCelNode(selection) || selection.op !== '.' || !Array.isArray(selection.args)) return null; + let root: unknown = selection.args[0]; + while (isCelNode(root) && (root.op === '.' || root.op === '.?') && Array.isArray(root.args)) { + root = root.args[0]; + } + return isCelNode(root) && root.op !== 'id' ? selection : null; +} + +/** Bounded memo of source → indexed-`has()`-lowered source (#25267). */ +const indexedHasCache = new Map(); +const INDEXED_HAS_CACHE_MAX = 500; +function rememberIndexedHasRewrite(source: string, rewritten: string): void { + if (indexedHasCache.size >= INDEXED_HAS_CACHE_MAX) { + const first = indexedHasCache.keys().next().value; + if (first !== undefined) indexedHasCache.delete(first); + } + indexedHasCache.set(source, rewritten); +} + /** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */ function coerce(value: unknown): unknown { if (typeof value === 'bigint') { @@ -1870,7 +1981,8 @@ export const celEngine: DialectEngine = { // #3306 — accept the null-guard idiom `cond ? : null`, which cel-js's // ternary unifier otherwise rejects (`int`/`double`/`string` ≠ `null`). Same // rewrite the runtime uses, so build and eval agree on what is valid. - const compiled = env.parse(rewriteNullableTernary(source)); + // #25267 — and `has()` over an indexed receiver, lowered the same way. + const compiled = env.parse(rewriteNullableTernary(rewriteIndexedHas(source))); // Surface check errors eagerly. cel-js's `check()` returns a // `TypeCheckResult` object (`{ valid, type?, error? }`) — NOT an array — // so the type fault (including `found no matching overload for 'PRIOR(dyn)'` @@ -1942,8 +2054,10 @@ export const celEngine: DialectEngine = { // row against the values in hand. Non-date operands are untouched. // #3306 — then relax the null-guard idiom `cond ? : null` so a // nullable numeric/string formula evaluates instead of faulting cel-js's - // ternary unifier. All rewrites are no-ops for sources that don't need them. - const evalSource = rewriteNullableTernary(rewriteTemporalEquality(source, scope)); + // ternary unifier. #25267 — and lower `has()` over an indexed receiver, + // which cel-js's macro refuses, to the spec-equivalent `e.?f.hasValue()`. + // All rewrites are no-ops for sources that don't need them. + const evalSource = rewriteNullableTernary(rewriteTemporalEquality(rewriteIndexedHas(source), scope)); try { const raw = runProgram(env, evalSource, scope); return { ok: true, value: coerce(raw) as T }; diff --git a/packages/formula/src/cel-has-indexed-receiver.test.ts b/packages/formula/src/cel-has-indexed-receiver.test.ts new file mode 100644 index 00000000000..bcc4b6e42da --- /dev/null +++ b/packages/formula/src/cel-has-indexed-receiver.test.ts @@ -0,0 +1,119 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, expect, it } from 'vitest'; +import { celEngine, firstUndeclaredReference, inferCelType, rewriteIndexedHas } from './cel-engine'; +import { validateExpression } from './validate'; + +/** + * #25267 — `has()` over a selection whose receiver is not a plain name. + * + * The CEL spec defines `has(e.f)` for any expression `e`; cel-js 8.0.0's macro + * refused every receiver chain not rooted in an identifier with `has() invalid + * argument`. Measured on `origin/main` 17f3c65a1d before the change: + * `has(data.environments[0].storage.usedMb)` answered `ok: false` from + * `evaluate`, `compile` and `validateExpression` alike, while a page's + * `visibleWhen` (parse-only gate) published it and the console failed open. + * + * The engine now lowers that call to the spec's equivalent `e.?f.hasValue()` + * ({@link rewriteIndexedHas}); these pins hold every door to one answer. + */ + +const ENVIRONMENTS = { environments: [{ storage: { usedMb: 5 } }, { storage: {} }] }; + +function evaluate(source: string, extra: Record) { + return celEngine.evaluate({ dialect: 'cel', source }, { extra }); +} + +describe('has() over an indexed receiver evaluates as a presence test (#25267)', () => { + it('the filed reading: present answers true, absent answers false', () => { + expect(evaluate('has(data.environments[0].storage.usedMb)', { data: ENVIRONMENTS })) + .toEqual({ ok: true, value: true }); + expect(evaluate('has(data.environments[1].storage.usedMb)', { data: ENVIRONMENTS })) + .toEqual({ ok: true, value: false }); + }); + + it('agrees with the `in` form the filing worked around with', () => { + for (const at of [0, 1]) { + const viaHas = evaluate(`has(data.environments[${at}].storage.usedMb)`, { data: ENVIRONMENTS }); + const viaIn = evaluate(`'usedMb' in data.environments[${at}].storage`, { data: ENVIRONMENTS }); + expect(viaHas).toEqual(viaIn); + } + }); + + it('answers exactly what the identifier-rooted form answers, over every value kind', () => { + // `has(v.…)` is cel-js's own macro (identifier root, untouched); `has(w[0].…)` + // is the lowered form over the same value. Faults compare by kind and the + // macro's first line, since the lowered source is echoed in the excerpt. + const kinds: Record = { + 'map with the key': { f: { g: 1 } }, + 'map with the key null': { f: null }, + 'map with the key false': { f: false }, + 'map without the key': { h: 1 }, + 'empty map': {}, + null: null, + list: [1], + int: 5, + string: 's', + bool: true, + }; + const shape = (r: ReturnType) => + r.ok ? { ok: true, value: r.value } : { ok: false, kind: r.error.kind, first: r.error.message.split('\n')[0] }; + for (const [kind, v] of Object.entries(kinds)) { + for (const path of ['f', 'f.g']) { + const native = shape(evaluate(`has(v.${path})`, { v, w: [v] })); + const lowered = shape(evaluate(`has(w[0].${path})`, { v, w: [v] })); + expect({ kind, path, ...lowered }).toEqual({ kind, path, ...native }); + } + } + }); + + it('a receiver that is a call result is a receiver too', () => { + expect(evaluate("has(dyn(m).a)", { m: { a: 1 } })).toEqual({ ok: true, value: true }); + expect(evaluate("has(dyn(m).b)", { m: { a: 1 } })).toEqual({ ok: true, value: false }); + }); +}); + +describe('every door gives the same verdict (#25267)', () => { + const lowered = 'has(record.items[0].name)'; + + it('compile and validateExpression accept what evaluate runs', () => { + expect(celEngine.compile(lowered).ok).toBe(true); + expect(validateExpression('predicate', { dialect: 'cel', source: lowered }).ok).toBe(true); + expect(celEngine.evaluate({ dialect: 'cel', source: lowered }, { record: { items: [{ name: 'x' }] } })) + .toEqual({ ok: true, value: true }); + }); + + it('the checker-backed helpers judge the lowered form', () => { + expect(inferCelType(lowered)).toBe('bool'); + // The `has()` fault no longer takes the checker's one error slot, so a bare + // reference behind it is reported instead of masked. + expect(firstUndeclaredReference(`${lowered} && status == 'x'`)).toBe('status'); + }); + + it('controls: an argument the spec refuses is still refused at every door', () => { + for (const source of ['has(status)', 'has(record.items[0])']) { + const compiled = celEngine.compile(source); + expect(compiled.ok).toBe(false); + expect(compiled.ok ? '' : compiled.error.message).toContain('has() invalid argument'); + expect(validateExpression('predicate', { dialect: 'cel', source }).ok).toBe(false); + const ran = celEngine.evaluate({ dialect: 'cel', source }, { record: { items: [{ name: 'x' }] } }); + expect(ran.ok).toBe(false); + } + }); +}); + +describe('rewriteIndexedHas (#25267)', () => { + it('lowers only a `has()` whose selection is rooted in something other than a name', () => { + expect(rewriteIndexedHas('has(a[0].b.c)')).toBe('a[0].b.?c.hasValue()'); + expect(rewriteIndexedHas('has(a[0].b) && has(c.d)')).toBe('a[0].?b.hasValue() && has(c.d)'); + for (const untouched of ['has(record.a.b)', 'has(status)', 'has(a[0])', "x.has('y')", 'record.has_items']) { + expect(rewriteIndexedHas(untouched)).toBe(untouched); + } + }); + + it('is idempotent, and lowers a `has()` nested in a lowered receiver', () => { + const once = rewriteIndexedHas('has(a[has(b[0].c) ? 0 : 1].d)'); + expect(once).not.toContain('has('); + expect(rewriteIndexedHas(once)).toBe(once); + }); +});