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

#23 Added generating JsonPatch from two objects #160

Merged

Conversation

LordXaosa
Copy link

Implementing of feature #23
Works with any kind of arrays and objects of any complexity.
Example of use:

var obj1 = new SomeObject(){Name="test"};
var obj2 = new SomeObject(){Name="test1", TestProperty="test"};
var po = obj1.CreatePatch(obj2);//forward diff
var po = obj2.CreatePatch(obj1);//backward diff
var jp = new JsonPatch(po);
...

Copy link
Owner

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

Wow! Thank you so much for this!

A few things:

  1. The notes below.
  2. Please have a look at the formatting. There seem to be some indentation inconsistences.
  3. Please add some tests:
    • simple checks for single operation changes that cover all of the operations the code handles (for both objects and arrays)
    • a few complex checks including multiple changes and JSON documents with multiple layers
    • a round-trip check that
      • creates the patches for each direction
      • apply the patch to the first, verify that you get the second
      • apply the reverse patch to the second, verify that you get the first again

JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
@gregsdennis gregsdennis changed the base branch from master to patch-diff September 9, 2021 20:12
@LordXaosa
Copy link
Author

Please review changes according to your comments

JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
JsonPatch.Tests/ExtensionTests.cs Outdated Show resolved Hide resolved
JsonPatch.Tests/ExtensionTests.cs Outdated Show resolved Hide resolved
JsonPatch.Tests/ExtensionTests.cs Outdated Show resolved Hide resolved
}

[Test]
public void Add_Test()
Copy link
Owner

Choose a reason for hiding this comment

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

These tests aren't quite what I'm looking for. They should generate the patch and compare that to an expected patch.

public void Add()
{
    var initial = // initial model;
    var target = // target model;
    var expectedPatch = // explicitly create the expected JsonPatch object

    var actualPatch = initial.CreatePatch(target);

    Assert.AreEqual(expectedPatch, actualPatch);
}

This is a more typical unit test pattern.

Copy link
Author

Choose a reason for hiding this comment

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

Well in most cases I'll agree, but what with complex objects? It's a bit hard to create large patch manually. By the logic if patch was correct so objects would be equal, isn't it?

Copy link
Owner

Choose a reason for hiding this comment

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

This comment is for all of the simple tests. I want to ensure that the patch operations are generated correctly. To do that you need to analyze the patch itself.

The more complex tests (e.g. the round trip) can stay as it is, but these simple ones need to change.

JsonPatch/Extensions.cs Outdated Show resolved Hide resolved
public int[] Numbers { get; set; }
public string[] Strings { get; set; }
public List<TestModel> InnerObjects { get; set; }
public JsonDocument Attributes { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what including this property is accomplishing. It seems to be making the tests a bit confusing.

Copy link
Author

Choose a reason for hiding this comment

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

We use JsonDocument in our projects when we need some dynamic objects here. So I thought that it would be quite right to test this too.

Copy link
Owner

Choose a reason for hiding this comment

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

JsonDocument is tested implicitly becuase you convert objects to JsonDocument before generating the patch. I don't think this is explicitly necessary in the tests.

JsonPatch.Tests/ExtensionTests.cs Outdated Show resolved Hide resolved
@LordXaosa LordXaosa changed the title #23 Added generating PatchOperation list from two objects #23 Added generating JsonPatch from two objects Sep 10, 2021
return new JsonPatch(patch);
}

private static void PatchForObject(JsonElement orig, JsonElement mod, List<PatchOperation> patch, string path)
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd like to see the path represented as a JsonPointer here rather than a string. Check out JsonPointer.Create() and JsonPointer.Combine() for managing them. This would be a lot faster than going through parsing each time.

Copy link
Author

@LordXaosa LordXaosa Sep 10, 2021

Choose a reason for hiding this comment

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

I think I agree with Combine, but what with Create? It needs known type to perform, because it uses expression with exact type. In this case I can only use Combine+Parse but what the point if I have to use Parse anyway

Copy link
Owner

Choose a reason for hiding this comment

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

There's a non-generic Create that you can use as well that just takes segments (looks like I still need to define implicit conversions from ints and strings to segments).

https://github.com/gregsdennis/json-everything/blob/master/JsonPointer/JsonPointer.cs#L198

Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to update the pointer code to include casts from int and string to segments to make creating pointers easier. You'll need to rebase on top of that. It shouldn't have any conflicts since it's in a different project.

}
}
}
private static void PatchForArray(JsonElement orig, JsonElement mod, List<PatchOperation> patch, string path)
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

{
Id = Guid.Parse("a299e216-dbbe-40e4-b4d4-556d7e7e9c35")
};
var patchExpected = "[{\"op\":\"replace\",\"path\":\"/Id\",\"value\":\"a299e216-dbbe-40e4-b4d4-556d7e7e9c35\"}]";
Copy link
Owner

Choose a reason for hiding this comment

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

There are some static methods on PatchOperation that will help you build a JsonPatch object explicitly rather than having to type out the JSON.

@gregsdennis gregsdennis merged commit f40295e into gregsdennis:patch-diff Sep 13, 2021
@LordXaosa LordXaosa deleted the create-patch-from-two-objects branch September 13, 2021 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants