From b38749245c6ec3d7280c70612bdbb3f902bb1ad6 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Fri, 20 Dec 2024 12:51:38 -0800 Subject: [PATCH] Fix type resolver to preserve explicit set feature values. PiperOrigin-RevId: 708402850 --- src/google/protobuf/util/BUILD.bazel | 3 + .../protobuf/util/type_resolver_util.cc | 65 +++++---- .../protobuf/util/type_resolver_util_test.cc | 127 +++++++++++++++--- 3 files changed, 149 insertions(+), 46 deletions(-) diff --git a/src/google/protobuf/util/BUILD.bazel b/src/google/protobuf/util/BUILD.bazel index 5522789089..89bea6c58b 100644 --- a/src/google/protobuf/util/BUILD.bazel +++ b/src/google/protobuf/util/BUILD.bazel @@ -236,11 +236,14 @@ cc_test( "//src/google/protobuf:any_cc_proto", "//src/google/protobuf:cc_test_protos", "//src/google/protobuf:descriptor_legacy", + "//src/google/protobuf:test_textproto", "//src/google/protobuf:test_util", "//src/google/protobuf:type_cc_proto", "//src/google/protobuf:wrappers_cc_proto", "//src/google/protobuf/testing", "//src/google/protobuf/testing:file", + "@com_google_absl//absl/log:absl_check", + "@com_google_absl//absl/strings", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/src/google/protobuf/util/type_resolver_util.cc b/src/google/protobuf/util/type_resolver_util.cc index 3efe45060d..d1e481a972 100644 --- a/src/google/protobuf/util/type_resolver_util.cc +++ b/src/google/protobuf/util/type_resolver_util.cc @@ -21,7 +21,6 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" -#include "google/protobuf/descriptor_legacy.h" #include "google/protobuf/io/strtod.h" #include "google/protobuf/util/type_resolver.h" @@ -217,7 +216,8 @@ std::string GetTypeUrl(absl::string_view url_prefix, const T& descriptor) { } void ConvertFieldDescriptor(absl::string_view url_prefix, - const FieldDescriptor& descriptor, Field* field) { + const FieldDescriptor& descriptor, + const FieldDescriptorProto& proto, Field* field) { field->set_kind(static_cast(descriptor.type())); switch (descriptor.label()) { case FieldDescriptor::LABEL_OPTIONAL: @@ -249,24 +249,28 @@ void ConvertFieldDescriptor(absl::string_view url_prefix, field->set_packed(true); } - ConvertFieldOptions(descriptor.options(), *field->mutable_options()); + ConvertFieldOptions(proto.options(), *field->mutable_options()); } -Syntax ConvertSyntax(Edition edition) { - switch (edition) { - case Edition::EDITION_PROTO2: - return Syntax::SYNTAX_PROTO2; - case Edition::EDITION_PROTO3: - return Syntax::SYNTAX_PROTO3; - default: - return Syntax::SYNTAX_EDITIONS; +Syntax ConvertSyntax(absl::string_view syntax) { + if (syntax == "proto2" || syntax.empty()) { + return Syntax::SYNTAX_PROTO2; } + if (syntax == "proto3") { + return Syntax::SYNTAX_PROTO3; + } + + return Syntax::SYNTAX_EDITIONS; } -void ConvertEnumDescriptor(const EnumDescriptor& descriptor, Enum* enum_type) { +void ConvertEnumDescriptor(const EnumDescriptor& descriptor, + const FileDescriptorProto& file, + const EnumDescriptorProto& proto, Enum* enum_type) { enum_type->Clear(); - enum_type->set_syntax( - ConvertSyntax(FileDescriptorLegacy(descriptor.file()).edition())); + enum_type->set_syntax(ConvertSyntax(file.syntax())); + if (enum_type->syntax() == Syntax::SYNTAX_EDITIONS) { + enum_type->set_edition(absl::StrCat(file.edition())); + } enum_type->set_name(descriptor.full_name()); enum_type->mutable_source_context()->set_file_name(descriptor.file()->name()); @@ -276,28 +280,32 @@ void ConvertEnumDescriptor(const EnumDescriptor& descriptor, Enum* enum_type) { value->set_name(value_descriptor.name()); value->set_number(value_descriptor.number()); - ConvertEnumValueOptions(value_descriptor.options(), + ConvertEnumValueOptions(proto.value(i).options(), *value->mutable_options()); } - ConvertEnumOptions(descriptor.options(), *enum_type->mutable_options()); + ConvertEnumOptions(proto.options(), *enum_type->mutable_options()); } void ConvertDescriptor(absl::string_view url_prefix, - const Descriptor& descriptor, Type* type) { + const Descriptor& descriptor, + const FileDescriptorProto& file, + const DescriptorProto& proto, Type* type) { type->Clear(); type->set_name(descriptor.full_name()); - type->set_syntax( - ConvertSyntax(FileDescriptorLegacy(descriptor.file()).edition())); + type->set_syntax(ConvertSyntax(file.syntax())); + if (type->syntax() == Syntax::SYNTAX_EDITIONS) { + type->set_edition(absl::StrCat(file.edition())); + } for (int i = 0; i < descriptor.field_count(); ++i) { - ConvertFieldDescriptor(url_prefix, *descriptor.field(i), + ConvertFieldDescriptor(url_prefix, *descriptor.field(i), proto.field(i), type->add_fields()); } for (int i = 0; i < descriptor.oneof_decl_count(); ++i) { type->add_oneofs(descriptor.oneof_decl(i)->name()); } type->mutable_source_context()->set_file_name(descriptor.file()->name()); - ConvertMessageOptions(descriptor.options(), *type->mutable_options()); + ConvertMessageOptions(proto.options(), *type->mutable_options()); } class DescriptorPoolTypeResolver : public TypeResolver { @@ -319,7 +327,7 @@ class DescriptorPoolTypeResolver : public TypeResolver { return absl::NotFoundError( absl::StrCat("Invalid type URL, unknown type: ", type_name)); } - ConvertDescriptor(url_prefix_, *descriptor, type); + *type = ConvertDescriptorToType(url_prefix_, *descriptor); return absl::Status(); } @@ -336,7 +344,7 @@ class DescriptorPoolTypeResolver : public TypeResolver { return absl::InvalidArgumentError( absl::StrCat("Invalid type URL, unknown type: ", type_name)); } - ConvertEnumDescriptor(*descriptor, enum_type); + *enum_type = ConvertDescriptorToType(*descriptor); return absl::Status(); } @@ -369,14 +377,21 @@ TypeResolver* NewTypeResolverForDescriptorPool(absl::string_view url_prefix, Type ConvertDescriptorToType(absl::string_view url_prefix, const Descriptor& descriptor) { Type type; - ConvertDescriptor(url_prefix, descriptor, &type); + FileDescriptorProto proto; + descriptor.file()->CopyHeadingTo(&proto); + descriptor.CopyTo(proto.add_message_type()); + ConvertDescriptor(url_prefix, descriptor, proto, proto.message_type(0), + &type); return type; } // Performs a direct conversion from an enum descriptor to a type proto. Enum ConvertDescriptorToType(const EnumDescriptor& descriptor) { Enum enum_type; - ConvertEnumDescriptor(descriptor, &enum_type); + FileDescriptorProto proto; + descriptor.file()->CopyHeadingTo(&proto); + descriptor.CopyTo(proto.add_enum_type()); + ConvertEnumDescriptor(descriptor, proto, proto.enum_type(0), &enum_type); return enum_type; } diff --git a/src/google/protobuf/util/type_resolver_util_test.cc b/src/google/protobuf/util/type_resolver_util_test.cc index 5417633e08..1b52348ba4 100644 --- a/src/google/protobuf/util/type_resolver_util_test.cc +++ b/src/google/protobuf/util/type_resolver_util_test.cc @@ -8,25 +8,24 @@ #include "google/protobuf/util/type_resolver_util.h" #include -#include #include #include -#include #include "google/protobuf/any.pb.h" #include "google/protobuf/type.pb.h" #include "google/protobuf/wrappers.pb.h" #include "google/protobuf/descriptor.pb.h" -#include "google/protobuf/util/type_resolver.h" -#include #include +#include "absl/log/absl_check.h" +#include "absl/strings/string_view.h" #include "google/protobuf/descriptor.h" -#include "google/protobuf/descriptor_legacy.h" #include "google/protobuf/util/json_format_proto3.pb.h" #include "google/protobuf/map_unittest.pb.h" +#include "google/protobuf/test_textproto.h" #include "google/protobuf/unittest.pb.h" #include "google/protobuf/unittest_custom_options.pb.h" #include "google/protobuf/unittest_import.pb.h" +#include "google/protobuf/util/type_resolver.h" namespace google { namespace protobuf { @@ -421,18 +420,8 @@ class DescriptorPoolTypeResolverSyntaxTest : public testing::Test { DescriptorPoolTypeResolverSyntaxTest() : resolver_(NewTypeResolverForDescriptorPool(kUrlPrefix, &pool_)) {} - const FileDescriptor* BuildFile( - absl::string_view syntax, - absl::optional edition = absl::nullopt) { - FileDescriptorProto proto; - proto.set_package("test"); - proto.set_name("foo"); - proto.set_syntax(syntax); - if (edition.has_value()) { - proto.set_edition(*edition); - } - DescriptorProto* message = proto.add_message_type(); - message->set_name("MyMessage"); + const FileDescriptor* BuildFile(absl::string_view file_contents) { + FileDescriptorProto proto = ParseTextOrDie(file_contents); const FileDescriptor* file = pool_.BuildFile(proto); ABSL_CHECK(file != nullptr); return file; @@ -443,8 +432,12 @@ class DescriptorPoolTypeResolverSyntaxTest : public testing::Test { }; TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto2) { - const FileDescriptor* file = BuildFile("proto2"); - ASSERT_EQ(FileDescriptorLegacy(file).edition(), Edition::EDITION_PROTO2); + BuildFile(R"pb( + package: "test" + name: "foo" + syntax: "proto2" + message_type { name: "MyMessage" } + )pb"); Type type; ASSERT_TRUE( @@ -454,8 +447,12 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto2) { } TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto3) { - const FileDescriptor* file = BuildFile("proto3"); - ASSERT_EQ(FileDescriptorLegacy(file).edition(), Edition::EDITION_PROTO3); + BuildFile(R"pb( + package: "test" + name: "foo" + syntax: "proto3" + message_type { name: "MyMessage" } + )pb"); Type type; ASSERT_TRUE( @@ -464,6 +461,94 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto3) { EXPECT_EQ(type.edition(), ""); } +TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxEditions) { + BuildFile(R"pb( + package: "test" + name: "foo" + syntax: "editions" + edition: EDITION_2023 + message_type { name: "MyMessage" } + )pb"); + + Type type; + ASSERT_TRUE( + resolver_->ResolveMessageType(GetTypeUrl("test.MyMessage"), &type).ok()); + EXPECT_EQ(type.syntax(), Syntax::SYNTAX_EDITIONS); + EXPECT_EQ(type.edition(), "2023"); +} + +TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsFieldFeatures) { + BuildFile(R"pb( + package: "test" + name: "foo" + syntax: "editions" + edition: EDITION_2023 + message_type { + name: "MyMessage" + field { + name: "field" + number: 1 + type: TYPE_BYTES + options { + features { + [pb.cpp] { string_type: CORD } + } + } + } + } + )pb"); + + Type type; + ASSERT_TRUE( + resolver_->ResolveMessageType(GetTypeUrl("test.MyMessage"), &type).ok()); + ASSERT_EQ(type.fields_size(), 1); + EXPECT_THAT(type.fields(0), EqualsProto(R"pb( + kind: TYPE_BYTES + cardinality: CARDINALITY_OPTIONAL + number: 1 + name: "field" + options { + name: "features" + value { + [type.googleapis.com/google.protobuf.FeatureSet] { + [pb.cpp] { string_type: CORD } + } + } + } + json_name: "field" + )pb")); +} + +TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsEnumFeatures) { + BuildFile(R"pb( + package: "test" + name: "foo" + syntax: "editions" + edition: EDITION_2023 + enum_type { + name: "MyEnum" + value: { name: "FOO" number: 1 } + options { features { enum_type: CLOSED } } + } + )pb"); + + Enum enm; + ASSERT_TRUE(resolver_->ResolveEnumType(GetTypeUrl("test.MyEnum"), &enm).ok()); + EXPECT_THAT( + enm, EqualsProto(R"pb( + name: "test.MyEnum" + enumvalue { name: "FOO" number: 1 } + options { + name: "features" + value { + [type.googleapis.com/google.protobuf.FeatureSet] { enum_type: CLOSED } + } + } + source_context { file_name: "foo" } + syntax: SYNTAX_EDITIONS + edition: "2023" + )pb")); +} TEST(ConvertDescriptorToTypeTest, TestAllTypes) { Type type = ConvertDescriptorToType(