diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index a89ef2e8ad..4c2db3f531 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/src/google/protobuf/BUILD.bazel @@ -1109,6 +1109,9 @@ cc_test( "//src/google/protobuf/compiler:importer", "//src/google/protobuf/stubs", "//src/google/protobuf/testing", + "@com_google_absl//absl/log:absl_check", + "@com_google_absl//absl/log:absl_log", + "@com_google_absl//absl/log:die_if_null", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/src/google/protobuf/compiler/code_generator_unittest.cc b/src/google/protobuf/compiler/code_generator_unittest.cc index e0277f6e17..eea9d48407 100644 --- a/src/google/protobuf/compiler/code_generator_unittest.cc +++ b/src/google/protobuf/compiler/code_generator_unittest.cc @@ -177,7 +177,8 @@ TEST_F(CodeGeneratorTest, GetResolvedSourceFeaturesRoot) { EXPECT_EQ(features.field_presence(), FeatureSet::EXPLICIT); EXPECT_EQ(features.enum_type(), FeatureSet::CLOSED); - EXPECT_TRUE(ext.has_int_message_feature()); + // TODO(b/296638633) Flip this once generators can specify their feature sets. + EXPECT_FALSE(ext.has_int_message_feature()); EXPECT_EQ(ext.int_file_feature(), 8); EXPECT_EQ(ext.string_source_feature(), "file"); } diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 1d6d288297..c13391636e 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1353,7 +1353,7 @@ TEST_F(CommandLineInterfaceTest, AllowServicesHasService) { TEST_F(CommandLineInterfaceTest, EditionsAreNotAllowed) { CreateTempFile("foo.proto", - "edition = \"very-cool\";\n" + "edition = \"2023\";\n" "message FooRequest {}\n"); Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto"); @@ -1365,7 +1365,7 @@ TEST_F(CommandLineInterfaceTest, EditionsAreNotAllowed) { TEST_F(CommandLineInterfaceTest, EditionsFlagExplicitTrue) { CreateTempFile("foo.proto", - "edition = \"very-cool\";\n" + "edition = \"2023\";\n" "message FooRequest {}\n"); Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir " @@ -1477,14 +1477,16 @@ TEST_F(CommandLineInterfaceTest, FeatureExtensionError) { import "features.proto"; message Foo { int32 bar = 1; - int32 baz = 2 [features.(pb.test).int_feature = 5]; + int32 baz = 2 [features.(pb.test).repeated_feature = 5]; })schema"); Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir " "--experimental_editions foo.proto"); - ExpectErrorSubstring( - "Feature field pb.TestFeatures.repeated_feature is an unsupported " - "repeated field"); + // TODO(b/296638633) Flip this once generators can specify their feature sets. + ExpectNoErrors(); + // ExpectErrorSubstring( + // "Feature field pb.TestFeatures.repeated_feature is an unsupported " + // "repeated field"); } TEST_F(CommandLineInterfaceTest, Plugin_LegacyFeatures) { diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 54e6b7fd99..100d35eb32 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -44,6 +44,7 @@ #include #include #include +#include #include #include "google/protobuf/stubs/common.h" @@ -57,6 +58,7 @@ #include "absl/hash/hash.h" #include "absl/log/absl_check.h" #include "absl/log/absl_log.h" +#include "absl/memory/memory.h" #include "absl/status/statusor.h" #include "absl/strings/ascii.h" #include "absl/strings/escaping.h" @@ -1111,6 +1113,18 @@ bool AllowedExtendeeInProto3(const std::string& name) { allowed_proto3_extendees->end(); } +const FeatureSetDefaults& GetCppFeatureSetDefaults() { + static const FeatureSetDefaults* default_spec = [] { + auto default_spec = FeatureResolver::CompileDefaults( + FeatureSet::descriptor(), + // TODO(b/297261063) Move this range to a central location. + {pb::CppFeatures::descriptor()->file()->extension(0)}, "2023", "2023"); + ABSL_CHECK(default_spec.ok()) << default_spec.status(); + return new FeatureSetDefaults(std::move(default_spec).value()); + }(); + return *default_spec; +} + // Create global defaults for proto2/proto3 compatibility. FeatureSet* CreateProto2DefaultFeatures() { FeatureSet* features = new FeatureSet(); @@ -4577,14 +4591,7 @@ class DescriptorBuilder { const FileDescriptor* DescriptorPool::BuildFile( const FileDescriptorProto& proto) { - ABSL_CHECK(fallback_database_ == nullptr) - << "Cannot call BuildFile on a DescriptorPool that uses a " - "DescriptorDatabase. You must instead find a way to get your file " - "into the underlying database."; - ABSL_CHECK(mutex_ == nullptr); // Implied by the above ABSL_CHECK. - tables_->known_bad_symbols_.clear(); - tables_->known_bad_files_.clear(); - return DescriptorBuilder::New(this, tables_.get(), nullptr)->BuildFile(proto); + return BuildFileCollectingErrors(proto, nullptr); } const FileDescriptor* DescriptorPool::BuildFileCollectingErrors( @@ -4596,6 +4603,7 @@ const FileDescriptor* DescriptorPool::BuildFileCollectingErrors( ABSL_CHECK(mutex_ == nullptr); // Implied by the above ABSL_CHECK. tables_->known_bad_symbols_.clear(); tables_->known_bad_files_.clear(); + build_started_ = true; return DescriptorBuilder::New(this, tables_.get(), error_collector) ->BuildFile(proto); } @@ -4603,6 +4611,7 @@ const FileDescriptor* DescriptorPool::BuildFileCollectingErrors( const FileDescriptor* DescriptorPool::BuildFileFromDatabase( const FileDescriptorProto& proto) const { mutex_->AssertHeld(); + build_started_ = true; if (tables_->known_bad_files_.contains(proto.name())) { return nullptr; } @@ -4615,6 +4624,14 @@ const FileDescriptor* DescriptorPool::BuildFileFromDatabase( return result; } +void DescriptorPool::SetFeatureSetDefaults(FeatureSetDefaults spec) { + ABSL_CHECK(!build_started_) + << "Feature set defaults can't be changed once the pool has started " + "building."; + feature_set_defaults_spec_ = + absl::make_unique(std::move(spec)); +} + DescriptorBuilder::DescriptorBuilder( const DescriptorPool* pool, DescriptorPool::Tables* tables, DescriptorPool::ErrorCollector* error_collector) @@ -5699,8 +5716,13 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( } ABSL_CHECK(descriptor); + const FeatureSetDefaults& defaults = + pool_->feature_set_defaults_spec_ == nullptr + ? GetCppFeatureSetDefaults() + : *pool_->feature_set_defaults_spec_; + absl::StatusOr feature_resolver = - FeatureResolver::Create(proto.edition(), descriptor); + FeatureResolver::Create(proto.edition(), defaults); if (!feature_resolver.ok()) { AddError( proto.name(), proto, DescriptorPool::ErrorCollector::EDITIONS, @@ -5816,16 +5838,6 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( return nullptr; } - // Look for feature extensions in regular imports. - if (feature_resolver_.has_value() && dependency != nullptr) { - absl::Status status = feature_resolver_->RegisterExtensions(*dependency); - if (!status.ok()) { - AddError(dependency->name(), proto, - DescriptorPool::ErrorCollector::EDITIONS, - [&] { return std::string(status.message()); }); - } - } - if (dependency == nullptr) { if (!pool_->lazily_build_dependencies_) { if (pool_->allow_unknown_ || diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h index 15e0057298..2c6a779852 100644 --- a/src/google/protobuf/descriptor.h +++ b/src/google/protobuf/descriptor.h @@ -122,6 +122,7 @@ class MethodOptions; class FileOptions; class UninterpretedOption; class FeatureSet; +class FeatureSetDefaults; class SourceCodeInfo; // Defined in message_lite.h @@ -2289,6 +2290,13 @@ class PROTOBUF_EXPORT DescriptorPool { // DescriptorPool will report a import not found error. void EnforceWeakDependencies(bool enforce) { enforce_weak_ = enforce; } + // Sets the default feature mappings used during the build. If this function + // isn't called, the C++ feature set defaults are used. If this function is + // called, these defaults will be used instead. + // FeatureSetDefaults includes a minimum/maximum supported edition, which will + // be enforced while building proto files. + void SetFeatureSetDefaults(FeatureSetDefaults spec); + // Toggles enforcement of extension declarations. // This enforcement is disabled by default because it requires full // descriptors with source-retention options, which are generally not @@ -2469,11 +2477,16 @@ class PROTOBUF_EXPORT DescriptorPool { bool enforce_extension_declarations_; bool disallow_enforce_utf8_; bool deprecated_legacy_json_field_conflicts_; + mutable bool build_started_ = false; // Set of files to track for unused imports. The bool value when true means // unused imports are treated as errors (and as warnings when false). absl::flat_hash_map unused_import_track_files_; + // Specification of defaults to use for feature resolution. This defaults to + // just the global and C++ features, but can be overridden for other runtimes. + std::unique_ptr feature_set_defaults_spec_; + // Returns true if the field extends an option message of descriptor.proto. bool IsExtendingDescriptor(const FieldDescriptor& field) const; diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 10a69cbb51..43ef3fa24c 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -41,6 +41,7 @@ #include #include #include +#include #include #include "google/protobuf/stubs/common.h" @@ -67,6 +68,7 @@ #include "google/protobuf/descriptor_database.h" #include "google/protobuf/descriptor_legacy.h" #include "google/protobuf/dynamic_message.h" +#include "google/protobuf/feature_resolver.h" #include "google/protobuf/io/tokenizer.h" #include "google/protobuf/io/zero_copy_stream_impl_lite.h" #include "google/protobuf/test_textproto.h" @@ -514,17 +516,17 @@ TEST_F(FileDescriptorTest, Syntax) { } { proto.set_syntax("editions"); - proto.set_edition("very-cool"); + proto.set_edition("2023"); DescriptorPool pool; const FileDescriptor* file = pool.BuildFile(proto); ASSERT_TRUE(file != nullptr); EXPECT_EQ(FileDescriptorLegacy::Syntax::SYNTAX_EDITIONS, FileDescriptorLegacy(file).syntax()); - EXPECT_EQ("very-cool", file->edition()); + EXPECT_EQ("2023", file->edition()); FileDescriptorProto other; file->CopyTo(&other); EXPECT_EQ("editions", other.syntax()); - EXPECT_EQ("very-cool", other.edition()); + EXPECT_EQ("2023", other.edition()); } } @@ -552,7 +554,7 @@ TEST_F(FileDescriptorTest, CopyHeadingTo) { EXPECT_EQ(&other.options().features(), &FeatureSet::default_instance()); { proto.set_syntax("editions"); - proto.set_edition("very-cool"); + proto.set_edition("2023"); DescriptorPool pool; const FileDescriptor* file = pool.BuildFile(proto); @@ -563,7 +565,7 @@ TEST_F(FileDescriptorTest, CopyHeadingTo) { EXPECT_EQ(other.name(), "foo.proto"); EXPECT_EQ(other.package(), "foo.bar.baz"); EXPECT_EQ(other.syntax(), "editions"); - EXPECT_EQ(other.edition(), "very-cool"); + EXPECT_EQ(other.edition(), "2023"); EXPECT_EQ(other.options().java_package(), "foo.bar.baz"); EXPECT_TRUE(other.message_type().empty()); EXPECT_EQ(&other.options().features(), &FeatureSet::default_instance()); @@ -7235,7 +7237,24 @@ TEST_F(ValidationErrorTest, UnusedImportWithOtherError) { "foo.proto: Foo.foo: EXTENDEE: \"Baz\" is not defined.\n"); } -using FeaturesTest = ValidationErrorTest; +using FeaturesBaseTest = ValidationErrorTest; + +class FeaturesTest : public FeaturesBaseTest { + protected: + void SetUp() override { + ValidationErrorTest::SetUp(); + + auto default_spec = FeatureResolver::CompileDefaults( + FeatureSet::descriptor(), + {pb::CppFeatures::descriptor()->file()->extension(0), + pb::TestFeatures::descriptor()->file()->extension(0), + pb::TestMessage::descriptor()->extension(0), + pb::TestMessage::Nested::descriptor()->extension(0)}, + "2023", "2025"); + ASSERT_OK(default_spec); + pool_.SetFeatureSetDefaults(std::move(default_spec).value()); + } +}; template const FeatureSet& GetFeatures(const T* descriptor) { @@ -7544,12 +7563,36 @@ TEST_F(FeaturesTest, Edition2023Defaults) { [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. + // Since pb::test is registered in the pool, 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(FeaturesBaseTest, DefaultEdition2023Defaults) { + BuildDescriptorMessagesInTestPool(); + BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); + const FileDescriptor* file = BuildFile(R"pb( + name: "foo.proto" + syntax: "editions" + edition: "2023" + dependency: "google/protobuf/unittest_features.proto" + )pb"); + ASSERT_NE(file, nullptr); + + EXPECT_THAT(file->options(), EqualsProto("")); + EXPECT_THAT( + GetFeatures(file), EqualsProto(R"pb( + field_presence: EXPLICIT + enum_type: OPEN + repeated_field_encoding: PACKED + message_encoding: LENGTH_PREFIXED + json_format: ALLOW + [pb.cpp] { legacy_closed_enum: false utf8_validation: VERIFY_PARSE } + )pb")); + EXPECT_FALSE(GetFeatures(file).HasExtension(pb::test)); +} + TEST_F(FeaturesTest, ClearsOptions) { BuildDescriptorMessagesInTestPool(); const FileDescriptor* file = BuildFile(R"pb( @@ -7842,9 +7885,8 @@ TEST_F(FeaturesTest, InvalidEdition) { R"pb( name: "foo.proto" syntax: "editions" edition: "2022" )pb", - "foo.proto: foo.proto: EDITIONS: No valid default found for edition 2022 " - "in " - "feature field google.protobuf.FeatureSet.field_presence\n"); + "foo.proto: foo.proto: EDITIONS: Edition 2022 is earlier than the " + "minimum supported edition 2023\n"); } TEST_F(FeaturesTest, FileFeatures) { @@ -9110,48 +9152,6 @@ TEST_F(FeaturesTest, FeaturesOutsideEditions) { "editions.\n"); } -TEST_F(FeaturesTest, InvalidExtensionNonMessage) { - BuildDescriptorMessagesInTestPool(); - 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: "unittest_invalid_features.proto" - options { - 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: 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) { BuildDescriptorMessagesInTestPool(); BuildFileWithErrors( diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index 46b3bebe62..aa4858b0d5 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -31,6 +31,7 @@ #include "google/protobuf/feature_resolver.h" #include +#include #include #include #include @@ -38,13 +39,17 @@ #include #include "absl/algorithm/container.h" +#include "absl/container/btree_set.h" +#include "absl/container/flat_hash_set.h" #include "absl/log/absl_check.h" #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" @@ -71,24 +76,25 @@ absl::Status Error(Args... args) { return absl::FailedPreconditionError(absl::StrCat(args...)); } -bool EditionsLessThan(absl::string_view a, absl::string_view b) { - std::vector as = absl::StrSplit(a, '.'); - std::vector bs = absl::StrSplit(b, '.'); - size_t min_size = std::min(as.size(), bs.size()); - for (size_t i = 0; i < min_size; ++i) { - if (as[i].size() != bs[i].size()) { - return as[i].size() < bs[i].size(); - } else if (as[i] != bs[i]) { - return as[i] < bs[i]; +struct EditionsLessThan { + bool operator()(absl::string_view a, absl::string_view b) const { + std::vector as = absl::StrSplit(a, '.'); + std::vector bs = absl::StrSplit(b, '.'); + size_t min_size = std::min(as.size(), bs.size()); + for (size_t i = 0; i < min_size; ++i) { + if (as[i].size() != bs[i].size()) { + return as[i].size() < bs[i].size(); + } else if (as[i] != bs[i]) { + return as[i] < bs[i]; + } } + // Both strings are equal up until an extra element, which makes that string + // more recent. + return as.size() < bs.size(); } - // Both strings are equal up until an extra element, which makes that string - // more recent. - return as.size() < bs.size(); -} +}; -absl::Status ValidateDescriptor(absl::string_view edition, - const Descriptor& descriptor) { +absl::Status ValidateDescriptor(const Descriptor& descriptor) { if (descriptor.oneof_decl_count() > 0) { return Error("Type ", descriptor.full_name(), " contains unsupported oneof feature fields."); @@ -113,35 +119,57 @@ 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(), +absl::Status ValidateExtension(const Descriptor& feature_set, + const FieldDescriptor* extension) { + if (extension == nullptr) { + return Error("Unknown extension of ", feature_set.full_name(), "."); + } + + if (extension->containing_type() != &feature_set) { + return Error("Extension ", extension->full_name(), + " is not an extension of ", feature_set.full_name(), "."); + } + + 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()) { + if (extension->is_repeated()) { return Error( "Only singular features extensions are supported. Found " "repeated extension ", - extension.full_name()); + extension->full_name()); } - if (extension.message_type()->extension_count() > 0 || - extension.message_type()->extension_range_count() > 0) { + 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."); + extension->full_name(), " are not supported."); } return absl::OkStatus(); } +void CollectEditions(const Descriptor& descriptor, + absl::string_view minimum_edition, + absl::string_view maximum_edition, + absl::btree_set& editions) { + for (int i = 0; i < descriptor.field_count(); ++i) { + for (const auto& def : descriptor.field(i)->options().edition_defaults()) { + if (EditionsLessThan()(maximum_edition, def.edition())) continue; + editions.insert(def.edition()); + } + } +} + absl::Status FillDefaults(absl::string_view edition, Message& msg) { const Descriptor& descriptor = *msg.GetDescriptor(); auto comparator = [](const FieldOptions::EditionDefault& a, const FieldOptions::EditionDefault& b) { - return EditionsLessThan(a.edition(), b.edition()); + return EditionsLessThan()(a.edition(), b.edition()); }; FieldOptions::EditionDefault edition_lookup; edition_lookup.set_edition(edition); @@ -206,124 +234,100 @@ 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, - const DescriptorPool* generated_pool) { - if (descriptor == nullptr) { +absl::StatusOr FeatureResolver::CompileDefaults( + const Descriptor* feature_set, + absl::Span extensions, + absl::string_view minimum_edition, absl::string_view maximum_edition) { + // Find and validate the FeatureSet in the pool. + if (feature_set == nullptr) { return Error( "Unable to find definition of google.protobuf.FeatureSet in descriptor pool."); } + RETURN_IF_ERROR(ValidateDescriptor(*feature_set)); - RETURN_IF_ERROR(ValidateDescriptor(edition, *descriptor)); - - auto message_factory = absl::make_unique(); - auto defaults = - absl::WrapUnique(message_factory->GetPrototype(descriptor)->New()); - - 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(); + // Collect and validate all the FeatureSet extensions. + for (const auto* extension : extensions) { + RETURN_IF_ERROR(ValidateExtension(*feature_set, extension)); + RETURN_IF_ERROR(ValidateDescriptor(*extension->message_type())); } - return FeatureResolver(edition, *descriptor, std::move(message_factory), - std::move(defaults), std::move(generated_defaults)); -} - -absl::Status FeatureResolver::RegisterExtension( - const FieldDescriptor& extension) { - if (!extension.is_extension() || - extension.containing_type() != &descriptor_ || - extensions_.contains(&extension)) { - // These are valid but irrelevant extensions, just return ok. - return absl::OkStatus(); + // Collect all the editions with unique defaults. + absl::btree_set editions; + CollectEditions(*feature_set, minimum_edition, maximum_edition, editions); + for (const auto* extension : extensions) { + CollectEditions(*extension->message_type(), minimum_edition, + maximum_edition, editions); } - ABSL_CHECK(descriptor_.IsExtensionNumber(extension.number())); - - RETURN_IF_ERROR(ValidateExtension(extension)); - - RETURN_IF_ERROR(ValidateDescriptor(edition_, *extension.message_type())); - - Message* msg = - defaults_->GetReflection()->MutableMessage(defaults_.get(), &extension); - ABSL_CHECK(msg != nullptr); - RETURN_IF_ERROR(FillDefaults(edition_, *msg)); - - extensions_.insert(&extension); - return absl::OkStatus(); + // Fill the default spec. + FeatureSetDefaults defaults; + defaults.set_minimum_edition(minimum_edition); + defaults.set_maximum_edition(maximum_edition); + auto message_factory = absl::make_unique(); + for (const auto& edition : editions) { + auto defaults_dynamic = + absl::WrapUnique(message_factory->GetPrototype(feature_set)->New()); + RETURN_IF_ERROR(FillDefaults(edition, *defaults_dynamic)); + for (const auto* extension : extensions) { + RETURN_IF_ERROR(FillDefaults( + edition, *defaults_dynamic->GetReflection()->MutableMessage( + defaults_dynamic.get(), extension))); + } + auto* edition_defaults = defaults.mutable_defaults()->Add(); + edition_defaults->set_edition(edition); + edition_defaults->mutable_features()->MergeFromString( + defaults_dynamic->SerializeAsString()); + } + return defaults; } -absl::Status FeatureResolver::RegisterExtensions(const FileDescriptor& file) { - for (int i = 0; i < file.extension_count(); ++i) { - RETURN_IF_ERROR(RegisterExtension(*file.extension(i))); +absl::StatusOr FeatureResolver::Create( + absl::string_view edition, const FeatureSetDefaults& compiled_defaults) { + if (EditionsLessThan()(edition, compiled_defaults.minimum_edition())) { + return Error("Edition ", edition, + " is earlier than the minimum supported edition ", + compiled_defaults.minimum_edition()); } - for (int i = 0; i < file.message_type_count(); ++i) { - RETURN_IF_ERROR(RegisterExtensions(*file.message_type(i))); + if (EditionsLessThan()(compiled_defaults.maximum_edition(), edition)) { + return Error("Edition ", edition, + " is later than the maximum supported edition ", + compiled_defaults.maximum_edition()); } - return absl::OkStatus(); -} -absl::Status FeatureResolver::RegisterExtensions(const Descriptor& message) { - for (int i = 0; i < message.extension_count(); ++i) { - RETURN_IF_ERROR(RegisterExtension(*message.extension(i))); + // Validate compiled defaults. + absl::string_view prev_edition; + for (const auto& edition_default : compiled_defaults.defaults()) { + if (!prev_edition.empty()) { + if (!EditionsLessThan()(prev_edition, edition_default.edition())) { + return Error( + "Feature set defaults are not strictly increasing. Edition ", + prev_edition, " is greater than or equal to edition ", + edition_default.edition(), "."); + } + } + prev_edition = edition_default.edition(); } - for (int i = 0; i < message.nested_type_count(); ++i) { - RETURN_IF_ERROR(RegisterExtensions(*message.nested_type(i))); + + // Select the matching edition defaults. + auto comparator = [](const auto& a, const auto& b) { + return EditionsLessThan()(a.edition(), b.edition()); + }; + FeatureSetDefaults::FeatureSetEditionDefault search; + search.set_edition(edition); + auto first_nonmatch = + absl::c_upper_bound(compiled_defaults.defaults(), search, comparator); + if (first_nonmatch == compiled_defaults.defaults().begin()) { + return Error("No valid default found for edition ", edition); } - return absl::OkStatus(); + + return FeatureResolver(std::prev(first_nonmatch)->features()); } absl::StatusOr FeatureResolver::MergeFeatures( const FeatureSet& merged_parent, const FeatureSet& unmerged_child) const { - 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())); + FeatureSet merged = defaults_; 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 0d27ee1acf..6b4ca27ea2 100644 --- a/src/google/protobuf/feature_resolver.h +++ b/src/google/protobuf/feature_resolver.h @@ -39,6 +39,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/types/span.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" #include "google/protobuf/dynamic_message.h" @@ -56,48 +57,32 @@ class PROTOBUF_EXPORT FeatureResolver { FeatureResolver(FeatureResolver&&) = default; FeatureResolver& operator=(FeatureResolver&&) = delete; - // 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, - const DescriptorPool* generated_pool = - DescriptorPool::internal_generated_pool()); + // Compiles a set of FeatureSet extensions into a mapping of edition to unique + // defaults. This is the most complicated part of feature resolution, and by + // abstracting this out into an intermediate message, we can make feature + // resolution significantly more portable. + static absl::StatusOr CompileDefaults( + const Descriptor* feature_set, + absl::Span extensions, + absl::string_view minimum_edition, absl::string_view maximum_edition); - // Registers a potential extension of the FeatureSet proto. Any visible - // extensions will be used during merging. Returns an error if the extension - // is a feature extension but fails validation. - absl::Status RegisterExtensions(const FileDescriptor& file); + // Creates a new FeatureResolver at a specific edition. This calculates the + // default feature set for that edition, using the output of CompileDefaults. + static absl::StatusOr Create( + absl::string_view edition, const FeatureSetDefaults& defaults); // 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). + // to be a fully merged one. The returned FeatureSet will be fully resolved + // for any extensions that were used to construct the defaults. 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, - FeatureSet generated_defaults) - : edition_(edition), - descriptor_(descriptor), - message_factory_(std::move(message_factory)), - defaults_(std::move(defaults)), - generated_defaults_(std::move(generated_defaults)) {} - - absl::Status RegisterExtensions(const Descriptor& message); - absl::Status RegisterExtension(const FieldDescriptor& extension); + explicit FeatureResolver(FeatureSet defaults) + : defaults_(std::move(defaults)) {} - std::string edition_; - const Descriptor& descriptor_; - absl::flat_hash_set extensions_; - std::unique_ptr message_factory_; - std::unique_ptr defaults_; - FeatureSet generated_defaults_; + FeatureSet defaults_; }; } // namespace protobuf diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index d42c491a4e..aa49bfbb0d 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -30,10 +30,14 @@ #include "google/protobuf/feature_resolver.h" +#include #include #include #include +#include "absl/log/absl_check.h" +#include "absl/log/absl_log.h" +#include "absl/log/die_if_null.h" #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -43,6 +47,7 @@ #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" #include "google/protobuf/io/tokenizer.h" +#include "google/protobuf/io/zero_copy_stream_impl_lite.h" #include "google/protobuf/test_textproto.h" #include "google/protobuf/text_format.h" #include "google/protobuf/unittest_custom_options.pb.h" @@ -61,7 +66,6 @@ using ::testing::ExplainMatchResult; using ::testing::HasSubstr; // TODO(b/234474291): Use the gtest versions once that's available in OSS. -absl::Status GetStatus(const absl::Status& s) { return s; } template absl::Status GetStatus(const absl::StatusOr& s) { return s.status(); @@ -81,45 +85,28 @@ MATCHER_P(StatusIs, status, #define ASSERT_OK(x) ASSERT_THAT(x, StatusIs(absl::StatusCode::kOk)) template -const FileDescriptor& GetExtensionFile( +const FieldDescriptor* GetExtension( const ExtensionT& ext, const Descriptor* descriptor = FeatureSet::descriptor()) { - return *DescriptorPool::generated_pool() - ->FindExtensionByNumber(descriptor, ext.number()) - ->file(); + return ABSL_DIE_IF_NULL(descriptor->file()->pool()->FindExtensionByNumber( + descriptor, ext.number())); } -const DescriptorPool* EmptyPool() { - static DescriptorPool* empty_pool = new DescriptorPool(); - return empty_pool; -} - -template -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))...}; - for (absl::Status result : results) { - RETURN_IF_ERROR(result); - } - return resolver; -} template absl::StatusOr SetupFeatureResolver(absl::string_view edition, Extensions... extensions) { - return SetupFeatureResolverImpl(edition, EmptyPool(), extensions...); + absl::StatusOr defaults = + FeatureResolver::CompileDefaults(FeatureSet::descriptor(), + {GetExtension(extensions)...}, "2023", + "2023"); + RETURN_IF_ERROR(defaults.status()); + return FeatureResolver::Create(edition, *defaults); } -template -absl::StatusOr GetDefaultsImpl(absl::string_view edition, - const DescriptorPool* pool, - Extensions... extensions) { +absl::StatusOr GetDefaults(absl::string_view edition, + const FeatureSetDefaults& defaults) { absl::StatusOr resolver = - SetupFeatureResolverImpl(edition, pool, extensions...); + FeatureResolver::Create(edition, defaults); RETURN_IF_ERROR(resolver.status()); FeatureSet parent, child; return resolver->MergeFeatures(parent, child); @@ -128,7 +115,12 @@ absl::StatusOr GetDefaultsImpl(absl::string_view edition, template absl::StatusOr GetDefaults(absl::string_view edition, Extensions... extensions) { - return GetDefaultsImpl(edition, EmptyPool(), extensions...); + absl::StatusOr defaults = + FeatureResolver::CompileDefaults(FeatureSet::descriptor(), + {GetExtension(extensions)...}, "0", + "99999"); + RETURN_IF_ERROR(defaults.status()); + return GetDefaults(edition, *defaults); } FileDescriptorProto GetProto(const FileDescriptor* file) { @@ -184,8 +176,10 @@ TEST(FeatureResolverTest, DefaultsTestMessageExtension) { EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); + EXPECT_FALSE(merged->HasExtension(pb::test)); - const pb::TestFeatures& ext = merged->GetExtension(pb::test); + const pb::TestFeatures& ext = + merged->GetExtension(pb::TestMessage::test_message); EXPECT_EQ(ext.int_file_feature(), 1); EXPECT_EQ(ext.int_extension_range_feature(), 1); EXPECT_EQ(ext.int_message_feature(), 1); @@ -212,8 +206,10 @@ TEST(FeatureResolverTest, DefaultsTestNestedExtension) { EXPECT_EQ(merged->enum_type(), FeatureSet::OPEN); EXPECT_EQ(merged->repeated_field_encoding(), FeatureSet::PACKED); EXPECT_EQ(merged->message_encoding(), FeatureSet::LENGTH_PREFIXED); + EXPECT_FALSE(merged->HasExtension(pb::test)); - const pb::TestFeatures& ext = merged->GetExtension(pb::test); + const pb::TestFeatures& ext = + merged->GetExtension(pb::TestMessage::Nested::test_nested); EXPECT_EQ(ext.int_file_feature(), 1); EXPECT_EQ(ext.int_extension_range_feature(), 1); EXPECT_EQ(ext.int_message_feature(), 1); @@ -238,25 +234,19 @@ TEST(FeatureResolverTest, DefaultsGeneratedPoolCustom) { nullptr); ASSERT_NE(pool.BuildFile(GetProto(pb::TestFeatures::descriptor()->file())), nullptr); - absl::StatusOr merged = GetDefaultsImpl("2023", &pool); - ASSERT_OK(merged); + absl::StatusOr defaults = + FeatureResolver::CompileDefaults( + pool.FindMessageTypeByName("google.protobuf.FeatureSet"), + {pool.FindExtensionByName("pb.test")}, "2023", "2023"); + ASSERT_OK(defaults); + ASSERT_EQ(defaults->defaults().size(), 1); + ASSERT_EQ(defaults->defaults().at(0).edition(), "2023"); + FeatureSet merged = defaults->defaults().at(0).features(); - 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); + 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, DefaultsTooEarly) { @@ -328,28 +318,48 @@ TEST(FeatureResolverTest, DefaultsMessageMerge) { } } -TEST(FeatureResolverTest, InitializePoolMissingDescriptor) { - DescriptorPool pool; - EXPECT_THAT(FeatureResolver::Create("2023", nullptr, EmptyPool()), - HasError(HasSubstr("find definition of google.protobuf.FeatureSet"))); +TEST(FeatureResolverTest, CreateFromUnsortedDefaults) { + FeatureSetDefaults defaults = ParseTextOrDie(R"pb( + minimum_edition: "2022" + maximum_edition: "2024" + defaults { + edition: "2022" + features {} + } + defaults { + edition: "2024" + features {} + } + defaults { + edition: "2023" + features {} + } + )pb"); + EXPECT_THAT( + FeatureResolver::Create("2023", defaults), + HasError(AllOf( + HasSubstr("not strictly increasing."), + HasSubstr("Edition 2024 is greater than or equal to edition 2023")))); } -TEST(FeatureResolverTest, RegisterExtensionDifferentContainingType) { - auto resolver = - FeatureResolver::Create("2023", FeatureSet::descriptor(), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_OK(resolver->RegisterExtensions( - GetExtensionFile(protobuf_unittest::file_opt1, FileOptions::descriptor()))); +TEST(FeatureResolverTest, CompileDefaultsMissingDescriptor) { + EXPECT_THAT(FeatureResolver::CompileDefaults(nullptr, {}, "2023", "2023"), + HasError(HasSubstr("find definition of google.protobuf.FeatureSet"))); } -TEST(FeatureResolverTest, RegisterExtensionTwice) { - auto resolver = - FeatureResolver::Create("2023", FeatureSet::descriptor(), EmptyPool()); - ASSERT_OK(resolver); +TEST(FeatureResolverTest, CompileDefaultsMissingExtension) { + EXPECT_THAT(FeatureResolver::CompileDefaults(FeatureSet::descriptor(), + {nullptr}, "2023", "2023"), + HasError(HasSubstr("Unknown extension"))); +} - EXPECT_OK(resolver->RegisterExtensions(GetExtensionFile(pb::test))); - EXPECT_OK(resolver->RegisterExtensions(GetExtensionFile(pb::test))); +TEST(FeatureResolverTest, CompileDefaultsInvalidExtension) { + EXPECT_THAT( + FeatureResolver::CompileDefaults( + FeatureSet::descriptor(), + {GetExtension(protobuf_unittest::file_opt1, FileOptions::descriptor())}, + "2023", "2023"), + HasError(HasSubstr("is not an extension of"))); } TEST(FeatureResolverTest, MergeFeaturesChildOverrideCore) { @@ -508,13 +518,17 @@ TEST(FeatureResolverTest, MergeFeaturesExtensionEnumUnknown) { pb::TestFeatures::TEST_ENUM_FEATURE_UNKNOWN); } -// TODO(b/273611177) Move the following tests to descriptor_unittest.cc once -// FeatureResolver is hooked up. These will all fail during the descriptor -// build, invalidating the test setup. -// -// Note: We should make sure to keep coverage over the custom DescriptorPool -// cases. These are the only tests covering issues there (the ones above use -// the generated descriptor pool). +TEST(FeatureResolverTest, MergeFeaturesDistantPast) { + EXPECT_THAT(SetupFeatureResolver("1000"), + HasError(AllOf(HasSubstr("Edition 1000"), + HasSubstr("minimum supported edition 2023")))); +} + +TEST(FeatureResolverTest, MergeFeaturesDistantFuture) { + EXPECT_THAT(SetupFeatureResolver("9999"), + HasError(AllOf(HasSubstr("Edition 9999"), + HasSubstr("maximum supported edition 2023")))); +} class FakeErrorCollector : public io::ErrorCollector { public: @@ -534,6 +548,12 @@ class FeatureResolverPoolTest : public testing::Test { FileDescriptorProto file; FileDescriptorProto::GetDescriptor()->file()->CopyTo(&file); ASSERT_NE(pool_.BuildFile(file), nullptr); + feature_set_ = pool_.FindMessageTypeByName("google.protobuf.FeatureSet"); + ASSERT_NE(feature_set_, nullptr); + auto defaults = + FeatureResolver::CompileDefaults(feature_set_, {}, "2023", "2023"); + ASSERT_OK(defaults); + defaults_ = std::move(defaults).value(); } const FileDescriptor* ParseSchema(absl::string_view schema) { @@ -552,9 +572,11 @@ class FeatureResolverPoolTest : public testing::Test { DescriptorPool pool_; FileDescriptorProto file_proto_; + const Descriptor* feature_set_; + FeatureSetDefaults defaults_; }; -TEST_F(FeatureResolverPoolTest, RegisterExtensionNonMessage) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidNonMessage) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -567,16 +589,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionNonMessage) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_THAT(resolver->RegisterExtensions(*file), - HasError(AllOf(HasSubstr("test.bar"), - HasSubstr("is not of message type")))); + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError( + AllOf(HasSubstr("test.bar"), HasSubstr("is not of message type")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionRepeated) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidRepeated) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -589,16 +609,13 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionRepeated) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - resolver->RegisterExtensions(*file), + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), HasError(AllOf(HasSubstr("test.bar"), HasSubstr("repeated extension")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionWithExtensions) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithExtensions) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -619,16 +636,13 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithExtensions) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - resolver->RegisterExtensions(*file), + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), HasError(AllOf(HasSubstr("test.bar"), HasSubstr("Nested extensions")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionWithOneof) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithOneof) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -652,16 +666,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithOneof) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_THAT(resolver->RegisterExtensions(*file), - HasError(AllOf(HasSubstr("test.Foo"), - HasSubstr("oneof feature fields")))); + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError( + AllOf(HasSubstr("test.Foo"), HasSubstr("oneof feature fields")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRequired) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithRequired) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -679,16 +691,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRequired) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_THAT(resolver->RegisterExtensions(*file), - HasError(AllOf(HasSubstr("test.Foo.required_field"), - HasSubstr("required field")))); + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError(AllOf(HasSubstr("test.Foo.required_field"), + HasSubstr("required field")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRepeated) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithRepeated) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -706,16 +716,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithRepeated) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_THAT(resolver->RegisterExtensions(*file), - HasError(AllOf(HasSubstr("test.Foo.repeated_field"), - HasSubstr("repeated field")))); + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError(AllOf(HasSubstr("test.Foo.repeated_field"), + HasSubstr("repeated field")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionWithMissingTarget) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithMissingTarget) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -732,16 +740,15 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionWithMissingTarget) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); - - EXPECT_THAT(resolver->RegisterExtensions(*file), - HasError(AllOf(HasSubstr("test.Foo.int_field"), - HasSubstr("no target specified")))); + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError(AllOf(HasSubstr("test.Foo.int_field"), + HasSubstr("no target specified")))); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsMessageParsingError) { +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidDefaultsMessageParsingError) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -762,16 +769,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsMessageParsingError) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - resolver->RegisterExtensions(*file), + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), HasError(AllOf(HasSubstr("in edition_defaults"), HasSubstr("9987")))); } TEST_F(FeatureResolverPoolTest, - RegisterExtensionDefaultsMessageParsingErrorMerged) { + CompileDefaultsInvalidDefaultsMessageParsingErrorMerged) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -794,16 +799,14 @@ TEST_F(FeatureResolverPoolTest, )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - resolver->RegisterExtensions(*file), + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2025"), HasError(AllOf(HasSubstr("in edition_defaults"), HasSubstr("9987")))); } TEST_F(FeatureResolverPoolTest, - RegisterExtensionDefaultsMessageParsingErrorSkipped) { + CompileDefaultsInvalidDefaultsMessageParsingErrorSkipped) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -826,15 +829,19 @@ TEST_F(FeatureResolverPoolTest, )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2024", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); + const FieldDescriptor* ext = file->extension(0); + auto defaults = + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2024"); + ASSERT_OK(defaults); + + auto resolver = FeatureResolver::Create("2023", *defaults); ASSERT_OK(resolver); - EXPECT_OK(resolver->RegisterExtensions(*file)); FeatureSet parent, child; EXPECT_OK(resolver->MergeFeatures(parent, child)); } -TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsScalarParsingError) { +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidDefaultsScalarParsingError) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -852,16 +859,14 @@ TEST_F(FeatureResolverPoolTest, RegisterExtensionDefaultsScalarParsingError) { )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); - ASSERT_OK(resolver); + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - resolver->RegisterExtensions(*file), + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), HasError(AllOf(HasSubstr("in edition_defaults"), HasSubstr("1.23")))); } TEST_F(FeatureResolverPoolTest, - RegisterExtensionDefaultsScalarParsingErrorSkipped) { + CompileDefaultsInvalidDefaultsScalarParsingErrorSkipped) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; package test; @@ -880,41 +885,39 @@ TEST_F(FeatureResolverPoolTest, )schema"); ASSERT_NE(file, nullptr); - auto resolver = FeatureResolver::Create( - "2023", pool_.FindMessageTypeByName("google.protobuf.FeatureSet"), EmptyPool()); + const FieldDescriptor* ext = file->extension(0); + auto defaults = + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"); + ASSERT_OK(defaults); + + auto resolver = FeatureResolver::Create("2023", *defaults); ASSERT_OK(resolver); - EXPECT_OK(resolver->RegisterExtensions(*file)); FeatureSet parent, child; EXPECT_OK(resolver->MergeFeatures(parent, child)); } -TEST_F(FeatureResolverPoolTest, GeneratedPoolInvalidExtension) { +TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidDefaultsTooEarly) { 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; + optional Foo bar = 9999; + } + message Foo { + optional int32 int_field_feature = 12 [ + targets = TARGET_TYPE_FIELD, + edition_defaults = { edition: "2022", value: "1" } + ]; } )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); + const FieldDescriptor* ext = file->extension(0); EXPECT_THAT( - GetDefaultsImpl("2022", &pool_), - HasError(AllOf(HasSubstr("No valid default found"), HasSubstr("2022")))); + FeatureResolver::CompileDefaults(feature_set_, {ext}, "2023", "2023"), + HasError(HasSubstr("No valid default found for edition 2022"))); } } // namespace