From 27e53a9de1cf1805d017c7425e243f9c2697772d Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Fri, 9 Oct 2026 15:48:09 -0700 Subject: [PATCH] Decode lite fields with a single scan loop in ProtoLiteCelValueConverter PiperOrigin-RevId: 996805339 --- .../java/dev/cel/common/values/BUILD.bazel | 1 - .../values/ProtoLiteCelValueConverter.java | 152 +++++++----------- .../ProtoLiteCelValueConverterTest.java | 23 +-- 3 files changed, 69 insertions(+), 107 deletions(-) diff --git a/common/src/main/java/dev/cel/common/values/BUILD.bazel b/common/src/main/java/dev/cel/common/values/BUILD.bazel index d50e54fc5..23821e278 100644 --- a/common/src/main/java/dev/cel/common/values/BUILD.bazel +++ b/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", diff --git a/common/src/main/java/dev/cel/common/values/ProtoLiteCelValueConverter.java b/common/src/main/java/dev/cel/common/values/ProtoLiteCelValueConverter.java index 6aea2fb98..07709f603 100644 --- a/common/src/main/java/dev/cel/common/values/ProtoLiteCelValueConverter.java +++ b/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; @@ -316,17 +315,50 @@ private Map.Entry 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 : finalizeFieldValue(fieldValue); } @@ -394,71 +426,41 @@ 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 = + /* protoTypeName= */ ""), newFieldDescriptor( MAP_VALUE_FIELD_NUMBER, MAP_VALUE_FIELD_NAME, EncodingType.SINGULAR, mapEntrySpec.valueTypeCode(), - field.protoTypeName()); - CodedInputStream inputStream = bytes.newCodedInput(); - Map 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 : 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()); } @@ -503,24 +505,6 @@ private Object finalizeFieldValue(Object 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, @@ -577,23 +561,6 @@ private Object readSingularField( return repeatedValues; } - private Map 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 readMapField( @@ -642,22 +609,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); + 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) { diff --git a/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java b/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java index c22980ac5..ace0e5ebe 100644 --- a/common/src/test/java/dev/cel/common/values/ProtoLiteCelValueConverterTest.java +++ b/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; @@ -616,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) {