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

update-jsonpath: update jsonpath from 2.4.0 to 2.9.0 #894

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

stleary
Copy link
Owner

@stleary stleary commented May 15, 2024

What problem does this code solve?
Fixes #893 by updating jsonpath, which is only used for unit tests, from 2.4.0 to 2.9.0.

Does the code still compile with Java6?
Yes

Risks
Low

Changes to the API?
No

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No

Was any code refactored in this commit?
No executable code was changed

Review status
APPROVED - by myself

Starting 3-day comment window

@javadev
Copy link
Contributor

javadev commented Jun 6, 2024

@stleary
Copy link
Owner Author

stleary commented Jun 6, 2024

PRs can be submitted and approved but merges will remain pending until #884 and #891 are completed.

Copy link

@StacySager1977 StacySager1977 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not merge for me anyone know why?. Thank you 😊

@stleary
Copy link
Owner Author

stleary commented Jun 22, 2024

@StacySager1977 Only the repo owner is allowed to merge changes. This merge is being temporarily held back for the reasons stated above.

@rikkarth
Copy link
Contributor

PRs can be submitted and approved but merges will remain pending until #884 and #891 are completed.

is there any action required? i'm under the impression both have been addressed

@stleary
Copy link
Owner Author

stleary commented Jun 23, 2024

@rikkarth, Yes, I think additional action is needed. First, apologies to all for not addressing this sooner. A confluence of one-time events has consumed all of my free time for the past month, but I have cycles available now to dig deeper.
I added a note to #884 that I think demonstrates that invalid trailing chars have not been addressed yet for JSONObject. That should be easily fixable, but first let's make sure the existing implementation is the best we can come up with.

The purpose of strict mode is, I think, actually fairly simple: to enforce double quotes around strings and disallow invalid trailing chars at the end of the parsed document. Also, during parsing, we have to make sure that the JSONParserConfiguration object is passed or default initialized wherever it might be needed. If I missed anything else, please remind me.

For string checking, one would think that would be done in JSONTokener.nextString(), but it was not. Instead, a new method, parseUnquotedText() is added. This does not seem like the simplest and most direct implementation to me. For example, by doing the work in nextString(), we would not need JSONTokener.smallCharMemory, or the methods that manipulate it.

For trailing chars, we need to separate the top-level array or object from nested instances. Recall, we are only concerned with raw text parsing. Wouldn't the String and Tokener constructors be the best place to do this? Then we would not need JSONTokener.arrayLevel.

Regarding the tests, I still think that it's fair to limp along for now with manual strict mode tests, but as mentioned above, the missing JSONObject tests must be filled in.

In summary, some missed work in JSONObject, many missing JSONObject unit tests, and some concerns that the implementation could be much simpler and more direct.

@rikkarth
Copy link
Contributor

rikkarth commented Aug 1, 2024

@rikkarth, Yes, I think additional action is needed. First, apologies to all for not addressing this sooner. A confluence of one-time events has consumed all of my free time for the past month, but I have cycles available now to dig deeper. I added a note to #884 that I think demonstrates that invalid trailing chars have not been addressed yet for JSONObject. That should be easily fixable, but first let's make sure the existing implementation is the best we can come up with.

The purpose of strict mode is, I think, actually fairly simple: to enforce double quotes around strings and disallow invalid trailing chars at the end of the parsed document. Also, during parsing, we have to make sure that the JSONParserConfiguration object is passed or default initialized wherever it might be needed. If I missed anything else, please remind me.

For string checking, one would think that would be done in JSONTokener.nextString(), but it was not. Instead, a new method, parseUnquotedText() is added. This does not seem like the simplest and most direct implementation to me. For example, by doing the work in nextString(), we would not need JSONTokener.smallCharMemory, or the methods that manipulate it.

For trailing chars, we need to separate the top-level array or object from nested instances. Recall, we are only concerned with raw text parsing. Wouldn't the String and Tokener constructors be the best place to do this? Then we would not need JSONTokener.arrayLevel.

Regarding the tests, I still think that it's fair to limp along for now with manual strict mode tests, but as mentioned above, the missing JSONObject tests must be filled in.

In summary, some missed work in JSONObject, many missing JSONObject unit tests, and some concerns that the implementation could be much simpler and more direct.

Will address as much as possible next week.

@rikkarth
Copy link
Contributor

rikkarth commented Aug 6, 2024

Currently addressing issue #884 in PR #903

@stleary stleary merged commit e7e52da into master Nov 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update JsonPath version
4 participants