Make MAX_ERROR_TOKEN_LENGTH configurable via ErrorReportConfiguration

[JooHyukKim]

as discussed in google groups page.

Motivation

Currently, when Jackson throws an exception, it quotes the JSON that triggered the error. However, there's a limitation: the quoted message is truncated after MAX_ERROR_TOKEN_LENGTH as specified in...

jackson-core/src/main/java/com/fasterxml/jackson/core/base/ParserMinimalBase.java

Line 132 in 832b520

protected final static int MAX_ERROR_TOKEN_LENGTH = 256;

.... which makes it difficult to debug issues when the relevant section is further down in the JSON.

This PR elliviates that by introducing ErrorTokenConfiguration configurable via JsonFactory.Builder and JsonFactory.setErrorTokenConfiguration

[cowtowncoder]

Quick note: due to number of changes needed for configuration, I think I really want a configuration object to be created to pass this setting, along with others. Similar to StreamReadConstraints; immutable. Otherwise next time a size settings is needed same number of changes will be needed.

I don't know what a good name would be, nor if there are other settings that could already be added. But I think adding individual scalars in JsonFactory is not very maintainable.

[JooHyukKim]

a configuration object to be created to pass this setting, along with others. Similar to StreamReadConstraints; immutable...
But I think adding individual scalars in JsonFactory is not very maintainable....

Totally agree on this 👍🏻👍🏻 So I will come back with an object version(StreamReadConstraints as reference) and hopefully some reasonable draft of names 😆.

Until then... keep this as DRAFT, to reduce distraction.

[JooHyukKim]

Re-implemented Object-style, like StreamReadConstraints.

Do you think there is there any revision to do in other module, @cowtowncoder ?
Though I checked jackson-databind found no usage of original (now disapprearing) MAX_ERROR_TOKEN_LENGTH.

[cowwoc]

I would ask @cowtowncoder for review. I'm just an end-user :)

[cowtowncoder]

@JooHyukKim Sorry, I should have explained bit further: although the encapsulation as ErrorTokenConfiguration is fine, I think its scope is too small as in we won't really have too many aspects of error token configuration. What I'd be looking for is bit more generic; something where we would be adding more settings.
But I don't have a good idea immediately what that would be... something along lines of "DiagnosticsConfiguration" or maybe "ErrorReportingConfiguration". But it is difficult to predict what will be needed.

So I think that at this point I'd probably leave this PR as work-in-progress to see if we can use configuration for other aspects.

[JooHyukKim]

So I think that at this point I'd probably leave this PR as work-in-progress to see if we can use configuration for other aspects.

Sure thing! I will turn this into draft for the moment. Maybe what we need is time 😆.

@cowtowncoder No worries 🙂 explanation was not short at all. I just tried start with narrow scope and see how it looks from others' perspective. Couple of possible names that we might consider are....

• DiagnosticsConfiguration
• DiagnosticsSettings
• ErrorReportingConfiguration
• ErrorReportSettings
• DebugSettings
• ExceptionHandlingConfig
• ErrorHandlingSettings
• FailureResolutionConfig
• ErrorDebugConfig
  ...and so on

PS : I changed the class name and PR title to ErrorReportConfiguration because it sounds like a better starting point than ErrorTokenConfiguration

[cowtowncoder]

@JooHyukKim I like that name -- and yeah, let's let this wait for a bit -- I don't necessarily mind having just one setting as long as it seems plausible we could have more.

And now that you mention this, one thing we could perhaps use it for would be configuring inclusion (or not) of source.
We have added StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION which probably should remain.
But we could make max length of snippet configurable, perhaps.
OTOH, is this "error" information? I guess it kind of is.

[JooHyukKim]

@cowtowncoder The name and JavaDoc of StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION itself seem more general. Maybe we can have still have both StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION and new version (let's temporarily call it) INCLUDE_SOURCE_IN_EXCEPTIONS specifically for error report use case.

But we could make max length of snippet configurable, perhaps.
OTOH, is this "error" information? I guess it kind of is.

I guess it "can" be error-related, if we name and use it that way? 🙂

Another thing is that I hope this new max length of snippet config stays with StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION or with the new INCLUDE_SOURCE_IN_EXCEPTIONS

[cowtowncoder]

@JooHyukKim Sorry, what I meant to say is that we'd leave the on/off feature as-is (too much hassle moving thing just added IMO, unless for maybe 3.0).
But we could specify length of source to include (if enabled) -- max 500 characters, 1000, etc as a new configuration setting in this config value object.

[JooHyukKim]

@cowtowncoder Oh I see. And yes “specifying length” seems useful. though I can’t seem to think of appropriate default value for the new config.

Naming idea : ‘maxSourceLengthInLocation’ ? Or maybe something shorter in length. 🤔🤔

[cowtowncoder]

Ok yes this is better. I wish passing new configuration was not so awkward though, it is unfortunate to need to modify both ContentReference and (especially) IOContext. But perhaps that cannot be helped.

One thing that would help merging would, however, be splitting one set of change to be merged first:

1. Add new ErrorReportConfiguration type with builder, unit test(s)
2. Change TSFBuilder / JsonFactory to allow configuring and holding on to that config object

and then in a follow-up PR do the rest. This will be bit easier to merge cleanly; one problem are being JsonFactory that is somehow unlinked (git merge thinking it is deleted from master).

It would also be bit easier to maybe see if there's any way to minimize changes to ContentReference or IOContext.

I think easiest way might be to leave this PR as-is, create new "minimal" one from some of changes.

[JooHyukKim]

It would also be bit easier to maybe see if there's any way to minimize changes to ContentReference or IOContext.

The changes in those classes might have been reduced a bit. Will keep this in mind along the way.

I think easiest way might be to leave this PR as-is, create new "minimal" one from some of changes.

Sounds like a plan 👍🏻 Will take below steps unless suggested otherwise.

EDIT : Filed a container issue #1066 to track PRs and changes instead, since this PR will be closed.