Skip to content

Commit 8beb970

Browse files
mkruskal-googlecopybara-github
authored andcommitted
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 8dd8107 commit 8beb970

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
@@ -156,6 +156,11 @@ std::string group(uint32_t fieldnum, std::string content) {
156156
tag(fieldnum, WireFormatLite::WIRETYPE_END_GROUP));
157157
}
158158

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

372+
RunValidProtobufTest<TestAllTypesEdition2023>(
373+
absl::StrCat("ValidNonMessage"), REQUIRED,
374+
field(1, WireFormatLite::WIRETYPE_VARINT, varint(99)),
375+
R"pb(optional_int32: 99)pb");
376+
377+
RunValidProtobufTest<TestAllTypesEdition2023>(
378+
absl::StrCat("ValidLengthPrefixedField"), REQUIRED,
379+
len(18, field(1, WireFormatLite::WIRETYPE_VARINT, varint(99))),
380+
R"pb(optional_nested_message { a: 99 })pb");
381+
382+
RunValidProtobufTest<TestAllTypesEdition2023>(
383+
absl::StrCat("ValidMap.Integer"), REQUIRED,
384+
len(56,
385+
absl::StrCat(field(1, WireFormatLite::WIRETYPE_VARINT, varint(99)),
386+
field(2, WireFormatLite::WIRETYPE_VARINT, varint(87)))),
387+
R"pb(map_int32_int32 { key: 99 value: 87 })pb");
388+
389+
RunValidProtobufTest<TestAllTypesEdition2023>(
390+
absl::StrCat("ValidMap.LengthPrefixed"), REQUIRED,
391+
len(71, absl::StrCat(len(1, "a"),
392+
len(2, field(1, WireFormatLite::WIRETYPE_VARINT,
393+
varint(87))))),
394+
R"pb(map_string_nested_message {
395+
key: "a"
396+
value: { a: 87 }
397+
})pb");
398+
367399
RunValidProtobufTest<TestAllTypesEdition2023>(
368400
absl::StrCat("ValidDelimitedField.GroupLike"), REQUIRED,
369401
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
@@ -9,6 +9,7 @@ edition = "2023";
99

1010
package protobuf_test_messages.editions;
1111

12+
option features.message_encoding = DELIMITED;
1213
option java_package = "com.google.protobuf_test_messages.edition2023";
1314
option java_multiple_files = true;
1415
option objc_class_prefix = "Editions";
@@ -20,7 +21,8 @@ message ComplexMessage {
2021
message TestAllTypesEdition2023 {
2122
message NestedMessage {
2223
int32 a = 1;
23-
TestAllTypesEdition2023 corecursive = 2;
24+
TestAllTypesEdition2023 corecursive = 2
25+
[features.message_encoding = LENGTH_PREFIXED];
2426
}
2527

2628
enum NestedEnum {
@@ -47,16 +49,19 @@ message TestAllTypesEdition2023 {
4749
string optional_string = 14;
4850
bytes optional_bytes = 15;
4951

50-
NestedMessage optional_nested_message = 18;
51-
ForeignMessageEdition2023 optional_foreign_message = 19;
52+
NestedMessage optional_nested_message = 18
53+
[features.message_encoding = LENGTH_PREFIXED];
54+
ForeignMessageEdition2023 optional_foreign_message = 19
55+
[features.message_encoding = LENGTH_PREFIXED];
5256

5357
NestedEnum optional_nested_enum = 21;
5458
ForeignEnumEdition2023 optional_foreign_enum = 22;
5559

5660
string optional_string_piece = 24 [ctype = STRING_PIECE];
5761
string optional_cord = 25 [ctype = CORD];
5862

59-
TestAllTypesEdition2023 recursive_message = 27;
63+
TestAllTypesEdition2023 recursive_message = 27
64+
[features.message_encoding = LENGTH_PREFIXED];
6065

6166
// Repeated
6267
repeated int32 repeated_int32 = 31;
@@ -75,8 +80,10 @@ message TestAllTypesEdition2023 {
7580
repeated string repeated_string = 44;
7681
repeated bytes repeated_bytes = 45;
7782

78-
repeated NestedMessage repeated_nested_message = 48;
79-
repeated ForeignMessageEdition2023 repeated_foreign_message = 49;
83+
repeated NestedMessage repeated_nested_message = 48
84+
[features.message_encoding = LENGTH_PREFIXED];
85+
repeated ForeignMessageEdition2023 repeated_foreign_message = 49
86+
[features.message_encoding = LENGTH_PREFIXED];
8087

8188
repeated NestedEnum repeated_nested_enum = 51;
8289
repeated ForeignEnumEdition2023 repeated_foreign_enum = 52;
@@ -163,7 +170,8 @@ message TestAllTypesEdition2023 {
163170

164171
oneof oneof_field {
165172
uint32 oneof_uint32 = 111;
166-
NestedMessage oneof_nested_message = 112;
173+
NestedMessage oneof_nested_message = 112
174+
[features.message_encoding = LENGTH_PREFIXED];
167175
string oneof_string = 113;
168176
bytes oneof_bytes = 114;
169177
bool oneof_bool = 115;
@@ -181,8 +189,8 @@ message TestAllTypesEdition2023 {
181189
int32 group_int32 = 202;
182190
uint32 group_uint32 = 203;
183191
}
184-
GroupLikeType groupliketype = 201 [features.message_encoding = DELIMITED];
185-
GroupLikeType delimited_field = 202 [features.message_encoding = DELIMITED];
192+
GroupLikeType groupliketype = 201;
193+
GroupLikeType delimited_field = 202;
186194
}
187195

188196
message ForeignMessageEdition2023 {
@@ -204,6 +212,6 @@ message GroupLikeType {
204212
}
205213

206214
extend TestAllTypesEdition2023 {
207-
GroupLikeType groupliketype = 121 [features.message_encoding = DELIMITED];
208-
GroupLikeType delimited_ext = 122 [features.message_encoding = DELIMITED];
215+
GroupLikeType groupliketype = 121;
216+
GroupLikeType delimited_ext = 122;
209217
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,11 @@ internal void CrossLink()
412412
throw new DescriptorValidationException(this, $"\"{Proto.TypeName}\" is not a message type.");
413413
}
414414
messageType = m;
415+
if (m.Proto.Options?.MapEntry == true || ContainingType?.Proto.Options?.MapEntry == true)
416+
{
417+
// Maps can't inherit delimited encoding.
418+
FieldType = FieldType.Message;
419+
}
415420

416421
if (Proto.HasDefaultValue)
417422
{

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

@@ -690,12 +689,6 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix,
690689
UPB_DESC(FieldDescriptorProto_has_type_name)(field_proto);
691690

692691
f->type_ = (int)UPB_DESC(FieldDescriptorProto_type)(field_proto);
693-
if (f->type_ == kUpb_FieldType_Message &&
694-
// TODO: remove once we can deprecate kUpb_FieldType_Group.
695-
UPB_DESC(FeatureSet_message_encoding)(f->resolved_features) ==
696-
UPB_DESC(FeatureSet_DELIMITED)) {
697-
f->type_ = kUpb_FieldType_Group;
698-
}
699692

700693
if (has_type) {
701694
switch (f->type_) {
@@ -716,7 +709,7 @@ static void _upb_FieldDef_Create(upb_DefBuilder* ctx, const char* prefix,
716709
}
717710
}
718711

719-
if (!has_type && has_type_name) {
712+
if ((!has_type && has_type_name) || f->type_ == kUpb_FieldType_Message) {
720713
f->type_ =
721714
UPB_FIELD_TYPE_UNSPECIFIED; // We'll assign this in resolve_subdef()
722715
} else {
@@ -866,8 +859,15 @@ static void resolve_subdef(upb_DefBuilder* ctx, const char* prefix,
866859
break;
867860
case UPB_DEFTYPE_MSG:
868861
f->sub.msgdef = def;
869-
f->type_ = kUpb_FieldType_Message; // It appears there is no way of
870-
// this being a group.
862+
f->type_ = kUpb_FieldType_Message;
863+
// TODO: remove once we can deprecate
864+
// kUpb_FieldType_Group.
865+
if (UPB_DESC(FeatureSet_message_encoding)(f->resolved_features) ==
866+
UPB_DESC(FeatureSet_DELIMITED) &&
867+
!upb_MessageDef_IsMapEntry(def) &&
868+
!(f->msgdef && upb_MessageDef_IsMapEntry(f->msgdef))) {
869+
f->type_ = kUpb_FieldType_Group;
870+
}
871871
f->has_presence = !upb_FieldDef_IsRepeated(f);
872872
break;
873873
default:

0 commit comments

Comments
 (0)