diff --git a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel index 7258ffe34..920917860 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel @@ -170,6 +170,7 @@ java_library( name = "eval_create_list", srcs = ["EvalCreateList.java"], deps = [ + ":eval_const", ":eval_helpers", ":planned_interpretable", "//common/ast", @@ -184,6 +185,7 @@ java_library( name = "eval_create_map", srcs = ["EvalCreateMap.java"], deps = [ + ":eval_const", ":eval_helpers", ":localized_evaluation_exception", ":planned_interpretable", @@ -191,7 +193,6 @@ java_library( "//common/exceptions:duplicate_key", "//common/exceptions:invalid_argument", "//runtime:accumulated_unknowns", - "//runtime:evaluation_exception", "//runtime:interpretable", "@maven//:com_google_errorprone_error_prone_annotations", "@maven//:com_google_guava_guava", @@ -840,6 +841,7 @@ cel_android_library( name = "eval_create_list_android", srcs = ["EvalCreateList.java"], deps = [ + ":eval_const_android", ":eval_helpers_android", ":planned_interpretable_android", "//common/ast:ast_android", @@ -854,13 +856,13 @@ cel_android_library( name = "eval_create_map_android", srcs = ["EvalCreateMap.java"], deps = [ + ":eval_const_android", ":eval_helpers_android", ":localized_evaluation_exception_android", ":planned_interpretable_android", "//common/ast:ast_android", "//common/exceptions:duplicate_key", "//common/exceptions:invalid_argument", - "//runtime:evaluation_exception", "//runtime:interpretable_android", "//runtime/src/main/java/dev/cel/runtime:accumulated_unknowns_android", "@maven//:com_google_errorprone_error_prone_annotations", diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateList.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateList.java index 265da3ab3..f5616c634 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateList.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateList.java @@ -66,8 +66,18 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) { return builder.build(); } - static EvalCreateList create(CelExpr expr, PlannedInterpretable[] values, boolean[] isOptional) { - return new EvalCreateList(expr, values, isOptional); + static PlannedInterpretable create( + CelExpr expr, PlannedInterpretable[] values, boolean[] isOptional) { + for (int i = 0; i < values.length; i++) { + if (!(values[i] instanceof EvalConstant) || isOptional[i]) { + return new EvalCreateList(expr, values, isOptional); + } + } + ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(values.length); + for (PlannedInterpretable value : values) { + builder.add(((EvalConstant) value).constantValue()); + } + return EvalConstant.create(expr, builder.build()); } private EvalCreateList(CelExpr expr, PlannedInterpretable[] values, boolean[] isOptional) { diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateMap.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateMap.java index 8d34c10d0..ca3d35754 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateMap.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalCreateMap.java @@ -14,7 +14,8 @@ package dev.cel.runtime.planner; -import com.google.common.base.Preconditions; +import static com.google.common.base.Preconditions.checkArgument; + import com.google.common.collect.ImmutableMap; import com.google.common.collect.Sets; import com.google.common.primitives.UnsignedLong; @@ -23,10 +24,10 @@ import dev.cel.common.exceptions.CelDuplicateKeyException; import dev.cel.common.exceptions.CelInvalidArgumentException; import dev.cel.runtime.AccumulatedUnknowns; -import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.GlobalResolver; import java.util.HashSet; import java.util.Optional; +import java.util.Set; @Immutable final class EvalCreateMap extends PlannedInterpretable { @@ -44,7 +45,7 @@ final class EvalCreateMap extends PlannedInterpretable { private final boolean[] isOptional; @Override - Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEvaluationException { + Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) { ImmutableMap.Builder builder = ImmutableMap.builderWithExpectedSize(keys.length); HashSet keysSeen = Sets.newHashSetWithExpectedSize(keys.length); @@ -52,37 +53,12 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEva for (int i = 0; i < keys.length; i++) { PlannedInterpretable keyInterpretable = keys[i]; - Object key = keyInterpretable.eval(resolver, frame); + Object key = EvalHelpers.evalStrictly(keyInterpretable, resolver, frame); if (key instanceof AccumulatedUnknowns) { unknowns = AccumulatedUnknowns.maybeMerge(unknowns, key); } else { - if (!(key instanceof String - || key instanceof Long - || key instanceof UnsignedLong - || key instanceof Boolean)) { - throw new LocalizedEvaluationException( - new CelInvalidArgumentException("Unsupported key type: " + key), - keyInterpretable.expr().id()); - } - - boolean isDuplicate = !keysSeen.add(key); - if (!isDuplicate) { - if (key instanceof Long) { - long longVal = (Long) key; - if (longVal >= 0) { - isDuplicate = keysSeen.contains(UnsignedLong.valueOf(longVal)); - } - } else if (key instanceof UnsignedLong) { - UnsignedLong ulongVal = (UnsignedLong) key; - isDuplicate = keysSeen.contains(ulongVal.longValue()); - } - } - - if (isDuplicate) { - throw new LocalizedEvaluationException( - CelDuplicateKeyException.of(key), keyInterpretable.expr().id()); - } + validateAndTrackKey(keysSeen, keyInterpretable, key); } Object val = EvalHelpers.evalStrictly(values[i], resolver, frame); @@ -119,12 +95,61 @@ Object evalInternal(GlobalResolver resolver, ExecutionFrame frame) throws CelEva return builder.buildOrThrow(); } - static EvalCreateMap create( + private static void validateAndTrackKey( + Set keysSeen, PlannedInterpretable keyInterpretable, Object key) { + if (!(key instanceof String + || key instanceof Long + || key instanceof UnsignedLong + || key instanceof Boolean)) { + throw new LocalizedEvaluationException( + new CelInvalidArgumentException("Unsupported key type: " + key), + keyInterpretable.expr().id()); + } + + boolean isDuplicate = !keysSeen.add(key); + if (!isDuplicate) { + if (key instanceof Long) { + long longVal = (Long) key; + if (longVal >= 0) { + isDuplicate = keysSeen.contains(UnsignedLong.valueOf(longVal)); + } + } else if (key instanceof UnsignedLong) { + UnsignedLong ulongVal = (UnsignedLong) key; + isDuplicate = keysSeen.contains(ulongVal.longValue()); + } + } + + if (isDuplicate) { + throw new LocalizedEvaluationException( + CelDuplicateKeyException.of(key), keyInterpretable.expr().id()); + } + } + + static PlannedInterpretable create( CelExpr expr, PlannedInterpretable[] keys, PlannedInterpretable[] values, boolean[] isOptional) { - return new EvalCreateMap(expr, keys, values, isOptional); + for (int i = 0; i < keys.length; i++) { + if (!(keys[i] instanceof EvalConstant) + || !(values[i] instanceof EvalConstant) + || isOptional[i]) { + return new EvalCreateMap(expr, keys, values, isOptional); + } + } + ImmutableMap.Builder builder = + ImmutableMap.builderWithExpectedSize(keys.length); + HashSet keysSeen = Sets.newHashSetWithExpectedSize(keys.length); + try { + for (int i = 0; i < keys.length; i++) { + Object key = ((EvalConstant) keys[i]).constantValue(); + validateAndTrackKey(keysSeen, keys[i], key); + builder.put(key, ((EvalConstant) values[i]).constantValue()); + } + } catch (LocalizedEvaluationException e) { + return new EvalCreateMap(expr, keys, values, isOptional); + } + return EvalConstant.create(expr, builder.buildOrThrow()); } private EvalCreateMap( @@ -133,8 +158,8 @@ private EvalCreateMap( PlannedInterpretable[] values, boolean[] isOptional) { super(expr); - Preconditions.checkArgument(keys.length == values.length); - Preconditions.checkArgument(keys.length == isOptional.length); + checkArgument(keys.length == values.length); + checkArgument(keys.length == isOptional.length); this.keys = keys; this.values = values; this.isOptional = isOptional; diff --git a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java index f643f5c44..b2f8c68e3 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java @@ -562,6 +562,19 @@ public void plan_createList() throws Exception { ImmutableList result = (ImmutableList) program.eval(); assertThat(result).containsExactly(1L, "foo", true, ImmutableList.of(2L, false)).inOrder(); + assertThat(program.eval()).isSameInstanceAs(result); + } + + @Test + @SuppressWarnings("unchecked") // test only + public void plan_createList_withVariable() throws Exception { + CelAbstractSyntaxTree ast = compile("[1, int_var]"); + Program program = PLANNER.plan(ast); + + ImmutableList result = + (ImmutableList) program.eval(ImmutableMap.of("int_var", 2L)); + + assertThat(result).containsExactly(1L, 2L).inOrder(); } @Test @@ -573,6 +586,40 @@ public void plan_createMap() throws Exception { ImmutableMap result = (ImmutableMap) program.eval(); assertThat(result).containsExactly("foo", 1L, true, "bar").inOrder(); + assertThat(program.eval()).isSameInstanceAs(result); + } + + @Test + @TestParameters({ + "{expression: \"{'foo': 1, dyn_var: 2}\"}", + "{expression: \"{'foo': 1, 'bar': int_var}\"}" + }) + @SuppressWarnings("unchecked") // test only + public void plan_createMap_withVariable(String expression) throws Exception { + CelAbstractSyntaxTree ast = compile(expression); + Program program = PLANNER.plan(ast); + + ImmutableMap result = + (ImmutableMap) + program.eval(ImmutableMap.of("dyn_var", "bar", "int_var", 2L)); + + assertThat(result).containsExactly("foo", 1L, "bar", 2L).inOrder(); + } + + @Test + @TestParameters({ + "{expression: '[?1]'," + + " expectedError: 'Cannot initialize optional list element from non-optional value 1'}", + "{expression: '{?1: \"one\"}'," + + " expectedError: \"Cannot initialize optional entry '1' from non-optional value one\"}" + }) + public void plan_createContainer_optionalNonOptionalConstant_throwsAtEval( + String expression, String expectedError) throws Exception { + CelAbstractSyntaxTree ast = CEL_COMPILER.parse(expression).getAst(); + Program program = PLANNER.plan(ast); + + CelEvaluationException e = assertThrows(CelEvaluationException.class, program::eval); + assertThat(e).hasMessageThat().contains(expectedError); } @Test diff --git a/testing/src/main/java/dev/cel/testing/testrunner/CelCoverageIndex.java b/testing/src/main/java/dev/cel/testing/testrunner/CelCoverageIndex.java index d99525934..cdafb9907 100644 --- a/testing/src/main/java/dev/cel/testing/testrunner/CelCoverageIndex.java +++ b/testing/src/main/java/dev/cel/testing/testrunner/CelCoverageIndex.java @@ -257,7 +257,13 @@ private void traverseAndCalculateCoverage( nodeId, stats, exprText, precedingTabs, logUnencountered, reportBuilder); } + boolean propagateChildCoverage = + stats.covered.get() + && (node.getKind() == ExprKind.Kind.LIST || node.getKind() == ExprKind.Kind.MAP); for (CelNavigableExpr child : node.children().collect(toImmutableList())) { + if (propagateChildCoverage) { + statsMap.get(child.id()).covered.set(true); + } dotGraphBuilder.append(String.format("%d -> %d;\n", nodeId, child.id())); traverseAndCalculateCoverage( child, statsMap, logUnencountered, precedingTabs, reportBuilder, dotGraphBuilder); diff --git a/testing/src/test/java/dev/cel/testing/testrunner/CelCoverageIndexTest.java b/testing/src/test/java/dev/cel/testing/testrunner/CelCoverageIndexTest.java index a13d76914..639ed8d83 100644 --- a/testing/src/test/java/dev/cel/testing/testrunner/CelCoverageIndexTest.java +++ b/testing/src/test/java/dev/cel/testing/testrunner/CelCoverageIndexTest.java @@ -178,4 +178,22 @@ public void getCoverageReport_fullCoverage_multipleEvaluations() throws Exceptio assertThat(report.unencounteredNodes()).isEmpty(); assertThat(report.unencounteredBranches()).isEmpty(); } + + @Test + public void getCoverageReport_constantContainers_propagatesCoverageToEvaluatedBranchesOnly() + throws Exception { + Cel cel = CelFactory.plannerCelBuilder().addVar("x", SimpleType.INT).build(); + CelAbstractSyntaxTree ast = cel.compile("x > 1 ? {'a': 1}['a'] == 1 : [1, 2][0] == 1").getAst(); + CelRuntime.Program program = cel.createProgram(ast); + CelCoverageIndex coverageIndex = new CelCoverageIndex(); + coverageIndex.init(ast); + CelEvaluationListener listener = coverageIndex.newEvaluationListener(); + + program.trace(ImmutableMap.of("x", 2L), listener); + CoverageReport report = coverageIndex.generateCoverageReport(); + + assertThat(report.coveredNodes()).isEqualTo(11); + assertThat(report.nodes()).isEqualTo(18); + assertThat(report.unencounteredNodes()).containsExactly("Expression ID 18 ('[1, 2][0] == 1')"); + } }