-
Notifications
You must be signed in to change notification settings - Fork 148
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
Explicit Split of Replace Op | Null Json Node handling #120
Explicit Split of Replace Op | Null Json Node handling #120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes, especially wrt asJson
and whether or not asJsonNullSafe
is an appropriate choice; I think this is worth consideration.
Also, and this isn't why I'm requesting changes on the PR, I'm still trying to wrap my head around the use case; JSON documents are bare data and do not carry any semantic information, I simply can't figure out why you'd want the diff algorithm to do this instead of reasoning about the content in external code. Further, since this is strictly an optional postprocessing step (that relies on a non-RFC extension already present in the codebase), I'm of the opinion that a special use case should remain outside the library, but @vishwakarma might have a different view...
|
||
public static JsonNode asJsonNullSafe(final JsonNode source, final JsonNode target, EnumSet<DiffFlags> flags) { | ||
if (source == null && target == null) { | ||
return asJson(MAPPER.createObjectNode(), MAPPER.createObjectNode(), flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wasteful, might as well return null (since the contract is extended anyway) or an empty array node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Good point. Will optimize for add/remove cases as well
// return add node at root pointing to the target | ||
diff.diffs.add(Diff.generateDiff(Operation.ADD, JsonPointer.ROOT, target)); | ||
} | ||
if (source != null && target == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using else ifs for readability.
@@ -127,44 +127,33 @@ public void testPath() throws Exception { | |||
Assert.assertEquals(target, expected); | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests have been updated. Removed the asJsonNullSafe
method. Since the existing asJson
is enriched with the new contract, it's tests have too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine, now we're just down to "does this make sense in the library". @vishwakarma care to weigh in?
Thanks @isopropylcyanide for raising the diff. My opinion on this diff - |
In that case, are we good to merge this? @holograph @vishwakarma |
Summary
Description
This PR does two things
Adds a new flag
ADD_EXPLICIT_REMOVE_ADD_ON_REPLACE
which splits a replace operation into aremove
followed by anadd
operationUpdates the contract for
asJson
to handle null values for the input source / target nodes which currently blows up in an ugly NPE.Bumps up version to
0.4.11
Motivation & Context
New Flag
For e,g consider a system where users can add and remove their favourite brands. If in a single API operation, removal of existing brand B1 and addition of a new brand B2 is allowed for user ease, the patch contains one replace event.
This, however, needs to be processed into two events
It's not exactly clear how a new addition of a brand can replace the deletion of a previous brand. Both are separate events which need to be captured accordingly. Hence we should provide a flag which lets client make this decision. It is off by default.
Updating contract
The
asJson
doesn't handle null inputs well. Following conditions can be truesource
is nulltarget
is nullboth
are nullboth
are non null, which worksExpectation:
If source is null and target is not, a patch needs to be provided with all
add
events by considering source to be an empty nodeIf target is null and source is not, a patch needs to be provided with all
remove
events by considering source to be an empty nodeIf both are null, the patch should be empty
Reality:
Currently it blows up with an NPE in all 3 cases, even when both are null
To see why the inputs can be null, consider the use case of a change data capture system like Debezium or Maxwell that streams database changes in json. If a row is newly added or deleted, the source and target are null respectively.
Imagine a client trying to compute the diff between
before
andafter
. We shouldn't expect them to create an empty node before calling the differ. Hence, the contract has been updated to automatically handle null as there was no formal specification for the same earlier.How was this tested