diff --git a/bundle/src/main/java/dev/cel/bundle/Cel.java b/bundle/src/main/java/dev/cel/bundle/Cel.java index 964b21a5c..677a4af2c 100644 --- a/bundle/src/main/java/dev/cel/bundle/Cel.java +++ b/bundle/src/main/java/dev/cel/bundle/Cel.java @@ -20,4 +20,6 @@ /** Cel interface for parse, type-check, and evaluation of CEL programs. */ @ThreadSafe -public interface Cel extends CelCompiler, CelRuntime {} +public interface Cel extends CelCompiler, CelRuntime { + CelBuilder toCelBuilder(); +} diff --git a/bundle/src/main/java/dev/cel/bundle/CelFactory.java b/bundle/src/main/java/dev/cel/bundle/CelFactory.java index c69c4a3a5..a2080bc25 100644 --- a/bundle/src/main/java/dev/cel/bundle/CelFactory.java +++ b/bundle/src/main/java/dev/cel/bundle/CelFactory.java @@ -17,8 +17,10 @@ import dev.cel.checker.CelCheckerLegacyImpl; import dev.cel.common.CelOptions; import dev.cel.compiler.CelCompiler; +import dev.cel.compiler.CelCompilerImpl; import dev.cel.parser.CelParserImpl; import dev.cel.runtime.CelRuntime; +import dev.cel.runtime.CelRuntimeLegacyImpl; /** Helper class to configure the entire CEL stack in a common interface. */ public final class CelFactory { @@ -33,7 +35,10 @@ private CelFactory() {} * evaluation are enabled by default. */ public static CelBuilder standardCelBuilder() { - return CelImpl.newBuilder(CelParserImpl.newBuilder(), CelCheckerLegacyImpl.newBuilder()) + return CelImpl.newBuilder( + CelCompilerImpl.newBuilder( + CelParserImpl.newBuilder(), CelCheckerLegacyImpl.newBuilder()), + CelRuntimeLegacyImpl.newBuilder()) .setOptions(CelOptions.current().build()) .setStandardEnvironmentEnabled(true); } diff --git a/bundle/src/main/java/dev/cel/bundle/CelImpl.java b/bundle/src/main/java/dev/cel/bundle/CelImpl.java index 20086b114..ad062d113 100644 --- a/bundle/src/main/java/dev/cel/bundle/CelImpl.java +++ b/bundle/src/main/java/dev/cel/bundle/CelImpl.java @@ -45,7 +45,6 @@ import dev.cel.common.values.CelValueProvider; import dev.cel.compiler.CelCompiler; import dev.cel.compiler.CelCompilerBuilder; -import dev.cel.compiler.CelCompilerImpl; import dev.cel.compiler.CelCompilerLibrary; import dev.cel.parser.CelMacro; import dev.cel.parser.CelParserBuilder; @@ -53,7 +52,6 @@ import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.CelRuntime; import dev.cel.runtime.CelRuntimeBuilder; -import dev.cel.runtime.CelRuntimeLegacyImpl; import dev.cel.runtime.CelRuntimeLibrary; import java.util.Arrays; import java.util.function.Function; @@ -102,6 +100,31 @@ public void accept(EnvVisitor envVisitor) { } } + @Override + public CelBuilder toCelBuilder() { + return newBuilder(toCompilerBuilder(), toRuntimeBuilder()); + } + + @Override + public CelParserBuilder toParserBuilder() { + return compiler.get().toParserBuilder(); + } + + @Override + public CelCheckerBuilder toCheckerBuilder() { + return compiler.get().toCheckerBuilder(); + } + + @Override + public CelCompilerBuilder toCompilerBuilder() { + return compiler.get().toCompilerBuilder(); + } + + @Override + public CelRuntimeBuilder toRuntimeBuilder() { + return runtime.get().toRuntimeBuilder(); + } + /** Combines a prebuilt {@link CelCompiler} and {@link CelRuntime} into {@link CelImpl}. */ static CelImpl combine(CelCompiler compiler, CelRuntime runtime) { return new CelImpl(Suppliers.memoize(() -> compiler), Suppliers.memoize(() -> runtime)); @@ -112,8 +135,9 @@ static CelImpl combine(CelCompiler compiler, CelRuntime runtime) { * *

By default, {@link CelOptions#DEFAULT} are enabled, as is the CEL standard environment. */ - static CelBuilder newBuilder(CelParserBuilder parserBuilder, CelCheckerBuilder checkerBuilder) { - return new CelImpl.Builder(parserBuilder, checkerBuilder); + static CelBuilder newBuilder( + CelCompilerBuilder compilerBuilder, CelRuntimeBuilder celRuntimeBuilder) { + return new CelImpl.Builder(compilerBuilder, celRuntimeBuilder); } /** Builder class for CelImpl instances. */ @@ -122,9 +146,9 @@ public static final class Builder implements CelBuilder { private final CelCompilerBuilder compilerBuilder; private final CelRuntimeBuilder runtimeBuilder; - private Builder(CelParserBuilder parserBuilder, CelCheckerBuilder checkerBuilder) { - this.compilerBuilder = CelCompilerImpl.newBuilder(parserBuilder, checkerBuilder); - this.runtimeBuilder = CelRuntimeLegacyImpl.newBuilder(); + private Builder(CelCompilerBuilder celCompilerBuilder, CelRuntimeBuilder celRuntimeBuilder) { + this.compilerBuilder = celCompilerBuilder; + this.runtimeBuilder = celRuntimeBuilder; } @Override diff --git a/bundle/src/test/java/dev/cel/bundle/BUILD.bazel b/bundle/src/test/java/dev/cel/bundle/BUILD.bazel index 68ecfa5b2..cccfc8eed 100644 --- a/bundle/src/test/java/dev/cel/bundle/BUILD.bazel +++ b/bundle/src/test/java/dev/cel/bundle/BUILD.bazel @@ -12,6 +12,7 @@ java_library( "//:auto_value", "//:java_truth", "//bundle:cel", + "//checker", "//checker:checker_legacy_environment", "//checker:proto_type_mask", "//common", @@ -31,6 +32,7 @@ java_library( "//common/types:type_providers", "//compiler", "//compiler:compiler_builder", + "//parser", "//parser:macro", "//runtime", "//runtime:unknown_attributes", diff --git a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java index f844246c3..2b3cf30bb 100644 --- a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java +++ b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java @@ -50,6 +50,7 @@ import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; +import dev.cel.checker.CelCheckerLegacyImpl; import dev.cel.checker.DescriptorTypeProvider; import dev.cel.checker.ProtoTypeMask; import dev.cel.checker.TypeProvider; @@ -77,6 +78,8 @@ import dev.cel.common.types.StructTypeReference; import dev.cel.compiler.CelCompiler; import dev.cel.compiler.CelCompilerFactory; +import dev.cel.compiler.CelCompilerImpl; +import dev.cel.parser.CelParserImpl; import dev.cel.parser.CelStandardMacro; import dev.cel.runtime.CelAttribute; import dev.cel.runtime.CelAttribute.Qualifier; @@ -86,6 +89,7 @@ import dev.cel.runtime.CelRuntime.CelFunctionBinding; import dev.cel.runtime.CelRuntime.Program; import dev.cel.runtime.CelRuntimeFactory; +import dev.cel.runtime.CelRuntimeLegacyImpl; import dev.cel.runtime.CelUnknownSet; import dev.cel.runtime.CelVariableResolver; import dev.cel.runtime.UnknownContext; @@ -1812,6 +1816,27 @@ public boolean isAssignableFrom(CelType other) { assertThat(result).isEqualTo("5"); } + @Test + public void toBuilder_isImmutable() { + CelBuilder celBuilder = CelFactory.standardCelBuilder(); + CelImpl celImpl = (CelImpl) celBuilder.build(); + + CelImpl.Builder newCelBuilder = (CelImpl.Builder) celImpl.toCelBuilder(); + CelParserImpl.Builder newParserBuilder = (CelParserImpl.Builder) celImpl.toParserBuilder(); + CelCheckerLegacyImpl.Builder newCheckerBuilder = + (CelCheckerLegacyImpl.Builder) celImpl.toCheckerBuilder(); + CelCompilerImpl.Builder newCompilerBuilder = + (CelCompilerImpl.Builder) celImpl.toCompilerBuilder(); + CelRuntimeLegacyImpl.Builder newRuntimeBuilder = + (CelRuntimeLegacyImpl.Builder) celImpl.toRuntimeBuilder(); + + assertThat(newCelBuilder).isNotEqualTo(celBuilder); + assertThat(newParserBuilder).isNotEqualTo(celImpl.toParserBuilder()); + assertThat(newCheckerBuilder).isNotEqualTo(celImpl.toCheckerBuilder()); + assertThat(newCompilerBuilder).isNotEqualTo(celImpl.toCompilerBuilder()); + assertThat(newRuntimeBuilder).isNotEqualTo(celImpl.toRuntimeBuilder()); + } + private static TypeProvider aliasingProvider(ImmutableMap typeAliases) { return new TypeProvider() { @Override diff --git a/checker/src/main/java/dev/cel/checker/CelChecker.java b/checker/src/main/java/dev/cel/checker/CelChecker.java index 5a6e9c28b..4022212cb 100644 --- a/checker/src/main/java/dev/cel/checker/CelChecker.java +++ b/checker/src/main/java/dev/cel/checker/CelChecker.java @@ -28,4 +28,6 @@ public interface CelChecker { *

Check validates the type-agreement of the parsed {@code CelAbstractSyntaxTree}. */ CelValidationResult check(CelAbstractSyntaxTree ast); + + CelCheckerBuilder toCheckerBuilder(); } diff --git a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java index ccc9c2955..4ed263c11 100644 --- a/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java +++ b/checker/src/main/java/dev/cel/checker/CelCheckerLegacyImpl.java @@ -20,6 +20,7 @@ import dev.cel.expr.Decl; import dev.cel.expr.Type; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -62,8 +63,8 @@ public final class CelCheckerLegacyImpl implements CelChecker, EnvVisitable { private final CelOptions celOptions; private final String container; - private final ImmutableList identDeclarations; - private final ImmutableList functionDeclarations; + private final ImmutableSet identDeclarations; + private final ImmutableSet functionDeclarations; private final Optional expectedResultType; @SuppressWarnings("Immutable") @@ -71,6 +72,10 @@ public final class CelCheckerLegacyImpl implements CelChecker, EnvVisitable { private final boolean standardEnvironmentEnabled; + // Builder is mutable by design. APIs must make defensive copies in and out of this class. + @SuppressWarnings("Immutable") + private final Builder checkerBuilder; + @Override public CelValidationResult check(CelAbstractSyntaxTree ast) { CelSource source = ast.getSource(); @@ -88,6 +93,11 @@ public CelValidationResult check(CelAbstractSyntaxTree ast) { return new CelValidationResult(checkedAst, ImmutableList.of()); } + @Override + public CelCheckerBuilder toCheckerBuilder() { + return new Builder(checkerBuilder); + } + @Override public void accept(EnvVisitor envVisitor) { Errors errors = new Errors("", ""); @@ -132,9 +142,9 @@ public static CelCheckerBuilder newBuilder() { /** Builder class for the legacy {@code CelChecker} implementation. */ public static final class Builder implements CelCheckerBuilder { - private final ImmutableList.Builder identDeclarations; - private final ImmutableList.Builder functionDeclarations; - private final ImmutableList.Builder protoTypeMasks; + private final ImmutableSet.Builder identDeclarations; + private final ImmutableSet.Builder functionDeclarations; + private final ImmutableSet.Builder protoTypeMasks; private final ImmutableSet.Builder messageTypes; private final ImmutableSet.Builder fileTypes; private final ImmutableSet.Builder celCheckerLibraries; @@ -316,6 +326,38 @@ public CelCheckerBuilder addLibraries(Iterable libr return this; } + // The following getters exist for asserting immutability for collections held by this builder, + // and shouldn't be exposed to the public. + @VisibleForTesting + ImmutableSet.Builder getFunctionDecls() { + return this.functionDeclarations; + } + + @VisibleForTesting + ImmutableSet.Builder getIdentDecls() { + return this.identDeclarations; + } + + @VisibleForTesting + ImmutableSet.Builder getProtoTypeMasks() { + return this.protoTypeMasks; + } + + @VisibleForTesting + ImmutableSet.Builder getMessageTypes() { + return this.messageTypes; + } + + @VisibleForTesting + ImmutableSet.Builder getFileTypes() { + return this.fileTypes; + } + + @VisibleForTesting + ImmutableSet.Builder getCheckerLibraries() { + return this.celCheckerLibraries; + } + @Override @CheckReturnValue public CelCheckerLegacyImpl build() { @@ -348,13 +390,13 @@ public CelCheckerLegacyImpl build() { // Configure the declaration set, and possibly alter the type provider if ProtoDecl values // are provided as they may prevent the use of certain field selection patterns against the // proto. - ImmutableList identDeclarationSet = identDeclarations.build(); - ImmutableList protoTypeMaskSet = protoTypeMasks.build(); + ImmutableSet identDeclarationSet = identDeclarations.build(); + ImmutableSet protoTypeMaskSet = protoTypeMasks.build(); if (!protoTypeMaskSet.isEmpty()) { ProtoTypeMaskTypeProvider protoTypeMaskTypeProvider = new ProtoTypeMaskTypeProvider(messageTypeProvider, protoTypeMaskSet); identDeclarationSet = - ImmutableList.builder() + ImmutableSet.builder() .addAll(identDeclarationSet) .addAll(protoTypeMaskTypeProvider.computeDeclsFromProtoTypeMasks()) .build(); @@ -375,29 +417,55 @@ public CelCheckerLegacyImpl build() { functionDeclarations.build(), Optional.fromNullable(expectedResultType), legacyProvider, - standardEnvironmentEnabled); + standardEnvironmentEnabled, + this); } private Builder() { this.celOptions = CelOptions.newBuilder().build(); - this.identDeclarations = ImmutableList.builder(); - this.functionDeclarations = ImmutableList.builder(); + this.identDeclarations = ImmutableSet.builder(); + this.functionDeclarations = ImmutableSet.builder(); this.fileTypes = ImmutableSet.builder(); this.messageTypes = ImmutableSet.builder(); - this.protoTypeMasks = ImmutableList.builder(); + this.protoTypeMasks = ImmutableSet.builder(); this.celCheckerLibraries = ImmutableSet.builder(); this.container = ""; } + + private Builder(Builder builder) { + // The following properties are either immutable or simple primitives, thus can be assigned + // directly. + this.celOptions = builder.celOptions; + this.celTypeProvider = builder.celTypeProvider; + this.container = builder.container; + this.customTypeProvider = builder.customTypeProvider; + this.expectedResultType = builder.expectedResultType; + this.standardEnvironmentEnabled = builder.standardEnvironmentEnabled; + // The following needs to be deep copied as they are collection builders + this.functionDeclarations = deepCopy(builder.functionDeclarations); + this.identDeclarations = deepCopy(builder.identDeclarations); + this.fileTypes = deepCopy(builder.fileTypes); + this.messageTypes = deepCopy(builder.messageTypes); + this.protoTypeMasks = deepCopy(builder.protoTypeMasks); + this.celCheckerLibraries = deepCopy(builder.celCheckerLibraries); + } + + private static ImmutableSet.Builder deepCopy(ImmutableSet.Builder builderToCopy) { + ImmutableSet.Builder newBuilder = ImmutableSet.builder(); + newBuilder.addAll(builderToCopy.build()); + return newBuilder; + } } private CelCheckerLegacyImpl( CelOptions celOptions, String container, - ImmutableList identDeclarations, - ImmutableList functionDeclarations, + ImmutableSet identDeclarations, + ImmutableSet functionDeclarations, Optional expectedResultType, TypeProvider typeProvider, - boolean standardEnvironmentEnabled) { + boolean standardEnvironmentEnabled, + Builder checkerBuilder) { this.celOptions = celOptions; this.container = container; this.identDeclarations = identDeclarations; @@ -405,6 +473,7 @@ private CelCheckerLegacyImpl( this.expectedResultType = expectedResultType; this.typeProvider = typeProvider; this.standardEnvironmentEnabled = standardEnvironmentEnabled; + this.checkerBuilder = new Builder(checkerBuilder); } private static ImmutableList errorsToIssues(Errors errors) { diff --git a/checker/src/main/java/dev/cel/checker/Env.java b/checker/src/main/java/dev/cel/checker/Env.java index 17d563dc8..8ce80f488 100644 --- a/checker/src/main/java/dev/cel/checker/Env.java +++ b/checker/src/main/java/dev/cel/checker/Env.java @@ -958,7 +958,7 @@ private static CelFunctionDecl sanitizeFunction(CelFunctionDecl func) { } CelFunctionDecl.Builder funcBuilder = func.toBuilder(); - ImmutableList.Builder overloadsBuilder = new ImmutableList.Builder<>(); + ImmutableSet.Builder overloadsBuilder = new ImmutableSet.Builder<>(); for (CelOverloadDecl overloadDecl : funcBuilder.overloads()) { CelOverloadDecl.Builder overloadBuilder = overloadDecl.toBuilder(); CelType resultType = overloadBuilder.build().resultType(); @@ -966,7 +966,7 @@ private static CelFunctionDecl sanitizeFunction(CelFunctionDecl func) { overloadBuilder.setResultType(getWellKnownType(resultType)); } - ImmutableList.Builder parameterTypeBuilder = ImmutableList.builder(); + ImmutableSet.Builder parameterTypeBuilder = ImmutableSet.builder(); for (CelType paramType : overloadBuilder.parameterTypes()) { if (isWellKnownType(paramType)) { parameterTypeBuilder.add(getWellKnownType(paramType)); diff --git a/checker/src/main/java/dev/cel/checker/ProtoTypeMaskTypeProvider.java b/checker/src/main/java/dev/cel/checker/ProtoTypeMaskTypeProvider.java index 07325db00..61511c38d 100644 --- a/checker/src/main/java/dev/cel/checker/ProtoTypeMaskTypeProvider.java +++ b/checker/src/main/java/dev/cel/checker/ProtoTypeMaskTypeProvider.java @@ -44,10 +44,10 @@ public final class ProtoTypeMaskTypeProvider implements CelTypeProvider { @SuppressWarnings("Immutable") private final ImmutableMap allTypes; - private final ImmutableList protoTypeMasks; + private final ImmutableSet protoTypeMasks; ProtoTypeMaskTypeProvider( - CelTypeProvider delegateProvider, ImmutableList protoTypeMasks) { + CelTypeProvider delegateProvider, ImmutableSet protoTypeMasks) { this.protoTypeMasks = protoTypeMasks; this.allTypes = computeVisibleFieldsMap(delegateProvider, protoTypeMasks); } @@ -89,7 +89,7 @@ ImmutableList computeDeclsFromProtoTypeMasks() { } private static ImmutableMap computeVisibleFieldsMap( - CelTypeProvider delegateProvider, ImmutableList protoTypeMasks) { + CelTypeProvider delegateProvider, ImmutableSet protoTypeMasks) { Map> fieldMap = new HashMap<>(); for (ProtoTypeMask typeMask : protoTypeMasks) { Optional rootType = delegateProvider.findType(typeMask.getTypeName()); diff --git a/checker/src/test/java/dev/cel/checker/BUILD.bazel b/checker/src/test/java/dev/cel/checker/BUILD.bazel index 1498a6b9b..338dcbb10 100644 --- a/checker/src/test/java/dev/cel/checker/BUILD.bazel +++ b/checker/src/test/java/dev/cel/checker/BUILD.bazel @@ -14,6 +14,7 @@ java_library( "//:auto_value", "//checker", "//checker:cel_ident_decl", + "//checker:checker_builder", "//checker:checker_legacy_environment", "//checker:proto_expr_visitor", "//checker:proto_type_mask", diff --git a/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java b/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java new file mode 100644 index 000000000..1f96367f3 --- /dev/null +++ b/checker/src/test/java/dev/cel/checker/CelCheckerLegacyImplTest.java @@ -0,0 +1,106 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.checker; + +import static com.google.common.truth.Truth.assertThat; + +import dev.cel.common.CelFunctionDecl; +import dev.cel.common.CelOverloadDecl; +import dev.cel.common.CelVarDecl; +import dev.cel.common.types.SimpleType; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.testing.testdata.proto2.TestAllTypesProto.TestAllTypes; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class CelCheckerLegacyImplTest { + + @Test + public void toCheckerBuilder_isNewInstance() { + CelCheckerBuilder celCheckerBuilder = CelCompilerFactory.standardCelCheckerBuilder(); + CelCheckerLegacyImpl celChecker = (CelCheckerLegacyImpl) celCheckerBuilder.build(); + + CelCheckerLegacyImpl.Builder newCheckerBuilder = + (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); + + assertThat(newCheckerBuilder).isNotEqualTo(celCheckerBuilder); + } + + @Test + public void toCheckerBuilder_isImmutable() { + CelCheckerBuilder originalCheckerBuilder = CelCompilerFactory.standardCelCheckerBuilder(); + CelCheckerLegacyImpl celChecker = (CelCheckerLegacyImpl) originalCheckerBuilder.build(); + originalCheckerBuilder.addLibraries(new CelCheckerLibrary() {}); + + CelCheckerLegacyImpl.Builder newCheckerBuilder = + (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); + + assertThat(newCheckerBuilder.getCheckerLibraries().build()).isEmpty(); + } + + @Test + public void toCheckerBuilder_collectionProperties_copied() { + CelCheckerBuilder celCheckerBuilder = + CelCompilerFactory.standardCelCheckerBuilder() + .addFunctionDeclarations( + CelFunctionDecl.newFunctionDeclaration( + "test", CelOverloadDecl.newGlobalOverload("test_id", SimpleType.INT))) + .addVarDeclarations(CelVarDecl.newVarDeclaration("ident", SimpleType.INT)) + .addMessageTypes(TestAllTypes.getDescriptor()) + .addFileTypes(TestAllTypes.getDescriptor().getFile()) + .addProtoTypeMasks( + ProtoTypeMask.ofAllFields("dev.cel.testing.testdata.proto2.TestAllTypes")) + .addLibraries(new CelCheckerLibrary() {}); + CelCheckerLegacyImpl celChecker = (CelCheckerLegacyImpl) celCheckerBuilder.build(); + + 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.getMessageTypes().build()).hasSize(1); + assertThat(newCheckerBuilder.getFileTypes().build()).hasSize(1); + assertThat(newCheckerBuilder.getCheckerLibraries().build()).hasSize(1); + } + + @Test + public void toCheckerBuilder_collectionProperties_areImmutable() { + CelCheckerBuilder celCheckerBuilder = CelCompilerFactory.standardCelCheckerBuilder(); + CelCheckerLegacyImpl celChecker = (CelCheckerLegacyImpl) celCheckerBuilder.build(); + CelCheckerLegacyImpl.Builder newCheckerBuilder = + (CelCheckerLegacyImpl.Builder) celChecker.toCheckerBuilder(); + + // Mutate the original builder containing collections + celCheckerBuilder.addFunctionDeclarations( + CelFunctionDecl.newFunctionDeclaration( + "test", CelOverloadDecl.newGlobalOverload("test_id", SimpleType.INT))); + celCheckerBuilder.addVarDeclarations(CelVarDecl.newVarDeclaration("ident", SimpleType.INT)); + celCheckerBuilder.addMessageTypes(TestAllTypes.getDescriptor()); + celCheckerBuilder.addFileTypes(TestAllTypes.getDescriptor().getFile()); + celCheckerBuilder.addProtoTypeMasks( + ProtoTypeMask.ofAllFields("dev.cel.testing.testdata.proto2.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(); + } +} diff --git a/checker/src/test/java/dev/cel/checker/CelCompilerImplTest.java b/checker/src/test/java/dev/cel/checker/CelCompilerImplTest.java new file mode 100644 index 000000000..b3abc4c6b --- /dev/null +++ b/checker/src/test/java/dev/cel/checker/CelCompilerImplTest.java @@ -0,0 +1,45 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.checker; + +import static com.google.common.truth.Truth.assertThat; + +import dev.cel.common.CelFunctionDecl; +import dev.cel.common.CelOverloadDecl; +import dev.cel.common.types.SimpleType; +import dev.cel.compiler.CelCompilerBuilder; +import dev.cel.compiler.CelCompilerFactory; +import dev.cel.compiler.CelCompilerImpl; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class CelCompilerImplTest { + + @Test + public void toCompilerBuilder_isImmutable() { + CelCompilerBuilder celCompilerBuilder = CelCompilerFactory.standardCelCompilerBuilder(); + CelCompilerImpl celCompiler = (CelCompilerImpl) celCompilerBuilder.build(); + celCompilerBuilder.addFunctionDeclarations( + CelFunctionDecl.newFunctionDeclaration( + "test", CelOverloadDecl.newGlobalOverload("test_id", SimpleType.INT))); + + CelCompilerImpl.Builder newCompilerBuilder = + (CelCompilerImpl.Builder) celCompiler.toCompilerBuilder(); + + assertThat(newCompilerBuilder).isNotEqualTo(celCompilerBuilder); + } +} diff --git a/checker/src/test/java/dev/cel/checker/CelFunctionDeclTest.java b/checker/src/test/java/dev/cel/checker/CelFunctionDeclTest.java index fe386d8c7..662761f41 100644 --- a/checker/src/test/java/dev/cel/checker/CelFunctionDeclTest.java +++ b/checker/src/test/java/dev/cel/checker/CelFunctionDeclTest.java @@ -22,6 +22,7 @@ import dev.cel.common.CelFunctionDecl; import dev.cel.common.CelOverloadDecl; import dev.cel.common.types.SimpleType; +import java.util.Iterator; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -38,7 +39,7 @@ public void declareGlobalFunction_success() { assertThat(functionDecl.name()).isEqualTo("testGlobalFunction"); assertThat(functionDecl.overloads()).hasSize(1); - CelOverloadDecl overloadDecl = functionDecl.overloads().get(0); + CelOverloadDecl overloadDecl = functionDecl.overloads().iterator().next(); assertThat(overloadDecl.overloadId()).isEqualTo("overloadId"); assertThat(overloadDecl.isInstanceFunction()).isFalse(); assertThat(overloadDecl.resultType()).isEqualTo(SimpleType.BOOL); @@ -54,7 +55,7 @@ public void declareMemberFunction_success() { assertThat(functionDecl.name()).isEqualTo("testMemberFunction"); assertThat(functionDecl.overloads()).hasSize(1); - CelOverloadDecl overloadDecl = functionDecl.overloads().get(0); + CelOverloadDecl overloadDecl = functionDecl.overloads().iterator().next(); assertThat(overloadDecl.overloadId()).isEqualTo("overloadId"); assertThat(overloadDecl.isInstanceFunction()).isTrue(); assertThat(overloadDecl.resultType()).isEqualTo(SimpleType.TIMESTAMP); @@ -74,13 +75,14 @@ public void declareFunction_withBuilder_success() { assertThat(functionDecl.name()).isEqualTo("testFunction"); assertThat(functionDecl.overloads()).hasSize(2); - CelOverloadDecl memberOverloadDecl = functionDecl.overloads().get(0); + Iterator iterator = functionDecl.overloads().iterator(); + CelOverloadDecl memberOverloadDecl = iterator.next(); assertThat(memberOverloadDecl.overloadId()).isEqualTo("memberOverloadId"); assertThat(memberOverloadDecl.isInstanceFunction()).isTrue(); assertThat(memberOverloadDecl.resultType()).isEqualTo(SimpleType.INT); assertThat(memberOverloadDecl.parameterTypes()).containsExactly(SimpleType.UINT); - CelOverloadDecl globalOverloadDecl = functionDecl.overloads().get(1); + CelOverloadDecl globalOverloadDecl = iterator.next(); assertThat(globalOverloadDecl.overloadId()).isEqualTo("globalOverloadId"); assertThat(globalOverloadDecl.isInstanceFunction()).isFalse(); assertThat(globalOverloadDecl.resultType()).isEqualTo(SimpleType.STRING); diff --git a/checker/src/test/java/dev/cel/checker/ProtoTypeMaskTypeProviderTest.java b/checker/src/test/java/dev/cel/checker/ProtoTypeMaskTypeProviderTest.java index 31d0fb221..bf41c1e20 100644 --- a/checker/src/test/java/dev/cel/checker/ProtoTypeMaskTypeProviderTest.java +++ b/checker/src/test/java/dev/cel/checker/ProtoTypeMaskTypeProviderTest.java @@ -19,7 +19,6 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.protobuf.FieldMask; import com.google.rpc.context.AttributeContext; @@ -52,7 +51,7 @@ public void protoTypeMaskProvider_badFieldMask() { () -> new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder().addPaths("missing").build())))); @@ -62,16 +61,16 @@ public void protoTypeMaskProvider_badFieldMask() { public void lookupFieldNames_undeclaredMessageType() { CelTypeProvider celTypeProvider = new ProtoMessageTypeProvider(); ProtoTypeMaskTypeProvider protoTypeMaskProvider = - new ProtoTypeMaskTypeProvider(celTypeProvider, ImmutableList.of()); + new ProtoTypeMaskTypeProvider(celTypeProvider, ImmutableSet.of()); assertThat(protoTypeMaskProvider.findType(ATTRIBUTE_CONTEXT_TYPE)).isEmpty(); } @Test public void lookupFieldNames_noProtoDecls() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = - new ProtoTypeMaskTypeProvider(celTypeProvider, ImmutableList.of()); + new ProtoTypeMaskTypeProvider(celTypeProvider, ImmutableSet.of()); ProtoMessageType protoType = assertTypeFound(protoTypeMaskProvider, ATTRIBUTE_CONTEXT_TYPE); assertThat(protoType.fields().stream().map(f -> f.name()).collect(toImmutableList())) .containsExactly( @@ -91,10 +90,10 @@ public void lookupFieldNames_noProtoDecls() { @Test public void lookupFieldNames_fullProtoDecl() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( - celTypeProvider, ImmutableList.of(ProtoTypeMask.ofAllFields(ATTRIBUTE_CONTEXT_TYPE))); + celTypeProvider, ImmutableSet.of(ProtoTypeMask.ofAllFields(ATTRIBUTE_CONTEXT_TYPE))); ProtoMessageType protoType = assertTypeFound(protoTypeMaskProvider, ATTRIBUTE_CONTEXT_TYPE); assertTypeHasFields( protoType, @@ -114,11 +113,11 @@ public void lookupFieldNames_fullProtoDecl() { @Test public void lookupFieldNames_partialProtoDecl() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder() @@ -169,7 +168,7 @@ public void computeDecls() { ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder() @@ -191,11 +190,11 @@ public void computeDecls() { @Test public void lookupFieldType() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder() @@ -228,11 +227,11 @@ public void lookupFieldType() { @Test public void lookupFieldType_notExposedField() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder().addPaths("resource.name").build()))); @@ -243,11 +242,11 @@ public void lookupFieldType_notExposedField() { @Test public void lookupType_notExposed() { CelTypeProvider celTypeProvider = - new ProtoMessageTypeProvider(ImmutableList.of(AttributeContext.getDescriptor())); + new ProtoMessageTypeProvider(ImmutableSet.of(AttributeContext.getDescriptor())); ProtoTypeMaskTypeProvider protoTypeMaskProvider = new ProtoTypeMaskTypeProvider( celTypeProvider, - ImmutableList.of( + ImmutableSet.of( ProtoTypeMask.of( "google.rpc.context.AttributeContext", FieldMask.newBuilder().addPaths("resource.name").build()))); diff --git a/common/src/main/java/dev/cel/common/CelFunctionDecl.java b/common/src/main/java/dev/cel/common/CelFunctionDecl.java index 68b5ff406..12beb53d7 100644 --- a/common/src/main/java/dev/cel/common/CelFunctionDecl.java +++ b/common/src/main/java/dev/cel/common/CelFunctionDecl.java @@ -20,7 +20,7 @@ import dev.cel.expr.Decl; import dev.cel.expr.Decl.FunctionDecl; import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; @@ -36,7 +36,7 @@ public abstract class CelFunctionDecl { public abstract String name(); /** Required. List of function overloads. Must contain at least one overload. */ - public abstract ImmutableList overloads(); + public abstract ImmutableSet overloads(); /** Builder for configuring the {@link CelFunctionDecl}. */ @AutoValue.Builder @@ -46,12 +46,12 @@ public abstract static class Builder { /** Sets the function name {@link #name()} */ public abstract Builder setName(String name); - public abstract ImmutableList overloads(); + public abstract ImmutableSet overloads(); - public abstract ImmutableList.Builder overloadsBuilder(); + public abstract ImmutableSet.Builder overloadsBuilder(); @CanIgnoreReturnValue - public abstract Builder setOverloads(ImmutableList overloads); + public abstract Builder setOverloads(ImmutableSet overloads); /** Adds one or more function overloads */ @CanIgnoreReturnValue @@ -77,7 +77,7 @@ public Builder addOverloads(Iterable overloads) { /** Create a new builder to construct a {@code CelFunctionDecl} instance. */ public static Builder newBuilder() { - return new AutoValue_CelFunctionDecl.Builder().setOverloads(ImmutableList.of()); + return new AutoValue_CelFunctionDecl.Builder().setOverloads(ImmutableSet.of()); } /** Constructs a function declaration with any number of {@link CelOverloadDecl} */ diff --git a/common/src/main/java/dev/cel/common/CelOptions.java b/common/src/main/java/dev/cel/common/CelOptions.java index f4082f918..eaa812e2b 100644 --- a/common/src/main/java/dev/cel/common/CelOptions.java +++ b/common/src/main/java/dev/cel/common/CelOptions.java @@ -89,7 +89,6 @@ public abstract class CelOptions { public abstract boolean enableCelValue(); - public abstract int comprehensionMaxIterations(); public abstract Builder toBuilder(); diff --git a/common/src/main/java/dev/cel/common/CelOverloadDecl.java b/common/src/main/java/dev/cel/common/CelOverloadDecl.java index 247f86bc1..30bcac5a8 100644 --- a/common/src/main/java/dev/cel/common/CelOverloadDecl.java +++ b/common/src/main/java/dev/cel/common/CelOverloadDecl.java @@ -93,7 +93,7 @@ public abstract static class Builder { * Sets the parameter types {@link #parameterTypes()}. Note that this will override any * parameter types added via the accumulator methods {@link #addParameterTypes}. */ - public abstract Builder setParameterTypes(ImmutableList value); + public abstract Builder setParameterTypes(ImmutableSet value); public abstract CelType resultType(); diff --git a/compiler/src/main/java/dev/cel/compiler/CelCompiler.java b/compiler/src/main/java/dev/cel/compiler/CelCompiler.java index dd4d5dd10..602646ab4 100644 --- a/compiler/src/main/java/dev/cel/compiler/CelCompiler.java +++ b/compiler/src/main/java/dev/cel/compiler/CelCompiler.java @@ -57,4 +57,6 @@ default CelValidationResult compile(String expression, String description) { throw new IllegalStateException("this method must only be called when !hasError()", ex); } } + + CelCompilerBuilder toCompilerBuilder(); } diff --git a/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java b/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java index 5a99054a6..ac81f5d0c 100644 --- a/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java +++ b/compiler/src/main/java/dev/cel/compiler/CelCompilerImpl.java @@ -81,6 +81,21 @@ public void accept(EnvVisitor envVisitor) { } } + @Override + public CelCompilerBuilder toCompilerBuilder() { + return newBuilder(toParserBuilder(), toCheckerBuilder()); + } + + @Override + public CelCheckerBuilder toCheckerBuilder() { + return checker.toCheckerBuilder(); + } + + @Override + public CelParserBuilder toParserBuilder() { + return parser.toParserBuilder(); + } + /** Combines a prebuilt {@link CelParser} and {@link CelChecker} into {@link CelCompilerImpl}. */ static CelCompilerImpl combine(CelParser parser, CelChecker checker) { return new CelCompilerImpl(parser, checker); diff --git a/parser/src/main/java/dev/cel/parser/CelParser.java b/parser/src/main/java/dev/cel/parser/CelParser.java index 53879d11b..da1b7ba2a 100644 --- a/parser/src/main/java/dev/cel/parser/CelParser.java +++ b/parser/src/main/java/dev/cel/parser/CelParser.java @@ -52,4 +52,6 @@ default CelValidationResult parse(String expression) { */ @CheckReturnValue CelValidationResult parse(CelSource source); + + CelParserBuilder toParserBuilder(); } diff --git a/parser/src/main/java/dev/cel/parser/CelParserImpl.java b/parser/src/main/java/dev/cel/parser/CelParserImpl.java index 5d9944397..f630f7137 100644 --- a/parser/src/main/java/dev/cel/parser/CelParserImpl.java +++ b/parser/src/main/java/dev/cel/parser/CelParserImpl.java @@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkNotNull; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -50,6 +51,10 @@ public final class CelParserImpl implements CelParser { // Specific options for limits on parsing power. private final CelOptions options; + // Builder is mutable by design. APIs must make defensive copies in and out of this class. + @SuppressWarnings("Immutable") + private final Builder parserBuilder; + /** Creates a new {@link Builder}. */ public static CelParserBuilder newBuilder() { return new Builder().setOptions(CelOptions.DEFAULT); @@ -65,6 +70,11 @@ public CelValidationResult parse(CelSource source) { return Parser.parse(this, source, getOptions()); } + @Override + public CelParserBuilder toParserBuilder() { + return new Builder(parserBuilder); + } + Optional findMacro(String key) { return Optional.ofNullable(macros.get(key)); } @@ -136,6 +146,23 @@ public CelOptions getOptions() { return this.options; } + // The following getters exist for asserting immutability for collections held by this builder, + // and shouldn't be exposed to the public. + @VisibleForTesting + List getStandardMacros() { + return this.standardMacros; + } + + @VisibleForTesting + Map getMacros() { + return this.macros; + } + + @VisibleForTesting + ImmutableSet.Builder getParserLibraries() { + return this.celParserLibraries; + } + @Override @CheckReturnValue public CelParserImpl build() { @@ -149,7 +176,7 @@ public CelParserImpl build() { standardMacros.stream() .map(CelStandardMacro::getDefinition) .forEach(celMacro -> builder.put(celMacro.getKey(), celMacro)); - return new CelParserImpl(builder.buildOrThrow(), checkNotNull(options)); + return new CelParserImpl(builder.buildOrThrow(), checkNotNull(options), this); } private Builder() { @@ -157,10 +184,23 @@ private Builder() { this.celParserLibraries = ImmutableSet.builder(); this.standardMacros = new ArrayList<>(); } + + private Builder(Builder builder) { + // The following properties are either immutable or simple primitives, thus can be assigned + // directly. + this.options = builder.options; + // The following needs to be deep copied as they are collection builders + this.macros = new HashMap<>(builder.macros); + this.standardMacros = new ArrayList<>(builder.standardMacros); + this.celParserLibraries = ImmutableSet.builder(); + this.celParserLibraries.addAll(builder.celParserLibraries.build()); + } } - private CelParserImpl(ImmutableMap macros, CelOptions options) { + private CelParserImpl( + ImmutableMap macros, CelOptions options, Builder parserBuilder) { this.macros = macros; this.options = options; + this.parserBuilder = new Builder(parserBuilder); } } diff --git a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java index 7c01c3611..f12fef292 100644 --- a/parser/src/test/java/dev/cel/parser/CelParserImplTest.java +++ b/parser/src/test/java/dev/cel/parser/CelParserImplTest.java @@ -271,4 +271,44 @@ public void parse_macroArgumentContainsSyntaxError_throws(String expression) { assertThat(parseResult.getErrorString()).containsMatch("ERROR: .*mismatched input ','"); assertThrows(CelValidationException.class, parseResult::getAst); } + + @Test + public void toParserBuilder_isNewInstance() { + CelParserBuilder celParserBuilder = CelParserFactory.standardCelParserBuilder(); + CelParserImpl celParser = (CelParserImpl) celParserBuilder.build(); + + CelParserImpl.Builder newParserBuilder = (CelParserImpl.Builder) celParser.toParserBuilder(); + + assertThat(newParserBuilder).isNotEqualTo(celParserBuilder); + } + + @Test + public void toParserBuilder_isImmutable() { + CelParserBuilder originalParserBuilder = CelParserFactory.standardCelParserBuilder(); + CelParserImpl celParser = (CelParserImpl) originalParserBuilder.build(); + originalParserBuilder.addLibraries(new CelParserLibrary() {}); + + CelParserImpl.Builder newParserBuilder = (CelParserImpl.Builder) celParser.toParserBuilder(); + + assertThat(newParserBuilder.getParserLibraries().build()).isEmpty(); + } + + @Test + public void toParserBuilder_collectionProperties_copied() { + CelParserBuilder celParserBuilder = + CelParserFactory.standardCelParserBuilder() + .setStandardMacros(CelStandardMacro.STANDARD_MACROS) + .addMacros( + CelMacro.newGlobalMacro( + "test", 1, (a, b, c) -> Optional.of(CelExpr.newBuilder().build()))) + .addLibraries(new CelParserLibrary() {}); + CelParserImpl celParser = (CelParserImpl) celParserBuilder.build(); + + CelParserImpl.Builder newParserBuilder = (CelParserImpl.Builder) celParser.toParserBuilder(); + + assertThat(newParserBuilder.getStandardMacros()) + .hasSize(CelStandardMacro.STANDARD_MACROS.size()); + assertThat(newParserBuilder.getMacros()).hasSize(1); + assertThat(newParserBuilder.getParserLibraries().build()).hasSize(1); + } } diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntime.java b/runtime/src/main/java/dev/cel/runtime/CelRuntime.java index 01260fb79..2b28d015b 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntime.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntime.java @@ -37,6 +37,8 @@ public interface CelRuntime { @CanIgnoreReturnValue Program createProgram(CelAbstractSyntaxTree ast) throws CelEvaluationException; + CelRuntimeBuilder toRuntimeBuilder(); + /** Creates an evaluable {@code Program} instance which is thread-safe and immutable. */ @AutoValue @Immutable diff --git a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java index 5f8f3f259..19059506a 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java +++ b/runtime/src/main/java/dev/cel/runtime/CelRuntimeLegacyImpl.java @@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -43,6 +44,8 @@ import dev.cel.common.values.CelValueProvider; import dev.cel.common.values.ProtoMessageValueProvider; import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; import java.util.Optional; import java.util.function.Function; import org.jspecify.nullness.Nullable; @@ -60,12 +63,21 @@ public final class CelRuntimeLegacyImpl implements CelRuntime { private final Interpreter interpreter; private final CelOptions options; + // Builder is mutable by design. APIs must guarantee a new instance to be returned. + // CEL-Internal-4 + private final Builder runtimeBuilder; + @Override public CelRuntime.Program createProgram(CelAbstractSyntaxTree ast) { checkState(ast.isChecked(), "programs must be created from checked expressions"); return CelRuntime.Program.from(interpreter.createInterpretable(ast), options); } + @Override + public CelRuntimeBuilder toRuntimeBuilder() { + return new Builder(runtimeBuilder); + } + /** Create a new builder for constructing a {@code CelRuntime} instance. */ public static CelRuntimeBuilder newBuilder() { return new Builder(); @@ -75,7 +87,7 @@ public static CelRuntimeBuilder newBuilder() { public static final class Builder implements CelRuntimeBuilder { private final ImmutableSet.Builder fileTypes; - private final ImmutableMap.Builder functionBindings; + private final HashMap functionBindings; private final ImmutableSet.Builder celRuntimeLibraries; @SuppressWarnings("unused") @@ -99,7 +111,7 @@ public CelRuntimeBuilder addFunctionBindings(CelFunctionBinding... bindings) { @Override public CelRuntimeBuilder addFunctionBindings(Iterable bindings) { - bindings.forEach(o -> functionBindings.put(o.getOverloadId(), o)); + bindings.forEach(o -> functionBindings.putIfAbsent(o.getOverloadId(), o)); return this; } @@ -168,6 +180,23 @@ public CelRuntimeBuilder setExtensionRegistry(ExtensionRegistry extensionRegistr return this; } + // The following getters exist for asserting immutability for collections held by this builder, + // and shouldn't be exposed to the public. + @VisibleForTesting + Map getFunctionBindings() { + return this.functionBindings; + } + + @VisibleForTesting + ImmutableSet.Builder getRuntimeLibraries() { + return this.celRuntimeLibraries; + } + + @VisibleForTesting + ImmutableSet.Builder getFileTypes() { + return this.fileTypes; + } + /** Build a new {@code CelRuntimeLegacyImpl} instance from the builder config. */ @Override public CelRuntimeLegacyImpl build() { @@ -203,7 +232,8 @@ public CelRuntimeLegacyImpl build() { StandardFunctions.add(dispatcher, dynamicProto, options); } - ImmutableMap functionBindingMap = functionBindings.buildOrThrow(); + ImmutableMap functionBindingMap = + ImmutableMap.copyOf(functionBindings); functionBindingMap.forEach( (String overloadId, CelFunctionBinding func) -> dispatcher.add( @@ -238,7 +268,7 @@ public CelRuntimeLegacyImpl build() { } return new CelRuntimeLegacyImpl( - new DefaultInterpreter(runtimeTypeProvider, dispatcher, options), options); + new DefaultInterpreter(runtimeTypeProvider, dispatcher, options), options, this); } private static CelDescriptorPool newDescriptorPool( @@ -264,15 +294,35 @@ private static ProtoMessageFactory maybeCombineMessageFactory( private Builder() { this.options = CelOptions.newBuilder().build(); this.fileTypes = ImmutableSet.builder(); - this.functionBindings = ImmutableMap.builder(); + this.functionBindings = new HashMap<>(); this.celRuntimeLibraries = ImmutableSet.builder(); this.extensionRegistry = ExtensionRegistry.getEmptyRegistry(); this.customTypeFactory = null; } + + private Builder(Builder builder) { + // The following properties are either immutable or simple primitives, thus can be assigned + // directly. + this.options = builder.options; + this.extensionRegistry = builder.extensionRegistry; + this.customTypeFactory = builder.customTypeFactory; + // The following needs to be deep copied as they are collection builders + this.fileTypes = deepCopy(builder.fileTypes); + this.celRuntimeLibraries = deepCopy(builder.celRuntimeLibraries); + this.functionBindings = new HashMap<>(builder.functionBindings); + } + + private static ImmutableSet.Builder deepCopy(ImmutableSet.Builder builderToCopy) { + ImmutableSet.Builder newBuilder = ImmutableSet.builder(); + newBuilder.addAll(builderToCopy.build()); + return newBuilder; + } } - private CelRuntimeLegacyImpl(Interpreter interpreter, CelOptions options) { + private CelRuntimeLegacyImpl( + Interpreter interpreter, CelOptions options, Builder runtimeBuilder) { this.interpreter = interpreter; this.options = options; + this.runtimeBuilder = new Builder(runtimeBuilder); } } diff --git a/runtime/src/test/java/dev/cel/runtime/CelRuntimeLegacyImplTest.java b/runtime/src/test/java/dev/cel/runtime/CelRuntimeLegacyImplTest.java index 02fd0ae7c..2ce5e4d0b 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelRuntimeLegacyImplTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelRuntimeLegacyImplTest.java @@ -19,6 +19,8 @@ import dev.cel.common.CelException; import dev.cel.compiler.CelCompiler; import dev.cel.compiler.CelCompilerFactory; +import dev.cel.runtime.CelRuntime.CelFunctionBinding; +import dev.cel.testing.testdata.proto3.TestAllTypesProto.TestAllTypes; import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; @@ -35,4 +37,62 @@ public void evalException() throws CelException { CelEvaluationException e = Assert.assertThrows(CelEvaluationException.class, program::eval); assertThat(e).hasCauseThat().isInstanceOf(ArithmeticException.class); } + + @Test + public void toRuntimeBuilder_isNewInstance() { + CelRuntimeBuilder celRuntimeBuilder = CelRuntimeFactory.standardCelRuntimeBuilder(); + CelRuntimeLegacyImpl celRuntime = (CelRuntimeLegacyImpl) celRuntimeBuilder.build(); + + CelRuntimeLegacyImpl.Builder newRuntimeBuilder = + (CelRuntimeLegacyImpl.Builder) celRuntime.toRuntimeBuilder(); + + assertThat(newRuntimeBuilder).isNotEqualTo(celRuntimeBuilder); + } + + @Test + public void toRuntimeBuilder_isImmutable() { + CelRuntimeBuilder originalRuntimeBuilder = CelRuntimeFactory.standardCelRuntimeBuilder(); + CelRuntimeLegacyImpl celRuntime = (CelRuntimeLegacyImpl) originalRuntimeBuilder.build(); + originalRuntimeBuilder.addLibraries(runtimeBuilder -> {}); + + CelRuntimeLegacyImpl.Builder newRuntimeBuilder = + (CelRuntimeLegacyImpl.Builder) celRuntime.toRuntimeBuilder(); + + assertThat(newRuntimeBuilder.getRuntimeLibraries().build()).isEmpty(); + } + + @Test + public void toRuntimeBuilder_collectionProperties_copied() { + CelRuntimeBuilder celRuntimeBuilder = CelRuntimeFactory.standardCelRuntimeBuilder(); + celRuntimeBuilder.addMessageTypes(TestAllTypes.getDescriptor()); + celRuntimeBuilder.addFileTypes(TestAllTypes.getDescriptor().getFile()); + celRuntimeBuilder.addFunctionBindings(CelFunctionBinding.from("test", Integer.class, arg -> 1)); + celRuntimeBuilder.addLibraries(runtimeBuilder -> {}); + CelRuntimeLegacyImpl celRuntime = (CelRuntimeLegacyImpl) celRuntimeBuilder.build(); + + CelRuntimeLegacyImpl.Builder newRuntimeBuilder = + (CelRuntimeLegacyImpl.Builder) celRuntime.toRuntimeBuilder(); + + assertThat(newRuntimeBuilder.getFunctionBindings()).hasSize(1); + assertThat(newRuntimeBuilder.getRuntimeLibraries().build()).hasSize(1); + assertThat(newRuntimeBuilder.getFileTypes().build()).hasSize(6); + } + + @Test + public void toRuntimeBuilder_collectionProperties_areImmutable() { + CelRuntimeBuilder celRuntimeBuilder = CelRuntimeFactory.standardCelRuntimeBuilder(); + CelRuntimeLegacyImpl celRuntime = (CelRuntimeLegacyImpl) celRuntimeBuilder.build(); + CelRuntimeLegacyImpl.Builder newRuntimeBuilder = + (CelRuntimeLegacyImpl.Builder) celRuntime.toRuntimeBuilder(); + + // Mutate the original builder containing collections + celRuntimeBuilder.addMessageTypes(TestAllTypes.getDescriptor()); + celRuntimeBuilder.addFileTypes(TestAllTypes.getDescriptor().getFile()); + celRuntimeBuilder.addFunctionBindings(CelFunctionBinding.from("test", Integer.class, arg -> 1)); + celRuntimeBuilder.addLibraries(runtimeBuilder -> {}); + + assertThat(newRuntimeBuilder.getFunctionBindings()).isEmpty(); + assertThat(newRuntimeBuilder.getRuntimeLibraries().build()).isEmpty(); + assertThat(newRuntimeBuilder.getFileTypes().build()).isEmpty(); + } }