Skip to content

Various fixes to CelEnvironment, YAML parsing and serialization #987

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

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Various fixes to CelEnvironment, YAML parsing and serialization #987

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
57 changes: 43 additions & 14 deletions bundle/src/main/java/dev/cel/bundle/CelEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import dev.cel.common.types.OptionalType;
import dev.cel.common.types.SimpleType;
import dev.cel.common.types.TypeParamType;
import dev.cel.common.types.TypeType;
import dev.cel.compiler.CelCompiler;
import dev.cel.compiler.CelCompilerBuilder;
import dev.cel.compiler.CelCompilerLibrary;
Expand Down Expand Up @@ -71,38 +72,37 @@ public abstract class CelEnvironment {
"math", CanonicalCelExtension.MATH,
"optional", CanonicalCelExtension.OPTIONAL,
"protos", CanonicalCelExtension.PROTOS,
"regex", CanonicalCelExtension.REGEX,
"sets", CanonicalCelExtension.SETS,
"strings", CanonicalCelExtension.STRINGS,
"comprehensions", CanonicalCelExtension.COMPREHENSIONS);
"two-var-comprehensions", CanonicalCelExtension.COMPREHENSIONS);

private static final ImmutableMap<String, ObjIntConsumer<CelOptions.Builder>> LIMIT_HANDLERS =
ImmutableMap.of(
"cel.limit.expression_code_points",
(options, value) -> options.maxExpressionCodePointSize(value),
CelOptions.Builder::maxExpressionCodePointSize,
"cel.limit.parse_error_recovery",
(options, value) -> options.maxParseErrorRecoveryLimit(value),
CelOptions.Builder::maxParseErrorRecoveryLimit,
"cel.limit.parse_recursion_depth",
(options, value) -> options.maxParseRecursionDepth(value));
CelOptions.Builder::maxParseRecursionDepth);

private static final ImmutableMap<String, BooleanOptionConsumer> FEATURE_HANDLERS =
ImmutableMap.of(
"cel.feature.macro_call_tracking",
(options, enabled) -> options.populateMacroCalls(enabled),
CelOptions.Builder::populateMacroCalls,
"cel.feature.backtick_escape_syntax",
(options, enabled) -> options.enableQuotedIdentifierSyntax(enabled),
CelOptions.Builder::enableQuotedIdentifierSyntax,
"cel.feature.cross_type_numeric_comparisons",
(options, enabled) -> options.enableHeterogeneousNumericComparisons(enabled));
CelOptions.Builder::enableHeterogeneousNumericComparisons);

/** Environment source in textual format (ex: textproto, YAML). */
public abstract Optional<Source> source();

/** Name of the environment. */
public abstract String name();

/**
* Container, which captures default namespace and aliases for value resolution.
*/
public abstract CelContainer container();
/** Container, which captures default namespace and aliases for value resolution. */
public abstract Optional<CelContainer> container();

/**
* An optional description of the environment (example: location of the file containing the config
Expand Down Expand Up @@ -226,7 +226,6 @@ public static Builder newBuilder() {
return new AutoValue_CelEnvironment.Builder()
.setName("")
.setDescription("")
.setContainer(CelContainer.ofName(""))
.setVariables(ImmutableSet.of())
.setFunctions(ImmutableSet.of())
.setFeatures(ImmutableSet.of())
Expand All @@ -242,7 +241,6 @@ public CelCompiler extend(CelCompiler celCompiler, CelOptions celOptions)
CelCompilerBuilder compilerBuilder =
celCompiler
.toCompilerBuilder()
.setContainer(container())
.setOptions(celOptions)
.setTypeProvider(celTypeProvider)
.addVarDeclarations(
Expand All @@ -254,6 +252,8 @@ public CelCompiler extend(CelCompiler celCompiler, CelOptions celOptions)
.map(f -> f.toCelFunctionDecl(celTypeProvider))
.collect(toImmutableList()));

container().ifPresent(compilerBuilder::setContainer);

addAllCompilerExtensions(compilerBuilder, celOptions);

applyStandardLibrarySubset(compilerBuilder);
Expand Down Expand Up @@ -416,6 +416,8 @@ public abstract static class VariableDecl {
/** The type of the variable. */
public abstract TypeDecl type();

public abstract Optional<String> description();

/** Builder for {@link VariableDecl}. */
@AutoValue.Builder
public abstract static class Builder implements RequiredFieldsChecker {
Expand All @@ -428,6 +430,8 @@ public abstract static class Builder implements RequiredFieldsChecker {

public abstract VariableDecl.Builder setType(TypeDecl typeDecl);

public abstract VariableDecl.Builder setDescription(String name);

@Override
public ImmutableList<RequiredField> requiredFields() {
return ImmutableList.of(
Expand Down Expand Up @@ -459,6 +463,8 @@ public abstract static class FunctionDecl {

public abstract String name();

public abstract Optional<String> description();

public abstract ImmutableSet<OverloadDecl> overloads();

/** Builder for {@link FunctionDecl}. */
Expand All @@ -471,6 +477,8 @@ public abstract static class Builder implements RequiredFieldsChecker {

public abstract FunctionDecl.Builder setName(String name);

public abstract FunctionDecl.Builder setDescription(String description);

public abstract FunctionDecl.Builder setOverloads(ImmutableSet<OverloadDecl> overloads);

@Override
Expand Down Expand Up @@ -519,6 +527,9 @@ public abstract static class OverloadDecl {
/** List of function overload type values. */
public abstract ImmutableList<TypeDecl> arguments();

/** Examples for the overload. */
public abstract ImmutableList<String> examples();

/** Return type of the overload. Required. */
public abstract TypeDecl returnType();

Expand All @@ -537,8 +548,21 @@ public abstract static class Builder implements RequiredFieldsChecker {
// This should stay package-private to encourage add/set methods to be used instead.
abstract ImmutableList.Builder<TypeDecl> argumentsBuilder();

abstract ImmutableList.Builder<String> examplesBuilder();

public abstract OverloadDecl.Builder setArguments(ImmutableList<TypeDecl> args);

@CanIgnoreReturnValue
public OverloadDecl.Builder addExamples(Iterable<String> examples) {
this.examplesBuilder().addAll(checkNotNull(examples));
return this;
}

@CanIgnoreReturnValue
public OverloadDecl.Builder addExamples(String... examples) {
return addExamples(Arrays.asList(examples));
}

@CanIgnoreReturnValue
public OverloadDecl.Builder addArguments(Iterable<TypeDecl> args) {
this.argumentsBuilder().addAll(checkNotNull(args));
Expand Down Expand Up @@ -667,6 +691,10 @@ public CelType toCelType(CelTypeProvider celTypeProvider) {
CelType keyType = params().get(0).toCelType(celTypeProvider);
CelType valueType = params().get(1).toCelType(celTypeProvider);
return MapType.create(keyType, valueType);
case "type":
checkState(
params().size() == 1, "Expected 1 parameter for type, got %s", params().size());
return TypeType.create(params().get(0).toCelType(celTypeProvider));
default:
if (isTypeParam()) {
return TypeParamType.create(name());
Expand Down Expand Up @@ -838,6 +866,7 @@ enum CanonicalCelExtension {
SETS(
(options, version) -> CelExtensions.sets(options),
(options, version) -> CelExtensions.sets(options)),
REGEX((options, version) -> CelExtensions.regex(), (options, version) -> CelExtensions.regex()),
LISTS((options, version) -> CelExtensions.lists(), (options, version) -> CelExtensions.lists()),
COMPREHENSIONS(
(options, version) -> CelExtensions.comprehensions(),
Expand Down Expand Up @@ -1054,7 +1083,7 @@ public static OverloadSelector.Builder newBuilder() {
}

@FunctionalInterface
private static interface BooleanOptionConsumer {
private interface BooleanOptionConsumer {
void accept(CelOptions.Builder options, boolean value);
}
}
28 changes: 28 additions & 0 deletions bundle/src/main/java/dev/cel/bundle/CelEnvironmentYamlParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,9 @@ private VariableDecl parseVariable(ParserContext<Node> ctx, Node node) {
case "name":
builder.setName(newString(ctx, valueNode));
break;
case "description":
builder.setDescription(newString(ctx, valueNode));
break;
case "type":
if (typeDeclBuilder != null) {
ctx.reportError(
Expand Down Expand Up @@ -428,6 +431,9 @@ private FunctionDecl parseFunction(ParserContext<Node> ctx, Node node) {
case "overloads":
builder.setOverloads(parseOverloads(ctx, valueNode));
break;
case "description":
builder.setDescription(newString(ctx, valueNode).trim());
break;
default:
ctx.reportError(keyId, String.format("Unsupported function tag: %s", keyName));
break;
Expand Down Expand Up @@ -479,6 +485,9 @@ private static ImmutableSet<OverloadDecl> parseOverloads(ParserContext<Node> ctx
case "target":
overloadDeclBuilder.setTarget(parseTypeDecl(ctx, valueNode));
break;
case "examples":
overloadDeclBuilder.addExamples(parseOverloadExamples(ctx, valueNode));
break;
default:
ctx.reportError(keyId, String.format("Unsupported overload tag: %s", fieldName));
break;
Expand All @@ -494,6 +503,25 @@ private static ImmutableSet<OverloadDecl> parseOverloads(ParserContext<Node> ctx
return overloadSetBuilder.build();
}

private static ImmutableList<String> parseOverloadExamples(ParserContext<Node> ctx, Node node) {
long listValueId = ctx.collectMetadata(node);
if (!assertYamlType(ctx, listValueId, node, YamlNodeType.LIST)) {
return ImmutableList.of();
}
SequenceNode paramsListNode = (SequenceNode) node;
ImmutableList.Builder<String> builder = ImmutableList.builder();
for (Node elementNode : paramsListNode.getValue()) {
long elementNodeId = ctx.collectMetadata(elementNode);
if (!assertYamlType(ctx, elementNodeId, elementNode, YamlNodeType.STRING)) {
continue;
}

builder.add(((ScalarNode) elementNode).getValue());
}

return builder.build();
}

private static ImmutableList<TypeDecl> parseOverloadArguments(
ParserContext<Node> ctx, Node node) {
long listValueId = ctx.collectMetadata(node);
Expand Down
6 changes: 2 additions & 4 deletions bundle/src/main/java/dev/cel/bundle/CelEnvironmentYamlSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,8 @@ public Node representData(Object data) {
if (!environment.description().isEmpty()) {
configMap.put("description", environment.description());
}
if (!environment.container().name().isEmpty()
|| !environment.container().abbreviations().isEmpty()
|| !environment.container().aliases().isEmpty()) {
configMap.put("container", environment.container());
if (environment.container().isPresent()) {
configMap.put("container", environment.container().get());
}
if (!environment.extensions().isEmpty()) {
configMap.put("extensions", environment.extensions().asList());
Expand Down
3 changes: 1 addition & 2 deletions bundle/src/test/java/dev/cel/bundle/CelEnvironmentExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ public void container() {

CelEnvironmentExporter exporter = CelEnvironmentExporter.newBuilder().build();
CelEnvironment celEnvironment = exporter.export(cel);
CelContainer container = celEnvironment.container();
CelContainer container = celEnvironment.container().get();
assertThat(container.name()).isEqualTo("cntnr");
assertThat(container.abbreviations()).containsExactly("foo.Bar", "baz.Qux").inOrder();
assertThat(container.aliases()).containsAtLeast("nm", "user.name", "id", "user.id").inOrder();
Expand Down Expand Up @@ -368,4 +368,3 @@ public void options() {
CelEnvironment.Limit.create("cel.limit.parse_recursion_depth", 10));
}
}

15 changes: 7 additions & 8 deletions bundle/src/test/java/dev/cel/bundle/CelEnvironmentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ public void newBuilder_defaults() {
assertThat(environment.source()).isEmpty();
assertThat(environment.name()).isEmpty();
assertThat(environment.description()).isEmpty();
assertThat(environment.container().name()).isEmpty();
assertThat(environment.container().abbreviations()).isEmpty();
assertThat(environment.container().aliases()).isEmpty();
assertThat(environment.container()).isEmpty();
assertThat(environment.extensions()).isEmpty();
assertThat(environment.variables()).isEmpty();
assertThat(environment.functions()).isEmpty();
Expand All @@ -65,10 +63,10 @@ public void container() {
.build())
.build();

assertThat(environment.container().name()).isEqualTo("cntr");
assertThat(environment.container().abbreviations()).containsExactly("foo.Bar", "baz.Qux");
assertThat(environment.container().aliases())
.containsExactly("nm", "user.name", "id", "user.id");
CelContainer container = environment.container().get();
assertThat(container.name()).isEqualTo("cntr");
assertThat(container.abbreviations()).containsExactly("foo.Bar", "baz.Qux");
assertThat(container.aliases()).containsExactly("nm", "user.name", "id", "user.id");
}

@Test
Expand All @@ -81,9 +79,10 @@ public void extend_allExtensions() throws Exception {
ExtensionConfig.latest("math"),
ExtensionConfig.latest("optional"),
ExtensionConfig.latest("protos"),
ExtensionConfig.latest("regex"),
ExtensionConfig.latest("sets"),
ExtensionConfig.latest("strings"),
ExtensionConfig.latest("comprehensions"));
ExtensionConfig.latest("two-var-comprehensions"));
CelEnvironment environment =
CelEnvironment.newBuilder().addExtensions(extensionConfigs).build();

Expand Down
56 changes: 32 additions & 24 deletions bundle/src/test/java/dev/cel/bundle/CelEnvironmentYamlParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,9 +675,7 @@ private enum EnvironmentParseErrorTestcase {
+ " | - version: 0\n"
+ " | ..^"),
ILLEGAL_LIBRARY_SUBSET_TAG(
"name: 'test_suite_name'\n"
+ "stdlib:\n"
+ " unknown_tag: 'test_value'\n",
"name: 'test_suite_name'\n" + "stdlib:\n" + " unknown_tag: 'test_value'\n",
"ERROR: <input>:3:3: Unsupported library subset tag: unknown_tag\n"
+ " | unknown_tag: 'test_value'\n"
+ " | ..^"),
Expand Down Expand Up @@ -859,30 +857,40 @@ private enum EnvironmentYamlResourceTestCase {
.setVariables(
VariableDecl.newBuilder()
.setName("msg")
.setDescription(
"msg represents all possible type permutation which CEL understands from a"
+ " proto perspective")
.setType(TypeDecl.create("cel.expr.conformance.proto3.TestAllTypes"))
.build())
.setFunctions(
FunctionDecl.create(
"isEmpty",
ImmutableSet.of(
OverloadDecl.newBuilder()
.setId("wrapper_string_isEmpty")
.setTarget(TypeDecl.create("google.protobuf.StringValue"))
.setReturnType(TypeDecl.create("bool"))
.build(),
OverloadDecl.newBuilder()
.setId("list_isEmpty")
.setTarget(
TypeDecl.newBuilder()
.setName("list")
.addParams(
TypeDecl.newBuilder()
.setName("T")
.setIsTypeParam(true)
.build())
.build())
.setReturnType(TypeDecl.create("bool"))
.build())))
FunctionDecl.newBuilder()
.setName("isEmpty")
.setDescription(
"determines whether a list is empty,\nor a string has no characters")
.setOverloads(
ImmutableSet.of(
OverloadDecl.newBuilder()
.setId("wrapper_string_isEmpty")
.setTarget(TypeDecl.create("google.protobuf.StringValue"))
.addExamples("''.isEmpty() // true")
.setReturnType(TypeDecl.create("bool"))
.build(),
OverloadDecl.newBuilder()
.setId("list_isEmpty")
.addExamples("[].isEmpty() // true")
.addExamples("[1].isEmpty() // false")
.setTarget(
TypeDecl.newBuilder()
.setName("list")
.addParams(
TypeDecl.newBuilder()
.setName("T")
.setIsTypeParam(true)
.build())
.build())
.setReturnType(TypeDecl.create("bool"))
.build()))
.build())
.setFeatures(CelEnvironment.FeatureFlag.create("cel.feature.macro_call_tracking", true))
.setLimits(
ImmutableSet.of(
Expand Down
Loading
Loading