Fix type resolver to preserve explicit set feature values.

PiperOrigin-RevId: 708402850

src/google/protobuf/util/BUILD.bazel

@@ -236,11 +236,14 @@ cc_test(
         "//src/google/protobuf:any_cc_proto",
         "//src/google/protobuf:cc_test_protos",
         "//src/google/protobuf:descriptor_legacy",
+        "//src/google/protobuf:test_textproto",
         "//src/google/protobuf:test_util",
         "//src/google/protobuf:type_cc_proto",
         "//src/google/protobuf:wrappers_cc_proto",
         "//src/google/protobuf/testing",
         "//src/google/protobuf/testing:file",
+        "@com_google_absl//absl/log:absl_check",
+        "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
     ],

src/google/protobuf/util/type_resolver_util.cc

@@ -21,7 +21,6 @@
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
 #include "absl/strings/strip.h"
-#include "google/protobuf/descriptor_legacy.h"
 #include "google/protobuf/io/strtod.h"
 #include "google/protobuf/util/type_resolver.h"
 
@@ -217,7 +216,8 @@ std::string GetTypeUrl(absl::string_view url_prefix, const T& descriptor) {
 }
 
 void ConvertFieldDescriptor(absl::string_view url_prefix,
-                            const FieldDescriptor& descriptor, Field* field) {
+                            const FieldDescriptor& descriptor,
+                            const FieldDescriptorProto& proto, Field* field) {
   field->set_kind(static_cast<Field::Kind>(descriptor.type()));
   switch (descriptor.label()) {
     case FieldDescriptor::LABEL_OPTIONAL:
@@ -249,24 +249,28 @@ void ConvertFieldDescriptor(absl::string_view url_prefix,
     field->set_packed(true);
   }
 
-  ConvertFieldOptions(descriptor.options(), *field->mutable_options());
+  ConvertFieldOptions(proto.options(), *field->mutable_options());
 }
 
-Syntax ConvertSyntax(Edition edition) {
+Syntax ConvertSyntax(absl::string_view syntax) {
-  switch (edition) {
+  if (syntax == "proto2" || syntax.empty()) {
-    case Edition::EDITION_PROTO2:
     return Syntax::SYNTAX_PROTO2;
-    case Edition::EDITION_PROTO3:
+  }
+  if (syntax == "proto3") {
     return Syntax::SYNTAX_PROTO3;
-    default:
-      return Syntax::SYNTAX_EDITIONS;
   }
+
+  return Syntax::SYNTAX_EDITIONS;
 }
 
-void ConvertEnumDescriptor(const EnumDescriptor& descriptor, Enum* enum_type) {
+void ConvertEnumDescriptor(const EnumDescriptor& descriptor,
+                           const FileDescriptorProto& file,
+                           const EnumDescriptorProto& proto, Enum* enum_type) {
   enum_type->Clear();
-  enum_type->set_syntax(
+  enum_type->set_syntax(ConvertSyntax(file.syntax()));
-      ConvertSyntax(FileDescriptorLegacy(descriptor.file()).edition()));
+  if (enum_type->syntax() == Syntax::SYNTAX_EDITIONS) {
+    enum_type->set_edition(absl::StrCat(file.edition()));
+  }
 
   enum_type->set_name(descriptor.full_name());
   enum_type->mutable_source_context()->set_file_name(descriptor.file()->name());
@@ -276,28 +280,32 @@ void ConvertEnumDescriptor(const EnumDescriptor& descriptor, Enum* enum_type) {
     value->set_name(value_descriptor.name());
     value->set_number(value_descriptor.number());
 
-    ConvertEnumValueOptions(value_descriptor.options(),
+    ConvertEnumValueOptions(proto.value(i).options(),
                             *value->mutable_options());
   }
 
-  ConvertEnumOptions(descriptor.options(), *enum_type->mutable_options());
+  ConvertEnumOptions(proto.options(), *enum_type->mutable_options());
 }
 
 void ConvertDescriptor(absl::string_view url_prefix,
-                       const Descriptor& descriptor, Type* type) {
+                       const Descriptor& descriptor,
+                       const FileDescriptorProto& file,
+                       const DescriptorProto& proto, Type* type) {
   type->Clear();
   type->set_name(descriptor.full_name());
-  type->set_syntax(
+  type->set_syntax(ConvertSyntax(file.syntax()));
-      ConvertSyntax(FileDescriptorLegacy(descriptor.file()).edition()));
+  if (type->syntax() == Syntax::SYNTAX_EDITIONS) {
+    type->set_edition(absl::StrCat(file.edition()));
+  }
   for (int i = 0; i < descriptor.field_count(); ++i) {
-    ConvertFieldDescriptor(url_prefix, *descriptor.field(i),
+    ConvertFieldDescriptor(url_prefix, *descriptor.field(i), proto.field(i),
                            type->add_fields());
   }
   for (int i = 0; i < descriptor.oneof_decl_count(); ++i) {
     type->add_oneofs(descriptor.oneof_decl(i)->name());
   }
   type->mutable_source_context()->set_file_name(descriptor.file()->name());
-  ConvertMessageOptions(descriptor.options(), *type->mutable_options());
+  ConvertMessageOptions(proto.options(), *type->mutable_options());
 }
 
 class DescriptorPoolTypeResolver : public TypeResolver {
@@ -319,7 +327,7 @@ class DescriptorPoolTypeResolver : public TypeResolver {
       return absl::NotFoundError(
           absl::StrCat("Invalid type URL, unknown type: ", type_name));
     }
-    ConvertDescriptor(url_prefix_, *descriptor, type);
+    *type = ConvertDescriptorToType(url_prefix_, *descriptor);
     return absl::Status();
   }
 
@@ -336,7 +344,7 @@ class DescriptorPoolTypeResolver : public TypeResolver {
       return absl::InvalidArgumentError(
           absl::StrCat("Invalid type URL, unknown type: ", type_name));
     }
-    ConvertEnumDescriptor(*descriptor, enum_type);
+    *enum_type = ConvertDescriptorToType(*descriptor);
     return absl::Status();
   }
 
@@ -369,14 +377,21 @@ TypeResolver* NewTypeResolverForDescriptorPool(absl::string_view url_prefix,
 Type ConvertDescriptorToType(absl::string_view url_prefix,
                              const Descriptor& descriptor) {
   Type type;
-  ConvertDescriptor(url_prefix, descriptor, &type);
+  FileDescriptorProto proto;
+  descriptor.file()->CopyHeadingTo(&proto);
+  descriptor.CopyTo(proto.add_message_type());
+  ConvertDescriptor(url_prefix, descriptor, proto, proto.message_type(0),
+                    &type);
   return type;
 }
 
 // Performs a direct conversion from an enum descriptor to a type proto.
 Enum ConvertDescriptorToType(const EnumDescriptor& descriptor) {
   Enum enum_type;
-  ConvertEnumDescriptor(descriptor, &enum_type);
+  FileDescriptorProto proto;
+  descriptor.file()->CopyHeadingTo(&proto);
+  descriptor.CopyTo(proto.add_enum_type());
+  ConvertEnumDescriptor(descriptor, proto, proto.enum_type(0), &enum_type);
   return enum_type;
 }
 

src/google/protobuf/util/type_resolver_util_test.cc

@@ -8,25 +8,24 @@
 #include "google/protobuf/util/type_resolver_util.h"
 
 #include <cstdint>
-#include <limits>
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "google/protobuf/any.pb.h"
 #include "google/protobuf/type.pb.h"
 #include "google/protobuf/wrappers.pb.h"
 #include "google/protobuf/descriptor.pb.h"
-#include "google/protobuf/util/type_resolver.h"
-#include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include "absl/log/absl_check.h"
+#include "absl/strings/string_view.h"
 #include "google/protobuf/descriptor.h"
-#include "google/protobuf/descriptor_legacy.h"
 #include "google/protobuf/util/json_format_proto3.pb.h"
 #include "google/protobuf/map_unittest.pb.h"
+#include "google/protobuf/test_textproto.h"
 #include "google/protobuf/unittest.pb.h"
 #include "google/protobuf/unittest_custom_options.pb.h"
 #include "google/protobuf/unittest_import.pb.h"
+#include "google/protobuf/util/type_resolver.h"
 
 namespace google {
 namespace protobuf {
@@ -421,18 +420,8 @@ class DescriptorPoolTypeResolverSyntaxTest : public testing::Test {
   DescriptorPoolTypeResolverSyntaxTest()
       : resolver_(NewTypeResolverForDescriptorPool(kUrlPrefix, &pool_)) {}
 
-  const FileDescriptor* BuildFile(
+  const FileDescriptor* BuildFile(absl::string_view file_contents) {
-      absl::string_view syntax,
+    FileDescriptorProto proto = ParseTextOrDie(file_contents);
-      absl::optional<Edition> edition = absl::nullopt) {
-    FileDescriptorProto proto;
-    proto.set_package("test");
-    proto.set_name("foo");
-    proto.set_syntax(syntax);
-    if (edition.has_value()) {
-      proto.set_edition(*edition);
-    }
-    DescriptorProto* message = proto.add_message_type();
-    message->set_name("MyMessage");
     const FileDescriptor* file = pool_.BuildFile(proto);
     ABSL_CHECK(file != nullptr);
     return file;
@@ -443,8 +432,12 @@ class DescriptorPoolTypeResolverSyntaxTest : public testing::Test {
 };
 
 TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto2) {
-  const FileDescriptor* file = BuildFile("proto2");
+  BuildFile(R"pb(
-  ASSERT_EQ(FileDescriptorLegacy(file).edition(), Edition::EDITION_PROTO2);
+    package: "test"
+    name: "foo"
+    syntax: "proto2"
+    message_type { name: "MyMessage" }
+  )pb");
 
   Type type;
   ASSERT_TRUE(
@@ -454,8 +447,12 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto2) {
 }
 
 TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto3) {
-  const FileDescriptor* file = BuildFile("proto3");
+  BuildFile(R"pb(
-  ASSERT_EQ(FileDescriptorLegacy(file).edition(), Edition::EDITION_PROTO3);
+    package: "test"
+    name: "foo"
+    syntax: "proto3"
+    message_type { name: "MyMessage" }
+  )pb");
 
   Type type;
   ASSERT_TRUE(
@@ -464,6 +461,94 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxProto3) {
   EXPECT_EQ(type.edition(), "");
 }
 
+TEST_F(DescriptorPoolTypeResolverSyntaxTest, SyntaxEditions) {
+  BuildFile(R"pb(
+    package: "test"
+    name: "foo"
+    syntax: "editions"
+    edition: EDITION_2023
+    message_type { name: "MyMessage" }
+  )pb");
+
+  Type type;
+  ASSERT_TRUE(
+      resolver_->ResolveMessageType(GetTypeUrl("test.MyMessage"), &type).ok());
+  EXPECT_EQ(type.syntax(), Syntax::SYNTAX_EDITIONS);
+  EXPECT_EQ(type.edition(), "2023");
+}
+
+TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsFieldFeatures) {
+  BuildFile(R"pb(
+    package: "test"
+    name: "foo"
+    syntax: "editions"
+    edition: EDITION_2023
+    message_type {
+      name: "MyMessage"
+      field {
+        name: "field"
+        number: 1
+        type: TYPE_BYTES
+        options {
+          features {
+            [pb.cpp] { string_type: CORD }
+          }
+        }
+      }
+    }
+  )pb");
+
+  Type type;
+  ASSERT_TRUE(
+      resolver_->ResolveMessageType(GetTypeUrl("test.MyMessage"), &type).ok());
+  ASSERT_EQ(type.fields_size(), 1);
+  EXPECT_THAT(type.fields(0), EqualsProto(R"pb(
+                kind: TYPE_BYTES
+                cardinality: CARDINALITY_OPTIONAL
+                number: 1
+                name: "field"
+                options {
+                  name: "features"
+                  value {
+                    [type.googleapis.com/google.protobuf.FeatureSet] {
+                      [pb.cpp] { string_type: CORD }
+                    }
+                  }
+                }
+                json_name: "field"
+              )pb"));
+}
+
+TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsEnumFeatures) {
+  BuildFile(R"pb(
+    package: "test"
+    name: "foo"
+    syntax: "editions"
+    edition: EDITION_2023
+    enum_type {
+      name: "MyEnum"
+      value: { name: "FOO" number: 1 }
+      options { features { enum_type: CLOSED } }
+    }
+  )pb");
+
+  Enum enm;
+  ASSERT_TRUE(resolver_->ResolveEnumType(GetTypeUrl("test.MyEnum"), &enm).ok());
+  EXPECT_THAT(
+      enm, EqualsProto(R"pb(
+        name: "test.MyEnum"
+        enumvalue { name: "FOO" number: 1 }
+        options {
+          name: "features"
+          value {
+            [type.googleapis.com/google.protobuf.FeatureSet] { enum_type: CLOSED }
+          }
+        }
+        source_context { file_name: "foo" }
+        syntax: SYNTAX_EDITIONS
+        edition: "2023"
+      )pb"));
+}
 
 TEST(ConvertDescriptorToTypeTest, TestAllTypes) {
   Type type = ConvertDescriptorToType(