Allow configuration to exclude `"start marker...."' from exception messages
JooHyukKim opened this issue · 10 comments
(originally from google groups discussion)
Original Request (word for word)
When getting an JSON exception, even with source redacted - I was wondering if it was possible to configure jackson to not even mention the source, i.e instead of getting;
Unexpected end-of-input: expected close marker for Array (start marker at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 12])
just reporting:
Unexpected end-of-input: expected close marker for Array; line: 1, column: 12
I couldn't see anything in the source to possible configure that, so am looking at regexing it out myself, tho I'd rather not manually do this for all exceptions if possible - I could seem to see any way of just getting the base message, and line/column information from any exceptions to build up my own message from fields either.
Considerations / Suggestions
JacksonExceptionbase hasgetOriginalMessage()and is the base of
all Jackson-produced exceptions. Other than that, there's no functionality to support such.- If were to add new config,
ErrorReportConfigurationmay be considered to be the container class, to host such config.
This would really be useful.
The new redaction message introduced in #1039 is a pretty big regression IMO, as it very obviously leaks internal implementation details. Being able to disable that would be great.
This would really be useful.
The new redaction message introduced in #1039 is a pretty big regression IMO, as it very obviously leaks internal implementation details. Being able to disable that would be great.
@jamesagnew can you provide an example of the text that you find you objectionable?
@pjfanning Sure thing - my conern is that JsonProcessingException now produces messages like:
Unexpected character ('=' (code 61)): was expecting a colon to separate field name and value
at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 4, column: 18]
The reference to a java constant means this isn't a great message to return to REST API clients backed by Jackson any more since they shouldn't be aware of how the service is implemented internally.
@jamesagnew What about the previous message makes it any more or less parseable? I don't believe that it is ever recommended to return exception messages from 3rd party libs in your proprietary REST services.
This is just my opinion but what you return in your REST APIs does not really concern me and I don't think we should be tailoring our code to suit your specific use case. If you want to return error details in your REST services, you can very easily code it to parse out the details you need from the new and old JsonProcessingException strings.
Furthermore, there is an API to get the JSONLocation from the JsonProcessingException. I think you should use that instead.
@pjfanning I wouldn't say either is more or less parseable, I'm just trying to return a helpful error message to clients who have supplied an invalid request, including as much helpful information as possible about what the error was.
Previously we returned:
Unexpected character ('=' (code 61)): was expecting a colon to separate field name and value
at [Source: UNKNOWN); line: 4, column: 18]
..which was a bit weird because of the "Source: UNKNOWN" but ultimately pretty usable. Now we include this note about an internal java constant and I think that crosses a line into things our users will have security concerns with (for context, I help maintain a library that embeds Jackson - so I'm just trying to anticipate complaints we'll get from our users).
I don't think we should be tailoring our code to suit your specific use case
I don't really get this - There is already a setting that controls whether to include the offending request data or to replace it with the word redacted (JsonParser.Feature.INCLUDE_SOURCE_IN_LOCATION). I'm just suggesting that a little bit of extra configurability to allow users to choose to include neither would be helpful. But fair enough, if that's more configurability than you want to provide, I do see that I can construct my own message by calling JsonProcessingException#getOriginalMessage() and JsonProcessingException#getLocation(). I wasn't aware of those methods before.
Hmm, actually constructing my own message works sometimes but not other times. In the event of parsing a document with unexpected content after the final closing brace, even getOriginalMessage() includes the redacted note because the location always gets added to the message via _reportMismatchedEndMarker(int actCh, char expCh)
Yeah, Exception messages are meant to convey information to Developers NOT End Users.
So the reference to configuration seems perfectly reasonable, esp. since content is not included by default any more (for security reasons).
I would be supportive of further configurability, although filing this issue was perhaps bit pre-mature: @JooHyukKim thank you for doing this but going forward let's let developers request things so there is always someone who has clear idea of things they want.
(this is different from bugs where we can see something to fix: RFEs IMO are best filed by whoever has the suggestion)
going forward let's let developers request things so there is always someone who has clear idea of things they want.
True, @pjfanning @cowtowncoder sound right. Should've thought through. Closing until then! (until someone makes full requests)
@JooHyukKim you had best intentions and it's tricky to balance keeping track of things that are reported vs having tons of issues as backlog. So no harm.
I shall file a separte request for the additional configurability.