Skip to content

Commit 5bbb537

Browse files
l46kokcopybara-github
authored andcommitted
Avoid concatenating superfluous empty list for aggregate semantics
Add more aggregate policy conformance test cases PiperOrigin-RevId: 966065339
1 parent 05bf69c commit 5bbb537

2 files changed

Lines changed: 66 additions & 4 deletions

File tree

policy/src/main/java/dev/cel/policy/RuleComposer.java

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,12 @@ private Step optimizeRule(Cel cel, CelCompiledRule compiledRule, boolean asList)
148148

149149
private @Nullable Step createBaseStep(boolean returnList, boolean hasOptionalOutput) {
150150
if (returnList) {
151-
// If the rule is evaluated as a list (AGGREGATE), the base case is an empty list.
152-
return Step.newUnconditionalNonOptionalStep(newTrueLiteral(), newList());
151+
if (hasOptionalOutput) {
152+
// If a nested rule inside an aggregate context has an optional output, the last result in
153+
// the ternary should return an empty list to allow concatenation with other branches.
154+
return Step.newUnconditionalNonOptionalStep(newTrueLiteral(), newList());
155+
}
156+
return null;
153157
}
154158

155159
if (hasOptionalOutput) {
@@ -280,6 +284,10 @@ private Step combineAggregate(AstMutator astMutator, Step currentStep, Step accu
280284
conditionalListPart = currentListPart;
281285
}
282286

287+
if (accumulatedStep == null) {
288+
return Step.newUnconditionalNonOptionalStep(trueCondition, conditionalListPart);
289+
}
290+
283291
CelMutableAst concatenated =
284292
astMutator.newGlobalCall(
285293
Operator.ADD.getFunction(), conditionalListPart, accumulatedStep.expr);

policy/src/test/java/dev/cel/policy/CelPolicyCompilerImplTest.java

Lines changed: 56 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,62 @@ public void compileYamlPolicy_aggregate_macrosPreserved() throws Exception {
261261
String unparsed = CelUnparserFactory.newUnparser().unparse(ast);
262262
assertThat(unparsed)
263263
.isEqualTo(
264-
"(cond ? [payload.filter(x, x > 10, x).exists(y, y % 2 == 0)] : []) "
265-
+ "+ ([payload.all(x, x > 0)] + [])");
264+
"(cond ? [payload.filter(x, x > 10).exists(y, y % 2 == 0)] : []) "
265+
+ "+ [payload.all(x, x > 0)]");
266+
}
267+
268+
@Test
269+
public void compileYamlPolicy_aggregateSingleMatch_noSuperfluousConcatenation() throws Exception {
270+
String policySource =
271+
"name: aggregate_single_match\n"
272+
+ "rule:\n"
273+
+ " aggregate:\n"
274+
+ " - condition: \"cond\"\n"
275+
+ " output: \"payload.filter(x, x > 10).exists(y, y % 2 == 0)\"\n";
276+
Cel cel =
277+
newCel()
278+
.toCelBuilder()
279+
.addVar("cond", SimpleType.BOOL)
280+
.addVar("payload", ListType.create(SimpleType.INT))
281+
.build();
282+
283+
CelPolicy policy = POLICY_PARSER.parse(policySource);
284+
285+
CelAbstractSyntaxTree ast =
286+
CelPolicyCompilerFactory.newPolicyCompiler(cel).build().compile(policy);
287+
288+
String unparsed = CelUnparserFactory.newUnparser().unparse(ast);
289+
assertThat(unparsed).isEqualTo("cond ? [payload.filter(x, x > 10).exists(y, y % 2 == 0)] : []");
290+
}
291+
292+
@Test
293+
public void compileYamlPolicy_aggregateMultipleConditionalMatches_noSuperfluousConcatenation()
294+
throws Exception {
295+
String policySource =
296+
"name: aggregate_multiple_conditional\n"
297+
+ "rule:\n"
298+
+ " aggregate:\n"
299+
+ " - condition: \"cond1\"\n"
300+
+ " output: \"payload.all(x, x > 0)\"\n"
301+
+ " - condition: \"cond2\"\n"
302+
+ " output: \"payload.exists(x, x == 0)\"\n";
303+
Cel cel =
304+
newCel()
305+
.toCelBuilder()
306+
.addVar("cond1", SimpleType.BOOL)
307+
.addVar("cond2", SimpleType.BOOL)
308+
.addVar("payload", ListType.create(SimpleType.INT))
309+
.build();
310+
311+
CelPolicy policy = POLICY_PARSER.parse(policySource);
312+
313+
CelAbstractSyntaxTree ast =
314+
CelPolicyCompilerFactory.newPolicyCompiler(cel).build().compile(policy);
315+
316+
String unparsed = CelUnparserFactory.newUnparser().unparse(ast);
317+
assertThat(unparsed)
318+
.isEqualTo(
319+
"(cond1 ? [payload.all(x, x > 0)] : []) + (cond2 ? [payload.exists(x, x == 0)] : [])");
266320
}
267321

268322
@Test

0 commit comments

Comments
 (0)