added Approved - 3-day window Approved - by myself labels 3 weeks ago

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

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

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

@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, 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.

Currently addressing issue #884 in PR #903