Fix null assignment to fields

l46kok · copybara-github · commit aa9e629a3877 · 2026-02-27T11:35:10.000-08:00
PiperOrigin-RevId: 875509705
diff --git a/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java b/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java
@@ -204,8 +204,29 @@ public Optional<Object> adaptFieldToValue(FieldDescriptor fieldDescriptor, Objec
   @SuppressWarnings({"unchecked", "rawtypes"})
   public Optional<Object> adaptValueToFieldType(
       FieldDescriptor fieldDescriptor, Object fieldValue) {
-    if (isWrapperType(fieldDescriptor) && fieldValue.equals(NullValue.NULL_VALUE)) {
-      return Optional.empty();
+    if (fieldValue instanceof NullValue) {
+      // `null` assignment to fields indicate that the field would not be set
+      // in a protobuf message (e.g: Message{msg_field: null} -> Message{})
+      //
+      // We explicitly check below for invalid null assignments, such as repeated 
+      // or map fields. (e.g: Message{repeated_field: null} -> Error)
+      if (fieldDescriptor.isMapField()
+          || fieldDescriptor.isRepeated()
+          || fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE
+          || WellKnownProto.JSON_STRUCT_VALUE
+              .typeName()
+              .equals(fieldDescriptor.getMessageType().getFullName())
+          || WellKnownProto.JSON_LIST_VALUE
+              .typeName()
+              .equals(fieldDescriptor.getMessageType().getFullName())) {
+        throw new IllegalArgumentException("Unsupported field type");
+      }
+
+      String typeFullName = fieldDescriptor.getMessageType().getFullName();
+      if (!WellKnownProto.ANY_VALUE.typeName().equals(typeFullName)
+          && !WellKnownProto.JSON_VALUE.typeName().equals(typeFullName)) {
+        return Optional.empty();
+      }
     }
     if (fieldDescriptor.isMapField()) {
       Descriptor entryDescriptor = fieldDescriptor.getMessageType();
@@ -370,14 +391,6 @@ private static String typeName(Descriptor protoType) {
     return protoType.getFullName();
   }
 
-  private static boolean isWrapperType(FieldDescriptor fieldDescriptor) {
-    if (fieldDescriptor.getJavaType() != FieldDescriptor.JavaType.MESSAGE) {
-      return false;
-    }
-    String fieldTypeName = fieldDescriptor.getMessageType().getFullName();
-    return WellKnownProto.isWrapperType(fieldTypeName);
-  }
-
   private static int intCheckedCast(long value) {
     try {
       return Ints.checkedCast(value);
diff --git 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.
+   *
+   * <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);
 
diff --git a/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java b/common/src/main/java/dev/cel/common/values/ProtoCelValueConverter.java
@@ -67,7 +67,7 @@ protected Object fromWellKnownProto(MessageLiteOrBuilder msg, WellKnownProto wel
         try {
           unpackedMessage = dynamicProto.unpack((Any) message);
         } catch (InvalidProtocolBufferException e) {
-          throw new IllegalStateException(
+          throw new IllegalArgumentException(
               "Unpacking failed for message: " + message.getDescriptorForType().getFullName(), e);
         }
         return toRuntimeValue(unpackedMessage);
diff --git a/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java b/common/src/test/java/dev/cel/common/internal/ProtoAdapterTest.java
@@ -150,10 +150,7 @@ public static List<Object[]> data() {
     @Test
     public void adaptValueToProto_bidirectionalConversion() {
       DynamicProto dynamicProto = DynamicProto.create(DefaultMessageFactory.INSTANCE);
-      ProtoAdapter protoAdapter =
-          new ProtoAdapter(
-              dynamicProto,
-              CelOptions.current().build());
+      ProtoAdapter protoAdapter = new ProtoAdapter(dynamicProto, CelOptions.current().build());
       assertThat(protoAdapter.adaptValueToProto(value, proto.getDescriptorForType().getFullName()))
           .isEqualTo(proto);
       assertThat(protoAdapter.adaptProtoToValue(proto)).isEqualTo(value);
@@ -181,6 +178,18 @@ public void adaptAnyValue_hermeticTypes_bidirectionalConversion() {
 
   @RunWith(JUnit4.class)
   public static class AsymmetricConversionTest {
+
+    @Test
+    public void unpackAny_celNullValue() throws Exception {
+      ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT);
+      Any any =
+          (Any)
+              protoAdapter.adaptValueToProto(
+                  dev.cel.common.values.NullValue.NULL_VALUE, "google.protobuf.Any");
+      Object unpacked = protoAdapter.adaptProtoToValue(any);
+      assertThat(unpacked).isEqualTo(dev.cel.common.values.NullValue.NULL_VALUE);
+    }
+
     @Test
     public void adaptValueToProto_asymmetricFloatConversion() {
       ProtoAdapter protoAdapter = new ProtoAdapter(DYNAMIC_PROTO, CelOptions.DEFAULT);
diff --git a/common/src/test/java/dev/cel/common/values/CelValueConverterTest.java b/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());
   }
diff --git 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
@@ -124,14 +124,6 @@ _TESTS_TO_SKIP_LEGACY = [
     "string_ext/format",
     "string_ext/format_errors",
 
-    # TODO: Fix null assignment to a field
-    "proto2/set_null/single_message",
-    "proto2/set_null/single_duration",
-    "proto2/set_null/single_timestamp",
-    "proto3/set_null/single_message",
-    "proto3/set_null/single_duration",
-    "proto3/set_null/single_timestamp",
-
     # Future features for CEL 1.0
     # TODO: Strong typing support for enums, specified but not implemented.
     "enums/strong_proto2",
@@ -162,7 +154,6 @@ _TESTS_TO_SKIP_PLANNER = [
     "string_ext/format_errors",
 
     # TODO: Check behavior for go/cpp
-    "basic/functions/unbound",
     "basic/functions/unbound_is_runtime_error",
 
     # TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms.
@@ -176,20 +167,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",
-    "proto2/set_null/single_duration",
-    "proto2/set_null/single_timestamp",
-    "proto3/set_null/single_message",
-    "proto3/set_null/single_duration",
-    "proto3/set_null/single_timestamp",
 
     # Type inference edgecases around null(able) assignability.
     # These type check, but resolve to a different type.
diff --git a/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java b/conformance/src/test/java/dev/cel/conformance/ConformanceTest.java
@@ -210,10 +210,10 @@ public void evaluate() throws Throwable {
     }
 
     CelRuntime runtime = getRuntime(test, usePlanner);
-    Program program = runtime.createProgram(response.getAst());
     ExprValue result = null;
     CelEvaluationException error = null;
     try {
+      Program program = runtime.createProgram(response.getAst());
       result = toExprValue(program.eval(getBindings(test)), response.getAst().getResultType());
     } catch (CelEvaluationException e) {
       error = e;
diff --git a/extensions/src/test/java/dev/cel/extensions/BUILD.bazel b/extensions/src/test/java/dev/cel/extensions/BUILD.bazel
@@ -38,6 +38,7 @@ java_library(
         "//runtime:interpreter_util",
         "//runtime:lite_runtime",
         "//runtime:lite_runtime_factory",
+        "//runtime:runtime_planner_impl",
         "@cel_spec//proto/cel/expr/conformance/proto2:test_all_types_java_proto",
         "@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto",
         "@cel_spec//proto/cel/expr/conformance/test:simple_java_proto",
diff --git a/extensions/src/test/java/dev/cel/extensions/CelProtoExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelProtoExtensionsTest.java
@@ -46,6 +46,7 @@
 import dev.cel.runtime.CelFunctionBinding;
 import dev.cel.runtime.CelRuntime;
 import dev.cel.runtime.CelRuntimeFactory;
+import dev.cel.runtime.CelRuntimeImpl;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -60,9 +61,14 @@ public final class CelProtoExtensionsTest {
           .addVar("msg", StructTypeReference.create("cel.expr.conformance.proto2.TestAllTypes"))
           .setContainer(CelContainer.ofName("cel.expr.conformance.proto2"))
           .build();
-
   private static final CelRuntime CEL_RUNTIME =
-      CelRuntimeFactory.standardCelRuntimeBuilder()
+      CelRuntimeImpl.newBuilder()
+          .setOptions(
+              CelOptions.current()
+                  .enableTimestampEpoch(true)
+                  .enableHeterogeneousNumericComparisons(true)
+                  .build())
+          // CEL-Internal-2
           .addFileTypes(TestAllTypesExtensions.getDescriptor())
           .build();
 
diff --git 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<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);
   }
diff --git 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
@@ -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
diff --git a/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java b/runtime/src/main/java/dev/cel/runtime/planner/RelativeAttribute.java
diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteInterpreterTest.java
diff --git a/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java
diff --git a/runtime/src/test/resources/nullAssignability.baseline b/runtime/src/test/resources/nullAssignability.baseline
diff --git a/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java b/testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java

Original file line number	Diff line number	Diff line change
`@@ -67,7 +67,7 @@ protected Object fromWellKnownProto(MessageLiteOrBuilder msg, WellKnownProto wel`
`67`	`67`	`try {`
`68`	`68`	`unpackedMessage = dynamicProto.unpack((Any) message);`
`69`	`69`	`} catch (InvalidProtocolBufferException e) {`
`70`		`- throw new IllegalStateException(`
	`70`	`+ throw new IllegalArgumentException(`
`71`	`71`	`"Unpacking failed for message: " + message.getDescriptorForType().getFullName(), e);`
`72`	`72`	`}`
`73`	`73`	`return toRuntimeValue(unpackedMessage);`