-
Notifications
You must be signed in to change notification settings - Fork 19
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
Added fix for proper deserializing when nodes are missing #55
base: master
Are you sure you want to change the base?
Conversation
@@ -40,7 +40,7 @@ public static JsonNode serialize(Object value) { | |||
} | |||
|
|||
public static JsonNode convert(JsonNodeFactory nf, Node node) { | |||
if (node.isNull()) { | |||
if (node.isMissing() || node.isNull()) { |
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 produces different behaviour in Flavour and Jackson
@@ -30,7 +30,7 @@ public ArrayDeserializer(Class<?> itemType, JsonDeserializer itemDeserializer) { | |||
|
|||
@Override | |||
public Object deserialize(JsonDeserializerContext context, Node node) { | |||
if (node.isNull()) { | |||
if (node.isMissing() || node.isNull()) { |
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.
I don't understand why you add this 'isMissing' check to every type of deserializer instead of checking at at property deserialization level. Also, missing != null, they are different and must induce different behaviour.
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.
I understand that missing is undefined (that corresponds to undeclared field in JSON). Currently to avoid issues, I need to write "property": null to nullable properties. Jackson handles missing properties well, TeaVM deserializer don't. So this is the reason why I added this isMissing check. Actualy it can be simplified to value == null, but I didn't want to introduce non-standard method in JSON model.
My idea is that int value should fail during deserialization, but Integer value shouldn't because when it's undeclared and I emphasized that it's nullable. Currently it fails and forces me to put lots of "property": null redundant declarations into my model.
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 does not answer my question. I can't approve this PR until you resolve the issue I'm trying to address.
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.
I added it near isNull check. So as I understand, you're suggesting to move it out deserializers and remove all node.isMissing and node.isNull checks in deserializers?
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 exactly. 'null' check is ok here, because 'null' is valid JSON value. And 'null' in JSON ALWAYS means 'null' in Java, while missing property does not always mean 'null' in Java.
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.
Someone already tried to fix this issue, but his patch was incomplete, nor it included any tests.
No description provided.