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

[JOHNZON-277] Fails to deserialize inner empty JSON block {} at OBJECT_START #45

Merged
merged 1 commit into from
Sep 21, 2019

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented Sep 19, 2019

@rmannibucau Before filing another bug report I think it is better to show you this draft. It seems this time I really detected a bug. So we can discuss BEFORE posting this code to JIRA. :-)

@rmannibucau
Copy link
Contributor

Agree, our offsetting does not handle that

@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

@rmannibucau Thanks for your confirmation. :-)

Filed JIRA https://issues.apache.org/jira/browse/JOHNZON-277.

@mkarg mkarg changed the title DeserializerContextTest [JOHNZON-277] Fails to deserialize inner empty JSON block {} at OBJECT_START Sep 20, 2019
@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

@rmannibucau Shall I switch PR from draft state to PR state, so you can merge the test before there is bug fix developed?

@rmannibucau
Copy link
Contributor

@mkarg depends if you want to fix it or not but sure while it does not break the default build (@ignore can be fine if you dont want to do the fix yourself)

@rmannibucau
Copy link
Contributor

rmannibucau commented Sep 20, 2019

--- a/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/serializer/JohnzonDeserializationContext.java
+++ b/johnzon-jsonb/src/main/java/org/apache/johnzon/jsonb/serializer/JohnzonDeserializationContext.java
@@ -82,6 +82,8 @@ public class JohnzonDeserializationContext implements DeserializationContext {
                 return JsonValue.NULL;
             case VALUE_NUMBER:
                 return jsonp.createValue(parser.getBigDecimal());
+            case END_OBJECT:
+                return builderFactory.createObjectBuilder().build();
+            case END_ARRAY:
+                return builderFactory.createArrayBuilder().build();
             default:
                 throw new JsonParsingException("Unknown structure: " + next, parser.getLocation());
         }

seems enough, do you want to give it a try?

@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

@rmannibucau LOL, I actually developed exactly the same solution already :-D

Works like a charm, just pushed the commit.

@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

Ah, seems my solution is slightly different. As I know for sure that the caller was at START_OBJECT before, I simply return a constant.

@mkarg mkarg marked this pull request as ready for review September 20, 2019 12:42
@rmannibucau
Copy link
Contributor

@mkarg constant is better, just misses array case I guess

@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

The array case actually cannot happen. The switch is invoked only from deserialize, which in turn is only valid to be invoked for cursor standing at KEY_NAME or START_OBJECT. So END_ARRAY can never happen. If it does happen, cursor was at invalid location when calling, hence it is more correct to throw exception than to return an empty array.

@rmannibucau
Copy link
Contributor

Hmm,need to check if we cant set a deserializer in an array or collection

@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

@rmannibucau You are right. I checked the Javadocs and the same problem can occur with arrays. I will fix.

Custom deserializer fails with inner empty JSON block {} / [] at START_OBJECT /
START_ARRAY.

Signed-off-by: Markus KARG <markus@headcrashing.eu>
@mkarg
Copy link
Contributor Author

mkarg commented Sep 20, 2019

@rmannibucau Fixed END_ARRAY case, too, and covered it with a unit test. Interesting side aspect: Discovered a different but related bug with empty arrays, so I actually had to ignore a piece of the test. Will look inside that other bug later and cover it with a second PR. Can you please meanwhile review and merge this one? That would be great because my team is stuck due to that original bug and waits for the Johnzon nightly build containing my fix.

@rmannibucau rmannibucau merged commit 589fb9e into apache:master Sep 21, 2019
@mkarg mkarg deleted the deserializer branch September 21, 2019 09:43
@mkarg
Copy link
Contributor Author

mkarg commented Sep 21, 2019

@rmannibucau Thanks for merging. I really appreciate your supportive and agile way to onboard new contributors! 👍 Next on my list is looking into that new bug I mentioned.

@rmannibucau
Copy link
Contributor

@mkarg some people did it for me so it is the least I can do now, in particular when relevant ;). Thanks a lot for doing the PR in addition to investigate the issue, it is very appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants