From 748f57f2b67c56e050b660ffbafb9f3a5795c28a Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 25 Apr 2023 16:11:28 -0700 Subject: [PATCH] Update MessageDifferencer to conditionally force comparing additional fields while doing PARTIAL comparison (compare fields which are not repeated, have no presence and are set to their default value). PiperOrigin-RevId: 527100664 --- src/google/protobuf/util/BUILD.bazel | 15 ++ .../protobuf/util/message_differencer.cc | 26 +++ .../protobuf/util/message_differencer.h | 12 ++ .../util/message_differencer_unittest.cc | 181 ++++++++++++++++++ .../message_differencer_unittest_proto3.proto | 44 +++++ 5 files changed, 278 insertions(+) create mode 100644 src/google/protobuf/util/message_differencer_unittest_proto3.proto diff --git a/src/google/protobuf/util/BUILD.bazel b/src/google/protobuf/util/BUILD.bazel index 3afe464cff..35ede30466 100644 --- a/src/google/protobuf/util/BUILD.bazel +++ b/src/google/protobuf/util/BUILD.bazel @@ -82,6 +82,7 @@ cc_test( deps = [ ":differencer", ":message_differencer_unittest_cc_proto", + ":message_differencer_unittest_proto3_cc_proto", "//src/google/protobuf:cc_test_protos", "//src/google/protobuf:test_util", "//src/google/protobuf/testing", @@ -202,6 +203,7 @@ filegroup( "json_format.proto", "json_format_proto3.proto", "message_differencer_unittest.proto", + "message_differencer_unittest_proto3.proto", ], visibility = [ "//pkg:__pkg__", @@ -260,6 +262,19 @@ cc_proto_library( deps = [":message_differencer_unittest_proto"], ) +proto_library( + name = "message_differencer_unittest_proto3_proto", + testonly = 1, + srcs = ["message_differencer_unittest_proto3.proto"], + strip_import_prefix = "/src", +) + +cc_proto_library( + name = "message_differencer_unittest_proto3_cc_proto", + testonly = 1, + deps = [":message_differencer_unittest_proto3_proto"], +) + ################################################################################ # Distribution packaging ################################################################################ diff --git a/src/google/protobuf/util/message_differencer.cc b/src/google/protobuf/util/message_differencer.cc index 18a542a435..76e0267a7f 100644 --- a/src/google/protobuf/util/message_differencer.cc +++ b/src/google/protobuf/util/message_differencer.cc @@ -368,6 +368,10 @@ void MessageDifferencer::set_scope(Scope scope) { scope_ = scope; } MessageDifferencer::Scope MessageDifferencer::scope() const { return scope_; } +void MessageDifferencer::set_force_compare_no_presence(bool value) { + force_compare_no_presence_ = value; +} + void MessageDifferencer::set_float_comparison(FloatComparison comparison) { default_field_comparator_.set_float_comparison( comparison == EXACT ? DefaultFieldComparator::EXACT @@ -759,6 +763,17 @@ FieldDescriptorArray MessageDifferencer::CombineFields( } else if (FieldBefore(field2, field1)) { if (fields2_scope == FULL) { tmp_message_fields_.push_back(fields2[index2]); + } else if (fields2_scope == PARTIAL && force_compare_no_presence_ && + !field2->has_presence() && !field2->is_repeated()) { + // 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 + // are not set to their default value in message2 (the actual message). + // Those fields will appear in fields2 (since they have non default + // value) but will not appear in fields1 (since they have the default + // value or were never set). + force_compare_no_presence_fields_.insert(fields2[index2]); + tmp_message_fields_.push_back(fields2[index2]); } ++index2; } else { @@ -889,6 +904,10 @@ bool MessageDifferencer::CompareWithFieldsInternal( specific_field.new_index = -1; } + specific_field.forced_compare_no_presence_ = + force_compare_no_presence_ && + force_compare_no_presence_fields_.contains(specific_field.field); + parent_fields->push_back(specific_field); reporter_->ReportAdded(message1, message2, *parent_fields); parent_fields->pop_back(); @@ -944,6 +963,10 @@ bool MessageDifferencer::CompareWithFieldsInternal( specific_field.unpacked_any = unpacked_any; specific_field.field = field1; parent_fields->push_back(specific_field); + specific_field.forced_compare_no_presence_ = + force_compare_no_presence_ && + force_compare_no_presence_fields_.contains(field1); + if (fieldDifferent) { reporter_->ReportModified(message1, message2, *parent_fields); isDifferent = true; @@ -2048,6 +2071,9 @@ void MessageDifferencer::StreamReporter::PrintPath( printer_->Print("($name$)", "name", specific_field.field->full_name()); } else { printer_->PrintRaw(specific_field.field->name()); + if (specific_field.forced_compare_no_presence_) { + printer_->Print(" (added for better PARTIAL comparison)"); + } } if (specific_field.field->is_map()) { diff --git a/src/google/protobuf/util/message_differencer.h b/src/google/protobuf/util/message_differencer.h index d0e5c72bc5..d40430de2d 100644 --- a/src/google/protobuf/util/message_differencer.h +++ b/src/google/protobuf/util/message_differencer.h @@ -229,6 +229,10 @@ class PROTOBUF_EXPORT MessageDifferencer { // reporting an addition or deletion. int unknown_field_index1 = -1; int unknown_field_index2 = -1; + + // Was this field added to the diffing because set_force_compare_no_presence + // was called on the MessageDifferencer object. + bool forced_compare_no_presence_ = false; }; // Abstract base class from which all MessageDifferencer @@ -600,6 +604,12 @@ class PROTOBUF_EXPORT MessageDifferencer { // Returns the current scope used by this differencer. Scope scope() const; + // Only affects PARTIAL diffing. When set, all non-repeated no-presence fields + // which are set to their default value (which is the same as being unset) in + // message1 but are set to a non-default value in message2 will also be used + // in the comparison. + void set_force_compare_no_presence(bool value); + // 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) @@ -936,6 +946,7 @@ class PROTOBUF_EXPORT MessageDifferencer { DefaultFieldComparator default_field_comparator_; MessageFieldComparison message_field_comparison_; Scope scope_; + absl::flat_hash_set force_compare_no_presence_fields_; RepeatedFieldComparison repeated_field_comparison_; absl::flat_hash_map @@ -965,6 +976,7 @@ class PROTOBUF_EXPORT MessageDifferencer { bool report_matches_; bool report_moves_; bool report_ignores_; + bool force_compare_no_presence_ = false; std::string* output_string_; diff --git a/src/google/protobuf/util/message_differencer_unittest.cc b/src/google/protobuf/util/message_differencer_unittest.cc index aedbf28642..da9daf1187 100644 --- a/src/google/protobuf/util/message_differencer_unittest.cc +++ b/src/google/protobuf/util/message_differencer_unittest.cc @@ -58,7 +58,9 @@ #include "google/protobuf/unittest.pb.h" #include "google/protobuf/util/field_comparator.h" #include "google/protobuf/util/message_differencer_unittest.pb.h" +#include "google/protobuf/util/message_differencer_unittest_proto3.pb.h" #include "google/protobuf/wire_format.h" +#include "google/protobuf/wire_format_lite.h" namespace google { @@ -67,6 +69,15 @@ namespace protobuf { namespace { +proto3_unittest::TestNoPresenceField MakeTestNoPresenceField() { + proto3_unittest::TestNoPresenceField msg1, msg2; + msg1.set_no_presence_bool(true); + msg2 = msg1; + *msg1.mutable_no_presence_nested() = msg2; + *msg1.add_no_presence_repeated_nested() = msg2; + return msg1; +} + const FieldDescriptor* GetFieldDescriptor(const Message& message, const std::string& field_name) { std::vector field_path = @@ -201,6 +212,17 @@ TEST(MessageDifferencerTest, BasicPartialEqualityTest) { EXPECT_TRUE(differencer.Compare(msg1, msg2)); } +TEST(MessageDifferencerTest, BasicPartialEqualityTestNoPresenceForceCompare) { + util::MessageDifferencer differencer; + differencer.set_scope(util::MessageDifferencer::PARTIAL); + differencer.set_force_compare_no_presence(true); + + // Create the testing protos + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + EXPECT_TRUE(differencer.Compare(msg1, msg2)); +} + TEST(MessageDifferencerTest, PartialEqualityTestExtraField) { // Create the testing protos unittest::TestAllTypes msg1; @@ -217,6 +239,165 @@ TEST(MessageDifferencerTest, PartialEqualityTestExtraField) { EXPECT_TRUE(differencer.Compare(msg1, msg2)); } +TEST(MessageDifferencerTest, + PartialEqualityTestExtraFieldNoPresenceForceCompare) { + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + + // 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); + + + // Create the testing protos + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + + // Clearing a no presence field inside a repeated field in a nested message. + msg1.mutable_no_presence_repeated_nested(0)->clear_no_presence_bool(); + EXPECT_FALSE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + force_compare_differencer.ReportDifferencesTo(nullptr); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestForceCompareWorksForRepeatedField) { + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + + // 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); + + // Repeated fields always have presence, so clearing them would remove them + // from the comparison. + // Create the testing protos + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + + msg1.clear_no_presence_repeated_nested(); + EXPECT_TRUE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestForceCompareWorksForRepeatedFieldInstance) { + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + + // 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); + + // Clearing a field inside a repeated field will trigger a failure when + // forcing comparison for no presence fields. + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + + msg1.mutable_no_presence_nested()->clear_no_presence_bool(); + EXPECT_FALSE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestForceCompareIsNoOptForNestedMessages) { + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + + // 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); + + // Nested fields always have presence, so clearing them would remove them + // from the comparison. + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + + msg1.clear_no_presence_nested(); + EXPECT_TRUE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); + + // Creating an instance of the nested field will cause the comparison to fail + // since it contains a no presence singualr field. + msg1.mutable_no_presence_nested(); + EXPECT_FALSE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestSingularNoPresenceFieldMissing) { + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + + // 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(); + + msg1.clear_no_presence_bool(); + EXPECT_FALSE(force_compare_differencer.Compare(msg1, msg2)); + EXPECT_TRUE(default_differencer.Compare(msg1, msg2)); + + EXPECT_FALSE(force_compare_differencer.Compare(msg2, msg1)); + EXPECT_FALSE(default_differencer.Compare(msg2, msg1)); +} + +TEST(MessageDifferencerTest, + PartialEqualityTestExtraFieldNoPresenceForceCompareReporterAware) { + std::string output; + // Before we can check the output string, we must make sure the + // StreamReporter is destroyed because its destructor will + // flush the stream. + { + io::StringOutputStream output_stream(&output); + util::MessageDifferencer::StreamReporter reporter(&output_stream); + + util::MessageDifferencer force_compare_differencer; + force_compare_differencer.set_scope(util::MessageDifferencer::PARTIAL); + force_compare_differencer.set_force_compare_no_presence(true); + force_compare_differencer.ReportDifferencesTo(&reporter); + + // Clearing a no presence field inside a repeated field. + proto3_unittest::TestNoPresenceField msg1 = MakeTestNoPresenceField(); + proto3_unittest::TestNoPresenceField msg2 = MakeTestNoPresenceField(); + + msg1.mutable_no_presence_repeated_nested(0)->clear_no_presence_bool(); + EXPECT_FALSE(force_compare_differencer.Compare(msg1, msg2)); + } + EXPECT_EQ(output, + "added: no_presence_repeated_nested[0].no_presence_bool (added for " + "better PARTIAL comparison): true\n"); +} + TEST(MessageDifferencerTest, PartialEqualityTestSkipRequiredField) { // Create the testing protos unittest::TestRequired msg1; diff --git a/src/google/protobuf/util/message_differencer_unittest_proto3.proto b/src/google/protobuf/util/message_differencer_unittest_proto3.proto new file mode 100644 index 0000000000..038ccfe4e1 --- /dev/null +++ b/src/google/protobuf/util/message_differencer_unittest_proto3.proto @@ -0,0 +1,44 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// This file contains messages for testing repeated field comparison +// LINT: ALLOW_GROUPS + +syntax = "proto3"; + +package proto3_unittest; + +option optimize_for = SPEED; + +message TestNoPresenceField { + bool no_presence_bool = 1; + TestNoPresenceField no_presence_nested = 2; + repeated TestNoPresenceField no_presence_repeated_nested = 3; +}