Skip to content

Commit ad993ac

Browse files
l46kokcopybara-github
authored andcommitted
Add aggregate semantics to Policy Compiler
Aggregate walks through all matching rules (including nested ones) and appends them into a list: ```yaml rule: aggregate: - condition: "true" emit: "'FOO'" - condition: "true" emit: "'BAR'" # Output: ['FOO', 'BAR'] ``` Few noteworthy design decisions below. All examples assume all conditions matched: 1. For usability reasons, subrules under an aggregate ancestor will always have their **lists flattened**: ```YAML name: aggregate_flat_flattening_example rule: aggregate: - rule: match: - condition: "resource.is_admin == true" output: "['GDPR_STANDARD', 'EU_B2C_NOTICE']" - condition: "true" emit: "'FALLBACK'" # Output: ['GDPR_STANDARD', 'EU_B2C_NOTICE', 'FALLBACK'] ``` 2. Base case of an aggregate rule is an empty list. Nested conditional rules within an aggregate rule which outputs `optional.none()` are pruned (except in cases where policy output explicitly emits an `optional.none()`): ```YAML name: optional_pruning_example rule: aggregate: - rule: match: - condition: "1 == 2" output: "'EU_NOTICE'" - condition: "true" emit: "'ALWAYS'" # Output: ['ALWAYS'] ``` ```YAML name: explicit_optional_none_example rule: aggregate: - rule: match: - condition: "resource.is_b2c == true" output: "optional.none()" # Explicitly authored by user - condition: "true" emit: "optional.of('ALWAYS')" # Output: [optional.none(), optional.of('ALWAYS')] Note: nesting aggregate is currently not allowed. ``` PiperOrigin-RevId: 913930387
1 parent e947ca5 commit ad993ac

14 files changed

Lines changed: 466 additions & 63 deletions

File tree

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -664,8 +664,8 @@ private CelMutableSource mangleIdentsInMacroSource(
664664
return newSource;
665665
}
666666

667-
private static CelMutableSource combine(
668-
CelMutableSource celSource1, CelMutableSource celSource2) {
667+
/** Combines two {@link CelMutableSource} instances into a single new instance. */
668+
public static CelMutableSource combine(CelMutableSource celSource1, CelMutableSource celSource2) {
669669
return CelMutableSource.newInstance()
670670
.setDescription(
671671
Strings.isNullOrEmpty(celSource1.getDescription())
@@ -677,6 +677,7 @@ private static CelMutableSource combine(
677677
.addAllMacroCalls(celSource2.getMacroCalls());
678678
}
679679

680+
680681
/**
681682
* Stabilizes the incoming AST by ensuring that all of expr IDs are consistently renumbered
682683
* (monotonically increased) from the starting seed ID. If the AST contains any macro calls, its

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,7 @@ java_library(
179179
name = "compiled_rule",
180180
srcs = ["CelCompiledRule.java"],
181181
deps = [
182+
":policy",
182183
"//:auto_value",
183184
"//bundle:cel",
184185
"//common:cel_ast",
@@ -246,6 +247,7 @@ java_library(
246247
srcs = ["RuleComposer.java"],
247248
deps = [
248249
":compiled_rule",
250+
":policy",
249251
"//bundle:cel",
250252
"//common:cel_ast",
251253
"//common:compiler_common",
@@ -256,11 +258,13 @@ java_library(
256258
"//common/ast:mutable_expr",
257259
"//common/formats:value_string",
258260
"//common/navigation:mutable_navigation",
261+
"//common/types",
259262
"//common/types:cel_types",
260263
"//common/types:type_providers",
261264
"//extensions:optional_library",
262265
"//optimizer:ast_optimizer",
263266
"//optimizer:mutable_ast",
264267
"@maven//:com_google_guava_guava",
268+
"@maven//:org_jspecify_jspecify",
265269
],
266270
)

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import dev.cel.common.ast.CelConstant;
2424
import dev.cel.common.ast.CelExpr;
2525
import dev.cel.common.formats.ValueString;
26+
import dev.cel.policy.CelPolicy.EvaluationSemantic;
2627
import java.util.Optional;
2728

2829
/**
@@ -43,11 +44,20 @@ public abstract class CelCompiledRule {
4344

4445
public abstract Cel cel();
4546

47+
public abstract EvaluationSemantic semantic();
48+
4649
/**
4750
* HasOptionalOutput returns whether the rule returns a concrete or optional value. The rule may
4851
* return an optional value if all match expressions under the rule are conditional.
4952
*/
5053
public boolean hasOptionalOutput() {
54+
// AGGREGATE rules always return a concrete list (falling back to an empty list rather than
55+
// optional.none()), meaning they are never optional structurally. This also prevents dead
56+
// code evasion inside parent FIRST_MATCH rules.
57+
if (semantic() == EvaluationSemantic.AGGREGATE) {
58+
return false;
59+
}
60+
5161
boolean isOptionalOutput = false;
5262
for (CelCompiledMatch match : matches()) {
5363
if (match.result().kind().equals(CelCompiledMatch.Result.Kind.RULE)
@@ -157,7 +167,8 @@ static CelCompiledRule create(
157167
Optional<ValueString> ruleId,
158168
ImmutableList<CelCompiledVariable> variables,
159169
ImmutableList<CelCompiledMatch> matches,
160-
Cel cel) {
161-
return new AutoValue_CelCompiledRule(sourceId, ruleId, variables, matches, cel);
170+
Cel cel,
171+
CelPolicy.EvaluationSemantic semantic) {
172+
return new AutoValue_CelCompiledRule(sourceId, ruleId, variables, matches, cel, semantic);
162173
}
163174
}

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,12 @@
4040
@AutoValue
4141
public abstract class CelPolicy {
4242

43+
/** Evaluation semantic for a rule. */
44+
public enum EvaluationSemantic {
45+
FIRST_MATCH,
46+
AGGREGATE
47+
}
48+
4349
public abstract ValueString name();
4450

4551
public abstract Optional<ValueString> description();
@@ -176,12 +182,15 @@ public abstract static class Rule {
176182

177183
public abstract ImmutableSet<Match> matches();
178184

185+
public abstract EvaluationSemantic semantic();
186+
179187
/** Builder for {@link Rule}. */
180188
public static Builder newBuilder(long id) {
181189
return new AutoValue_CelPolicy_Rule.Builder()
182190
.setId(id)
183191
.setVariables(ImmutableSet.of())
184-
.setMatches(ImmutableSet.of());
192+
.setMatches(ImmutableSet.of())
193+
.setSemantic(EvaluationSemantic.FIRST_MATCH);
185194
}
186195

187196
/** Creates a new builder to construct a {@link Rule} instance. */
@@ -228,6 +237,8 @@ public Builder addMatches(Iterable<Match> matches) {
228237

229238
abstract Rule.Builder setMatches(ImmutableSet<Match> matches);
230239

240+
public abstract Rule.Builder setSemantic(EvaluationSemantic semantic);
241+
231242
public abstract Rule build();
232243
}
233244
}

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

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import dev.cel.policy.CelCompiledRule.CelCompiledMatch.Result;
4545
import dev.cel.policy.CelCompiledRule.CelCompiledMatch.Result.Kind;
4646
import dev.cel.policy.CelCompiledRule.CelCompiledVariable;
47+
import dev.cel.policy.CelPolicy.EvaluationSemantic;
4748
import dev.cel.policy.CelPolicy.Import;
4849
import dev.cel.policy.CelPolicy.Match;
4950
import dev.cel.policy.CelPolicy.Variable;
@@ -91,7 +92,8 @@ public CelCompiledRule compileRule(CelPolicy policy) throws CelPolicyValidationE
9192
extendedCel = extendedCel.toCelBuilder().setContainer(containerBuilder.build()).build();
9293
}
9394

94-
CelCompiledRule compiledRule = compileRuleImpl(policy.rule(), extendedCel, compilerContext);
95+
CelCompiledRule compiledRule =
96+
compileRuleImpl(policy.rule(), extendedCel, compilerContext, false);
9597
if (compilerContext.hasError()) {
9698
throw new CelPolicyValidationException(compilerContext.getIssueString());
9799
}
@@ -172,7 +174,14 @@ private void assertAstDepthIsSafe(CelAbstractSyntaxTree ast, Cel cel)
172174
}
173175

174176
private CelCompiledRule compileRuleImpl(
175-
CelPolicy.Rule rule, Cel ruleCel, CompilerContext compilerContext) {
177+
CelPolicy.Rule rule,
178+
Cel ruleCel,
179+
CompilerContext compilerContext,
180+
boolean hasAggregateAncestor) {
181+
if (hasAggregateAncestor && rule.semantic().equals(EvaluationSemantic.AGGREGATE)) {
182+
compilerContext.addIssue(
183+
rule.id(), CelIssue.formatError(1, 0, "nested aggregate rules are not allowed"));
184+
}
176185
// A local CEL environment used to compile a single rule. This temporary environment
177186
// is used to declare policy variables iteratively in a given policy, ensuring proper scoping
178187
// across a single / nested rule.
@@ -227,8 +236,11 @@ private CelCompiledRule compileRuleImpl(
227236
matchResult = Result.ofOutput(output.id(), outputAst);
228237
break;
229238
case RULE:
239+
boolean nextHasAggregateAncestor =
240+
hasAggregateAncestor || rule.semantic().equals(EvaluationSemantic.AGGREGATE);
230241
CelCompiledRule nestedRule =
231-
compileRuleImpl(match.result().rule(), localCel, compilerContext);
242+
compileRuleImpl(
243+
match.result().rule(), localCel, compilerContext, nextHasAggregateAncestor);
232244
matchResult = Result.ofRule(nestedRule);
233245
break;
234246
default:
@@ -240,7 +252,12 @@ private CelCompiledRule compileRuleImpl(
240252

241253
CelCompiledRule compiledRule =
242254
CelCompiledRule.create(
243-
rule.id(), rule.ruleId(), variableBuilder.build(), matchBuilder.build(), ruleCel);
255+
rule.id(),
256+
rule.ruleId(),
257+
variableBuilder.build(),
258+
matchBuilder.build(),
259+
ruleCel,
260+
rule.semantic());
244261

245262
// Validate that all branches in the policy are reachable
246263
checkUnreachableCode(compiledRule, compilerContext);
@@ -255,6 +272,12 @@ private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext
255272
CelCompiledMatch compiledMatch = compiledMatches.get(i);
256273
boolean isTriviallyTrue = compiledMatch.isConditionTriviallyTrue();
257274

275+
// Flag literally false conditions as dead code regardless of semantic
276+
if (isConditionLiterallyFalse(compiledMatch.condition())) {
277+
compilerContext.addIssue(
278+
compiledMatch.sourceId(), CelIssue.formatError(1, 0, "Condition is always false"));
279+
}
280+
258281
// If the match is a single output or a nested rule that always returns a value, it is
259282
// exhaustive. If the condition is trivially true, then all subsequent branches are
260283
// unreachable.
@@ -263,7 +286,9 @@ private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext
263286
&& (compiledMatch.result().kind().equals(Kind.OUTPUT)
264287
|| !compiledMatch.result().rule().hasOptionalOutput());
265288

266-
if (isExhaustive && i != matchCount - 1) {
289+
if (compiledRule.semantic() == EvaluationSemantic.FIRST_MATCH
290+
&& isExhaustive
291+
&& i != matchCount - 1) {
267292
if (compiledMatch.result().kind().equals(Kind.OUTPUT)) {
268293
compilerContext.addIssue(
269294
compiledMatch.sourceId(),
@@ -277,6 +302,12 @@ private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext
277302
}
278303
}
279304

305+
private static boolean isConditionLiterallyFalse(CelAbstractSyntaxTree condition) {
306+
CelExpr celExpr = condition.getExpr();
307+
return celExpr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
308+
&& !celExpr.constant().booleanValue();
309+
}
310+
280311
private static CelAbstractSyntaxTree newErrorAst() {
281312
return CelAbstractSyntaxTree.newParsedAst(
282313
CelExpr.ofConstant(0, CelConstant.ofValue("*error*")), CelSource.newBuilder().build());

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

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import dev.cel.common.formats.YamlHelper.YamlNodeType;
2929
import dev.cel.common.formats.YamlParserContextImpl;
3030
import dev.cel.common.internal.CelCodePointArray;
31+
import dev.cel.policy.CelPolicy.EvaluationSemantic;
3132
import dev.cel.policy.CelPolicy.Import;
3233
import dev.cel.policy.CelPolicy.Invariant;
3334
import dev.cel.policy.CelPolicy.Match;
@@ -271,6 +272,8 @@ public CelPolicy.Rule parseRule(
271272
return ruleBuilder.build();
272273
}
273274

275+
boolean hasMatch = false;
276+
boolean hasAggregate = false;
274277
for (NodeTuple nodeTuple : ((MappingNode) node).getValue()) {
275278
Node key = nodeTuple.getKeyNode();
276279
long tagId = ctx.collectMetadata(key);
@@ -290,8 +293,24 @@ public CelPolicy.Rule parseRule(
290293
ruleBuilder.addVariables(parseVariables(ctx, policyBuilder, value));
291294
break;
292295
case "match":
293-
ruleBuilder.addMatches(parseMatches(ctx, policyBuilder, value));
296+
if (hasAggregate) {
297+
ctx.reportError(tagId, "Only one of 'match' or 'aggregate' may be set in a rule");
298+
}
299+
hasMatch = true;
300+
ruleBuilder
301+
.addMatches(parseMatches(ctx, policyBuilder, value, false))
302+
.setSemantic(EvaluationSemantic.FIRST_MATCH);
303+
break;
304+
case "aggregate":
305+
if (hasMatch) {
306+
ctx.reportError(tagId, "Only one of 'match' or 'aggregate' may be set in a rule");
307+
}
308+
hasAggregate = true;
309+
ruleBuilder
310+
.addMatches(parseMatches(ctx, policyBuilder, value, true))
311+
.setSemantic(EvaluationSemantic.AGGREGATE);
294312
break;
313+
295314
default:
296315
tagVisitor.visitRuleTag(ctx, tagId, fieldName, value, policyBuilder, ruleBuilder);
297316
break;
@@ -301,7 +320,10 @@ public CelPolicy.Rule parseRule(
301320
}
302321

303322
private ImmutableSet<CelPolicy.Match> parseMatches(
304-
PolicyParserContext<Node> ctx, CelPolicy.Builder policyBuilder, Node node) {
323+
PolicyParserContext<Node> ctx,
324+
CelPolicy.Builder policyBuilder,
325+
Node node,
326+
boolean isAggregate) {
305327
long valueId = ctx.collectMetadata(node);
306328
ImmutableSet.Builder<CelPolicy.Match> matchesBuilder = ImmutableSet.builder();
307329
if (!assertYamlType(ctx, valueId, node, YamlNodeType.LIST)) {
@@ -310,7 +332,7 @@ private ImmutableSet<CelPolicy.Match> parseMatches(
310332

311333
SequenceNode matchListNode = (SequenceNode) node;
312334
for (Node elementNode : matchListNode.getValue()) {
313-
matchesBuilder.add(parseMatch(ctx, policyBuilder, elementNode));
335+
matchesBuilder.add(parseMatchInternal(ctx, policyBuilder, elementNode, isAggregate));
314336
}
315337

316338
return matchesBuilder.build();
@@ -319,6 +341,14 @@ private ImmutableSet<CelPolicy.Match> parseMatches(
319341
@Override
320342
public CelPolicy.Match parseMatch(
321343
PolicyParserContext<Node> ctx, CelPolicy.Builder policyBuilder, Node node) {
344+
return parseMatchInternal(ctx, policyBuilder, node, false);
345+
}
346+
347+
private CelPolicy.Match parseMatchInternal(
348+
PolicyParserContext<Node> ctx,
349+
CelPolicy.Builder policyBuilder,
350+
Node node,
351+
boolean isAggregate) {
322352
long nodeId = ctx.collectMetadata(node);
323353
if (!assertYamlType(ctx, nodeId, node, YamlNodeType.MAP)) {
324354
return ERROR_MATCH;
@@ -339,6 +369,20 @@ public CelPolicy.Match parseMatch(
339369
matchBuilder.setCondition(ctx.newSourceString(value));
340370
break;
341371
case "output":
372+
if (isAggregate) {
373+
ctx.reportError(tagId, "Rule aggregate requires 'emit' tag instead of 'output'");
374+
}
375+
matchBuilder
376+
.result()
377+
.filter(result -> result.kind().equals(Match.Result.Kind.RULE))
378+
.ifPresent(
379+
result -> ctx.reportError(tagId, "Only the rule or the output may be set"));
380+
matchBuilder.setResult(Match.Result.ofOutput(ctx.newSourceString(value)));
381+
break;
382+
case "emit":
383+
if (!isAggregate) {
384+
ctx.reportError(tagId, "Rule match requires 'output' tag instead of 'emit'");
385+
}
342386
matchBuilder
343387
.result()
344388
.filter(result -> result.kind().equals(Match.Result.Kind.RULE))

0 commit comments

Comments
 (0)