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

New unit tests for Strict mode using JSONTokener and JSONParserConfiguration #934

Conversation

Simulant87
Copy link
Contributor

@Simulant87 Simulant87 commented Jan 8, 2025

Adding new test cases with invalid characters at th end of the input to validate that the strict mode is enforced by the constructors of JSONObject and JSONArray. Turns out, that the constructor with JSONTokener instead of String input does not enforce the strict mode at the end of the object creation.

demonstrating, but not fixing, the bug behavior described in #935

…NTokener and strict mode

they are failing, because the expected strict mode is not validated
@stleary
Copy link
Owner

stleary commented Jan 9, 2025

Unit tests are failing, was this tested locally before committing?

@Simulant87
Copy link
Contributor Author

Simulant87 commented Jan 9, 2025

The PR is not intended to be merged right away, while still having a complete green test result. So the currently failing behavior of 2 out of 4 provided tests is intended to demonstrate the current bug behavior and describe the expected behavior after a fix, which I haven't provided.

I provided 2 tests each for JSONObject and JSONArray. Each with the same String input to parse a valid Object/Array, followed by additional invalid parameters and having the strict mode enabled. So all tests are expected to throw an exception, because of the strict mode enabled and the invalid characters at end.
The plain String input works as expected, while the same String input wrapped by a JSONTokener doees not throw the expected exception. This is demonstrated by this PR.

I did not provide a fix for this bug. I tried adding this kind of check here. Although it makes my provided failing test green, it lead to other existing tests to fail, where nested Objects are parsed. As at the end of the nested object, the following characters for the outer object are matched incorrectly eagerly as invalid content following the nested object. To fix this I assume the JSONTokener should carry some kind of nesting/indent-level information, which has to be check as well and the check should only be applied at the outer most Object/Array level.

@stleary
Copy link
Owner

stleary commented Jan 9, 2025

I think I understand what you are saying. However, the unit tests failures are hidden behind unknown symbol errors, because you did not include this line in JSONParserConfigurationTest.java:

import org.json.JSONTokener;

@Simulant87
Copy link
Contributor Author

Simulant87 commented Jan 9, 2025

right, I forgot to push the missing import. should be fixed now.

@stleary stleary changed the title Bug: Strict mode is not checked on constructor using JSONTokener and JSONParserConfiguration New unit tests for Strict mode using JSONTokener and JSONParserConfiguration Jan 11, 2025
@stleary
Copy link
Owner

stleary commented Jan 11, 2025

@Simulant87 These are good tests, but 2 of them will need to be updated to match actual exception messages, after #937 is merged

@Simulant87
Copy link
Contributor Author

Thank you, I could update them after #937 is merged, or as an alternative I integrated everything togher in #938

@Simulant87
Copy link
Contributor Author

This PR should be closed in favour of #938 (when that one is merged).

@stleary stleary added Fixed and removed In review labels Jan 17, 2025
@stleary
Copy link
Owner

stleary commented Jan 19, 2025

Closed due to issue resolved in another commit

@stleary stleary closed this Jan 19, 2025
@stleary stleary removed the Fixed label Jan 19, 2025
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.

2 participants