Chiebot-Mirror
/
protobuf
mirror of https://github.com/protocolbuffers/protobuf.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Clarify map behaviors in editions.

Browse Source 
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
pull/16729/head
Mike Kruskal 7 months ago
parent 41149253ac
commit 61c91874c6
2 changed files with 143 additions and 39 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 19
      src/google/protobuf/descriptor.cc
  2. 159
      src/google/protobuf/descriptor_unittest.cc

19
src/google/protobuf/descriptor.cc
Unescape View File

@ -4362,7 +4362,8 @@ class DescriptorBuilder {
DescriptorPool::ErrorCollector::ErrorLocation error_location, DescriptorPool::ErrorCollector::ErrorLocation error_location,
bool force_merge = false); 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 // 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 // `proto_name`, and the second one is the full name. Full proto name is
@ -5532,7 +5533,8 @@ void DescriptorBuilder::ResolveFeatures(const FileDescriptorProto& proto,
/*force_merge=*/true); /*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` // TODO This can be replace by a runtime check in `is_required`
// once the `label` getter is hidden. // once the `label` getter is hidden.
if (field.features().field_presence() == FeatureSet::LEGACY_REQUIRED && if (field.features().field_presence() == FeatureSet::LEGACY_REQUIRED &&
@ -5542,9 +5544,16 @@ void DescriptorBuilder::PostProcessFieldFeatures(FieldDescriptor& field) {
// TODO This can be replace by a runtime check of `is_delimited` // TODO This can be replace by a runtime check of `is_delimited`
// once the `TYPE_GROUP` value is removed. // once the `TYPE_GROUP` value is removed.
if (field.type_ == FieldDescriptor::TYPE_MESSAGE && if (field.type_ == FieldDescriptor::TYPE_MESSAGE &&
!field.containing_type()->options().map_entry() &&
field.features().message_encoding() == FeatureSet::DELIMITED) { field.features().message_encoding() == FeatureSet::DELIMITED) {
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; field.type_ = FieldDescriptor::TYPE_GROUP;
} }
}
} }
// A common pattern: We want to convert a repeated field in the descriptor // A common pattern: We want to convert a repeated field in the descriptor
@ -6089,8 +6098,10 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
}); });
// Post-process cleanup for field features. // Post-process cleanup for field features.
internal::VisitDescriptors(*result, [&](const FieldDescriptor& field) { internal::VisitDescriptors(
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field)); *result, proto,
[&](const FieldDescriptor& field, const FieldDescriptorProto& proto) {
PostProcessFieldFeatures(const_cast<FieldDescriptor&>(field), proto);
}); });
// Interpret any remaining uninterpreted options gathered into // Interpret any remaining uninterpreted options gathered into

159
src/google/protobuf/descriptor_unittest.cc
Unescape View File

@ -4059,6 +4059,22 @@ class ValidationErrorTest : public testing::Test {
return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto)); 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 // Add file_proto to the DescriptorPool. Expect errors to be produced which
// match the given error text. // match the given error text.
@ -8613,7 +8629,9 @@ TEST_F(FeaturesTest, OneofFieldFeaturesOverride) {
} }
TEST_F(FeaturesTest, MapFieldFeaturesOverride) { 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"; edition = "2023";
import "google/protobuf/unittest_features.proto"; import "google/protobuf/unittest_features.proto";
@ -8630,22 +8648,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
features.(pb.test).multiple_feature = VALUE3 features.(pb.test).multiple_feature = VALUE3
]; ];
} }
)schema"; )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);
ASSERT_THAT(file, NotNull()); ASSERT_THAT(file, NotNull());
const FieldDescriptor* map_field = file->message_type(0)->field(0); const FieldDescriptor* map_field = file->message_type(0)->field(0);
@ -8673,7 +8676,8 @@ TEST_F(FeaturesTest, MapFieldFeaturesOverride) {
} }
TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) { TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
constexpr absl::string_view kProtoFile = R"schema( BuildDescriptorMessagesInTestPool();
const FileDescriptor* file = ParseAndBuildFile("foo.proto", R"schema(
edition = "2023"; edition = "2023";
message Foo { message Foo {
@ -8687,21 +8691,7 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
features.utf8_validation = NONE features.utf8_validation = NONE
]; ];
} }
)schema"; )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);
ASSERT_THAT(file, NotNull()); ASSERT_THAT(file, NotNull());
auto validate_map_field = [](const FieldDescriptor* field) { auto validate_map_field = [](const FieldDescriptor* field) {
@ -8718,6 +8708,109 @@ TEST_F(FeaturesTest, MapFieldFeaturesStringValidation) {
validate_map_field(file->message_type(0)->field(2)); 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) { TEST_F(FeaturesTest, RootExtensionFeaturesOverride) {
BuildDescriptorMessagesInTestPool(); BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); BuildFileInTestPool(pb::TestFeatures::descriptor()->file());

Write Preview
Loading…
Cancel
Save