Skip to content

Commit 30fa67f

Browse files
l46kokcopybara-github
authored andcommitted
Fix typename imports to properly propagate environment changes when composing rules
PiperOrigin-RevId: 791045142
1 parent 8a1ad06 commit 30fa67f

4 files changed

Lines changed: 39 additions & 7 deletions

File tree

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

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,13 +158,17 @@ private void assertAstDepthIsSafe(CelAbstractSyntaxTree ast, Cel cel)
158158

159159
private CelCompiledRule compileRuleImpl(
160160
CelPolicy.Rule rule, Cel ruleCel, CompilerContext compilerContext) {
161+
// A local CEL environment used to compile a single rule. This temporary environment
162+
// is used to declare policy variables iteratively in a given policy, ensuring proper scoping
163+
// across a single / nested rule.
164+
Cel localCel = ruleCel;
161165
ImmutableList.Builder<CelCompiledVariable> variableBuilder = ImmutableList.builder();
162166
for (Variable variable : rule.variables()) {
163167
ValueString expression = variable.expression();
164168
CelAbstractSyntaxTree varAst;
165169
CelType outputType = SimpleType.DYN;
166170
try {
167-
varAst = ruleCel.compile(expression.value()).getAst();
171+
varAst = localCel.compile(expression.value()).getAst();
168172
outputType = varAst.getResultType();
169173
} catch (CelValidationException e) {
170174
compilerContext.addIssue(expression.id(), e.getErrors());
@@ -174,15 +178,15 @@ private CelCompiledRule compileRuleImpl(
174178
String variableName = variable.name().value();
175179
CelVarDecl newVariable =
176180
CelVarDecl.newVarDeclaration(variablesPrefix + variableName, outputType);
177-
ruleCel = ruleCel.toCelBuilder().addVarDeclarations(newVariable).build();
181+
localCel = localCel.toCelBuilder().addVarDeclarations(newVariable).build();
178182
variableBuilder.add(CelCompiledVariable.create(variableName, varAst, newVariable));
179183
}
180184

181185
ImmutableList.Builder<CelCompiledMatch> matchBuilder = ImmutableList.builder();
182186
for (Match match : rule.matches()) {
183187
CelAbstractSyntaxTree conditionAst;
184188
try {
185-
conditionAst = ruleCel.compile(match.condition().value()).getAst();
189+
conditionAst = localCel.compile(match.condition().value()).getAst();
186190
if (!conditionAst.getResultType().equals(SimpleType.BOOL)) {
187191
compilerContext.addIssue(
188192
match.condition().id(),
@@ -199,7 +203,7 @@ private CelCompiledRule compileRuleImpl(
199203
CelAbstractSyntaxTree outputAst;
200204
ValueString output = match.result().output();
201205
try {
202-
outputAst = ruleCel.compile(output.value()).getAst();
206+
outputAst = localCel.compile(output.value()).getAst();
203207
} catch (CelValidationException e) {
204208
compilerContext.addIssue(output.id(), e.getErrors());
205209
continue;
@@ -209,7 +213,7 @@ private CelCompiledRule compileRuleImpl(
209213
break;
210214
case RULE:
211215
CelCompiledRule nestedRule =
212-
compileRuleImpl(match.result().rule(), ruleCel, compilerContext);
216+
compileRuleImpl(match.result().rule(), localCel, compilerContext);
213217
matchResult = Result.ofRule(nestedRule);
214218
break;
215219
default:
@@ -221,7 +225,7 @@ private CelCompiledRule compileRuleImpl(
221225

222226
CelCompiledRule compiledRule =
223227
CelCompiledRule.create(
224-
rule.id(), rule.ruleId(), variableBuilder.build(), matchBuilder.build(), cel);
228+
rule.id(), rule.ruleId(), variableBuilder.build(), matchBuilder.build(), ruleCel);
225229

226230
// Validate that all branches in the policy are reachable
227231
checkUnreachableCode(compiledRule, compilerContext);

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ java_library(
2020
"//common/formats:value_string",
2121
"//common/internal",
2222
"//common/resources/testdata/proto3:standalone_global_enum_java_proto",
23+
"//common/types",
2324
"//compiler",
2425
"//extensions:optional_library",
2526
"//parser:macro",
@@ -35,6 +36,7 @@ java_library(
3536
"//runtime",
3637
"//runtime:function_binding",
3738
"//runtime:late_function_binding",
39+
"//testing/protos:single_file_java_proto",
3840
"@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto",
3941
"@maven//:com_google_guava_guava",
4042
"@maven//:com_google_testparameterinjector_test_parameter_injector",

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import dev.cel.bundle.CelFactory;
3232
import dev.cel.common.CelAbstractSyntaxTree;
3333
import dev.cel.common.CelOptions;
34+
import dev.cel.common.types.OptionalType;
3435
import dev.cel.expr.conformance.proto3.TestAllTypes;
3536
import dev.cel.extensions.CelOptionalLibrary;
3637
import dev.cel.parser.CelStandardMacro;
@@ -43,6 +44,7 @@
4344
import dev.cel.policy.PolicyTestHelper.TestYamlPolicy;
4445
import dev.cel.runtime.CelFunctionBinding;
4546
import dev.cel.runtime.CelLateFunctionBindings;
47+
import dev.cel.testing.testdata.SingleFileProto.SingleFile;
4648
import dev.cel.testing.testdata.proto3.StandaloneGlobalEnum;
4749
import java.io.IOException;
4850
import java.util.Map;
@@ -76,6 +78,28 @@ public void compileYamlPolicy_success(@TestParameter TestYamlPolicy yamlPolicy)
7678
assertThat(CelUnparserFactory.newUnparser().unparse(ast)).isEqualTo(yamlPolicy.getUnparsed());
7779
}
7880

81+
@Test
82+
public void compileYamlPolicy_withImportsOnNestedRules() throws Exception {
83+
String policySource =
84+
"imports:\n"
85+
+ " - name: cel.expr.conformance.proto3.TestAllTypes\n"
86+
+ " - name: dev.cel.testing.testdata.SingleFile\n"
87+
+ "rule:\n"
88+
+ " match:\n"
89+
+ " - rule:\n"
90+
+ " id: 'nested rule with imports'\n"
91+
+ " match:\n"
92+
+ " - condition: 'TestAllTypes{}.single_string == SingleFile{}.name'\n"
93+
+ " output: 'true'\n";
94+
Cel cel = newCel();
95+
CelPolicy policy = POLICY_PARSER.parse(policySource);
96+
97+
CelAbstractSyntaxTree ast =
98+
CelPolicyCompilerFactory.newPolicyCompiler(cel).build().compile(policy);
99+
100+
assertThat(ast.getResultType()).isInstanceOf(OptionalType.class);
101+
}
102+
79103
@Test
80104
public void compileYamlPolicy_containsCompilationError_throws(
81105
@TestParameter TestErrorYamlPolicy testCase) throws Exception {
@@ -292,7 +316,7 @@ private static Cel newCel() {
292316
.addCompilerLibraries(CelOptionalLibrary.INSTANCE)
293317
.addRuntimeLibraries(CelOptionalLibrary.INSTANCE)
294318
.addFileTypes(StandaloneGlobalEnum.getDescriptor().getFile())
295-
.addMessageTypes(TestAllTypes.getDescriptor())
319+
.addMessageTypes(TestAllTypes.getDescriptor(), SingleFile.getDescriptor())
296320
.setOptions(CEL_OPTIONS)
297321
.addFunctionBindings(
298322
CelFunctionBinding.from(

testing/src/test/resources/protos/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ proto_library(
2020

2121
java_proto_library(
2222
name = "single_file_java_proto",
23+
tags = [
24+
],
2325
deps = [":single_file_proto"],
2426
)
2527

0 commit comments

Comments
 (0)