JsonPatch
JsonPatch copied to clipboard
When setting values, retain the original object references if types are compatible (instead of serializing/deserializing)
Just noticed a potential issue with the implementation for move
. In PathHelper.ConvertValue()
, we currently set values by serializing then deserializing the object (in order to convert it to the appropriate type at the destination path):
private static object ConvertValue(object value, Type type)
{
return JsonConvert.DeserializeObject(JsonConvert.SerializeObject(value), type);
}
This generally works well for add
and replace
, since value
is going to be coming in as a JObject
, which is how it gets read in from the stream by Json.NET. It's not likely that value
is going to be of the appropriate type (unless the user's entity is working directly with JObject
), so we need to convert it, and this serialization/deserialization technique does the trick (an alternative would be to use JToken.ToObject).
However, this conversion is unnecessary and can actually be problematic for move
, since value
is now going to be a reference to whatever object was inside the from
path. I think it makes more sense to just keep the same object reference when assigning the value at the destination path (rather than destroying the old instance at the from
path, and newing up a brand new instance at the destination path). Doing otherwise can be surprising, and maybe even problematic (especially if the PATCH is being applied directly against an EF entity class - you could run into JsonSerializationException
inside JsonConvert.SerializeObject()
due to self-referencing loops, or run into errors in Entity Framework when actually trying to save the entity, because it's doing reference comparisons internally to validate relationships).
My proposed fix would be to change ConvertValue
to check if the value is already an instance of the given type, and if so, then just return it - otherwise, keep the existing conversion logic:
private static object ConvertValue(object value, Type type)
{
if (type.IsInstanceOfType(value))
{
return value;
}
return JsonConvert.DeserializeObject(JsonConvert.SerializeObject(value), type);
}
Does that sound OK? If so, I'll also add some tests and put up a pull request.