CelValueConverter cleanups

Author: l46kok

common/src/main/java/dev/cel/common/values/CelValueConverter.java

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.primitives.UnsignedLong;
 import com.google.errorprone.annotations.Immutable;
-import com.google.protobuf.ByteString;
 import dev.cel.common.annotations.Internal;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -34,7 +33,7 @@
 @SuppressWarnings("unchecked") // Unchecked cast of generics due to type-erasure (ex: MapValue).
 @Internal
 @Immutable
-public class CelValueConverter {
+public abstract class CelValueConverter {
 
   /** Adapts a {@link CelValue} to a plain old Java Object. */
   public Object fromCelValueToJavaObject(CelValue celValue) {
@@ -77,10 +76,7 @@ public CelValue fromJavaObjectToCelValue(Object value) {
       return (CelValue) value;
     }
 
-    if (value instanceof ByteString) {
-      // TODO: CelConstant should hold this value instead of adapting it here
-      return BytesValue.create(CelByteString.of(((ByteString) value).toByteArray()));
-    } else if (value instanceof Iterable) {
+    if (value instanceof Iterable) {
       return toListValue((Iterable<Object>) value);
     } else if (value instanceof Map) {
       return toMapValue((Map<Object, Object>) value);
@@ -91,9 +87,6 @@ public CelValue fromJavaObjectToCelValue(Object value) {
           .orElse(OptionalValue.EMPTY);
     } else if (value instanceof Exception) {
       return ErrorValue.create((Exception) value);
-    } else if (value instanceof com.google.protobuf.NullValue) {
-      // TODO: CelConstant should hold this value instead of adapting it here
-      return NullValue.NULL_VALUE;
     }
 
     return fromJavaPrimitiveToCelValue(value);
@@ -152,4 +145,6 @@ private MapValue<CelValue, CelValue> toMapValue(Map<Object, Object> map) {
 
     return ImmutableMapValue.create(mapBuilder.buildOrThrow());
   }
+
+  protected CelValueConverter() {}
 }

runtime/src/test/java/dev/cel/runtime/planner/BUILD.bazel

@@ -47,6 +47,7 @@ java_library(
         "//common/values:cel_value_provider",
         "//common/values:proto_message_lite_value",
         "//common/values:proto_message_lite_value_provider",
+        "//common/values:proto_message_value",
         "//common/values:proto_message_value_provider",
         "//compiler",
         "//compiler:compiler_builder",

runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java

@@ -40,6 +40,7 @@
 import dev.cel.common.values.ListValue;
 import dev.cel.common.values.MapValue;
 import dev.cel.common.values.NullValue;
+import dev.cel.common.values.ProtoCelValueConverter;
 import dev.cel.common.values.ProtoMessageValueProvider;
 import dev.cel.common.values.StringValue;
 import dev.cel.common.values.TimestampValue;
@@ -105,14 +106,17 @@ public final class ProgramPlannerTest {
               .addLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.comprehensions())
               .addMessageTypes(TestAllTypes.getDescriptor())
               .build();
+
+  // Note that the following deps are ordinarily built from top-level builder APIs
   private static final CelDescriptorPool DESCRIPTOR_POOL =
       DefaultDescriptorPool.create(CelDescriptorUtil.getAllDescriptorsFromFileDescriptor(TestAllTypes.getDescriptor().getFile()));
-  private static final CelValueConverter CEL_VALUE_CONVERTER = new CelValueConverter();
+  private static final DynamicProto DYNAMIC_PROTO = DynamicProto.create(DefaultMessageFactory.create(DESCRIPTOR_POOL));
+  private static final CelValueConverter CEL_VALUE_CONVERTER = ProtoCelValueConverter.newInstance(DESCRIPTOR_POOL,
+      DYNAMIC_PROTO);
   private static final ProgramPlanner PLANNER = ProgramPlanner.newPlanner(
       DefaultTypeProvider.create(),
       ProtoMessageValueProvider.newInstance(
-          CEL_OPTIONS,
-          DynamicProto.create(DefaultMessageFactory.create(DESCRIPTOR_POOL))
+          CEL_OPTIONS, DYNAMIC_PROTO
       ),
       CEL_VALUE_CONVERTER,
       newDispatcher()