Update proto parsing to keep track of parsed fields that have no presence and were parsed to their default value, hence had no effect on the result proto.

protobuf-github-bot · copybara-github · commit bf1ae222c9fa · 2023-03-21T07:06:55.000-07:00
Update the Partially proto matcher to cause proto parsing to check fail if the above condition was triggered.
This is to address a known issue where test are silently passing when proto3 protos are the arguments to Partially(EqualsProto) and have some of the fields ignored by the EqualsProto matcher since they were set to their default value in the expected proto.

PiperOrigin-RevId: 518262030
diff --git a/src/google/protobuf/text_format.cc b/src/google/protobuf/text_format.cc
@@ -320,7 +320,8 @@ class TextFormat::Parser::ParserImpl {
              bool allow_case_insensitive_field, bool allow_unknown_field,
              bool allow_unknown_extension, bool allow_unknown_enum,
              bool allow_field_number, bool allow_relaxed_whitespace,
-             bool allow_partial, int recursion_limit)
+             bool allow_partial, int recursion_limit,
+             bool error_on_no_op_fields)
       : error_collector_(error_collector),
         finder_(finder),
         parse_info_tree_(parse_info_tree),
@@ -337,7 +338,8 @@ class TextFormat::Parser::ParserImpl {
         initial_recursion_limit_(recursion_limit),
         recursion_limit_(recursion_limit),
         had_silent_marker_(false),
-        had_errors_(false) {
+        had_errors_(false),
+        error_on_no_op_fields_(error_on_no_op_fields) {
     // For backwards-compatibility with proto1, we need to allow the 'f' suffix
     // for floats.
     tokenizer_.set_allow_f_after_float(true);
@@ -817,75 +819,86 @@ class TextFormat::Parser::ParserImpl {
 // Define an easy to use macro for setting fields. This macro checks
 // to see if the field is repeated (in which case we need to use the Add
 // methods or not (in which case we need to use the Set methods).
-#define SET_FIELD(CPPTYPE, VALUE)                    \
-  if (field->is_repeated()) {                        \
-    reflection->Add##CPPTYPE(message, field, VALUE); \
-  } else {                                           \
-    reflection->Set##CPPTYPE(message, field, VALUE); \
+// 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)); \
+    }                                                             \
   }
 
     switch (field->cpp_type()) {
       case FieldDescriptor::CPPTYPE_INT32: {
         int64_t value;
         DO(ConsumeSignedInteger(&value, kint32max));
-        SET_FIELD(Int32, static_cast<int32_t>(value));
+        SET_FIELD(Int32, int32, static_cast<int32_t>(value));
         break;
       }
 
       case FieldDescriptor::CPPTYPE_UINT32: {
         uint64_t value;
         DO(ConsumeUnsignedInteger(&value, kuint32max));
-        SET_FIELD(UInt32, static_cast<uint32_t>(value));
+        SET_FIELD(UInt32, uint32, static_cast<uint32_t>(value));
         break;
       }
 
       case FieldDescriptor::CPPTYPE_INT64: {
         int64_t value;
         DO(ConsumeSignedInteger(&value, kint64max));
-        SET_FIELD(Int64, value);
+        SET_FIELD(Int64, int64, value);
         break;
       }
 
       case FieldDescriptor::CPPTYPE_UINT64: {
         uint64_t value;
         DO(ConsumeUnsignedInteger(&value, kuint64max));
-        SET_FIELD(UInt64, value);
+        SET_FIELD(UInt64, uint64, value);
         break;
       }
 
       case FieldDescriptor::CPPTYPE_FLOAT: {
         double value;
         DO(ConsumeDouble(&value));
-        SET_FIELD(Float, io::SafeDoubleToFloat(value));
+        SET_FIELD(Float, float, io::SafeDoubleToFloat(value));
         break;
       }
 
       case FieldDescriptor::CPPTYPE_DOUBLE: {
         double value;
         DO(ConsumeDouble(&value));
-        SET_FIELD(Double, value);
+        SET_FIELD(Double, double, value);
         break;
       }
 
       case FieldDescriptor::CPPTYPE_STRING: {
         std::string value;
         DO(ConsumeString(&value));
-        SET_FIELD(String, std::move(value));
+        SET_FIELD(String, string, value);
         break;
       }
 
       case FieldDescriptor::CPPTYPE_BOOL: {
         if (LookingAtType(io::Tokenizer::TYPE_INTEGER)) {
           uint64_t value;
           DO(ConsumeUnsignedInteger(&value, 1));
-          SET_FIELD(Bool, value);
+          SET_FIELD(Bool, bool, value);
         } else {
           std::string value;
           DO(ConsumeIdentifier(&value));
           if (value == "true" || value == "True" || value == "t") {
-            SET_FIELD(Bool, true);
+            SET_FIELD(Bool, bool, true);
           } else if (value == "false" || value == "False" || value == "f") {
-            SET_FIELD(Bool, false);
+            SET_FIELD(Bool, bool, false);
           } else {
             ReportError(absl::StrCat("Invalid value for boolean field \"",
                                      field->name(), "\". Value: \"", value,
@@ -921,7 +934,7 @@ class TextFormat::Parser::ParserImpl {
         if (enum_value == nullptr) {
           if (int_value != kint64max &&
               !field->legacy_enum_field_treated_as_closed()) {
-            SET_FIELD(EnumValue, int_value);
+            SET_FIELD(EnumValue, int64, int_value);
             return true;
           } else if (!allow_unknown_enum_) {
             ReportError(absl::StrCat("Unknown enumeration value of \"", value,
@@ -935,7 +948,7 @@ class TextFormat::Parser::ParserImpl {
           }
         }
 
-        SET_FIELD(Enum, enum_value);
+        SET_FIELD(Enum, enum, enum_value);
         break;
       }
 
@@ -1404,6 +1417,8 @@ class TextFormat::Parser::ParserImpl {
   int recursion_limit_;
   bool had_silent_marker_;
   bool had_errors_;
+  bool error_on_no_op_fields_;
+
 };
 
 // ===========================================================================
@@ -1700,7 +1715,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_);
+                    allow_partial_, recursion_limit_, error_on_no_op_fields_);
   return MergeUsingImpl(input, output, &parser);
 }
 
@@ -1725,7 +1740,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_);
+                    allow_partial_, recursion_limit_, error_on_no_op_fields_);
   return MergeUsingImpl(input, output, &parser);
 }
 
@@ -1755,12 +1770,13 @@ bool TextFormat::Parser::ParseFieldValueFromString(const std::string& input,
                                                    const FieldDescriptor* field,
                                                    Message* output) {
   io::ArrayInputStream input_stream(input.data(), input.size());
-  ParserImpl parser(
-      output->GetDescriptor(), &input_stream, error_collector_, finder_,
-      parse_info_tree_, ParserImpl::ALLOW_SINGULAR_OVERWRITES,
-      allow_case_insensitive_field_, allow_unknown_field_,
-      allow_unknown_extension_, allow_unknown_enum_, allow_field_number_,
-      allow_relaxed_whitespace_, allow_partial_, recursion_limit_);
+  ParserImpl parser(output->GetDescriptor(), &input_stream, error_collector_,
+                    finder_, parse_info_tree_,
+                    ParserImpl::ALLOW_SINGULAR_OVERWRITES,
+                    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_);
   return parser.ParseField(field, output);
 }
 
diff --git a/src/google/protobuf/text_format.h b/src/google/protobuf/text_format.h
@@ -694,6 +694,13 @@ 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
+    // 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;
+    }
+
    private:
     // Forward declaration of an internal class used to parse text
     // representations (see text_format.cc for implementation).
@@ -716,6 +723,7 @@ class PROTOBUF_EXPORT TextFormat {
     bool allow_relaxed_whitespace_;
     bool allow_singular_overwrites_;
     int recursion_limit_;
+    bool error_on_no_op_fields_ = false;
   };
 
 
diff --git a/src/google/protobuf/text_format_unittest.cc b/src/google/protobuf/text_format_unittest.cc
@@ -901,6 +901,89 @@ TEST_F(TextFormatTest, ParseUnknownEnumFieldProto3) {
   EXPECT_EQ(-2147483648, proto.repeated_nested_enum(3));
 }
 
+TEST_F(TextFormatTest, ErrorOnNoOpFieldsProto3) {
+  proto3_unittest::TestAllTypes proto;
+  TextFormat::Parser parser;
+  parser.ErrorOnNoOpFields(true);
+
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+  {
+    // 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));
+  }
+  {
+    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));
+  }
+  {
+    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));
+  }
+}
+
 TEST_F(TextFormatTest, ParseStringEscape) {
   // Create a parse string with escaped characters in it.
   std::string parse_string =
