Skip to content

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