From 361c80274e9f0a16320f5e8b03aa38d742333e77 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Wed, 3 Sep 2025 14:30:43 -0700 Subject: [PATCH] Properly retain celStandardDeclarations when invoking toBuilder on checker PiperOrigin-RevId: 802710376 --- .../src/main/java/dev/cel/checker/BUILD.bazel | 1 + .../dev/cel/checker/CelCheckerLegacyImpl.java | 38 +++++++--- .../cel/checker/CelCheckerLegacyImplTest.java | 69 +++++++++++++++---- 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/checker/src/main/java/dev/cel/checker/BUILD.bazel b/checker/src/main/java/dev/cel/checker/BUILD.bazel index ff854662d..80f6e1a8a 100644 --- a/checker/src/main/java/dev/cel/checker/BUILD.bazel +++ b/checker/src/main/java/dev/cel/checker/BUILD.bazel @@ -91,6 +91,7 @@ java_library( "@maven//:com_google_errorprone_error_prone_annotations", "@maven//:com_google_guava_guava", "@maven//:com_google_protobuf_protobuf_java", + "@maven//:org_jspecify_jspecify", ], ) diff --git a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java index 4641b0f18..41d1ca073 100644 --- a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java +++ b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java @@ -55,6 +55,7 @@ import java.util.List; import java.util.SortedSet; import java.util.TreeSet; +import org.jspecify.annotations.Nullable; /** * {@code CelChecker} implementation which uses the original CEL-Java APIs to provide a simple, @@ -128,6 +129,10 @@ public CelCheckerBuilder toCheckerBuilder() { builder.setResultType(expectedResultType.get()); } + if (overriddenStandardDeclarations != null) { + builder.setStandardDeclarations(overriddenStandardDeclarations); + } + return builder; } @@ -379,38 +384,53 @@ Builder addIdentDeclarations(ImmutableSet identDeclarations) { return this; } - // The following getters exist for asserting immutability for collections held by this builder, - // and shouldn't be exposed to the public. + // The following getters marked @VisibleForTesting exist for testing toCheckerBuilder copies + // over all properties. Do not expose these to public @VisibleForTesting - ImmutableSet.Builder getFunctionDecls() { + ImmutableSet.Builder functionDecls() { return this.functionDeclarations; } @VisibleForTesting - ImmutableSet.Builder getIdentDecls() { + ImmutableSet.Builder identDecls() { return this.identDeclarations; } @VisibleForTesting - ImmutableSet.Builder getProtoTypeMasks() { + ImmutableSet.Builder protoTypeMasks() { return this.protoTypeMasks; } @VisibleForTesting - ImmutableSet.Builder getMessageTypes() { + ImmutableSet.Builder messageTypes() { return this.messageTypes; } @VisibleForTesting - ImmutableSet.Builder getFileTypes() { + ImmutableSet.Builder fileTypes() { return this.fileTypes; } @VisibleForTesting - ImmutableSet.Builder getCheckerLibraries() { + ImmutableSet.Builder checkerLibraries() { return this.celCheckerLibraries; } + @VisibleForTesting + CelStandardDeclarations standardDeclarations() { + return this.standardDeclarations; + } + + @VisibleForTesting + CelOptions options() { + return this.celOptions; + } + + @VisibleForTesting + CelTypeProvider celTypeProvider() { + return this.celTypeProvider; + } + @Override @CheckReturnValue public CelCheckerLegacyImpl build() { @@ -506,7 +526,7 @@ private CelCheckerLegacyImpl( TypeProvider typeProvider, CelTypeProvider celTypeProvider, boolean standardEnvironmentEnabled, - CelStandardDeclarations overriddenStandardDeclarations, + @Nullable CelStandardDeclarations overriddenStandardDeclarations, ImmutableSet checkerLibraries, ImmutableSet fileDescriptors, ImmutableSet protoTypeMasks) { diff --git a/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java b/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java index f7cfb654d..c0c54381d 100644 --- a/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java +++ b/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java @@ -16,12 +16,19 @@ import static com.google.common.truth.Truth.assertThat; +import com.google.common.collect.ImmutableList; +import dev.cel.checker.CelStandardDeclarations.StandardFunction; +import dev.cel.common.CelContainer; import dev.cel.common.CelFunctionDecl; +import dev.cel.common.CelOptions; import dev.cel.common.CelOverloadDecl; import dev.cel.common.CelVarDecl; +import dev.cel.common.types.CelType; +import dev.cel.common.types.CelTypeProvider; import dev.cel.common.types.SimpleType; import dev.cel.compiler.CelCompilerFactory; import dev.cel.expr.conformance.proto3.TestAllTypes; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -49,7 +56,45 @@ public void toCheckerBuilder_isImmutable() { CelCheckerLegacyImpl.Builder newCheckerBuilder = (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); - assertThat(newCheckerBuilder.getCheckerLibraries().build()).isEmpty(); + assertThat(newCheckerBuilder.checkerLibraries().build()).isEmpty(); + } + + @Test + public void toCheckerBuilder_singularFields_copied() { + CelStandardDeclarations subsetDecls = + CelStandardDeclarations.newBuilder().includeFunctions(StandardFunction.BOOL).build(); + CelOptions celOptions = CelOptions.current().enableTimestampEpoch(true).build(); + CelContainer celContainer = CelContainer.ofName("foo"); + CelType expectedResultType = SimpleType.BOOL; + CelTypeProvider customTypeProvider = + new CelTypeProvider() { + @Override + public ImmutableList types() { + return ImmutableList.of(); + } + + @Override + public Optional findType(String typeName) { + return Optional.empty(); + } + }; + CelCheckerBuilder celCheckerBuilder = + CelCompilerFactory.standardCelCheckerBuilder() + .setOptions(celOptions) + .setContainer(celContainer) + .setResultType(expectedResultType) + .setTypeProvider(customTypeProvider) + .setStandardEnvironmentEnabled(false) + .setStandardDeclarations(subsetDecls); + CelCheckerLegacyImpl celChecker = (CelCheckerLegacyImpl) celCheckerBuilder.build(); + + CelCheckerLegacyImpl.Builder newCheckerBuilder = + (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); + + assertThat(newCheckerBuilder.standardDeclarations()).isEqualTo(subsetDecls); + assertThat(newCheckerBuilder.options()).isEqualTo(celOptions); + assertThat(newCheckerBuilder.container()).isEqualTo(celContainer); + assertThat(newCheckerBuilder.celTypeProvider()).isEqualTo(customTypeProvider); } @Test @@ -70,12 +115,12 @@ public void toCheckerBuilder_collectionProperties_copied() { CelCheckerLegacyImpl.Builder newCheckerBuilder = (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); - assertThat(newCheckerBuilder.getFunctionDecls().build()).hasSize(1); - assertThat(newCheckerBuilder.getIdentDecls().build()).hasSize(1); - assertThat(newCheckerBuilder.getProtoTypeMasks().build()).hasSize(1); - assertThat(newCheckerBuilder.getFileTypes().build()) + assertThat(newCheckerBuilder.functionDecls().build()).hasSize(1); + assertThat(newCheckerBuilder.identDecls().build()).hasSize(1); + assertThat(newCheckerBuilder.protoTypeMasks().build()).hasSize(1); + assertThat(newCheckerBuilder.fileTypes().build()) .hasSize(1); // MessageTypes and FileTypes deduped into the same file descriptor - assertThat(newCheckerBuilder.getCheckerLibraries().build()).hasSize(1); + assertThat(newCheckerBuilder.checkerLibraries().build()).hasSize(1); } @Test @@ -96,11 +141,11 @@ public void toCheckerBuilder_collectionProperties_areImmutable() { ProtoTypeMask.ofAllFields("cel.expr.conformance.proto3.TestAllTypes")); celCheckerBuilder.addLibraries(new CelCheckerLibrary() {}); - assertThat(newCheckerBuilder.getFunctionDecls().build()).isEmpty(); - assertThat(newCheckerBuilder.getIdentDecls().build()).isEmpty(); - assertThat(newCheckerBuilder.getProtoTypeMasks().build()).isEmpty(); - assertThat(newCheckerBuilder.getMessageTypes().build()).isEmpty(); - assertThat(newCheckerBuilder.getFileTypes().build()).isEmpty(); - assertThat(newCheckerBuilder.getCheckerLibraries().build()).isEmpty(); + assertThat(newCheckerBuilder.functionDecls().build()).isEmpty(); + assertThat(newCheckerBuilder.identDecls().build()).isEmpty(); + assertThat(newCheckerBuilder.protoTypeMasks().build()).isEmpty(); + assertThat(newCheckerBuilder.messageTypes().build()).isEmpty(); + assertThat(newCheckerBuilder.fileTypes().build()).isEmpty(); + assertThat(newCheckerBuilder.checkerLibraries().build()).isEmpty(); } }