Creating a JSONObject from a string that contains a duplicate key (any level) throws a JSONException that includes location

[migueltt]

Constructor JSONObject(JSONTokener) has been updated to provide an error message that includes error location within the source string using local method syntaxError(String).
JUnit test cases has been updated as well (see org.json.unit.JSONObjectTest#jsonObjectParsingErrors())

[stleary]

@migueltt Thanks, will review and get back to you. This looks a little different than the code in the issue you opened a few days ago. Please explain why in your description. Also, if it was incorrect to use putOnce(), you don't need to comment why you are taking it out (i.e. its OK to remove those comment lines from the code).

[migueltt]

The previous pull request didn't consider the recursive constructor usage. Therefore, the syntaxError message was concatenated for every exception within the stack.
This fix addresses that problem.
I'll fix the sourcehcode comments tonight.

[johnjaylward]

It's also replacing flow control via exception with standard flow control. This matches with the other changes we made regarding using exceptions as flow control.

This looks good to me if all the tests are passing.

[migueltt]

I may push another change tonight. I think we can call back() on the JSONTokener to get a better location in syntaxError(String)

[johnjaylward]

back() on the tokener only moves back 1 character, it won't back up to the previous key/value read. Also, if back() was already called in the tokener as part of it's internal process that may throw an exception. You are probably better off not calling it.

[migueltt]

Ok we'll see... Problem is that the error location msg is off a few characters - right now is pointing to the key/value colon (:)

[stleary]

Just a heads-up: Consistently including an error location in JSONExceptions during parsing is a reasonable enhancement. Tweaking the location pointer after the fact to try to get it exactly right without causing subtle regressions will probably be considered an unacceptable risk.

[migueltt]

Adding JSONTokener.back() prior to JSONTokener.syntaxError(..) does not impacts any previous tests. The error message now includes location pointing to the last character of the duplicate key.
JUnit test cases updated accordingly.

[stleary]

What problem does this code solve?
This is an enhancement, not a bug fix.
All of the tokener errors that occur while parsing a JSONObject result in a JSONException with a message that identifies the location in the document, except for one where putOnce() is called for a duplicate key. It would be reasonable to include document location information for any error that occurs while parsing. This change checks for a duplicate key before calling putOnce(), and if found, includes location information in the JSONException.

Risks
Low risk, making this change does not require a behavior change. The exception would have been thrown anyway, but now the exception message has more content.

Changes to the API?
No

Will this require a new release?
This can be rolled into the next release

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
There was minimal refactoring, call to putOnce() was replaced by put().

Review status
ACCEPTED

Starting 3 day timer for comments

[johnjaylward]

For the first point, I agree that I'd rather not call back at that point. I think anyone trying to debug the "Duplicate Key" exception message would easily be able to tell which key was the duplicate without calling it there.

@stleary, for your second point on the code changes, I believe @migueltt explained it here: #360 (comment)

It was because of the way the constructor works with the tokener. It ends up being recursive calls instead of iterative. Maintaining the original putOnce with exception handling only was not effective since it ended up catching and re-wrapping the exceptions as the recursive stack unwound.

The reason for not calling putOnce was to reduce existence checking the key twice (once for the syntax error, then a second time that we know will pass since we just checked it at line 239)

The null check on the value is to maintain consistency with the putOnce method which does not insert null into the map.

[johnjaylward]

In short, I think the only change I'd recommend is removing the call to back(); I don't think it adds any value to the error message here, and I'm wary of any potential problem it may cause due to lack of testing all possible data points.

The other changes I agree with as they match the original functionality and they are only checking the map once to see if the key exists, where calling putOnce after the existence check would do that existence checking twice.

[stleary]

@johnjaylward Updated the review comment.

[migueltt]

Rationale to include JSONTokener.back(): if not used, then the error message points to the colon character.

• If we want this enhancement to be in line with all the other error messages, then the actual location should point to the start index of the first character of the duplicate key. To achieve this, extensive changes should be added into JSONTokener (e.g. package level accessors for line, index, overload syntaxError(..), allowing to pass error location)
• If a JSONTokener is created manually and passed to JSONObject to create an instance, then we would like such instance to keep the pointer where the error is located (not after). At this point this cannot be done since JSONTokener parses character by character (thus, back() cannot point to the duplicate key start)
• By calling back() at least we are restoring state to the JSONTokener instance allowing the caller to continue using it knowing that the next "token" is the one following the error.
• Note that when using constructor JSONObject(String) and errors exists, all the JSONTokener instances are garbage-collected. Therefore, calling back() will not have any subsequent effect.

[johnjaylward]

• I just don't see that moving back 1 character adds any value to the message. Having the error message point to the : instead of white space or the end quote of the key is very minor. I agree that having it point to the start of the duplicate key would be optimal, but as you stated, without major changes to the code, that's not possible.
• If a developer is creating their own JSONTokener and passing it to the JSONObject(JSONTokener) constructor, then I'd rather not call back and let them call it. If they have existing code that is doing this, then calling back() in their code may now fail, where it didn't before.

I think it's best to just remove the back() call and let the error fall where it does naturally as part of the processing here.

[migueltt]

Sure, if that simplifies the whole JSON-java sdk/api, back() will be removed from the pull-request (tonight).

[stleary]

Review updated

[migueltt]

JSONTokener.back() removed from JSONObject(JSONTokener) as recommended by @stleary and @johnjaylward .
JUnit test cases updated as well.