From 1fa54c961493bad5feff62f4b7eb502fb23b72c9 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Thu, 24 Aug 2023 11:42:47 -0700 Subject: [PATCH] Add shorthands for retrieving CelExpr variants or its default instance. This is useful to avoid having to check for the underlying kind before accessing its properties. Example: expr.getKind().equals(Kind.CALL) && expr.call().function().equals("foo") We can instead write: expr.callOrDefault().function().equals("foo") PiperOrigin-RevId: 559821213 --- .../java/dev/cel/common/ast/CelConstant.java | 18 +++ .../main/java/dev/cel/common/ast/CelExpr.java | 144 +++++++++++++++++- .../dev/cel/common/ast/CelExprConverter.java | 4 + .../common/ast/CelExprV1Alpha1Converter.java | 4 + .../dev/cel/common/ast/CelConstantTest.java | 8 + .../cel/common/ast/CelExprConverterTest.java | 3 + .../java/dev/cel/common/ast/CelExprTest.java | 102 +++++++++++++ .../ast/CelExprV1Alpha1ConverterTest.java | 3 + 8 files changed, 282 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/dev/cel/common/ast/CelConstant.java b/common/src/main/java/dev/cel/common/ast/CelConstant.java index 33f00e30..a21639fc 100644 --- a/common/src/main/java/dev/cel/common/ast/CelConstant.java +++ b/common/src/main/java/dev/cel/common/ast/CelConstant.java @@ -15,6 +15,7 @@ package dev.cel.common.ast; import com.google.auto.value.AutoOneOf; +import com.google.auto.value.AutoValue; import com.google.common.primitives.UnsignedLong; import com.google.errorprone.annotations.Immutable; import com.google.protobuf.ByteString; @@ -34,6 +35,7 @@ public abstract class CelConstant { /** Represents the type of the Constant */ public enum Kind { + NOT_SET, NULL_VALUE, BOOLEAN_VALUE, INT64_VALUE, @@ -55,6 +57,18 @@ public enum Kind { public abstract Kind getKind(); + /** + * An unset constant. + * + *

As the name implies, this constant does nothing. This is used to represent a default + * instance of CelConstant. + */ + @AutoValue + @Immutable + public abstract static class CelConstantNotSet {} + + public abstract CelConstantNotSet notSet(); + public abstract NullValue nullValue(); public abstract boolean booleanValue(); @@ -81,6 +95,10 @@ public enum Kind { @Deprecated public abstract Duration durationValue(); + public static CelConstant ofNotSet() { + return AutoOneOf_CelConstant.notSet(new AutoValue_CelConstant_CelConstantNotSet()); + } + public static CelConstant ofValue(NullValue value) { return AutoOneOf_CelConstant.nullValue(value); } diff --git a/common/src/main/java/dev/cel/common/ast/CelExpr.java b/common/src/main/java/dev/cel/common/ast/CelExpr.java index a57462b3..3e9f0bc1 100644 --- a/common/src/main/java/dev/cel/common/ast/CelExpr.java +++ b/common/src/main/java/dev/cel/common/ast/CelExpr.java @@ -23,6 +23,7 @@ import com.google.errorprone.annotations.CheckReturnValue; import com.google.errorprone.annotations.Immutable; import dev.cel.common.annotations.Internal; +import dev.cel.common.ast.CelExpr.ExprKind.Kind; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -50,42 +51,167 @@ @Immutable public abstract class CelExpr { + /** + * Required. An id assigned to this node by the parser which is unique in a given expression tree. + * This is used to associate type information and other attributes to a node in the parse tree. + */ public abstract long id(); + /** Represents the variant of the expression. */ public abstract ExprKind exprKind(); + /** + * Gets the underlying constant expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#CONSTANT}. + */ public CelConstant constant() { return exprKind().constant(); } + /** + * Gets the underlying identifier expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#IDENT}. + */ public CelIdent ident() { return exprKind().ident(); } + /** + * Gets the underlying select expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#SELECT}. + */ public CelSelect select() { return exprKind().select(); } + /** + * Gets the underlying call expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#CALL}. + */ public CelCall call() { return exprKind().call(); } + /** + * Gets the underlying createList expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#CREATE_LIST}. + */ public CelCreateList createList() { return exprKind().createList(); } + /** + * Gets the underlying createStruct expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#CREATE_STRUCT}. + */ public CelCreateStruct createStruct() { return exprKind().createStruct(); } + /** + * Gets the underlying createMap expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#createMap}. + */ public CelCreateMap createMap() { return exprKind().createMap(); } + /** + * Gets the underlying comprehension expression. + * + * @throws UnsupportedOperationException if expression is not {@link Kind#COMPREHENSION}. + */ public CelComprehension comprehension() { return exprKind().comprehension(); } + /** + * Gets the underlying constant expression or a default instance of one if expression is not + * {@link Kind#CONSTANT}. + */ + public CelConstant constantOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.CONSTANT) + ? exprKind().constant() + : CelConstant.ofNotSet(); + } + + /** + * Gets the underlying identifier expression or a default instance of one if expression is not + * {@link Kind#IDENT}. + */ + public CelIdent identOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.IDENT) + ? exprKind().ident() + : CelIdent.newBuilder().build(); + } + + /** + * Gets the underlying select expression or a default instance of one if expression is not {@link + * Kind#SELECT}. + */ + public CelSelect selectOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.SELECT) + ? exprKind().select() + : CelSelect.newBuilder().build(); + } + + /** + * Gets the underlying call expression or a default instance of one if expression is not {@link + * Kind#CALL}. + */ + public CelCall callOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.CALL) + ? exprKind().call() + : CelCall.newBuilder().build(); + } + + /** + * Gets the underlying createList expression or a default instance of one if expression is not + * {@link Kind#CREATE_LIST}. + */ + public CelCreateList createListOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.CREATE_LIST) + ? exprKind().createList() + : CelCreateList.newBuilder().build(); + } + + /** + * Gets the underlying createStruct expression or a default instance of one if expression is not + * {@link Kind#CREATE_STRUCT}. + */ + public CelCreateStruct createStructOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.CREATE_STRUCT) + ? exprKind().createStruct() + : CelCreateStruct.newBuilder().build(); + } + + /** + * Gets the underlying createMap expression or a default instance of one if expression is not + * {@link Kind#CREATE_MAP}. + */ + public CelCreateMap createMapOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.CREATE_MAP) + ? exprKind().createMap() + : CelCreateMap.newBuilder().build(); + } + + /** + * Gets the underlying comprehension expression or a default instance of one if expression is not + * {@link Kind#COMPREHENSION}. + */ + public CelComprehension comprehensionOrDefault() { + return exprKind().getKind().equals(ExprKind.Kind.COMPREHENSION) + ? exprKind().comprehension() + : CelComprehension.newBuilder().build(); + } + /** Builder for CelExpr. */ @AutoValue.Builder public abstract static class Builder { @@ -212,7 +338,7 @@ public abstract static class Builder { public abstract Builder toBuilder(); public static Builder newBuilder() { - return new AutoValue_CelExpr_CelIdent.Builder(); + return new AutoValue_CelExpr_CelIdent.Builder().setName(""); } } @@ -261,7 +387,10 @@ public abstract static class Builder { public abstract Builder toBuilder(); public static Builder newBuilder() { - return new AutoValue_CelExpr_CelSelect.Builder().setTestOnly(false); + return new AutoValue_CelExpr_CelSelect.Builder() + .setField("") + .setOperand(CelExpr.newBuilder().build()) + .setTestOnly(false); } } @@ -345,7 +474,7 @@ public Builder toBuilder() { } public static Builder newBuilder() { - return new AutoValue_CelExpr_CelCall.Builder(); + return new AutoValue_CelExpr_CelCall.Builder().setFunction(""); } } @@ -761,7 +890,14 @@ public abstract static class Builder { public abstract Builder toBuilder(); public static Builder newBuilder() { - return new AutoValue_CelExpr_CelComprehension.Builder(); + return new AutoValue_CelExpr_CelComprehension.Builder() + .setIterVar("") + .setIterRange(CelExpr.newBuilder().build()) + .setAccuVar("") + .setAccuInit(CelExpr.newBuilder().build()) + .setLoopCondition(CelExpr.newBuilder().build()) + .setLoopStep(CelExpr.newBuilder().build()) + .setResult(CelExpr.newBuilder().build()); } } diff --git a/common/src/main/java/dev/cel/common/ast/CelExprConverter.java b/common/src/main/java/dev/cel/common/ast/CelExprConverter.java index afa53e1c..a17350cc 100644 --- a/common/src/main/java/dev/cel/common/ast/CelExprConverter.java +++ b/common/src/main/java/dev/cel/common/ast/CelExprConverter.java @@ -174,6 +174,8 @@ public static CelReference exprReferenceToCelReference(Reference reference) { */ public static CelConstant exprConstantToCelConstant(Constant constExpr) { switch (constExpr.getConstantKindCase()) { + case CONSTANTKIND_NOT_SET: + return CelConstant.ofNotSet(); case NULL_VALUE: return CelConstant.ofValue(constExpr.getNullValue()); case BOOL_VALUE: @@ -243,6 +245,8 @@ private static Expr.Builder newExprBuilder(CelExpr expr) { */ public static Constant celConstantToExprConstant(CelConstant celConstant) { switch (celConstant.getKind()) { + case NOT_SET: + return Constant.getDefaultInstance(); case NULL_VALUE: return Constant.newBuilder().setNullValue(celConstant.nullValue()).build(); case BOOLEAN_VALUE: diff --git a/common/src/main/java/dev/cel/common/ast/CelExprV1Alpha1Converter.java b/common/src/main/java/dev/cel/common/ast/CelExprV1Alpha1Converter.java index 232f119c..377910c0 100644 --- a/common/src/main/java/dev/cel/common/ast/CelExprV1Alpha1Converter.java +++ b/common/src/main/java/dev/cel/common/ast/CelExprV1Alpha1Converter.java @@ -174,6 +174,8 @@ public static CelReference exprReferenceToCelReference(Reference reference) { */ public static CelConstant exprConstantToCelConstant(Constant constExpr) { switch (constExpr.getConstantKindCase()) { + case CONSTANTKIND_NOT_SET: + return CelConstant.ofNotSet(); case NULL_VALUE: return CelConstant.ofValue(constExpr.getNullValue()); case BOOL_VALUE: @@ -243,6 +245,8 @@ private static Expr.Builder newExprBuilder(CelExpr expr) { */ public static Constant celConstantToExprConstant(CelConstant celConstant) { switch (celConstant.getKind()) { + case NOT_SET: + return Constant.getDefaultInstance(); case NULL_VALUE: return Constant.newBuilder().setNullValue(celConstant.nullValue()).build(); case BOOLEAN_VALUE: diff --git a/common/src/test/java/dev/cel/common/ast/CelConstantTest.java b/common/src/test/java/dev/cel/common/ast/CelConstantTest.java index 26d338b1..abf09ac4 100644 --- a/common/src/test/java/dev/cel/common/ast/CelConstantTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelConstantTest.java @@ -137,6 +137,14 @@ public void constructTimestampValue() { .isEqualTo(Timestamp.newBuilder().setSeconds(100L).build()); } + @Test + public void constructNotSetConstant() { + CelConstant constant = CelConstant.ofNotSet(); + + assertThat(constant).isNotNull(); + assertThat(constant.getKind()).isEqualTo(Kind.NOT_SET); + } + private enum CelConstantTestCase { NULL(CelConstant.ofValue(NullValue.NULL_VALUE)), BOOLEAN(CelConstant.ofValue(true)), diff --git a/common/src/test/java/dev/cel/common/ast/CelExprConverterTest.java b/common/src/test/java/dev/cel/common/ast/CelExprConverterTest.java index 29fea13e..55820c3a 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprConverterTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprConverterTest.java @@ -41,6 +41,9 @@ @RunWith(TestParameterInjector.class) public class CelExprConverterTest { private enum ConstantTestCase { + NOT_SET( + Expr.newBuilder().setId(1).setConstExpr(Constant.getDefaultInstance()).build(), + CelExpr.ofConstantExpr(1, CelConstant.ofNotSet())), NULL( Expr.newBuilder() .setId(1) diff --git a/common/src/test/java/dev/cel/common/ast/CelExprTest.java b/common/src/test/java/dev/cel/common/ast/CelExprTest.java index bbd206de..42a40d7a 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprTest.java @@ -15,12 +15,14 @@ package dev.cel.common.ast; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import dev.cel.common.ast.CelExpr.CelCall; import dev.cel.common.ast.CelExpr.CelComprehension; import dev.cel.common.ast.CelExpr.CelCreateList; +import dev.cel.common.ast.CelExpr.CelCreateMap; import dev.cel.common.ast.CelExpr.CelCreateStruct; import dev.cel.common.ast.CelExpr.CelIdent; import dev.cel.common.ast.CelExpr.CelSelect; @@ -64,6 +66,9 @@ private enum BuilderExprKindTestCase { .build()) .build(), Kind.SELECT), + CREATE_MAP( + CelExpr.newBuilder().setCreateMap(CelCreateMap.newBuilder().build()).build(), + Kind.CREATE_MAP), CREATE_LIST( CelExpr.newBuilder().setCreateList(CelCreateList.newBuilder().build()).build(), Kind.CREATE_LIST), @@ -305,4 +310,101 @@ public void celExprBuilder_setComprehension() { assertThat(celExpr.comprehension()).isEqualTo(celComprehension); } + + @Test + public void getUnderlyingExpression_unmatchedKind_throws( + @TestParameter BuilderExprKindTestCase testCase) { + if (!testCase.expectedExprKind.equals(Kind.NOT_SET)) { + assertThrows(UnsupportedOperationException.class, () -> testCase.expr.exprKind().notSet()); + } + if (!testCase.expectedExprKind.equals(Kind.CONSTANT)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::constant); + } + if (!testCase.expectedExprKind.equals(Kind.IDENT)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::ident); + } + if (!testCase.expectedExprKind.equals(Kind.SELECT)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::select); + } + if (!testCase.expectedExprKind.equals(Kind.CALL)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::call); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_LIST)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::createList); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_STRUCT)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::createStruct); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_MAP)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::createMap); + } + if (!testCase.expectedExprKind.equals(Kind.COMPREHENSION)) { + assertThrows(UnsupportedOperationException.class, testCase.expr::comprehension); + } + } + + @Test + public void getDefault_unmatchedKind_returnsDefaultInstance( + @TestParameter BuilderExprKindTestCase testCase) { + if (!testCase.expectedExprKind.equals(Kind.CONSTANT)) { + assertThat(testCase.expr.constantOrDefault()).isEqualTo(CelConstant.ofNotSet()); + } + if (!testCase.expectedExprKind.equals(Kind.IDENT)) { + assertThat(testCase.expr.identOrDefault()).isEqualTo(CelIdent.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.SELECT)) { + assertThat(testCase.expr.selectOrDefault()).isEqualTo(CelSelect.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.CALL)) { + assertThat(testCase.expr.callOrDefault()).isEqualTo(CelCall.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_LIST)) { + assertThat(testCase.expr.createListOrDefault()).isEqualTo(CelCreateList.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_STRUCT)) { + assertThat(testCase.expr.createStructOrDefault()) + .isEqualTo(CelCreateStruct.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.CREATE_MAP)) { + assertThat(testCase.expr.createMapOrDefault()).isEqualTo(CelCreateMap.newBuilder().build()); + } + if (!testCase.expectedExprKind.equals(Kind.COMPREHENSION)) { + assertThat(testCase.expr.comprehensionOrDefault()) + .isEqualTo(CelComprehension.newBuilder().build()); + } + } + + @Test + public void getDefault_matchedKind_returnsUnderlyingExpression( + @TestParameter BuilderExprKindTestCase testCase) { + switch (testCase.expectedExprKind) { + case NOT_SET: + // no-op + break; + case CONSTANT: + assertThat(testCase.expr.constantOrDefault()).isEqualTo(testCase.expr.constant()); + break; + case IDENT: + assertThat(testCase.expr.identOrDefault()).isEqualTo(testCase.expr.ident()); + break; + case SELECT: + assertThat(testCase.expr.selectOrDefault()).isEqualTo(testCase.expr.select()); + break; + case CALL: + assertThat(testCase.expr.callOrDefault()).isEqualTo(testCase.expr.call()); + break; + case CREATE_LIST: + assertThat(testCase.expr.createListOrDefault()).isEqualTo(testCase.expr.createList()); + break; + case CREATE_STRUCT: + assertThat(testCase.expr.createStructOrDefault()).isEqualTo(testCase.expr.createStruct()); + break; + case CREATE_MAP: + assertThat(testCase.expr.createMapOrDefault()).isEqualTo(testCase.expr.createMap()); + break; + case COMPREHENSION: + assertThat(testCase.expr.comprehensionOrDefault()).isEqualTo(testCase.expr.comprehension()); + break; + } + } } diff --git a/common/src/test/java/dev/cel/common/ast/CelExprV1Alpha1ConverterTest.java b/common/src/test/java/dev/cel/common/ast/CelExprV1Alpha1ConverterTest.java index 339aad63..283fecaf 100644 --- a/common/src/test/java/dev/cel/common/ast/CelExprV1Alpha1ConverterTest.java +++ b/common/src/test/java/dev/cel/common/ast/CelExprV1Alpha1ConverterTest.java @@ -41,6 +41,9 @@ @RunWith(TestParameterInjector.class) public class CelExprV1Alpha1ConverterTest { private enum ConstantTestCase { + NOT_SET( + Expr.newBuilder().setId(1).setConstExpr(Constant.getDefaultInstance()).build(), + CelExpr.ofConstantExpr(1, CelConstant.ofNotSet())), NULL( Expr.newBuilder() .setId(1)