diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index bf24e18a17b71..7accef3852577 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -189,17 +189,38 @@ absl::Status ValidateExtension(const Descriptor& feature_set, return absl::OkStatus(); } +void MaybeInsertEdition(Edition edition, Edition maximum_edition, + absl::btree_set& editions) { + if (edition <= maximum_edition) { + editions.insert(edition); + } +} + +// This collects all of the editions that are relevant to any features defined +// in a message descriptor. We only need to consider editions where something +// has changed. void CollectEditions(const Descriptor& descriptor, Edition 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 (maximum_edition < def.edition()) continue; + const FieldOptions& options = descriptor.field(i)->options(); + // Editions where a new feature is introduced should be captured. + MaybeInsertEdition(options.feature_support().edition_introduced(), + maximum_edition, editions); + + // Editions where a feature is removed should be captured. + if (options.feature_support().has_edition_removed()) { + MaybeInsertEdition(options.feature_support().edition_removed(), + maximum_edition, editions); + } + + // Any edition where a default value changes should be captured. + for (const auto& def : options.edition_defaults()) { // TODO Remove this once all features use EDITION_LEGACY. if (def.edition() == Edition::EDITION_LEGACY) { editions.insert(Edition::EDITION_PROTO2); continue; } - editions.insert(def.edition()); + MaybeInsertEdition(def.edition(), maximum_edition, editions); } } } diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index e5aaf05ffe6da..bbd4750c4e0f1 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -1311,6 +1311,81 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumTooEarly) { HasError(HasSubstr("edition 1_TEST_ONLY is earlier than the oldest"))); } +TEST_F(FeatureResolverPoolTest, CompileDefaultsRemovedOnly) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum Bar { + TEST_ENUM_FEATURE_UNKNOWN = 0; + VALUE1 = 1; + VALUE2 = 2; + } + message Foo { + optional Bar file_feature = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + feature_support.edition_removed = EDITION_99998_TEST_ONLY, + edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + auto compiled_defaults = FeatureResolver::CompileDefaults( + feature_set_, {ext}, EDITION_99997_TEST_ONLY, EDITION_99999_TEST_ONLY); + ASSERT_OK(compiled_defaults); + const auto& defaults = *compiled_defaults->defaults().rbegin(); + EXPECT_THAT(defaults.edition(), EDITION_99998_TEST_ONLY); + EXPECT_THAT(defaults.fixed_features().GetExtension(pb::test).file_feature(), + pb::VALUE1); + EXPECT_FALSE(defaults.overridable_features() + .GetExtension(pb::test) + .has_file_feature()); +} + +TEST_F(FeatureResolverPoolTest, CompileDefaultsIntroducedOnly) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum Bar { + TEST_ENUM_FEATURE_UNKNOWN = 0; + VALUE1 = 1; + VALUE2 = 2; + } + message Foo { + optional Bar file_feature = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_99998_TEST_ONLY, + edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + auto compiled_defaults = FeatureResolver::CompileDefaults( + feature_set_, {ext}, EDITION_99997_TEST_ONLY, EDITION_99999_TEST_ONLY); + ASSERT_OK(compiled_defaults); + const auto& defaults = *compiled_defaults->defaults().rbegin(); + EXPECT_THAT(defaults.edition(), EDITION_99998_TEST_ONLY); + EXPECT_THAT( + defaults.overridable_features().GetExtension(pb::test).file_feature(), + pb::VALUE1); + EXPECT_FALSE( + defaults.fixed_features().GetExtension(pb::test).has_file_feature()); +} + TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumCovered) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto index d32f91c38abf4..ddf510c6b6ce1 100644 --- a/src/google/protobuf/unittest_features.proto +++ b/src/google/protobuf/unittest_features.proto @@ -193,7 +193,7 @@ message TestFeatures { targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, feature_support = { - edition_introduced: EDITION_PROTO2 + edition_introduced: EDITION_PROTO3 edition_removed: EDITION_2023 }, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },