Add sanity checks for ObjectMapper.readXXX() methods

[ebundy]

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]

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]

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

[Robin479]

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]

@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]

@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]

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

[Robin479]

@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]

@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.