Skip to content

Commit

Permalink
Clarify map behaviors in editions.
Browse files Browse the repository at this point in the history 
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 protocolbuffers#16549

PiperOrigin-RevId: 630191163
  • Loading branch information
@mkruskal-google
mkruskal-google committed May 2, 2024
1 parent 4114925 commit 61c9187
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 39 deletions.
23 changes: 17 additions & 6 deletions src/google/protobuf/descriptor.cc
View file
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 @@ -5532,7 +5533,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 @@ -5542,8 +5544,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 @@ -6089,9 +6098,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
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 @@ -8613,7 +8629,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 @@ -8630,22 +8648,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 @@ -8673,7 +8676,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 @@ -8687,21 +8691,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 @@ -8718,6 +8708,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

0 comments on commit 61c9187

Please sign in to comment.