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

MissingKotlinParameterException should extend MismatchedInputException #321

Closed
MateuszStefek opened this issue Apr 10, 2020 · 6 comments
Closed
Milestone

Comments

@MateuszStefek
Copy link

MateuszStefek commented Apr 10, 2020

Should MissingKotlinParameterException extend MismatchedInputException?

The javadoc of MismatchedInputException suggests that MissingKotlinParameterException is a valid subtype:

/**
 * General exception type used as the base class for all {@link JsonMappingException}s
 * that are due to input not mapping to target definition; these are typically
 * considered "client errors" since target type definition itself is not the root cause
 * but mismatching input. This is in contrast to {@link InvalidDefinitionException} which
 * signals a problem with target type definition and not input.
 *<p>

In a recent version of Spring, a change was introduced that differentiate handling of exceptions when parsing a HTTP input:
https://github.com/spring-projects/spring-framework/blob/b889700548681382911e06701b257549bb81e929/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java#L242

		catch (MismatchedInputException ex) {  // specific kind of JsonMappingException
			throw new HttpMessageNotReadableException("Invalid JSON input: " + ex.getOriginalMessage(), ex, inputMessage);
		}
		catch (InvalidDefinitionException ex) {  // another kind of JsonMappingException
			throw new HttpMessageConversionException("Type definition error: " + ex.getType(), ex);
		}
		catch (JsonMappingException ex) {  // typically ValueInstantiationException
			throw new HttpMessageConversionException("JSON conversion problem: " + ex.getOriginalMessage(), ex);
		}
		catch (JsonProcessingException ex) {
			throw new HttpMessageNotReadableException("JSON parse error: " + ex.getOriginalMessage(), ex, inputMessage);
		}

Basically, if Jackson throws MismatchedInputException our controllers respond with 400 Bad Request, otherwise they respond with 500 Internal Server Error.

Because of this change, Kotlin dtos cannot be properly rejected when a non-nullable constructor argument is missing.

An example problem it causes:
spring-projects/spring-framework#24610

@dinomite
Copy link
Member

This makes a lot of sense to me.

It's been a few years since the code was written, but I'd like to hear from @jamesbassett, who wrote the exception, or @apatrida, who merged it as part of #32, to see if they have any thoughts.

@cowtowncoder
Copy link
Member

Sounds good to me as well.

@dinomite
Copy link
Member

Draft PR: #325

@jamesbassett
Copy link

Yeah seems good to me - as it's a subclass it shouldn't introduce any real backwards compatibility issues.

At work, we have a Spring controller advice that handles MissingKotlinParameterException explicitly, so we might be able to remove that once this is done.

@cowtowncoder
Copy link
Member

@dinomite Only one suggestion: it might make sense to rebase to 2.11 branch as master (3.0) will not be available very soon. Of course can also merge, then cherry-pick back.

I am also finally giving access to @dinomite and @viartemev and so I think we have more committers who can do merge.

@dinomite
Copy link
Member

👍 I'll get it based onto 2.11, makes more sense there

@cowtowncoder cowtowncoder added this to the 2.11.0 milestone Apr 23, 2020
@cowtowncoder cowtowncoder changed the title MissingKotlinParameterException should extend MismatchedInputException MissingKotlinParameterException should extend MismatchedInputException Apr 23, 2020
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

No branches or pull requests

4 participants