Skip to content

Commit be424e1

Browse files
l46kokcopybara-github
authored andcommitted
Internal Changes
PiperOrigin-RevId: 996162785
1 parent d9cf3f9 commit be424e1

7 files changed

Lines changed: 117 additions & 17 deletions

File tree

‎optimizer/src/main/java/dev/cel/optimizer/AstMutator.java‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -889,14 +889,35 @@ private CelMutableSource normalizeMacroSource(
889889
}
890890
}
891891

892-
if (exprIdToReplace > 0) {
892+
// Handle replacing the synthetic single-element list inside a `map` or `filter` loop_step.
893+
//
894+
// Example: `[1].map(x, 10)`
895+
// - In the main AST, `loop_step` is `@result + [10]` (where `[10]` is a synthetic LIST
896+
// node wrapping the body `10`).
897+
// - In `macro_calls`, the call is `[1].map(x, 10)`, which only references the inner `10`
898+
// node—NOT the synthetic `[10]` LIST node.
899+
//
900+
// If a mutation replaces the `[10]` LIST node itself (e.g. with `[20]`), the loop above
901+
// won't find `[10]`'s ID in `macro_calls`, so we unwrap `20` from `[20]` into the macro
902+
// call (`[1].map(x, 20)`).
903+
//
904+
// We must first verify that:
905+
// 1. This macro is actually a COMPREHENSION (e.g. `has(msg.f)` is in `macro_calls` too,
906+
// but expands to a SELECT node, not a COMPREHENSION).
907+
// 2. The replaced LIST node is inside *this* comprehension's `loop_step` (not an unrelated
908+
// list replacement elsewhere in the AST, such as folding `[1, 2] + [3, 4]`).
909+
if (exprIdToReplace > 0
910+
&& allExprs.get(callId).getKind().equals(ExprKind.Kind.COMPREHENSION)) {
893911
long replacedId = idGenerator.generate(exprIdToReplace);
912+
CelMutableComprehension comprehension = allExprs.get(callId).comprehension();
894913
boolean isListExprBeingReplaced =
895914
allExprs.containsKey(replacedId)
896-
&& allExprs.get(replacedId).getKind().equals(ExprKind.Kind.LIST);
915+
&& allExprs.get(replacedId).getKind().equals(ExprKind.Kind.LIST)
916+
&& CelNavigableMutableExpr.fromExpr(comprehension.loopStep())
917+
.allNodes()
918+
.anyMatch(node -> node.id() == replacedId);
897919
if (isListExprBeingReplaced) {
898-
unwrapListArgumentsInMacroCallExpr(
899-
allExprs.get(callId).comprehension(), newMacroCallExpr);
920+
unwrapListArgumentsInMacroCallExpr(comprehension, newMacroCallExpr);
900921
}
901922
}
902923

‎optimizer/src/test/java/dev/cel/optimizer/AstMutatorTest.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,25 @@ public void list_replaceElement() throws Exception {
579579
assertThat(CEL_UNPARSER.unparse(result.toParsedAst())).isEqualTo("[2, 3, 5]");
580580
}
581581

582+
@Test
583+
public void list_replaceSubtreeWithListInAstWithHasMacro_success() throws Exception {
584+
CelAbstractSyntaxTree ast = CEL.compile("has(msg.single_int64) && 1 in [2]").getAst();
585+
CelMutableAst mutableAst = CelMutableAst.fromCelAst(ast);
586+
CelMutableExpr foldedList =
587+
CelMutableExpr.ofList(
588+
CelMutableList.create(
589+
CelMutableExpr.ofConstant(CelConstant.ofValue(1)),
590+
CelMutableExpr.ofConstant(CelConstant.ofValue(2))));
591+
592+
// Node 8 is `[2]`; replacing it with a LIST triggers normalizeMacroSource while `has(...)` is
593+
// present in macroCalls as a SELECT node rather than a COMPREHENSION node.
594+
CelAbstractSyntaxTree replacedAst =
595+
AST_MUTATOR.replaceSubtree(mutableAst, foldedList, 8).toParsedAst();
596+
597+
assertThat(CEL_UNPARSER.unparse(replacedAst)).isEqualTo("has(msg.single_int64) && 1 in [1, 2]");
598+
assertConsistentMacroCalls(replacedAst);
599+
}
600+
582601
@Test
583602
public void struct_replaceValue() throws Exception {
584603
// Tree shape (brackets are expr IDs):

‎policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import dev.cel.optimizer.CelOptimizer;
3939
import dev.cel.optimizer.CelOptimizerFactory;
4040
import dev.cel.optimizer.optimizers.ConstantFoldingOptimizer;
41+
import dev.cel.optimizer.optimizers.ConstantFoldingOptimizer.ConstantFoldingOptions;
4142
import dev.cel.optimizer.optimizers.SubexpressionOptimizer;
4243
import dev.cel.optimizer.optimizers.SubexpressionOptimizer.SubexpressionOptimizerOptions;
4344
import dev.cel.policy.CelCompiledRule.CelCompiledMatch;
@@ -419,9 +420,15 @@ static Builder newBuilder(Cel cel) {
419420
.setIterationLimit(DEFAULT_ITERATION_LIMIT)
420421
.setOptimizers(
421422
ImmutableList.of(
422-
ConstantFoldingOptimizer.getInstance(),
423+
ConstantFoldingOptimizer.newInstance(
424+
ConstantFoldingOptions.newBuilder()
425+
.maxIterationLimit(DEFAULT_ITERATION_LIMIT)
426+
.build()),
423427
SubexpressionOptimizer.newInstance(
424-
SubexpressionOptimizerOptions.newBuilder().populateMacroCalls(true).build())));
428+
SubexpressionOptimizerOptions.newBuilder()
429+
.iterationLimit(DEFAULT_ITERATION_LIMIT)
430+
.populateMacroCalls(true)
431+
.build())));
425432
}
426433

427434
private CelPolicyCompilerImpl(

‎policy/src/main/java/dev/cel/policy/tools/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ java_library(
2525
"//common:proto_v1alpha1_ast",
2626
"//extensions",
2727
"//extensions:optional_library",
28+
"//optimizer:ast_optimizer",
2829
"//optimizer/optimizers:common_subexpression_elimination",
2930
"//optimizer/optimizers:constant_folding",
3031
"//optimizer/optimizers:select_optimizer",

‎policy/src/main/java/dev/cel/policy/tools/CelPolicyCompilerTool.java‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@
3636
import dev.cel.common.CelProtoV1Alpha1AbstractSyntaxTree;
3737
import dev.cel.extensions.CelExtensions;
3838
import dev.cel.extensions.CelOptionalLibrary;
39+
import dev.cel.optimizer.CelAstOptimizer;
3940
import dev.cel.optimizer.optimizers.ConstantFoldingOptimizer;
41+
import dev.cel.optimizer.optimizers.ConstantFoldingOptimizer.ConstantFoldingOptions;
4042
import dev.cel.optimizer.optimizers.SelectOptimizer;
4143
import dev.cel.optimizer.optimizers.SelectOptimizer.SelectOptimizerOptions;
4244
import dev.cel.optimizer.optimizers.SubexpressionOptimizer;
@@ -125,6 +127,12 @@ public final class CelPolicyCompilerTool implements Callable<Integer> {
125127
description = "Enable inline variable definitions (e.g., '- var_name: expr') in the policy")
126128
private boolean simpleVariables = false;
127129

130+
@Option(
131+
names = {"--iteration_limit"},
132+
defaultValue = "1000",
133+
description = "Maximum iteration limit for composing and optimizing the policy")
134+
private int iterationLimit = 1000;
135+
128136
private static final CelOptions CEL_OPTIONS =
129137
CelOptions.current()
130138
.populateMacroCalls(true)
@@ -214,19 +222,29 @@ public Integer call() {
214222
}
215223

216224
try {
217-
CelPolicyCompilerBuilder policyCompilerBuilder =
218-
CelPolicyCompilerFactory.newPolicyCompiler(cel);
219-
225+
ImmutableList.Builder<CelAstOptimizer> optimizersBuilder =
226+
ImmutableList.<CelAstOptimizer>builder()
227+
.add(
228+
ConstantFoldingOptimizer.newInstance(
229+
ConstantFoldingOptions.newBuilder()
230+
.maxIterationLimit(iterationLimit)
231+
.build()),
232+
SubexpressionOptimizer.newInstance(
233+
SubexpressionOptimizerOptions.newBuilder()
234+
.iterationLimit(iterationLimit)
235+
.populateMacroCalls(true)
236+
.build()));
220237
if (optimizeFieldSelection) {
221-
policyCompilerBuilder.setOptimizers(
222-
ImmutableList.of(
223-
ConstantFoldingOptimizer.getInstance(),
224-
SubexpressionOptimizer.newInstance(
225-
SubexpressionOptimizerOptions.newBuilder().populateMacroCalls(true).build()),
226-
SelectOptimizer.newInstance(
227-
SelectOptimizerOptions.newBuilder().build(), transitiveFileDescriptors)));
238+
optimizersBuilder.add(
239+
SelectOptimizer.newInstance(
240+
SelectOptimizerOptions.newBuilder().build(), transitiveFileDescriptors));
228241
}
229242

243+
CelPolicyCompilerBuilder policyCompilerBuilder =
244+
CelPolicyCompilerFactory.newPolicyCompiler(cel)
245+
.setIterationLimit(iterationLimit)
246+
.setOptimizers(optimizersBuilder.build());
247+
230248
CelPolicyCompiler policyCompiler = policyCompilerBuilder.build();
231249
CelAbstractSyntaxTree ast = policyCompiler.compile(policy);
232250

‎policy/src/test/java/dev/cel/policy/tools/CelPolicyCompilerToolTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,34 @@ public void compile_withSimpleVariables_success() throws Exception {
299299
assertThat(celRuntime.createProgram(ast).eval()).isEqualTo(Optional.of(true));
300300
}
301301

302+
@Test
303+
public void compile_withIterationLimitReached_returnsError() throws Exception {
304+
String configPath = createFile("config.yaml", "name: test-env\n");
305+
String policyPath =
306+
createFile(
307+
"policy.yaml",
308+
"name: p\n"
309+
+ "rule:\n"
310+
+ " variables:\n"
311+
+ " - a: 1 + 2\n"
312+
+ " - b: variables.a + 3\n"
313+
+ " match:\n"
314+
+ " - condition: variables.b == 6\n"
315+
+ " output: 'true'\n");
316+
317+
String stdErr =
318+
executeExpectingError(
319+
"--policy",
320+
policyPath,
321+
"--config",
322+
configPath,
323+
"--simple_variables",
324+
"--iteration_limit",
325+
"1");
326+
327+
assertThat(stdErr).contains("Reason: Unexpected error while composing rules.");
328+
}
329+
302330
@Test
303331
public void compile_withOptimizeFieldSelection_rewritesSelectAndEvaluates() throws Exception {
304332
String configRlocation =

‎policy/tools/compile_cel_policy.bzl‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ def compile_cel_policy(
2727
output_version = "canonical",
2828
optimize_field_selection = False,
2929
simple_variables = False,
30+
iteration_limit = None,
3031
visibility = None):
3132
"""Compiles a CEL policy into a CheckedExpr binarypb or textpb with optional select optimization.
3233
@@ -45,12 +46,14 @@ def compile_cel_policy(
4546
output_format: (optional) str either "binarypb", "textpb", or "textproto" (default "binarypb")
4647
output_version: (optional) str either "canonical" or "v1alpha1" (default "canonical")
4748
optimize_field_selection: (optional) bool whether to enable AST select optimization (default False).
48-
When True, embeds field numbers, wire types, and default values directly into the compiled
49+
When True, embeds field numbers, field types, and default values directly into the compiled
4950
`CheckedExpr` AST. This enables version-skew and field-rename resilience (evaluating unknown
5051
fields from raw wire bytes on older clients) and faster field selections, at the cost of a
5152
larger serialized AST payload.
5253
simple_variables: (optional) bool whether to enable inline variable definitions (e.g.,
5354
`- var_name: expr` instead of `- name: var_name` / `expression: expr`) in the policy (default False)
55+
iteration_limit: (optional) int maximum iteration limit for composing and optimizing the policy
56+
(default None, which uses the compiler default of 1000)
5457
visibility: (optional) visibility to use on the genrule macro (default None)
5558
"""
5659
if output_format not in ("binarypb", "textpb", "textproto"):
@@ -97,6 +100,9 @@ def compile_cel_policy(
97100
if simple_variables:
98101
args.append("--simple_variables")
99102

103+
if iteration_limit != None:
104+
args.append("--iteration_limit=%d" % iteration_limit)
105+
100106
cmd = (
101107
"$(location //policy/tools:cel_policy_compiler_tool) " +
102108
" ".join(args)

0 commit comments

Comments
 (0)