diff --git a/common/src/main/java/dev/cel/common/values/CelValueConverter.java b/common/src/main/java/dev/cel/common/values/CelValueConverter.java index fda014f31..2af0a76cb 100644 --- a/common/src/main/java/dev/cel/common/values/CelValueConverter.java +++ b/common/src/main/java/dev/cel/common/values/CelValueConverter.java @@ -41,24 +41,39 @@ public static CelValueConverter getDefaultInstance() { return DEFAULT_INSTANCE; } - /** Adapts a {@link CelValue} to a plain old Java Object. */ - public Object unwrap(CelValue celValue) { - Preconditions.checkNotNull(celValue); + /** + * Unwraps the {@code value} into its plain old Java Object representation. + * + *

The value may be a {@link CelValue}, a {@link Collection} or a {@link Map}. + */ + public Object maybeUnwrap(Object value) { + if (value instanceof CelValue) { + return unwrap((CelValue) value); + } - if (celValue instanceof OptionalValue) { - OptionalValue optionalValue = (OptionalValue) celValue; - if (optionalValue.isZeroValue()) { - return Optional.empty(); + if (value instanceof Collection) { + Collection collection = (Collection) value; + ImmutableList.Builder builder = + ImmutableList.builderWithExpectedSize(collection.size()); + for (Object element : collection) { + builder.add(maybeUnwrap(element)); } - return Optional.of(optionalValue.value()); + return builder.build(); } - if (celValue instanceof ErrorValue) { - return celValue; + if (value instanceof Map) { + Map map = (Map) value; + ImmutableMap.Builder builder = + ImmutableMap.builderWithExpectedSize(map.size()); + for (Map.Entry entry : map.entrySet()) { + builder.put(maybeUnwrap(entry.getKey()), maybeUnwrap(entry.getValue())); + } + + return builder.buildOrThrow(); } - return celValue.value(); + return value; } /** @@ -101,6 +116,26 @@ protected Object normalizePrimitive(Object value) { return value; } + /** Adapts a {@link CelValue} to a plain old Java Object. */ + private static Object unwrap(CelValue celValue) { + Preconditions.checkNotNull(celValue); + + if (celValue instanceof OptionalValue) { + OptionalValue optionalValue = (OptionalValue) celValue; + if (optionalValue.isZeroValue()) { + return Optional.empty(); + } + + return Optional.of(optionalValue.value()); + } + + if (celValue instanceof ErrorValue) { + return celValue; + } + + return celValue.value(); + } + private ImmutableList toListValue(Collection iterable) { Preconditions.checkNotNull(iterable); diff --git a/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java b/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java index 308d7b510..ccb8e605f 100644 --- a/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java +++ b/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java @@ -37,7 +37,8 @@ public void toRuntimeValue_optionalValue() { @Test @SuppressWarnings("unchecked") // Test only public void unwrap_optionalValue() { - Optional result = (Optional) CEL_VALUE_CONVERTER.unwrap(OptionalValue.create(2L)); + Optional result = + (Optional) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.create(2L)); assertThat(result).isEqualTo(Optional.of(2L)); } @@ -45,7 +46,7 @@ public void unwrap_optionalValue() { @Test @SuppressWarnings("unchecked") // Test only public void unwrap_emptyOptionalValue() { - Optional result = (Optional) CEL_VALUE_CONVERTER.unwrap(OptionalValue.EMPTY); + Optional result = (Optional) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.EMPTY); assertThat(result).isEqualTo(Optional.empty()); } diff --git a/common/src/test/java/dev/cel/common/values/ProtoCelValueConverterTest.java b/common/src/test/java/dev/cel/common/values/ProtoCelValueConverterTest.java index a517931c2..17c012db7 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoCelValueConverterTest.java +++ b/common/src/test/java/dev/cel/common/values/ProtoCelValueConverterTest.java @@ -35,7 +35,7 @@ public class ProtoCelValueConverterTest { @Test public void unwrap_nullValue() { - NullValue nullValue = (NullValue) PROTO_CEL_VALUE_CONVERTER.unwrap(NullValue.NULL_VALUE); + NullValue nullValue = (NullValue) PROTO_CEL_VALUE_CONVERTER.maybeUnwrap(NullValue.NULL_VALUE); // Note: No conversion is attempted. We're using dev.cel.common.values.NullValue.NULL_VALUE as // the diff --git a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel index 717f7aaa0..d6b2296e5 100644 --- a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel +++ b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel @@ -176,12 +176,6 @@ _TESTS_TO_SKIP_PLANNER = [ # Skip until fixed. "parse/receiver_function_names", - "proto2/extensions_get/package_scoped_test_all_types_ext", - "proto2/extensions_get/package_scoped_repeated_test_all_types", - "proto2/extensions_get/message_scoped_nested_ext", - "proto2/extensions_get/message_scoped_repeated_test_all_types", - "proto2_ext/get_ext/package_scoped_repeated_test_all_types", - "proto2_ext/get_ext/message_scoped_repeated_test_all_types", # TODO: Fix null assignment to a field "proto2/set_null/single_message", diff --git a/runtime/src/main/java/dev/cel/runtime/CelValueRuntimeTypeProvider.java b/runtime/src/main/java/dev/cel/runtime/CelValueRuntimeTypeProvider.java index e071289ca..38365127c 100644 --- a/runtime/src/main/java/dev/cel/runtime/CelValueRuntimeTypeProvider.java +++ b/runtime/src/main/java/dev/cel/runtime/CelValueRuntimeTypeProvider.java @@ -22,7 +22,6 @@ import dev.cel.common.exceptions.CelAttributeNotFoundException; import dev.cel.common.values.BaseProtoCelValueConverter; import dev.cel.common.values.BaseProtoMessageValueProvider; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueProvider; import dev.cel.common.values.CombinedCelValueProvider; import dev.cel.common.values.SelectableValue; @@ -64,7 +63,7 @@ static CelValueRuntimeTypeProvider newInstance(CelValueProvider valueProvider) { @Override public Object createMessage(String messageName, Map values) { - return maybeUnwrapCelValue( + return protoCelValueConverter.maybeUnwrap( valueProvider .newValue(messageName, values) .orElseThrow( @@ -87,7 +86,7 @@ public Object selectField(Object message, String fieldName) { SelectableValue selectableValue = getSelectableValueOrThrow(message, fieldName); Object value = selectableValue.select(fieldName); - return maybeUnwrapCelValue(value); + return protoCelValueConverter.maybeUnwrap(value); } @Override @@ -120,24 +119,12 @@ public Object adapt(String messageName, Object message) { } if (message instanceof MessageLite) { - return maybeUnwrapCelValue(protoCelValueConverter.toRuntimeValue(message)); + return protoCelValueConverter.maybeUnwrap(protoCelValueConverter.toRuntimeValue(message)); } return message; } - /** - * DefaultInterpreter cannot handle CelValue and instead expects plain Java objects. - * - *

This will become unnecessary once we introduce a rewrite of a Cel runtime. - */ - private Object maybeUnwrapCelValue(Object object) { - if (object instanceof CelValue) { - return protoCelValueConverter.unwrap((CelValue) object); - } - return object; - } - private static void throwInvalidFieldSelection(String fieldName) { throw CelAttributeNotFoundException.forFieldResolution(fieldName); } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel index f7912cff2..6561e4e5c 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel @@ -128,7 +128,6 @@ java_library( "//common/types", "//common/types:type_providers", "//common/values", - "//common/values:cel_value", "//runtime:interpretable", "@maven//:com_google_errorprone_error_prone_annotations", "@maven//:com_google_guava_guava", @@ -380,7 +379,6 @@ java_library( "//common:error_codes", "//common/exceptions:runtime_exception", "//common/values", - "//common/values:cel_value", "//runtime:evaluation_exception", "//runtime:interpretable", "//runtime:resolved_overload", diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java index 08f4fa8a8..92d234acc 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java @@ -17,7 +17,6 @@ import com.google.common.base.Joiner; import dev.cel.common.CelErrorCode; import dev.cel.common.exceptions.CelRuntimeException; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.common.values.ErrorValue; import dev.cel.runtime.CelEvaluationException; @@ -56,23 +55,21 @@ static Object evalStrictly( } } - static Object dispatch(CelResolvedOverload overload, CelValueConverter valueConverter, Object[] args) throws CelEvaluationException { + static Object dispatch( + CelResolvedOverload overload, CelValueConverter valueConverter, Object[] args) + throws CelEvaluationException { try { Object result = overload.getDefinition().apply(args); - Object runtimeValue = valueConverter.toRuntimeValue(result); - if (runtimeValue instanceof CelValue) { - return valueConverter.unwrap((CelValue) runtimeValue); - } - - return runtimeValue; + return valueConverter.maybeUnwrap(valueConverter.toRuntimeValue(result)); } catch (CelRuntimeException e) { // Function dispatch failure that's already been handled -- just propagate. throw e; } catch (RuntimeException e) { // Unexpected function dispatch failure. - throw new IllegalArgumentException(String.format( - "Function '%s' failed with arg(s) '%s'", - overload.getOverloadId(), Joiner.on(", ").join(args)), + throw new IllegalArgumentException( + String.format( + "Function '%s' failed with arg(s) '%s'", + overload.getOverloadId(), Joiner.on(", ").join(args)), e); } } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java b/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java index de1a90291..cc8ca1d97 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/NamespacedAttribute.java @@ -22,7 +22,6 @@ import dev.cel.common.types.EnumType; import dev.cel.common.types.SimpleType; import dev.cel.common.types.TypeType; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.GlobalResolver; import java.util.NoSuchElementException; @@ -148,11 +147,7 @@ private static Object applyQualifiers( obj = qualifier.qualify(obj); } - if (obj instanceof CelValue) { - obj = celValueConverter.unwrap((CelValue) obj); - } - - return obj; + return celValueConverter.maybeUnwrap(obj); } static NamespacedAttribute create( diff --git a/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java b/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java index 54eb26f21..b3d83c390 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java @@ -16,7 +16,6 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.Immutable; -import dev.cel.common.values.CelValue; import dev.cel.common.values.CelValueConverter; import dev.cel.runtime.GlobalResolver; @@ -41,10 +40,7 @@ public Object resolve(GlobalResolver ctx, ExecutionFrame frame) { } // TODO: Handle unknowns - if (obj instanceof CelValue) { - obj = celValueConverter.unwrap((CelValue) obj); - } - return obj; + return celValueConverter.maybeUnwrap(obj); } @Override diff --git a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java index 30b100bc2..20b4e641a 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java @@ -316,6 +316,35 @@ public void plan_ident_variable() throws Exception { assertThat(result).isEqualTo(1); } + @Test + public void plan_ident_variableWithStructInList() throws Exception { + CelAbstractSyntaxTree ast = compile("dyn_var"); + Program program = PLANNER.plan(ast); + + Object result = + program.eval( + ImmutableMap.of( + "dyn_var", ImmutableList.of(TestAllTypes.newBuilder().setSingleInt32(42).build()))); + + assertThat(result) + .isEqualTo(ImmutableList.of(TestAllTypes.newBuilder().setSingleInt32(42).build())); + } + + @Test + public void plan_ident_variableWithStructInMap() throws Exception { + CelAbstractSyntaxTree ast = compile("dyn_var"); + Program program = PLANNER.plan(ast); + + Object result = + program.eval( + ImmutableMap.of( + "dyn_var", + ImmutableMap.of("foo", TestAllTypes.newBuilder().setSingleInt32(42).build()))); + + assertThat(result) + .isEqualTo(ImmutableMap.of("foo", TestAllTypes.newBuilder().setSingleInt32(42).build())); + } + @Test public void planIdent_typeLiteral(@TestParameter TypeLiteralTestCase testCase) throws Exception { CelAbstractSyntaxTree ast = compile(testCase.expression);