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
protocolbuffers · Apr 25, 2023 · 748f57f · 748f57f
1 parent 28060e4
commit 748f57f
Show file tree

Hide file tree

Showing 5 changed files with 278 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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<const FieldDescriptor*> force_compare_no_presence_fields_;
   RepeatedFieldComparison repeated_field_comparison_;
 
   absl::flat_hash_map<const FieldDescriptor*, RepeatedFieldComparison>
@@ -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
@@ -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<std::string> 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
@@ -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;
+}