Move check for wrapper types into the message branch of the field mas…

…k merge logic
protocolbuffers · Apr 12, 2022 · cef9f67 · cef9f67
1 parent c8a7868
commit cef9f67
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 4 deletions.
diff --git a/csharp/src/Google.Protobuf.Test/FieldMaskTreeTest.cs b/csharp/src/Google.Protobuf.Test/FieldMaskTreeTest.cs
@@ -433,7 +433,9 @@ public void Merge(bool useDynamicMessage)
         }
 
         [Test]
-        public void MergeWrapperFields()
+        [TestCase(true)]
+        [TestCase(false)]
+        public void MergeWrapperFields(bool replaceFields)
         {
             // Instantiate a destination with wrapper-based field types.
             var destination = new TestWellKnownTypes()
@@ -452,7 +454,17 @@ public void MergeWrapperFields()
             };
 
             Merge(new FieldMaskTree().AddFieldPath("string_field").AddFieldPath("int64_field"),
-                source, destination, new FieldMask.MergeOptions(), false);
+                source,
+                destination,
+                new FieldMask.MergeOptions()
+                {
+                    // Prove that setting the replace options has no bearing on the outcome of a merge operation
+                    // for well-known wrapper fields as they are semantically equivalent to primitive value fields.
+                    // For those fields "merge" is the same as "replace".
+                    ReplaceMessageFields = replaceFields,
+                    ReplacePrimitiveFields = replaceFields
+                },
+                false);
 
             // Make sure only the targeted fields were updated.
             Assert.AreEqual("Hi", destination.StringField);

diff --git a/csharp/src/Google.Protobuf/FieldMaskTree.cs b/csharp/src/Google.Protobuf/FieldMaskTree.cs
@@ -316,9 +316,10 @@ public void Merge(IMessage source, IMessage destination, FieldMask.MergeOptions
                 else
                 {
                     var sourceField = field.Accessor.GetValue(source);
-                    if (field.FieldType == FieldType.Message && !field.MessageType.IsWrapperType)
+                    if (field.FieldType == FieldType.Message)
                     {
-                        if (options.ReplaceMessageFields)
+                        // Semantically well-known wrapper types represent primitive values, so we do not "merge" them.
+                        if (options.ReplaceMessageFields || field.MessageType.IsWrapperType)
                         {
                             if (sourceField == null)
                             {