From 7821d808e409e4852ee85f94f0580b379ed47166 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Mon, 15 Apr 2024 15:46:20 -0700 Subject: [PATCH] Use newly split feature defaults in plugins and runtimes. The new fields fixed_features and overridable_features can be simply merged to recover the old aggregate defaults. By splitting them though, plugins and runtimes get some extra information about lifetimes for enforcement. PiperOrigin-RevId: 625107743 --- editions/defaults_test.cc | 100 +++++++++++------- .../java/com/google/protobuf/Descriptors.java | 6 +- .../com/google/protobuf/DescriptorsTest.java | 2 +- python/google/protobuf/descriptor_pool.py | 5 +- .../protobuf/internal/descriptor_pool_test.py | 54 +++++++--- .../protobuf/internal/descriptor_test.py | 4 +- .../command_line_interface_unittest.cc | 40 ++++--- src/google/protobuf/descriptor_unittest.cc | 8 +- src/google/protobuf/feature_resolver.cc | 8 +- src/google/protobuf/feature_resolver_test.cc | 43 +++++++- upb/reflection/file_def.c | 23 +++- 11 files changed, 204 insertions(+), 89 deletions(-) diff --git a/editions/defaults_test.cc b/editions/defaults_test.cc index 2bb0587c15ba9..99432c8c7812d 100644 --- a/editions/defaults_test.cc +++ b/editions/defaults_test.cc @@ -50,11 +50,13 @@ TEST(DefaultsTest, Check2023) { EXPECT_EQ(defaults->defaults()[0].edition(), EDITION_PROTO2); EXPECT_EQ(defaults->defaults()[1].edition(), EDITION_PROTO3); EXPECT_EQ(defaults->defaults()[2].edition(), EDITION_2023); - EXPECT_EQ(defaults->defaults()[2].features().field_presence(), + EXPECT_EQ(defaults->defaults()[2].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[2].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults->defaults()[2] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); } TEST(DefaultsTest, CheckFuture) { @@ -67,23 +69,29 @@ TEST(DefaultsTest, CheckFuture) { EXPECT_EQ(defaults->defaults()[0].edition(), EDITION_PROTO2); EXPECT_EQ(defaults->defaults()[1].edition(), EDITION_PROTO3); EXPECT_EQ(defaults->defaults()[2].edition(), EDITION_2023); - EXPECT_EQ(defaults->defaults()[2].features().field_presence(), + EXPECT_EQ(defaults->defaults()[2].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[2].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults->defaults()[2] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); EXPECT_EQ(defaults->defaults()[3].edition(), EDITION_2024); - EXPECT_EQ(defaults->defaults()[3].features().field_presence(), + EXPECT_EQ(defaults->defaults()[3].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[3].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults->defaults()[3] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); EXPECT_EQ(defaults->defaults()[4].edition(), EDITION_99997_TEST_ONLY); - EXPECT_EQ(defaults->defaults()[4].features().field_presence(), + EXPECT_EQ(defaults->defaults()[4].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[4].features().GetExtension(pb::test).file_feature(), - pb::VALUE4); + EXPECT_EQ(defaults->defaults()[4] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE4); } TEST(DefaultsTest, CheckFarFuture) { @@ -96,29 +104,37 @@ TEST(DefaultsTest, CheckFarFuture) { EXPECT_EQ(defaults->defaults()[0].edition(), EDITION_PROTO2); EXPECT_EQ(defaults->defaults()[1].edition(), EDITION_PROTO3); EXPECT_EQ(defaults->defaults()[2].edition(), EDITION_2023); - EXPECT_EQ(defaults->defaults()[2].features().field_presence(), + EXPECT_EQ(defaults->defaults()[2].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[2].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults->defaults()[2] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); EXPECT_EQ(defaults->defaults()[3].edition(), EDITION_2024); - EXPECT_EQ(defaults->defaults()[3].features().field_presence(), + EXPECT_EQ(defaults->defaults()[3].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[3].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults->defaults()[3] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); EXPECT_EQ(defaults->defaults()[4].edition(), EDITION_99997_TEST_ONLY); - EXPECT_EQ(defaults->defaults()[4].features().field_presence(), + EXPECT_EQ(defaults->defaults()[4].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[4].features().GetExtension(pb::test).file_feature(), - pb::VALUE4); + EXPECT_EQ(defaults->defaults()[4] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE4); EXPECT_EQ(defaults->defaults()[5].edition(), EDITION_99998_TEST_ONLY); - EXPECT_EQ(defaults->defaults()[5].features().field_presence(), + EXPECT_EQ(defaults->defaults()[5].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults->defaults()[5].features().GetExtension(pb::test).file_feature(), - pb::VALUE5); + EXPECT_EQ(defaults->defaults()[5] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE5); } TEST(DefaultsTest, Embedded) { @@ -133,11 +149,13 @@ TEST(DefaultsTest, Embedded) { EXPECT_EQ(defaults.defaults()[0].edition(), EDITION_PROTO2); EXPECT_EQ(defaults.defaults()[1].edition(), EDITION_PROTO3); EXPECT_EQ(defaults.defaults()[2].edition(), EDITION_2023); - EXPECT_EQ(defaults.defaults()[2].features().field_presence(), + EXPECT_EQ(defaults.defaults()[2].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults.defaults()[2].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults.defaults()[2] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); } TEST(DefaultsTest, EmbeddedBase64) { @@ -155,11 +173,13 @@ TEST(DefaultsTest, EmbeddedBase64) { EXPECT_EQ(defaults.defaults()[0].edition(), EDITION_PROTO2); EXPECT_EQ(defaults.defaults()[1].edition(), EDITION_PROTO3); EXPECT_EQ(defaults.defaults()[2].edition(), EDITION_2023); - EXPECT_EQ(defaults.defaults()[2].features().field_presence(), + EXPECT_EQ(defaults.defaults()[2].overridable_features().field_presence(), FeatureSet::EXPLICIT); - EXPECT_EQ( - defaults.defaults()[2].features().GetExtension(pb::test).file_feature(), - pb::VALUE3); + EXPECT_EQ(defaults.defaults()[2] + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::VALUE3); } } // namespace diff --git a/java/core/src/main/java/com/google/protobuf/Descriptors.java b/java/core/src/main/java/com/google/protobuf/Descriptors.java index 9aaa4799b2809..f4aa9ca3ba054 100644 --- a/java/core/src/main/java/com/google/protobuf/Descriptors.java +++ b/java/core/src/main/java/com/google/protobuf/Descriptors.java @@ -127,18 +127,18 @@ static FeatureSet getEditionDefaults(Edition edition) { + javaEditionDefaults.getMaximumEdition() + "!"); } - FeatureSet found = null; + FeatureSetEditionDefault found = null; for (FeatureSetEditionDefault editionDefault : javaEditionDefaults.getDefaultsList()) { if (editionDefault.getEdition().getNumber() > edition.getNumber()) { break; } - found = editionDefault.getFeatures(); + found = editionDefault; } if (found == null) { throw new IllegalArgumentException( "Edition " + edition + " does not have a valid default FeatureSet!"); } - return found; + return found.getFixedFeatures().toBuilder().mergeFrom(found.getOverridableFeatures()).build(); } private static FeatureSet internFeatures(FeatureSet features) { diff --git a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java index da53f22e2febf..d64186ad7ad8d 100644 --- a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java +++ b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java @@ -1242,7 +1242,7 @@ public static class FeatureInheritanceTest { public void setUp() { FeatureSetDefaults.Builder defaults = Descriptors.getJavaEditionDefaults().toBuilder(); for (FeatureSetEditionDefault.Builder editionDefaults : defaults.getDefaultsBuilderList()) { - setTestFeature(editionDefaults.getFeaturesBuilder(), 1); + setTestFeature(editionDefaults.getOverridableFeaturesBuilder(), 1); } Descriptors.setTestJavaEditionDefaults(defaults.build()); diff --git a/python/google/protobuf/descriptor_pool.py b/python/google/protobuf/descriptor_pool.py index 2bb3ceecdc651..5545aed9a2516 100644 --- a/python/google/protobuf/descriptor_pool.py +++ b/python/google/protobuf/descriptor_pool.py @@ -737,7 +737,7 @@ def _CreateDefaultFeatures(self, edition): for d in self._edition_defaults.defaults: if d.edition > edition: break - found = d.features + found = d if found is None: raise TypeError( 'No valid default found for edition %s!' @@ -745,7 +745,8 @@ def _CreateDefaultFeatures(self, edition): ) defaults = descriptor_pb2.FeatureSet() - defaults.CopyFrom(found) + defaults.CopyFrom(found.fixed_features) + defaults.MergeFrom(found.overridable_features) return defaults def _InternFeatures(self, features): diff --git a/python/google/protobuf/internal/descriptor_pool_test.py b/python/google/protobuf/internal/descriptor_pool_test.py index bfe1e154dc8ef..c903bcc702446 100644 --- a/python/google/protobuf/internal/descriptor_pool_test.py +++ b/python/google/protobuf/internal/descriptor_pool_test.py @@ -1131,19 +1131,49 @@ def testDefault(self): file._GetFeatures().HasExtension(unittest_features_pb2.test) ) + def testMergedDefaults(self): + pool = descriptor_pool.DescriptorPool() + fixed = descriptor_pb2.FeatureSet() + fixed.CopyFrom(unittest_features_pb2.DESCRIPTOR._GetFeatures()) + fixed.field_presence = descriptor_pb2.FeatureSet.IMPLICIT + fixed.ClearField('message_encoding') + defaults = descriptor_pb2.FeatureSetDefaults( + defaults=[ + descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( + edition=descriptor_pb2.Edition.EDITION_PROTO2, + fixed_features=fixed, + overridable_features=descriptor_pb2.FeatureSet( + message_encoding=descriptor_pb2.FeatureSet.DELIMITED + ), + ) + ], + minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, + maximum_edition=descriptor_pb2.Edition.EDITION_2023, + ) + pool.SetFeatureSetDefaults(defaults) + file_desc = descriptor_pb2.FileDescriptorProto(name='some/file.proto') + file = pool.AddSerializedFile(file_desc.SerializeToString()) + self.assertEqual( + file._GetFeatures().message_encoding, + descriptor_pb2.FeatureSet.DELIMITED, + ) + self.assertEqual( + file._GetFeatures().field_presence, descriptor_pb2.FeatureSet.IMPLICIT + ) + def testOverride(self): pool = descriptor_pool.DescriptorPool() defaults = descriptor_pb2.FeatureSetDefaults( defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, maximum_edition=descriptor_pb2.Edition.EDITION_2023, ) - defaults.defaults[0].features.Extensions[ + defaults.defaults[0].overridable_features.Extensions[ unittest_features_pb2.test ].file_feature = unittest_features_pb2.VALUE9 pool.SetFeatureSetDefaults(defaults) @@ -1177,7 +1207,7 @@ def testInvalidEditionRange(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_2023, @@ -1197,11 +1227,11 @@ def testNotStrictlyIncreasing(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO3, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ), descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ), ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, @@ -1217,11 +1247,11 @@ def testUnknownEdition(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_UNKNOWN, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ), descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ), ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, @@ -1238,7 +1268,7 @@ def testChangeAfterBuild(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, @@ -1252,7 +1282,7 @@ def testChangeDefaultPool(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, @@ -1267,7 +1297,7 @@ def testNoValidFeatures(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_2023, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, @@ -1285,7 +1315,7 @@ def testBelowMinimum(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO3, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO3, @@ -1305,7 +1335,7 @@ def testAboveMaximum(self): defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_features_pb2.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, diff --git a/python/google/protobuf/internal/descriptor_test.py b/python/google/protobuf/internal/descriptor_test.py index 3e82c4a0ad8aa..ce16d0363ac01 100755 --- a/python/google/protobuf/internal/descriptor_test.py +++ b/python/google/protobuf/internal/descriptor_test.py @@ -1455,13 +1455,13 @@ class ReturnObject: defaults=[ descriptor_pb2.FeatureSetDefaults.FeatureSetEditionDefault( edition=descriptor_pb2.Edition.EDITION_PROTO2, - features=unittest_pb2.TestAllTypes.DESCRIPTOR._GetFeatures(), + overridable_features=unittest_pb2.TestAllTypes.DESCRIPTOR._GetFeatures(), ) ], minimum_edition=descriptor_pb2.Edition.EDITION_PROTO2, maximum_edition=descriptor_pb2.Edition.EDITION_2023, ) - defaults.defaults[0].features.Extensions[ + defaults.defaults[0].overridable_features.Extensions[ unittest_features_pb2.test ].multiple_feature = 1 ret.pool.SetFeatureSetDefaults(defaults) diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 1b6351415b64c..8318bda0b83f5 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -2102,21 +2102,31 @@ TEST_F(CommandLineInterfaceTest, EditionDefaultsWithExtension) { EXPECT_EQ(defaults.defaults(3).edition(), EDITION_2024); EXPECT_EQ(defaults.defaults(4).edition(), EDITION_99997_TEST_ONLY); EXPECT_EQ(defaults.defaults(5).edition(), EDITION_99998_TEST_ONLY); - EXPECT_EQ( - defaults.defaults(0).features().GetExtension(pb::test).file_feature(), - pb::EnumFeature::VALUE1); - EXPECT_EQ( - defaults.defaults(2).features().GetExtension(pb::test).file_feature(), - pb::EnumFeature::VALUE3); - EXPECT_EQ( - defaults.defaults(3).features().GetExtension(pb::test).file_feature(), - pb::EnumFeature::VALUE3); - EXPECT_EQ( - defaults.defaults(4).features().GetExtension(pb::test).file_feature(), - pb::EnumFeature::VALUE4); - EXPECT_EQ( - defaults.defaults(5).features().GetExtension(pb::test).file_feature(), - pb::EnumFeature::VALUE5); + EXPECT_EQ(defaults.defaults(0) + .fixed_features() + .GetExtension(pb::test) + .file_feature(), + pb::EnumFeature::VALUE1); + EXPECT_EQ(defaults.defaults(2) + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::EnumFeature::VALUE3); + EXPECT_EQ(defaults.defaults(3) + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::EnumFeature::VALUE3); + EXPECT_EQ(defaults.defaults(4) + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::EnumFeature::VALUE4); + EXPECT_EQ(defaults.defaults(5) + .overridable_features() + .GetExtension(pb::test) + .file_feature(), + pb::EnumFeature::VALUE5); } #ifndef _WIN32 diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 6e16541ca707a..d03afc359320d 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -10730,7 +10730,7 @@ TEST_F(DescriptorPoolFeaturesTest, UnknownDefaults) { FeatureSetDefaults defaults = ParseTextOrDie(R"pb( defaults { edition: EDITION_UNKNOWN - features {} + overridable_features {} } minimum_edition: EDITION_PROTO2 maximum_edition: EDITION_2023 @@ -10744,11 +10744,11 @@ TEST_F(DescriptorPoolFeaturesTest, NotStrictlyIncreasing) { FeatureSetDefaults defaults = ParseTextOrDie(R"pb( defaults { edition: EDITION_PROTO3 - features {} + overridable_features {} } defaults { edition: EDITION_PROTO2 - features {} + overridable_features {} } minimum_edition: EDITION_PROTO2 maximum_edition: EDITION_2023 @@ -10766,7 +10766,7 @@ TEST_F(DescriptorPoolFeaturesTest, OverrideDefaults) { FeatureSetDefaults defaults = ParseTextOrDie(R"pb( defaults { edition: EDITION_PROTO2 - features { + overridable_features { field_presence: EXPLICIT enum_type: CLOSED repeated_field_encoding: EXPANDED diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index f5a87f1b32a23..55caa3c162a0c 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -418,7 +418,9 @@ absl::StatusOr FeatureResolver::Create( edition_default.edition(), "."); } } - RETURN_IF_ERROR(ValidateMergedFeatures(edition_default.features())); + FeatureSet features = edition_default.fixed_features(); + features.MergeFrom(edition_default.overridable_features()); + RETURN_IF_ERROR(ValidateMergedFeatures(features)); prev_edition = edition_default.edition(); } @@ -435,7 +437,9 @@ absl::StatusOr FeatureResolver::Create( return Error("No valid default found for edition ", edition); } - return FeatureResolver(std::prev(first_nonmatch)->features()); + FeatureSet features = std::prev(first_nonmatch)->fixed_features(); + features.MergeFrom(std::prev(first_nonmatch)->overridable_features()); + return FeatureResolver(std::move(features)); } absl::StatusOr FeatureResolver::MergeFeatures( diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index fb21761efcb42..4a064e49db1ab 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -215,6 +215,39 @@ TEST(FeatureResolverTest, DefaultsGeneratedPoolCustom) { EXPECT_FALSE(merged.HasExtension(pb::cpp)); } +TEST(FeatureResolverTest, DefaultsMergedFeatures) { + absl::StatusOr defaults = + FeatureResolver::CompileDefaults(FeatureSet::descriptor(), + {GetExtension(pb::test)}, EDITION_2023, + EDITION_2023); + ASSERT_OK(defaults); + ASSERT_EQ(defaults->defaults_size(), 3); + + defaults->mutable_defaults(2) + ->mutable_fixed_features() + ->MutableExtension(pb::test) + ->set_file_feature(pb::VALUE7); + defaults->mutable_defaults(2) + ->mutable_fixed_features() + ->MutableExtension(pb::test) + ->set_multiple_feature(pb::VALUE6); + defaults->mutable_defaults(2) + ->mutable_overridable_features() + ->MutableExtension(pb::test) + ->clear_file_feature(); + defaults->mutable_defaults(2) + ->mutable_overridable_features() + ->MutableExtension(pb::test) + ->set_multiple_feature(pb::VALUE8); + + absl::StatusOr features = GetDefaults(EDITION_2023, *defaults); + ASSERT_OK(features); + + const pb::TestFeatures& ext = features->GetExtension(pb::test); + EXPECT_EQ(ext.file_feature(), pb::VALUE7); + EXPECT_EQ(ext.multiple_feature(), pb::VALUE8); +} + TEST(FeatureResolverTest, DefaultsTooEarly) { absl::StatusOr defaults = FeatureResolver::CompileDefaults(FeatureSet::descriptor(), @@ -437,13 +470,15 @@ TEST(FeatureResolverTest, CreateUnknownEnumFeature) { for (int i = 0; i < descriptor.field_count(); ++i) { const FieldDescriptor& field = *descriptor.field(i); + // Clear the feature, which should be invalid. FeatureSetDefaults defaults = *valid_defaults; FeatureSet* features = - defaults.mutable_defaults()->Mutable(0)->mutable_features(); - const Reflection& reflection = *features->GetReflection(); + defaults.mutable_defaults()->Mutable(0)->mutable_overridable_features(); + features->GetReflection()->ClearField(features, &field); + features = + defaults.mutable_defaults()->Mutable(0)->mutable_fixed_features(); + features->GetReflection()->ClearField(features, &field); - // Clear the feature, which should be invalid. - reflection.ClearField(features, &field); EXPECT_THAT(FeatureResolver::Create(EDITION_2023, defaults), HasError(AllOf(HasSubstr(field.name()), HasSubstr("must resolve to a known value")))); diff --git a/upb/reflection/file_def.c b/upb/reflection/file_def.c index 34774f3df0035..5a51deecf947e 100644 --- a/upb/reflection/file_def.c +++ b/upb/reflection/file_def.c @@ -232,20 +232,35 @@ const UPB_DESC(FeatureSet*) size_t n; const UPB_DESC(FeatureSetDefaults_FeatureSetEditionDefault)* const* d = UPB_DESC(FeatureSetDefaults_defaults)(defaults, &n); - const UPB_DESC(FeatureSet)* ret = NULL; + const UPB_DESC(FeatureSetDefaults_FeatureSetEditionDefault)* result = NULL; for (size_t i = 0; i < n; i++) { if (UPB_DESC(FeatureSetDefaults_FeatureSetEditionDefault_edition)(d[i]) > edition) { break; } - ret = UPB_DESC(FeatureSetDefaults_FeatureSetEditionDefault_features)(d[i]); + result = d[i]; } - if (ret == NULL) { + if (result == NULL) { _upb_DefBuilder_Errf(ctx, "No valid default found for edition %s", upb_FileDef_EditionName(edition)); return NULL; } - return ret; + + // Merge the fixed and overridable features to get the edition's default + // feature set. + const UPB_DESC(FeatureSet)* fixed = UPB_DESC( + FeatureSetDefaults_FeatureSetEditionDefault_fixed_features)(result); + const UPB_DESC(FeatureSet)* overridable = UPB_DESC( + FeatureSetDefaults_FeatureSetEditionDefault_overridable_features)(result); + if (!fixed && !overridable) { + _upb_DefBuilder_Errf(ctx, "No valid default found for edition %s", + upb_FileDef_EditionName(edition)); + return NULL; + } else if (!fixed) { + return overridable; + } + return _upb_DefBuilder_DoResolveFeatures(ctx, fixed, overridable, + /*is_implicit=*/true); } // Allocate and initialize one file def, and add it to the context object.