|
| 1 | +--- |
| 2 | +"@objectstack/formula": patch |
| 3 | +--- |
| 4 | + |
| 5 | +fix(formula): the ADR-0032 §1c retry rewrites only the operands that faulted (#7098) |
| 6 | + |
| 7 | +**A CEL expression could return a silently wrong boolean.** No fault, no log |
| 8 | +line, no failing test — `{ ok: true }` with the wrong answer. If you have |
| 9 | +compound CEL that mixes a numeric comparison with a string equality over |
| 10 | +string-serialized fields, read the "which expressions change answer" list below: |
| 11 | +those expressions answer differently after this fix, and the new answer is the |
| 12 | +right one. |
| 13 | + |
| 14 | +## What was wrong |
| 15 | + |
| 16 | +When a comparison faults on a string-serialized numeric or date field |
| 17 | +(`record.rating >= 4` where `rating` reads back as `"5.0"` — #1530 / #1534), |
| 18 | +ADR-0032 §1c hydrates and retries. The retry hydrated the **entire scope** and |
| 19 | +re-ran the **entire expression**, justified by a docblock claim that it |
| 20 | + |
| 21 | +> can never change a comparison that already evaluated cleanly — it only rescues |
| 22 | +> one that already faulted. |
| 23 | +
|
| 24 | +That claim was false, and it was load-bearing: it was the stated reason the |
| 25 | +hydration was allowed to be unconditional and scope-wide. The retry knows only |
| 26 | +that the *whole expression* faulted, not that each sub-comparison did. So: |
| 27 | + |
| 28 | +```text |
| 29 | +record.n >= 4 && record.s == "5.0" with { n: "7", s: "5.0" } |
| 30 | + before -> { ok: true, value: false } after -> { ok: true, value: true } |
| 31 | +``` |
| 32 | + |
| 33 | +`record.n >= 4` faults and is correctly rescued. But `record.s` was hydrated to |
| 34 | +the number `5` as well, so the author's deliberate string equality — **true** |
| 35 | +when it was evaluated the first time — became `5 == "5.0"`, which CEL answers |
| 36 | +`false` across types. The expression returned `false`, and nothing reported that |
| 37 | +a clean answer had been overruled. |
| 38 | + |
| 39 | +## Which expressions change answer |
| 40 | + |
| 41 | +Only expressions that **already reached the §1c retry** — i.e. some operand |
| 42 | +faulted `no such overload`. Everything that evaluates without faulting is |
| 43 | +untouched. Within that set, an expression changes answer when it also contains: |
| 44 | + |
| 45 | +- **a string equality / inequality on a numeric-looking or ISO-date field** — |
| 46 | + `record.n >= 4 && record.s == "5.0"`, and the `!=` and ternary forms. Now |
| 47 | + answers on the string the author wrote. |
| 48 | +- **a string membership test** — `record.s in ["5.0", "x"]`. |
| 49 | +- **the same field compared as a number in one place and as a string in |
| 50 | + another** — `record.n >= 4 && record.n == "7"`. Both answers are now correct |
| 51 | + at once; previously the second was collateral damage from the first. |
| 52 | +- **a numeric-looking string the expression RETURNS rather than compares** — |
| 53 | + `record.n >= 4 ? record.s : "none"` returned the number `5`; it now returns |
| 54 | + the string `"5.0"`. A `Field.formula` of type text was storing a different |
| 55 | + value than the record held. |
| 56 | + |
| 57 | +One class becomes a **loud fault where it used to be silently rescued**: an |
| 58 | +operand whose value the rewrite cannot read before deciding — bound by a |
| 59 | +comprehension (`record.items.exists(i, i.price > 100)`), or behind a computed |
| 60 | +index. That is the deliberate trade of this fix. Rescuing an operand we cannot |
| 61 | +prove faulted is exactly the defect being closed, so those report the original |
| 62 | +`no such overload` instead of guessing. The reported error is unchanged in shape |
| 63 | +and message. |
| 64 | + |
| 65 | +## What replaces it |
| 66 | + |
| 67 | +The coercion is now **per operand position** — the same discipline |
| 68 | +`rewriteTemporalEquality` already documents ("no field-wide trade-off"), one |
| 69 | +step stricter. The scope is never rewritten; the faulting operand is wrapped in |
| 70 | +`double(…)` or `date(…)` in place. An operand is rewritten only when all three |
| 71 | +hold, which makes the docblock's guarantee true by construction rather than by |
| 72 | +assertion: |
| 73 | + |
| 74 | +1. the operator **raises** on a string-versus-number/Timestamp pair instead of |
| 75 | + answering one, so the comparison cannot have produced an answer; |
| 76 | +2. the counterpart is a number or a Timestamp **in this scope**, read off the |
| 77 | + values in hand rather than off a static type (every field is `dyn` under |
| 78 | + `unlistedVariablesAreDyn`); |
| 79 | +3. the operand's own value is a §1c serialization artifact — an entirely-numeric |
| 80 | + string or an ISO-8601 date. A zip like `"02134"`, or free text, still faults |
| 81 | + loudly. |
| 82 | + |
| 83 | +Measured per operator on cel-js 8.0.0 and pinned in the new tests: `<` `<=` `>` |
| 84 | +`>=` `+` `-` `*` `/` `%` **fault** on a mixed pair and are eligible. `==`, `!=` |
| 85 | +and `in` **answer** across types — CEL equality is total — so they already had |
| 86 | +an answer and are never rewritten. That measurement is the root of the defect: |
| 87 | +the string equality above never faulted at all. |
| 88 | + |
| 89 | +`Field.date` strings not matching a Timestamp under `==` remains owned by |
| 90 | +`rewriteTemporalEquality`, which wraps them statically on the clean path, where |
| 91 | +both sides are known from the source instead of inferred from an unrelated |
| 92 | +conjunct's fault. |
| 93 | + |
| 94 | +## Reach |
| 95 | + |
| 96 | +`celEngine.evaluate` — the only home of this retry — does **not** reach RLS. |
| 97 | +Row-level security compiles its `using` / `check` predicates through |
| 98 | +`compileCelToFilter` (SQL pushdown) and `matchesFilterCondition` (write-side |
| 99 | +post-image), and declared sharing rules do the same; neither calls this |
| 100 | +evaluator. No access-control decision could be inverted by this. |
| 101 | + |
| 102 | +It does reach write-gating decisions, which is why the behaviour was not |
| 103 | +acceptable as documented: validation-rule predicates and `when` conditionals, |
| 104 | +`readonlyWhen`, hook `condition`s, automation/flow conditions, and formula |
| 105 | +fields and default values. A validation rule is **fail-closed** on a fault |
| 106 | +(#4649) — but a silently flipped boolean is not a fault, so a rule that should |
| 107 | +have rejected a write instead read as "not violated" and let it through. |
0 commit comments