Skip to content

Commit 9feb06f

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: semantic: aggregate match: - condition: "true" output: "'FOO'" - condition: "true" output: "'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: semantic: aggregate match: - rule: semantic: first_match match: - condition: "resource.is_admin == true" rule: semantic: aggregate match: - condition: "resource.has_standard_pii == true" output: "'GDPR_STANDARD'" - condition: "resource.is_b2c == true" output: "'EU_B2C_NOTICE'" - condition: "true" output: "'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: semantic: aggregate match: - rule: semantic: first_match match: - condition: "1 == 2" output: "'EU_NOTICE'" - condition: "true" output: "'ALWAYS'" # Output: ['ALWAYS'] ``` ```YAML name: explicit_optional_none_example rule: semantic: aggregate match: - rule: semantic: first_match match: - condition: "resource.is_b2c == true" output: "optional.none()" # Explicitly authored by user - condition: "true" output: "optional.of('ALWAYS')" # Output: [optional.none(), optional.of('ALWAYS')] ``` PiperOrigin-RevId: 913930387
1 parent 14d4c2e commit 9feb06f

16 files changed

Lines changed: 424 additions & 50 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)
@@ -154,7 +164,8 @@ static CelCompiledRule create(
154164
Optional<ValueString> ruleId,
155165
ImmutableList<CelCompiledVariable> variables,
156166
ImmutableList<CelCompiledMatch> matches,
157-
Cel cel) {
158-
return new AutoValue_CelCompiledRule(sourceId, ruleId, variables, matches, cel);
167+
Cel cel,
168+
CelPolicy.EvaluationSemantic semantic) {
169+
return new AutoValue_CelCompiledRule(sourceId, ruleId, variables, matches, cel, semantic);
159170
}
160171
}

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

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

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

4450
public abstract Optional<ValueString> description();
@@ -143,12 +149,15 @@ public abstract static class Rule {
143149

144150
public abstract ImmutableSet<Match> matches();
145151

152+
public abstract EvaluationSemantic semantic();
153+
146154
/** Builder for {@link Rule}. */
147155
public static Builder newBuilder(long id) {
148156
return new AutoValue_CelPolicy_Rule.Builder()
149157
.setId(id)
150158
.setVariables(ImmutableSet.of())
151-
.setMatches(ImmutableSet.of());
159+
.setMatches(ImmutableSet.of())
160+
.setSemantic(EvaluationSemantic.FIRST_MATCH);
152161
}
153162

154163
/** Creates a new builder to construct a {@link Rule} instance. */
@@ -195,6 +204,8 @@ public Builder addMatches(Iterable<Match> matches) {
195204

196205
abstract Rule.Builder setMatches(ImmutableSet<Match> matches);
197206

207+
public abstract Rule.Builder setSemantic(EvaluationSemantic semantic);
208+
198209
public abstract Rule build();
199210
}
200211
}

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

Lines changed: 23 additions & 2 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;
@@ -240,7 +241,12 @@ private CelCompiledRule compileRuleImpl(
240241

241242
CelCompiledRule compiledRule =
242243
CelCompiledRule.create(
243-
rule.id(), rule.ruleId(), variableBuilder.build(), matchBuilder.build(), ruleCel);
244+
rule.id(),
245+
rule.ruleId(),
246+
variableBuilder.build(),
247+
matchBuilder.build(),
248+
ruleCel,
249+
rule.semantic());
244250

245251
// Validate that all branches in the policy are reachable
246252
checkUnreachableCode(compiledRule, compilerContext);
@@ -256,7 +262,16 @@ private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext
256262
CelCompiledMatch compiledMatch = compiledMatches.get(i);
257263
boolean isTriviallyTrue = compiledMatch.isConditionTriviallyTrue();
258264

259-
if (isTriviallyTrue && !ruleHasOptional && i != matchCount - 1) {
265+
// Flag literally false conditions as dead code regardless of semantic
266+
if (isConditionLiterallyFalse(compiledMatch.condition())) {
267+
compilerContext.addIssue(
268+
compiledMatch.sourceId(), CelIssue.formatError(1, 0, "Condition is always false"));
269+
}
270+
271+
if (compiledRule.semantic() == EvaluationSemantic.FIRST_MATCH
272+
&& isTriviallyTrue
273+
&& !ruleHasOptional
274+
&& i != matchCount - 1) {
260275
if (compiledMatch.result().kind().equals(Kind.OUTPUT)) {
261276
compilerContext.addIssue(
262277
compiledMatch.sourceId(),
@@ -270,6 +285,12 @@ private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext
270285
}
271286
}
272287

288+
private static boolean isConditionLiterallyFalse(CelAbstractSyntaxTree condition) {
289+
CelExpr celExpr = condition.getExpr();
290+
return celExpr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
291+
&& !celExpr.constant().booleanValue();
292+
}
293+
273294
private static CelAbstractSyntaxTree newErrorAst() {
274295
return CelAbstractSyntaxTree.newParsedAst(
275296
CelExpr.ofConstant(0, CelConstant.ofValue("*error*")), CelSource.newBuilder().build());

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import dev.cel.common.formats.YamlHelper.YamlNodeType;
2828
import dev.cel.common.formats.YamlParserContextImpl;
2929
import dev.cel.common.internal.CelCodePointArray;
30+
import dev.cel.policy.CelPolicy.EvaluationSemantic;
3031
import dev.cel.policy.CelPolicy.Import;
3132
import dev.cel.policy.CelPolicy.Match;
3233
import dev.cel.policy.CelPolicy.Match.Result;
@@ -223,6 +224,15 @@ public CelPolicy.Rule parseRule(
223224
case "match":
224225
ruleBuilder.addMatches(parseMatches(ctx, policyBuilder, value));
225226
break;
227+
case "semantic":
228+
long semanticId = ctx.collectMetadata(value);
229+
if (!assertYamlType(ctx, semanticId, value, YamlNodeType.STRING, YamlNodeType.TEXT)) {
230+
break;
231+
}
232+
String semanticStr = ((ScalarNode) value).getValue();
233+
ruleBuilder.setSemantic(parseSemantic(ctx, semanticId, semanticStr));
234+
break;
235+
226236
default:
227237
tagVisitor.visitRuleTag(ctx, tagId, fieldName, value, policyBuilder, ruleBuilder);
228238
break;
@@ -409,6 +419,21 @@ private Variable parseVariableObject(
409419
return builder.build();
410420
}
411421

422+
private static EvaluationSemantic parseSemantic(
423+
PolicyParserContext<Node> ctx, long semanticId, String semanticStr) {
424+
switch (semanticStr) {
425+
case "first_match":
426+
return EvaluationSemantic.FIRST_MATCH;
427+
case "aggregate":
428+
return EvaluationSemantic.AGGREGATE;
429+
default:
430+
ctx.reportError(semanticId, "Invalid semantic: " + semanticStr);
431+
// Just return FIRST_MATCH as a sentinel value. The end result is a compilation error
432+
// anyway.
433+
return EvaluationSemantic.FIRST_MATCH;
434+
}
435+
}
436+
412437
private ParserImpl(
413438
TagVisitor<Node> tagVisitor,
414439
boolean enableSimpleVariables,

0 commit comments

Comments
 (0)