From 0dbd99a41db54a1adcfdc0946e9b8b724739a6e5 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Thu, 2 May 2024 15:07:47 -0700 Subject: [PATCH] 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 --- src/google/protobuf/descriptor.cc | 23 ++- src/google/protobuf/descriptor_unittest.cc | 159 ++++++++++++++++----- 2 files changed, 143 insertions(+), 39 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index e16e11db4d71..96df6f02bd4f 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -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 @@ -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 && @@ -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; + } } } @@ -6099,9 +6108,11 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( }); // Post-process cleanup for field features. - internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) { - PostProcessFieldFeatures(const_cast(field)); - }); + internal::VisitDescriptors( + *result, proto, + [&](const FieldDescriptor& field, const FieldDescriptorProto& proto) { + PostProcessFieldFeatures(const_cast(field), proto); + }); // Interpret any remaining uninterpreted options gathered into // options_to_interpret_ during descriptor building. Cross-linking has made diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index adb07750aeba..f2797b9f43a6 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -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. @@ -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"; @@ -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); @@ -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 { @@ -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) { @@ -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 message_map = 1; + map string_map = 2; + } + )schema"); + ASSERT_THAT(editions, NotNull()); + const FileDescriptor* proto3 = ParseAndBuildFile("proto3.proto", R"schema( + syntax = "proto3"; + + message Bar { + map message_map = 1; + map 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 message_map = 1; + map string_map = 2; + } + )schema"); + ASSERT_THAT(editions, NotNull()); + const FileDescriptor* proto2 = ParseAndBuildFile("google.protobuf.proto", R"schema( + syntax = "proto2"; + + message Bar { + map message_map = 1; + map 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 message_map = 1; + map 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());