Skip to content

Commit c4f359e

Browse files
Fix delimited inheritance in all languages.
This was previously fixed in C++ (#16549), but not ported to other languages. Delimited field encoding can be inherited by fields where it's invalid, such as non-messages and maps. In these cases, the encoding should be ignored and length-prefixed should be used. PiperOrigin-RevId: 642792988
1 parent eb61e6b commit c4f359e

File tree

7 files changed

+74
-28
lines changed

7 files changed

+74
-28
lines changed

conformance/binary_json_conformance_suite.cc

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,11 @@ std::string group(uint32_t fieldnum, std::string content) {
155155
tag(fieldnum, WireFormatLite::WIRETYPE_END_GROUP));
156156
}
157157

158+
std::string len(uint32_t fieldnum, std::string content) {
159+
return absl::StrCat(tag(fieldnum, WireFormatLite::WIRETYPE_LENGTH_DELIMITED),
160+
delim(content));
161+
}
162+
158163
std::string GetDefaultValue(FieldDescriptor::Type type) {
159164
switch (type) {
160165
case FieldDescriptor::TYPE_INT32:
@@ -363,6 +368,33 @@ void BinaryAndJsonConformanceSuite::RunDelimitedFieldTests() {
363368
TestAllTypesEdition2023 prototype;
364369
SetTypeUrl(GetTypeUrl(TestAllTypesEdition2023::GetDescriptor()));
365370

371+
RunValidProtobufTest<TestAllTypesEdition2023>(
372+
absl::StrCat("ValidNonMessage"), REQUIRED,
373+
field(1, WireFormatLite::WIRETYPE_VARINT, varint(99)),
374+
R"pb(optional_int32: 99)pb");
375+
376+
RunValidProtobufTest<TestAllTypesEdition2023>(
377+
absl::StrCat("ValidLengthPrefixedField"), REQUIRED,
378+
len(18, field(1, WireFormatLite::WIRETYPE_VARINT, varint(99))),
379+
R"pb(optional_nested_message { a: 99 })pb");
380+
381+
RunValidProtobufTest<TestAllTypesEdition2023>(
382+
absl::StrCat("ValidMap.Integer"), REQUIRED,
383+
len(56,
384+
absl::StrCat(field(1, WireFormatLite::WIRETYPE_VARINT, varint(99)),
385+
field(2, WireFormatLite::WIRETYPE_VARINT, varint(87)))),
386+
R"pb(map_int32_int32 { key: 99 value: 87 })pb");
387+
388+
RunValidProtobufTest<TestAllTypesEdition2023>(
389+
absl::StrCat("ValidMap.LengthPrefixed"), REQUIRED,
390+
len(71, absl::StrCat(len(1, "a"),
391+
len(2, field(1, WireFormatLite::WIRETYPE_VARINT,
392+
varint(87))))),
393+
R"pb(map_string_nested_message {
394+
key: "a"
395+
value: { a: 87 }
396+
})pb");
397+
366398
RunValidProtobufTest<TestAllTypesEdition2023>(
367399
absl::StrCat("ValidDelimitedField.GroupLike"), REQUIRED,
368400
group(201, field(202, WireFormatLite::WIRETYPE_VARINT, varint(99))),

conformance/test_protos/test_messages_edition2023.proto

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,16 @@ edition = "2023";
22

33
package protobuf_test_messages.editions;
44

5+
option features.message_encoding = DELIMITED;
56
option java_package = "com.google.protobuf_test_messages.edition2023";
67
option java_multiple_files = true;
78
option objc_class_prefix = "Editions";
89

910
message TestAllTypesEdition2023 {
1011
message NestedMessage {
1112
int32 a = 1;
12-
TestAllTypesEdition2023 corecursive = 2;
13+
TestAllTypesEdition2023 corecursive = 2
14+
[features.message_encoding = LENGTH_PREFIXED];
1315
}
1416

1517
enum NestedEnum {
@@ -36,16 +38,19 @@ message TestAllTypesEdition2023 {
3638
string optional_string = 14;
3739
bytes optional_bytes = 15;
3840

39-
NestedMessage optional_nested_message = 18;
40-
ForeignMessageEdition2023 optional_foreign_message = 19;
41+
NestedMessage optional_nested_message = 18
42+
[features.message_encoding = LENGTH_PREFIXED];
43+
ForeignMessageEdition2023 optional_foreign_message = 19
44+
[features.message_encoding = LENGTH_PREFIXED];
4145

4246
NestedEnum optional_nested_enum = 21;
4347
ForeignEnumEdition2023 optional_foreign_enum = 22;
4448

4549
string optional_string_piece = 24 [ctype = STRING_PIECE];
4650
string optional_cord = 25 [ctype = CORD];
4751

48-
TestAllTypesEdition2023 recursive_message = 27;
52+
TestAllTypesEdition2023 recursive_message = 27
53+
[features.message_encoding = LENGTH_PREFIXED];
4954

5055
// Repeated
5156
repeated int32 repeated_int32 = 31;
@@ -64,8 +69,10 @@ message TestAllTypesEdition2023 {
6469
repeated string repeated_string = 44;
6570
repeated bytes repeated_bytes = 45;
6671

67-
repeated NestedMessage repeated_nested_message = 48;
68-
repeated ForeignMessageEdition2023 repeated_foreign_message = 49;
72+
repeated NestedMessage repeated_nested_message = 48
73+
[features.message_encoding = LENGTH_PREFIXED];
74+
repeated ForeignMessageEdition2023 repeated_foreign_message = 49
75+
[features.message_encoding = LENGTH_PREFIXED];
6976

7077
repeated NestedEnum repeated_nested_enum = 51;
7178
repeated ForeignEnumEdition2023 repeated_foreign_enum = 52;
@@ -152,7 +159,8 @@ message TestAllTypesEdition2023 {
152159

153160
oneof oneof_field {
154161
uint32 oneof_uint32 = 111;
155-
NestedMessage oneof_nested_message = 112;
162+
NestedMessage oneof_nested_message = 112
163+
[features.message_encoding = LENGTH_PREFIXED];
156164
string oneof_string = 113;
157165
bytes oneof_bytes = 114;
158166
bool oneof_bool = 115;
@@ -170,8 +178,8 @@ message TestAllTypesEdition2023 {
170178
int32 group_int32 = 202;
171179
uint32 group_uint32 = 203;
172180
}
173-
GroupLikeType groupliketype = 201 [features.message_encoding = DELIMITED];
174-
GroupLikeType delimited_field = 202 [features.message_encoding = DELIMITED];
181+
GroupLikeType groupliketype = 201;
182+
GroupLikeType delimited_field = 202;
175183
}
176184

177185
message ForeignMessageEdition2023 {
@@ -193,6 +201,6 @@ message GroupLikeType {
193201
}
194202

195203
extend TestAllTypesEdition2023 {
196-
GroupLikeType groupliketype = 121 [features.message_encoding = DELIMITED];
197-
GroupLikeType delimited_ext = 122 [features.message_encoding = DELIMITED];
204+
GroupLikeType groupliketype = 121;
205+
GroupLikeType delimited_ext = 122;
198206
}

csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,11 @@ internal void CrossLink()
407407
throw new DescriptorValidationException(this, $"\"{Proto.TypeName}\" is not a message type.");
408408
}
409409
messageType = m;
410+
if (m.Proto.Options?.MapEntry == true || ContainingType?.Proto.Options?.MapEntry == true)
411+
{
412+
// Maps can't inherit delimited encoding.
413+
FieldType = FieldType.Message;
414+
}
410415

411416
if (Proto.HasDefaultValue)
412417
{

java/core/src/main/java/com/google/protobuf/Descriptors.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,6 +1297,8 @@ public Type getType() {
12971297
// since these are used before feature resolution when parsing java feature set defaults
12981298
// (custom options) into unknown fields.
12991299
if (type == Type.MESSAGE
1300+
&& !(messageType != null && messageType.toProto().getOptions().getMapEntry())
1301+
&& !(containingType != null && containingType.toProto().getOptions().getMapEntry())
13001302
&& this.features != null
13011303
&& getFeatures().getMessageEncoding() == FeatureSet.MessageEncoding.DELIMITED) {
13021304
return Type.GROUP;
@@ -1476,8 +1478,7 @@ public boolean hasPresence() {
14761478
* been upgraded to editions.
14771479
*/
14781480
boolean isGroupLike() {
1479-
if (getFeatures().getMessageEncoding()
1480-
!= DescriptorProtos.FeatureSet.MessageEncoding.DELIMITED) {
1481+
if (getType() != Type.GROUP) {
14811482
// Groups are always tag-delimited.
14821483
return false;
14831484
}

python/google/protobuf/descriptor.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,9 @@ def type(self):
708708
if (
709709
self._GetFeatures().message_encoding
710710
== _FEATURESET_MESSAGE_ENCODING_DELIMITED
711+
and self.message_type
712+
and not self.message_type.GetOptions().map_entry
713+
and not self.containing_type.GetOptions().map_entry
711714
):
712715
return FieldDescriptor.TYPE_GROUP
713716
return self._type

python/google/protobuf/text_format.py

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,10 +195,7 @@ def _IsGroupLike(field):
195195
True if this field is group-like, false otherwise.
196196
"""
197197
# Groups are always tag-delimited.
198-
if (
199-
field._GetFeatures().message_encoding
200-
!= descriptor._FEATURESET_MESSAGE_ENCODING_DELIMITED
201-
):
198+
if field.type != descriptor.FieldDescriptor.TYPE_GROUP:
202199
return False
203200

204201
# Group fields always are always the lowercase type name.

upb/reflection/field_def.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -252,8 +252,7 @@ bool _upb_FieldDef_ValidateUtf8(const upb_FieldDef* f) {
252252

253253
bool _upb_FieldDef_IsGroupLike(const upb_FieldDef* f) {
254254
// Groups are always tag-delimited.
255-
if (UPB_DESC(FeatureSet_message_encoding)(upb_FieldDef_ResolvedFeatures(f)) !=
256-
UPB_DESC(FeatureSet_DELIMITED)) {
255+
if (f->type_ != kUpb_FieldType_Group) {
257256
return false;
258257
}
259258

@@ -687,12 +686,6 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix,
687686
UPB_DESC(FieldDescriptorProto_has_type_name)(field_proto);
688687

689688
f->type_ = (int)UPB_DESC(FieldDescriptorProto_type)(field_proto);
690-
if (f->type_ == kUpb_FieldType_Message &&
691-
// TODO: remove once we can deprecate kUpb_FieldType_Group.
692-
UPB_DESC(FeatureSet_message_encoding)(f->resolved_features) ==
693-
UPB_DESC(FeatureSet_DELIMITED)) {
694-
f->type_ = kUpb_FieldType_Group;
695-
}
696689

697690
if (has_type) {
698691
switch (f->type_) {
@@ -713,7 +706,7 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix,
713706
}
714707
}
715708

716-
if (!has_type && has_type_name) {
709+
if ((!has_type && has_type_name) || f->type_ == kUpb_FieldType_Message) {
717710
f->type_ =
718711
UPB_FIELD_TYPE_UNSPECIFIED; // We'll assign this in resolve_subdef()
719712
} else {
@@ -863,8 +856,15 @@ static void resolve_subdef(upb_DefBuilder* ctx, const char* prefix,
863856
break;
864857
case UPB_DEFTYPE_MSG:
865858
f->sub.msgdef = def;
866-
f->type_ = kUpb_FieldType_Message; // It appears there is no way of
867-
// this being a group.
859+
f->type_ = kUpb_FieldType_Message;
860+
// TODO: remove once we can deprecate
861+
// kUpb_FieldType_Group.
862+
if (UPB_DESC(FeatureSet_message_encoding)(f->resolved_features) ==
863+
UPB_DESC(FeatureSet_DELIMITED) &&
864+
!upb_MessageDef_IsMapEntry(def) &&
865+
!(f->msgdef && upb_MessageDef_IsMapEntry(f->msgdef))) {
866+
f->type_ = kUpb_FieldType_Group;
867+
}
868868
f->has_presence = !upb_FieldDef_IsRepeated(f);
869869
break;
870870
default:

0 commit comments

Comments
 (0)