From 15863ffc9aff9d31e9f25a7f11a2d499b0170bc3 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Tue, 15 Aug 2023 13:30:13 -0700 Subject: [PATCH] Editions: Include defaults for any features in the generated pool. This helps make the API more complete, since the FeatureSet object will always be fully resolved on any accessible features. This specifically targets C++ plugins though, which will now have their features filled in by default. Before, any proto files that didn't include the language-specific features would result in unresolved extensions in the generators. PiperOrigin-RevId: 557232654 --- src/google/protobuf/BUILD.bazel | 1 - .../command_line_interface_unittest.cc | 8 + src/google/protobuf/descriptor_unittest.cc | 223 +++++++++++++----- src/google/protobuf/editions/BUILD | 20 +- .../protobuf/editions/generated_files_test.cc | 56 ++++- .../proto/editions_default_features.proto} | 24 +- src/google/protobuf/feature_resolver.cc | 101 ++++++-- src/google/protobuf/feature_resolver.h | 17 +- src/google/protobuf/feature_resolver_test.cc | 129 ++++++++-- 9 files changed, 452 insertions(+), 127 deletions(-) rename src/google/protobuf/{unittest_invalid_features.proto => editions/proto/editions_default_features.proto} (77%) diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index ae6012349aa81..9284b315eb5c1 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -685,7 +685,6 @@ filegroup( "unittest_features.proto", "unittest_import.proto", "unittest_import_public.proto", - "unittest_invalid_features.proto", "unittest_lite_imports_nonlite.proto", "unittest_mset.proto", "unittest_mset_wire_format.proto", diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 66e81f05237f1..af99450f50692 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1564,6 +1564,10 @@ TEST_F(CommandLineInterfaceTest, Plugin_RuntimeFeatures) { message_encoding: LENGTH_PREFIXED json_format: ALLOW raw_features { field_presence: IMPLICIT } + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + } )pb")); EXPECT_THAT(request.source_file_descriptors(0) .message_type(0) @@ -1577,6 +1581,10 @@ TEST_F(CommandLineInterfaceTest, Plugin_RuntimeFeatures) { message_encoding: LENGTH_PREFIXED json_format: ALLOW raw_features { field_presence: IMPLICIT } + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + } )pb")); } diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index f1430a8d659a2..51e483cecd2a3 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -69,7 +69,6 @@ #include "google/protobuf/unittest.pb.h" #include "google/protobuf/unittest_custom_options.pb.h" #include "google/protobuf/unittest_features.pb.h" -#include "google/protobuf/unittest_invalid_features.pb.h" #include "google/protobuf/unittest_lazy_dependencies.pb.h" #include "google/protobuf/unittest_lazy_dependencies_custom_option.pb.h" #include "google/protobuf/unittest_lazy_dependencies_enum.pb.h" @@ -7179,6 +7178,16 @@ const FeatureSet& GetFeatures(const T* descriptor) { return internal::InternalFeatureHelper::GetFeatures(*descriptor); } +template +FeatureSet GetCoreFeatures(const T* descriptor) { + FeatureSet features = GetFeatures(descriptor); + // Strip test features to avoid excessive brittleness. + features.ClearExtension(pb::test); + features.ClearExtension(pb::TestMessage::test_message); + features.ClearExtension(pb::TestMessage::Nested::test_nested); + return features; +} + TEST_F(FeaturesTest, InvalidProto2Features) { BuildDescriptorMessagesInTestPool(); BuildFileWithErrors( @@ -7440,6 +7449,32 @@ TEST_F(FeaturesTest, Proto3Extensions) { R"pb([bar_ext] { baz: 1 })pb")); } +TEST_F(FeaturesTest, Edition2023Defaults) { + FileDescriptorProto file_proto = + ParseTextOrDie(R"pb( + name: "foo.proto" syntax: "editions" edition: "2023" + )pb"); + + BuildDescriptorMessagesInTestPool(); + const FileDescriptor* file = ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto)); + EXPECT_THAT(file->options(), EqualsProto("")); + EXPECT_THAT( + GetCoreFeatures(file), EqualsProto(R"pb( + field_presence: EXPLICIT + enum_type: OPEN + repeated_field_encoding: PACKED + string_field_validation: MANDATORY + message_encoding: LENGTH_PREFIXED + json_format: ALLOW + [pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE } + )pb")); + + // Since pb::test is linked in, it should end up with defaults in our + // FeatureSet. + EXPECT_TRUE(GetFeatures(file).HasExtension(pb::test)); + EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 1); +} + TEST_F(FeaturesTest, ClearsOptions) { BuildDescriptorMessagesInTestPool(); const FileDescriptor* file = BuildFile(R"pb( @@ -7452,13 +7487,17 @@ TEST_F(FeaturesTest, ClearsOptions) { } )pb"); EXPECT_THAT(file->options(), EqualsProto("java_package: 'bar'")); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, RestoresOptionsRoundTrip) { @@ -7711,13 +7750,17 @@ TEST_F(FeaturesTest, NoOptions) { name: "foo.proto" syntax: "editions" edition: "2023" )pb"); EXPECT_EQ(&file->options(), &FileOptions::default_instance()); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, InvalidEdition) { @@ -7740,13 +7783,17 @@ TEST_F(FeaturesTest, FileFeatures) { options { features { field_presence: IMPLICIT } } )pb"); EXPECT_THAT(file->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, FileFeaturesExtension) { @@ -7761,7 +7808,7 @@ TEST_F(FeaturesTest, FileFeaturesExtension) { )pb"); EXPECT_THAT(file->options(), EqualsProto("")); EXPECT_EQ(GetFeatures(file).field_presence(), FeatureSet::IMPLICIT); - EXPECT_EQ(GetFeatures(file).string_field_validation(), FeatureSet::MANDATORY); + EXPECT_EQ(GetFeatures(file).enum_type(), FeatureSet::OPEN); EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 4); EXPECT_EQ(GetFeatures(file) .GetExtension(pb::TestMessage::test_message) @@ -7792,7 +7839,7 @@ TEST_F(FeaturesTest, FileFeaturesExtensionOverride) { )pb"); EXPECT_THAT(file->options(), EqualsProto("")); EXPECT_EQ(GetFeatures(file).field_presence(), FeatureSet::IMPLICIT); - EXPECT_EQ(GetFeatures(file).string_field_validation(), FeatureSet::MANDATORY); + EXPECT_EQ(GetFeatures(file).enum_type(), FeatureSet::OPEN); EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 9); EXPECT_EQ(GetFeatures(file) .GetExtension(pb::TestMessage::test_message) @@ -7814,13 +7861,17 @@ TEST_F(FeaturesTest, MessageFeaturesDefault) { )pb"); const Descriptor* message = file->message_type(0); EXPECT_THAT(message->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(message), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(message), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, MessageFeaturesInherit) { @@ -7918,13 +7969,17 @@ TEST_F(FeaturesTest, FieldFeaturesDefault) { )pb"); const FieldDescriptor* field = file->message_type(0)->field(0); EXPECT_THAT(field->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(field), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(field), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, FieldFeaturesInherit) { @@ -7968,7 +8023,7 @@ TEST_F(FeaturesTest, FieldFeaturesOverride) { dependency: "google/protobuf/unittest_features.proto" options { features { - string_field_validation: HINT + enum_type: CLOSED field_presence: IMPLICIT [pb.test] { int_multiple_feature: 2 } } @@ -7987,7 +8042,7 @@ TEST_F(FeaturesTest, FieldFeaturesOverride) { type: TYPE_STRING options { features { - string_field_validation: NONE + field_presence: EXPLICIT [pb.test] { int_multiple_feature: 9 } } } @@ -7996,8 +8051,8 @@ TEST_F(FeaturesTest, FieldFeaturesOverride) { )pb"); const FieldDescriptor* field = file->message_type(0)->field(0); EXPECT_THAT(field->options(), EqualsProto("")); - EXPECT_EQ(GetFeatures(field).field_presence(), FeatureSet::IMPLICIT); - EXPECT_EQ(GetFeatures(field).string_field_validation(), FeatureSet::NONE); + EXPECT_EQ(GetFeatures(field).field_presence(), FeatureSet::EXPLICIT); + EXPECT_EQ(GetFeatures(field).enum_type(), FeatureSet::CLOSED); EXPECT_EQ(GetFeatures(field).GetExtension(pb::test).int_multiple_feature(), 9); } @@ -8074,7 +8129,6 @@ TEST_F(FeaturesTest, OneofFieldFeaturesOverride) { type: TYPE_STRING options { features { - string_field_validation: NONE [pb.test] { int_multiple_feature: 9 } } } @@ -8213,7 +8267,7 @@ TEST_F(FeaturesTest, RootExtensionFeaturesOverride) { dependency: "google/protobuf/unittest_features.proto" options { features { - string_field_validation: HINT + enum_type: CLOSED field_presence: IMPLICIT [pb.test] { int_multiple_feature: 2 } } @@ -8225,7 +8279,7 @@ TEST_F(FeaturesTest, RootExtensionFeaturesOverride) { type: TYPE_STRING options { features { - string_field_validation: NONE + enum_type: OPEN [pb.test] { int_multiple_feature: 9 } } } @@ -8239,7 +8293,7 @@ TEST_F(FeaturesTest, RootExtensionFeaturesOverride) { const FieldDescriptor* field = file->extension(0); EXPECT_THAT(field->options(), EqualsProto("")); EXPECT_EQ(GetFeatures(field).field_presence(), FeatureSet::IMPLICIT); - EXPECT_EQ(GetFeatures(field).string_field_validation(), FeatureSet::NONE); + EXPECT_EQ(GetFeatures(field).enum_type(), FeatureSet::OPEN); EXPECT_EQ(GetFeatures(field).GetExtension(pb::test).int_multiple_feature(), 9); } @@ -8254,7 +8308,7 @@ TEST_F(FeaturesTest, MessageExtensionFeaturesOverride) { dependency: "google/protobuf/unittest_features.proto" options { features { - string_field_validation: HINT + enum_type: CLOSED field_presence: IMPLICIT [pb.test] { int_multiple_feature: 2 } } @@ -8271,7 +8325,7 @@ TEST_F(FeaturesTest, MessageExtensionFeaturesOverride) { number: 1 label: LABEL_OPTIONAL type: TYPE_STRING - options { features { string_field_validation: NONE } } + options { features { enum_type: OPEN } } extendee: "Foo2" } } @@ -8288,7 +8342,7 @@ TEST_F(FeaturesTest, MessageExtensionFeaturesOverride) { const FieldDescriptor* field = file->message_type(0)->extension(0); EXPECT_THAT(field->options(), EqualsProto("")); EXPECT_EQ(GetFeatures(field).field_presence(), FeatureSet::IMPLICIT); - EXPECT_EQ(GetFeatures(field).string_field_validation(), FeatureSet::NONE); + EXPECT_EQ(GetFeatures(field).enum_type(), FeatureSet::OPEN); EXPECT_EQ(GetFeatures(field).GetExtension(pb::test).int_multiple_feature(), 3); } @@ -8306,13 +8360,17 @@ TEST_F(FeaturesTest, EnumFeaturesDefault) { )pb"); const EnumDescriptor* enm = file->enum_type(0); EXPECT_THAT(enm->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(enm), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(enm), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, EnumFeaturesInherit) { @@ -8412,13 +8470,17 @@ TEST_F(FeaturesTest, EnumValueFeaturesDefault) { )pb"); const EnumValueDescriptor* value = file->enum_type(0)->value(0); EXPECT_THAT(value->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(value), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(value), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, EnumValueFeaturesInherit) { @@ -8503,13 +8565,17 @@ TEST_F(FeaturesTest, OneofFeaturesDefault) { )pb"); const OneofDescriptor* oneof = file->message_type(0)->oneof_decl(0); EXPECT_THAT(oneof->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(oneof), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(oneof), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, OneofFeaturesInherit) { @@ -8601,13 +8667,17 @@ TEST_F(FeaturesTest, ExtensionRangeFeaturesDefault) { const Descriptor::ExtensionRange* range = file->message_type(0)->extension_range(0); EXPECT_THAT(range->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(range), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(range), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, ExtensionRangeFeaturesInherit) { @@ -8684,13 +8754,17 @@ TEST_F(FeaturesTest, ServiceFeaturesDefault) { )pb"); const ServiceDescriptor* service = file->service(0); EXPECT_THAT(service->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(service), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(service), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, ServiceFeaturesInherit) { @@ -8749,13 +8823,17 @@ TEST_F(FeaturesTest, MethodFeaturesDefault) { )pb"); const MethodDescriptor* method = file->service(0)->method(0); EXPECT_THAT(method->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(method), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(method), EqualsProto(R"pb( field_presence: EXPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, MethodFeaturesInherit) { @@ -9006,23 +9084,44 @@ TEST_F(FeaturesTest, FeaturesOutsideEditions) { TEST_F(FeaturesTest, InvalidExtensionNonMessage) { BuildDescriptorMessagesInTestPool(); - BuildFileInTestPool(::pb::TestInvalid::descriptor()->file()); + ASSERT_NE(BuildFile(R"pb( + name: "unittest_invalid_features.proto" + syntax: "proto2" + package: "pb" + dependency: "google/protobuf/descriptor.proto" + message_type { + name: "TestInvalid" + extension { + name: "scalar_extension" + number: 9996 + label: LABEL_OPTIONAL + type: TYPE_STRING + extendee: ".google.protobuf.FeatureSet" + } + } + )pb"), + nullptr); BuildFileWithErrors( R"pb( name: "foo.proto" syntax: "editions" edition: "2023" - dependency: "google/protobuf/unittest_invalid_features.proto" + dependency: "unittest_invalid_features.proto" options { - features { - [pb.TestInvalid.scalar_extension]: "hello" + uninterpreted_option { + name { name_part: "features" is_extension: false } + name { + name_part: "pb.TestInvalid.scalar_extension" + is_extension: true + } + identifier_value: "hello" } } )pb", - "foo.proto: google/protobuf/unittest_invalid_features.proto: " - "EDITIONS: FeatureSet extension pb.TestInvalid.scalar_extension is not " - "of message type. Feature extensions should always use messages to " - "allow for evolution.\n"); + "foo.proto: unittest_invalid_features.proto: EDITIONS: FeatureSet " + "extension pb.TestInvalid.scalar_extension is not of message type. " + "Feature extensions should always use messages to allow for " + "evolution.\n"); } TEST_F(FeaturesTest, InvalidFieldImplicitDefault) { @@ -9416,13 +9515,17 @@ TEST_F(FeaturesTest, UninterpretedOptions) { } )pb"); EXPECT_THAT(file->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, UninterpretedOptionsMerge) { @@ -9434,8 +9537,8 @@ TEST_F(FeaturesTest, UninterpretedOptionsMerge) { options { uninterpreted_option { name { name_part: "features" is_extension: false } - name { name_part: "string_field_validation" is_extension: false } - identifier_value: "HINT" + name { name_part: "enum_type" is_extension: false } + identifier_value: "CLOSED" } } message_type { @@ -9448,8 +9551,8 @@ TEST_F(FeaturesTest, UninterpretedOptionsMerge) { options { uninterpreted_option { name { name_part: "features" is_extension: false } - name { name_part: "string_field_validation" is_extension: false } - identifier_value: "NONE" + name { name_part: "enum_type" is_extension: false } + identifier_value: "OPEN" } } } @@ -9458,8 +9561,8 @@ TEST_F(FeaturesTest, UninterpretedOptionsMerge) { const FieldDescriptor* field = file->message_type(0)->field(0); EXPECT_THAT(file->options(), EqualsProto("")); EXPECT_THAT(field->options(), EqualsProto("")); - EXPECT_EQ(GetFeatures(file).string_field_validation(), FeatureSet::HINT); - EXPECT_EQ(GetFeatures(field).string_field_validation(), FeatureSet::NONE); + EXPECT_EQ(GetFeatures(file).enum_type(), FeatureSet::CLOSED); + EXPECT_EQ(GetFeatures(field).enum_type(), FeatureSet::OPEN); } TEST_F(FeaturesTest, UninterpretedOptionsMergeExtension) { @@ -9540,13 +9643,17 @@ TEST_F(FeaturesTest, RawFeatures) { options { features { raw_features { field_presence: IMPLICIT } } } )pb"); EXPECT_THAT(file->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, RawFeaturesConflict) { @@ -9563,13 +9670,17 @@ TEST_F(FeaturesTest, RawFeaturesConflict) { } )pb"); EXPECT_THAT(file->options(), EqualsProto("")); - EXPECT_THAT(GetFeatures(file), EqualsProto(R"pb( + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( field_presence: IMPLICIT enum_type: OPEN repeated_field_encoding: PACKED string_field_validation: MANDATORY message_encoding: LENGTH_PREFIXED - json_format: ALLOW)pb")); + json_format: ALLOW + [pb.cpp] { + legacy_closed_enum: false + utf8_validation: VERIFY_PARSE + })pb")); } TEST_F(FeaturesTest, InvalidJsonUniquenessDefaultWarning) { diff --git a/src/google/protobuf/editions/BUILD b/src/google/protobuf/editions/BUILD index 9cf1f25307295..8b31ffda8197a 100644 --- a/src/google/protobuf/editions/BUILD +++ b/src/google/protobuf/editions/BUILD @@ -7,6 +7,11 @@ proto_library( deps = ["//src/google/protobuf:cpp_features_proto"], ) +cc_proto_library( + name = "test_messages_proto2_cc_proto", + deps = [":test_messages_proto2_proto"], +) + proto_library( name = "test_messages_proto3_proto", srcs = ["golden/test_messages_proto3.proto"], @@ -23,19 +28,26 @@ proto_library( ) cc_proto_library( - name = "test_messages_proto2_cc_proto", - deps = [":test_messages_proto2_proto"], + name = "test_messages_proto3_cc_proto", + deps = [":test_messages_proto3_proto"], +) + +proto_library( + name = "editions_default_features_proto", + srcs = ["proto/editions_default_features.proto"], + strip_import_prefix = "/src", ) cc_proto_library( - name = "test_messages_proto3_cc_proto", - deps = [":test_messages_proto3_proto"], + name = "editions_default_features_cc_proto", + deps = [":editions_default_features_proto"], ) cc_test( name = "generated_files_test", srcs = ["generated_files_test.cc"], deps = [ + ":editions_default_features_cc_proto", ":test_messages_proto2_cc_proto", ":test_messages_proto3_cc_proto", "//src/google/protobuf", diff --git a/src/google/protobuf/editions/generated_files_test.cc b/src/google/protobuf/editions/generated_files_test.cc index c772230e27874..0f9ab590612e9 100644 --- a/src/google/protobuf/editions/generated_files_test.cc +++ b/src/google/protobuf/editions/generated_files_test.cc @@ -33,6 +33,7 @@ #include "google/protobuf/descriptor.h" #include "google/protobuf/editions/golden/test_messages_proto2.pb.h" #include "google/protobuf/editions/golden/test_messages_proto3.pb.h" +#include "google/protobuf/editions/proto/editions_default_features.pb.h" #include "google/protobuf/test_textproto.h" // These tests provide some basic minimal coverage that protos work as expected. @@ -42,12 +43,13 @@ namespace google { namespace protobuf { namespace { +using ::protobuf_editions_test::EditionsDefaultMessage; using ::protobuf_test_messages::proto2::TestAllRequiredTypesProto2; using ::protobuf_test_messages::proto2::TestAllTypesProto2; using ::protobuf_test_messages::proto3::TestAllTypesProto3; using ::testing::NotNull; -TEST(Gemerated, Parsing) { +TEST(Generated, Parsing) { TestAllTypesProto2 message = ParseTextOrDie(R"pb( Data { group_int32: 9 } )pb"); @@ -55,21 +57,21 @@ TEST(Gemerated, Parsing) { EXPECT_EQ(message.data().group_int32(), 9); } -TEST(Gemerated, GeneratedMethods) { +TEST(Generated, GeneratedMethods) { TestAllTypesProto3 message; message.set_optional_int32(9); EXPECT_THAT(message, EqualsProto(R"pb(optional_int32: 9)pb")); } -TEST(Gemerated, ExplicitPresence) { +TEST(Generated, ExplicitPresence) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName("default_int32"); ASSERT_THAT(field, NotNull()); EXPECT_TRUE(field->has_presence()); } -TEST(Gemerated, RequiredPresence) { +TEST(Generated, RequiredPresence) { const FieldDescriptor* field = TestAllRequiredTypesProto2::descriptor()->FindFieldByName( "required_int32"); @@ -79,63 +81,63 @@ TEST(Gemerated, RequiredPresence) { EXPECT_EQ(field->label(), FieldDescriptor::LABEL_REQUIRED); } -TEST(Gemerated, ImplicitPresence) { +TEST(Generated, ImplicitPresence) { const FieldDescriptor* field = TestAllTypesProto3::descriptor()->FindFieldByName("optional_int32"); ASSERT_THAT(field, NotNull()); EXPECT_FALSE(field->has_presence()); } -TEST(Gemerated, ClosedEnum) { +TEST(Generated, ClosedEnum) { const EnumDescriptor* enm = TestAllTypesProto2::descriptor()->FindEnumTypeByName("NestedEnum"); ASSERT_THAT(enm, NotNull()); EXPECT_TRUE(enm->is_closed()); } -TEST(Gemerated, OpenEnum) { +TEST(Generated, OpenEnum) { const EnumDescriptor* enm = TestAllTypesProto3::descriptor()->FindEnumTypeByName("NestedEnum"); ASSERT_THAT(enm, NotNull()); EXPECT_FALSE(enm->is_closed()); } -TEST(Gemerated, PackedRepeated) { +TEST(Generated, PackedRepeated) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName("packed_int32"); ASSERT_THAT(field, NotNull()); EXPECT_TRUE(field->is_packed()); } -TEST(Gemerated, ExpandedRepeated) { +TEST(Generated, ExpandedRepeated) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName("repeated_int32"); ASSERT_THAT(field, NotNull()); EXPECT_FALSE(field->is_packed()); } -TEST(Gemerated, DoesNotEnforceUtf8) { +TEST(Generated, DoesNotEnforceUtf8) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName("optional_string"); ASSERT_THAT(field, NotNull()); EXPECT_FALSE(field->requires_utf8_validation()); } -TEST(Gemerated, EnforceUtf8) { +TEST(Generated, EnforceUtf8) { const FieldDescriptor* field = TestAllTypesProto3::descriptor()->FindFieldByName("optional_string"); ASSERT_THAT(field, NotNull()); EXPECT_TRUE(field->requires_utf8_validation()); } -TEST(Gemerated, DelimitedEncoding) { +TEST(Generated, DelimitedEncoding) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName("data"); ASSERT_THAT(field, NotNull()); EXPECT_EQ(field->type(), FieldDescriptor::TYPE_GROUP); } -TEST(Gemerated, LengthPrefixedEncoding) { +TEST(Generated, LengthPrefixedEncoding) { const FieldDescriptor* field = TestAllTypesProto2::descriptor()->FindFieldByName( "optional_nested_message"); @@ -143,6 +145,34 @@ TEST(Gemerated, LengthPrefixedEncoding) { EXPECT_EQ(field->type(), FieldDescriptor::TYPE_MESSAGE); } +TEST(Generated, EditionDefaults2023) { + const Descriptor* desc = EditionsDefaultMessage::descriptor(); + EXPECT_TRUE(desc->FindFieldByName("int32_field")->has_presence()); + EXPECT_TRUE( + desc->FindFieldByName("string_field")->requires_utf8_validation()); + EXPECT_FALSE(desc->FindFieldByName("enum_field") + ->legacy_enum_field_treated_as_closed()); + EXPECT_FALSE(desc->FindFieldByName("enum_field")->enum_type()->is_closed()); + EXPECT_TRUE(desc->FindFieldByName("repeated_int32_field")->is_packed()); + EXPECT_EQ(desc->FindFieldByName("sub_message_field")->type(), + FieldDescriptor::TYPE_MESSAGE); +} + +TEST(Generated, EditionDefaults2023InternalFeatures) { + EXPECT_THAT( + internal::InternalFeatureHelper::GetFeatures( + *EditionsDefaultMessage::descriptor()), + google::protobuf::EqualsProto(R"pb( + field_presence: EXPLICIT + enum_type: OPEN + repeated_field_encoding: PACKED + string_field_validation: MANDATORY + message_encoding: LENGTH_PREFIXED + json_format: ALLOW + [pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE } + )pb")); +} + } // namespace } // namespace protobuf } // namespace google diff --git a/src/google/protobuf/unittest_invalid_features.proto b/src/google/protobuf/editions/proto/editions_default_features.proto similarity index 77% rename from src/google/protobuf/unittest_invalid_features.proto rename to src/google/protobuf/editions/proto/editions_default_features.proto index d19c8c4388416..a5755f09ed742 100644 --- a/src/google/protobuf/unittest_invalid_features.proto +++ b/src/google/protobuf/editions/proto/editions_default_features.proto @@ -28,14 +28,26 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -syntax = "proto2"; +edition = "2023"; -package pb; +package protobuf_editions_test; -import "google/protobuf/descriptor.proto"; +// This file tests the default features in the absence of any dependencies. -message TestInvalid { - extend .google.protobuf.FeatureSet { - optional string scalar_extension = 9996; +enum EditionsDefaultEnum { + EDITIONS_DEFAULT_ENUM_UNKNOWN = 0; + EDITIONS_DEFAULT_ENUM_VALUE1 = 1; +} + +message EditionsDefaultMessage { + int32 int32_field = 1; + string string_field = 2; + EditionsDefaultEnum enum_field = 3; + + repeated int32 repeated_int32_field = 4; + + message SubMessage { + int32 nested_int32_field = 1; } + SubMessage sub_message_field = 6; } diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index feda73ed4447e..1387d7e9503e6 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -44,6 +44,8 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_split.h" +#include "absl/strings/string_view.h" +#include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" #include "google/protobuf/dynamic_message.h" @@ -118,6 +120,29 @@ absl::Status ValidateDescriptor(absl::string_view edition, return absl::OkStatus(); } +absl::Status ValidateExtension(const FieldDescriptor& extension) { + if (extension.message_type() == nullptr) { + return Error("FeatureSet extension ", extension.full_name(), + " is not of message type. Feature extensions should " + "always use messages to allow for evolution."); + } + + if (extension.is_repeated()) { + return Error( + "Only singular features extensions are supported. Found " + "repeated extension ", + extension.full_name()); + } + + if (extension.message_type()->extension_count() > 0 || + extension.message_type()->extension_range_count() > 0) { + return Error("Nested extensions in feature extension ", + extension.full_name(), " are not supported."); + } + + return absl::OkStatus(); +} + absl::Status FillDefaults(absl::string_view edition, Message& msg) { const Descriptor& descriptor = *msg.GetDescriptor(); @@ -189,10 +214,41 @@ absl::Status ValidateMergedFeatures(const Message& msg) { return absl::OkStatus(); } +// Forces calculation of the defaults for any features from the generated pool +// that may be missed if the proto file doesn't import them, giving the final +// resolved FeatureSet object maximal coverage. +absl::StatusOr FillGeneratedDefaults(absl::string_view edition, + const DescriptorPool* pool) { + const Descriptor* desc = + pool->FindMessageTypeByName(FeatureSet::descriptor()->full_name()); + // If there's no FeatureSet, there's no extensions. + if (desc == nullptr) return FeatureSet(); + + DynamicMessageFactory message_factory; + auto defaults = absl::WrapUnique(message_factory.GetPrototype(desc)->New()); + std::vector extensions; + pool->FindAllExtensions(desc, &extensions); + for (const auto* extension : extensions) { + RETURN_IF_ERROR(ValidateExtension(*extension)); + Message* msg = + defaults->GetReflection()->MutableMessage(defaults.get(), extension); + ABSL_CHECK(msg != nullptr); + RETURN_IF_ERROR(FillDefaults(edition, *msg)); + } + + // Our defaults are reflectively built in a custom pool, while the + // returned object here is in the generated pool. We parse/serialize to + // convert from the potentially skewed FeatureSet descriptors. + FeatureSet features; + ABSL_CHECK(features.MergeFromString(defaults->SerializeAsString())); + return features; +} + } // namespace absl::StatusOr FeatureResolver::Create( - absl::string_view edition, const Descriptor* descriptor) { + absl::string_view edition, const Descriptor* descriptor, + const DescriptorPool* generated_pool) { if (descriptor == nullptr) { return Error( "Unable to find definition of google.protobuf.FeatureSet in descriptor pool."); @@ -206,8 +262,22 @@ absl::StatusOr FeatureResolver::Create( RETURN_IF_ERROR(FillDefaults(edition, *defaults)); + FeatureSet generated_defaults; + if (descriptor->file()->pool() == generated_pool) { + // If we're building the generated pool, the best we can do is load the C++ + // language features, which should always be present for code using the C++ + // runtime. + RETURN_IF_ERROR( + FillDefaults(edition, *generated_defaults.MutableExtension(pb::cpp))); + } else { + absl::StatusOr defaults = + FillGeneratedDefaults(edition, generated_pool); + RETURN_IF_ERROR(defaults.status()); + generated_defaults = std::move(defaults).value(); + } + return FeatureResolver(edition, *descriptor, std::move(message_factory), - std::move(defaults)); + std::move(defaults), std::move(generated_defaults)); } absl::Status FeatureResolver::RegisterExtension( @@ -221,24 +291,7 @@ absl::Status FeatureResolver::RegisterExtension( ABSL_CHECK(descriptor_.IsExtensionNumber(extension.number())); - if (extension.message_type() == nullptr) { - return Error("FeatureSet extension ", extension.full_name(), - " is not of message type. Feature extensions should " - "always use messages to allow for evolution."); - } - - if (extension.is_repeated()) { - return Error( - "Only singular features extensions are supported. Found " - "repeated extension ", - extension.full_name()); - } - - if (extension.message_type()->extension_count() > 0 || - extension.message_type()->extension_range_count() > 0) { - return Error("Nested extensions in feature extension ", - extension.full_name(), " are not supported."); - } + RETURN_IF_ERROR(ValidateExtension(extension)); RETURN_IF_ERROR(ValidateDescriptor(edition_, *extension.message_type())); @@ -273,8 +326,12 @@ absl::Status FeatureResolver::RegisterExtensions(const Descriptor& message) { absl::StatusOr FeatureResolver::MergeFeatures( const FeatureSet& merged_parent, const FeatureSet& unmerged_child) const { - FeatureSet merged; - ABSL_CHECK(merged.ParseFromString(defaults_->SerializeAsString())); + FeatureSet merged = generated_defaults_; + + // Our defaults are a dynamic message located in the build pool, while the + // returned object here is in the generated pool. We parse/serialize to + // convert from the potentially skewed FeatureSet descriptors. + ABSL_CHECK(merged.MergeFromString(defaults_->SerializeAsString())); merged.MergeFrom(merged_parent); merged.MergeFrom(unmerged_child); diff --git a/src/google/protobuf/feature_resolver.h b/src/google/protobuf/feature_resolver.h index 013dc65e8508f..0d27ee1acf0c9 100644 --- a/src/google/protobuf/feature_resolver.h +++ b/src/google/protobuf/feature_resolver.h @@ -37,6 +37,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" @@ -58,8 +59,10 @@ class PROTOBUF_EXPORT FeatureResolver { // Creates a new FeatureResolver at a specific edition. This validates the // built-in features for the given edition, and calculates the default feature // set. - static absl::StatusOr Create(absl::string_view edition, - const Descriptor* descriptor); + static absl::StatusOr Create( + absl::string_view edition, const Descriptor* descriptor, + const DescriptorPool* generated_pool = + DescriptorPool::internal_generated_pool()); // Registers a potential extension of the FeatureSet proto. Any visible // extensions will be used during merging. Returns an error if the extension @@ -69,17 +72,22 @@ class PROTOBUF_EXPORT FeatureResolver { // Creates a new feature set using inheritance and default behavior. This is // designed to be called recursively, and the parent feature set is expected // to be a fully merged one. + // The returned FeatureSet will be fully resolved for any extensions that were + // explicitly registered (in the custom pool) or linked into this binary (in + // the generated pool). absl::StatusOr MergeFeatures( const FeatureSet& merged_parent, const FeatureSet& unmerged_child) const; private: FeatureResolver(absl::string_view edition, const Descriptor& descriptor, std::unique_ptr message_factory, - std::unique_ptr defaults) + std::unique_ptr defaults, + FeatureSet generated_defaults) : edition_(edition), descriptor_(descriptor), message_factory_(std::move(message_factory)), - defaults_(std::move(defaults)) {} + defaults_(std::move(defaults)), + generated_defaults_(std::move(generated_defaults)) {} absl::Status RegisterExtensions(const Descriptor& message); absl::Status RegisterExtension(const FieldDescriptor& extension); @@ -89,6 +97,7 @@ class PROTOBUF_EXPORT FeatureResolver { absl::flat_hash_set extensions_; std::unique_ptr message_factory_; std::unique_ptr defaults_; + FeatureSet generated_defaults_; }; } // namespace protobuf diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index 8d1d7ce47beee..5c5ef41c8ffab 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -37,7 +37,9 @@ #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "google/protobuf/compiler/parser.h" +#include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" #include "google/protobuf/io/tokenizer.h" @@ -87,10 +89,17 @@ const FileDescriptor& GetExtensionFile( ->file(); } +const DescriptorPool* EmptyPool() { + static DescriptorPool* empty_pool = new DescriptorPool(); + return empty_pool; +} + template -absl::StatusOr SetupFeatureResolver(absl::string_view edition, - Extensions... extensions) { - auto resolver = FeatureResolver::Create(edition, FeatureSet::descriptor()); +absl::StatusOr SetupFeatureResolverImpl( + absl::string_view edition, const DescriptorPool* pool, + Extensions... extensions) { + auto resolver = + FeatureResolver::Create(edition, FeatureSet::descriptor(), pool); RETURN_IF_ERROR(resolver.status()); std::vector results{ resolver->RegisterExtensions(GetExtensionFile(extensions))...}; @@ -99,17 +108,35 @@ absl::StatusOr SetupFeatureResolver(absl::string_view edition, } return resolver; } +template +absl::StatusOr SetupFeatureResolver(absl::string_view edition, + Extensions... extensions) { + return SetupFeatureResolverImpl(edition, EmptyPool(), extensions...); +} template -absl::StatusOr GetDefaults(absl::string_view edition, - Extensions... extensions) { +absl::StatusOr GetDefaultsImpl(absl::string_view edition, + const DescriptorPool* pool, + Extensions... extensions) { absl::StatusOr resolver = - SetupFeatureResolver(edition, extensions...); + SetupFeatureResolverImpl(edition, pool, extensions...); RETURN_IF_ERROR(resolver.status()); FeatureSet parent, child; return resolver->MergeFeatures(parent, child); } +template +absl::StatusOr GetDefaults(absl::string_view edition, + Extensions... extensions) { + return GetDefaultsImpl(edition, EmptyPool(), extensions...); +} + +FileDescriptorProto GetProto(const FileDescriptor* file) { + FileDescriptorProto proto; + file->CopyTo(&proto); + return proto; +} + TEST(FeatureResolverTest, DefaultsCore2023) { absl::StatusOr merged = GetDefaults("2023"); ASSERT_OK(merged); @@ -119,6 +146,7 @@ TEST(FeatureResolverTest, DefaultsCore2023) { EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); EXPECT_EQ(merged->string_field_validation(), FeatureSet::MANDATORY); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); + EXPECT_FALSE(merged->HasExtension(pb::test)); } TEST(FeatureResolverTest, DefaultsTest2023) { @@ -207,6 +235,34 @@ TEST(FeatureResolverTest, DefaultsTestNestedExtension) { EXPECT_EQ(ext.enum_field_feature(), pb::TestFeatures::ENUM_VALUE1); } +TEST(FeatureResolverTest, DefaultsGeneratedPoolCustom) { + DescriptorPool pool; + ASSERT_NE( + pool.BuildFile(GetProto(google::protobuf::DescriptorProto::descriptor()->file())), + nullptr); + ASSERT_NE(pool.BuildFile(GetProto(pb::TestFeatures::descriptor()->file())), + nullptr); + absl::StatusOr merged = GetDefaultsImpl("2023", &pool); + ASSERT_OK(merged); + + EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); + EXPECT_TRUE(merged->HasExtension(pb::test)); + EXPECT_EQ(merged->GetExtension(pb::test).int_file_feature(), 1); + EXPECT_FALSE(merged->HasExtension(pb::cpp)); +} + +TEST(FeatureResolverTest, DefaultsGeneratedPoolGenerated) { + absl::StatusOr merged = + GetDefaultsImpl("2023", DescriptorPool::generated_pool()); + ASSERT_OK(merged); + + EXPECT_EQ(merged->field_presence(), FeatureSet::EXPLICIT); + EXPECT_FALSE(merged->HasExtension(pb::test)); + EXPECT_TRUE(merged->HasExtension(pb::cpp)); + EXPECT_EQ(merged->GetExtension(pb::cpp).utf8_validation(), + pb::CppFeatures::VERIFY_PARSE); +} + TEST(FeatureResolverTest, DefaultsTooEarly) { absl::StatusOr merged = GetDefaults("2022", pb::test); EXPECT_THAT(merged, HasError(AllOf(HasSubstr("No valid default found"), @@ -278,12 +334,13 @@ TEST(FeatureResolverTest, DefaultsMessageMerge) { TEST(FeatureResolverTest, InitializePoolMissingDescriptor) { DescriptorPool pool; - EXPECT_THAT(FeatureResolver::Create("2023", nullptr), + EXPECT_THAT(FeatureResolver::Create("2023", nullptr, EmptyPool()), HasError(HasSubstr("find definition of google.protobuf.FeatureSet"))); } TEST(FeatureResolverTest, RegisterExtensionDifferentContainingType) { - auto resolver = FeatureResolver::Create("2023", FeatureSet::descriptor()); + auto resolver = + FeatureResolver::Create("2023", FeatureSet::descriptor(), EmptyPool()); ASSERT_OK(resolver); EXPECT_OK(resolver->RegisterExtensions( @@ -291,7 +348,8 @@ TEST(FeatureResolverTest, RegisterExtensionDifferentContainingType) { } TEST(FeatureResolverTest, RegisterExtensionTwice) { - auto resolver = FeatureResolver::Create("2023", FeatureSet::descriptor()); + auto resolver = + FeatureResolver::Create("2023", FeatureSet::descriptor(), EmptyPool()); ASSERT_OK(resolver); EXPECT_OK(resolver->RegisterExtensions(GetExtensionFile(pb::test))); @@ -530,7 +588,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionNonMessage) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT(resolver->RegisterExtensions(*file), @@ -552,7 +610,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionRepeated) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT( @@ -582,7 +640,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithExtensions) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT( @@ -615,7 +673,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithOneof) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT(resolver->RegisterExtensions(*file), @@ -642,7 +700,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRequired) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT(resolver->RegisterExtensions(*file), @@ -669,7 +727,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRepeated) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT(resolver->RegisterExtensions(*file), @@ -695,7 +753,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithMissingTarget) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT(resolver->RegisterExtensions(*file), @@ -725,7 +783,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsMessageParsingError) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT( resolver->RegisterExtensions(*file), @@ -757,7 +815,7 @@ TEST_F(FeatureResolverPoolTest, ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT( resolver->RegisterExtensions(*file), @@ -789,7 +847,7 @@ TEST_F(FeatureResolverPoolTest, ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_OK(resolver->RegisterExtensions(*file)); FeatureSet parent, child; @@ -815,7 +873,7 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsScalarParsingError) { ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_THAT( resolver->RegisterExtensions(*file), @@ -843,13 +901,42 @@ TEST_F(FeatureResolverPoolTest, ASSERT_NE(file, nullptr); auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet")); + "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); ASSERT_OK(resolver); EXPECT_OK(resolver->RegisterExtensions(*file)); FeatureSet parent, child; EXPECT_OK(resolver->MergeFeatures(parent, child)); } +TEST_F(FeatureResolverPoolTest, GeneratedPoolInvalidExtension) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + message Foo {} + extend google.protobuf.FeatureSet { + optional string bar = 9999; + } + )schema"); + ASSERT_NE(file, nullptr); + + EXPECT_THAT(GetDefaultsImpl("2023", &pool_), + HasError(AllOf(HasSubstr("test.bar"), + HasSubstr("is not of message type")))); +} + +TEST_F(FeatureResolverPoolTest, GeneratedPoolDefaultsFailure) { + ASSERT_NE( + pool_.BuildFile(GetProto(google::protobuf::DescriptorProto::descriptor()->file())), + nullptr); + ASSERT_NE(pool_.BuildFile(GetProto(pb::TestFeatures::descriptor()->file())), + nullptr); + EXPECT_THAT( + GetDefaultsImpl("2022", &pool_), + HasError(AllOf(HasSubstr("No valid default found"), HasSubstr("2022")))); +} + } // namespace } // namespace protobuf } // namespace google