From 4f9a3a8987fdb24297216b5817e4fcef7cb11b3a Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 15 Apr 2026 11:26:41 -0700 Subject: [PATCH] Add parsed-only evaluation coverage to Comprehensions Extensions Includes a fix to preserve first encountered error message for comprehensions PiperOrigin-RevId: 900264092 --- .../CelComprehensionsExtensionsTest.java | 150 +++++++++--------- .../extensions/CelListsExtensionsTest.java | 2 - .../java/dev/cel/runtime/planner/EvalAnd.java | 5 +- .../java/dev/cel/runtime/planner/EvalOr.java | 5 +- .../planner_unknownResultSet_errors.baseline | 4 +- 5 files changed, 89 insertions(+), 77 deletions(-) diff --git a/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java index 34696b688..fbe160cd3 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java @@ -15,11 +15,15 @@ package dev.cel.extensions; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; +import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableMap; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; +import dev.cel.bundle.Cel; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelFunctionDecl; import dev.cel.common.CelOptions; @@ -28,15 +32,15 @@ import dev.cel.common.exceptions.CelIndexOutOfBoundsException; import dev.cel.common.types.SimpleType; import dev.cel.common.types.TypeParamType; -import dev.cel.compiler.CelCompiler; -import dev.cel.compiler.CelCompilerFactory; import dev.cel.parser.CelMacro; import dev.cel.parser.CelStandardMacro; import dev.cel.parser.CelUnparser; import dev.cel.parser.CelUnparserFactory; import dev.cel.runtime.CelEvaluationException; -import dev.cel.runtime.CelRuntime; -import dev.cel.runtime.CelRuntimeFactory; +import dev.cel.testing.CelRuntimeFlavor; +import java.util.Map; +import org.junit.Assume; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -46,26 +50,35 @@ public class CelComprehensionsExtensionsTest { private static final CelOptions CEL_OPTIONS = CelOptions.current() + .enableHeterogeneousNumericComparisons(true) // Enable macro call population for unparsing .populateMacroCalls(true) .build(); - private static final CelCompiler CEL_COMPILER = - CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CEL_OPTIONS) - .setStandardMacros(CelStandardMacro.STANDARD_MACROS) - .addLibraries(CelExtensions.comprehensions()) - .addLibraries(CelExtensions.lists()) - .addLibraries(CelExtensions.strings()) - .addLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings()) - .build(); - private static final CelRuntime CEL_RUNTIME = - CelRuntimeFactory.standardCelRuntimeBuilder() - .addLibraries(CelOptionalLibrary.INSTANCE) - .addLibraries(CelExtensions.lists()) - .addLibraries(CelExtensions.strings()) - .addLibraries(CelExtensions.comprehensions()) - .build(); + @TestParameter public CelRuntimeFlavor runtimeFlavor; + @TestParameter public boolean isParseOnly; + + private Cel cel; + + @Before + public void setUp() { + // Legacy runtime does not support parsed-only evaluation mode. + Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.LEGACY) && isParseOnly); + this.cel = + runtimeFlavor + .builder() + .setOptions(CEL_OPTIONS) + .setStandardMacros(CelStandardMacro.STANDARD_MACROS) + .addCompilerLibraries(CelExtensions.comprehensions()) + .addCompilerLibraries(CelExtensions.lists()) + .addCompilerLibraries(CelExtensions.strings()) + .addCompilerLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings()) + .addRuntimeLibraries(CelOptionalLibrary.INSTANCE) + .addRuntimeLibraries(CelExtensions.lists()) + .addRuntimeLibraries(CelExtensions.strings()) + .addRuntimeLibraries(CelExtensions.comprehensions()) + .build(); + } private static final CelUnparser UNPARSER = CelUnparserFactory.newUnparser(); @@ -101,11 +114,7 @@ public void allMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -127,11 +136,7 @@ public void existsMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -156,11 +161,7 @@ public void exists_oneMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -182,11 +183,7 @@ public void transformListMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -210,11 +207,7 @@ public void transformMapMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -238,24 +231,22 @@ public void transformMapEntryMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test public void comprehension_onTypeParam_success() throws Exception { - CelCompiler celCompiler = - CelCompilerFactory.standardCelCompilerBuilder() + Assume.assumeFalse(isParseOnly); + Cel customCel = + runtimeFlavor + .builder() .setOptions(CEL_OPTIONS) .setStandardMacros(CelStandardMacro.STANDARD_MACROS) - .addLibraries(CelExtensions.comprehensions()) + .addCompilerLibraries(CelExtensions.comprehensions()) .addVar("items", TypeParamType.create("T")) .build(); - CelAbstractSyntaxTree ast = celCompiler.compile("items.all(i, v, v > 0)").getAst(); + CelAbstractSyntaxTree ast = customCel.compile("items.all(i, v, v > 0)").getAst(); assertThat(ast.getResultType()).isEqualTo(SimpleType.BOOL); } @@ -275,7 +266,7 @@ public void unparseAST_twoVarComprehension( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); + CelAbstractSyntaxTree ast = isParseOnly ? cel.parse(expr).getAst() : cel.compile(expr).getAst(); String unparsed = UNPARSER.unparse(ast); assertThat(unparsed).isEqualTo(expr); } @@ -318,8 +309,9 @@ public void unparseAST_twoVarComprehension( "{expr: \"{'hello': 'world', 'greetings': 'tacocat'}.transformMapEntry(k, v, []) == {}\"," + " err: 'no matching overload'}") public void twoVarComprehension_compilerErrors(String expr, String err) throws Exception { + Assume.assumeFalse(isParseOnly); CelValidationException e = - assertThrows(CelValidationException.class, () -> CEL_COMPILER.compile(expr).getAst()); + assertThrows(CelValidationException.class, () -> cel.compile(expr).getAst()); assertThat(e).hasMessageThat().contains(err); } @@ -339,34 +331,50 @@ public void twoVarComprehension_compilerErrors(String expr, String err) throws E + " '2.0' already exists\"}") public void twoVarComprehension_keyCollision_runtimeError(String expr, String err) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - - assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class); - assertThat(e).hasCauseThat().hasMessageThat().contains(err); + // Planner does not allow decimals for map keys + Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.PLANNER) && expr.contains("2.0")); + + CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> eval(expr)); + Throwable cause = + Throwables.getCausalChain(e).stream() + .filter(IllegalArgumentException.class::isInstance) + .filter(t -> t.getMessage() != null && t.getMessage().contains(err)) + .findFirst() + .orElse(null); + + assertWithMessage( + "Expected IllegalArgumentException with message containing '%s' in cause chain", err) + .that(cause) + .isNotNull(); } @Test public void twoVarComprehension_arithmeticException_runtimeError() throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[0].all(i, k, i/k < k)").getAst(); - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - + assertThrows(CelEvaluationException.class, () -> eval("[0].all(i, k, i/k < k)")); assertThat(e).hasCauseThat().isInstanceOf(CelDivideByZeroException.class); assertThat(e).hasCauseThat().hasMessageThat().contains("/ by zero"); } @Test public void twoVarComprehension_outOfBounds_runtimeError() throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[1, 2].exists(i, v, [0][v] > 0)").getAst(); - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - + assertThrows(CelEvaluationException.class, () -> eval("[1, 2].exists(i, v, [0][v] > 0)")); assertThat(e).hasCauseThat().isInstanceOf(CelIndexOutOfBoundsException.class); assertThat(e).hasCauseThat().hasMessageThat().contains("Index out of bounds: 1"); } + + private Object eval(String expression) throws Exception { + return eval(this.cel, expression, ImmutableMap.of()); + } + + private Object eval(Cel cel, String expression, Map variables) throws Exception { + CelAbstractSyntaxTree ast; + if (isParseOnly) { + ast = cel.parse(expression).getAst(); + } else { + ast = cel.compile(expression).getAst(); + } + return cel.createProgram(ast).eval(variables); + } } diff --git a/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java index 2083ccc42..b36e0e92e 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java @@ -334,8 +334,6 @@ private static Cel setupEnv(CelBuilder celBuilder) { .build(); } - - private Object eval(Cel cel, String expr) throws Exception { return eval(cel, expr, ImmutableMap.of()); } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java index eb7406071..91f5b2ff4 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java @@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) { return false; } } else if (argVal instanceof ErrorValue) { - errorValue = (ErrorValue) argVal; + // Preserve the first encountered error instead of overwriting it with subsequent errors. + if (errorValue == null) { + errorValue = (ErrorValue) argVal; + } } else if (argVal instanceof AccumulatedUnknowns) { unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal); } else { diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java index bc19ed81a..62e617d9d 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java @@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) { return true; } } else if (argVal instanceof ErrorValue) { - errorValue = (ErrorValue) argVal; + // Preserve the first encountered error instead of overwriting it with subsequent errors. + if (errorValue == null) { + errorValue = (ErrorValue) argVal; + } } else if (argVal instanceof AccumulatedUnknowns) { unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal); } else { diff --git a/runtime/src/test/resources/planner_unknownResultSet_errors.baseline b/runtime/src/test/resources/planner_unknownResultSet_errors.baseline index 812067ddf..7885e9da1 100644 --- a/runtime/src/test/resources/planner_unknownResultSet_errors.baseline +++ b/runtime/src/test/resources/planner_unknownResultSet_errors.baseline @@ -32,7 +32,7 @@ single_timestamp { seconds: 15 } , unknown_attributes=[x.single_int32]} -error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0 +error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0 error_code: BAD_FORMAT Source: x.single_int32 == 1 || x.single_timestamp <= timestamp("bad timestamp string") @@ -69,7 +69,7 @@ single_timestamp { seconds: 15 } , unknown_attributes=[x.single_int32]} -error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0 +error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0 error_code: BAD_FORMAT Source: x