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

Fix interpretation of trailing comma when setLenient() is true. #494

Closed
GoogleCodeExporter opened this issue Mar 19, 2015 · 15 comments
Closed

Comments

@GoogleCodeExporter
Copy link

The following code exhibits the issue:

    StringReader reader = new StringReader("{foo: ['bar', 'baz',]}");
    JsonReader jsonReader = new JsonReader(reader);
    jsonReader.setLenient(true);
    JsonParser parser = new JsonParser();
    JsonElement root = parser.parse(jsonReader);
    assertEquals(
        "Trailing comma should be ignored like in ECMAScript 5.",
        2,
        root.getAsJsonObject().get("foo").getAsJsonArray().size());

Currently, GSON returns an array that corresponds to:

    ["bar", "baz", null]

Instead of:

    ["bar", "baz"]

This is atrocious. The current behavior is consistent with bad browser behavior 
from the last decade. The expected behavior is consistent with ECMAScript 5, as 
well as any programmer who has ever wanted trailing commas to be ignored so 
that it is easier to write lists of things. Here is more information on the 
badness of the current JSON spec:

    http://bolinfest.com/essays/json.html

Please, please fix this. Anyone who uses JSON for a config language wants to be 
able to write data like this:

   [
     "foo",
     "bar",
   ]

such that it is possible to add something to the list without having to clean 
up the comma from the previous line. The current behavior is incredibly 
counterintuitive.

Also, while we're on the subject, the trailing comma should also be supported 
for map entries:

  {
    "foo": 42,
    "bar": 24,
  }

Again, this makes it considerably easier to maintain JSON data files.

Original issue reported on code.google.com by bolinf...@gmail.com on 6 Jan 2013 at 11:58

@GoogleCodeExporter
Copy link
Author

I'm worried about breaking others by making such a change. Our current behavior 
was initially implemented to be consistent with the json.org reference 
implementation.

That said, it would a good idea for a 3.0 release, where we can document that 
we're breaking things in the name of forward progress.

Original comment by limpbizkit on 4 Feb 2013 at 3:59

  • Added labels: Type-Enhancement, Milestone-Release3.0
  • Removed labels: Type-Defect

@GoogleCodeExporter
Copy link
Author

Yes please. I just got burned by this. No parse error, but runtime error by the 
thing that consumed my JSON array because there was an unexpected null at the 
end.

Original comment by bolinf...@gmail.com on 21 Mar 2013 at 11:31

@GoogleCodeExporter
Copy link
Author

This is the only issue that is tagged 3.0. Is 3.0 actually on the horizon?

Original comment by bolinf...@gmail.com on 19 Aug 2014 at 5:23

@GoogleCodeExporter
Copy link
Author

IMHO allowing the trailing comma should be separated from the lenient mode. I'd 
never come to the idea of using lenient mode with features like "Array elements 
separated by ; instead of ," - that's plain awful. But trailing commas is 
something what happens all the time when editing, especially with maps.

Original comment by Maaarti...@gmail.com on 27 Aug 2014 at 3:48

@BoD
Copy link

BoD commented Feb 4, 2017

I've just been bitten by this.
Just to clarify: I am not using jsonReader.setLenient(true); but don't get an exception while parsing files with trailing commas - is it expected behavior?

@BoD
Copy link

BoD commented Feb 4, 2017

Wow I've just seen bug #401 and from what I understand, jsonReader.setLenient is simply ignored, and therefore (please correct me if I'm wrong),
1/ trailing commas are always valid
2/ they are interpreted as null items
3/ there's no way to change this

🙀

@swankjesse
Copy link
Collaborator

🙀

BoD added a commit to BoD/android-contentprovider-generator that referenced this issue Feb 5, 2017
@chris-henderson-alation

Just accidentally stumbled across this while writing tests for our Hadoop REST bindings. It was incredibly surprising and we were lucky to accidentally trigger the bug.

Please do correct this, it's not what anybody expects.

@inder123
Copy link
Collaborator

We will not support this behavior change by default. Write better JSON.
We could consider a GsonBuilder property ignoreTrailingCommas() but that looks hidious.

@BoD
Copy link

BoD commented Oct 10, 2018

You mean more hideous than the current "happily parse invalid JSON without a warning and result in a very surprising results that nobody should reasonably expect" behavior?

@JakeWharton
Copy link
Contributor

You can use getAdapter().fromJson() to work around our bad default of using lenient parsing.

@mitasov-ra
Copy link

mitasov-ra commented Dec 27, 2022

Stumbled across this bug just now and really surprised that this issue is closed.

As @BoD said this behaviour, or I really should say this bug, is not a joke, it's a serious thing, causing unexpected hard to catch bugs. And yes, it reproduces even if lenient is false

I really hope this bug will be revisited, analysed and fixed without somewhat insulting "Write better JSON" answers.

@eamonnmcmanus
Copy link
Member

I do think there's a good case to be made for a proper "strict mode" in Gson, which would accept only fully-conformant JSON input. As noted in the documentation for JsonReader.setLenient, there are some things that Gson currently allows even when lenient is false. (Trailing commas should probably be added to that list.)

We couldn't make strict mode be the default, because that would risk breaking existing users, but we could certainly recommend its use.

I logged #2295 to track having a strict-mode feature.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Jan 8, 2023

there are some things that Gson currently allows even when lenient is false. (Trailing commas should probably be added to that list.)

@eamonnmcmanus, are you sure about that? For me a non-lenient JsonReader rejects a trailing comma with a MalformedJsonException. See also the relevant code:

// fall-through to handle ",]"
case ';':
case ',':
// In lenient mode, a 0-length literal in an array means 'null'.
if (peekStack == JsonScope.EMPTY_ARRAY || peekStack == JsonScope.NONEMPTY_ARRAY) {
checkLenient();

@mitasov-ra, please follow the advise of the "Lenient JSON handling" section of the Gson class documentation to get non-lenient behavior, except for derivations from the JSON specification listed by JsonReader.setLenient, which were mentioned above.

@eamonnmcmanus
Copy link
Member

For me a non-lenient JsonReader rejects a trailing comma with a MalformedJsonException.

So is the earlier comment simply wrong? We probably still should have a strict mode for the other reasons detailed in #2295, but if trailing commas are already disallowed when not using lenient mode I think that is perfectly fine. I personally am skeptical whether anybody should ever use lenient mode at all, since it recognizes a more or less random superset of correct JSON.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants