Fix for invalid processing of trailing / for JSON Pointer#411
Conversation
|
@erosb Please review since you are most intimate with JSONPointer. |
edfbda0 to
2362c93
Compare
|
Thank you for fixing this @johnjaylward . I'm a bit sorry to see there was no simpler way to do it. Do you also plan to add a unittest? |
|
New unit tests are available. I think I covered the cases. |
|
There may be an easier way, but I figured this should be fast enough given how modern JVM and string library implementations handle substrings. |
|
@johnjaylward Breaks a unit test, can you investigate? |
|
The test was using incorrect syntax for the unit test. To get the item in a JSONArray at index 1 it was using I updated the test case and it should pass now. |
|
Hello, thank you for putting effort into fixing this. |
|
Accepted, starting 3 day comment window. |
What problem does this code solve?
Fixes #410.
String.splitdoes not take into account trailing nulls when splitting. It instead trims them off and they are "lost".Risks
Moderate. This is a bug fix to account for faulty processing of the token string in a JSONPointer expression. There is some change to existing behavior.
Changes to the API?
No
Will this require a new release?
Yes
Should the documentation be updated?
No
Does it break the unit tests?
Yes. One test case accidentally had a trailing
/that causes an error with the corrected code. That test case has been corrected to use the correct JSONPointer expression. There were no unit tests to cover this type of input as expected input. New test cases have been created in stleary/JSON-Java-unit-test#85Was any code refactored in this commit?
Yes. I replaced the String.split call with custom processing of the token string.
Review status
ACCEPTED
Changes to existing behavior are not usually accepted, but in this case the new code changes an edge case of a new feature. For more information please see the updated Wiki page: https://github.com/stleary/JSON-java/wiki/FAQ#what-kind-of-changes-will-be-accepted-for-this-project