Strict mode for JSON parsing

[marten-voorberg]

Note that this PR is still a work in progress.

Aims to resolve #2295.

In a non-lenient JSON parser, certain non-spec compliant values are accepted. Thie code in this PR allows you to set a JSONReader to strict mode in which non-spec compliant values are not allowed.

The following non-spec compliant values are accepted by the default JSONReader but not a JSONReader in strict mode:

• Capitalization in keywords. In normal mode, any combination of lower and uppercase letters in the keywords false, true, and null is allowed (e.g. nUlL or FALSE). In strict mode, only the fully lower-case values are allowed.
• JsonReader allows unescaped control characters (U+0000 through U+001F) in normal mode but not in strict mode.
• JsonReader supports the escape sequence \', representing a ' in normal mode. In strict mode, this raises an exception.
• JsonReader supports the escape sequence \LF (with LF being the Unicode character U+000A), resulting in a LF within the read JSON string. In strict mode, this raises an exception.

[marten-voorberg]

The API for setStrict is not final and will definitely be changed to one of the options discussed in #2295.

[MaicolAntali]

Thanks! 😄

[eamonnmcmanus]

Thanks, this is a good start!

We do require new code to follow the Google Java Style Guide, even if not all existing code does. You may be able to configure your IDE to use google-java-format.

[eamonnmcmanus]

Rather than checking strict in all these ifs, I think it would be simpler to start the for loop below at 0 instead of 1. Also, instead of checking strict in the loop, before the loop you could set keywordUpper to be the same as keyword if strict is true. (Maybe renaming it to something like alternativeKeyword.)

[marten-voorberg]

I've rewritten this in the most recent version. Personally, I'm not convinced its cleaner than what was there before so let me know what you think.

[marten-voorberg]

@eamonnmcmanus What do you think of this bit of logic has been rewritten?

[eamonnmcmanus]

I was going to say the same thing as @Marcono1234: if someone calls GsonBuilder.setStrictness, a new method, then we should do the right thing and apply that to toJson and fromJson. (I might have implied otherwise earlier but I have come around to this way of thinking.)

[marten-voorberg]

Thanks for the response. I won't be able to work on this in the coming week but I'll be sure to take look when I'm back.

…

On Sat, 8 Apr 2023, 21:26 Éamonn McManus, ***@***.***> wrote: I was going to say the same thing as @Marcono1234 <https://github.com/Marcono1234>: if someone calls GsonBuilder.setStrictness, a new method, then we should do the right thing and apply that to toJson and fromJson. (I might have implied otherwise earlier but I have come around to this way of thinking.) — Reply to this email directly, view it on GitHub <#2323 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADMQBHAY46OR6GWW6AUMSQDXAG3XZANCNFSM6AAAAAAVG56EYI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[marten-voorberg]

I comitted some changes where the strictness now works as follows:

• By default, a GSON object has a strictness == null. When this is the case, the GSON instance should behave exactly as a GSON instance does in the current release to maintain backwards compatibility.
• If the GSON strictness is set, then this strictness value will be imposed on any writers and readers that are used through the instance, but there old values will be restored when the method returns.

I think if we chose to go this way, we should indeed rename Strictness.DEFAULT to something along the lines of Strictness.LEGACY_STRICT.

I have not updated any of the JavaDoc but will do so once we agree on this approach (or decide to go some other way).

[eamonnmcmanus]

That approach sounds good to me.

[marten-voorberg]

A couple things which we might want to consider:

• Should it be possible to set the strictness to null manually by calling GsonBuilder.setStrictness(null)? I am inclined to disallow this as calling this method means you are actively thinking about stricntess and you should just chose which one you prefer.
• What should setLenient(boolean) do for the JsonWriter and JsonReader? The functionality of passing true is obvious but what should the value be if false is passed? I guess it should be LEGACY_STRICT for backwards compatability reasons although I also see the case for making this method parameterless for consistency with the GsonBuilder or deprecating it as it now seems to be preferable to use setStrictness(LENIENT) instead.

[marten-voorberg]

A couple things which we might want to consider:

* Should it be possible to set the strictness to `null` manually by calling `GsonBuilder.setStrictness(null)`? I am inclined to disallow this as calling this method means you are actively thinking about stricntess and you should just chose which one you prefer.

* What should `setLenient(boolean)` do for the `JsonWriter` and `JsonReader`? The functionality of passing true is obvious but what should the value be if false is passed? I guess it should be `LEGACY_STRICT` for backwards compatability reasons although I also see the case for making this method parameterless for consistency with the `GsonBuilder` or deprecating it as it now seems to be preferable to use `setStrictness(LENIENT)` instead.

@Marcono1234 @eamonnmcmanus Could you comment on the above? Once this is sorted, we can finalize that JavaDoc and then merge it in :).

[Marcono1234]

Sorry for the late response; I don't know what Éamonn thinks about this, but personally I agree with you:

• GsonBuilder.setStrictness(null) should not be possible but instead throw a NullPointerException
• For JsonReader / JsonWriter setLenient(false) use LEGACY_STRICT to remain backward compatible, but deprecate these setLenient methods and in the @deprecated Javadoc tag refer to setStrictness

Also, as side note, this question is still unanswered I think:

One open question was whether you missed marten-voorberg@4f6c4e0 by accident in your pull request. (Might be good to check if any other commit is missing as well.)

[Marcono1234]

@eamonnmcmanus, do you agree with that?

[eamonnmcmanus]

Yes, I agree your earlier comment.

Sorry I haven't been following this more closely. Is it ready to be merged into the strictness branch?

[marten-voorberg]

Sorry I haven't been following this more closely. Is it ready to be merged into the strictness branch?

No problem. I think this can be merged into the branch! @eamonnmcmanus

One open question was whether you missed marten-voorberg@4f6c4e0 by accident in your pull request. (Might be good to check if any other commit is missing as well.)

@Marcono1234 Good point. I've not directly added the commit back in but I have added an equivalent test case that uses the more modern assertThrows(). As far as I can tell, there are no other missing commits.

I've now deprecated setLenient in JsonReader, JsonWriter, and GsonBuilder. I think the next step is to merge it into the strictness branch. After that, we should probably make sure that the JavaDoc is up-to-date everywhere as we changed the functionality quite a bit througout this PR so there might be some lingering, outdated documentation.

[Marcono1234]

Thanks for the changes! Sorry, I hope you weren't waiting for my response. Just to make sure there is no misunderstanding, to me the changes look good.

There might be a few small documentation changes I want to make, and maybe extending the U+2028 and U+2029 tests, but this is definitely not blocking this PR.

As you mentioned, it would be good to merge this to the strictness branch already and then perform any additional changes in separate PRs. The first thing would be merging master back into the strictness branch.

@eamonnmcmanus, what do you think?

[eamonnmcmanus]

OK, my attempts to merge the strictness branch seem to have left it in a not-great state. I will see if I can fix it tomorrow.

[eamonnmcmanus]

I believe the new strictness2 branch now reflects what I should have done with strictness. Please confirm. For now I have removed the deprecation tags from setLenient since they cause compilation failures now that we compile with -Werror. It would make sense to restore the deprecation and fix those failures, either by calling the replacement methods or by using @SuppressWarnings("deprecation").

[Marcono1234]

Thanks a lot for resolving the merge conflicts! Comparing the original state of strictness after the merge with strictness2 using https://github.com/google/gson/compare/9ce668e10bb2aed284cb8b5ad0345f0dbea8670b..strictness2 (I hope that URL is correct), it looks to me like nothing was lost (except the setLenient changes you mentioned).

In the end it would be good to compare strictness2 and master again before merging, just to make sure it does not accidentally revert something from master.