Skip to content

Commit

Permalink
Fix toproto behavior for groups and required under editions
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 619359463
  • Loading branch information
mkruskal-google authored and copybara-github committed Mar 27, 2024
1 parent 6c45efd commit 7662fdd
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 2 deletions.
21 changes: 21 additions & 0 deletions upb/util/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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 = [
Expand Down
19 changes: 17 additions & 2 deletions upb/util/def_to_proto.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
#include <inttypes.h>
#include <math.h>

#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"
Expand Down Expand Up @@ -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(
Expand Down
8 changes: 8 additions & 0 deletions upb/util/def_to_proto_editions_test.proto
Original file line number Diff line number Diff line change
@@ -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];
}
27 changes: 27 additions & 0 deletions upb/util/def_to_proto_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,22 @@

#include "upb/util/def_to_proto.h"

#include <cstddef>
#include <memory>
#include <string>

#include "google/protobuf/descriptor.pb.h"
#include "google/protobuf/descriptor.upb.h"
#include "google/protobuf/descriptor.upbdefs.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#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"

Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 7662fdd

Please sign in to comment.