Skip to content

Commit 3572797

Browse files
l46kokcopybara-github
l46kok
authored and
copybara-github
committed
Various fixes to CelEnvironment, YAML parsing and serialization
Includes: - Container setting is made optional to prevent the CEL environment from overriding an already set container with an empty one in case the env is extended multiple times - Brings CEL environment YAML serialization in parity (examples, type, description) - Fixes "comprehensions" to be "two-var-comprehensions" - Partitioned newVaueString into newYamlString and newSourceString, where the former respects YAML multiline syntax PiperOrigin-RevId: 886408709
1 parent b0a3f58 commit 3572797

File tree

13 files changed

+259
-101
lines changed

13 files changed

+259
-101
lines changed

‎bundle/src/main/java/dev/cel/bundle/CelEnvironment.java‎

Lines changed: 43 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import dev.cel.common.types.OptionalType;
4444
import dev.cel.common.types.SimpleType;
4545
import dev.cel.common.types.TypeParamType;
46+
import dev.cel.common.types.TypeType;
4647
import dev.cel.compiler.CelCompiler;
4748
import dev.cel.compiler.CelCompilerBuilder;
4849
import dev.cel.compiler.CelCompilerLibrary;
@@ -71,38 +72,37 @@ public abstract class CelEnvironment {
7172
"math", CanonicalCelExtension.MATH,
7273
"optional", CanonicalCelExtension.OPTIONAL,
7374
"protos", CanonicalCelExtension.PROTOS,
75+
"regex", CanonicalCelExtension.REGEX,
7476
"sets", CanonicalCelExtension.SETS,
7577
"strings", CanonicalCelExtension.STRINGS,
76-
"comprehensions", CanonicalCelExtension.COMPREHENSIONS);
78+
"two-var-comprehensions", CanonicalCelExtension.COMPREHENSIONS);
7779

7880
private static final ImmutableMap<String, ObjIntConsumer<CelOptions.Builder>> LIMIT_HANDLERS =
7981
ImmutableMap.of(
8082
"cel.limit.expression_code_points",
81-
(options, value) -> options.maxExpressionCodePointSize(value),
83+
CelOptions.Builder::maxExpressionCodePointSize,
8284
"cel.limit.parse_error_recovery",
83-
(options, value) -> options.maxParseErrorRecoveryLimit(value),
85+
CelOptions.Builder::maxParseErrorRecoveryLimit,
8486
"cel.limit.parse_recursion_depth",
85-
(options, value) -> options.maxParseRecursionDepth(value));
87+
CelOptions.Builder::maxParseRecursionDepth);
8688

8789
private static final ImmutableMap<String, BooleanOptionConsumer> FEATURE_HANDLERS =
8890
ImmutableMap.of(
8991
"cel.feature.macro_call_tracking",
90-
(options, enabled) -> options.populateMacroCalls(enabled),
92+
CelOptions.Builder::populateMacroCalls,
9193
"cel.feature.backtick_escape_syntax",
92-
(options, enabled) -> options.enableQuotedIdentifierSyntax(enabled),
94+
CelOptions.Builder::enableQuotedIdentifierSyntax,
9395
"cel.feature.cross_type_numeric_comparisons",
94-
(options, enabled) -> options.enableHeterogeneousNumericComparisons(enabled));
96+
CelOptions.Builder::enableHeterogeneousNumericComparisons);
9597

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

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

102-
/**
103-
* Container, which captures default namespace and aliases for value resolution.
104-
*/
105-
public abstract CelContainer container();
104+
/** Container, which captures default namespace and aliases for value resolution. */
105+
public abstract Optional<CelContainer> container();
106106

107107
/**
108108
* An optional description of the environment (example: location of the file containing the config
@@ -226,7 +226,6 @@ public static Builder newBuilder() {
226226
return new AutoValue_CelEnvironment.Builder()
227227
.setName("")
228228
.setDescription("")
229-
.setContainer(CelContainer.ofName(""))
230229
.setVariables(ImmutableSet.of())
231230
.setFunctions(ImmutableSet.of())
232231
.setFeatures(ImmutableSet.of())
@@ -242,7 +241,6 @@ public CelCompiler extend(CelCompiler celCompiler, CelOptions celOptions)
242241
CelCompilerBuilder compilerBuilder =
243242
celCompiler
244243
.toCompilerBuilder()
245-
.setContainer(container())
246244
.setOptions(celOptions)
247245
.setTypeProvider(celTypeProvider)
248246
.addVarDeclarations(
@@ -254,6 +252,8 @@ public CelCompiler extend(CelCompiler celCompiler, CelOptions celOptions)
254252
.map(f -> f.toCelFunctionDecl(celTypeProvider))
255253
.collect(toImmutableList()));
256254

255+
container().ifPresent(compilerBuilder::setContainer);
256+
257257
addAllCompilerExtensions(compilerBuilder, celOptions);
258258

259259
applyStandardLibrarySubset(compilerBuilder);
@@ -416,6 +416,8 @@ public abstract static class VariableDecl {
416416
/** The type of the variable. */
417417
public abstract TypeDecl type();
418418

419+
public abstract Optional<String> description();
420+
419421
/** Builder for {@link VariableDecl}. */
420422
@AutoValue.Builder
421423
public abstract static class Builder implements RequiredFieldsChecker {
@@ -428,6 +430,8 @@ public abstract static class Builder implements RequiredFieldsChecker {
428430

429431
public abstract VariableDecl.Builder setType(TypeDecl typeDecl);
430432

433+
public abstract VariableDecl.Builder setDescription(String name);
434+
431435
@Override
432436
public ImmutableList<RequiredField> requiredFields() {
433437
return ImmutableList.of(
@@ -459,6 +463,8 @@ public abstract static class FunctionDecl {
459463

460464
public abstract String name();
461465

466+
public abstract Optional<String> description();
467+
462468
public abstract ImmutableSet<OverloadDecl> overloads();
463469

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

472478
public abstract FunctionDecl.Builder setName(String name);
473479

480+
public abstract FunctionDecl.Builder setDescription(String description);
481+
474482
public abstract FunctionDecl.Builder setOverloads(ImmutableSet<OverloadDecl> overloads);
475483

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

530+
/** Examples for the overload. */
531+
public abstract ImmutableList<String> examples();
532+
522533
/** Return type of the overload. Required. */
523534
public abstract TypeDecl returnType();
524535

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

551+
abstract ImmutableList.Builder<String> examplesBuilder();
552+
540553
public abstract OverloadDecl.Builder setArguments(ImmutableList<TypeDecl> args);
541554

555+
@CanIgnoreReturnValue
556+
public OverloadDecl.Builder addExamples(Iterable<String> examples) {
557+
this.examplesBuilder().addAll(checkNotNull(examples));
558+
return this;
559+
}
560+
561+
@CanIgnoreReturnValue
562+
public OverloadDecl.Builder addExamples(String... examples) {
563+
return addExamples(Arrays.asList(examples));
564+
}
565+
542566
@CanIgnoreReturnValue
543567
public OverloadDecl.Builder addArguments(Iterable<TypeDecl> args) {
544568
this.argumentsBuilder().addAll(checkNotNull(args));
@@ -667,6 +691,10 @@ public CelType toCelType(CelTypeProvider celTypeProvider) {
667691
CelType keyType = params().get(0).toCelType(celTypeProvider);
668692
CelType valueType = params().get(1).toCelType(celTypeProvider);
669693
return MapType.create(keyType, valueType);
694+
case "type":
695+
checkState(
696+
params().size() == 1, "Expected 1 parameter for type, got %s", params().size());
697+
return TypeType.create(params().get(0).toCelType(celTypeProvider));
670698
default:
671699
if (isTypeParam()) {
672700
return TypeParamType.create(name());
@@ -838,6 +866,7 @@ enum CanonicalCelExtension {
838866
SETS(
839867
(options, version) -> CelExtensions.sets(options),
840868
(options, version) -> CelExtensions.sets(options)),
869+
REGEX((options, version) -> CelExtensions.regex(), (options, version) -> CelExtensions.regex()),
841870
LISTS((options, version) -> CelExtensions.lists(), (options, version) -> CelExtensions.lists()),
842871
COMPREHENSIONS(
843872
(options, version) -> CelExtensions.comprehensions(),
@@ -1054,7 +1083,7 @@ public static OverloadSelector.Builder newBuilder() {
10541083
}
10551084

10561085
@FunctionalInterface
1057-
private static interface BooleanOptionConsumer {
1086+
private interface BooleanOptionConsumer {
10581087
void accept(CelOptions.Builder options, boolean value);
10591088
}
10601089
}

‎bundle/src/main/java/dev/cel/bundle/CelEnvironmentYamlParser.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,9 @@ private VariableDecl parseVariable(ParserContext<Node> ctx, Node node) {
353353
case "name":
354354
builder.setName(newString(ctx, valueNode));
355355
break;
356+
case "description":
357+
builder.setDescription(newString(ctx, valueNode));
358+
break;
356359
case "type":
357360
if (typeDeclBuilder != null) {
358361
ctx.reportError(
@@ -428,6 +431,9 @@ private FunctionDecl parseFunction(ParserContext<Node> ctx, Node node) {
428431
case "overloads":
429432
builder.setOverloads(parseOverloads(ctx, valueNode));
430433
break;
434+
case "description":
435+
builder.setDescription(newString(ctx, valueNode).trim());
436+
break;
431437
default:
432438
ctx.reportError(keyId, String.format("Unsupported function tag: %s", keyName));
433439
break;
@@ -479,6 +485,9 @@ private static ImmutableSet<OverloadDecl> parseOverloads(ParserContext<Node> ctx
479485
case "target":
480486
overloadDeclBuilder.setTarget(parseTypeDecl(ctx, valueNode));
481487
break;
488+
case "examples":
489+
overloadDeclBuilder.addExamples(parseOverloadExamples(ctx, valueNode));
490+
break;
482491
default:
483492
ctx.reportError(keyId, String.format("Unsupported overload tag: %s", fieldName));
484493
break;
@@ -494,6 +503,25 @@ private static ImmutableSet<OverloadDecl> parseOverloads(ParserContext<Node> ctx
494503
return overloadSetBuilder.build();
495504
}
496505

506+
private static ImmutableList<String> parseOverloadExamples(ParserContext<Node> ctx, Node node) {
507+
long listValueId = ctx.collectMetadata(node);
508+
if (!assertYamlType(ctx, listValueId, node, YamlNodeType.LIST)) {
509+
return ImmutableList.of();
510+
}
511+
SequenceNode paramsListNode = (SequenceNode) node;
512+
ImmutableList.Builder<String> builder = ImmutableList.builder();
513+
for (Node elementNode : paramsListNode.getValue()) {
514+
long elementNodeId = ctx.collectMetadata(elementNode);
515+
if (!assertYamlType(ctx, elementNodeId, elementNode, YamlNodeType.STRING)) {
516+
continue;
517+
}
518+
519+
builder.add(((ScalarNode) elementNode).getValue());
520+
}
521+
522+
return builder.build();
523+
}
524+
497525
private static ImmutableList<TypeDecl> parseOverloadArguments(
498526
ParserContext<Node> ctx, Node node) {
499527
long listValueId = ctx.collectMetadata(node);

‎bundle/src/main/java/dev/cel/bundle/CelEnvironmentYamlSerializer.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,8 @@ public Node representData(Object data) {
7979
if (!environment.description().isEmpty()) {
8080
configMap.put("description", environment.description());
8181
}
82-
if (!environment.container().name().isEmpty()
83-
|| !environment.container().abbreviations().isEmpty()
84-
|| !environment.container().aliases().isEmpty()) {
85-
configMap.put("container", environment.container());
82+
if (environment.container().isPresent()) {
83+
configMap.put("container", environment.container().get());
8684
}
8785
if (!environment.extensions().isEmpty()) {
8886
configMap.put("extensions", environment.extensions().asList());

‎bundle/src/test/java/dev/cel/bundle/CelEnvironmentExporterTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ public void container() {
333333

334334
CelEnvironmentExporter exporter = CelEnvironmentExporter.newBuilder().build();
335335
CelEnvironment celEnvironment = exporter.export(cel);
336-
CelContainer container = celEnvironment.container();
336+
CelContainer container = celEnvironment.container().get();
337337
assertThat(container.name()).isEqualTo("cntnr");
338338
assertThat(container.abbreviations()).containsExactly("foo.Bar", "baz.Qux").inOrder();
339339
assertThat(container.aliases()).containsAtLeast("nm", "user.name", "id", "user.id").inOrder();
@@ -368,4 +368,3 @@ public void options() {
368368
CelEnvironment.Limit.create("cel.limit.parse_recursion_depth", 10));
369369
}
370370
}
371-

‎bundle/src/test/java/dev/cel/bundle/CelEnvironmentTest.java‎

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@
2828
import dev.cel.common.CelOptions;
2929
import dev.cel.common.CelValidationException;
3030
import dev.cel.common.CelValidationResult;
31+
import dev.cel.common.types.CelType;
32+
import dev.cel.common.types.CelTypeProvider;
33+
import dev.cel.common.types.SimpleType;
34+
import dev.cel.common.types.TypeType;
3135
import dev.cel.compiler.CelCompiler;
3236
import dev.cel.compiler.CelCompilerFactory;
3337
import dev.cel.parser.CelStandardMacro;
@@ -44,9 +48,7 @@ public void newBuilder_defaults() {
4448
assertThat(environment.source()).isEmpty();
4549
assertThat(environment.name()).isEmpty();
4650
assertThat(environment.description()).isEmpty();
47-
assertThat(environment.container().name()).isEmpty();
48-
assertThat(environment.container().abbreviations()).isEmpty();
49-
assertThat(environment.container().aliases()).isEmpty();
51+
assertThat(environment.container()).isEmpty();
5052
assertThat(environment.extensions()).isEmpty();
5153
assertThat(environment.variables()).isEmpty();
5254
assertThat(environment.functions()).isEmpty();
@@ -65,10 +67,10 @@ public void container() {
6567
.build())
6668
.build();
6769

68-
assertThat(environment.container().name()).isEqualTo("cntr");
69-
assertThat(environment.container().abbreviations()).containsExactly("foo.Bar", "baz.Qux");
70-
assertThat(environment.container().aliases())
71-
.containsExactly("nm", "user.name", "id", "user.id");
70+
CelContainer container = environment.container().get();
71+
assertThat(container.name()).isEqualTo("cntr");
72+
assertThat(container.abbreviations()).containsExactly("foo.Bar", "baz.Qux");
73+
assertThat(container.aliases()).containsExactly("nm", "user.name", "id", "user.id");
7274
}
7375

7476
@Test
@@ -81,9 +83,10 @@ public void extend_allExtensions() throws Exception {
8183
ExtensionConfig.latest("math"),
8284
ExtensionConfig.latest("optional"),
8385
ExtensionConfig.latest("protos"),
86+
ExtensionConfig.latest("regex"),
8487
ExtensionConfig.latest("sets"),
8588
ExtensionConfig.latest("strings"),
86-
ExtensionConfig.latest("comprehensions"));
89+
ExtensionConfig.latest("two-var-comprehensions"));
8790
CelEnvironment environment =
8891
CelEnvironment.newBuilder().addExtensions(extensionConfigs).build();
8992

@@ -435,4 +438,30 @@ public void stdlibSubset_functionOverloadExcluded() throws Exception {
435438
result = extendedCompiler.compile("1 == 1 && 1 != 1 + 1");
436439
assertThat(result.getErrorString()).contains("found no matching overload for '_+_'");
437440
}
441+
442+
@Test
443+
public void typeDecl_toCelType_type() {
444+
CelTypeProvider typeProvider =
445+
CelCompilerFactory.standardCelCompilerBuilder().build().getTypeProvider();
446+
CelEnvironment.TypeDecl typeDecl =
447+
CelEnvironment.TypeDecl.newBuilder()
448+
.setName("type")
449+
.addParams(CelEnvironment.TypeDecl.create("int"))
450+
.build();
451+
452+
CelType celType = typeDecl.toCelType(typeProvider);
453+
454+
assertThat(celType).isEqualTo(TypeType.create(SimpleType.INT));
455+
}
456+
457+
@Test
458+
public void typeDecl_toCelType_type_wrongParamCount_throws() {
459+
CelTypeProvider typeProvider =
460+
CelCompilerFactory.standardCelCompilerBuilder().build().getTypeProvider();
461+
CelEnvironment.TypeDecl typeDecl = CelEnvironment.TypeDecl.newBuilder().setName("type").build();
462+
463+
IllegalStateException e =
464+
assertThrows(IllegalStateException.class, () -> typeDecl.toCelType(typeProvider));
465+
assertThat(e).hasMessageThat().contains("Expected 1 parameter for type, got 0");
466+
}
438467
}

0 commit comments

Comments
 (0)