From 887e95dade3b93daf6cc14e1ac7ccbccc66053a9 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Thu, 30 May 2024 22:18:22 -0700 Subject: [PATCH] Fix a bug in edition defaults calculation. Since the fixed/overridable split can occur whenever a feature is introduced or removed, we need to include those editions in the resulting compiled defaults. This does bug only affects edition 2024 and later, where features may be removed or introduced in isolation. PiperOrigin-RevId: 638903990 --- src/google/protobuf/feature_resolver.cc | 27 ++++++- src/google/protobuf/feature_resolver_test.cc | 75 ++++++++++++++++++++ src/google/protobuf/unittest_features.proto | 2 +- 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index bf24e18a17b7..7accef385257 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 e5aaf05ffe6d..bbd4750c4e0f 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 d32f91c38abf..ddf510c6b6ce 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" },