Bug fix for FieldMask.MergeFrom() with unset fields.

The Python field mask implementation was incorrectly setting fields in the `destination` even if the `source` proto had presence bits and the field was unset. This behavior differed from C++ where the [`HasField()`](http://google3/third_party/protobuf/util/field_mask_util.cc;l=460;rcl=647700575) is checked before setting.

PiperOrigin-RevId: 746132583

Author: protobuf-github-bot

python/google/protobuf/internal/field_mask.py

@@ -293,8 +293,10 @@ def _MergeMessage(
           destination.ClearField(_StrConvert(name))
         if source.HasField(name):
           getattr(destination, name).MergeFrom(getattr(source, name))
-      else:
+      elif not field.has_presence or source.HasField(name):
         setattr(destination, name, getattr(source, name))
+      else:
+        destination.ClearField(_StrConvert(name))
 
 
 def _AddFieldPaths(node, prefix, field_mask):

python/google/protobuf/internal/field_mask_test.py

@@ -9,11 +9,13 @@
 
 import unittest
 
-from google.protobuf import field_mask_pb2
+from google.protobuf import descriptor
 from google.protobuf.internal import field_mask
 from google.protobuf.internal import test_util
-from google.protobuf import descriptor
+
+from google.protobuf import field_mask_pb2
 from google.protobuf import map_unittest_pb2
+from google.protobuf import unittest_no_field_presence_pb2
 from google.protobuf import unittest_pb2
 
 
@@ -106,22 +108,16 @@ def testCanonicalFrom(self):
     self.assertEqual('bar,foo.b1,foo.b2', out_mask.ToJsonString())
 
     # Test more deeply nested cases.
-    mask.FromJsonString(
-        'foo.bar.baz1,foo.bar.baz2.quz,foo.bar.baz2')
+    mask.FromJsonString('foo.bar.baz1,foo.bar.baz2.quz,foo.bar.baz2')
     out_mask.CanonicalFormFromMask(mask)
-    self.assertEqual('foo.bar.baz1,foo.bar.baz2',
-                     out_mask.ToJsonString())
-    mask.FromJsonString(
-        'foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz')
+    self.assertEqual('foo.bar.baz1,foo.bar.baz2', out_mask.ToJsonString())
+    mask.FromJsonString('foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz')
     out_mask.CanonicalFormFromMask(mask)
-    self.assertEqual('foo.bar.baz1,foo.bar.baz2',
-                     out_mask.ToJsonString())
-    mask.FromJsonString(
-        'foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz,foo.bar')
+    self.assertEqual('foo.bar.baz1,foo.bar.baz2', out_mask.ToJsonString())
+    mask.FromJsonString('foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz,foo.bar')
     out_mask.CanonicalFormFromMask(mask)
     self.assertEqual('foo.bar', out_mask.ToJsonString())
-    mask.FromJsonString(
-        'foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz,foo')
+    mask.FromJsonString('foo.bar.baz1,foo.bar.baz2,foo.bar.baz2.quz,foo')
     out_mask.CanonicalFormFromMask(mask)
     self.assertEqual('foo', out_mask.ToJsonString())
 
@@ -291,6 +287,36 @@ def testMergeMessageWithoutMapFields(self):
     self.assertTrue(dst.HasField('foo_message'))
     self.assertFalse(dst.HasField('foo_lazy_message'))
 
+  def testMergeMessageWithoutMapFieldsOrFieldPresence(self):
+    # Test merge one field.
+    src = unittest_no_field_presence_pb2.TestAllTypes()
+    test_util.SetAllFields(src)
+    for field in src.DESCRIPTOR.fields:
+      if field.containing_oneof:
+        continue
+      field_name = field.name
+      dst = unittest_no_field_presence_pb2.TestAllTypes()
+      # Only set one path to mask.
+      mask = field_mask_pb2.FieldMask()
+      mask.paths.append(field_name)
+      mask.MergeMessage(src, dst)
+      # The expected result message.
+      msg = unittest_no_field_presence_pb2.TestAllTypes()
+      if field.label == descriptor.FieldDescriptor.LABEL_REPEATED:
+        repeated_src = getattr(src, field_name)
+        repeated_msg = getattr(msg, field_name)
+        if field.cpp_type == descriptor.FieldDescriptor.CPPTYPE_MESSAGE:
+          for item in repeated_src:
+            repeated_msg.add().CopyFrom(item)
+        else:
+          repeated_msg.extend(repeated_src)
+      elif field.cpp_type == descriptor.FieldDescriptor.CPPTYPE_MESSAGE:
+        getattr(msg, field_name).CopyFrom(getattr(src, field_name))
+      else:
+        setattr(msg, field_name, getattr(src, field_name))
+      # Only field specified in mask is merged.
+      self.assertEqual(msg, dst)
+
   def testMergeMessageWithMapField(self):
     empty_map = map_unittest_pb2.TestRecursiveMapMessage()
     src_level_2 = map_unittest_pb2.TestRecursiveMapMessage()
@@ -314,6 +340,74 @@ def testMergeMessageWithMapField(self):
     self.assertEqual(dst.a['src level 1'], src_level_2)
     self.assertEqual(dst.a['dst level 1'], empty_map)
 
+  def testMergeMessageWithUnsetFieldsWithFieldPresence(self):
+    # Test merging each empty field one at a time.
+    src = unittest_pb2.TestAllTypes()
+    for field in src.DESCRIPTOR.fields:
+      if field.containing_oneof:
+        continue
+      field_name = field.name
+      dst = unittest_pb2.TestAllTypes()
+      # Only set one path to mask.
+      mask = field_mask_pb2.FieldMask()
+      mask.paths.append(field_name)
+      mask.MergeMessage(src, dst)
+      # Nothing should be merged.
+      self.assertEqual(unittest_pb2.TestAllTypes(), dst)
+
+    # Test merge clears previously set fields when source is unset.
+    dst_template = unittest_pb2.TestAllTypes()
+    test_util.SetAllFields(dst_template)
+    for field in src.DESCRIPTOR.fields:
+      if field.containing_oneof:
+        continue
+      dst = unittest_pb2.TestAllTypes()
+      dst.CopyFrom(dst_template)
+      # Only set one path to mask.
+      mask = field_mask_pb2.FieldMask()
+      mask.paths.append(field.name)
+      mask.MergeMessage(
+          src, dst, replace_message_field=True, replace_repeated_field=True
+      )
+      msg = unittest_pb2.TestAllTypes()
+      msg.CopyFrom(dst_template)
+      msg.ClearField(field.name)
+      self.assertEqual(msg, dst)
+
+  def testMergeMessageWithUnsetFieldsWithoutFieldPresence(self):
+    # Test merging each empty field one at a time.
+    src = unittest_no_field_presence_pb2.TestAllTypes()
+    for field in src.DESCRIPTOR.fields:
+      if field.containing_oneof:
+        continue
+      field_name = field.name
+      dst = unittest_no_field_presence_pb2.TestAllTypes()
+      # Only set one path to mask.
+      mask = field_mask_pb2.FieldMask()
+      mask.paths.append(field_name)
+      mask.MergeMessage(src, dst)
+      # Nothing should be merged.
+      self.assertEqual(unittest_no_field_presence_pb2.TestAllTypes(), dst)
+
+    # Test merge clears previously set fields when source is unset.
+    dst_template = unittest_no_field_presence_pb2.TestAllTypes()
+    test_util.SetAllFields(dst_template)
+    for field in src.DESCRIPTOR.fields:
+      if field.containing_oneof:
+        continue
+      dst = unittest_no_field_presence_pb2.TestAllTypes()
+      dst.CopyFrom(dst_template)
+      # Only set one path to mask.
+      mask = field_mask_pb2.FieldMask()
+      mask.paths.append(field.name)
+      mask.MergeMessage(
+          src, dst, replace_message_field=True, replace_repeated_field=True
+      )
+      msg = unittest_no_field_presence_pb2.TestAllTypes()
+      msg.CopyFrom(dst_template)
+      msg.ClearField(field.name)
+      self.assertEqual(msg, dst)
+
   def testMergeErrors(self):
     src = unittest_pb2.TestAllTypes()
     dst = unittest_pb2.TestAllTypes()
@@ -322,25 +416,26 @@ def testMergeErrors(self):
     mask.FromJsonString('optionalInt32.field')
     with self.assertRaises(ValueError) as e:
       mask.MergeMessage(src, dst)
-    self.assertEqual('Error: Field optional_int32 in message '
-                     'proto2_unittest.TestAllTypes is not a singular '
-                     'message field and cannot have sub-fields.',
-                     str(e.exception))
+    self.assertEqual(
+        'Error: Field optional_int32 in message '
+        'proto2_unittest.TestAllTypes is not a singular '
+        'message field and cannot have sub-fields.',
+        str(e.exception),
+    )
 
   def testSnakeCaseToCamelCase(self):
-    self.assertEqual('fooBar',
-                     field_mask._SnakeCaseToCamelCase('foo_bar'))
-    self.assertEqual('FooBar',
-                     field_mask._SnakeCaseToCamelCase('_foo_bar'))
-    self.assertEqual('foo3Bar',
-                     field_mask._SnakeCaseToCamelCase('foo3_bar'))
+    self.assertEqual('fooBar', field_mask._SnakeCaseToCamelCase('foo_bar'))
+    self.assertEqual('FooBar', field_mask._SnakeCaseToCamelCase('_foo_bar'))
+    self.assertEqual('foo3Bar', field_mask._SnakeCaseToCamelCase('foo3_bar'))
 
     # No uppercase letter is allowed.
     self.assertRaisesRegex(
         ValueError,
         'Fail to print FieldMask to Json string: Path name Foo must '
         'not contain uppercase letters.',
-        field_mask._SnakeCaseToCamelCase, 'Foo')
+        field_mask._SnakeCaseToCamelCase,
+        'Foo',
+    )
     # Any character after a "_" must be a lowercase letter.
     #   1. "_" cannot be followed by another "_".
     #   2. "_" cannot be followed by a digit.
@@ -349,28 +444,34 @@ def testSnakeCaseToCamelCase(self):
         ValueError,
         'Fail to print FieldMask to Json string: The character after a '
         '"_" must be a lowercase letter in path name foo__bar.',
-        field_mask._SnakeCaseToCamelCase, 'foo__bar')
+        field_mask._SnakeCaseToCamelCase,
+        'foo__bar',
+    )
     self.assertRaisesRegex(
         ValueError,
         'Fail to print FieldMask to Json string: The character after a '
         '"_" must be a lowercase letter in path name foo_3bar.',
-        field_mask._SnakeCaseToCamelCase, 'foo_3bar')
+        field_mask._SnakeCaseToCamelCase,
+        'foo_3bar',
+    )
     self.assertRaisesRegex(
         ValueError,
         'Fail to print FieldMask to Json string: Trailing "_" in path '
-        'name foo_bar_.', field_mask._SnakeCaseToCamelCase, 'foo_bar_')
+        'name foo_bar_.',
+        field_mask._SnakeCaseToCamelCase,
+        'foo_bar_',
+    )
 
   def testCamelCaseToSnakeCase(self):
-    self.assertEqual('foo_bar',
-                     field_mask._CamelCaseToSnakeCase('fooBar'))
-    self.assertEqual('_foo_bar',
-                     field_mask._CamelCaseToSnakeCase('FooBar'))
-    self.assertEqual('foo3_bar',
-                     field_mask._CamelCaseToSnakeCase('foo3Bar'))
+    self.assertEqual('foo_bar', field_mask._CamelCaseToSnakeCase('fooBar'))
+    self.assertEqual('_foo_bar', field_mask._CamelCaseToSnakeCase('FooBar'))
+    self.assertEqual('foo3_bar', field_mask._CamelCaseToSnakeCase('foo3Bar'))
     self.assertRaisesRegex(
         ValueError,
         'Fail to parse FieldMask: Path name foo_bar must not contain "_"s.',
-        field_mask._CamelCaseToSnakeCase, 'foo_bar')
+        field_mask._CamelCaseToSnakeCase,
+        'foo_bar',
+    )
 
 
 if __name__ == '__main__':

python/google/protobuf/internal/test_util.py

@@ -67,8 +67,12 @@ def SetAllNonLazyFields(message):
     message.optionalgroup.a = 117
   message.optional_nested_message.bb = 118
   message.optional_foreign_message.c = 119
-  message.optional_import_message.d = 120
-  message.optional_public_import_message.e = 126
+  if hasattr(message, 'optional_import_message'):
+    message.optional_import_message.d = 120
+  if hasattr(message, 'optional_public_import_message'):
+    message.optional_public_import_message.e = 126
+  if hasattr(message, 'optional_proto2_message'):
+    SetAllFields(message.optional_proto2_message)
 
   message.optional_nested_enum = unittest_pb2.TestAllTypes.BAZ
   message.optional_foreign_enum = unittest_pb2.FOREIGN_BAZ
@@ -77,7 +81,8 @@ def SetAllNonLazyFields(message):
 
   message.optional_string_piece = u'124'
   message.optional_cord = u'125'
-  message.optional_bytes_cord = b'optional bytes cord'
+  if hasattr(message, 'optional_bytes_cord'):
+    message.optional_bytes_cord = b'optional bytes cord'
 
   #
   # Repeated fields.
@@ -103,7 +108,8 @@ def SetAllNonLazyFields(message):
     message.repeatedgroup.add().a = 217
   message.repeated_nested_message.add().bb = 218
   message.repeated_foreign_message.add().c = 219
-  message.repeated_import_message.add().d = 220
+  if hasattr(message, 'repeated_import_message'):
+    message.repeated_import_message.add().d = 220
   message.repeated_lazy_message.add().bb = 227
 
   message.repeated_nested_enum.append(unittest_pb2.TestAllTypes.BAR)
@@ -150,7 +156,8 @@ def SetAllNonLazyFields(message):
     message.repeatedgroup.add().a = 317
   message.repeated_nested_message.add().bb = 318
   message.repeated_foreign_message.add().c = 319
-  message.repeated_import_message.add().d = 320
+  if hasattr(message, 'repeated_import_message'):
+    message.repeated_import_message.add().d = 320
   message.repeated_lazy_message.add().bb = 327
 
   message.repeated_nested_enum.append(unittest_pb2.TestAllTypes.BAR)
@@ -193,13 +200,15 @@ def SetAllNonLazyFields(message):
   message.oneof_uint32 = 601
   message.oneof_nested_message.bb = 602
   message.oneof_string = '603'
-  message.oneof_bytes = b'604'
+  if hasattr(message, 'oneof_bytes'):
+    message.oneof_bytes = b'604'
 
 
 def SetAllFields(message):
   SetAllNonLazyFields(message)
   message.optional_lazy_message.bb = 127
-  message.optional_unverified_lazy_message.bb = 128
+  if hasattr(message, 'optional_unverified_lazy_message'):
+    message.optional_unverified_lazy_message.bb = 128
 
 
 def SetAllExtensions(message):