Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failing FieldMask.Merge for well-known wrapper field types #9602

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions csharp/src/Google.Protobuf.Test/FieldMaskTreeTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -432,5 +432,89 @@ public void Merge(bool useDynamicMessage)
Assert.IsNotNull(destination.Payload);
}

[Test]
public void MergeWrapperFieldsWithNonNullFieldsInSource()
{
// Instantiate a destination with wrapper-based field types.
var destination = new TestWellKnownTypes()
{
StringField = "Hello",
Int32Field = 12,
Int64Field = 24,
BoolField = true,
};

// Set up a targeted update.
var source = new TestWellKnownTypes()
{
StringField = "Hi",
Int64Field = 240
};

Merge(new FieldMaskTree().AddFieldPath("string_field").AddFieldPath("int64_field"),
source,
destination,
new FieldMask.MergeOptions(),
false);

// Make sure the targeted fields changed.
Assert.AreEqual("Hi", destination.StringField);
Assert.AreEqual(240, destination.Int64Field);

// Prove that non-targeted fields stay intact...
Assert.AreEqual(12, destination.Int32Field);
Assert.IsTrue(destination.BoolField);

// ...including default values which were not explicitly set in the destination object.
Assert.IsNull(destination.FloatField);
}

[Test]
[TestCase(false, "Hello", 24)]
[TestCase(true, null, null)]
public void MergeWrapperFieldsWithNullFieldsInSource(
bool replaceMessageFields,
string expectedStringValue,
long? expectedInt64Value)
{
// Instantiate a destination with wrapper-based field types.
var destination = new TestWellKnownTypes()
{
StringField = "Hello",
Int32Field = 12,
Int64Field = 24,
BoolField = true,
};

// Set up a targeted update with null valued fields.
var source = new TestWellKnownTypes()
{
StringField = null,
Int64Field = null
jskeet marked this conversation as resolved.
Show resolved Hide resolved
};

Merge(new FieldMaskTree().AddFieldPath("string_field").AddFieldPath("int64_field"),
source,
destination,
new FieldMask.MergeOptions()
{
ReplaceMessageFields = replaceMessageFields
},
false);

// Make sure the targeted fields changed according to our expectations, depending on the value of ReplaceMessageFields.
// When ReplaceMessageFields is false, the null values are not applied to the destination, because, although wrapped types
// are semantically primitives, FieldMaskTree.Merge still treats them as message types in order to maintain consistency with other Protobuf
// libraries such as Java and C++.
Assert.AreEqual(expectedStringValue, destination.StringField);
Assert.AreEqual(expectedInt64Value, destination.Int64Field);

// Prove that non-targeted fields stay intact...
Assert.AreEqual(12, destination.Int32Field);
Assert.IsTrue(destination.BoolField);

// ...including default values which were not explicitly set in the destination object.
Assert.IsNull(destination.FloatField);
}
}
}
19 changes: 14 additions & 5 deletions csharp/src/Google.Protobuf/FieldMaskTree.cs
Original file line number Diff line number Diff line change
Expand Up @@ -333,15 +333,24 @@ public void Merge(IMessage source, IMessage destination, FieldMask.MergeOptions
{
if (sourceField != null)
{
var sourceByteString = ((IMessage)sourceField).ToByteString();
var destinationValue = (IMessage)field.Accessor.GetValue(destination);
if (destinationValue != null)
// Well-known wrapper types are represented as nullable primitive types, so we do not "merge" them.
vassilvk marked this conversation as resolved.
Show resolved Hide resolved
// Instead, any non-null value just overwrites the previous value directly.
if (field.MessageType.IsWrapperType)
{
destinationValue.MergeFrom(sourceByteString);
field.Accessor.SetValue(destination, sourceField);
}
else
{
field.Accessor.SetValue(destination, field.MessageType.Parser.ParseFrom(sourceByteString));
var sourceByteString = ((IMessage)sourceField).ToByteString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for anyone else reviewing this: the code here really hasn't changed at all other than indentation - the diff is just confusing. (It's slightly better in split mode than in unified mode, but either way it's not clear that really we're just conditionalizing the existing behavior.)

var destinationValue = (IMessage)field.Accessor.GetValue(destination);
if (destinationValue != null)
{
destinationValue.MergeFrom(sourceByteString);
}
else
{
field.Accessor.SetValue(destination, field.MessageType.Parser.ParseFrom(sourceByteString));
}
}
}
}
Expand Down