dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

Enhancement to JsonPatch to provide a comparison method which compares two objects and returns a JsonPatchDocument #3104

Closed jmcginty closed 5 years ago

jmcginty commented 6 years ago

Would you consider a pull request to implement this? I was thinking the API would be a static method like

/// <summary>
/// Compares two objects and returns a JsonPatchDocument describing the changes
/// </summary>
/// <param name="original">original object</param>
/// <param name="modified">modified object</param>
/// <returns></returns>
public static JsonPatchDocument CreatePatch(object original, object modified)

called like so:

 JsonPatchDocument patch = JsonPatchDocument.CreatePatch(originalObject, modifiedObject);
jimmyheaddon commented 6 years ago

Upvoting, as this'd really round off this library. Currently you have to generate the JSONPatch documents with other libraries (e.g. JsonDiffPatch), whereas a first-class API like the suggestion above would be great.

One thing to watch out for is that Newtonsoft is used by the Microsoft.AspNetCore.JsonPatch library, so you can end up with JSONPatch documents that result in semantically identical objects, but the JSONPatch document itself can be different at the text level. For example, Newtonsoft breaks ISO DateTime objects (see this Github Issue), and also uses JToken.ToString() which loses all precision below milliseconds, for DateTime objects, meaning you get invalid JSONPatch documents. You need tests like the two below to spot these issues, which could translate in to this new feature, if not careful:

[TestCase("{a:\"2017-01-01T12:34:56.789Z\"}", "{a:\"2017-01-01T16:34:56.789+0400\"}", ExpectedResult = "[{\"op\":\"replace\",\"path\":\"/a\",\"value\":\"2017-01-01T12:34:56.789+00:00\"}]", TestName = "JsonPatch replace works for semantically identical ISO 8601 date/time strings where only 1 uses the zero UTC designation")]

[TestCase("{a:\"2017-01-01T00:00:00.000Z\"}", "{a:\"2017-01-01T00:00:00.001Z\"}", ExpectedResult = "[{\"op\":\"replace\",\"path\":\"/a\",\"value\":\"2017-01-01T00:00:00.001Z\"}]", TestName = "JsonPatch replace works for high-precision date/time strings with just the milliseconds changing")]

I'd be happy to contribute to a PR, if this is likely to be accepted?

jimmyheaddon commented 6 years ago

For reference, it looks like someone is already working on this, although the author mentions they haven't checked deeply nested objects, etc.

mkArtakMSFT commented 5 years ago

Hi. Thanks for contacting us. We're closing this issue as there was not much community interest in this ask for quite a while now.