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

Add sanity checks for ObjectMapper.readXXX() methods #2348

Closed
wants to merge 2 commits into from

Conversation

ebundy
Copy link

@ebundy ebundy commented Jun 8, 2019

Today by trying to read something that I though being an InputStream :

InputStream stream = getClass().getResourceAsStream("/myFile.json");
List<Foo> foos= Arrays.asList(objectMapper.readValue(stream, Foo[].class));

I get a quite broad end-of-input exception :

com.fasterxml.jackson.databind.exc.MismatchedInputException: No content to map due to end-of-input
at [Source: UNKNOWN; line: 1, column: 0]
at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59) ~[jackson-databind-2.9.8.jar:2.9.8]
at com.fasterxml.jackson.databind.ObjectMapper._initForReading(ObjectMapper.java:4145) ~[jackson-databind-2.9.8.jar:2.9.8]
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4000) ~[jackson-databind-2.9.8.jar:2.9.8]
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3070) ~[jackson-databind-2.9.8.jar:2.9.8]

Actually, readValue(InputStream src, Class<T> valueType) as well as ObjectMapper reading methods do not perform sanity checks against the source parameters that they receive. In my case the InputStream variable referenced a null.
The error message is so misleading and too broad.
For example on SO, we can find other usecases that produces this error :
https://stackoverflow.com/questions/26925058/no-content-to-map-due-to-end-of-input-jackson-parser

Applying fail fast principle and throwing IllegalArgumentException or NullPointerException with a meaningful message would make much more sense.
So here is a pull request with this change. I didn't write whole unit tests for every case. But I can of course add it if the pull request makes sense for reviewers.

Regards,
David

@cowtowncoder
Copy link
Member

Seems reasonable to me. I think same would also apply to ObjectReader and ObjectWriter I think.

One other note: if PR is based on master, any improvements would only apply to 3.0 (which will take a long time to complete yet). So it would make sense to base it on 2.10 branch instead, as 2.10.0.pr1 (and eventual 2.10.0) are due to be released much sooner (within a month I hope).

@cowtowncoder cowtowncoder changed the title add sanitify checks for ObjectMapper.readXXX() methods Add sanity checks for ObjectMapper.readXXX() methods Aug 7, 2019
@cowtowncoder cowtowncoder added this to the 2.10.0.pr2 milestone Aug 10, 2019
cowtowncoder added a commit that referenced this pull request Aug 10, 2019
@cowtowncoder
Copy link
Member

Since I want to add this to 2.10, merged manually: thank you for the contribution!

@Robin479
Copy link

Robin479 commented Dec 2, 2019

Since I stumbled upon this myself, I wanted to point out:
The corresponding code change turns "some" down-stream checked IOExceptions and previously acceptable null-reference into early unchecked IllegalArgumentExceptions, which might not be handled by existing user code and therefore (silently) break clients. Users might be used to wrapping ObjectMapper code into try-catch-blocks catching only IOException (or JsonProcessingException). Some methods used to work fine with null arguments but now they throw uncheck RuntimeExceptions, e.g. see the different behavior of readValue(InputStream, Class) for a null-InputStream, before and after the introduction of the non-null assertions.

@cowtowncoder
Copy link
Member

@Robin479 it would be useful to enumerate specific cases where null was accepted as that seems like a bug, and something to possibly revert. Filing a new issue for one or more of these would be useful.

Change from previously checked exceptions are unfortunate too of course, but seem like things that should have been caught earlier, assuming failure was due to invalid argument. So in that case I consider situation a bug (missing early detection of invalid argument) that is getting fixes.

@niemannd
Copy link

niemannd commented Dec 3, 2019

@cowtowncoder The example of @Robin479 just happend to us. We had some (acceptable) null references that were nicely wrapped in try-catch-blocks. Now those are useless and e.g. our tests broke.

@cowtowncoder
Copy link
Member

@hazeglide I would like you to think very carefully here as to whether your statement is helpful, in light of my previous comment.

@Robin479
Copy link

Robin479 commented Jan 6, 2020

@cowtowncoder Have a look here: https://github.com/Robin479/jackson-nulltest/blob/master/src/test/java/com/fasterxml/jackson/databind/ObjectMapperNullabilityTest.java

@Robin479 it would be useful to enumerate specific cases where null was accepted as that seems like a bug, and something to possibly revert. Filing a new issue for one or more of these would be useful.

Change from previously checked exceptions are unfortunate too of course, but seem like things that should have been caught earlier, assuming failure was due to invalid argument. So in that case I consider situation a bug (missing early detection of invalid argument) that is getting fixes.

@cowtowncoder
Copy link
Member

@Robin479 Could you please file a new issue, including link to that project, and brief explanation of the issue. I do not usually re-open closed issues as that can make release notes difficult to follow.
Extracting specific failing tests cases would be helpful too (and speed up finding solution) if possible.

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.

4 participants