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

Make nextString throw a JSONException instead of a NumberFormatExcept… #236

Merged
merged 2 commits into from
Jun 8, 2016

Conversation

madsager
Copy link
Contributor

@madsager madsager commented Jun 2, 2016

…ion for malformed input.

The documentation for the JSONObject(String) constructor says that JSONExceptions are thrown for invalid input. However, the implementation can throw a NumberFormatException as well. This fixes that.

@johnjaylward
Copy link
Contributor

johnjaylward commented Jun 2, 2016

I'd like to see the original NumberFormatException preserved. Could you update the syntaxError method to take an optional Throwable and pass that as the causedBy in the generated JSONException?

@madsager
Copy link
Contributor Author

madsager commented Jun 2, 2016

Good idea. Updated the pull request.

The error will now look something like this:

Exception in thread "main" org.json.JSONException: Illegal escape. at 9 [character 10 line 1]
at org.json.JSONTokener.syntaxError(JSONTokener.java:448)
at org.json.JSONTokener.nextString(JSONTokener.java:284)
at org.json.JSONTokener.nextValue(JSONTokener.java:365)
at org.json.JSONObject.(JSONObject.java:206)
at org.json.JSONObject.(JSONObject.java:319)
at fix.JSONFix.main(JSONFix.java:8)
Caused by: java.lang.NumberFormatException: For input string: "rl":"
at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
at java.lang.Integer.parseInt(Integer.java:580)
at org.json.JSONTokener.nextString(JSONTokener.java:282)
... 4 more

@stleary
Copy link
Owner

stleary commented Jun 2, 2016

Looks reasonable. Let me know if you are planning on updating the unit tests.

@madsager
Copy link
Contributor Author

madsager commented Jun 2, 2016

I didn't see any tests in the repo, so it wasn't clear to me where to add it. I will be happy to add a regression test for this. Is that in a separate repo?

@stleary
Copy link
Owner

stleary commented Jun 2, 2016

Yeah, they are kept in a separate project: https://github.com/stleary/JSON-Java-unit-test

@madsager
Copy link
Contributor Author

madsager commented Jun 2, 2016

Thanks. Pull request created for a simple regression test that check that the right type of exception is thrown.

@stleary
Copy link
Owner

stleary commented Jun 2, 2016

Thank you, will review this evening, if everything seems OK will open a pull request. We generally leave PRs open for about 3 days for comment before merging.

@stleary
Copy link
Owner

stleary commented Jun 3, 2016

What problem does this code solve?
JSONTokener.nextString() JavaDocs state that JSONExceptions are thrown, but NumberFormatExceptions may also be thrown.

Changes to the API?
No.

Will this require a new release?
This change will be rolled into the next release, which is expected in June 2016 timeframe.

Should the documentation be updated?
No, this brings the code in line with the documentation.

Change to unit tests?
A new unit test is added, which will only succeed with the code change. See stleary/JSON-Java-unit-test#49

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