From 21268447f0a7dd5c2d12b036b3572fddc4d0f5bc Mon Sep 17 00:00:00 2001 From: Santiago Date: Mon, 27 Jul 2026 14:39:52 -0500 Subject: [PATCH] feat: add v2-compat mode to evaluation entry points All evaluation entry points now accept a trailing options list; mode: :v2 expands to the v2 context-normalization flags (lowercase_keys: true, coerce_strings: true), with explicitly passed flags winning over the expansion. This lets consumers select v2 or v3 semantics per evaluation in the same VM - the mechanism engage uses to keep existing journeys on v2 behavior while new journeys adopt v3. The audit for this work corrected two stale claims in the release notes: the binary-literal recursion removal was reverted before rc.0 (6e8355a), and the documented v2 date-only DateTime = comparison was unreachable dead code because the parser collapses = and == into the same operator in both versions. Neither needs a compat flag; the compat test corpus pins both findings. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 22 ++-- V3_PLAN.md | 63 +++--------- lib/expression.ex | 76 +++++++++++--- test/expression/v2_compat_test.exs | 159 +++++++++++++++++++++++++++++ 4 files changed, 250 insertions(+), 70 deletions(-) create mode 100644 test/expression/v2_compat_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index fe5c5d1..2ccbdb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ This is the first release candidate for v3.0.0. It contains the breaking changes intended for the major version. Cut to allow downstream CI runs -(primarily engage) to surface migration work before the final v3.0.0 tag. +to surface migration work before the final v3.0.0 tag. ### Breaking changes @@ -16,12 +16,6 @@ changes intended for the major version. Cut to allow downstream CI runs `Context.new(ctx, lowercase_keys: true, coerce_strings: true)`. - **`:skip_context_evaluation?` option has been removed.** Use `coerce_strings: false` instead. -- **String literals inside expressions are no longer re-evaluated as - templates.** `Eval.eval!({:literal, "hello @name"}, ctx, mod)` now - returns the string verbatim. Top-level template behavior via - `Expression.evaluate_as_string!/3` is unchanged. Use - `Expression.evaluate_template/3` for explicit template resolution - inside callback arguments. - **`Expression.error/1` has been removed.** Use `Expression.error_map/1` to produce the legacy error map shape, or raise `Expression.Error` for new code. @@ -33,9 +27,17 @@ changes intended for the major version. Cut to allow downstream CI runs using V2 for static analysis (`Expression.V2.parse_block/1`, `Expression.V2.Compile.to_quoted/1`) will need to migrate to V1's AST surface or build their own analysis layer. -- **`=` on two `DateTime` values now compares both date and time**, - matching `==`. v2 incorrectly compared only the date portion via - `Date.compare/2`. + +### Added + +- **v2-compat mode on all evaluation entry points.** `evaluate!/4`, + `evaluate/4`, `evaluate_block!/4`, `evaluate_block/4`, + `evaluate_as_string!/4`, `evaluate_as_boolean!/4` and + `evaluate_template!/4` accept a trailing options list. `mode: :v2` + expands to `lowercase_keys: true, coerce_strings: true`, restoring the + v2 context normalization per evaluation; explicitly passed flags win + over the expansion. This lets a consumer run v2- and v3-semantics + evaluations side by side in the same VM. ### Migration diff --git a/V3_PLAN.md b/V3_PLAN.md index 9c2ee6a..048d333 100644 --- a/V3_PLAN.md +++ b/V3_PLAN.md @@ -16,11 +16,13 @@ its dependencies on the current behavior are noted inline. ### In scope (breaking) 1. **Flip `Context.new/2` defaults**: `coerce_strings: false`, `lowercase_keys: false`. *(Section 10.2)* -2. **Remove implicit binary-literal recursion**: `Eval.eval!({:literal, binary}, ...)` returns the string verbatim. Templates require `Expression.evaluate_template/3`. *(Section 10.5)* -3. **Remove `Expression.error/1`** (deprecated since v2.x). *(Section 10.1)* -4. **Remove `Expression.V2.Compat`** (deprecated since v2.x). *(Section 10.7)* -5. **Drop the `:skip_context_evaluation?` legacy alias** on `Context.new/2` (replaced by `coerce_strings:`). -6. **Fix `=` vs `==` DateTime asymmetry**: `=` on two DateTimes now compares full DateTime (matches `==`). v2 incorrectly compared only the date portion. *(Section 2.4)* +2. **Remove `Expression.error/1`** (deprecated since v2.x). *(Section 10.1)* +3. **Remove the entire V2 namespace** including `Expression.V2.Compat`. *(Section 10.7)* +4. **Drop the `:skip_context_evaluation?` legacy alias** on `Context.new/2` (replaced by `coerce_strings:`). + +### In scope (additive) + +5. **v2-compat mode**: all evaluation entry points accept a trailing options list; `mode: :v2` expands to `lowercase_keys: true, coerce_strings: true`, restoring v2 context normalization per evaluation so consumers can run v2- and v3-semantics evaluations side by side in one VM. ### Out of scope (defer to v3.x or v4) @@ -60,33 +62,7 @@ The evaluator already supports case-insensitive lookup (`Eval.case_insensitive_g - String coercion is the risk: engage may rely on `"2020-12-13T..."` → `DateTime` conversion implicitly. Audit `Build.default_context/1` and any `Context.new/1` call sites in engage before merging. - Engage test `test/turn/build/callbacks_test.exs:291` passes `[]` opts explicitly — stays correct, but its expectations may shift if the test data contains coercible strings. -### 2. Remove binary-literal recursion - -**Change** - -```elixir -# v2: re-evaluates the string as an expression template -Eval.eval!({:literal, "hello @name"}, ctx, mod) -#=> "hello world" (if @name resolves) - -# v3: returns the literal verbatim -Eval.eval!({:literal, "hello @name"}, ctx, mod) -#=> "hello @name" -``` - -The `eval!({:literal, literal}, context, mod) when is_binary(literal)` clause delegates to `Expression.evaluate_as_string!/3`, which is what makes nested template resolution work inside expressions. Removing the clause means literals are inert — `evaluate_template/3` is the explicit replacement. - -**Migration** - -- Top-level callers already get template behavior via `Expression.evaluate_as_string!/3` (entry point), so most templates keep working. -- The break is for callers who put `@var`-containing strings as **arguments to functions** and expected resolution. Those must move to `evaluate_template/3` at the call site, or the callback must call `evaluate_template/3` on the arg explicitly. - -**Engage impact** - -- `Build.Callbacks` uses this for nested template resolution (called out in review §2.1). Needs an audit pass: for every callback that takes a string argument and expects `@var` to be resolved, decide whether to (a) call `Expression.evaluate_template/3` inside the callback, or (b) require the caller to pre-resolve. -- This is the highest-risk break in v3. Recommend running engage's full test suite against the v3 branch before tagging. - -### 3. Remove `Expression.error/1` +### 2. Remove `Expression.error/1` **Change** @@ -96,7 +72,7 @@ The `eval!({:literal, literal}, context, mod) when is_binary(literal)` clause de - One call site: `lib/turn/build/callbacks.ex:188` (`Expression.error("Unable to generate token")`). Engage must switch to `Expression.error_map/1` or to raising/returning `Expression.Error`. Coordinate the engage PR with the v3 bump. -### 4. Remove `Expression.V2.Compat` +### 3. Remove `Expression.V2.Compat` **Change** @@ -107,7 +83,7 @@ The module is deleted. `test/expression/v2/eval_compat_test.exs` is removed too. - None. Engage's only `Compat` references are to `Turn.Elasticsearch.Index.Compat.Bulk` (unrelated). Verified. - `flow_runner` does not reference it either (verified — no matches in `deps/`). -### 5. Drop `:skip_context_evaluation?` alias +### 4. Drop `:skip_context_evaluation?` alias **Change** @@ -141,10 +117,9 @@ Each step is its own commit; each commit ships green tests. 3. **Remove `Expression.error/1`**; keep `error_map/1` as `@doc true` public. 4. **Drop `:skip_context_evaluation?` alias** in `Context.new/2`. 5. **Flip `Context.new/2` defaults** to `lowercase_keys: false, coerce_strings: false`. Update doctests in `lib/expression/context.ex`. Run targeted tests on `test/expression/context_test.exs` and any test that builds contexts with mixed-case or string-coercible values. -6. **Remove the binary-literal recursion clause** in `lib/expression/eval.ex:140-142`. Run targeted tests on `test/expression_test.exs` and `test/expression/eval_test.exs`. Decide per failing test whether the test was asserting the recursion (update to use `evaluate_template`) or hit the clause incidentally (update assertion). -7. **Fix `=`/`==` DateTime asymmetry** in `op/3` (§2.4). -8. **Tag `3.0.0-rc.0`** and run full engage CI against it. Address fallout (engage's `Expression.error/1` call site, any binary-literal recursion dependencies). -9. **Bump `@version` to `3.0.0`** and finalize `CHANGELOG.md` once rc is green. +6. **Tag `3.0.0-rc.0`** and run full engage CI against it. Address fallout (engage's `Expression.error/1` call site). +7. **Add the v2-compat mode** (`mode: :v2` options on all evaluation entry points) with a test corpus proving the v2 semantics. +8. **Bump `@version` to `3.0.0`** and finalize `CHANGELOG.md` once rc is green. --- @@ -160,15 +135,12 @@ Each step is its own commit; each commit ships green tests. v2 behavior. Variable lookup remains case-insensitive at evaluation time. - The `:skip_context_evaluation?` option on `Context.new/2` has been removed. Use `coerce_strings: false` instead. -- String literals inside expressions are no longer re-evaluated as - templates. Use `Expression.evaluate_template/3` for explicit template - resolution. Top-level `evaluate_as_string!/3` is unchanged. - `Expression.error/1` has been removed. Use `Expression.error_map/1` for the legacy map shape, or raise `Expression.Error` for new code. - `Expression.V2.Compat` has been removed. The shim returned V1 results with V2 disabled — call `Expression` directly. -- (If included) `=` on two `DateTime` values now compares both date and - time, matching `==`. v2 incorrectly compared only the date portion. +- All evaluation entry points accept a trailing options list; `mode: :v2` + restores the v2 context normalization per evaluation. ### Migration @@ -179,6 +151,5 @@ See V3_MIGRATION.md (or a section here). ## Open questions for the user -1. **Include the `=`/`==` DateTime fix?** Bug fix, breaking, but tiny and well-isolated. -2. **Tag a `3.0.0-rc.0` first?** Lets engage CI run against the rc before we finalize. -3. **Coordinate the engage PR before tagging v3.0.0?** Engage `callbacks.ex:188` needs to migrate off `Expression.error/1`. Either (a) land an engage-side fix to `error_map/1` first (compatible with v2.49 since `error_map/1` already exists), then bump engage to v3, or (b) tag v3 and let engage break until the engage PR lands. +1. **Tag a `3.0.0-rc.0` first?** Lets engage CI run against the rc before we finalize. *(Done — rc.0 is tagged.)* +2. **Coordinate the engage PR before tagging v3.0.0?** Engage `callbacks.ex:188` needs to migrate off `Expression.error/1`. Either (a) land an engage-side fix to `error_map/1` first (compatible with v2.49 since `error_map/1` already exists), then bump engage to v3, or (b) tag v3 and let engage break until the engage PR lands. *(Resolved: (a) — engage migrated on v2.49.1 in turnhub/engage#13034.)* diff --git a/lib/expression.ex b/lib/expression.ex index ff9e834..6e97531 100644 --- a/lib/expression.ex +++ b/lib/expression.ex @@ -35,6 +35,17 @@ defmodule Expression do Hello @contact.name, you were born in @(YEAR(contact.birthday)) ``` + ## v2-compat mode + + All evaluation entry points accept a trailing options list. Passing + `mode: :v2` restores the v2 evaluation semantics that changed in v3 + (context key lowercasing and string value coercion): + + ``` + Expression.evaluate_as_string!("@date", %{"date" => "2020-12-13T23:34:45"}, mod, mode: :v2) + ``` + + See `t:evaluation_opts/0` for the individual flags. """ @type expression_type :: @@ -113,12 +124,44 @@ defmodule Expression do @spec time_struct?(String.t() | Time.t()) :: boolean def time_struct?(value), do: is_struct(value, Time) + @typedoc """ + Options accepted by the evaluation entry points. + + `mode: :v2` expands to the v2-compat flags: + `lowercase_keys: true, coerce_strings: true`. Explicitly passed flags win + over the expansion. `mode: :v3` (or omitting `:mode`) keeps the v3 + defaults. + + * `:lowercase_keys` / `:coerce_strings` - see `Expression.Context.new/2`. + + Note: v2's operator semantics need no compat flags — the parser collapses + `=` and `==` into the same operator in both v2 and v3, so the documented + v2 "date-only `=`" behavior was unreachable dead code and evaluation + semantics beyond context normalization are unchanged. + """ + @type evaluation_opts :: [ + mode: :v2 | :v3, + lowercase_keys: boolean(), + coerce_strings: boolean() + ] + + @v2_mode_opts [lowercase_keys: true, coerce_strings: true] + + @spec normalize_opts(evaluation_opts()) :: Keyword.t() + defp normalize_opts(opts) do + case Keyword.pop(opts, :mode) do + {:v2, rest} -> Keyword.merge(@v2_mode_opts, rest) + {_v3_or_nil, rest} -> rest + end + end + def evaluate_block!( expression, context \\ %{}, mod \\ Expression.Callbacks, opts \\ [] ) do + opts = normalize_opts(opts) ast = parse_expression!(expression) Eval.eval!([expression: ast], Context.new(context, opts), mod) rescue @@ -137,10 +180,12 @@ defmodule Expression do e in Expression.Error -> {:error, e} end - def evaluate!(expression, context \\ %{}, mod \\ Expression.Callbacks) do + def evaluate!(expression, context \\ %{}, mod \\ Expression.Callbacks, opts \\ []) do + opts = normalize_opts(opts) + expression |> parse! - |> Eval.eval!(Context.new(context), mod) + |> Eval.eval!(Context.new(context, opts), mod) |> Eval.default_value() rescue e in Expression.Error -> @@ -155,22 +200,25 @@ defmodule Expression do @spec evaluate_as_string!( String.t() | Number.t() | nil, map(), - module() + module(), + evaluation_opts() ) :: String.t() - def evaluate_as_string!(expression, context \\ %{}, mod \\ Expression.Callbacks) + def evaluate_as_string!(expression, context \\ %{}, mod \\ Expression.Callbacks, opts \\ []) + + def evaluate_as_string!(nil, _context, _mod, _opts), do: "" - def evaluate_as_string!(nil, _context, _mod), do: "" + def evaluate_as_string!(expression, context, mod, opts) do + opts = normalize_opts(opts) - def evaluate_as_string!(expression, context, mod) do expression |> parse! - |> Eval.eval!(Context.new(context), mod) + |> Eval.eval!(Context.new(context, opts), mod) |> Eval.default_value(handle_not_found: true) |> stringify() end - def evaluate_as_boolean!(expression, context \\ %{}, mod \\ Expression.Callbacks) do - case evaluate!(expression, context, mod) do + def evaluate_as_boolean!(expression, context \\ %{}, mod \\ Expression.Callbacks, opts \\ []) do + case evaluate!(expression, context, mod, opts) do boolean when is_boolean(boolean) -> boolean @@ -196,8 +244,8 @@ defmodule Expression do def stringify(map) when is_map(map), do: "#{inspect(map)}" def stringify(other), do: to_string(other) - def evaluate(expression, context \\ %{}, mod \\ Expression.Callbacks) do - {:ok, evaluate!(expression, context, mod)} + def evaluate(expression, context \\ %{}, mod \\ Expression.Callbacks, opts \\ []) do + {:ok, evaluate!(expression, context, mod, opts)} rescue e in Expression.Error -> {:error, e} end @@ -238,9 +286,9 @@ defmodule Expression do "1 + 1 = 2" """ - @spec evaluate_template!(String.t(), map(), module()) :: String.t() - def evaluate_template!(template, context \\ %{}, mod \\ Expression.Callbacks) do - evaluate_as_string!(template, context, mod) + @spec evaluate_template!(String.t(), map(), module(), evaluation_opts()) :: String.t() + def evaluate_template!(template, context \\ %{}, mod \\ Expression.Callbacks, opts \\ []) do + evaluate_as_string!(template, context, mod, opts) end defdelegate prewalk(ast, fun), to: Macro diff --git a/test/expression/v2_compat_test.exs b/test/expression/v2_compat_test.exs new file mode 100644 index 0000000..d9a35be --- /dev/null +++ b/test/expression/v2_compat_test.exs @@ -0,0 +1,159 @@ +defmodule Expression.V2CompatTest do + @moduledoc """ + Proves that `mode: :v2` on the evaluation entry points restores the v2 + evaluation semantics that changed in v3: + + 1. context keys are lowercased (`lowercase_keys: true`) + 2. string values are coerced to typed equivalents (`coerce_strings: true`) + + Also pins down what did NOT change: operator semantics are identical in + v2 and v3. In particular the parser collapses `=` and `==` into the same + operator (see `OperatorHelpers.eq/1`), so the "date-only `=`" behavior + documented for v2 was unreachable — DateTime equality always compares the + full value, in both versions and both modes. + """ + use ExUnit.Case, async: true + + @morning ~U[2023-01-15 10:00:00Z] + @evening ~U[2023-01-15 22:30:00Z] + + describe "context string coercion" do + test "evaluate!/4 coerces string values under mode: :v2" do + ctx = %{"age" => "30", "opted_in" => "true", "date" => "2020-12-13T23:34:45"} + + assert Expression.evaluate!("@age", ctx, Expression.Callbacks, mode: :v2) == 30 + assert Expression.evaluate!("@opted_in", ctx, Expression.Callbacks, mode: :v2) == true + + assert Expression.evaluate!("@date", ctx, Expression.Callbacks, mode: :v2) == + ~U[2020-12-13 23:34:45.0Z] + end + + test "evaluate!/4 preserves string values by default" do + ctx = %{"age" => "30", "opted_in" => "true", "date" => "2020-12-13T23:34:45"} + + assert Expression.evaluate!("@age", ctx) == "30" + assert Expression.evaluate!("@opted_in", ctx) == "true" + assert Expression.evaluate!("@date", ctx) == "2020-12-13T23:34:45" + end + + test "evaluate_as_string!/4 renders coerced datetimes in the v2 shape" do + ctx = %{"date" => "2020-12-13T23:34:45"} + + assert Expression.evaluate_as_string!("@date", ctx, Expression.Callbacks, mode: :v2) == + "2020-12-13T23:34:45.0Z" + + assert Expression.evaluate_as_string!("@date", ctx) == "2020-12-13T23:34:45" + end + + test "coerced values participate in comparisons under mode: :v2" do + ctx = %{"age" => "30"} + + assert Expression.evaluate!("@(age > 21)", ctx, Expression.Callbacks, mode: :v2) == true + end + end + + describe "context key lowercasing" do + test "maps resolved under mode: :v2 have lowercased keys" do + ctx = %{"Contact" => %{"Name" => "Jane"}} + + assert Expression.evaluate!("@contact", ctx, Expression.Callbacks, mode: :v2) == + %{"name" => "Jane"} + + assert Expression.evaluate!("@contact", ctx) == %{"Name" => "Jane"} + end + + test "variable lookup is case-insensitive in both modes" do + ctx = %{"Contact" => %{"Name" => "Jane"}} + + assert Expression.evaluate!("@contact.name", ctx, Expression.Callbacks, mode: :v2) == + "Jane" + + assert Expression.evaluate!("@contact.name", ctx) == "Jane" + end + end + + describe "operator semantics are mode-independent" do + test "`=` and `==` on DateTimes compare the full value in both modes" do + ctx = %{"a" => @morning, "b" => @evening, "same" => @morning} + + for opts <- [[], [mode: :v2]] do + assert Expression.evaluate!("@(a = b)", ctx, Expression.Callbacks, opts) == false + assert Expression.evaluate!("@(a == b)", ctx, Expression.Callbacks, opts) == false + assert Expression.evaluate!("@(a = same)", ctx, Expression.Callbacks, opts) == true + assert Expression.evaluate!("@(a == same)", ctx, Expression.Callbacks, opts) == true + end + end + end + + describe "entry point coverage" do + test "evaluate_block!/4 and evaluate_block/4 accept the mode" do + ctx = %{"age" => "30"} + + assert Expression.evaluate_block!("age", ctx, Expression.Callbacks, mode: :v2) == 30 + assert Expression.evaluate_block!("age", ctx) == "30" + + assert Expression.evaluate_block("age", ctx, Expression.Callbacks, mode: :v2) == + {:ok, 30} + end + + test "evaluate/4 accepts the mode" do + ctx = %{"age" => "30"} + + assert Expression.evaluate("@age", ctx, Expression.Callbacks, mode: :v2) == {:ok, 30} + assert Expression.evaluate("@age", ctx) == {:ok, "30"} + end + + test "evaluate_as_boolean!/4 accepts the mode" do + ctx = %{"opted_in" => "true"} + + assert Expression.evaluate_as_boolean!("@opted_in", ctx, Expression.Callbacks, mode: :v2) == + true + end + + test "evaluate_template!/4 accepts the mode" do + ctx = %{"date" => "2020-12-13T23:34:45"} + + assert Expression.evaluate_template!("on @date", ctx, Expression.Callbacks, mode: :v2) == + "on 2020-12-13T23:34:45.0Z" + end + + test "nested template literals see the v2-normalized context" do + ctx = %{"Date" => "2020-12-13T23:34:45"} + + assert Expression.evaluate_as_string!( + ~s[@if(true, "on @date", "")], + ctx, + Expression.Callbacks, + mode: :v2 + ) == "on 2020-12-13T23:34:45.0Z" + end + end + + describe "explicit flags vs mode expansion" do + test "explicitly passed flags override the mode expansion" do + ctx = %{"age" => "30", "Contact" => %{"Name" => "Jane"}} + + assert Expression.evaluate!("@age", ctx, Expression.Callbacks, + mode: :v2, + coerce_strings: false + ) == "30" + + assert Expression.evaluate!("@contact", ctx, Expression.Callbacks, + mode: :v2, + lowercase_keys: false + ) == %{"Name" => "Jane"} + end + + test "mode: :v3 keeps the v3 defaults" do + ctx = %{"age" => "30"} + + assert Expression.evaluate!("@age", ctx, Expression.Callbacks, mode: :v3) == "30" + end + + test "individual flags work without the mode sugar" do + ctx = %{"age" => "30"} + + assert Expression.evaluate!("@age", ctx, Expression.Callbacks, coerce_strings: true) == 30 + end + end +end