-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
StrictMode Implementation for JSONArray #877
Conversation
…serConfiguration docs(stleary#871-strictMode): add javadoc
…tion test(stleary#871-strictMode): initial test implementation
…otes implementation
@rikkarth Thanks for the PR. This project maintains backwards compatibility with Java 6 for the benefit of Android developers. Please look into updating the code to compile with Java 6. Also, please revert all refactorings that are not directly related to the purpose of this PR. |
@stleary thank you for looking into my PR and for providing your feedback. I have done as you requested. Let me know if there's any further action you require. |
Please revert all refactorings that are not directly related to the purpose of this PR. Currently, there are about 3000 changed lines of code in your PR, which is too large for an effective review. |
Sorry I now understand what you mean. I will do it and provide a more compliant commit. Thank you. |
Ok, it should be a lot more manageable now. I left only the changes that I consider directly related to this PR. Sorry for the bloat before. |
@rikkarth Due to a recent merge, this code is out of date, please merge from the latest master branch |
Good morning, @stleary, Done as requested. Let me know if there's anything else you need. Have a good day. |
Changing JSONTokener and the parsing code can be risky. These changes typically require additional review because some contributors are not fully up to speed on how parsers work, and the unit tests do not have good coverage of parser internals. Previous experience has shown that seemingly simple parser changes can result in unexpected regressions. Going forward you can expedite the review process by explaining your reasoning for changes to critical sections of the code, either in the PR or as comments in the code itself. Alternatively, you can try an implementation that minimizes changes to the parsing code. In the meanwhile, I will continue reviewing this code. |
I will apply the modifications you've suggested at my first opportunity. Thank you for the review. ✌️🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing your feedback thoroughly I've added new changes based on your input.
Please let me know if you have any further questions or requests. :)
…ements and simplification
test(stleary#871-strictMode): adjusted related tests, add more test cases for non-compliant quotes in strict mode
@rikkarth Also, FYI, a recent commit that modified JSONTokener is causing a conflict. Please merge from the latest master branch and resolve the conflict, I don't think it will be a complex merge. |
} | ||
case '{': | ||
this.back(); | ||
return getJsonObject(jsonParserConfiguration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert code in this method that is not needed for this feature. I.E. lines 435-440, except for passing the JsonParserConfiguration object to the JSONObject and JSONArray ctors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Notes to myself for a future PR, no action needed at this time.
|
chore: removed PII from json sample chore: JSONParserConfiguration.java cleanup chore: JSONTokener.java nextValue partial rollback
Thank you again mr @stleary . I've performed all the changes as requested, please let me know if there's anything else that should be adjusted or changed and thank you for your incredible support in this PR. |
What problem does this code solve? Does the code still compile with Java6? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status Starting 3-day comment window Thanks @rikkarth this was a lot of work, it looks good. |
JSONArray construction improved to recursive validation JSONTokener implemented smallCharMemory and array level for improved validation Added new test cases and minor test case adaption
This fixes issue #871
StrictMode Implementation for JSONArray
JSONParserConfiguration
Followed the same pattern already in place in JSONParserConfiguration.
Implementation Details
JSONArray main constructor now requires JSONParserConfiguration which controls the behaviour of the JSONArray.
If no JSONParserConfiguration is provided, a default JSONParserConfiguration will be added maintaining all the previous functionality. StrictMode is false/off by default.
2nd Part - Quotes Logic
You can find the quotes logic at the JSONTokener
Unit Tests
You can find my test cases in JSONParserConfigurationTest
Every time a non-compliant case is found while in StrictMode, a JSONException is thrown at the exact position of where the conflict happened.
If a non-compliant case is found while StrictMode is off, then no exception will be thrown and the JSONArray will behave as before.
I've added several test cases on the unit test and created a platform to add more easily, feel free to play with it and add more if I missed some case.
Non Compliant Test Cases Log
Bellow is how the errors will be shown to the user, depending on which type of error the user produced with StrictMode it will attempt to show exactly what happened.
I hope this meets your requirements.
Feel free to contact me for any further clarification,
Ricardo Mendes - a fan of this lib