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: 640267061
pull/16992/head
Mike Kruskal 6 months ago committed by Copybara-Service
parent bf7ac9f2f1
commit f3c0c5995c
  1. 27
      src/google/protobuf/feature_resolver.cc
  2. 75
      src/google/protobuf/feature_resolver_test.cc
  3. 2
      src/google/protobuf/unittest_features.proto

@ -187,12 +187,33 @@ absl::Status ValidateExtension(const Descriptor& feature_set,
return absl::OkStatus();
}
void MaybeInsertEdition(Edition edition, Edition maximum_edition,
absl::btree_set<Edition>& 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<Edition>& 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;
editions.insert(def.edition());
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()) {
MaybeInsertEdition(def.edition(), maximum_edition, editions);
}
}
}

@ -1310,6 +1310,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";

@ -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" },

Loading…
Cancel
Save