diff --git a/.changeset/temporal-equality-date-valued-binding.md b/.changeset/temporal-equality-date-valued-binding.md new file mode 100644 index 0000000000..904e9ddab4 --- /dev/null +++ b/.changeset/temporal-equality-date-valued-binding.md @@ -0,0 +1,57 @@ +--- +"@objectstack/formula": patch +--- + +fix(formula): `==` / `!=` between a date STRING field and a Date-valued binding no longer answers a silent `false` (#7168) + +A mixed-provenance comparison — the shape a hook or validation predicate writes +every day — returned the wrong boolean with no fault and no log line: + +```text +record.due == previous.due + with { record: { due: "2026-06-20" }, previous: { due: Date(2026-06-20T00:00:00Z) } } + -> { ok: true, value: false } // same field, same instant +``` + +`previous` arrives from the driver hydrated as a `Date`; `record` arrives from a +JSON payload as a `"YYYY-MM-DD"` string. cel-js compares a `string` against a +`google.protobuf.Timestamp` and never matches, so the predicate answered `false` +— and `!=` on the same pair answered `true`. Nothing errored, so nothing pointed +at it. This is the failure class that hurts most in an AI-authored filter: the +wrong answer is shaped exactly like a legitimate one. + +`rewriteTemporalEquality` already fixed this for a temporal **call** counterpart +(`record.due == today()`, #3183) by coercing the string operand with `date(...)`. +It now covers a Date-valued **binding** counterpart as well. A binding's runtime +type is not visible in the AST, so this arm is decided per row against the values +in the evaluation scope, and its verdict is deliberately never cached against the +expression source. + +**Comparisons that change answer** — one operand an ISO-8601 date/date-time +string, the other a binding holding a `Date`: + +- `record.due == previous.due` (same instant) — was `false`, now `true` +- `record.due != previous.due` (same instant) — was `true`, now `false` +- either operand order, and a `"…T14:33:00Z"` string against the same instant + +**Comparisons that deliberately do NOT change** — the coercion requires the +counterpart to be a real `Date` *and* this operand to be an ISO-8601 string that +parses, so everything below answers exactly as it did before: + +- two strings — `"2026-06-20" == "2026-06-20"` stays STRING equality +- two `Date`s — already compared as instants +- a different calendar day — stays `false` +- a non-date string against a `Date` (`"hello"`) — stays `false` +- a **numeric** string against a `Date` (`"5"`) — stays `false`. Load-bearing: + `new Date("5")` and `new Date("05")` both parse to 2001-05-01, so coercing here + would invent an equality between two different strings +- a date-ONLY string against a `Date` carrying wall-clock time — stays `false`. + `date()` parses, it does not truncate to a calendar day, and those are + genuinely different instants; truncating both sides would turn a correct + `false` into a wrong `true` for real datetime comparisons +- ordering (`<` / `>=`) is untouched — that path is ADR-0032 §1c's retry +- a string LITERAL counterpart is untouched — it is not a binding + +Cross-type `in` membership (`record.n in [1, 7]` with `n: "7"`) is a separate +clean-path question and is unchanged, deferred by maintainer ruling on #7168 +pending a measured victim. diff --git a/content/docs/data-modeling/formulas.mdx b/content/docs/data-modeling/formulas.mdx index 102341885a..4d526d1190 100644 --- a/content/docs/data-modeling/formulas.mdx +++ b/content/docs/data-modeling/formulas.mdx @@ -452,15 +452,23 @@ easiest way to ship a formula whose sign is inverted on every row. -**`dateField == today()` now matches (#3183).** A `date` field reads back as a -`YYYY-MM-DD` string, and CEL treats a string and a timestamp as unequal — so the -natural "is it due today" predicate used to silently return `false`. The engine -now rewrites temporal `==` / `!=` comparisons (coercing the field operand with -`date(...)`), so `record.due_date == today()` matches on the calendar day. This -applies to formulas, defaults, validation rules, and hook/flow conditions. -Ordering (`< > <= >=`) and string equality (`record.d == "2026-06-20"`) were -always fine. This is the read-side counterpart to the date-arithmetic build -error above — equality is rewritten and works; `+`/`-` arithmetic is rejected. +**`dateField == today()` now matches (#3183, #7168).** A `date` field reads back +as a `YYYY-MM-DD` string, and CEL treats a string and a timestamp as unequal — so +the natural "is it due today" predicate used to silently return `false`. The +engine now rewrites temporal `==` / `!=` comparisons (coercing the field operand +with `date(...)`), so `record.due_date == today()` matches on the calendar day. +The counterpart may be a temporal **call** (`today()`, `now()`, `daysFromNow()`, +`daysAgo()`) or a **binding that holds a `Date`** — which covers the +mixed-provenance comparison `record.due == previous.due`, where `previous` +arrives from the driver as a `Date` while `record` arrives from a JSON payload as +a `YYYY-MM-DD` string (#7168). This applies to formulas, defaults, validation +rules, and hook/flow conditions. Ordering (`< > <= >=`) and string equality +(`record.d == "2026-06-20"`) were always fine. The coercion is deliberately +narrow — it requires one side to be a real `Date` and the other an ISO-8601 +date/date-time string — so a non-date string compared against a `Date` still +answers `false` instead of being coerced into a match. This is the read-side +counterpart to the date-arithmetic build error above — equality is rewritten and +works; `+`/`-` arithmetic is rejected. ### Financial Calculations diff --git a/packages/formula/src/cel-engine.test.ts b/packages/formula/src/cel-engine.test.ts index 63db5586df..f74b990df6 100644 --- a/packages/formula/src/cel-engine.test.ts +++ b/packages/formula/src/cel-engine.test.ts @@ -557,6 +557,79 @@ describe('celEngine', () => { expect(rewriteTemporalEquality('$'.repeat(5000))).toBe('$'.repeat(5000)); expect(rewriteTemporalEquality('now('.repeat(2000))).toBe('now('.repeat(2000)); }); + + // #7168 — the Date-valued BINDING arm. The counterpart is not a temporal + // call but a binding that HOLDS a Date at evaluation time, which the AST + // cannot see — so this arm only fires when a scope is supplied. + describe('Date-valued binding counterpart (#7168)', () => { + const day = new Date('2026-06-20T00:00:00Z'); + const mixed = { record: { due: '2026-06-20' }, previous: { due: day } }; + + it('wraps the ISO-string operand when its counterpart is a Date-valued binding', () => { + expect(rewriteTemporalEquality('record.due == previous.due', mixed)) + .toBe('date(record.due) == previous.due'); + expect(rewriteTemporalEquality('previous.due == record.due', mixed)) + .toBe('previous.due == date(record.due)'); + expect(rewriteTemporalEquality('record.due != previous.due', mixed)) + .toBe('date(record.due) != previous.due'); + }); + + it('never fires without a scope — the runtime type of a binding is not in the AST', () => { + expect(rewriteTemporalEquality('record.due == previous.due')) + .toBe('record.due == previous.due'); + }); + + it('is idempotent — an already-coerced operand is not re-wrapped', () => { + expect(rewriteTemporalEquality('date(record.due) == previous.due', mixed)) + .toBe('date(record.due) == previous.due'); + }); + + it('composes with the temporal-call arm on one expression', () => { + expect(rewriteTemporalEquality('record.due == today() && record.due == previous.due', mixed)) + .toBe('date(record.due) == today() && date(record.due) == previous.due'); + }); + + // ── The fence: nothing else may start being coerced ────────────────── + it('leaves every comparison that is NOT string-vs-Date untouched', () => { + // two ISO strings — the author's string equality, still string equality + expect(rewriteTemporalEquality('record.due == previous.due', + { record: { due: '2026-06-20' }, previous: { due: '2026-06-20' } })) + .toBe('record.due == previous.due'); + // two Dates — already compares as instants, nothing to coerce + expect(rewriteTemporalEquality('record.due == previous.due', + { record: { due: day }, previous: { due: day } })) + .toBe('record.due == previous.due'); + // a non-date string opposite a Date — a genuine mismatch, not a + // serialization artifact. `date("hello")` is an Invalid Date. + expect(rewriteTemporalEquality('record.a == previous.b', + { record: { a: 'hello' }, previous: { b: day } })) + .toBe('record.a == previous.b'); + // a NUMERIC string opposite a Date. Load-bearing: `new Date("5")` and + // `new Date("05")` both parse to 2001-05-01, so coercing here would + // invent an equality between two different strings. + expect(rewriteTemporalEquality('record.a == previous.b', + { record: { a: '5' }, previous: { b: day } })) + .toBe('record.a == previous.b'); + // a number opposite a Date — not a string, nothing to coerce + expect(rewriteTemporalEquality('record.a == previous.b', + { record: { a: 5 }, previous: { b: day } })) + .toBe('record.a == previous.b'); + // null / absent operands + expect(rewriteTemporalEquality('record.a == previous.b', + { record: { a: null }, previous: { b: day } })) + .toBe('record.a == previous.b'); + expect(rewriteTemporalEquality('record.a == previous.b', { previous: { b: day } })) + .toBe('record.a == previous.b'); + // ORDERING against a Date binding is untouched — this arm is `==`/`!=` + // only; `<`/`>` against a string-serialized field is ADR-0032 §1c's + // retry path (#7098), which fires on a fault and is not clean-path. + expect(rewriteTemporalEquality('record.due >= previous.due', mixed)) + .toBe('record.due >= previous.due'); + // a string LITERAL counterpart is not a binding + expect(rewriteTemporalEquality('record.due == "2026-06-20"', mixed)) + .toBe('record.due == "2026-06-20"'); + }); + }); }); // #3183 — the end-to-end runtime behavior the rewrite delivers: a `Field.date` @@ -598,6 +671,84 @@ describe('celEngine', () => { }); }); + // #7168 — the end-to-end behavior of the Date-valued-binding arm. The + // reachable shape is a MIXED-PROVENANCE comparison: `previous` hydrated by the + // driver as a `Date`, `record` parsed from a JSON payload as a `YYYY-MM-DD` + // string. Same logical field, same instant, and the predicate answered a + // silent `false` — `{ ok: true, value: false }`, no fault, no log line. + describe('date-string == Date-valued binding runtime fix (#7168)', () => { + const now = new Date('2026-06-20T08:00:00Z'); + const midnight = new Date('2026-06-20T00:00:00Z'); + const row = (due: unknown, prev: unknown) => ({ now, record: { due }, previous: { due: prev } }); + + it('a same-day mixed-provenance comparison answers true instead of a silent false', () => { + expect(celEngine.evaluate(cel('record.due == previous.due'), row('2026-06-20', midnight))) + .toEqual({ ok: true, value: true }); + // operand order does not matter + expect(celEngine.evaluate(cel('previous.due == record.due'), row('2026-06-20', midnight))) + .toEqual({ ok: true, value: true }); + // `!=` was the same defect inverted — it answered a silent `true` + expect(celEngine.evaluate(cel('record.due != previous.due'), row('2026-06-20', midnight))) + .toEqual({ ok: true, value: false }); + }); + + it('a datetime string matches the same INSTANT (date() parses, it does not truncate)', () => { + const afternoon = new Date('2026-06-20T14:33:00Z'); + expect(celEngine.evaluate(cel('record.due == previous.due'), row('2026-06-20T14:33:00Z', afternoon))) + .toEqual({ ok: true, value: true }); + }); + + // ── The fence: what SHOULD stay false still does ────────────────────── + it('a different calendar day stays false', () => { + expect(celEngine.evaluate(cel('record.due == previous.due'), row('2026-06-19', midnight))) + .toEqual({ ok: true, value: false }); + expect(celEngine.evaluate(cel('record.due != previous.due'), row('2026-06-19', midnight))) + .toEqual({ ok: true, value: true }); + }); + + it('a date-ONLY string against a Date carrying wall-clock time stays false', () => { + // Deliberately not "fixed": these are genuinely different instants, and + // truncating both sides to a calendar day would flip a correct `false` + // into a wrong `true` for real datetime comparisons. + expect(celEngine.evaluate(cel('record.due == previous.due'), + row('2026-06-20', new Date('2026-06-20T14:33:00Z')))) + .toEqual({ ok: true, value: false }); + }); + + it('non-date and numeric strings against a Date stay false — no invented equality', () => { + expect(celEngine.evaluate(cel('record.due == previous.due'), row('hello', midnight))) + .toEqual({ ok: true, value: false }); + expect(celEngine.evaluate(cel('record.due == previous.due'), row('5', midnight))) + .toEqual({ ok: true, value: false }); + expect(celEngine.evaluate(cel('record.due == previous.due'), row(null, midnight))) + .toEqual({ ok: true, value: false }); + }); + + it('a string-vs-string comparison keeps STRING equality, including the lenient-parse pairs', () => { + // "5" and "05" are different strings; both parse to 2001-05-01 as dates. + // Neither side is a Date binding, so no coercion happens and they differ. + expect(celEngine.evaluate(cel('record.due == previous.due'), row('5', '05'))) + .toEqual({ ok: true, value: false }); + expect(celEngine.evaluate(cel('record.due == previous.due'), row('2026-06-20', '2026-06-20'))) + .toEqual({ ok: true, value: true }); + }); + + it('the verdict is per ROW, not cached against the source', () => { + // The binding arm depends on values, not on the expression — so the same + // source must be re-decided for every scope. If its result were memoized + // under the source (as the temporal-CALL arm safely is), row 2 would + // evaluate `date(record.due) == previous.due`, comparing a Timestamp + // against a string, and answer a wrong `false`. + const src = cel('record.due == previous.due'); + expect(celEngine.evaluate(src, row('2026-06-20', midnight))).toEqual({ ok: true, value: true }); + expect(celEngine.evaluate(src, row('2026-06-20', '2026-06-20'))).toEqual({ ok: true, value: true }); + // …and in the other order, to catch poisoning either way. + const src2 = cel('previous.due == record.due'); + expect(celEngine.evaluate(src2, row('2026-06-20', '2026-06-20'))).toEqual({ ok: true, value: true }); + expect(celEngine.evaluate(src2, row('2026-06-20', midnight))).toEqual({ ok: true, value: true }); + }); + }); + // #3306 — the blessed null-guard idiom `cond ? : null`. cel-js's ternary // unifier rejects a concrete branch against `null`; the engine's AST rewrite // wraps the non-null branch in `dyn(...)` so it compiles AND evaluates, and the diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 8f9d52fa23..5170cfdc43 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -855,8 +855,12 @@ function wrapInDate(node: CelNode): CelNode { * actually happened — the ~99% case that needs no rewrite evaluates the original * source untouched. Memoized per source string; a parse fault returns the source * unchanged (compile()/evaluate() report it). + * + * This arm is a pure function of the SOURCE, which is why it is memoized under + * the source alone. The Date-valued-BINDING arm (#7168) cannot be — see + * {@link rewriteTemporalEquality}. */ -export function rewriteTemporalEquality(source: string): string { +function rewriteTemporalCallEquality(source: string): string { if (typeof source !== 'string' || !source.trim()) return source; const cached = temporalRewriteCache.get(source); if (cached !== undefined) return cached; @@ -905,6 +909,165 @@ function rememberRewrite(source: string, rewritten: string): void { temporalRewriteCache.set(source, rewritten); } +/** + * A `==`/`!=` occurrence whose verdict depends on the SCOPE and not on the + * source: BOTH operands name a scope path, so which one (if either) holds a + * Date at evaluation time is unknowable from the AST. See + * {@link bindingEqualityCandidates}. + */ +type BindingEqualityCandidate = { readonly a: readonly string[]; readonly b: readonly string[] }; + +/** + * Every ` ==/!= ` occurrence in `source`, as the two scope paths. + * + * This is the part of the #7168 analysis that IS a pure function of the source, + * so it is parsed once per source and memoized — which is what keeps the + * scope-aware arm off the hot path. A source with no such occurrence (a literal + * comparison, an arithmetic operand, no equality at all) can never need the + * binding rewrite, and that verdict is final for every row. + */ +function bindingEqualityCandidates(source: string): readonly BindingEqualityCandidate[] { + const cached = bindingEqCandidateCache.get(source); + if (cached !== undefined) return cached; + const out: BindingEqualityCandidate[] = []; + // Cheap gate first: no equality operator, no candidate — and no parse. + if (source.includes('==') || source.includes('!=')) { + try { + const ast = (recordScopeEnv ??= buildScopedEnv([])).parse(source).ast; + const visit = (node: unknown): void => { + if (!isCelNode(node)) return; + if ((node.op === '==' || node.op === '!=') && Array.isArray(node.args) && node.args.length === 2) { + const a = scopePath(node.args[0]); + const b = scopePath(node.args[1]); + if (a && b) out.push({ a, b }); + } + if (Array.isArray(node.args)) for (const child of node.args) visit(child); + }; + visit(ast); + } catch { + // A parse fault is compile()/evaluate()'s to report; no candidates here. + out.length = 0; + } + } + rememberCandidates(source, out); + return out; +} + +/** Bounded memo of source → its {@link BindingEqualityCandidate}s (#7168). */ +const bindingEqCandidateCache = new Map(); +function rememberCandidates(source: string, candidates: readonly BindingEqualityCandidate[]): void { + if (bindingEqCandidateCache.size >= TEMPORAL_REWRITE_CACHE_MAX) { + const first = bindingEqCandidateCache.keys().next().value; + if (first !== undefined) bindingEqCandidateCache.delete(first); + } + bindingEqCandidateCache.set(source, candidates); +} + +/** + * True when this occurrence is the #7168 shape **in this scope**: one side holds + * a `Date` and the other an ISO-8601 temporal string. Both conditions are read + * off the values in hand — never off a static type, which under + * `unlistedVariablesAreDyn` says nothing. + */ +function isDateBindingPair(c: BindingEqualityCandidate, scope: Record): boolean { + const va = resolveScopePath(scope, c.a); + const vb = resolveScopePath(scope, c.b); + return (vb instanceof Date && coercionFor(va, 'temporal') === 'date') + || (va instanceof Date && coercionFor(vb, 'temporal') === 'date'); +} + +/** + * #7168 — wrap the ISO-string operand of a ` ==/!= ` whose + * counterpart is a **Date-valued binding**, so the comparison is Timestamp vs + * Timestamp instead of string vs Timestamp. + * + * Same three-condition discipline as {@link rewriteFaultedOperands}, and the + * middle one is what keeps the fix from becoming the mirror-image defect: + * 1. the operand is a scope path — never a literal, a call, or an arith + * sub-tree, so its value is readable before deciding; + * 2. the counterpart is a `Date` **in this scope** — not a temporal call (the + * {@link rewriteTemporalCallEquality} arm owns that), not a `date(...)` + * call, not a date-shaped string; + * 3. this operand's own value is an ISO-8601 temporal string that parses + * ({@link coercionFor}). + * + * Condition 3 is load-bearing and deliberately strict. `date()` is `toDate` — + * `new Date(String(v))` — so wrapping unconditionally would coerce operands that + * are not dates at all, and JS date parsing is lenient enough to invent + * equalities: `"5"` and `"05"` are different strings but BOTH parse to + * 2001-05-01, turning a correct `false` into a silent `true`. Requiring an + * ISO-8601 string leaves every non-date comparison exactly as it answers today. + * + * Deliberately NOT covered: a date-ONLY string (`"2026-06-20"`) against a Date + * carrying wall-clock time (`2026-06-20T14:33:00Z`) stays `false`. `date()` + * parses, it does not truncate to a calendar day, and those two operands are + * genuinely different instants. Truncating both sides to a day would make + * `record.dt == previous.dt` a day-granularity comparison — the same + * silent-wrong-answer defect pointing the other way. + * + * Returns the rewritten source, or null when no operand qualifies. + */ +function rewriteDateBindingEquality(source: string, scope: Record): string | null { + let ast: unknown; + try { + ast = (recordScopeEnv ??= buildScopedEnv([])).parse(source).ast; + } catch { + return null; + } + let changed = false; + const visit = (node: unknown): void => { + if (!isCelNode(node)) return; + if ((node.op === '==' || node.op === '!=') && Array.isArray(node.args) && node.args.length === 2) { + const args = node.args as unknown[]; + for (const side of [0, 1] as const) { + const path = scopePath(args[side]); + if (!path) continue; + const counterpartPath = scopePath(args[1 - side]); + if (!counterpartPath) continue; + if (!(resolveScopePath(scope, counterpartPath) instanceof Date)) continue; + if (coercionFor(resolveScopePath(scope, path), 'temporal') !== 'date') continue; + args[side] = wrapInCall('date', args[side] as CelNode); + changed = true; + } + } + if (Array.isArray(node.args)) for (const child of node.args) visit(child); + }; + visit(ast); + return changed ? serialize(ast as Parameters[0]) : null; +} + +/** + * The temporal-equality rewrite: coerce a date-string field operand so `==`/`!=` + * against a Timestamp compares instants instead of silently never matching. + * + * Two arms, layered so they compose on one expression: + * - **temporal CALL** counterpart (#3183) — `record.due == today()`. A pure + * function of the source, memoized per source string; + * - **Date-valued BINDING** counterpart (#7168) — `record.due == previous.due` + * where `previous.due` arrived from the driver as a `Date` and `record.due` + * from a JSON payload as `"2026-06-20"`. Same logical field, same instant, + * and the comparison answered a silent `false` — `{ ok: true, value: false }`, + * no fault, no log line. Runs only when a `scope` is supplied. + * + * Why the second arm needs the scope, and why it is NOT memoized under the + * source: a binding's runtime type is not in the AST. `record.due` and + * `previous.due` are the same shape whether either holds a `Date`, a string, or + * null, and under `unlistedVariablesAreDyn` the static type says nothing either. + * The verdict is therefore a property of the ROW, not of the expression — so + * caching it under the source key would apply row 1's verdict to row 2. What IS + * cached per source is the *analysis* ({@link bindingEqualityCandidates}): the + * occurrences whose verdict could depend on a scope at all. Sources without one + * — the overwhelming majority — take the memoized static path and never reparse. + */ +export function rewriteTemporalEquality(source: string, scope?: Record): string { + const out = rewriteTemporalCallEquality(source); + if (!scope) return out; + const candidates = bindingEqualityCandidates(out); + if (candidates.length === 0) return out; + if (!candidates.some((c) => isDateBindingPair(c, scope))) return out; + return rewriteDateBindingEquality(out, scope) ?? out; +} + /** True when `node` is the CEL `null` literal (`{ op: 'value', args: null }`). */ function isNullLiteral(node: unknown): boolean { return isCelNode(node) && node.op === 'value' && node.args === null; @@ -1450,10 +1613,14 @@ export const celEngine: DialectEngine = { // temporal function (`date(record.d) == today()`), so a `Field.date` string // matches the Timestamp instead of silently never equalling it. No-op (and // no reserialize) for any source without such a comparison. + // #7168 — `scope` extends that to a Date-valued BINDING counterpart + // (`record.due == previous.due`, `previous` hydrated by the driver): the + // runtime type of a binding is not in the AST, so this arm is decided per + // 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. Both rewrites are no-ops for sources that don't need them. - const evalSource = rewriteNullableTernary(rewriteTemporalEquality(source)); + // ternary unifier. All rewrites are no-ops for sources that don't need them. + const evalSource = rewriteNullableTernary(rewriteTemporalEquality(source, scope)); try { const raw = env.evaluate(evalSource, scope); return { ok: true, value: coerce(raw) as T };