Change default of DeserializationFeature.FAIL_ON_TRAILING_TOKENS to true for 3.0

[yawkat]

imo it is better to fail by default here. If you're parsing line-delimited json, you'll notice immediately if you forget to turn off FAIL_ON_TRAILING_TOKENS. However in the other direction, if it was off by default and you forget to turn it on, you probably would never notice (few people test for failures), which could also open up the door to parsing differential vulnerabilities in an application.

See discussion on #3400

[cowtowncoder]

Have not yet decided but will include in "potentially for 3.0.0" list, will bring up on discussion (to be created).

[JooHyukKim]

Though the rationale here makes sense (and the one from #3400), this seems to be have similar aspects to #493 about FAIL_ON_UNKNOWN_PROPERTIES feature.

So main opposing idea would be the Postel's Law saying...

"be conservative in what you do, be liberal in what you accept from others".

... at the moment I don't have strong opinion on either side yet. I wish we could invite over the same crowd from #493 for discussion 😅.

[cowtowncoder]

See discussion at FasterXML/jackson-future-ideas#74 wrt whether to make proposed change in 3.0.0.

[cowtowncoder]

Proposal approved: can proceed with this change.

[cowtowncoder]

Ok: trying to work through PR shows a few issues -- initially about 30 unit test failures, of which only 2 are for specific checking of configuration settings.

I have been able to resolve about 2/3 of them, leaving (as of now) 11 failures. Will keep on working through them to get closer to 0.

However: I am finding potential concerns about this change... will see how remaining failures look like, and will outline my concerns if some remain.
It is possible we'd need to consider only verifying trailing tokens for case where readValue() is called with input source other than JsonParser, since passing JsonParser may suggest desire for partial reads.
But I'll look into this bit more first.

[cowtowncoder]

Ok. So, all remaining 11 failures are actually weird, but due to same underlying issue: use of External Type Id leading to:

Unexpected close marker '}': expected ']'

failures. This must be due to the way JsonTypeInfo.Include.EXTERNAL_PROPERTY is internally handled and not due to actual parsing.

This suggests problem unrelated to DeserializationFeature.FAIL_ON_TRAILING_TOKENS, actually, which is good. But unfortunately I am at loss as to how to actually resolve it at this point.
So without somehow resolving this issue, change to defaults cannot proceed because:

1. Unit test must pass, and in this case changing defaults is wrong (hiding true problem). But more importantly...
2. ... some users would probably hit this problem with default change (otherwise they would only do so if explicitly enabling feature).

So will need to put this on hold for now, probably. :-/

[cowtowncoder]

Turns out issue was not as serious as I thought: odd error message is a flaw to fix via:

FasterXML/jackson-core#1394

but was triggered by erroneous JSON in test.

Now merged: will see how many downstream test fails we get :)

[cowtowncoder]

Wow. This change has been great for teasing out various bigger and smaller flaws on both core and format modules (last fix was for Protobuf backend that had regression in 3.0.0, only caught by trailing tokens check). Still got one remaining XML issue to eventually fix.