AbstractJackson2HttpMessageConverter + Jackson 2.10: handle ValueInstantiationException properly

[Romster]

In jackson-databind version 2.10 a new exception was introduced: ValueInstantiationException
It appears when we can't instantiate a new instance but successfully parsed input json.
In my case, it was null-checks in the constructor.
Previously exception which were thrown in constructor caused InvalidDefinitionException and was wrapped into HttpMessageConversionException by AbstractJackson2HttpMessageConverter:

protected void writeInternal(Object object, @Nullable Type type, HttpOutputMessage outputMessage)
			throws IOException, HttpMessageNotWritableException {

		MediaType contentType = outputMessage.getHeaders().getContentType();
		JsonEncoding encoding = getJsonEncoding(contentType);
		JsonGenerator generator = this.objectMapper.getFactory().createGenerator(outputMessage.getBody(), encoding);
		try {
                ......
                ......

		}
		catch (InvalidDefinitionException ex) {
			throw new HttpMessageConversionException("Type definition error: " + ex.getType(), ex);
		}
		catch (JsonProcessingException ex) {
			throw new HttpMessageNotWritableException("Could not write JSON: " + ex.getOriginalMessage(), ex);
		}
	}

But with a new version of jackson-databind (2.10.2, which comes with spring-boot) a new ValueInstantiationException is thrown and it is handled as a JsonProcessingException. Thereby HttpMessageNotWritableException is thrown instead (which also leads to RestClientException if it happens in RestTemplate)

I think ValueInstantiationException should cause HttpMessageConversionException and not HttpMessageNotWritableException.

[jhoeller]

This sounds worthwhile to me, and necessary for full Jackson 2.10 support. The only downside in catching such a new exception type is that it introduces a hard dependency on Jackson 2.10 at runtime... otherwise we'll have to work around the non-presence of that exception type through reflection.

[Romster]

This sounds worthwhile to me, and necessary for full Jackson 2.10 support. The only downside in catching such a new exception type is that it introduces a hard dependency on Jackson 2.10 at runtime... otherwise we'll have to work around the non-presence of that exception type through reflection.

Jackson provides com.fasterxml.jackson.databind.cfg.PackageVersion.version(), so maybe we could check it first?

[jhoeller]

On review, the ValueInstantiationException javadoc hints at an important nuance: With value instantiation failures, it is unclear whether this is due to invalid input or an invalid type definition. In your case with something hitting a non-null check, it arguably seems to be invalid input... in which case HttpMessageNotWritableException does not seem wrong. It's a matter of definition of how it actually differentiates from HttpMessageConversionException, that is, whether assertions in target types count as conversion problem rather than highlighting invalid input.

If we decide to revisit this, we could specifically handle MismatchedInputException and turn it into HttpMessageNotWritableException, whereas any other JsonMappingException base type (InvalidDefinitionException or ValueInstantiationException) could be turned into HttpMessageConversionException. This would avoid a hard dependency on ValueInstantiationException in our code, remaining runtime-compatible with both Jackson 2.9 and 2.10. So the remaining question is just whether this is semantically correct...

[Romster]

I like the approach of handling JsonMappingException. It should keep the behavior the same as it used to be with Jackson 2.9.
Otherwise, something should be done with org.springframework.web.client.HttpMessageConverterExtractor#extractData exceptions handling - getting just a general "RestClientException" isn't very useful (and going through the "getCause" is a bit hacky and doesn't look like a trustworthy contract), IMO.

So my vote is for interpreting JsonMappingException as HttpMessageConversionException.

[jhoeller]

Good point about RestTemplate, we're really considering HttpMessageNotReadableException to be just another kind of IOException there. In that sense, anything with conversion impact should be propagated through as an HttpMessageConversionException.

[rstoyanchev]

Wouldn't that impact applications that handle RestClientException and currently do not expect HttpMessageNotReadableException as a top-level exception?

[jhoeller]

I'm just considering to turn a few more kinds of Jackson exception (such as ValueInstantiationException or possibly any JsonMappingException) into HttpMessageConversionException at the converter level. HttpMessageConversionException propagates through as-is already, so that shouldn't come as a surprise at the application level. Anything that we keep turning into HttpMessageNotReadableException will still be wrapped as a RestClientException, so HttpMessageNotReadableException will never shine through as a top-level exception.

[Romster]

#24499 created a PR