Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle byte[] and char[] sourceRef's specially in JsonLocation.toString() #343

Closed
wants to merge 1 commit into from

Conversation

laszlovandenhoek
Copy link

@laszlovandenhoek laszlovandenhoek commented Dec 27, 2016

Currently, when attempting to parse a syntactically incorrect byte array as JSON using ObjectMapper.readValue, the JsonParseException that is thrown from ParserBase._reportMismatchedEndMarker looks as follows:

Invalid Json: Unexpected close marker '}': expected ']' (for ROOT starting at [Source: [B@4928787c; line: 1, column: 0])
at [Source: [B@4928787c; line: 1, column: 2]

Since the "Source" in question is a byte array, which in this context can safely be assumed to be a UTF-8 stream, I would much prefer to have the original source that caused this exception included in my Exception, instead of a non-descript default "[B@deadbeef" hash. This eases debugging, especially if the source data cannot be deduced otherwise.

I believe the intent of the original code is like that, considering the cover-all .toString() call in the final } else { block.

@laszlovandenhoek laszlovandenhoek changed the title handle byte[] and char[] sourceRef's specially handle byte[] and char[] sourceRef's specially Dec 27, 2016
@laszlovandenhoek
Copy link
Author

Also, where do I send my signed CLA? If I release the above code into the public domain anyway, is that even necessary?

@laszlovandenhoek laszlovandenhoek changed the title handle byte[] and char[] sourceRef's specially handle byte[] and char[] sourceRef's specially in JsonLocation.toString() Dec 27, 2016
@cowtowncoder
Copy link
Member

Apologies for slow response: I was out on vacation.

Since the patch is small, I think in this specific case I can use it without CLA, if you add a comment stating that you'd like this contribution to be in public domain.

As to the patch; while I think it makes sense, I have some concern about printing the whole contents.
It might make sense to (for example) truncate contents. I realize that for String source this isn't being done anyway, but it seems like a good way. Another question is whether there are security concerns; but in general code probably should not be just printing out toString() of location. Challenge here is that it is actually being used by exceptions which do include output as is.

I will bring this up on jackson-dev list and see how others feel about it. Changes, regardless, will need to be made in master (for 2.9) since this is a non-trivial change to observed API.

@cowtowncoder
Copy link
Member

@laszlovandenhoek I decided to work a bit more on this one, and result (via #356) will be in 2.9.0.

In addition to indicating content (snippet, limited to first 500 characters), type is now indicated for all sources. May also add content from other source references where sensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants