Fix CelValue adaptation for lists/maps

PiperOrigin-RevId: 876477992

Author: l46kok

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.
+   *
+   * <p>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<Object, Object> optionalValue = (OptionalValue<Object, Object>) celValue;
-      if (optionalValue.isZeroValue()) {
-        return Optional.empty();
+    if (value instanceof Collection) {
+      Collection<Object> collection = (Collection<Object>) value;
+      ImmutableList.Builder<Object> 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<Object, Object> map = (Map<Object, Object>) value;
+      ImmutableMap.Builder<Object, Object> builder =
+          ImmutableMap.builderWithExpectedSize(map.size());
+      for (Map.Entry<Object, Object> 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<Object, Object> optionalValue = (OptionalValue<Object, Object>) celValue;
+      if (optionalValue.isZeroValue()) {
+        return Optional.empty();
+      }
+
+      return Optional.of(optionalValue.value());
+    }
+
+    if (celValue instanceof ErrorValue) {
+      return celValue;
+    }
+
+    return celValue.value();
+  }
+
   private ImmutableList<Object> toListValue(Collection<Object> iterable) {
     Preconditions.checkNotNull(iterable);
 

common/src/test/java/dev/cel/common/values/CelValueConverterTest.java

@@ -37,15 +37,16 @@ public void toRuntimeValue_optionalValue() {
   @Test
   @SuppressWarnings("unchecked") // Test only
   public void unwrap_optionalValue() {
-    Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.unwrap(OptionalValue.create(2L));
+    Optional<Long> result =
+        (Optional<Long>) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.create(2L));
 
     assertThat(result).isEqualTo(Optional.of(2L));
   }
 
   @Test
   @SuppressWarnings("unchecked") // Test only
   public void unwrap_emptyOptionalValue() {
-    Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.unwrap(OptionalValue.EMPTY);
+    Optional<Long> result = (Optional<Long>) CEL_VALUE_CONVERTER.maybeUnwrap(OptionalValue.EMPTY);
 
     assertThat(result).isEqualTo(Optional.empty());
   }

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

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",

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<String, Object> values) {
-    return maybeUnwrapCelValue(
+    return protoCelValueConverter.maybeUnwrap(
         valueProvider
             .newValue(messageName, values)
             .orElseThrow(
@@ -87,7 +86,7 @@ public Object selectField(Object message, String fieldName) {
     SelectableValue<String> 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.
-   *
-   * <p>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);
   }

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",

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);
     }
   }

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(

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

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);