Chiebot-Mirror
/
protobuf
mirror of https://github.com/protocolbuffers/protobuf.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Change the ids used the unset field tracking to be Message+FieldDescriptor.

Browse Source 
The absolute address of a member is unstable and can be invalidated by changes to other
fields. For example, for split fields.

PiperOrigin-RevId: 596072639
pull/15264/head
Protobuf Team Bot 1 year ago committed by Copybara-Service
parent d1b328ace3
commit 7b2e9aac80
6 changed files with 82 additions and 124 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 1
      src/google/protobuf/message.h
  2. 36
      src/google/protobuf/text_format.cc
  3. 20
      src/google/protobuf/text_format.h
  4. 120
      src/google/protobuf/text_format_unittest.cc
  5. 6
      src/google/protobuf/util/message_differencer.cc
  6. 23
      src/google/protobuf/util/message_differencer_unittest.cc

1
src/google/protobuf/message.h
Unescape View File

@ -977,7 +977,6 @@ class PROTOBUF_EXPORT Reflection final {
return schema_.IsSplit(field);
}
friend class google::protobuf::TextFormat;
friend class FastReflectionBase;
friend class FastReflectionMessageMutator;
friend bool internal::IsDescendant(Message& root, const Message& message);

36
src/google/protobuf/text_format.cc
Unescape View File

@ -297,12 +297,9 @@ const Descriptor* DefaultFinderFindAnyType(const Message& message,
}
} // namespace
const void* TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress(
const Message& message, const Reflection& reflection,
const FieldDescriptor& fd) {
// reflection->GetRaw() is a simple cast for any non-repeated type, so for
// simplicity we just pass in char as the template argument.
return &reflection.GetRaw<char>(message, &fd);
auto TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldId(
const Message& message, const FieldDescriptor& fd) -> Id {
return {&message, &fd};
}
// ===========================================================================
@ -845,20 +842,19 @@ class TextFormat::Parser::ParserImpl {
// the message and the new value are the default. If the existing field value is
// not the default, setting it to the default should not be treated as a no-op.
// The pointer of this is kept in no_op_fields_ for bookkeeping.
#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \
if (field->is_repeated()) { \
reflection->Add##CPPTYPE(message, field, VALUE); \
} else { \
if (no_op_fields_ && !field->has_presence() && \
field->default_value_##CPPTYPELCASE() == \
reflection->Get##CPPTYPE(*message, field) && \
field->default_value_##CPPTYPELCASE() == VALUE) { \
no_op_fields_->addresses_.insert( \
UnsetFieldsMetadata::GetUnsetFieldAddress(*message, *reflection, \
*field)); \
} else { \
reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \
} \
#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \
if (field->is_repeated()) { \
reflection->Add##CPPTYPE(message, field, VALUE); \
} else { \
if (no_op_fields_ && !field->has_presence() && \
field->default_value_##CPPTYPELCASE() == \
reflection->Get##CPPTYPE(*message, field) && \
field->default_value_##CPPTYPELCASE() == VALUE) { \
no_op_fields_->ids_.insert( \
UnsetFieldsMetadata::GetUnsetFieldId(*message, *field)); \
} else { \
reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \
} \
}
switch (field->cpp_type()) {

20
src/google/protobuf/text_format.h
Unescape View File

@ -726,25 +726,25 @@ class PROTOBUF_EXPORT TextFormat {
// the maximum allowed nesting of proto messages.
void SetRecursionLimit(int limit) { recursion_limit_ = limit; }
// Uniquely addresses fields in a message that was explicitly unset in
// Metadata representing all the fields that were explicitly unset in
// textproto. Example:
// "some_int_field: 0"
// where some_int_field is non-optional.
// where some_int_field has implicit presence.
//
// This class should only be used to pass data between the text_format
// parser and the MessageDifferencer.
// This class should only be used to pass data between TextFormat and the
// MessageDifferencer.
class UnsetFieldsMetadata {
public:
UnsetFieldsMetadata() = default;
private:
// Return a pointer to the unset field in the given message.
static const void* GetUnsetFieldAddress(const Message& message,
const Reflection& reflection,
const FieldDescriptor& fd);
using Id = std::pair<const Message*, const FieldDescriptor*>;
// Return an id representing the unset field in the given message.
static Id GetUnsetFieldId(const Message& message,
const FieldDescriptor& fd);
// List of addresses of explicitly unset proto fields.
absl::flat_hash_set<const void*> addresses_;
// List of ids of explicitly unset proto fields.
absl::flat_hash_set<Id> ids_;
friend class ::google::protobuf::internal::
UnsetFieldsMetadataMessageDifferencerTestUtil;

120
src/google/protobuf/text_format_unittest.cc
Unescape View File

@ -44,6 +44,7 @@
#include "google/protobuf/test_util.h"
#include "google/protobuf/test_util2.h"
#include "google/protobuf/unittest.pb.h"
#include "google/protobuf/unittest.pb.h"
#include "google/protobuf/unittest_mset.pb.h"
#include "google/protobuf/unittest_mset_wire_format.pb.h"
#include "google/protobuf/unittest_proto3.pb.h"
@ -59,21 +60,13 @@ namespace protobuf {
namespace internal {
class UnsetFieldsMetadataTextFormatTestUtil {
public:
static std::vector<const void*> GetRawAddresses(
static const auto& GetRawIds(
const TextFormat::Parser::UnsetFieldsMetadata& metadata) {
return std::vector<const void*>(metadata.addresses_.begin(),
metadata.addresses_.end());
}
static const void* GetAddress(const Message& message,
const Reflection& reflection,
const FieldDescriptor& fd) {
return TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress(
message, reflection, fd);
return metadata.ids_;
}
static int32_t NumFields(
const TextFormat::Parser::UnsetFieldsMetadata& metadata) {
return metadata.addresses_.size();
static auto GetId(const Message& message, absl::string_view field) {
return TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldId(
message, *message.GetDescriptor()->FindFieldByName(field));
}
};
} // namespace internal
@ -85,6 +78,7 @@ using ::google::protobuf::internal::kDebugStringSilentMarker;
using ::google::protobuf::internal::UnsetFieldsMetadataTextFormatTestUtil;
using ::testing::AllOf;
using ::testing::HasSubstr;
using ::testing::UnorderedElementsAre;
// A basic string with different escapable characters for testing.
constexpr absl::string_view kEscapeTestString =
@ -1029,14 +1023,15 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
TextFormat::Parser parser;
TextFormat::Parser::UnsetFieldsMetadata no_op_fields;
parser.OutputNoOpFields(&no_op_fields);
using Peer = UnsetFieldsMetadataTextFormatTestUtil;
{
no_op_fields = {};
const absl::string_view singular_int_parse_string = "optional_int32: 0";
EXPECT_TRUE(TextFormat::ParseFromString(singular_int_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(singular_int_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_int32")));
}
{
no_op_fields = {};
@ -1044,8 +1039,8 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(singular_bool_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(singular_bool_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_bool")));
}
{
no_op_fields = {};
@ -1054,8 +1049,8 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(singular_string_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(singular_string_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_string")));
}
{
no_op_fields = {};
@ -1064,8 +1059,9 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(nested_message_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(nested_message_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(
Peer::GetId(proto.optional_nested_message(), "bb")));
}
{
no_op_fields = {};
@ -1074,8 +1070,7 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(nested_message_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(nested_message_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
0);
EXPECT_THAT(Peer::GetRawIds(no_op_fields), UnorderedElementsAre());
}
{
no_op_fields = {};
@ -1084,8 +1079,9 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(foreign_message_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(foreign_message_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(
Peer::GetId(proto.optional_foreign_message(), "c")));
}
{
no_op_fields = {};
@ -1093,8 +1089,9 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
"optional_nested_enum: ZERO ";
EXPECT_TRUE(TextFormat::ParseFromString(nested_enum_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(nested_enum_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(
Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_nested_enum")));
}
{
no_op_fields = {};
@ -1102,8 +1099,9 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
"optional_foreign_enum: FOREIGN_ZERO ";
EXPECT_TRUE(TextFormat::ParseFromString(foreign_enum_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(foreign_enum_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(
Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_foreign_enum")));
}
{
no_op_fields = {};
@ -1111,16 +1109,17 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
"optional_string_piece: '' ";
EXPECT_TRUE(TextFormat::ParseFromString(string_piece_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(string_piece_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(
Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_string_piece")));
}
{
no_op_fields = {};
const absl::string_view cord_parse_string = "optional_cord: '' ";
EXPECT_TRUE(TextFormat::ParseFromString(cord_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(cord_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_cord")));
}
{
no_op_fields = {};
@ -1129,8 +1128,7 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_int32_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_int32_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
0);
EXPECT_THAT(Peer::GetRawIds(no_op_fields), UnorderedElementsAre());
}
{
no_op_fields = {};
@ -1139,8 +1137,7 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_bool_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_bool_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
0);
EXPECT_THAT(Peer::GetRawIds(no_op_fields), UnorderedElementsAre());
}
{
no_op_fields = {};
@ -1149,12 +1146,12 @@ TEST_F(TextFormatTest, PopulatesNoOpFields) {
EXPECT_TRUE(
TextFormat::ParseFromString(repeated_string_parse_string, &proto));
EXPECT_TRUE(parser.ParseFromString(repeated_string_parse_string, &proto));
EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
0);
EXPECT_THAT(Peer::GetRawIds(no_op_fields), UnorderedElementsAre());
}
}
TEST_F(TextFormatTest, FieldsPopulatedCorrectly) {
using Peer = UnsetFieldsMetadataTextFormatTestUtil;
proto3_unittest::TestAllTypes proto;
TextFormat::Parser parser;
TextFormat::Parser::UnsetFieldsMetadata no_op_fields;
@ -1167,18 +1164,10 @@ TEST_F(TextFormatTest, FieldsPopulatedCorrectly) {
optional_nested_message { bb: 0 }
)pb";
EXPECT_TRUE(parser.ParseFromString(parse_string, &proto));
ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
2);
std::vector<const void*> ptrs =
UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields);
EXPECT_EQ(*static_cast<const int32_t*>(ptrs[0]), 0);
EXPECT_EQ(*static_cast<const int32_t*>(ptrs[1]), 0);
proto.set_optional_int32(20);
proto.mutable_optional_nested_message()->set_bb(30);
const std::vector<int32_t> new_values{
*static_cast<const int32_t*>(ptrs[0]),
*static_cast<const int32_t*>(ptrs[1])};
EXPECT_THAT(new_values, testing::UnorderedElementsAre(20, 30));
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(
Peer::GetId(proto, "optional_int32"),
Peer::GetId(proto.optional_nested_message(), "bb")));
}
{
no_op_fields = {};
@ -1188,13 +1177,8 @@ TEST_F(TextFormatTest, FieldsPopulatedCorrectly) {
optional_nested_message { bb: 20 }
)pb";
EXPECT_TRUE(parser.ParseFromString(parse_string, &proto));
ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
std::vector<const void*> ptrs =
UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields);
EXPECT_EQ(*static_cast<const bool*>(ptrs[0]), false);
proto.set_optional_bool(true);
EXPECT_EQ(*static_cast<const bool*>(ptrs[0]), true);
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_bool")));
}
{
// The address returned by the field is a string_view, which is a separate
@ -1202,14 +1186,8 @@ TEST_F(TextFormatTest, FieldsPopulatedCorrectly) {
no_op_fields = {};
const absl::string_view parse_string = "optional_string: \"\"";
EXPECT_TRUE(parser.ParseFromString(parse_string, &proto));
ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
const Reflection* reflection = proto.GetReflection();
const FieldDescriptor* field = proto.GetDescriptor()->FindFieldByNumber(14);
EXPECT_EQ(
UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields)[0],
UnsetFieldsMetadataTextFormatTestUtil::GetAddress(proto, *reflection,
*field));
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_string")));
}
{
// The address returned by the field is a string_view, which is a separate
@ -1217,14 +1195,8 @@ TEST_F(TextFormatTest, FieldsPopulatedCorrectly) {
no_op_fields = {};
const absl::string_view parse_string = "optional_bytes: \"\"";
EXPECT_TRUE(parser.ParseFromString(parse_string, &proto));
ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields),
1);
const Reflection* reflection = proto.GetReflection();
const FieldDescriptor* field = proto.GetDescriptor()->FindFieldByNumber(15);
EXPECT_EQ(
UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields)[0],
UnsetFieldsMetadataTextFormatTestUtil::GetAddress(proto, *reflection,
*field));
EXPECT_THAT(Peer::GetRawIds(no_op_fields),
UnorderedElementsAre(Peer::GetId(proto, "optional_bytes")));
}
}

6
src/google/protobuf/util/message_differencer.cc
Unescape View File

@ -759,9 +759,9 @@ bool MessageDifferencer::ShouldCompareNoPresence(
const bool compare_no_presence_by_address =
!field2->is_repeated() && !field2->has_presence() &&
ValidMissingField(*field2) &&
require_no_presence_fields_.addresses_.contains(
TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress(
message1, reflection1, *field2));
require_no_presence_fields_.ids_.contains(
TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldId(message1,
*field2));
return compare_no_presence_by_address;
}

23
src/google/protobuf/util/message_differencer_unittest.cc
Unescape View File

@ -50,12 +50,9 @@ namespace internal {
class UnsetFieldsMetadataMessageDifferencerTestUtil {
public:
static void AddExplicitUnsetField(
const Message& message, const Reflection& reflection,
const FieldDescriptor& fd,
const Message& message, const FieldDescriptor& fd,
TextFormat::Parser::UnsetFieldsMetadata* metadata) {
metadata->addresses_.insert(
TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress(
message, reflection, fd));
metadata->ids_.insert(metadata->GetUnsetFieldId(message, fd));
}
};
} // namespace internal
@ -414,11 +411,10 @@ TEST(MessageDifferencerTest,
proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField();
proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField();
const Reflection* reflection1 = msg1.GetReflection();
const FieldDescriptor* fd1 = msg1.GetDescriptor()->FindFieldByNumber(1);
TextFormat::Parser::UnsetFieldsMetadata metadata;
UnsetFieldsMetadataMessageDifferencerTestUtil::AddExplicitUnsetField(
msg1, *reflection1, *fd1, &metadata);
msg1, *fd1, &metadata);
address_differencer.set_require_no_presence_fields(metadata);
EXPECT_TRUE(address_differencer.Compare(msg1, msg2));
@ -455,11 +451,10 @@ TEST(MessageDifferencerTest,
proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField();
proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField();
const Reflection* reflection1 = msg1.GetReflection();
const FieldDescriptor* fd1 = msg1.GetDescriptor()->FindFieldByNumber(4);
TextFormat::Parser::UnsetFieldsMetadata metadata;
UnsetFieldsMetadataMessageDifferencerTestUtil::AddExplicitUnsetField(
msg1, *reflection1, *fd1, &metadata);
msg1, *fd1, &metadata);
address_differencer.set_require_no_presence_fields(metadata);
EXPECT_TRUE(address_differencer.Compare(msg1, msg2));
@ -498,11 +493,10 @@ TEST(MessageDifferencerTest,
proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField();
proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField();
const Reflection* reflection1 = msg1.GetReflection();
const FieldDescriptor* fd1 = msg1.GetDescriptor()->FindFieldByNumber(5);
TextFormat::Parser::UnsetFieldsMetadata metadata;
UnsetFieldsMetadataMessageDifferencerTestUtil::AddExplicitUnsetField(
msg1, *reflection1, *fd1, &metadata);
msg1, *fd1, &metadata);
address_differencer.set_require_no_presence_fields(metadata);
EXPECT_TRUE(address_differencer.Compare(msg1, msg2));
@ -537,11 +531,10 @@ TEST(MessageDifferencerTest,
proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField();
proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField();
const Reflection* reflection1 = msg1.no_presence_nested().GetReflection();
const FieldDescriptor* fd1 = msg1.GetDescriptor()->FindFieldByNumber(1);
TextFormat::Parser::UnsetFieldsMetadata metadata;
UnsetFieldsMetadataMessageDifferencerTestUtil::AddExplicitUnsetField(
msg1.no_presence_nested(), *reflection1, *fd1, &metadata);
msg1.no_presence_nested(), *fd1, &metadata);
address_differencer.set_require_no_presence_fields(metadata);
EXPECT_TRUE(address_differencer.Compare(msg1, msg2));
@ -578,12 +571,10 @@ TEST(MessageDifferencerTest,
proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField();
proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField();
const Reflection* reflection1 =
msg1.no_presence_repeated_nested(0).GetReflection();
const FieldDescriptor* fd1 = msg1.GetDescriptor()->FindFieldByNumber(1);
TextFormat::Parser::UnsetFieldsMetadata metadata;
UnsetFieldsMetadataMessageDifferencerTestUtil::AddExplicitUnsetField(
msg1.no_presence_repeated_nested(0), *reflection1, *fd1, &metadata);
msg1.no_presence_repeated_nested(0), *fd1, &metadata);
address_differencer.set_require_no_presence_fields(metadata);
EXPECT_TRUE(address_differencer.Compare(msg1, msg2));

Write Preview
Loading…
Cancel
Save