Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify map behaviors in editions. #16576

Closed
wants to merge 1 commit into from
Closed

Clarify map behaviors in editions. #16576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/google/protobuf/descriptor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4362,7 +4362,8 @@ class DescriptorBuilder {
DescriptorPool::ErrorCollector::ErrorLocation error_location,
bool force_merge = false);

void PostProcessFieldFeatures(FieldDescriptor& field);
void PostProcessFieldFeatures(FieldDescriptor& field,
const FieldDescriptorProto& proto);

// Allocates an array of two strings, the first one is a copy of
// `proto_name`, and the second one is the full name. Full proto name is
Expand Down Expand Up @@ -5542,7 +5543,8 @@ void DescriptorBuilder::ResolveFeatures(const FileDescriptorProto& proto,
/*force_merge=*/true);
}

void DescriptorBuilder::PostProcessFieldFeatures(FieldDescriptor& field) {
void DescriptorBuilder::PostProcessFieldFeatures(
FieldDescriptor& field, const FieldDescriptorProto& proto) {
// TODO This can be replace by a runtime check in `is_required`
// once the `label` getter is hidden.
if (field.features().field_presence() == FeatureSet::LEGACY_REQUIRED &&
Expand All @@ -5552,8 +5554,15 @@ void DescriptorBuilder::PostProcessFieldFeatures(FieldDescriptor& field) {
// TODO This can be replace by a runtime check of `is_delimited`
// once the `TYPE_GROUP` value is removed.
if (field.type_ == FieldDescriptor::TYPE_MESSAGE &&
!field.containing_type()->options().map_entry() &&
field.features().message_encoding() == FeatureSet::DELIMITED) {
field.type_ = FieldDescriptor::TYPE_GROUP;
Symbol type =
LookupSymbol(proto.type_name(), field.full_name(),
DescriptorPool::PLACEHOLDER_MESSAGE, LOOKUP_TYPES, false);
if (type.descriptor() == nullptr ||
!type.descriptor()->options().map_entry()) {
field.type_ = FieldDescriptor::TYPE_GROUP;
}
}
}

Expand Down Expand Up @@ -6099,9 +6108,11 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
});

// Post-process cleanup for field features.
internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) {
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field));
});
internal::VisitDescriptors(
*result, proto,
[&](const FieldDescriptor& field, const FieldDescriptorProto& proto) {
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field), proto);
});

// Interpret any remaining uninterpreted options gathered into
// options_to_interpret_ during descriptor building. Cross-linking has made
Expand Down
159 changes: 126 additions & 33 deletions src/google/protobuf/descriptor_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4059,6 +4059,22 @@ class ValidationErrorTest : public testing::Test {
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
}

const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
absl::string_view file_text) {
io::ArrayInputStream input_stream(file_text.data(), file_text.size());
SimpleErrorCollector error_collector;
io::Tokenizer tokenizer(&input_stream, &error_collector);
compiler::Parser parser;
parser.RecordErrorsTo(&error_collector);
FileDescriptorProto proto;
ABSL_CHECK(parser.Parse(&tokenizer, &proto))
<< error_collector.last_error() << "\n"
<< file_text;
ABSL_CHECK_EQ("", error_collector.last_error());
proto.set_name(file_name);
return pool_.BuildFile(proto);
}


// Add file_proto to the DescriptorPool. Expect errors to be produced which
// match the given error text.
Expand Down Expand Up @@ -8684,7 +8700,9 @@ TEST_F(FeaturesTest, OneofFieldFeaturesOverride) {
}

TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
constexpr absl::string_view kProtoFile = R"schema(
BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
edition = "2023";

import "google/protobuf/unittest_features.proto";
Expand All @@ -8701,22 +8719,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
features.(pb.test).multiple_feature = VALUE3
];
}
)schema";
io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size());
SimpleErrorCollector error_collector;
io::Tokenizer tokenizer(&input_stream, &error_collector);
compiler::Parser parser;
parser.RecordErrorsTo(&error_collector);
FileDescriptorProto proto;
ASSERT_TRUE(parser.Parse(&tokenizer, &proto))
<< error_collector.last_error() << "\n"
<< kProtoFile;
ASSERT_EQ("", error_collector.last_error());
proto.set_name("foo.proto");

BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
const FileDescriptor* file = pool_.BuildFile(proto);
)schema");
ASSERT_THAT(file, NotNull());

const FieldDescriptor* map_field = file->message_type(0)->field(0);
Expand Down Expand Up @@ -8744,7 +8747,8 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
}

TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
constexpr absl::string_view kProtoFile = R"schema(
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
edition = "2023";

message Foo {
Expand All @@ -8758,21 +8762,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
features.utf8_validation = NONE
];
}
)schema";
io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size());
SimpleErrorCollector error_collector;
io::Tokenizer tokenizer(&input_stream, &error_collector);
compiler::Parser parser;
parser.RecordErrorsTo(&error_collector);
FileDescriptorProto proto;
ASSERT_TRUE(parser.Parse(&tokenizer, &proto))
<< error_collector.last_error() << "\n"
<< kProtoFile;
ASSERT_EQ("", error_collector.last_error());
proto.set_name("foo.proto");

BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = pool_.BuildFile(proto);
)schema");
ASSERT_THAT(file, NotNull());

auto validate_map_field = [](const FieldDescriptor* field) {
Expand All @@ -8789,6 +8779,109 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
validate_map_field(file->message_type(0)->field(2));
}

TEST_F(FeaturesTest, MapFieldFeaturesImplicitPresence) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* editions = ParseAndBuildFile("editions.proto", R"schema(
edition = "2023";

option features.field_presence = IMPLICIT;

message Foo {
map<string, Foo> message_map = 1;
map<string, string> string_map = 2;
}
)schema");
ASSERT_THAT(editions, NotNull());
const FileDescriptor* proto3 = ParseAndBuildFile("proto3.proto", R"schema(
syntax = "proto3";

message Bar {
map<string, Bar> message_map = 1;
map<string, string> string_map = 2;
}
)schema");
ASSERT_THAT(proto3, NotNull());

auto validate_maps = [](const FileDescriptor* file) {
const FieldDescriptor* message_map = file->message_type(0)->field(0);
EXPECT_FALSE(message_map->has_presence());
EXPECT_FALSE(message_map->message_type()->field(0)->has_presence());
EXPECT_TRUE(message_map->message_type()->field(1)->has_presence());

const FieldDescriptor* string_map = file->message_type(0)->field(1);
EXPECT_FALSE(string_map->has_presence());
EXPECT_FALSE(string_map->message_type()->field(0)->has_presence());
EXPECT_FALSE(string_map->message_type()->field(1)->has_presence());
};
validate_maps(editions);
validate_maps(proto3);
}

TEST_F(FeaturesTest, MapFieldFeaturesExplicitPresence) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* editions = ParseAndBuildFile("editions.proto", R"schema(
edition = "2023";

message Foo {
map<string, Foo> message_map = 1;
map<string, string> string_map = 2;
}
)schema");
ASSERT_THAT(editions, NotNull());
const FileDescriptor* proto2 = ParseAndBuildFile("google.protobuf.proto", R"schema(
syntax = "proto2";

message Bar {
map<string, Bar> message_map = 1;
map<string, string> string_map = 2;
}
)schema");
ASSERT_THAT(proto2, NotNull());

auto validate_maps = [](const FileDescriptor* file) {
const FieldDescriptor* message_map = file->message_type(0)->field(0);
EXPECT_FALSE(message_map->has_presence());
EXPECT_TRUE(message_map->message_type()->field(0)->has_presence());
EXPECT_TRUE(message_map->message_type()->field(1)->has_presence());

const FieldDescriptor* string_map = file->message_type(0)->field(1);
EXPECT_FALSE(string_map->has_presence());
EXPECT_TRUE(string_map->message_type()->field(0)->has_presence());
EXPECT_TRUE(string_map->message_type()->field(1)->has_presence());
};
validate_maps(editions);
validate_maps(proto2);
}

TEST_F(FeaturesTest, MapFieldFeaturesInheritedMessageEncoding) {
BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
edition = "2023";

option features.message_encoding = DELIMITED;

message Foo {
map<int32, Foo> message_map = 1;
map<string, string> string_map = 2;
}
)schema");
ASSERT_THAT(file, NotNull());

const FieldDescriptor* message_map = file->message_type(0)->field(0);
EXPECT_EQ(message_map->type(), FieldDescriptor::TYPE_MESSAGE);
EXPECT_EQ(message_map->message_type()->field(0)->type(),
FieldDescriptor::TYPE_INT32);
EXPECT_EQ(message_map->message_type()->field(1)->type(),
FieldDescriptor::TYPE_MESSAGE);

const FieldDescriptor* string_map = file->message_type(0)->field(1);
EXPECT_EQ(string_map->type(), FieldDescriptor::TYPE_MESSAGE);
EXPECT_EQ(string_map->message_type()->field(0)->type(),
FieldDescriptor::TYPE_STRING);
EXPECT_EQ(string_map->message_type()->field(1)->type(),
FieldDescriptor::TYPE_STRING);
}

TEST_F(FeaturesTest, RootExtensionFeaturesOverride) {
BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
Expand Down