No public description

PiperOrigin-RevId: 996749917

Author: l46kok

common/src/main/java/dev/cel/common/values/BUILD.bazel

@@ -372,7 +372,6 @@ cel_android_library(
         "//common/values:cel_byte_string",
         "//protobuf:cel_lite_descriptor",
         "@maven//:com_google_errorprone_error_prone_annotations",
-        "@maven//:com_google_guava_guava",
         "@maven//:org_jspecify_jspecify",
         "@maven_android//:com_google_guava_guava",
         "@maven_android//:com_google_protobuf_protobuf_javalite",

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

@@ -16,7 +16,6 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Defaults;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -59,8 +58,6 @@
 @Immutable
 @Internal
 public final class ProtoLiteCelValueConverter extends BaseProtoCelValueConverter {
-  private static final String MAP_KEY_FIELD_NAME = "key";
-  private static final String MAP_VALUE_FIELD_NAME = "value";
   private static final int MAP_KEY_FIELD_NUMBER = 1;
   private static final int MAP_VALUE_FIELD_NUMBER = 2;
 
@@ -316,19 +313,52 @@ private Map.Entry<Object, Object> readSingleMapEntry(
    */
   @Nullable Object readSingleField(ByteString bytes, FieldLiteDescriptor fieldDescriptor)
       throws IOException {
+    return scanField(bytes, fieldDescriptor, null, null);
+  }
+
+  /** Scans for {@code fieldDescriptor}; null map entry descriptors are looked up in the pool. */
+  private @Nullable Object scanField(
+      ByteString bytes,
+      FieldLiteDescriptor fieldDescriptor,
+      @Nullable FieldLiteDescriptor keyDescriptor,
+      @Nullable FieldLiteDescriptor valueDescriptor)
+      throws IOException {
     CodedInputStream inputStream = bytes.newCodedInput();
     int targetFieldNumber = fieldDescriptor.getFieldNumber();
     Object fieldValue = null;
     for (int tag = inputStream.readTag(); tag != 0; tag = inputStream.readTag()) {
-      int fieldNumber = WireFormat.getTagFieldNumber(tag);
-      if (fieldNumber != targetFieldNumber) {
+      if (WireFormat.getTagFieldNumber(tag) != targetFieldNumber) {
         skipWireField(tag, inputStream);
         continue;
       }
       int tagWireType = WireFormat.getTagWireType(tag);
-      fieldValue = readFieldValue(tagWireType, inputStream, fieldDescriptor, fieldValue);
+      switch (fieldDescriptor.getEncodingType()) {
+        case SINGULAR:
+          fieldValue = readSingularField(tagWireType, inputStream, fieldDescriptor, fieldValue);
+          break;
+        case LIST:
+          fieldValue = readRepeatedField(tagWireType, inputStream, fieldDescriptor, fieldValue);
+          break;
+        case MAP:
+          if (keyDescriptor == null) {
+            // Looked up at the first entry since an absent map needs no entry descriptor.
+            MessageLiteDescriptor entryDescriptor =
+                descriptorPool.getDescriptorOrThrow(fieldDescriptor.getFieldProtoTypeName());
+            keyDescriptor = entryDescriptor.getByFieldNumberOrThrow(MAP_KEY_FIELD_NUMBER);
+            valueDescriptor = entryDescriptor.getByFieldNumberOrThrow(MAP_VALUE_FIELD_NUMBER);
+          }
+          fieldValue =
+              readMapField(
+                  tagWireType,
+                  inputStream,
+                  fieldDescriptor,
+                  keyDescriptor,
+                  valueDescriptor,
+                  fieldValue);
+          break;
+      }
     }
-    return fieldValue == null ? null : resolveFieldValue(finalizeFieldValue(fieldValue));
+    return fieldValue == null ? null : finalizeFieldValue(fieldValue);
   }
 
   boolean hasSingleField(ByteString bytes, FieldLiteDescriptor fieldDescriptor) throws IOException {
@@ -394,71 +424,37 @@ boolean hasFieldByNumber(ByteString bytes, SelectField field) throws IOException
 
   private @Nullable Object readFieldByNumber(ByteString bytes, SelectField field)
       throws IOException {
-    FieldLiteDescriptor fieldDescriptor = newFieldDescriptor(field);
     SelectField.MapEntrySpec mapEntrySpec = field.mapEntrySpec();
     if (mapEntrySpec == null) {
-      return readSingleField(bytes, fieldDescriptor);
+      return readSingleField(bytes, newFieldDescriptor(field));
     }
     // The map entry type has no descriptor either, so its key and value are described by the spec.
-    FieldLiteDescriptor keyDescriptor =
+    return scanField(
+        bytes,
+        newFieldDescriptor(field),
         newFieldDescriptor(
-            MAP_KEY_FIELD_NUMBER,
-            MAP_KEY_FIELD_NAME,
-            EncodingType.SINGULAR,
-            mapEntrySpec.keyTypeCode(),
-            /* protoTypeName= */ "");
-    FieldLiteDescriptor valueDescriptor =
+            MAP_KEY_FIELD_NUMBER, "key", EncodingType.SINGULAR, mapEntrySpec.keyTypeCode(), ""),
         newFieldDescriptor(
             MAP_VALUE_FIELD_NUMBER,
-            MAP_VALUE_FIELD_NAME,
+            "value",
             EncodingType.SINGULAR,
             mapEntrySpec.valueTypeCode(),
-            field.protoTypeName());
-    CodedInputStream inputStream = bytes.newCodedInput();
-    Map<Object, Object> mapValues = null;
-    for (int tag = inputStream.readTag(); tag != 0; tag = inputStream.readTag()) {
-      if (WireFormat.getTagFieldNumber(tag) != field.fieldNumber()) {
-        skipWireField(tag, inputStream);
-        continue;
-      }
-      mapValues =
-          readMapField(
-              WireFormat.getTagWireType(tag),
-              inputStream,
-              fieldDescriptor,
-              keyDescriptor,
-              valueDescriptor,
-              mapValues);
-    }
-    return mapValues == null ? null : resolveFieldValue(finalizeFieldValue(mapValues));
+            field.protoTypeName()));
   }
 
   /** Describes {@code field} by the type information it carries. */
   private static FieldLiteDescriptor newFieldDescriptor(SelectField field) {
-    if (field.mapEntrySpec() != null) {
-      // SelectField doesn't name the map entry type; its protoTypeName() is the map's value type.
-      return newFieldDescriptor(
-          field.fieldNumber(),
-          field.fieldName(),
-          EncodingType.MAP,
-          SelectField.MESSAGE_TYPE_CODE,
-          /* protoTypeName= */ "");
-    }
-    if (field.typeCode() == SelectField.NO_TYPE_CODE) {
-      // Only presence tests omit the type code. Their presence check doesn't depend on the type,
-      // and the fields they navigate through are always messages.
-      return newFieldDescriptor(
-          field.fieldNumber(),
-          field.fieldName(),
-          EncodingType.SINGULAR,
-          SelectField.MESSAGE_TYPE_CODE,
-          /* protoTypeName= */ "");
-    }
+    // Only presence tests omit the type code: their presence check doesn't depend on the type, and
+    // the fields they navigate through are messages, as are map entries.
+    boolean isMap = field.mapEntrySpec() != null;
+    boolean isMessage = isMap || field.typeCode() == SelectField.NO_TYPE_CODE;
     return newFieldDescriptor(
         field.fieldNumber(),
         field.fieldName(),
-        field.defaultValue() instanceof List ? EncodingType.LIST : EncodingType.SINGULAR,
-        field.typeCode(),
+        isMap
+            ? EncodingType.MAP
+            : (field.defaultValue() instanceof List ? EncodingType.LIST : EncodingType.SINGULAR),
+        isMessage ? SelectField.MESSAGE_TYPE_CODE : field.typeCode(),
         field.protoTypeName());
   }
 
@@ -480,59 +476,29 @@ private static FieldLiteDescriptor newFieldDescriptor(
         protoTypeName);
   }
 
-  /** Returns the CEL value of a scanned field, completing any conversion that was deferred. */
-  private Object resolveFieldValue(Object fieldValue) {
-    if (fieldValue instanceof DeferredConversion) {
-      return toRuntimeValue(((DeferredConversion) fieldValue).value);
-    }
-    return fieldValue;
-  }
-
   /**
    * Converts a value accumulated while scanning a field into its final immutable form.
    *
    * <p>Repeated and map fields accumulate into mutable containers, which are copied into immutable
    * ones. Well-known types other than FieldMask (see {@link #isStructLike}) are kept as parsed
-   * {@link MessageLite}s until the scan completes so that split occurrences can be merged, and
-   * values holding them are wrapped in a {@link DeferredConversion}. All other messages are wrapped
-   * as CEL values as soon as they are read.
+   * {@link MessageLite}s until the scan completes, so that split occurrences can be merged and map
+   * values replaced by a repeated key are never converted. All other messages are wrapped as CEL
+   * values as soon as they are read.
    */
-  private static Object finalizeFieldValue(Object accumulatedValue) {
+  private Object finalizeFieldValue(Object accumulatedValue) {
     if (accumulatedValue instanceof List) {
       ImmutableList<?> list = ImmutableList.copyOf((List<?>) accumulatedValue);
-      return Iterables.any(list, MessageLite.class::isInstance)
-          ? new DeferredConversion(list)
-          : list;
+      return Iterables.any(list, MessageLite.class::isInstance) ? toRuntimeValue(list) : list;
     }
     if (accumulatedValue instanceof Map) {
       ImmutableMap<?, ?> map = ImmutableMap.copyOf((Map<?, ?>) accumulatedValue);
-      return Iterables.any(map.values(), MessageLite.class::isInstance)
-          ? new DeferredConversion(map)
-          : map;
+      return Iterables.any(map.values(), MessageLite.class::isInstance) ? toRuntimeValue(map) : map;
     }
     return accumulatedValue instanceof MessageLite
-        ? new DeferredConversion(accumulatedValue)
+        ? toRuntimeValue(accumulatedValue)
         : accumulatedValue;
   }
 
-  private @Nullable Object readFieldValue(
-      int tagWireType,
-      CodedInputStream inputStream,
-      FieldLiteDescriptor fieldDescriptor,
-      @Nullable Object existingValue)
-      throws IOException {
-    EncodingType encodingType = fieldDescriptor.getEncodingType();
-    switch (encodingType) {
-      case SINGULAR:
-        return readSingularField(tagWireType, inputStream, fieldDescriptor, existingValue);
-      case LIST:
-        return readRepeatedField(tagWireType, inputStream, fieldDescriptor, existingValue);
-      case MAP:
-        return readMapField(tagWireType, inputStream, fieldDescriptor, existingValue);
-    }
-    throw new IllegalStateException("Unexpected encoding type: " + encodingType);
-  }
-
   private Object readSingularField(
       int tagWireType,
       CodedInputStream inputStream,
@@ -589,23 +555,6 @@ private Object readSingularField(
     return repeatedValues;
   }
 
-  private Map<Object, Object> readMapField(
-      int tagWireType,
-      CodedInputStream inputStream,
-      FieldLiteDescriptor fieldDescriptor,
-      @Nullable Object existingValue)
-      throws IOException {
-    MessageLiteDescriptor entryDescriptor =
-        descriptorPool.getDescriptorOrThrow(fieldDescriptor.getFieldProtoTypeName());
-    return readMapField(
-        tagWireType,
-        inputStream,
-        fieldDescriptor,
-        entryDescriptor.getByFieldNameOrThrow(MAP_KEY_FIELD_NAME),
-        entryDescriptor.getByFieldNameOrThrow(MAP_VALUE_FIELD_NAME),
-        existingValue);
-  }
-
   // Safe because MAP fields only ever store a LinkedHashMap as their accumulated value.
   @SuppressWarnings("unchecked")
   private Map<Object, Object> readMapField(
@@ -654,34 +603,13 @@ private static boolean isPackable(FieldLiteDescriptor fieldDescriptor) {
     return fieldDescriptor.getProtoFieldType().toWireFormatFieldType().isPackable();
   }
 
-  @VisibleForTesting
-  static void skipWireField(int tag, CodedInputStream inputStream) throws IOException {
+  private static void skipWireField(int tag, CodedInputStream inputStream) throws IOException {
     int tagWireType = WireFormat.getTagWireType(tag);
-    switch (tagWireType) {
-      case WireFormat.WIRETYPE_VARINT:
-      case WireFormat.WIRETYPE_FIXED64:
-      case WireFormat.WIRETYPE_LENGTH_DELIMITED:
-      case WireFormat.WIRETYPE_FIXED32:
-        inputStream.skipField(tag);
-        return;
-      case WireFormat.WIRETYPE_START_GROUP:
-      case WireFormat.WIRETYPE_END_GROUP:
-        throw new UnsupportedOperationException("Groups are not supported");
-      default:
-        throw new IllegalArgumentException("Unknown wire type: " + tagWireType);
-    }
-  }
-
-  /**
-   * A field value holding well-known type messages, whose conversion to CEL values is deferred
-   * until {@link #resolveFieldValue}.
-   */
-  private static final class DeferredConversion {
-    private final Object value;
-
-    private DeferredConversion(Object value) {
-      this.value = checkNotNull(value);
+    if (tagWireType == WireFormat.WIRETYPE_START_GROUP
+        || tagWireType == WireFormat.WIRETYPE_END_GROUP) {
+      throw new UnsupportedOperationException("Groups are not supported");
     }
+    inputStream.skipField(tag);
   }
 
   private ProtoLiteCelValueConverter(CelLiteDescriptorPool celLiteDescriptorPool) {

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

@@ -38,7 +38,6 @@
 import com.google.protobuf.Timestamp;
 import com.google.protobuf.UInt32Value;
 import com.google.protobuf.UInt64Value;
-import com.google.protobuf.WireFormat;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import dev.cel.common.internal.CelLiteDescriptorPool;
@@ -539,6 +538,29 @@ public void readSingleField_skipsOtherFieldsAndDecodesTarget(
     assertThat(result).isEqualTo(testCase.expected);
   }
 
+  @Test
+  public void readSingleField_repeatedMapKey_convertsOnlyFinalValue() throws Exception {
+    // The first value is out of Timestamp's range, so converting it would throw.
+    ByteString bytes =
+        TestAllTypes.newBuilder()
+            .putMapStringTimestamp("k", Timestamp.newBuilder().setSeconds(1L << 60).build())
+            .build()
+            .toByteString()
+            .concat(
+                TestAllTypes.newBuilder()
+                    .putMapStringTimestamp("k", Timestamp.newBuilder().setSeconds(100).build())
+                    .build()
+                    .toByteString());
+    FieldLiteDescriptor fd =
+        fieldDescriptor(
+            "cel.expr.conformance.proto3.TestAllTypes",
+            TestAllTypes.MAP_STRING_TIMESTAMP_FIELD_NUMBER);
+
+    Object result = PROTO_LITE_CEL_VALUE_CONVERTER.readSingleField(bytes, fd);
+
+    assertThat(result).isEqualTo(ImmutableMap.of("k", Instant.ofEpochSecond(100)));
+  }
+
   @Test
   public void hasSingleField_emptyBytes_returnsFalse() throws Exception {
     FieldLiteDescriptor singleInt64Fd =
@@ -593,15 +615,21 @@ public void hasSingleField_emptyPackedFollowedByPopulatedPacked_returnsTrue() th
     assertThat(result).isTrue();
   }
 
+  // Wire types 3 and 4 are START_GROUP and END_GROUP.
   @Test
-  public void skipWireField_groupWireType_throwsUnsupportedOperationException() {
-    int startGroupTag = (1 << 3) | WireFormat.WIRETYPE_START_GROUP;
-
-    assertThrows(
-        UnsupportedOperationException.class,
-        () ->
-            ProtoLiteCelValueConverter.skipWireField(
-                startGroupTag, ByteString.EMPTY.newCodedInput()));
+  public void readSingleField_groupInSkippedField_throwsUnsupportedOperationException(
+      @TestParameter({"3", "4"}) int groupWireType) {
+    ByteString bytes = ByteString.copyFrom(new byte[] {(byte) ((1 << 3) | groupWireType)});
+    FieldLiteDescriptor fd =
+        fieldDescriptor(
+            "cel.expr.conformance.proto3.TestAllTypes", TestAllTypes.SINGLE_STRING_FIELD_NUMBER);
+
+    UnsupportedOperationException e =
+        assertThrows(
+            UnsupportedOperationException.class,
+            () -> PROTO_LITE_CEL_VALUE_CONVERTER.readSingleField(bytes, fd));
+
+    assertThat(e).hasMessageThat().isEqualTo("Groups are not supported");
   }
 
   private static FieldLiteDescriptor fieldDescriptor(String messageName, int fieldNumber) {