Repository navigation
support JSON Merge Patch (RFC 7396) diff creation - #4965
satelliteprogrammer wants to merge 4 commits into
Conversation
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @satelliteprogrammer |
|
I am not sure if this feature would be widely used, so I'm opening a discussion. |
|
We had a (somewhat niche) need for this at work, that's why I've contributed here. It's essentially a combination of 2 things:
For very large data structures, reading through a log line to find the one variable that did change is a pain. Not only that, but on some interfaces only a small amount of member variables on the entire structure are actually changing. |
|
This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions! |
We had this exact need as well: logging changes in simple JSON structures without all the noise of the formal JSON Patch syntax. Would have been convenient to have this capability built in. |
| if (diff.is_null()) | ||
| { | ||
| JSON_THROW(other_error::create(503, detail::concat("cannot set \"", itf.key(), "\" to null"), &target)); | ||
| } |
There was a problem hiding this comment.
This may be overly constraining. An alternative would be to interpret "field set to null" as "field removed".
The RFC isn't very clear on this. While it says:
This design means that merge patch documents are suitable for describing modifications to JSON documents that primarily use objects for their structure and do not make use of explicit null values.
It also lists examples in appendix where some fields are set to null in the source JSON. So they may have meant "do not make use of explicit null values" as "do not attribute specific meaning to explicit null values".
There was a problem hiding this comment.
I've changed the if to explicitly check if we have a transition from !null -> null. That one is impossible to generate, as it would collide with the actual meaning of null in the patch file.
Null values in the merge patch are given special meaning to indicate the removal of existing values in the target.
However, the end result is the same. We would reach here if the source != target AND target == null. There's no other way for the diff to be null.
| { | ||
| JSON_THROW(other_error::create(503, detail::concat("cannot set \"", itf.key(), "\" to null"), &target)); | ||
| } | ||
| result[it.key()] = merge_diff(it.value(), itf.value()); |
There was a problem hiding this comment.
Inefficient; merge_diff was already called on the same inputs above and diff could be reused here.
There was a problem hiding this comment.
Thanks! I had figured this one out already, but since the PR wasn't going anywhere I didn't fix it here.
| auto itf = target.find(it.key()); | ||
| if (itf != target.end()) | ||
| { | ||
| if (it.value() != itf.value()) |
There was a problem hiding this comment.
Inefficient: this will traverse the whole depth of the JSON structure to check for equality of all sub fields, and we'll do that again in merge_diff if going inside the if. This check could be removed, calling merge_diff unconditionally, then checking the output isn't an empty object.
There was a problem hiding this comment.
Though, this will need checking for value types, and only call merge_diff if both source and target are objects.
if (it.value().is_object() && itf.value().is_object())
{
auto diff = merge_diff(it.value(), itf.value());
if (!diff.empty())
{
result[it.key()] = std::move(diff);
}
}
else if (it.value() != itf.value())
{
result[it.key()] = itf.value();
}There was a problem hiding this comment.
this will traverse the whole depth of the JSON structure to check for equality of all sub fields
I realised it, but since it would break on the first inequality, I conceded. What nudged me in this direction was that we were already checking if the target is not an object at the start, so it didn't feel right to check again before calling the recursive function.
I've now realised where I was wrong.
For two non-object identical values, it they are present at the top-level of the JSON, then the patch must contain them, as otherwise it will not apply them.
However, if the identical non-objects are nested within an object, then, even though it would still work, they aren't necessary in the patch file, as the target object will retain its previous values.
I think this difference in behaviour is what requires the extra work within the for-loop. And then you're correct, we need to check the type to make sure we're applying the most efficient comparison.
|
This is a feature that would be very useful to me. Our usecase is we would like to create a hierarchical data storage system that allows users to override values set in a base layer of json, storing user "overrides" as a merge patch. So being able to easily apply and generate merge patch documents would be essential to such a use case. I find the merge patch formatting to be more human-readable than the json patch "action list" format. They are more intuitive to interpret for my usecase, where you are essentially looking at a sparse overlay document. You have the context of the changed element's path in the document providing self-documentation as to the intent of the change. A flat list of change actions lacks some of this context when it isn't structured like a typical document. Thank you for your time, have a nice day. |
|
@cschreib-ibex just in case it's useful for you. You can represent null values iff the source and target represent the same container. In that scenario you don't need null to represent removing existing values. |
af9f513 to
376be93
Compare
|
This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions! |
nlohmann
left a comment
There was a problem hiding this comment.
Thanks for the contribution, and for iterating on the earlier feedback. The feature is well-motivated: several users have asked for it, and Jakarta JSON-P (Json.createMergeDiff) and Python's json-merge-patch (create_patch) offer the same thing. It's also a purely additive API.
However, I found two correctness issues where merge_patch(a, merge_diff(a, b)) != b, and the PR needs some housekeeping before it can be merged. I tested against the PR head (376be93) with this harness:
json a = json::parse(src), b = json::parse(dst);
json c = a;
c.merge_patch(json::merge_diff(a, b));
// expect c == b| source | target | patch produced | result after merge_patch |
|---|---|---|---|
{"a":{"x":1}} |
{"a":[]} |
{} |
{"a":{"x":1}} ❌ |
{"a":{}} |
{"a":[]} |
{} |
{"a":{}} ❌ |
{} |
{"a":{"b":null}} |
{"a":{"b":null}} |
{"a":{}} ❌ |
{"a":1} |
{"a":{"b":null}} |
{"a":{"b":null}} |
{"a":{}} ❌ |
1 |
{"a":{"b":null}} |
{"a":{"b":null}} |
{"a":{}} ❌ |
{"a":{"b":1}} |
{"a":{"b":null}} |
throws other_error.503 |
– |
Details are in the inline comments. To summarize:
Must fix
- Only recurse when both values are objects (the empty-array case above).
- Handle nulls in the target the same way at every depth. Right now a top-level null throws, but a null nested inside an added or replaced value is silently lost.
- Rebase onto current
develop. The branch is ~350 commits behind and conflicts ininclude/nlohmann/json.hpp. - Run
make amalgamate(the amalgamation check failed).
Documentation
- Add
docs/mkdocs/docs/api/basic_json/merge_diff.md, modeled onmerge_patch.mdanddiff.md: signature, parameters, return value, exceptions, complexity, an example indocs/mkdocs/docs/examples/, and a "Version history" entry. Also add it tomkdocs.yml, thebasic_jsonindex page, and the "See also" sections ofmerge_patch.mdanddiff.md. - If the exception stays, document
other_error.503indocs/mkdocs/docs/home/exceptions.md. - Expand the one-line
@briefto the usual@salink to the docs page.
Tests
- Add cases for the rows in the table above.
- Assert the shape of the produced patch in at least some cases, not only the round-trip. That would have caught the empty
{}patch. - Also run the tests with
nlohmann::ordered_json.
This comment was written by Claude Code on behalf of @nlohmann.
|
@satelliteprogrammer Are you willing to continue working on this? |
Hi @nlohmann thanks for the interest and taking the time to review the PR. |
376be93 to
5b0be28
Compare
| auto itf = source.find(it.key()); | ||
| if (itf == source.end()) | ||
| { | ||
| result[it.key()] = it.value(); |
There was a problem hiding this comment.
Nit (non-blocking): when a key is missing from source and its value in target is null, this copies "a": null into the patch. Applying it removes a member that isn't there, so it has no effect. Since the docs say a null member of target is treated as absent, it would be a bit cleaner to leave it out:
if (itf == source.end() && !it.value().is_null())(The test "empty object to object with null value" would then expect {}.)
This comment was written by Claude Code on behalf of @nlohmann.
There was a problem hiding this comment.
I get the point, but wouldn't this also fit the idea of inconsistent null handling?
{} -> {"a":{"b":null}} results in {"a":{"b":null}}, not {"a":{}}, unless I recursively search for nulls.
There was a problem hiding this comment.
Fair point, you're right. Skipping only the top-level null would just move the inconsistency one level down, and stripping nulls recursively isn't worth the extra cost for a patch entry that does nothing anyway. The documented guarantee already excludes targets with null members. Let's keep it as it is, so consider this nit withdrawn.
This comment was written by Claude Code on behalf of @nlohmann.
|
|
||
| ## Complexity | ||
|
|
||
| Linear in the sizes of `source` and `target`, times the cost of looking up a key in an object: logarithmic in the size |
There was a problem hiding this comment.
Nit (non-blocking): this leaves out the cost of comparing changed non-object values: two arrays (or nested arrays inside them) are compared element by element with !=. Something like "plus the cost of comparing non-object values" would make it complete.
This comment was written by Claude Code on behalf of @nlohmann.
There was a problem hiding this comment.
Actually, isn't the cost of comparing non-object values included in the "linear in the sizes of ..."?
For the following 2 JSONs: {"a": [1,2,3]} and {"a": {"1": "one", "2": "two", "3": "three"}, what would you consider to be the size of each?
nlohmann
left a comment
There was a problem hiding this comment.
Great progress! I added some small nits, but otherwise good to go.
Please fix the DCO requirements.
satelliteprogrammer
left a comment
There was a problem hiding this comment.
Let me know what you thing.
I'll sign the last commit when I push again.
|
|
||
| ## Complexity | ||
|
|
||
| Linear in the sizes of `source` and `target`, times the cost of looking up a key in an object: logarithmic in the size |
| auto itf = source.find(it.key()); | ||
| if (itf == source.end()) | ||
| { | ||
| result[it.key()] = it.value(); |
There was a problem hiding this comment.
I get the point, but wouldn't this also fit the idea of inconsistent null handling?
{} -> {"a":{"b":null}} results in {"a":{"b":null}}, not {"a":{}}, unless I recursively search for nulls.
|
Thanks, this looks good to merge from my side. Two things left before I can approve:
This comment was written by Claude Code on behalf of @nlohmann. |
The JSON merge patch document format describes the set of modifications to a resource's content, that more closely mimics the syntax of the resource being modified. However, and in contrast to JSON Patch (RFC 6902), a JSON Merge Patch cannot express certain modifications, e.g., changing an array element at a specific index, or setting a specific object value to null. The null value in a JSON Merge Patch is used to remove the key from the object. The diff algorithm is not part of the RFC 7396, but it was tested against all examples provided, plus additional cases on how null values are handled. Signed-off-by: Luís Murta <luis@murta.dev>
Signed-off-by: Luís Murta <luis@murta.dev>
Also add CHECKs for the patch, documentation and run `make amalgamate`. Signed-off-by: Luís Murta <luis@murta.dev>
Signed-off-by: Luís Murta <luis@murta.dev>
5b0be28 to
93e1ba9
Compare
| { | ||
| if (it.value().is_object() && itf.value().is_object()) | ||
| { | ||
| auto diff = merge_diff(it.value(), itf.value()); |
There was a problem hiding this comment.
This is a new recursive function and doesn't have the depth limit fallback to an iterative function that is being added to all other recursive functions.
There was a problem hiding this comment.
I'll take a look at what changed in the other recursive functions.
| result[it.key()] = std::move(diff); | ||
| } | ||
| } | ||
| else if (it.value() != itf.value()) |
There was a problem hiding this comment.
Should there be handling for arrays here? Otherwise, a one element difference in an array will result in the diff having a full replacement of the array.
There was a problem hiding this comment.
That's the specification.
Also, it is not possible to patch part of a target that is not an object, such as to replace just some of the values in an array.
There was a problem hiding this comment.
Oh, that's unfortunate, but I guess there's nothing to do about it.
The JSON merge patch document format describes the set of modifications to a resource's content, that more closely mimics the syntax of the resource being modified.
However, and in contrast to JSON Patch (RFC 6902), a JSON Merge Patch cannot express certain modifications, e.g., changing an array element at a specific index, or setting a specific object value to null. The null value in a JSON Merge Patch is used to remove the key from the object.
The diff algorithm is not part of the RFC 7396, but it was tested against all examples provided, plus additional cases on how null values are handled.
JSON Merge Patch PR: #876
PR discussing the diff: #2018
If the content is approved, please let me know if/what documentation needs to be updated.
[Describe your pull request here. Please read the text below the line and make sure you follow the checklist.]
make amalgamate.Read the Contribution Guidelines for detailed information.