You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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):
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.
The text was updated successfully, but these errors were encountered:
Just noticed a potential issue with the implementation for
move
. InPathHelper.ConvertValue()
, we currently set values by serializing then deserializing the object (in order to convert it to the appropriate type at the destination path):This generally works well for
add
andreplace
, sincevalue
is going to be coming in as aJObject
, which is how it gets read in from the stream by Json.NET. It's not likely thatvalue
is going to be of the appropriate type (unless the user's entity is working directly withJObject
), 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
, sincevalue
is now going to be a reference to whatever object was inside thefrom
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 thefrom
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 intoJsonSerializationException
insideJsonConvert.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:Does that sound OK? If so, I'll also add some tests and put up a pull request.
The text was updated successfully, but these errors were encountered: