Skip to content

Commit 61c9187

Browse files
mkruskal-googlemkruskal-google
committed
Clarify map behaviors in editions.
Map fields should remain length-prefixed for now, even if DELIMITED is inherited. Field presence will remain unchanged, but unit-tests are added to make sure proto2/proto3 behaviors stay consistent. Closes #16549 PiperOrigin-RevId: 630191163
1 parent 4114925 commit 61c9187

File tree

2 files changed

+143
-39
lines changed

2 files changed

+143
-39
lines changed

‎src/google/protobuf/descriptor.cc

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4362,7 +4362,8 @@ class DescriptorBuilder {
43624362
DescriptorPool::ErrorCollector::ErrorLocation error_location,
43634363
bool force_merge = false);
43644364

4365-
void PostProcessFieldFeatures(FieldDescriptor& field);
4365+
void PostProcessFieldFeatures(FieldDescriptor& field,
4366+
const FieldDescriptorProto& proto);
43664367

43674368
// Allocates an array of two strings, the first one is a copy of
43684369
// `proto_name`, and the second one is the full name. Full proto name is
@@ -5532,7 +5533,8 @@ void DescriptorBuilder::ResolveFeatures(const FileDescriptorProto& proto,
55325533
/*force_merge=*/true);
55335534
}
55345535

5535-
void DescriptorBuilder::PostProcessFieldFeatures(FieldDescriptor& field) {
5536+
void DescriptorBuilder::PostProcessFieldFeatures(
5537+
FieldDescriptor& field, const FieldDescriptorProto& proto) {
55365538
// TODO This can be replace by a runtime check in `is_required`
55375539
// once the `label` getter is hidden.
55385540
if (field.features().field_presence() == FeatureSet::LEGACY_REQUIRED &&
@@ -5542,8 +5544,15 @@ void DescriptorBuilder::PostProcessFieldFeatures(FieldDescriptor& field) {
55425544
// TODO This can be replace by a runtime check of `is_delimited`
55435545
// once the `TYPE_GROUP` value is removed.
55445546
if (field.type_ == FieldDescriptor::TYPE_MESSAGE &&
5547+
!field.containing_type()->options().map_entry() &&
55455548
field.features().message_encoding() == FeatureSet::DELIMITED) {
5546-
field.type_ = FieldDescriptor::TYPE_GROUP;
5549+
Symbol type =
5550+
LookupSymbol(proto.type_name(), field.full_name(),
5551+
DescriptorPool::PLACEHOLDER_MESSAGE, LOOKUP_TYPES, false);
5552+
if (type.descriptor() == nullptr ||
5553+
!type.descriptor()->options().map_entry()) {
5554+
field.type_ = FieldDescriptor::TYPE_GROUP;
5555+
}
55475556
}
55485557
}
55495558

@@ -6089,9 +6098,11 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
60896098
});
60906099

60916100
// Post-process cleanup for field features.
6092-
internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) {
6093-
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field));
6094-
});
6101+
internal::VisitDescriptors(
6102+
*result, proto,
6103+
[&](const FieldDescriptor& field, const FieldDescriptorProto& proto) {
6104+
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field), proto);
6105+
});
60956106

60966107
// Interpret any remaining uninterpreted options gathered into
60976108
// options_to_interpret_ during descriptor building. Cross-linking has made

‎src/google/protobuf/descriptor_unittest.cc

Lines changed: 126 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4059,6 +4059,22 @@ class ValidationErrorTest : public testing::Test {
40594059
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
40604060
}
40614061

4062+
const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
4063+
absl::string_view file_text) {
4064+
io::ArrayInputStream input_stream(file_text.data(), file_text.size());
4065+
SimpleErrorCollector error_collector;
4066+
io::Tokenizer tokenizer(&input_stream, &error_collector);
4067+
compiler::Parser parser;
4068+
parser.RecordErrorsTo(&error_collector);
4069+
FileDescriptorProto proto;
4070+
ABSL_CHECK(parser.Parse(&tokenizer, &proto))
4071+
<< error_collector.last_error() << "\n"
4072+
<< file_text;
4073+
ABSL_CHECK_EQ("", error_collector.last_error());
4074+
proto.set_name(file_name);
4075+
return pool_.BuildFile(proto);
4076+
}
4077+
40624078

40634079
// Add file_proto to the DescriptorPool. Expect errors to be produced which
40644080
// match the given error text.
@@ -8613,7 +8629,9 @@ TEST_F(FeaturesTest, OneofFieldFeaturesOverride) {
86138629
}
86148630

86158631
TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
8616-
constexpr absl::string_view kProtoFile = R"schema(
8632+
BuildDescriptorMessagesInTestPool();
8633+
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
8634+
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
86178635
edition = "2023";
86188636
86198637
import "google/protobuf/unittest_features.proto";
@@ -8630,22 +8648,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
86308648
features.(pb.test).multiple_feature = VALUE3
86318649
];
86328650
}
8633-
)schema";
8634-
io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size());
8635-
SimpleErrorCollector error_collector;
8636-
io::Tokenizer tokenizer(&input_stream, &error_collector);
8637-
compiler::Parser parser;
8638-
parser.RecordErrorsTo(&error_collector);
8639-
FileDescriptorProto proto;
8640-
ASSERT_TRUE(parser.Parse(&tokenizer, &proto))
8641-
<< error_collector.last_error() << "\n"
8642-
<< kProtoFile;
8643-
ASSERT_EQ("", error_collector.last_error());
8644-
proto.set_name("foo.proto");
8645-
8646-
BuildDescriptorMessagesInTestPool();
8647-
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
8648-
const FileDescriptor* file = pool_.BuildFile(proto);
8651+
)schema");
86498652
ASSERT_THAT(file, NotNull());
86508653

86518654
const FieldDescriptor* map_field = file->message_type(0)->field(0);
@@ -8673,7 +8676,8 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
86738676
}
86748677

86758678
TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
8676-
constexpr absl::string_view kProtoFile = R"schema(
8679+
BuildDescriptorMessagesInTestPool();
8680+
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
86778681
edition = "2023";
86788682
86798683
message Foo {
@@ -8687,21 +8691,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
86878691
features.utf8_validation = NONE
86888692
];
86898693
}
8690-
)schema";
8691-
io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size());
8692-
SimpleErrorCollector error_collector;
8693-
io::Tokenizer tokenizer(&input_stream, &error_collector);
8694-
compiler::Parser parser;
8695-
parser.RecordErrorsTo(&error_collector);
8696-
FileDescriptorProto proto;
8697-
ASSERT_TRUE(parser.Parse(&tokenizer, &proto))
8698-
<< error_collector.last_error() << "\n"
8699-
<< kProtoFile;
8700-
ASSERT_EQ("", error_collector.last_error());
8701-
proto.set_name("foo.proto");
8702-
8703-
BuildDescriptorMessagesInTestPool();
8704-
const FileDescriptor* file = pool_.BuildFile(proto);
8694+
)schema");
87058695
ASSERT_THAT(file, NotNull());
87068696

87078697
auto validate_map_field = [](const FieldDescriptor* field) {
@@ -8718,6 +8708,109 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
87188708
validate_map_field(file->message_type(0)->field(2));
87198709
}
87208710

8711+
TEST_F(FeaturesTest, MapFieldFeaturesImplicitPresence) {
8712+
BuildDescriptorMessagesInTestPool();
8713+
const FileDescriptor* editions = ParseAndBuildFile("editions.proto", R"schema(
8714+
edition = "2023";
8715+
8716+
option features.field_presence = IMPLICIT;
8717+
8718+
message Foo {
8719+
map<string, Foo> message_map = 1;
8720+
map<string, string> string_map = 2;
8721+
}
8722+
)schema");
8723+
ASSERT_THAT(editions, NotNull());
8724+
const FileDescriptor* proto3 = ParseAndBuildFile("proto3.proto", R"schema(
8725+
syntax = "proto3";
8726+
8727+
message Bar {
8728+
map<string, Bar> message_map = 1;
8729+
map<string, string> string_map = 2;
8730+
}
8731+
)schema");
8732+
ASSERT_THAT(proto3, NotNull());
8733+
8734+
auto validate_maps = [](const FileDescriptor* file) {
8735+
const FieldDescriptor* message_map = file->message_type(0)->field(0);
8736+
EXPECT_FALSE(message_map->has_presence());
8737+
EXPECT_FALSE(message_map->message_type()->field(0)->has_presence());
8738+
EXPECT_TRUE(message_map->message_type()->field(1)->has_presence());
8739+
8740+
const FieldDescriptor* string_map = file->message_type(0)->field(1);
8741+
EXPECT_FALSE(string_map->has_presence());
8742+
EXPECT_FALSE(string_map->message_type()->field(0)->has_presence());
8743+
EXPECT_FALSE(string_map->message_type()->field(1)->has_presence());
8744+
};
8745+
validate_maps(editions);
8746+
validate_maps(proto3);
8747+
}
8748+
8749+
TEST_F(FeaturesTest, MapFieldFeaturesExplicitPresence) {
8750+
BuildDescriptorMessagesInTestPool();
8751+
const FileDescriptor* editions = ParseAndBuildFile("editions.proto", R"schema(
8752+
edition = "2023";
8753+
8754+
message Foo {
8755+
map<string, Foo> message_map = 1;
8756+
map<string, string> string_map = 2;
8757+
}
8758+
)schema");
8759+
ASSERT_THAT(editions, NotNull());
8760+
const FileDescriptor* proto2 = ParseAndBuildFile("google.protobuf.proto", R"schema(
8761+
syntax = "proto2";
8762+
8763+
message Bar {
8764+
map<string, Bar> message_map = 1;
8765+
map<string, string> string_map = 2;
8766+
}
8767+
)schema");
8768+
ASSERT_THAT(proto2, NotNull());
8769+
8770+
auto validate_maps = [](const FileDescriptor* file) {
8771+
const FieldDescriptor* message_map = file->message_type(0)->field(0);
8772+
EXPECT_FALSE(message_map->has_presence());
8773+
EXPECT_TRUE(message_map->message_type()->field(0)->has_presence());
8774+
EXPECT_TRUE(message_map->message_type()->field(1)->has_presence());
8775+
8776+
const FieldDescriptor* string_map = file->message_type(0)->field(1);
8777+
EXPECT_FALSE(string_map->has_presence());
8778+
EXPECT_TRUE(string_map->message_type()->field(0)->has_presence());
8779+
EXPECT_TRUE(string_map->message_type()->field(1)->has_presence());
8780+
};
8781+
validate_maps(editions);
8782+
validate_maps(proto2);
8783+
}
8784+
8785+
TEST_F(FeaturesTest, MapFieldFeaturesInheritedMessageEncoding) {
8786+
BuildDescriptorMessagesInTestPool();
8787+
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
8788+
edition = "2023";
8789+
8790+
option features.message_encoding = DELIMITED;
8791+
8792+
message Foo {
8793+
map<int32, Foo> message_map = 1;
8794+
map<string, string> string_map = 2;
8795+
}
8796+
)schema");
8797+
ASSERT_THAT(file, NotNull());
8798+
8799+
const FieldDescriptor* message_map = file->message_type(0)->field(0);
8800+
EXPECT_EQ(message_map->type(), FieldDescriptor::TYPE_MESSAGE);
8801+
EXPECT_EQ(message_map->message_type()->field(0)->type(),
8802+
FieldDescriptor::TYPE_INT32);
8803+
EXPECT_EQ(message_map->message_type()->field(1)->type(),
8804+
FieldDescriptor::TYPE_MESSAGE);
8805+
8806+
const FieldDescriptor* string_map = file->message_type(0)->field(1);
8807+
EXPECT_EQ(string_map->type(), FieldDescriptor::TYPE_MESSAGE);
8808+
EXPECT_EQ(string_map->message_type()->field(0)->type(),
8809+
FieldDescriptor::TYPE_STRING);
8810+
EXPECT_EQ(string_map->message_type()->field(1)->type(),
8811+
FieldDescriptor::TYPE_STRING);
8812+
}
8813+
87218814
TEST_F(FeaturesTest, RootExtensionFeaturesOverride) {
87228815
BuildDescriptorMessagesInTestPool();
87238816
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());

0 commit comments

Comments
 (0)