Bug Report JSON trailing comma

General JUCE discussion
1

Hi,
trailing commas are not allowed in the json specification, yet the juce json parser gracefully ignores them.

I created a pr that fixes it here:

However I’m unsure if the JUCE team actually looks at the PRs since some of them are as old as 2017. Could someone from the juce team jump in and clarify what the correct way to report a bug is?

2

In general it’s useful to have parsers that are a bit more lenient than the spec. It doesn’t hurt anything to have a trailing comma in the input file, so why not accept it?

3

This kind of PR is naive, because it assumes that everyone using JUCE is conformant to the spec - and it assumes that the (potentially) tens of thousands of .json files that are out there in use in JUCE projects, will be updated to conform with the change.

If you must enforce the spec in your own projects, to ensure that .json files produced by your project are conformant, then simply add your own qualification code to do that. Please, think about the side-effects on the entire community when you make these kinds of suggestions …

2 Likes
4

For me it created problems because other parsers throw errors while the juce parser handles it just fine. I used the juce implementation to write a sanitizer for a custom json to make sure that all the needed values are set correctly. Further down the line I experienced a crash…

Recent commits 379afb1e and 1e5c8889 already have breaking changes regarding JSON, so I thought it might be a good time to bring this up as well.

However if you still think it’s naive I’ll close and use another JSON library :grimacing:

5

We could add it but I think there’s probably a fair chance some people are relying on this so we would probably need to allow it to at least be configurable. i.e. to specify if trailing commas are acceptable or not. This is what we done for the ASCII/UTF8 encoding. We didn’t do this for the BEL character but the feeling was that the subset of users relying on that was probably so close to 0 it didn’t warrant the effort.

In the case of switching to UTF8 we thought the chances this actually causes an issue for end users was probably relatively low, but worth supporting the other way for those that really need it. So I would be tempted to say that if we were to adopt this we would

  • Allowing reading trailing spaces by default
  • Allow specifying an option when reading a JSON to not accept trailing spaces
  • Never write a JSON containing a trailing space

At the moment we only have FormatOptions for specifying options when writing a JSON. So I think we would need a ParseOptions or DecodingOptions or something along those lines.

6

Alternatively a static method

static bool JSON::validate (const juce::StringRef);

could serve that purpose.

I think nobody wants to code around such errors that don’t create any ambiguity at all. So definitely an opt-in feature.

7

thanks @anthony-nicholls!

For me both, ParseOptions and a static method sound like viable solutions, although ParseOptions would probably be a bit more performant because validation and parsing could be done at the same time

8

Does the JUCE parser also allow comments, which aren’t part of the official spec either? I don’t remember, but either way best to leave the JUCE parser as it is.

If you need a more faithful parser I’d try something like GitHub - nlohmann/json: JSON for Modern C++, and maybe write a juce::VariantConverter<nlohmann::json>. They’ve explicitly taken the stance of disallowing trailing commas, btw: FAQ - JSON for Modern C++