Skip to content

Commit 754b4f4

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Surface type-mismatch errors in a readable fashion during rule composition
PiperOrigin-RevId: 652537276
1 parent d20d377 commit 754b4f4

16 files changed

Lines changed: 316 additions & 61 deletions

File tree

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ java_library(
222222
name = "compiled_rule",
223223
srcs = ["CelCompiledRule.java"],
224224
deps = [
225+
":value_string",
225226
"//:auto_value",
226227
"//bundle:cel",
227228
"//common",
@@ -301,12 +302,14 @@ java_library(
301302
"//:auto_value",
302303
"//bundle:cel",
303304
"//common",
305+
"//common:compiler_common",
304306
"//common:mutable_ast",
305307
"//common/ast",
306308
"//extensions:optional_library",
307309
"//optimizer:ast_optimizer",
308310
"//optimizer:mutable_ast",
309311
"//parser:operator",
312+
"//policy:value_string",
310313
"@maven//:com_google_errorprone_error_prone_annotations",
311314
"@maven//:com_google_guava_guava",
312315
],

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

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,16 @@
2020
import dev.cel.bundle.Cel;
2121
import dev.cel.common.CelAbstractSyntaxTree;
2222
import dev.cel.common.CelVarDecl;
23+
import java.util.Optional;
2324

2425
/**
2526
* Abstract representation of a compiled rule. This contains set of compiled variables and match
2627
* statements which defines an expression graph for a policy.
2728
*/
2829
@AutoValue
2930
public abstract class CelCompiledRule {
31+
public abstract Optional<ValueString> id();
32+
3033
public abstract ImmutableList<CelCompiledVariable> variables();
3134

3235
public abstract ImmutableList<CelCompiledMatch> matches();
@@ -63,14 +66,15 @@ public abstract static class CelCompiledMatch {
6366
/** Encapsulates the result of this match when condition is met. (either an output or a rule) */
6467
@AutoOneOf(CelCompiledMatch.Result.Kind.class)
6568
public abstract static class Result {
66-
public abstract CelAbstractSyntaxTree output();
69+
public abstract OutputValue output();
6770

6871
public abstract CelCompiledRule rule();
6972

7073
public abstract Kind kind();
7174

72-
static Result ofOutput(CelAbstractSyntaxTree value) {
73-
return AutoOneOf_CelCompiledRule_CelCompiledMatch_Result.output(value);
75+
static Result ofOutput(long id, CelAbstractSyntaxTree ast) {
76+
return AutoOneOf_CelCompiledRule_CelCompiledMatch_Result.output(
77+
OutputValue.create(id, ast));
7478
}
7579

7680
static Result ofRule(CelCompiledRule value) {
@@ -84,16 +88,32 @@ public enum Kind {
8488
}
8589
}
8690

91+
/**
92+
* Encapsulates the output value of the match with its original ID that was used to compile
93+
* with.
94+
*/
95+
@AutoValue
96+
public abstract static class OutputValue {
97+
public abstract long id();
98+
99+
public abstract CelAbstractSyntaxTree ast();
100+
101+
public static OutputValue create(long id, CelAbstractSyntaxTree ast) {
102+
return new AutoValue_CelCompiledRule_CelCompiledMatch_OutputValue(id, ast);
103+
}
104+
}
105+
87106
static CelCompiledMatch create(
88107
CelAbstractSyntaxTree condition, CelCompiledMatch.Result result) {
89108
return new AutoValue_CelCompiledRule_CelCompiledMatch(condition, result);
90109
}
91110
}
92111

93112
static CelCompiledRule create(
113+
Optional<ValueString> id,
94114
ImmutableList<CelCompiledVariable> variables,
95115
ImmutableList<CelCompiledMatch> matches,
96116
Cel cel) {
97-
return new AutoValue_CelCompiledRule(variables, matches, cel);
117+
return new AutoValue_CelCompiledRule(id, variables, matches, cel);
98118
}
99119
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public interface CelPolicyCompiler {
2626
* CEL environment.
2727
*/
2828
default CelAbstractSyntaxTree compile(CelPolicy policy) throws CelPolicyValidationException {
29-
return compose(compileRule(policy));
29+
return compose(policy, compileRule(policy));
3030
}
3131

3232
/**
@@ -40,5 +40,6 @@ default CelAbstractSyntaxTree compile(CelPolicy policy) throws CelPolicyValidati
4040
* Composes {@link CelCompiledRule}, representing an expression graph, into a single expression
4141
* value.
4242
*/
43-
CelAbstractSyntaxTree compose(CelCompiledRule compiledRule) throws CelPolicyValidationException;
43+
CelAbstractSyntaxTree compose(CelPolicy policy, CelCompiledRule compiledRule)
44+
throws CelPolicyValidationException;
4445
}

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

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package dev.cel.policy;
1616

1717
import static com.google.common.base.Preconditions.checkNotNull;
18+
import static com.google.common.collect.ImmutableList.toImmutableList;
1819

1920
import com.google.common.collect.ImmutableList;
2021
import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -37,7 +38,9 @@
3738
import dev.cel.policy.CelCompiledRule.CelCompiledVariable;
3839
import dev.cel.policy.CelPolicy.Match;
3940
import dev.cel.policy.CelPolicy.Variable;
41+
import dev.cel.policy.RuleComposer.RuleCompositionException;
4042
import java.util.ArrayList;
43+
import java.util.Arrays;
4144
import java.util.List;
4245
import java.util.Optional;
4346

@@ -61,7 +64,7 @@ public CelCompiledRule compileRule(CelPolicy policy) throws CelPolicyValidationE
6164
}
6265

6366
@Override
64-
public CelAbstractSyntaxTree compose(CelCompiledRule compiledRule)
67+
public CelAbstractSyntaxTree compose(CelPolicy policy, CelCompiledRule compiledRule)
6568
throws CelPolicyValidationException {
6669
CelOptimizer optimizer =
6770
CelOptimizerFactory.standardCelOptimizerBuilder(compiledRule.cel())
@@ -77,8 +80,28 @@ public CelAbstractSyntaxTree compose(CelCompiledRule compiledRule)
7780
ast = cel.compile("true").getAst();
7881
ast = optimizer.optimize(ast);
7982
} catch (CelValidationException | CelOptimizationException e) {
80-
// TODO: Surface these errors better
81-
throw new CelPolicyValidationException("Failed composing the rules", e);
83+
if (e.getCause() instanceof RuleCompositionException) {
84+
RuleCompositionException re = (RuleCompositionException) e.getCause();
85+
CompilerContext compilerContext = new CompilerContext(policy.policySource());
86+
// The exact CEL error message produced from composition failure isn't too useful for users.
87+
// Ex: ERROR: :1:1: found no matching overload for '_?_:_' applied to '(bool, map(int, int),
88+
// bool)' (candidates: (bool, %A0, %A0))
89+
// Transform the error messages in a user-friendly way while retaining the original
90+
// CelValidationException as its originating cause.
91+
92+
ImmutableList<CelIssue> transformedIssues =
93+
re.compileException.getErrors().stream()
94+
.map(x -> CelIssue.formatError(x.getSourceLocation(), re.failureReason))
95+
.collect(toImmutableList());
96+
for (long id : re.errorIds) {
97+
compilerContext.addIssue(id, transformedIssues);
98+
}
99+
100+
throw new CelPolicyValidationException(compilerContext.getIssueString(), re.getCause());
101+
}
102+
103+
// Something has gone seriously wrong.
104+
throw new CelPolicyValidationException("Unexpected error while composing rules.", e);
82105
}
83106

84107
return ast;
@@ -111,6 +134,11 @@ private CelCompiledRule compileRuleImpl(
111134
CelAbstractSyntaxTree conditionAst;
112135
try {
113136
conditionAst = ruleCel.compile(match.condition().value()).getAst();
137+
if (!conditionAst.getResultType().equals(SimpleType.BOOL)) {
138+
compilerContext.addIssue(
139+
match.condition().id(),
140+
CelIssue.formatError(1, 0, "condition must produce a boolean output."));
141+
}
114142
} catch (CelValidationException e) {
115143
compilerContext.addIssue(match.condition().id(), e.getErrors());
116144
continue;
@@ -120,14 +148,15 @@ private CelCompiledRule compileRuleImpl(
120148
switch (match.result().kind()) {
121149
case OUTPUT:
122150
CelAbstractSyntaxTree outputAst;
151+
ValueString output = match.result().output();
123152
try {
124-
outputAst = ruleCel.compile(match.result().output().value()).getAst();
153+
outputAst = ruleCel.compile(output.value()).getAst();
125154
} catch (CelValidationException e) {
126-
compilerContext.addIssue(match.result().output().id(), e.getErrors());
155+
compilerContext.addIssue(output.id(), e.getErrors());
127156
continue;
128157
}
129158

130-
matchResult = Result.ofOutput(outputAst);
159+
matchResult = Result.ofOutput(output.id(), outputAst);
131160
break;
132161
case RULE:
133162
CelCompiledRule nestedRule =
@@ -141,7 +170,7 @@ private CelCompiledRule compileRuleImpl(
141170
matchBuilder.add(CelCompiledMatch.create(conditionAst, matchResult));
142171
}
143172

144-
return CelCompiledRule.create(variableBuilder.build(), matchBuilder.build(), cel);
173+
return CelCompiledRule.create(rule.id(), variableBuilder.build(), matchBuilder.build(), cel);
145174
}
146175

147176
private static CelAbstractSyntaxTree newErrorAst() {
@@ -153,6 +182,10 @@ private static final class CompilerContext {
153182
private final ArrayList<CelIssue> issues;
154183
private final CelPolicySource celPolicySource;
155184

185+
private void addIssue(long id, CelIssue... issues) {
186+
addIssue(id, Arrays.asList(issues));
187+
}
188+
156189
private void addIssue(long id, List<CelIssue> issues) {
157190
for (CelIssue issue : issues) {
158191
CelSourceLocation absoluteLocation = computeAbsoluteLocation(id, issue);
@@ -163,6 +196,9 @@ private void addIssue(long id, List<CelIssue> issues) {
163196
private CelSourceLocation computeAbsoluteLocation(long id, CelIssue issue) {
164197
int policySourceOffset =
165198
Optional.ofNullable(celPolicySource.getPositionsMap().get(id)).orElse(-1);
199+
if (policySourceOffset == -1) {
200+
return CelSourceLocation.NONE;
201+
}
166202
CelSourceLocation policySourceLocation =
167203
celPolicySource.getOffsetLocation(policySourceOffset).orElse(null);
168204
if (policySourceLocation == null) {

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

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,25 @@
1616

1717
import static com.google.common.base.Preconditions.checkNotNull;
1818
import static com.google.common.collect.ImmutableList.toImmutableList;
19+
import static java.util.stream.Collectors.toCollection;
1920

2021
import com.google.auto.value.AutoValue;
21-
import com.google.common.collect.ImmutableList;
2222
import com.google.common.collect.Lists;
2323
import dev.cel.bundle.Cel;
2424
import dev.cel.common.CelAbstractSyntaxTree;
2525
import dev.cel.common.CelMutableAst;
26+
import dev.cel.common.CelValidationException;
2627
import dev.cel.common.ast.CelConstant.Kind;
2728
import dev.cel.extensions.CelOptionalLibrary.Function;
2829
import dev.cel.optimizer.AstMutator;
2930
import dev.cel.optimizer.CelAstOptimizer;
3031
import dev.cel.parser.Operator;
3132
import dev.cel.policy.CelCompiledRule.CelCompiledMatch;
33+
import dev.cel.policy.CelCompiledRule.CelCompiledMatch.OutputValue;
3234
import dev.cel.policy.CelCompiledRule.CelCompiledVariable;
35+
import java.util.ArrayList;
36+
import java.util.Arrays;
37+
import java.util.List;
3338

3439
/** Package-private class for composing various rules into a single expression using optimizer. */
3540
final class RuleComposer implements CelAstOptimizer {
@@ -39,13 +44,8 @@ final class RuleComposer implements CelAstOptimizer {
3944

4045
@Override
4146
public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) {
42-
RuleOptimizationResult result = optimizeRule(compiledRule);
43-
return OptimizationResult.create(
44-
result.ast().toParsedAst(),
45-
compiledRule.variables().stream()
46-
.map(CelCompiledVariable::celVarDecl)
47-
.collect(toImmutableList()),
48-
ImmutableList.of());
47+
RuleOptimizationResult result = optimizeRule(cel, compiledRule);
48+
return OptimizationResult.create(result.ast().toParsedAst());
4949
}
5050

5151
@AutoValue
@@ -59,20 +59,33 @@ static RuleOptimizationResult create(CelMutableAst ast, boolean isOptionalResult
5959
}
6060
}
6161

62-
private RuleOptimizationResult optimizeRule(CelCompiledRule compiledRule) {
62+
private RuleOptimizationResult optimizeRule(Cel cel, CelCompiledRule compiledRule) {
63+
cel =
64+
cel.toCelBuilder()
65+
.addVarDeclarations(
66+
compiledRule.variables().stream()
67+
.map(CelCompiledVariable::celVarDecl)
68+
.collect(toImmutableList()))
69+
.build();
70+
6371
CelMutableAst matchAst = astMutator.newGlobalCall(Function.OPTIONAL_NONE.getFunction());
6472
boolean isOptionalResult = true;
73+
// Keep track of the last output ID that might cause type-check failure while attempting to
74+
// compose the subgraphs.
75+
long lastOutputId = 0;
6576
for (CelCompiledMatch match : Lists.reverse(compiledRule.matches())) {
6677
CelAbstractSyntaxTree conditionAst = match.condition();
6778
boolean isTriviallyTrue =
6879
conditionAst.getExpr().constantOrDefault().getKind().equals(Kind.BOOLEAN_VALUE)
6980
&& conditionAst.getExpr().constant().booleanValue();
7081
switch (match.result().kind()) {
7182
case OUTPUT:
72-
CelMutableAst outAst = CelMutableAst.fromCelAst(match.result().output());
83+
OutputValue matchOutput = match.result().output();
84+
CelMutableAst outAst = CelMutableAst.fromCelAst(matchOutput.ast());
7385
if (isTriviallyTrue) {
7486
matchAst = outAst;
7587
isOptionalResult = false;
88+
lastOutputId = matchOutput.id();
7689
continue;
7790
}
7891
if (isOptionalResult) {
@@ -85,9 +98,13 @@ private RuleOptimizationResult optimizeRule(CelCompiledRule compiledRule) {
8598
CelMutableAst.fromCelAst(conditionAst),
8699
outAst,
87100
matchAst);
101+
assertComposedAstIsValid(
102+
cel, matchAst, "conflicting output types found.", matchOutput.id(), lastOutputId);
103+
lastOutputId = matchOutput.id();
88104
continue;
89105
case RULE:
90-
RuleOptimizationResult nestedRule = optimizeRule(match.result().rule());
106+
CelCompiledRule matchNestedRule = match.result().rule();
107+
RuleOptimizationResult nestedRule = optimizeRule(cel, matchNestedRule);
91108
CelMutableAst nestedRuleAst = nestedRule.ast();
92109
if (isOptionalResult && !nestedRule.isOptionalResult()) {
93110
nestedRuleAst =
@@ -101,6 +118,13 @@ private RuleOptimizationResult optimizeRule(CelCompiledRule compiledRule) {
101118
throw new IllegalArgumentException("Subrule early terminates policy");
102119
}
103120
matchAst = astMutator.newMemberCall(nestedRuleAst, Function.OR.getFunction(), matchAst);
121+
assertComposedAstIsValid(
122+
cel,
123+
matchAst,
124+
String.format(
125+
"failed composing the subrule '%s' due to conflicting output types.",
126+
matchNestedRule.id().map(ValueString::value).orElse("")),
127+
lastOutputId);
104128
break;
105129
}
106130
}
@@ -127,9 +151,38 @@ static RuleComposer newInstance(
127151
return new RuleComposer(compiledRule, variablePrefix, iterationLimit);
128152
}
129153

154+
private void assertComposedAstIsValid(
155+
Cel cel, CelMutableAst composedAst, String failureMessage, Long... ids) {
156+
assertComposedAstIsValid(cel, composedAst, failureMessage, Arrays.asList(ids));
157+
}
158+
159+
private void assertComposedAstIsValid(
160+
Cel cel, CelMutableAst composedAst, String failureMessage, List<Long> ids) {
161+
try {
162+
cel.check(composedAst.toParsedAst()).getAst();
163+
} catch (CelValidationException e) {
164+
ids = ids.stream().filter(id -> id > 0).collect(toCollection(ArrayList::new));
165+
throw new RuleCompositionException(failureMessage, e, ids);
166+
}
167+
}
168+
130169
private RuleComposer(CelCompiledRule compiledRule, String variablePrefix, int iterationLimit) {
131170
this.compiledRule = checkNotNull(compiledRule);
132171
this.variablePrefix = variablePrefix;
133172
this.astMutator = AstMutator.newInstance(iterationLimit);
134173
}
174+
175+
static final class RuleCompositionException extends RuntimeException {
176+
final String failureReason;
177+
final List<Long> errorIds;
178+
final CelValidationException compileException;
179+
180+
private RuleCompositionException(
181+
String failureReason, CelValidationException e, List<Long> errorIds) {
182+
super(e);
183+
this.failureReason = failureReason;
184+
this.errorIds = errorIds;
185+
this.compileException = e;
186+
}
187+
}
135188
}

0 commit comments

Comments
 (0)