Skip to content

Commit

Permalink
Fix a bug in edition defaults calculation.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mkruskal-google committed Jun 4, 2024
1 parent d2edc49 commit 887e95d
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 4 deletions.
27 changes: 24 additions & 3 deletions src/google/protobuf/feature_resolver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,17 +189,38 @@ 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 comment has been minimized.

Copy link
@rrohit9579mali

rrohit9579mali Jun 19, 2024

hello


// 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;
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);
}
}
}
Expand Down
75 changes: 75 additions & 0 deletions src/google/protobuf/feature_resolver_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/unittest_features.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down

0 comments on commit 887e95d

Please sign in to comment.