Skip to content

Properly retain celStandardDeclarations when invoking toBuilder on checker #804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Sep 3, 2025
Merged

Properly retain celStandardDeclarations when invoking toBuilder on checker #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions checker/src/main/java/dev/cel/checker/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

Expand Down
38 changes: 29 additions & 9 deletions checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -128,6 +129,10 @@ public CelCheckerBuilder toCheckerBuilder() {
builder.setResultType(expectedResultType.get());
}

if (overriddenStandardDeclarations != null) {
builder.setStandardDeclarations(overriddenStandardDeclarations);
}

return builder;
}

Expand Down Expand Up @@ -379,38 +384,53 @@ Builder addIdentDeclarations(ImmutableSet<CelIdentDecl> 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<CelFunctionDecl> getFunctionDecls() {
ImmutableSet.Builder<CelFunctionDecl> functionDecls() {
return this.functionDeclarations;
}

@VisibleForTesting
ImmutableSet.Builder<CelIdentDecl> getIdentDecls() {
ImmutableSet.Builder<CelIdentDecl> identDecls() {
return this.identDeclarations;
}

@VisibleForTesting
ImmutableSet.Builder<ProtoTypeMask> getProtoTypeMasks() {
ImmutableSet.Builder<ProtoTypeMask> protoTypeMasks() {
return this.protoTypeMasks;
}

@VisibleForTesting
ImmutableSet.Builder<Descriptor> getMessageTypes() {
ImmutableSet.Builder<Descriptor> messageTypes() {
return this.messageTypes;
}

@VisibleForTesting
ImmutableSet.Builder<FileDescriptor> getFileTypes() {
ImmutableSet.Builder<FileDescriptor> fileTypes() {
return this.fileTypes;
}

@VisibleForTesting
ImmutableSet.Builder<CelCheckerLibrary> getCheckerLibraries() {
ImmutableSet.Builder<CelCheckerLibrary> 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() {
Expand Down Expand Up @@ -506,7 +526,7 @@ private CelCheckerLegacyImpl(
TypeProvider typeProvider,
CelTypeProvider celTypeProvider,
boolean standardEnvironmentEnabled,
CelStandardDeclarations overriddenStandardDeclarations,
@Nullable CelStandardDeclarations overriddenStandardDeclarations,
ImmutableSet<CelCheckerLibrary> checkerLibraries,
ImmutableSet<FileDescriptor> fileDescriptors,
ImmutableSet<ProtoTypeMask> protoTypeMasks) {
Expand Down
69 changes: 57 additions & 12 deletions checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CelType> types() {
return ImmutableList.of();
}

@Override
public Optional<CelType> 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
Expand All @@ -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
Expand All @@ -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();
}
}
Loading