Add `JsonParser.ALLOW_TRAILING_COMMA` to work for Arrays and Objects

[bdhess]

Not sure if the inconsistency is intentional?

Seems weird that the feature permits ["a","b",] but not {"a": 1, "b": 2, }.

[bdhess]

See also #118

[cowtowncoder]

Did you read documentation of this feature?

[bdhess]

Yes-- the behavior is consistent with its documentation. However...

I submitted a pull request in #234 that covers the trailing commas use case for both arrays and objects, which I closed after you mentioned that this feature had been merged. I'd really like some level of support for trailing commas in objects as well. I'm happy to reopen the pull request and/or take another approach that you suggest.

[cowtowncoder]

@bdhess Hmmh. Ok. So, the issue as I recall is/was that whereas with arrays it is possible to stuff add logical nulls where missing value is, but with Objects this can not be done. So the question would be what the behavior is then.
I think one possibility considered was to perhaps allow just trailing comma.

I am open to improvements here; perhaps a separate feature would make sense if we can figure out reasonable behavior and implementation for backend.

[bdhess]

Agree that there's no logical parallel to inserting nulls into the Objects, so I think you're right to push this toward a separate feature.

I would propose that a feature ALLOW_TRAILING_COMMAS be defined as allowing a single trailing comma at the end of an array or object definition. Below I've outlined how I it should intersect with ALLOW_MISSING_VALUES in practice.

Based on the work I did previously in #234, I think it would be straightforward to implement this way, so if you're okay with this outline in principle, I'd be happy to dust that review off and implement with the below as test cases.

Default

["a", "b"] => ["a", "b"]
[,"a","b"] => invalid
["a",,"b"] => invalid
["a","b",] => invalid
["a",,"b",] => invalid
["a","b",,] => invalid

{"a": true, "b": false} => {"a": true, "b": false}
{,"a": true, "b": false} => invalid
{"a": true,, "b": false} => invalid
{"a": true, "b": false,} => invalid
{"a": true,, "b": false,} => invalid
{"a": true, "b": false,,} => invalid

ALLOW_MISSING_VALUES enabled

["a", "b"] => ["a", "b"]
[,"a","b"] => [null, "a", "b"]
["a",,"b"] => ["a", null, "b"]
["a","b",] => ["a", "b", null]
["a",,"b",] => ["a", null, "b", null]
["a","b",,] => invalid ["a","b",,] => ["a","b",null,null]

{"a": true, "b": false} => {"a": true, "b": false}
{,"a": true, "b": false} => invalid
{"a": true,, "b": false} => invalid
{"a": true, "b": false,} => invalid
{"a": true,, "b": false,} => invalid
{"a": true, "b": false,,} => invalid

ALLOW_TRAILING_COMMAS enabled

["a", "b"] => ["a", "b"]
[,"a","b"] => invalid
["a",,"b"] => invalid
["a","b",] => ["a", "b"]
["a",,"b",] => invalid
["a","b",,] => invalid

{"a": true, "b": false} => {"a": true, "b": false}
{,"a": true, "b": false} => invalid
{"a": true,, "b": false} => invalid
{"a": true, "b": false,} => {"a": true, "b": false}
{"a": true,, "b": false,} => invalid
{"a": true, "b": false,,} => invalid

ALLOW_MISSING_VALUES and ALLOW_TRAILING_COMMAS enabled

["a", "b"] => ["a", "b"]
[,"a","b"] => [null, "a", "b"]
["a",,"b"] => ["a", null, "b"]
["a","b",] => ["a", "b"]
["a",,"b",] => ["a", null, "b"]
["a","b",,] => invalid ["a","b",,] => ["a","b",null]

{"a": true, "b": false} => {"a": true, "b": false}
{,"a": true, "b": false} => invalid
{"a": true,, "b": false} => invalid
{"a": true, "b": false,} => {"a": true, "b": false}
{"a": true,, "b": false,} => invalid
{"a": true, "b": false,,} => invalid

[bdhess]

As I think about this more, I don't think it would be any more complicated to support multiple trailing commas at the end of an array or object definition, and some may find that behavior more desirable.

[cowtowncoder]

Agreed, trailing commas seem like something that could be supported.

My only concern here is (as before) in keeping performance overhead minimal; and that just means trying to keep control flow changes to minimum (and run perf tests before & after).

So, PR sounds like a good idea.

On naming, perhaps it should be ALLOW_TRAILING_COMMA just to avoid possible confusion that multiple trailing commas are acceptable?

One small (?) question wrt backwards compatibility is wrt behavior with trailing array commas... I think those are accepted currently, and if so would be good to also support without users having to enable another feature. But not sure how that should work.

[bdhess]

A couple of observations as I've implemented:

I mistakenly proposed that ["a","b",,] be invalid under ALLOW_MISSING_VALUES, which would be a regression (it's equivalent to ["a","b",null,null] today). Updated the proposal above under both the ALLOW_MISSING_VALUES cases to reflect non-regressive behavior.

Looked into @cowtowncoder's feedback regarding trailing commas in an array being permitted today. This doesn't appear to be the case, and I've written a test case that asserts the opposite, which passes against master:

        String doc = "[true,false,]";
        JsonParser p1 = createParser(f, mode, doc);
        assertToken(JsonToken.START_ARRAY, p1.nextToken());
        assertToken(JsonToken.VALUE_TRUE, p1.nextToken());
        assertToken(JsonToken.VALUE_FALSE, p1.nextToken());
        try {
            p1.nextToken();
            fail("no exception");
        } catch (Exception e) {
            assertTrue(e.getMessage().startsWith("Unexpected character (']' (code 93)"));
        }

Perf tests are running now, and assuming they look good, I'll drop the PR shortly.