From 8c2416311f7b2c5410e620197baacf9d82b86ebd Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 8 Nov 2023 18:26:59 -0800 Subject: [PATCH] Compare explicit zeroes from prototext in partially. Right now, zeros are not allowed in explicit prototext comparison, frequently leading to very awkward manual checking. This tracks such explicit zeros, and ensures they remain 0. PiperOrigin-RevId: 580729313 --- src/google/protobuf/message.h | 2 + src/google/protobuf/text_format.cc | 48 ++-- src/google/protobuf/text_format.h | 43 +++- src/google/protobuf/text_format_unittest.cc | 169 ++++++++++++- .../protobuf/util/message_differencer.cc | 53 +++- .../protobuf/util/message_differencer.h | 15 ++ .../util/message_differencer_unittest.cc | 227 ++++++++++++++++++ .../message_differencer_unittest_proto3.proto | 3 + 8 files changed, 518 insertions(+), 42 deletions(-) diff --git a/src/google/protobuf/message.h b/src/google/protobuf/message.h index 8630cdc369f06..27a5772d6b740 100644 --- a/src/google/protobuf/message.h +++ b/src/google/protobuf/message.h @@ -135,6 +135,7 @@ class MapValueConstRef; class MapValueRef; class MapIterator; class MapReflectionTester; +class TextFormat; namespace internal { struct FuzzPeer; @@ -1010,6 +1011,7 @@ 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); diff --git a/src/google/protobuf/text_format.cc b/src/google/protobuf/text_format.cc index 9abd84772a10d..e29b07b32b22b 100644 --- a/src/google/protobuf/text_format.cc +++ b/src/google/protobuf/text_format.cc @@ -295,6 +295,14 @@ 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(message, &fd); +} + // =========================================================================== // Internal class for parsing an ASCII representation of a Protocol Message. // This class makes use of the Protocol Message compiler's tokenizer found @@ -331,7 +339,7 @@ class TextFormat::Parser::ParserImpl { bool allow_unknown_extension, bool allow_unknown_enum, bool allow_field_number, bool allow_relaxed_whitespace, bool allow_partial, int recursion_limit, - bool error_on_no_op_fields) + UnsetFieldsMetadata* no_op_fields) : error_collector_(error_collector), finder_(finder), parse_info_tree_(parse_info_tree), @@ -349,7 +357,7 @@ class TextFormat::Parser::ParserImpl { recursion_limit_(recursion_limit), had_silent_marker_(false), had_errors_(false), - error_on_no_op_fields_(error_on_no_op_fields) { + no_op_fields_(no_op_fields) { // For backwards-compatibility with proto1, we need to allow the 'f' suffix // for floats. tokenizer_.set_allow_f_after_float(true); @@ -834,19 +842,21 @@ class TextFormat::Parser::ParserImpl { // When checking for no-op operations, We verify that both the existing value in // 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. -#define SET_FIELD(CPPTYPE, CPPTYPELCASE, VALUE) \ - if (field->is_repeated()) { \ - reflection->Add##CPPTYPE(message, field, VALUE); \ - } else { \ - if (error_on_no_op_fields_ && !field->has_presence() && \ - field->default_value_##CPPTYPELCASE() == \ - reflection->Get##CPPTYPE(*message, field) && \ - field->default_value_##CPPTYPELCASE() == VALUE) { \ - ReportError("Input field " + field->full_name() + \ - " did not change resulting proto."); \ - } else { \ - reflection->Set##CPPTYPE(message, field, std::move(VALUE)); \ - } \ +// 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)); \ + } \ } switch (field->cpp_type()) { @@ -1429,7 +1439,7 @@ class TextFormat::Parser::ParserImpl { int recursion_limit_; bool had_silent_marker_; bool had_errors_; - bool error_on_no_op_fields_; + UnsetFieldsMetadata* no_op_fields_{}; }; @@ -1727,7 +1737,7 @@ bool TextFormat::Parser::Parse(io::ZeroCopyInputStream* input, allow_case_insensitive_field_, allow_unknown_field_, allow_unknown_extension_, allow_unknown_enum_, allow_field_number_, allow_relaxed_whitespace_, - allow_partial_, recursion_limit_, error_on_no_op_fields_); + allow_partial_, recursion_limit_, no_op_fields_); return MergeUsingImpl(input, output, &parser); } @@ -1752,7 +1762,7 @@ bool TextFormat::Parser::Merge(io::ZeroCopyInputStream* input, allow_case_insensitive_field_, allow_unknown_field_, allow_unknown_extension_, allow_unknown_enum_, allow_field_number_, allow_relaxed_whitespace_, - allow_partial_, recursion_limit_, error_on_no_op_fields_); + allow_partial_, recursion_limit_, no_op_fields_); return MergeUsingImpl(input, output, &parser); } @@ -1788,7 +1798,7 @@ bool TextFormat::Parser::ParseFieldValueFromString(absl::string_view input, allow_case_insensitive_field_, allow_unknown_field_, allow_unknown_extension_, allow_unknown_enum_, allow_field_number_, allow_relaxed_whitespace_, - allow_partial_, recursion_limit_, error_on_no_op_fields_); + allow_partial_, recursion_limit_, no_op_fields_); return parser.ParseField(field, output); } diff --git a/src/google/protobuf/text_format.h b/src/google/protobuf/text_format.h index f851c910f8272..e16ed985d0337 100644 --- a/src/google/protobuf/text_format.h +++ b/src/google/protobuf/text_format.h @@ -21,6 +21,7 @@ #include #include "absl/container/flat_hash_map.h" +#include "absl/container/flat_hash_set.h" #include "absl/strings/cord.h" #include "absl/strings/string_view.h" #include "google/protobuf/descriptor.h" @@ -81,6 +82,9 @@ PROTOBUF_EXPORT enum class Option; // Converts a protobuf message to a string with redaction enabled. PROTOBUF_EXPORT std::string StringifyMessage(const Message& message, Option option); + +class UnsetFieldsMetadataTextFormatTestUtil; +class UnsetFieldsMetadataMessageDifferencerTestUtil; } // namespace internal // This class implements protocol buffer text format, colloquially known as text @@ -719,11 +723,40 @@ class PROTOBUF_EXPORT TextFormat { // the maximum allowed nesting of proto messages. void SetRecursionLimit(int limit) { recursion_limit_ = limit; } - // If called, the parser will report an error if a parsed field had no + // Uniquely addresses fields in a message that was explicitly unset in + // textproto. Example: + // "some_int_field: 0" + // where some_int_field is non-optional. + // + // This class should only be used to pass data between the text_format + // parser 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); + + // List of addresses of explicitly unset proto fields. + absl::flat_hash_set addresses_; + + friend class ::google::protobuf::internal:: + UnsetFieldsMetadataMessageDifferencerTestUtil; + friend class ::google::protobuf::internal::UnsetFieldsMetadataTextFormatTestUtil; + friend class ::google::protobuf::util::MessageDifferencer; + friend class ::google::protobuf::TextFormat::Parser; + }; + + // If called, the parser will report the parsed fields that had no // effect on the resulting proto (for example, fields with no presence that - // were set to their default value). - void ErrorOnNoOpFields(bool return_error) { - error_on_no_op_fields_ = return_error; + // were set to their default value). These can be passed to the Partially() + // matcher as an indicator to explicitly check these fields are missing + // in the actual. + void OutputNoOpFields(UnsetFieldsMetadata* no_op_fields) { + no_op_fields_ = no_op_fields; } private: @@ -748,7 +781,7 @@ class PROTOBUF_EXPORT TextFormat { bool allow_relaxed_whitespace_; bool allow_singular_overwrites_; int recursion_limit_; - bool error_on_no_op_fields_ = false; + UnsetFieldsMetadata* no_op_fields_ = nullptr; }; diff --git a/src/google/protobuf/text_format_unittest.cc b/src/google/protobuf/text_format_unittest.cc index 9f60c9f3fdc3e..13fbf33905406 100644 --- a/src/google/protobuf/text_format_unittest.cc +++ b/src/google/protobuf/text_format_unittest.cc @@ -15,9 +15,11 @@ #include #include +#include #include #include #include +#include #include "google/protobuf/testing/file.h" #include "google/protobuf/testing/file.h" @@ -34,6 +36,7 @@ #include "absl/strings/str_format.h" #include "absl/strings/str_replace.h" #include "absl/strings/substitute.h" +#include "google/protobuf/descriptor.h" #include "google/protobuf/io/tokenizer.h" #include "google/protobuf/io/zero_copy_stream_impl.h" #include "google/protobuf/map_unittest.pb.h" @@ -52,10 +55,33 @@ namespace google { namespace protobuf { +namespace internal { +class UnsetFieldsMetadataTextFormatTestUtil { + public: + static std::vector GetRawAddresses( + const TextFormat::Parser::UnsetFieldsMetadata& metadata) { + return std::vector(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); + } + + static int32_t NumFields( + const TextFormat::Parser::UnsetFieldsMetadata& metadata) { + return metadata.addresses_.size(); + } +}; +} // namespace internal + // Can't use an anonymous namespace here due to brokenness of Tru64 compiler. namespace text_format_unittest { using ::google::protobuf::internal::kDebugStringSilentMarker; +using ::google::protobuf::internal::UnsetFieldsMetadataTextFormatTestUtil; using ::testing::AllOf; using ::testing::HasSubstr; @@ -997,86 +1023,207 @@ TEST_F(TextFormatTest, ParseUnknownEnumFieldProto3) { EXPECT_EQ(-2147483648, proto.repeated_nested_enum(3)); } -TEST_F(TextFormatTest, ErrorOnNoOpFieldsProto3) { +TEST_F(TextFormatTest, PopulatesNoOpFields) { proto3_unittest::TestAllTypes proto; TextFormat::Parser parser; - parser.ErrorOnNoOpFields(true); + TextFormat::Parser::UnsetFieldsMetadata no_op_fields; + parser.OutputNoOpFields(&no_op_fields); { + no_op_fields = {}; const absl::string_view singular_int_parse_string = "optional_int32: 0"; EXPECT_TRUE(TextFormat::ParseFromString(singular_int_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(singular_int_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(singular_int_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view singular_bool_parse_string = "optional_bool: false"; EXPECT_TRUE( TextFormat::ParseFromString(singular_bool_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(singular_bool_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(singular_bool_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view singular_string_parse_string = "optional_string: ''"; EXPECT_TRUE( TextFormat::ParseFromString(singular_string_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(singular_string_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(singular_string_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view nested_message_parse_string = "optional_nested_message { bb: 0 } "; EXPECT_TRUE( TextFormat::ParseFromString(nested_message_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(nested_message_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(nested_message_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); + } + { + no_op_fields = {}; + const absl::string_view nested_message_parse_string = + "optional_nested_message { bb: 1 } "; + 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); } { + no_op_fields = {}; const absl::string_view foreign_message_parse_string = "optional_foreign_message { c: 0 } "; EXPECT_TRUE( TextFormat::ParseFromString(foreign_message_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(foreign_message_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(foreign_message_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view nested_enum_parse_string = "optional_nested_enum: ZERO "; EXPECT_TRUE(TextFormat::ParseFromString(nested_enum_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(nested_enum_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(nested_enum_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view foreign_enum_parse_string = "optional_foreign_enum: FOREIGN_ZERO "; EXPECT_TRUE(TextFormat::ParseFromString(foreign_enum_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(foreign_enum_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(foreign_enum_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view string_piece_parse_string = "optional_string_piece: '' "; EXPECT_TRUE(TextFormat::ParseFromString(string_piece_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(string_piece_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(string_piece_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; const absl::string_view cord_parse_string = "optional_cord: '' "; EXPECT_TRUE(TextFormat::ParseFromString(cord_parse_string, &proto)); - EXPECT_FALSE(parser.ParseFromString(cord_parse_string, &proto)); + EXPECT_TRUE(parser.ParseFromString(cord_parse_string, &proto)); + EXPECT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); } { + no_op_fields = {}; // Sanity check that repeated fields work the same. const absl::string_view repeated_int32_parse_string = "repeated_int32: 0 "; 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); } { + no_op_fields = {}; const absl::string_view repeated_bool_parse_string = "repeated_bool: false "; 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); } { + no_op_fields = {}; const absl::string_view repeated_string_parse_string = "repeated_string: '' "; 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); + } +} + +TEST_F(TextFormatTest, FieldsPopulatedCorrectly) { + proto3_unittest::TestAllTypes proto; + TextFormat::Parser parser; + TextFormat::Parser::UnsetFieldsMetadata no_op_fields; + parser.OutputNoOpFields(&no_op_fields); + { + no_op_fields = {}; + const absl::string_view parse_string = R"pb( + optional_int32: 0 + optional_uint32: 10 + optional_nested_message { bb: 0 } + )pb"; + EXPECT_TRUE(parser.ParseFromString(parse_string, &proto)); + ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 2); + std::vector ptrs = + UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields); + EXPECT_EQ(*static_cast(ptrs[0]), 0); + EXPECT_EQ(*static_cast(ptrs[1]), 0); + proto.set_optional_int32(20); + proto.mutable_optional_nested_message()->set_bb(30); + const std::vector new_values{ + *static_cast(ptrs[0]), + *static_cast(ptrs[1])}; + EXPECT_THAT(new_values, testing::UnorderedElementsAre(20, 30)); + } + { + no_op_fields = {}; + const absl::string_view parse_string = R"pb( + optional_bool: false + optional_uint32: 10 + optional_nested_message { bb: 20 } + )pb"; + EXPECT_TRUE(parser.ParseFromString(parse_string, &proto)); + ASSERT_EQ(UnsetFieldsMetadataTextFormatTestUtil::NumFields(no_op_fields), + 1); + std::vector ptrs = + UnsetFieldsMetadataTextFormatTestUtil::GetRawAddresses(no_op_fields); + EXPECT_EQ(*static_cast(ptrs[0]), false); + proto.set_optional_bool(true); + EXPECT_EQ(*static_cast(ptrs[0]), true); + } + { + // The address returned by the field is a string_view, which is a separate + // alocation. Check address directly. + 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)); + } + { + // The address returned by the field is a string_view, which is a separate + // alocation. Check address directly. + 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)); } } diff --git a/src/google/protobuf/util/message_differencer.cc b/src/google/protobuf/util/message_differencer.cc index 853d5dac1f69d..df0311cd4c329 100644 --- a/src/google/protobuf/util/message_differencer.cc +++ b/src/google/protobuf/util/message_differencer.cc @@ -696,7 +696,7 @@ bool MessageDifferencer::CompareRequestedFieldsUsingSettings( // we are merely checking for a difference in field values, // rather than the addition or deletion of fields). std::vector fields_union = - CombineFields(message1_fields, FULL, message2_fields, FULL); + CombineFields(message1, message1_fields, FULL, message2_fields, FULL); return CompareWithFieldsInternal(message1, message2, unpacked_any, fields_union, fields_union, parent_fields); @@ -719,8 +719,8 @@ bool MessageDifferencer::CompareRequestedFieldsUsingSettings( // but only the intersection for message2. This way, any fields // only present in message2 will be ignored, but any fields only // present in message1 will be marked as a difference. - std::vector fields_intersection = - CombineFields(message1_fields, PARTIAL, message2_fields, PARTIAL); + std::vector fields_intersection = CombineFields( + message1, message1_fields, PARTIAL, message2_fields, PARTIAL); return CompareWithFieldsInternal(message1, message2, unpacked_any, message1_fields, fields_intersection, parent_fields); @@ -728,9 +728,48 @@ bool MessageDifferencer::CompareRequestedFieldsUsingSettings( } } +namespace { +bool ValidMissingField(const FieldDescriptor& f) { + switch (f.cpp_type()) { + case FieldDescriptor::CPPTYPE_INT32: + case FieldDescriptor::CPPTYPE_UINT32: + case FieldDescriptor::CPPTYPE_INT64: + case FieldDescriptor::CPPTYPE_UINT64: + case FieldDescriptor::CPPTYPE_FLOAT: + case FieldDescriptor::CPPTYPE_DOUBLE: + case FieldDescriptor::CPPTYPE_STRING: + case FieldDescriptor::CPPTYPE_BOOL: + case FieldDescriptor::CPPTYPE_ENUM: + return true; + default: + return false; + } +} +} // namespace + +bool MessageDifferencer::ShouldCompareNoPresence( + const Message& message1, const Reflection& reflection1, + const FieldDescriptor* field2) const { + const bool compare_no_presence_by_field = force_compare_no_presence_ && + !field2->has_presence() && + !field2->is_repeated(); + if (compare_no_presence_by_field) { + return true; + } + 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)); + return compare_no_presence_by_address; +} + std::vector MessageDifferencer::CombineFields( - const std::vector& fields1, Scope fields1_scope, - const std::vector& fields2, Scope fields2_scope) { + const Message& message1, const std::vector& fields1, + Scope fields1_scope, const std::vector& fields2, + Scope fields2_scope) { + const Reflection* reflection1 = message1.GetReflection(); size_t index1 = 0; size_t index2 = 0; @@ -748,8 +787,8 @@ std::vector MessageDifferencer::CombineFields( } else if (FieldBefore(field2, field1)) { if (fields2_scope == FULL) { tmp_message_fields_.push_back(field2); - } else if (fields2_scope == PARTIAL && force_compare_no_presence_ && - !field2->has_presence() && !field2->is_repeated()) { + } else if (fields2_scope == PARTIAL && + ShouldCompareNoPresence(message1, *reflection1, field2)) { // In order to make MessageDifferencer play nicely with no-presence // fields in unit tests, we want to check if the expected proto // (message1) has some fields which are set to their default value but diff --git a/src/google/protobuf/util/message_differencer.h b/src/google/protobuf/util/message_differencer.h index 5c058f918b673..6d8e9a9b00586 100644 --- a/src/google/protobuf/util/message_differencer.h +++ b/src/google/protobuf/util/message_differencer.h @@ -32,6 +32,7 @@ #include "absl/log/absl_check.h" #include "google/protobuf/descriptor.h" // FieldDescriptor #include "google/protobuf/message.h" // Message +#include "google/protobuf/text_format.h" #include "google/protobuf/unknown_field_set.h" #include "google/protobuf/util/field_comparator.h" @@ -581,6 +582,13 @@ class PROTOBUF_EXPORT MessageDifferencer { // in the comparison. void set_force_compare_no_presence(bool value); + // If set, the fields in message1 that equal the fields passed here will be + // treated as required for comparison, even if they are absent. + void set_require_no_presence_fields( + const google::protobuf::TextFormat::Parser::UnsetFieldsMetadata& fields) { + require_no_presence_fields_ = fields; + } + // DEPRECATED. Pass a DefaultFieldComparator instance instead. // Sets the type of comparison (as defined in the FloatComparison enumeration // above) that is used by this differencer when comparing float (and double) @@ -768,6 +776,7 @@ class PROTOBUF_EXPORT MessageDifferencer { // list. Fields only present in one of the lists will only appear in the // combined list if the corresponding fields_scope option is set to FULL. std::vector CombineFields( + const Message& message1, const std::vector& fields1, Scope fields1_scope, const std::vector& fields2, Scope fields2_scope); @@ -919,11 +928,17 @@ class PROTOBUF_EXPORT MessageDifferencer { const FieldDescriptor* field, const RepeatedFieldComparison& new_comparison); + // Whether we should still compare the field despite its absence in message1. + bool ShouldCompareNoPresence(const Message& message1, + const Reflection& reflection1, + const FieldDescriptor* field2) const; + Reporter* reporter_; DefaultFieldComparator default_field_comparator_; MessageFieldComparison message_field_comparison_; Scope scope_; absl::flat_hash_set force_compare_no_presence_fields_; + google::protobuf::TextFormat::Parser::UnsetFieldsMetadata require_no_presence_fields_; absl::flat_hash_set force_compare_failure_triggering_fields_; RepeatedFieldComparison repeated_field_comparison_; diff --git a/src/google/protobuf/util/message_differencer_unittest.cc b/src/google/protobuf/util/message_differencer_unittest.cc index e673d029504e1..191281a74ddb8 100644 --- a/src/google/protobuf/util/message_differencer_unittest.cc +++ b/src/google/protobuf/util/message_differencer_unittest.cc @@ -29,8 +29,10 @@ #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "google/protobuf/any_test.pb.h" +#include "google/protobuf/descriptor.h" #include "google/protobuf/map_test_util.h" #include "google/protobuf/map_unittest.pb.h" +#include "google/protobuf/message.h" #include "google/protobuf/test_util.h" #include "google/protobuf/text_format.h" #include "google/protobuf/unittest.pb.h" @@ -44,12 +46,31 @@ namespace google { namespace protobuf { +namespace internal { +class UnsetFieldsMetadataMessageDifferencerTestUtil { + public: + static void AddExplicitUnsetField( + const Message& message, const Reflection& reflection, + const FieldDescriptor& fd, + TextFormat::Parser::UnsetFieldsMetadata* metadata) { + metadata->addresses_.insert( + TextFormat::Parser::UnsetFieldsMetadata::GetUnsetFieldAddress( + message, reflection, fd)); + } +}; +} // namespace internal + namespace { +using ::google::protobuf::internal::UnsetFieldsMetadataMessageDifferencerTestUtil; + proto3_unittest::TestNoPresenceField MakeTestNoPresenceField() { proto3_unittest::TestNoPresenceField msg1, msg2; msg1.set_no_presence_bool(true); + msg1.set_no_presence_bool2(true); + msg1.set_no_presence_bool3(true); + msg1.set_no_presence_string("yolo"); msg2 = msg1; *msg1.mutable_no_presence_nested() = msg2; *msg1.add_no_presence_repeated_nested() = msg2; @@ -378,6 +399,212 @@ TEST(MessageDifferencerTest, EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); } +TEST(MessageDifferencerTest, + PartialEqualityTestBooleanPresenceFieldMissingWithAddress) { + util::MessageDifferencer address_differencer; + address_differencer.set_scope(util::MessageDifferencer::PARTIAL); + + // This differencer is not setting force_compare_no_presence. + util::MessageDifferencer default_differencer; + default_differencer.set_scope(util::MessageDifferencer::PARTIAL); + default_differencer.set_force_compare_no_presence(false); + + // When clearing a singular no presence field, it will be included in the + // comparison. + 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); + address_differencer.set_require_no_presence_fields(metadata); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.clear_no_presence_bool(); + + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg2.clear_no_presence_bool(); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.set_no_presence_bool(true); + + EXPECT_FALSE(default_differencer.Compare(msg1, msg2)); + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestStringPresenceFieldMissingWithAddress) { + util::MessageDifferencer address_differencer; + address_differencer.set_scope(util::MessageDifferencer::PARTIAL); + + // This differencer is not setting force_compare_no_presence. + util::MessageDifferencer default_differencer; + default_differencer.set_scope(util::MessageDifferencer::PARTIAL); + default_differencer.set_force_compare_no_presence(false); + + // When clearing a singular no presence field, it will be included in the + // comparison. + 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); + address_differencer.set_require_no_presence_fields(metadata); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.clear_no_presence_string(); + + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg2.clear_no_presence_string(); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.set_no_presence_string("yolo"); + + EXPECT_FALSE(default_differencer.Compare(msg1, msg2)); + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); +} + +// Ensure multiple booleans are addressed distinctly. This is trivially the case +// now, but tests against possible optimizations in the future to use bitfields. +TEST(MessageDifferencerTest, + PartialEqualityTestTwoBoolsPresenceFieldMissingWithAddress) { + util::MessageDifferencer address_differencer; + address_differencer.set_scope(util::MessageDifferencer::PARTIAL); + + // This differencer is not setting force_compare_no_presence. + util::MessageDifferencer default_differencer; + default_differencer.set_scope(util::MessageDifferencer::PARTIAL); + default_differencer.set_force_compare_no_presence(false); + + // When clearing a singular no presence field, it will be included in the + // comparison. + 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); + address_differencer.set_require_no_presence_fields(metadata); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + // Trigger on bool2. + msg1.clear_no_presence_bool2(); + + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + // Triggering on bool2 still ignores bool3. + msg1.set_no_presence_bool2(true); + msg1.clear_no_presence_bool3(); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestBooleanNestedMessagePresenceFieldMissingWithAddress) { + util::MessageDifferencer address_differencer; + address_differencer.set_scope(util::MessageDifferencer::PARTIAL); + + // This differencer is not setting force_compare_no_presence. + util::MessageDifferencer default_differencer; + default_differencer.set_scope(util::MessageDifferencer::PARTIAL); + default_differencer.set_force_compare_no_presence(false); + + // When clearing a singular no presence field, it will be included in the + // comparison. + 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); + address_differencer.set_require_no_presence_fields(metadata); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.mutable_no_presence_nested()->clear_no_presence_bool(); + + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg2.mutable_no_presence_nested()->clear_no_presence_bool(); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.mutable_no_presence_nested()->set_no_presence_bool(true); + + EXPECT_FALSE(default_differencer.Compare(msg1, msg2)); + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestBooleanRepeatedMessagePresenceFieldMissingWithAddress) { + util::MessageDifferencer address_differencer; + address_differencer.set_scope(util::MessageDifferencer::PARTIAL); + + // This differencer is not setting force_compare_no_presence. + util::MessageDifferencer default_differencer; + default_differencer.set_scope(util::MessageDifferencer::PARTIAL); + default_differencer.set_force_compare_no_presence(false); + + // When clearing a singular no presence field, it will be included in the + // comparison. + 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); + address_differencer.set_require_no_presence_fields(metadata); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.mutable_no_presence_repeated_nested(0)->clear_no_presence_bool(); + + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg2.mutable_no_presence_repeated_nested(0)->clear_no_presence_bool(); + + EXPECT_TRUE(address_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + msg1.mutable_no_presence_repeated_nested(0)->set_no_presence_bool(true); + + EXPECT_FALSE(default_differencer.Compare(msg1, msg2)); + EXPECT_FALSE(address_differencer.Compare(msg1, msg2)); +} + TEST(MessageDifferencerTest, PartialEqualityTestExtraFieldNoPresenceForceCompareReporterAware) { std::string output; diff --git a/src/google/protobuf/util/message_differencer_unittest_proto3.proto b/src/google/protobuf/util/message_differencer_unittest_proto3.proto index 55540cefd3925..b13955456e216 100644 --- a/src/google/protobuf/util/message_differencer_unittest_proto3.proto +++ b/src/google/protobuf/util/message_differencer_unittest_proto3.proto @@ -18,4 +18,7 @@ message TestNoPresenceField { bool no_presence_bool = 1; TestNoPresenceField no_presence_nested = 2; repeated TestNoPresenceField no_presence_repeated_nested = 3; + string no_presence_string = 4; + bool no_presence_bool2 = 5; + bool no_presence_bool3 = 6; }