respect the lenient flag in Gson.fromJson

[sgbrown]

This fix causes Gson to respect the lenient flag without affecting the default behavior. Gson will continue to be lenient by default, but now the lenient flag will be respected if it is set to false in the GsonBuilder. The lenient flag will also be respected from a JsonReader. It is unfortunate that the default behavior of Gson and the default behavior of JsonReader are different with respect to the lenient flag; however, this seems to be the only solution that will respect the settings used without drastically changing the expected behavior.

The following is an explanation of some of the issues with the behavior prior to my changes. I think Gson.lenient flag probably does more harm than good in the previous state. The comments on the GsonBuilder.setLenient() method say

By default, Gson is strict and only accepts JSON as specified by RFC 4627.

The comments on Gson.setLenient() also references the JsonReader.setLenient(boolean) method which has a great explanation of what should and shouldn't be considered when using the lenient flag.

The unfortunate fact here is that due to the Gson.fromJson(JsonReader, Type) method always setting the lenient flag to true when parsing, almost none of what is commented in GsonBuilder about the default behavior of Gson is true. The only affect that setting the lenient flag on Gson will do is to allow comments to be at the end of your json buffer when calling Gson.fromJson(Reader, Type) since the assertFullConsumption(Object, JsonReader) method will be called from here and only checks for data at the end of the buffer having not been consumed (e.g. comments at the end).

Consider, for example, the test in com.google.gson.functional.LeniencyTest

    @Override
   protected void setUp() throws Exception {
     super.setUp();
     gson = new GsonBuilder().setLenient().create();
   }

   public void testLenientFromJson() {
     List<String> json = gson.fromJson(""
         + "[ # One!\n"
         + "  'Hi' #Element!\n"
         + "] # Array!", new TypeToken<List<String>>() {}.getType());
     assertEquals(singletonList("Hi"), json);
   }

If you were to remove the comment at the end of the String ("# Array!"), then it would not make any difference at all whether or not the lenient flag had been set. The following would also pass:

     List<String> json = new GsonBuilder().create().fromJson(""
         + "[ # One!\n"
         + "  'Hi' #Element!\n"
         + "]", new TypeToken<List<String>>() {}.getType());
     assertEquals(singletonList("Hi"), json);

It seems counterintuitive that with the lenient flag unset, comments in the middle of the JSON data would be ignored but comments at the end would cause a failure when the Gson parser is supposed to be "strict".

[JakeWharton]

This is an API incompatible change.

[sgbrown]

In order to maintain compatibility, we could add a setLenient(boolean) method to GsonBuilder in addition to keeping the setLenient(void) method even though calling the setLenient(void) method would not have much effect. Perhaps the setLenient(void) method should also be marked deprecated in order to discourage usage of a method that has little effect.

I also considered adding a setStrict(void) method which would set the lenient flag to false instead of having the setLenient(boolean) method

Would you be okay with either of these solutions?

[JakeWharton]

The unfortunate fact here is that due to the Gson.fromJson(JsonReader, Type) method always setting the lenient flag to true when parsing

I've found it a best practice to avoid using convenience methods on Gson for this very reason. The Gson instance just becomes a place to lookup type adapters.

[sgbrown]

I've found it a best practice to avoid using convenience methods on Gson for this very reason. The Gson instance just becomes a place to lookup type adapters.

Having the behavior of Gson force a best practice of avoiding using convenience methods seems contrary to the first goal listed in the gson goals

Also, it may be worth referring to a comment @inder123 made on pull request #806 when considering your suggestion that Gson should just be used as a place to lookup type adapters:

Gson.toJson()/fromJson() work really well for most use-cases. Getting an adapter and using it is cumbersome, and sometimes doesn't produce correct results (I have had issues in the past where it will serialize nulls incorrectly, or not pretty print).

[inder123]

No need to deprecate.

[inder123]

👍 I like this change as it is a simple, backward-compatible way to enable a strict mode.
@swankjesse @JakeWharton Any concerns from your side? I am ready to merge this.

[sgbrown]

I just incorporated the comment from @inder123 about not deprecating the GsonBuilder.setLenient(void) method; and rebased and squashed my commits.

[swankjesse]

It's a good solution, well implemented.

I'm reluctant to change behavior like this in Gson. I think it's incompatible in potentially surprising ways.

An alternative is to recommend avoiding the convenience methods on Gson that suffer this problem. That's what I'll do in my applications. It's also what I recommend anyone who cares about performance does. Doing extra work for every serialized object is sloppy.

Since I'm not using these APIs, I'm okay if we merge this.

[sshock]

Why wasn't this ever merged? It is very confusing to a newcomer that Gson is supposedly strict by default (as indicated both by comments and by the fact that DEFAULT_LENIENT = false), yet in reality it is not strict by default, at least not when using fromJson(), and there is also no way to make this strict.

[JakeWharton]

Consider using Moshi where we fixed this behavior.

…

On Wed, Sep 9, 2020, at 7:42 PM, Phillip Hellewell wrote: Why wasn't this ever merged? It is very confusing to a newcomer that Gson is supposedly strict by default (as indicated both by comments and by the fact that DEFAULT_LENIENT = false), yet in reality it is not strict by default, at least not when using fromJson(), and there is also no way to make this strict. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#804 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQIEK2IXJW56R36HY2PEDSFAHG5ANCNFSM4B5AN3QQ>.

[sshock]

Consider using Moshi where we fixed this behavior.

Good to know, but is there a reason not to merge this for Gson? Is it deemed too risky? If so, maybe the least we could do here is add a comment or something to indicate that the convenience methods like fromJson() are always lenient.

[JakeWharton]

It will break people who rely on the existing behavior, which is why Gson is effectively frozen in time.

…

On Wed, Sep 9, 2020, at 8:36 PM, Phillip Hellewell wrote: > Consider using Moshi where we fixed this behavior. Good to know, but is there a reason not to merge this for Gson. Is it deemed too risky? If so, maybe the least we could do here is add a comment or something to indicate that the convenience methods like fromJson() are always lenient. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#804 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQIEKQ4N6Z5DVAZUGA2EDSFANRTANCNFSM4B5AN3QQ>.

[Marcono1234]

This seems to be obsolete with the Strictness mode API introduced by #2437 for Gson version 2.11.0, in particular the new method GsonBuilder#setStrictness.

Thanks nonetheless for the work you have put into this!