diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java index eedef672c..3c38eda02 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java @@ -755,12 +755,13 @@ public abstract static class Builder { /** * Adds a collection of custom functions that will be a candidate for common subexpression - * elimination. By default, standard functions are eliminable. + * elimination. Only takes effect with {@code eliminateAllFunctions(false)}, where standard + * functions are eliminable by default. * *
Note that the implementation of custom functions must be free of side effects. * - * @deprecated Use {@code eliminateAllFunctions(true)} and {@link #addNonEliminableFunctions} - * to exclude functions with side effects instead. + * @deprecated Use {@link #addNonEliminableFunctions} to exclude functions with side effects + * instead. */ @Deprecated @CanIgnoreReturnValue @@ -781,7 +782,7 @@ public Builder addEliminableFunctions(String... functions) { /** * Treats all functions, including custom ones, as candidates for common subexpression - * elimination. Functions with side effects must be excluded via {@link + * elimination (enabled by default). Functions with side effects must be excluded via {@link * #addNonEliminableFunctions}. */ public abstract Builder eliminateAllFunctions(boolean value); @@ -807,7 +808,7 @@ public static Builder newBuilder() { .iterationLimit(500) .enableCelBlock(true) .populateMacroCalls(false) - .eliminateAllFunctions(false); + .eliminateAllFunctions(true); } SubexpressionOptimizerOptions() {} diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java index 48991d72d..a50a0823b 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java @@ -222,10 +222,6 @@ private enum CseNoOpTestCase { // Constants and identifiers within a function CONST_WITHIN_FUNCTION("size(\"hello\" + \"hello\" + \"hello\")"), IDENT_WITHIN_FUNCTION("string(x + x + x)"), - // Non-standard functions that have not been explicitly added as a candidate are not - // optimized. - NON_STANDARD_FUNCTION_1("non_pure_custom_func(1) + non_pure_custom_func(1)"), - NON_STANDARD_FUNCTION_2("1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)"), // Duplicated but nested calls. NESTED_FUNCTION("int(timestamp(int(timestamp(1000000000))))"), // This cannot be optimized. Extracting the common subexpression would presence test @@ -265,19 +261,35 @@ public void cse_withCelBlock_noop(@TestParameter CseNoOpTestCase testCase) throw } @Test - public void cse_eliminateAllFunctions_customFunctionEliminated() throws Exception { + public void cse_customFunction_eliminatedByDefault() throws Exception { CelAbstractSyntaxTree ast = cel.compile("non_pure_custom_func(1) + non_pure_custom_func(1)").getAst(); CelAbstractSyntaxTree optimizedAst = - newCseOptimizer( - SubexpressionOptimizerOptions.newBuilder().eliminateAllFunctions(true).build()) - .optimize(ast); + newCseOptimizer(SubexpressionOptimizerOptions.newBuilder().build()).optimize(ast); assertThat(CEL_UNPARSER.unparse(optimizedAst)) .isEqualTo("cel.@block([non_pure_custom_func(1)], @index0 + @index0)"); } + @Test + public void cse_eliminateAllFunctionsDisabled_customFunctionRetained( + @TestParameter({ + "non_pure_custom_func(1) + non_pure_custom_func(1)", + "1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)" + }) + String source) + throws Exception { + CelAbstractSyntaxTree ast = cel.compile(source).getAst(); + + CelAbstractSyntaxTree optimizedAst = + newCseOptimizer( + SubexpressionOptimizerOptions.newBuilder().eliminateAllFunctions(false).build()) + .optimize(ast); + + assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(source); + } + @Test public void cse_eliminateAllFunctions_nonEliminableFunctionRetained() throws Exception { CelAbstractSyntaxTree ast = diff --git a/policy/src/test/java/dev/cel/policy/PolicyTestHelper.java b/policy/src/test/java/dev/cel/policy/PolicyTestHelper.java index 0315c7c02..3e00aa5bd 100644 --- a/policy/src/test/java/dev/cel/policy/PolicyTestHelper.java +++ b/policy/src/test/java/dev/cel/policy/PolicyTestHelper.java @@ -87,12 +87,11 @@ enum TestYamlPolicy { RESTRICTED_DESTINATIONS( "restricted_destinations", false, - "cel.@block([request.auth.claims, has(@index0.nationality), resource.labels.location in" + "cel.@block([request.auth.claims, has(@index0.nationality), locationCode(destination.ip) in" + + " spec.restricted_destinations || resource.labels.location in" + " spec.restricted_destinations], (@index1 && @index0.nationality == spec.origin &&" - + " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :" - + " ((!@index1 && locationCode(origin.ip) == spec.origin &&" - + " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :" - + " false))"), + + " @index2) ? true : ((!@index1 && locationCode(origin.ip) == spec.origin && @index2)" + + " ? true : false))"), K8S( "k8s", true,