From 7662fdd8edc529c326c1978b4dcb1cc0682aeaad Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Tue, 26 Mar 2024 17:21:01 -0700 Subject: [PATCH] Fix toproto behavior for groups and required under editions PiperOrigin-RevId: 619359463 --- upb/util/BUILD | 21 ++++++++++++++++++ upb/util/def_to_proto.c | 19 ++++++++++++++-- upb/util/def_to_proto_editions_test.proto | 8 +++++++ upb/util/def_to_proto_test.cc | 27 +++++++++++++++++++++++ 4 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 upb/util/def_to_proto_editions_test.proto diff --git a/upb/util/BUILD b/upb/util/BUILD index 0c1846394934..fe7e6c07e02f 100644 --- a/upb/util/BUILD +++ b/upb/util/BUILD @@ -18,6 +18,8 @@ cc_library( hdrs = ["def_to_proto.h"], visibility = ["//visibility:public"], deps = [ + "//upb:base", + "//upb:descriptor_upb_proto", "//upb:port", "//upb:reflection", "//upb/reflection:internal", @@ -64,11 +66,15 @@ cc_test( srcs = ["def_to_proto_test.cc"], deps = [ ":def_to_proto", + ":def_to_proto_editions_test_upb_proto", + ":def_to_proto_editions_test_upb_proto_reflection", ":def_to_proto_test_lib", ":def_to_proto_test_upb_proto", ":def_to_proto_test_upb_proto_reflection", "//:protobuf", "//src/google/protobuf/util:differencer", + "//upb:base", + "//upb:descriptor_upb_proto", "//upb:descriptor_upb_proto_reflection", "//upb:mem", "//upb:reflection", @@ -108,6 +114,21 @@ cc_library( ], ) +proto_library( + name = "def_to_proto_editions_test_proto", + srcs = ["def_to_proto_editions_test.proto"], +) + +upb_c_proto_library( + name = "def_to_proto_editions_test_upb_proto", + deps = ["def_to_proto_editions_test_proto"], +) + +upb_proto_reflection_library( + name = "def_to_proto_editions_test_upb_proto_reflection", + deps = ["def_to_proto_editions_test_proto"], +) + proto_library( name = "required_fields_test_proto", srcs = [ diff --git a/upb/util/def_to_proto.c b/upb/util/def_to_proto.c index 2c0ecbe48482..b7a6f42c78d3 100644 --- a/upb/util/def_to_proto.c +++ b/upb/util/def_to_proto.c @@ -10,7 +10,10 @@ #include #include +#include "google/protobuf/descriptor.upb.h" +#include "upb/base/descriptor_constants.h" #include "upb/port/vsnprintf_compat.h" +#include "upb/reflection/def.h" #include "upb/reflection/enum_reserved_range.h" #include "upb/reflection/extension_range.h" #include "upb/reflection/internal/field_def.h" @@ -218,8 +221,20 @@ static google_protobuf_FieldDescriptorProto* fielddef_toproto(upb_ToProto_Contex google_protobuf_FieldDescriptorProto_set_name(proto, strviewdup(ctx, upb_FieldDef_Name(f))); google_protobuf_FieldDescriptorProto_set_number(proto, upb_FieldDef_Number(f)); - google_protobuf_FieldDescriptorProto_set_label(proto, upb_FieldDef_Label(f)); - google_protobuf_FieldDescriptorProto_set_type(proto, upb_FieldDef_Type(f)); + + if (upb_FieldDef_IsRequired(f) && + upb_FileDef_Edition(upb_FieldDef_File(f)) >= UPB_DESC(EDITION_2023)) { + google_protobuf_FieldDescriptorProto_set_label( + proto, UPB_DESC(FieldDescriptorProto_LABEL_OPTIONAL)); + } else { + google_protobuf_FieldDescriptorProto_set_label(proto, upb_FieldDef_Label(f)); + } + if (upb_FieldDef_Type(f) == kUpb_FieldType_Group && + upb_FileDef_Edition(upb_FieldDef_File(f)) >= UPB_DESC(EDITION_2023)) { + google_protobuf_FieldDescriptorProto_set_type(proto, kUpb_FieldType_Message); + } else { + google_protobuf_FieldDescriptorProto_set_type(proto, upb_FieldDef_Type(f)); + } if (upb_FieldDef_HasJsonName(f)) { google_protobuf_FieldDescriptorProto_set_json_name( diff --git a/upb/util/def_to_proto_editions_test.proto b/upb/util/def_to_proto_editions_test.proto new file mode 100644 index 000000000000..91310d17c7bc --- /dev/null +++ b/upb/util/def_to_proto_editions_test.proto @@ -0,0 +1,8 @@ +edition = "2023"; + +package pkg; + +message EditionsMessage { + int32 required_field = 1 [features.field_presence = LEGACY_REQUIRED]; + EditionsMessage delimited_field = 2 [features.message_encoding = DELIMITED]; +} diff --git a/upb/util/def_to_proto_test.cc b/upb/util/def_to_proto_test.cc index 523efd2708bf..0c7b2038f4bc 100644 --- a/upb/util/def_to_proto_test.cc +++ b/upb/util/def_to_proto_test.cc @@ -7,18 +7,22 @@ #include "upb/util/def_to_proto.h" +#include #include #include #include "google/protobuf/descriptor.pb.h" +#include "google/protobuf/descriptor.upb.h" #include "google/protobuf/descriptor.upbdefs.h" #include #include #include "google/protobuf/dynamic_message.h" #include "google/protobuf/util/message_differencer.h" +#include "upb/base/string_view.h" #include "upb/mem/arena.hpp" #include "upb/reflection/def.hpp" #include "upb/test/parse_text_proto.h" +#include "upb/util/def_to_proto_editions_test.upbdefs.h" #include "upb/util/def_to_proto_test.h" #include "upb/util/def_to_proto_test.upbdefs.h" @@ -115,6 +119,29 @@ TEST(DefToProto, Test) { CheckFile(file, file_desc); } +// Verifies that editions don't leak out legacy feature APIs (e.g. TYPE_GROUP +// and LABEL_REQUIRED): +// serialized descriptor -> upb def -> serialized descriptor +TEST(DefToProto, TestEditionsLegacyFeatures) { + upb::Arena arena; + upb::DefPool defpool; + upb_StringView test_file_desc = + upb_util_def_to_proto_editions_test_proto_upbdefinit + .descriptor; + const auto* file = google_protobuf_FileDescriptorProto_parse( + test_file_desc.data, test_file_desc.size, arena.ptr()); + + size_t size; + const auto* messages = google_protobuf_FileDescriptorProto_message_type(file, &size); + ASSERT_EQ(size, 1); + const auto* fields = google_protobuf_DescriptorProto_field(messages[0], &size); + ASSERT_EQ(size, 2); + EXPECT_EQ(google_protobuf_FieldDescriptorProto_label(fields[0]), + google_protobuf_FieldDescriptorProto_LABEL_OPTIONAL); + EXPECT_EQ(google_protobuf_FieldDescriptorProto_type(fields[1]), + google_protobuf_FieldDescriptorProto_TYPE_MESSAGE); +} + // Like the previous test, but uses a message layout built at runtime. TEST(DefToProto, TestRuntimeReflection) { upb::Arena arena;