-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
respect the lenient flag in Gson.fromJson #804
Conversation
* | ||
* @return a reference to this {@code GsonBuilder} object to fulfill the "Builder" pattern | ||
* @see JsonReader#setLenient(boolean) | ||
*/ | ||
public GsonBuilder setLenient() { | ||
lenient = true; | ||
public GsonBuilder setLenient(boolean lenient) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an API incompatible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
I've found it a best practice to avoid using convenience methods on |
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:
|
} | ||
|
||
/** | ||
* @deprecated Gson is lenient by default. Use {@link #setLenient(boolean)} if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to deprecate.
👍 I like this change as it is a simple, backward-compatible way to enable a strict mode. |
294fc67
to
b76e3d1
Compare
I just incorporated the comment from @inder123 about not deprecating the GsonBuilder.setLenient(void) method; and rebased and squashed my commits. |
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. |
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. |
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>.
|
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. |
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>.
|
This seems to be obsolete with the Thanks nonetheless for the work you have put into this! |
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
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
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:
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".