Editions: Expand validation in feature resolution

- Check that the maximum edition is always after the minimum
- Make sure that the minimum edition gets included in the default mapping when it's necessary

PiperOrigin-RevId: 563236159

src/google/protobuf/BUILD.bazel

@@ -685,6 +685,7 @@ filegroup(
         "unittest_features.proto",
         "unittest_import.proto",
         "unittest_import_public.proto",
+        "unittest_invalid_features.proto",
         "unittest_lite_imports_nonlite.proto",
         "unittest_mset.proto",
         "unittest_mset_wire_format.proto",

src/google/protobuf/compiler/code_generator_unittest.cc

@@ -282,7 +282,7 @@ TEST_F(CodeGeneratorTest, BuildFeatureSetDefaultsInvalidExtension) {
 TEST_F(CodeGeneratorTest, BuildFeatureSetDefaults) {
   TestGenerator generator;
   generator.set_feature_extensions({});
-  generator.set_minimum_edition(EDITION_1_TEST_ONLY);
+  generator.set_minimum_edition(EDITION_99997_TEST_ONLY);
   generator.set_maximum_edition(EDITION_99999_TEST_ONLY);
   EXPECT_THAT(generator.BuildFeatureSetDefaults(),
               IsOkAndHolds(EqualsProto(R"pb(
@@ -296,7 +296,7 @@ TEST_F(CodeGeneratorTest, BuildFeatureSetDefaults) {
                     json_format: ALLOW
                   }
                 }
-                minimum_edition_enum: EDITION_1_TEST_ONLY
+                minimum_edition_enum: EDITION_99997_TEST_ONLY
                 maximum_edition_enum: EDITION_99999_TEST_ONLY
               )pb")));
 }

src/google/protobuf/compiler/command_line_interface_unittest.cc

@@ -44,6 +44,7 @@
 #include "absl/strings/str_format.h"
 #include "google/protobuf/compiler/command_line_interface_tester.h"
 #include "google/protobuf/unittest_features.pb.h"
+#include "google/protobuf/unittest_invalid_features.pb.h"
 
 #ifndef _MSC_VER
 #include <unistd.h>

src/google/protobuf/feature_resolver.cc

@@ -134,8 +134,7 @@ absl::Status ValidateExtension(const Descriptor& feature_set,
   return absl::OkStatus();
 }
 
-void CollectEditions(const Descriptor& descriptor, Edition minimum_edition,
+void CollectEditions(const Descriptor& descriptor, Edition maximum_edition,
-                     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()) {
@@ -221,6 +220,10 @@ absl::StatusOr<FeatureSetDefaults> FeatureResolver::CompileDefaults(
     const Descriptor* feature_set,
     absl::Span<const FieldDescriptor* const> extensions,
     Edition minimum_edition, Edition maximum_edition) {
+  if (minimum_edition > maximum_edition) {
+    return Error("Invalid edition range, edition ", minimum_edition,
+                 " is newer than edition ", maximum_edition);
+  }
   // Find and validate the FeatureSet in the pool.
   if (feature_set == nullptr) {
     return Error(
@@ -236,10 +239,14 @@ absl::StatusOr<FeatureSetDefaults> FeatureResolver::CompileDefaults(
 
   // Collect all the editions with unique defaults.
   absl::btree_set<Edition> editions;
-  CollectEditions(*feature_set, minimum_edition, maximum_edition, editions);
+  CollectEditions(*feature_set, maximum_edition, editions);
   for (const auto* extension : extensions) {
-    CollectEditions(*extension->message_type(), minimum_edition,
+    CollectEditions(*extension->message_type(), maximum_edition, editions);
-                    maximum_edition, editions);
+  }
+  if (editions.empty() || *editions.begin() > minimum_edition) {
+    // Always insert the minimum edition to make sure the full range is covered
+    // in valid defaults.
+    editions.insert(minimum_edition);
   }
 
   // Fill the default spec.

src/google/protobuf/feature_resolver_test.cc

@@ -251,10 +251,11 @@ TEST(FeatureResolverTest, DefaultsGeneratedPoolCustom) {
 
 TEST(FeatureResolverTest, DefaultsTooEarly) {
   absl::StatusOr<FeatureSetDefaults> defaults =
-      FeatureResolver::CompileDefaults(
+      FeatureResolver::CompileDefaults(FeatureSet::descriptor(),
-          FeatureSet::descriptor(), {GetExtension(pb::test)},
+                                       {GetExtension(pb::test)}, EDITION_2023,
-          EDITION_1_TEST_ONLY, EDITION_99999_TEST_ONLY);
+                                       EDITION_2023);
   ASSERT_OK(defaults);
+  defaults->set_minimum_edition_enum(EDITION_1_TEST_ONLY);
   absl::StatusOr<FeatureSet> merged =
       GetDefaults(EDITION_1_TEST_ONLY, *defaults);
   EXPECT_THAT(merged, HasError(AllOf(HasSubstr("No valid default found"),
@@ -387,6 +388,15 @@ TEST(FeatureResolverTest, CompileDefaultsInvalidExtension) {
       HasError(HasSubstr("is not an extension of")));
 }
 
+TEST(FeatureResolverTest, CompileDefaultsMinimumLaterThanMaximum) {
+  EXPECT_THAT(
+      FeatureResolver::CompileDefaults(FeatureSet::descriptor(), {},
+                                       EDITION_99999_TEST_ONLY, EDITION_2023),
+      HasError(AllOf(HasSubstr("Invalid edition range"),
+                     HasSubstr("99999_TEST_ONLY is newer"),
+                     HasSubstr("2023"))));
+}
+
 TEST(FeatureResolverTest, MergeFeaturesChildOverrideCore) {
   absl::StatusOr<FeatureResolver> resolver = SetupFeatureResolver(EDITION_2023);
   ASSERT_OK(resolver);
@@ -952,6 +962,83 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidDefaultsTooEarly) {
       HasError(HasSubstr("No valid default found for edition 2_TEST_ONLY")));
 }
 
+TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumTooEarly) {
+  const FileDescriptor* file = ParseSchema(R"schema(
+    syntax = "proto2";
+    package test;
+    import "google/protobuf/descriptor.proto";
+
+    extend google.protobuf.FeatureSet {
+      optional Foo bar = 9999;
+    }
+    message Foo {
+      optional int32 int_field_feature = 12 [
+        targets = TARGET_TYPE_FIELD,
+        edition_defaults = { edition_enum: EDITION_2023, value: "1" }
+      ];
+    }
+  )schema");
+  ASSERT_NE(file, nullptr);
+
+  const FieldDescriptor* ext = file->extension(0);
+  EXPECT_THAT(
+      FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_1_TEST_ONLY,
+                                       EDITION_99997_TEST_ONLY),
+      HasError(HasSubstr("No valid default found for edition 1_TEST_ONLY")));
+}
+
+TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumCovered) {
+  const FileDescriptor* file = ParseSchema(R"schema(
+    syntax = "proto2";
+    package test;
+    import "google/protobuf/descriptor.proto";
+
+    extend google.protobuf.FeatureSet {
+      optional Foo bar = 9999;
+    }
+    message Foo {
+      optional int32 int_file_feature = 1 [
+        targets = TARGET_TYPE_FIELD,
+        edition_defaults = { edition_enum: EDITION_99998_TEST_ONLY, value: "2" },
+        edition_defaults = { edition_enum: EDITION_2023, value: "1" }
+      ];
+    }
+  )schema");
+  ASSERT_NE(file, nullptr);
+
+  const FieldDescriptor* ext = file->extension(0);
+  auto defaults = FeatureResolver::CompileDefaults(
+      feature_set_, {ext}, EDITION_99997_TEST_ONLY, EDITION_99999_TEST_ONLY);
+  ASSERT_OK(defaults);
+
+  EXPECT_THAT(*defaults, EqualsProto(R"pb(
+    minimum_edition_enum: EDITION_99997_TEST_ONLY
+    maximum_edition_enum: EDITION_99999_TEST_ONLY
+    defaults {
+      edition_enum: EDITION_2023
+      features {
+        field_presence: EXPLICIT
+        enum_type: OPEN
+        repeated_field_encoding: PACKED
+        message_encoding: LENGTH_PREFIXED
+        json_format: ALLOW
+        [pb.test] { int_file_feature: 1 }
+      }
+    }
+    defaults {
+      edition_enum: EDITION_99998_TEST_ONLY
+      features {
+        field_presence: EXPLICIT
+        enum_type: OPEN
+        repeated_field_encoding: PACKED
+        message_encoding: LENGTH_PREFIXED
+        json_format: ALLOW
+        [pb.test] { int_file_feature: 2 }
+      }
+    }
+  )pb"));
+}
+
 }  // namespace
 }  // namespace protobuf
 }  // namespace google

src/google/protobuf/unittest_features.proto

@@ -49,10 +49,6 @@ message TestMessage {
   }
 }
 
-extend google.protobuf.FeatureSet {
-  optional TestInvalidFeatures test_invalid = 9996;
-}
-
 message TestFeatures {
   optional int32 int_file_feature = 1 [
     retention = RETENTION_RUNTIME,
@@ -201,11 +197,3 @@ message TestFeatures {
     edition_defaults = { edition_enum: EDITION_2023, value: "'2023'" }
   ];
 }
-
-message TestInvalidFeatures {
-  repeated int32 repeated_feature = 1 [
-    retention = RETENTION_RUNTIME,
-    targets = TARGET_TYPE_FIELD,
-    edition_defaults = { edition: "2023", value: "3" }
-  ];
-}

src/google/protobuf/unittest_invalid_features.proto

@@ -0,0 +1,47 @@
+// Protocol Buffers - Google's data interchange format
+// Copyright 2023 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+syntax = "proto2";
+
+package pb;
+
+import "google/protobuf/descriptor.proto";
+
+extend google.protobuf.FeatureSet {
+  optional TestInvalidFeatures test_invalid = 9996;
+}
+
+message TestInvalidFeatures {
+  repeated int32 repeated_feature = 1 [
+    retention = RETENTION_RUNTIME,
+    targets = TARGET_TYPE_FIELD,
+    edition_defaults = { edition: "2023", value: "3" }
+  ];
+}