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

Add support for nulls in maps #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

zsperske
Copy link
Collaborator

@zsperske zsperske commented Nov 1, 2017

Putting this up to start a convo about a... bug (?) I'm noticing, not really sure. This fails to pass the delayed object check because when parsing as a JSONObject:

"myStringMapWithSingleNullValue": { "key1": null },

Once the parser reaches the {, the next call to JsonReader's peek method returns JsonToken.NULL. So we end up with a completely empty map. @ndtaylor is that expected? Is "key1" not a token?

@ndtaylor
Copy link
Contributor

ndtaylor commented Nov 2, 2017

There are two problems that need to be addressed. One is that we're putting null into a JSONObject in parseAsJsonObject() and parseSpecificJsonObjectDelayed(), which basically removes the key in the JSONObject. Instead, we should put JSONObject.NULL as the value.

Second, in convertJsonObjectToMap(), jsonObject.opt(name) returns JSONObject.NULL if the value is null. We should use that as our signal to put null into the map we're generating.

@KennethNickles
Copy link
Contributor

Can't maps have a single null key as well or is that a java specific implementation detail?

@ndtaylor
Copy link
Contributor

ndtaylor commented Nov 2, 2017

Can't maps have a single null key as well or is that a java specific implementation detail?

I don't believe the JSON standard allows for a null key. Also, is this in response to the conversation in #6?

@KennethNickles
Copy link
Contributor

Yeah sorry too many prs open.

Copy link
Contributor

@KennethNickles KennethNickles left a comment

Choose a reason for hiding this comment

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

Zach told me to rescind this because of test failures.

@zsperske
Copy link
Collaborator Author

zsperske commented Nov 6, 2017

There are two problems that need to be addressed. One is that we're putting null into a JSONObject in parseAsJsonObject() and parseSpecificJsonObjectDelayed(), which basically removes the key in the JSONObject. Instead, we should put JSONObject.NULL as the value.

Second, in convertJsonObjectToMap(), jsonObject.opt(name) returns JSONObject.NULL if the value is null. We should use that as our signal to put null into the map we're generating.

Back from KotlinConf. Let me see I'm following. In parseAsJsonObject() we call parseNextValue(), are you talking about returning JSONObject.NULL instead of just null from case NULL:? Pushed that change up but it comes with a couple problems.

"myNullInt": null hits java.lang.NumberFormatException: For input string: "null" with this generated code:

out.myNullInt = Integer.valueOf(jsonObject.optString("myNullInt"));

And "key1": { "myString": null } parses the same as "key3": { "myString": "null" }

@ndtaylor
Copy link
Contributor

ndtaylor commented Nov 6, 2017

In parseAsJsonObject() we call parseNextValue(), are you talking about returning JSONObject.NULL instead of just null from case NULL:? Pushed that change up but it comes with a couple problems.

This is what I meant.

@zsperske
Copy link
Collaborator Author

zsperske commented Nov 6, 2017

This is what I meant.

Ok cool, seems pretty similar to what I was doing. It still has the problems I listed above though. "Key1" parses to com.workday.autoparse.json.demo.SimpleTestObject@342c38f8[myString=post-parse:null,discrimValue=<null>]

@ndtaylor
Copy link
Contributor

ndtaylor commented Nov 7, 2017

"Key1" parses to com.workday.autoparse.json.demo.SimpleTestObject@342c38f8[myString=post-parse:null,discrimValue=]

TestObject has an OnPostCreateChild() method that changes the value of myString. So it's working as intended.

@zsperske
Copy link
Collaborator Author

zsperske commented Nov 7, 2017

TestObject has an OnPostCreateChild() method that changes the value of myString. So it's working as intended.

I looked at that method, and it only does that if the String value isn't null. I would have expected it to be null and not have that bit added. I'll look into it more.

@ndtaylor
Copy link
Contributor

ndtaylor commented Nov 7, 2017

I looked at that method, and it only does that if the String value isn't null.

Lol. Yup, there is something else going on there.

For the test that's failing because of a NumberFormatException, it's trying to convert null to an int, which doesn't work. I'd say it's questionable to allow null values in the JSON for primitives. If you (as a user of Autoparse) really want to allow for that, then I'd annotate a setter that takes a String, and have the setter handle the null value. Is this a use case you've actually run into?

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

Successfully merging this pull request may close these issues.

3 participants