From 9a7ab38f42890ec4943652014f55c4ed6bcc5e66 Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Tue, 6 Oct 2026 13:26:16 -0700 Subject: [PATCH] Remove resolveTypeDependencies CelOptions This option is always on by default. PiperOrigin-RevId: 994589891 --- .../test/java/dev/cel/bundle/CelImplTest.java | 162 ++---------------- .../dev/cel/checker/CelCheckerLegacyImpl.java | 1 - .../dev/cel/common/CelDescriptorUtil.java | 18 +- .../main/java/dev/cel/common/CelOptions.java | 14 -- .../types/ProtoMessageTypeProvider.java | 15 +- .../java/dev/cel/common/CelOptionsTest.java | 1 - .../types/ProtoMessageTypeProviderTest.java | 21 ++- .../java/dev/cel/runtime/CelRuntimeImpl.java | 1 - .../dev/cel/runtime/CelRuntimeLegacyImpl.java | 3 +- 9 files changed, 29 insertions(+), 207 deletions(-) diff --git a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java index e417d05ea..2cfe62ec3 100644 --- a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java +++ b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java @@ -826,68 +826,15 @@ public void program_hermeticDescriptors_wellKnownProtobuf() throws Exception { assertThat(program.eval()).isEqualTo(Instant.ofEpochSecond(12)); } - @Test - public void program_partialMessageTypes() throws Exception { - String packageName = CheckedExpr.getDescriptor().getFile().getPackage(); - Cel cel = - plannerCelBuilderWithMacros() - .addFileTypes(CheckedExpr.getDescriptor().getFile()) - // Disabling the resolution of type dependencies can be risky as message types which - // are expected to be available in an imported file may not be present if the type - // is not referenced in a field within the provided file descriptors. - // - // In this test 'Expr' is defined in syntax.proto, but the descriptor provided is - // defined in checked.proto. Because the `Expr` type is referenced within a message - // field of the CheckedExpr, it is available for use. - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(false) - .build()) - .setContainer(CelContainer.ofName(packageName)) - .setResultType(StructTypeReference.create(packageName + ".Expr")) - .build(); - CelRuntime.Program program = cel.createProgram(cel.compile("Expr{}").getAst()); - assertThat(program.eval()).isEqualTo(Expr.getDefaultInstance()); - } - - @Test - public void program_partialMessageTypeFailure() { - String packageName = CheckedExpr.getDescriptor().getFile().getPackage(); - Cel cel = - plannerCelBuilderWithMacros() - .addFileTypes(CheckedExpr.getDescriptor().getFile()) - // In this test 'ParsedExpr' is defined in syntax.proto, but the descriptor provided is - // defined in checked.proto. Because the `ParsedExpr` type is not referenced, it is not - // available for use within CEL when deep type resolution is disabled. - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(false) - .build()) - .setContainer(CelContainer.ofName(packageName)) - .setResultType(StructTypeReference.create(packageName + ".ParsedExpr")) - .build(); - CelValidationException e = - Assert.assertThrows( - CelValidationException.class, () -> cel.compile("ParsedExpr{}").getAst()); - assertThat(e).hasMessageThat().contains("undeclared reference to 'ParsedExpr'"); - } - @Test public void program_deepTypeResolution() throws Exception { String packageName = CheckedExpr.getDescriptor().getFile().getPackage(); Cel cel = plannerCelBuilderWithMacros() .addFileTypes(CheckedExpr.getDescriptor().getFile()) - // In this test 'ParsedExpr' is defined in syntax.proto, but the descriptor provided is - // defined in checked.proto. Because deep type dependency resolution is enabled, the - // `ParsedExpr` may be used within CEL. - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(true) - .build()) + // In this test 'ParsedExpr' is defined in syntax.proto, while the descriptor provided + // is defined in checked.proto, which imports syntax.proto. + .setOptions(CelOptions.current().enableHeterogeneousNumericComparisons(true).build()) .setContainer(CelContainer.ofName(packageName)) .setResultType(StructTypeReference.create(packageName + ".ParsedExpr")) .build(); @@ -896,7 +843,7 @@ public void program_deepTypeResolution() throws Exception { } @Test - public void program_deepTypeResolutionEnabledForRuntime_success() throws Exception { + public void program_deepTypeResolutionForRuntime_success() throws Exception { String packageName = CheckedExpr.getDescriptor().getFile().getPackage(); CelCompiler celCompiler = CelCompilerFactory.standardCelCompilerBuilder() @@ -910,55 +857,16 @@ public void program_deepTypeResolutionEnabledForRuntime_success() throws Excepti CelRuntime celRuntime = CelRuntimeFactory.plannerRuntimeBuilder() .addFileTypes(CheckedExpr.getDescriptor().getFile()) - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(true) - .build()) + .setOptions(CelOptions.current().enableHeterogeneousNumericComparisons(true).build()) // CEL-Internal-2 .build(); CelRuntime.Program program = celRuntime.createProgram(ast); - // 'ParsedExpr' is defined in syntax.proto but the descriptor provided to the runtime is from - // 'checked.proto'. - // 'ParsedExpr' is transitively available for use because deep type resolution is enabled. + // 'ParsedExpr' is defined in syntax.proto while the descriptor provided to the runtime is from + // 'checked.proto', so 'ParsedExpr' is transitively resolved. assertThat(program.eval()).isEqualTo(ParsedExpr.getDefaultInstance()); } - @Test - public void program_deepTypeResolutionDisabledForRuntime_fails() throws Exception { - String packageName = CheckedExpr.getDescriptor().getFile().getPackage(); - CelCompiler celCompiler = - CelCompilerFactory.standardCelCompilerBuilder() - .addFileTypes(CheckedExpr.getDescriptor().getFile()) - .setOptions(CelOptions.current().resolveTypeDependencies(true).build()) - .setResultType(StructTypeReference.create(packageName + ".ParsedExpr")) - .setContainer(CelContainer.ofName(packageName)) - .build(); - - // 'ParsedExpr' is defined in syntax.proto but the descriptor provided is from 'checked.proto'. - // 'ParsedExpr' is transitively available for use because deep type resolution is enabled. - CelAbstractSyntaxTree ast = celCompiler.compile("ParsedExpr{}").getAst(); - - // TODO: Planner runtime ignores CelOptions.resolveTypeDependencies(false). - CelRuntime celRuntime = - CelRuntimeFactory.legacyCelRuntimeBuilder() - .addFileTypes(CheckedExpr.getDescriptor().getFile()) - .setOptions(CelOptions.current().resolveTypeDependencies(false).build()) - // CEL-Internal-2 - .build(); - CelRuntime.Program program = celRuntime.createProgram(ast); - - // In this case, linked types are disabled so the same descriptors - // provided to the CelCompiler must also be provided into the runtime. - // As deep type resolution is disabled, 'ParsedExpr' is not available for use in runtime so an - // error is thrown. - CelEvaluationException e = Assert.assertThrows(CelEvaluationException.class, program::eval); - assertThat(e) - .hasMessageThat() - .contains(String.format("cannot resolve '%s.ParsedExpr' as a message", packageName)); - } - @Test @SuppressWarnings("deprecation") // Test for existing deprecated method setTypeProvider public void program_typeProvider() throws Exception { @@ -1000,23 +908,16 @@ public void program_protoActivation() throws Exception { } @Test - public void program_enumTypeDirectResolution(@TestParameter boolean resolveTypeDependencies) - throws Exception { + public void program_enumTypeDirectResolution() throws Exception { Cel cel = plannerCelBuilderWithMacros() .addFileTypes(StandaloneGlobalEnum.getDescriptor().getFile()) - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(resolveTypeDependencies) - .build()) + .setOptions(CelOptions.current().enableHeterogeneousNumericComparisons(true).build()) .setContainer( CelContainer.ofName("dev.cel.testing.testdata.proto3.StandaloneGlobalEnum")) .setResultType(SimpleType.BOOL) .build(); - // Providing an enum proto file directly should not cause an error - // regardless of the resolveTypeDependencies settings StandaloneGlobalEnum testEnum = StandaloneGlobalEnum.SGAR; CelRuntime.Program program = cel.createProgram( @@ -1025,23 +926,16 @@ public void program_enumTypeDirectResolution(@TestParameter boolean resolveTypeD } @Test - public void program_enumTypeReferenceResolution(@TestParameter boolean resolveTypeDependencies) - throws Exception { + public void program_enumTypeReferenceResolution() throws Exception { Cel cel = plannerCelBuilderWithMacros() - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(resolveTypeDependencies) - .build()) + .setOptions(CelOptions.current().enableHeterogeneousNumericComparisons(true).build()) .addMessageTypes(Struct.getDescriptor()) .setResultType(StructTypeReference.create("google.protobuf.NullValue")) .setContainer(CelContainer.ofName("google.protobuf")) .build(); // `Value` is defined in `Struct` proto and NullValue is an enum within this `Value` struct. - // The following evaluation should work regardless of resolveTypeDependencies settings - // as the enum definition is found in the same `Struct` proto definition. CelRuntime.Program program = cel.createProgram(cel.compile("Value{null_value: NullValue.NULL_VALUE}").getAst()); assertThat(program.eval()).isEqualTo(NullValue.NULL_VALUE); @@ -1051,11 +945,7 @@ public void program_enumTypeReferenceResolution(@TestParameter boolean resolveTy public void program_enumTypeTransitiveResolution() throws Exception { Cel cel = plannerCelBuilderWithMacros() - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(true) - .build()) + .setOptions(CelOptions.current().enableHeterogeneousNumericComparisons(true).build()) .addMessageTypes(Proto2ExtensionScopedMessage.getDescriptor()) .setResultType(StructTypeReference.create("google.protobuf.NullValue")) .setContainer(CelContainer.ofName("google.protobuf")) @@ -1063,8 +953,7 @@ public void program_enumTypeTransitiveResolution() throws Exception { // 'Value' is a struct defined as a dependency of messages_proto2.proto and 'NullValue' is an // enum within this 'Value' struct. - // As deep type dependency is enabled, the following evaluation should work by as the - // 'NullValue' enum type is transitively discovered + // The following evaluation works as the 'NullValue' enum type is transitively discovered. CelRuntime.Program program = cel.createProgram(cel.compile("Value{null_value: NullValue.NULL_VALUE}").getAst()); assertThat(program.eval()).isEqualTo(NullValue.NULL_VALUE); @@ -1084,31 +973,6 @@ public void compile_enumTypeIsEquivalentToInt() throws Exception { assertThat(ast).isNotNull(); } - @Test - public void compile_enumTypeTransitiveResolutionFailure() { - Cel cel = - plannerCelBuilderWithMacros() - .setOptions( - CelOptions.current() - .enableHeterogeneousNumericComparisons(true) - .resolveTypeDependencies(false) - .build()) - .addMessageTypes(Proto2ExtensionScopedMessage.getDescriptor()) - .setResultType(StructTypeReference.create("google.protobuf.NullValue")) - .setContainer(CelContainer.ofName("google.protobuf")) - .build(); - - // 'Value' is a struct defined as a dependency of messages_proto2.proto and 'NullValue' is an - // enum within this 'Value' struct. - // As deep type dependency is disabled, the following evaluation will fail as CEL will not be - // aware of the dependent enum type - CelValidationException e = - Assert.assertThrows( - CelValidationException.class, - () -> cel.compile("Value{null_value: NullValue.NULL_VALUE}").getAst()); - assertThat(e).hasMessageThat().contains("undeclared reference to 'NullValue'"); - } - @Test public void compile_multipleInstancesOfEnumDescriptor_dedupedByFullName() throws Exception { String enumTextProto = diff --git a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java index 394389dfb..52aea8b23 100644 --- a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java +++ b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java @@ -437,7 +437,6 @@ public CelCheckerLegacyImpl build() { CelTypeProvider messageTypeProvider = ProtoMessageTypeProvider.newBuilder() .setAllowJsonFieldNames(celOptions.enableJsonFieldNames()) - .setResolveTypeDependencies(celOptions.resolveTypeDependencies()) .addFileDescriptors(fileTypeSet) .build(); diff --git a/common/src/main/java/dev/cel/common/CelDescriptorUtil.java b/common/src/main/java/dev/cel/common/CelDescriptorUtil.java index 7695e96d8..32375c22a 100644 --- a/common/src/main/java/dev/cel/common/CelDescriptorUtil.java +++ b/common/src/main/java/dev/cel/common/CelDescriptorUtil.java @@ -79,24 +79,8 @@ public static CelDescriptors getAllDescriptorsFromFileDescriptor( */ public static CelDescriptors getAllDescriptorsFromFileDescriptor( Iterable fileDescriptors) { - return getAllDescriptorsFromFileDescriptor(fileDescriptors, true); - } - - /** - * Extract the full message {@code FileDescriptor} set from the input set of {@code - * fileDescriptors}. All message type, enum, extension and file descriptors will be extracted. - * - * @param resolveTypeDependencies Performs a deep type dependency resolution by expanding all the - * FileDescriptors marked as dependents listed in their imports (Ex: If FileDescriptor A - * imports on FileDescriptor B, FD B's descriptors will be pulled in). Setting false will - * disable this. - */ - public static CelDescriptors getAllDescriptorsFromFileDescriptor( - Iterable fileDescriptors, boolean resolveTypeDependencies) { ImmutableSet allFileDescriptors = - resolveTypeDependencies - ? getFileDescriptorsAndDependencies(fileDescriptors) - : ImmutableSet.copyOf(fileDescriptors); + getFileDescriptorsAndDependencies(fileDescriptors); CelDescriptors.Builder celDescriptorsBuilder = CelDescriptors.builder(); allFileDescriptors.forEach( diff --git a/common/src/main/java/dev/cel/common/CelOptions.java b/common/src/main/java/dev/cel/common/CelOptions.java index 29ed87381..3e4dfd54b 100644 --- a/common/src/main/java/dev/cel/common/CelOptions.java +++ b/common/src/main/java/dev/cel/common/CelOptions.java @@ -104,8 +104,6 @@ public enum ProtoUnsetFieldOptions { public abstract boolean errorOnIntWrap(); - public abstract boolean resolveTypeDependencies(); - public abstract boolean enableUnknownTracking(); public abstract boolean enableCelValue(); @@ -161,7 +159,6 @@ public static Builder newBuilder() { .enableProtoDifferencerEquality(false) .errorOnIntWrap(false) .errorOnDuplicateMapKeys(false) - .resolveTypeDependencies(true) .enableUnknownTracking(false) .enableCelValue(false) .comprehensionMaxIterations(-1) @@ -186,7 +183,6 @@ public static Builder current() { .errorOnDuplicateMapKeys(true) .evaluateCanonicalTypesToNativeValues(true) .errorOnIntWrap(true) - .resolveTypeDependencies(true) .disableCelStandardEquality(false); } @@ -426,16 +422,6 @@ public abstract static class Builder { */ public abstract Builder errorOnIntWrap(boolean value); - /** - * Enable or disable the resolution of {@code Descriptor} type dependencies as part of the CEL - * environment setup. Defaults to disabled. - * - *

Disabling this feature should only be done when you know that only the types provided will - * be referenced within the CEL expression. This means that either the type set provided was - * complete, or that the type set is only what is referenced within expressions. - */ - public abstract Builder resolveTypeDependencies(boolean value); - /** * Enable tracking unknown attributes and function invocations encountered during evaluation. * diff --git a/common/src/main/java/dev/cel/common/types/ProtoMessageTypeProvider.java b/common/src/main/java/dev/cel/common/types/ProtoMessageTypeProvider.java index 022f5cd8e..bf0781256 100644 --- a/common/src/main/java/dev/cel/common/types/ProtoMessageTypeProvider.java +++ b/common/src/main/java/dev/cel/common/types/ProtoMessageTypeProvider.java @@ -286,7 +286,6 @@ private Optional findFieldInternal(FieldDescriptor fieldDescriptor) { public static final class Builder { private final ImmutableSet.Builder fileDescriptors = ImmutableSet.builder(); private boolean allowJsonFieldNames; - private boolean resolveTypeDependencies; private CelDescriptors celDescriptors; /** Adds a {@link FileDescriptor} to the provider. */ @@ -321,16 +320,6 @@ public Builder setAllowJsonFieldNames(boolean allowJsonFieldNames) { return this; } - /** - * If true, all transitive dependencies of the added {@link FileDescriptor}s will be resolved - * and their types will be made available to the type provider. By default, this is disabled. - */ - @CanIgnoreReturnValue - public Builder setResolveTypeDependencies(boolean resolveTypeDependencies) { - this.resolveTypeDependencies = resolveTypeDependencies; - return this; - } - /** * Sets the CEL descriptors. Note this cannot be used in conjunction with other descriptor * adders such as {@link #addDescriptors}. @@ -350,9 +339,7 @@ public ProtoMessageTypeProvider build() { } if (celDescriptors == null) { - celDescriptors = - CelDescriptorUtil.getAllDescriptorsFromFileDescriptor( - fileDescriptors.build(), resolveTypeDependencies); + celDescriptors = CelDescriptorUtil.getAllDescriptorsFromFileDescriptor(fds); } return new ProtoMessageTypeProvider(celDescriptors, allowJsonFieldNames); diff --git a/common/src/test/java/dev/cel/common/CelOptionsTest.java b/common/src/test/java/dev/cel/common/CelOptionsTest.java index cc5203a25..53eb4fed4 100644 --- a/common/src/test/java/dev/cel/common/CelOptionsTest.java +++ b/common/src/test/java/dev/cel/common/CelOptionsTest.java @@ -34,7 +34,6 @@ public void current_success_celOptions() { public void current_defaults() { // Defaults that aren't represented in deprecated CelOptions assertThat(CelOptions.current().build().enableUnknownTracking()).isFalse(); - assertThat(CelOptions.current().build().resolveTypeDependencies()).isTrue(); assertThat(CelOptions.current().build().enablePrattParser()).isFalse(); } } diff --git a/common/src/test/java/dev/cel/common/types/ProtoMessageTypeProviderTest.java b/common/src/test/java/dev/cel/common/types/ProtoMessageTypeProviderTest.java index adfb47088..1b35804bd 100644 --- a/common/src/test/java/dev/cel/common/types/ProtoMessageTypeProviderTest.java +++ b/common/src/test/java/dev/cel/common/types/ProtoMessageTypeProviderTest.java @@ -32,18 +32,23 @@ @RunWith(JUnit4.class) public final class ProtoMessageTypeProviderTest { - private final ProtoMessageTypeProvider emptyProvider = new ProtoMessageTypeProvider(); + private final ProtoMessageTypeProvider emptyProvider = + ProtoMessageTypeProvider.newBuilder().build(); private final ProtoMessageTypeProvider proto3Provider = - new ProtoMessageTypeProvider( - ImmutableList.of(dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor())); + ProtoMessageTypeProvider.newBuilder() + .addDescriptors( + ImmutableList.of(dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor())) + .build(); private final ProtoMessageTypeProvider proto2Provider = - new ProtoMessageTypeProvider( - ImmutableSet.of( - dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor().getFile(), - TestAllTypes.getDescriptor().getFile(), - TestAllTypesExtensions.getDescriptor())); + ProtoMessageTypeProvider.newBuilder() + .addFileDescriptors( + ImmutableSet.of( + dev.cel.expr.conformance.proto3.TestAllTypes.getDescriptor().getFile(), + TestAllTypes.getDescriptor().getFile(), + TestAllTypesExtensions.getDescriptor())) + .build(); @Test public void types_emptyTypeSet() { diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntimeImpl.java b/runtime/src/main/java/dev/cel/runtime/CelRuntimeImpl.java index a83c899da..4d13ca354 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntimeImpl.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntimeImpl.java @@ -548,7 +548,6 @@ public CelRuntime build() { ProtoMessageTypeProvider.newBuilder() .setCelDescriptors(celDescriptors) .setAllowJsonFieldNames(options().enableJsonFieldNames()) - .setResolveTypeDependencies(options().resolveTypeDependencies()) .build(); CelTypeProvider combinedTypeProvider = diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java index 144de7e9d..a41ab9656 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java @@ -290,8 +290,7 @@ public CelRuntimeLegacyImpl build() { ImmutableSet fileDescriptors = fileTypes.build(); CelDescriptors celDescriptors = - CelDescriptorUtil.getAllDescriptorsFromFileDescriptor( - fileDescriptors, options.resolveTypeDependencies()); + CelDescriptorUtil.getAllDescriptorsFromFileDescriptor(fileDescriptors); CelDescriptorPool celDescriptorPool = newDescriptorPool(