Skip to content

Commit c7030f4

Browse files
mkruskal-googlecopybara-github
authored andcommitted
Preserve features in type resolver
While we previously preserved unresolved features set directly on message and enum types and the edition specified in the file, and features set at the file or nesting scopes were discarded. This throws away important semantic information, resulting in incorrect Type and Enum objects under editions. This change solves the problem by partially resolving features down to the scope returned. Users will still need to figure out the defaults for each edition on their own, but these can now be converted back into descriptors and built correctly. PiperOrigin-RevId: 791393429
1 parent 8a775f4 commit c7030f4

File tree

2 files changed

+124
-0
lines changed

2 files changed

+124
-0
lines changed

src/google/protobuf/util/type_resolver_util.cc

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "absl/strings/str_cat.h"
2222
#include "absl/strings/string_view.h"
2323
#include "absl/strings/strip.h"
24+
#include "google/protobuf/descriptor.h"
2425
#include "google/protobuf/io/strtod.h"
2526
#include "google/protobuf/util/type_resolver.h"
2627

@@ -362,6 +363,30 @@ class DescriptorPoolTypeResolver : public TypeResolver {
362363
const DescriptorPool* pool_;
363364
};
364365

366+
template <typename DescriptorT, typename DescriptorProtoT>
367+
void PartiallyResolveFeatures(const FileDescriptorProto& file,
368+
const DescriptorT& descriptor,
369+
DescriptorProtoT& proto) {
370+
FeatureSet features = file.options().features();
371+
std::vector<FeatureSet> nested_features;
372+
const Descriptor* parent = descriptor.containing_type();
373+
while (parent != nullptr) {
374+
DescriptorProto parent_proto;
375+
parent->CopyHeadingTo(&parent_proto);
376+
if (parent_proto.options().has_features()) {
377+
nested_features.push_back(parent_proto.options().features());
378+
}
379+
parent = parent->containing_type();
380+
}
381+
for (int i = nested_features.size() - 1; i >= 0; --i) {
382+
features.MergeFrom(nested_features[i]);
383+
}
384+
if (features.ByteSizeLong() > 0) {
385+
features.MergeFrom(proto.options().features());
386+
*proto.mutable_options()->mutable_features() = features;
387+
}
388+
}
389+
365390
} // namespace
366391

367392
TypeResolver* NewTypeResolverForDescriptorPool(absl::string_view url_prefix,
@@ -376,6 +401,7 @@ Type ConvertDescriptorToType(absl::string_view url_prefix,
376401
FileDescriptorProto proto;
377402
descriptor.file()->CopyHeadingTo(&proto);
378403
descriptor.CopyTo(proto.add_message_type());
404+
PartiallyResolveFeatures(proto, descriptor, *proto.mutable_message_type(0));
379405
ConvertDescriptor(url_prefix, descriptor, proto, proto.message_type(0),
380406
&type);
381407
return type;
@@ -387,6 +413,7 @@ Enum ConvertDescriptorToType(const EnumDescriptor& descriptor) {
387413
FileDescriptorProto proto;
388414
descriptor.file()->CopyHeadingTo(&proto);
389415
descriptor.CopyTo(proto.add_enum_type());
416+
PartiallyResolveFeatures(proto, descriptor, *proto.mutable_enum_type(0));
390417
ConvertEnumDescriptor(descriptor, proto, proto.enum_type(0), &enum_type);
391418
return enum_type;
392419
}

src/google/protobuf/util/type_resolver_util_test.cc

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,62 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsFieldFeatures) {
520520
)pb"));
521521
}
522522

523+
TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsInheritedMessageFeatures) {
524+
ASSERT_NE(BuildFile(R"pb(
525+
package: "test"
526+
name: "foo"
527+
syntax: "editions"
528+
edition: EDITION_2023
529+
options {
530+
features {
531+
repeated_field_encoding: EXPANDED
532+
[pb.cpp] { string_type: CORD }
533+
}
534+
}
535+
message_type {
536+
name: "Parent"
537+
options {
538+
features {
539+
field_presence: IMPLICIT
540+
[pb.cpp] { string_type: VIEW }
541+
}
542+
}
543+
nested_type {
544+
name: "MyMessage"
545+
field { name: "field" number: 1 type: TYPE_BYTES }
546+
}
547+
}
548+
)pb"),
549+
nullptr);
550+
551+
Type type;
552+
ASSERT_TRUE(
553+
resolver_->ResolveMessageType(GetTypeUrl("test.Parent.MyMessage"), &type)
554+
.ok());
555+
EXPECT_THAT(type, EqualsProto(R"pb(
556+
name: "test.Parent.MyMessage"
557+
fields {
558+
kind: TYPE_BYTES
559+
cardinality: CARDINALITY_OPTIONAL
560+
number: 1
561+
name: "field"
562+
json_name: "field"
563+
}
564+
options {
565+
name: "features"
566+
value {
567+
[type.googleapis.com/google.protobuf.FeatureSet] {
568+
field_presence: IMPLICIT
569+
repeated_field_encoding: EXPANDED
570+
[pb.cpp] { string_type: VIEW }
571+
}
572+
}
573+
}
574+
source_context { file_name: "foo" }
575+
syntax: SYNTAX_EDITIONS
576+
edition: "2023")pb"));
577+
}
578+
523579
TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsEnumFeatures) {
524580
BuildFile(R"pb(
525581
package: "test"
@@ -551,6 +607,47 @@ TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsEnumFeatures) {
551607
)pb"));
552608
}
553609

610+
TEST_F(DescriptorPoolTypeResolverSyntaxTest, EditionsEnumInheritedFeatures) {
611+
ASSERT_NE(BuildFile(R"pb(
612+
package: "test"
613+
name: "foo"
614+
syntax: "editions"
615+
edition: EDITION_2023
616+
options { features { repeated_field_encoding: EXPANDED } }
617+
message_type {
618+
name: "Parent"
619+
options { features { field_presence: IMPLICIT } }
620+
enum_type {
621+
name: "MyEnum"
622+
options { features { enum_type: CLOSED } }
623+
value: { name: "FOO" number: 1 }
624+
}
625+
}
626+
)pb"),
627+
nullptr);
628+
629+
Enum enm;
630+
ASSERT_TRUE(
631+
resolver_->ResolveEnumType(GetTypeUrl("test.Parent.MyEnum"), &enm).ok());
632+
EXPECT_THAT(enm, EqualsProto(R"pb(
633+
name: "test.Parent.MyEnum"
634+
enumvalue { name: "FOO" number: 1 }
635+
options {
636+
name: "features"
637+
value {
638+
[type.googleapis.com/google.protobuf.FeatureSet] {
639+
field_presence: IMPLICIT
640+
enum_type: CLOSED
641+
repeated_field_encoding: EXPANDED
642+
}
643+
}
644+
}
645+
source_context { file_name: "foo" }
646+
syntax: SYNTAX_EDITIONS
647+
edition: "2023"
648+
)pb"));
649+
}
650+
554651
TEST(ConvertDescriptorToTypeTest, TestAllTypes) {
555652
Type type = ConvertDescriptorToType(
556653
kUrlPrefix, *proto2_unittest::TestAllTypes::GetDescriptor());

0 commit comments

Comments
 (0)