Invalid request results in 500 INTERNAL ERROR instead of 400 BAD REQUEST (with Lombok and Kotlin)

[quiram]

Affects: Spring Boot 2.2.5
More specifically spring-web 5.2.4

Scenario

Assume a bean definition with Lombok annotations used for annotation, for instance:

import lombok.Builder;
import lombok.NonNull;
import lombok.Value;

import java.util.Map;

@Value
@Builder
public class InformationReceivedRequest {
    @NonNull
    private String informationType;
    private Map<String, Object> data;
}

when making a JSON request with null as informationType, Jackson fails to create the corresponding object due to the non-null restriction, creating a ValueInstantiationException. This exception extends JsonMappingException, which extends JsonProcessingException.

In spring-web 5.2.3, class AbstractJackson2HttpMessageConverter would convert any JsonProcessingException to HttpMessageNotReadableException. This second exception is then treated at DefaultHandlerExceptionResolver (part of spring-webmvc), and transformed into a 400 BAD REQUEST response.

However, in spring-web 5.2.4, the class AbstractJackson2HttpMessageConverter has been modified so as to convert JsonMappingException into HttpMessageConversionException. This exception is not covered by DefaultHandlerExceptionResolver, which means the exception bubbles up to produce a 500 INTERNAL ERROR response.

This means that the same ValueInstantiationException that used to result in a 400 BAD REQUEST response (as expected), now results in a 500 INTERNAL ERROR response.

[rstoyanchev]

@quiram see #24455 (comment). The ValueInstantiationException javadoc says for "generic failures", in contrast to the sibling MistmatchedInputException for client errors.

[mario-philipps-icw]

We are facing the same issue, although we are using the Jackson Kotlin module instead of Lombok. In case of a missing attribute, this module throws a com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException, which is a direct subclass of JsonMappingException.

[rstoyanchev]

We might have to handle MissingKotlinParameterException the way we do MismatchedInputException. It's too bad the latter is not a sub-class of the former as it seems to have those semantics. Nevertheless that's a different example from the one above.

[quiram]

Thanks @rstoyanchev, I think I can see the dilemma here. On one side, if we look at the combined behaviour of lombok (or Kotlin in the case of @mario-philipps-icw) + jackson + spring-web + spring-webmvc, what we had up to Spring Boot 2.2.4 (spring-web 5.2.3) seemed to make sense because:

1. If I define a bean as requiring a non-null parameter via Lombok annotations
2. and I indicate that I expect such bean in a REST endpoint
3. when I send a JSON payload that has a value of null in the annotated parameter
4. then the web service responds with a 400 BAD REQUEST automatically

Looking at the issue as just the combined behaviour, what we have now could be seen as a regression.

On the other hand, it is true that, taking into account the Javadoc description for ValueInstantiationException, the new behaviour of AbstractJackson2HttpMessageConverter does seem to make sense, and we could place the fault at StdValueInstantiator.rewrapCtorProblem (jackson-databind), who should maybe be wrapping the NullPointerException in an InvalidNullException, which is a subtype of MismatchedInputException, which AbstractJackson2HttpMessageConverter would convert to a HttpMessageNotReadableException achieving the desired 400 BAD REQUEST.

Maybe we can raise this at jackson-databind and see if they are happy to change the root exception from ValueInstatiationException to InvalidNullException for the case of a NullPointerException. That would ultimately fix the problem.

Thoughts?

[rstoyanchev]

It appears as a regression but the hierarchy under JsonMappingException is relatively new introducing nuances between internal vs external reasons. We didn't have anything for it.

I think for Jackson it's unclear what the NPE means and it's probably why it treats it as ValueInstantiationException. Maybe there is a way to tip off Jackson about required fields. Or alternatively I see that Lombok can be configured to raise IllegalArgumentException instead of NPE. The latter is arguably a much stronger indication about bad input vs internal error.

[quiram]

This is promising. The Lombok configuration can be applied per directory, so provided one keeps all the bean definitions in the same package, the flag to raise IllegalArgumentException wouldn't have to have ramifications beyond that. A change would still be needed in jackson-databind to throw MismatchedInputException (or maybe even InvalidFormatException) upon IllegalArgumentException, but like you said it might be easier to make that case.

Is the next step raising an issue in jackson-databind?

[mario-philipps-icw]

We might have to handle MissingKotlinParameterException the way we do MismatchedInputException. It's too bad the latter is not a sub-class of the former as it seems to have those semantics. Nevertheless that's a different example from the one above.

@rstoyanchev Shall I open a new issue for it?

[jhoeller]

@mario-philipps-icw Let's keep it under this ticket for the time being. Looks like there will be a single revision refining the exception handling algorithm which will cover all cases here.

[rstoyanchev]

This is the FasterXML/jackson-databind#2126 that changed the treatment of instantiation errors from InvalidDefinitionException to ValueInstantiationException. I think the issue says, the most likely reason for constructor failure is something to do with the binding of input (rather than an internal problem), but since there is no way to know for sure, a separate ValueInstantiationException is raised instead.

ValueInstantiationException does not apply to writing where we serialize an Object, and not trying to create one. I believe the confusion starts in #24455 where @Romster described the issue (erroniously) in the context of writing while probably running into it on the reading side.

This has since caused all sorts of issues with value instantiation and null input validation including 2 here (Lombok, Kotlin), and one under spring-projects/spring-boot#20365 (Standard bean validation), which arguably should be treated as HttpMessageNotReadableException (i.e. 400) errors and not HttpMessageConversion (500). This is in line with the motivation for the creation of ValueInstantiationException which is to allow treating it differently by default from InvalidDefiintionException even if it isn't 100% possible to rule out something other than bad input.

In light of this I propose treating this as a regression, and reverting back to the original behavior:

catch (InvalidDefinitionException ex) {  // another kind of JsonMappingException
	throw new HttpMessageConversionException("Type definition error: " + ex.getType(), ex);
}
catch (JsonProcessingException ex) {
	throw new HttpMessageNotReadableException("JSON parse error: " + ex.getOriginalMessage(), ex, inputMessage);
}

and likewise for writing where ValueInstantiationException does not apply at all.

[alirezazareVisma]

We are facing the same issue, although we are using the Jackson Kotlin module instead of Lombok. In case of a missing attribute, this module throws a com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException, which is a direct subclass of JsonMappingException.

We are affected as well. is there an overview of when and by whom (Spring or Jackson) this issue will be resolved?