Remove MissingKotlinParameterException and replace with MismatchedInputException

[k163377]

This change was originally scheduled to take place in 2.16, but later discussions postponed the decision to the 2.17 transition.

---

If the work on removing kotlin-reflect is reflected in jackson-module-kotlin, the MissingKotlinParameterException change is unavoidable since it references KParameter.
#450

The same applies to resolving the following issue
#572

I have also found a way to significant improve performance when the strictNullChecks option is enabled, but it is difficult to avoid changing error messages when enabling this option as well.
ProjectMapK/jackson-module-kogera#44

So, remove MissingKotlinParameterException and replace it with MismatchedInputException(MismatchedInputException is a superclass of MissingKotlinParameterException).
This removal process will be done in phases, with deprecation in 2.15 and removal in 2.16.

[k163377]

From the following issue, I personally feel it would be better to remove MissingKotlinParameterException and replace it with MismatchedInputException.
#234 (comment)

[k163377]

@cowtowncoder
Can you please confirm this issue?

[cowtowncoder]

@k163377 I do not have enough knowledge to give strong opinion, but it does sound good to me. So please proceed if it makes sense to you.

[cowtowncoder]

@k163377 Is the idea to change exception used in 2.15, deprecate old one (not used but left in-place), then remove from 2.16. Because that sounds like a reasonable plan to me.
But if so, may want to consider this ticket for replacement part & create follow-up for removal in 2.16?

[k163377]

@cowtowncoder

Is the idea to change exception used in 2.15, deprecate old one (not used but left in-place), then remove from 2.16.

Yes.
However, I was going to use this issue to discuss whether to really delete it, so I am not going to close this issue.

[cowtowncoder]

@k163377 Sure, deletion may or may not occur. But there should be one issue closed, for changing exception (like this one) so it can be added in release notes; and then another one for deletion if any (leaving open until and if it is done).

[remen]

Would it be possible to keep MissingKotlinParameterException but change the type of the parameter to just a string (i.e. parameter.name)? Rationale: our global error handler in our project uses this exception to generate a simpler error message than "Instantiation of [simple type, class ...] etc." which leaks very internal implementation details.

[k163377]

@remen
Perhaps you can get the kind of information you need from MismatchedInputException.
Please check Javadoc once.
https://javadoc.io/static/com.fasterxml.jackson.core/jackson-databind/2.15.2/com/fasterxml/jackson/databind/exc/MismatchedInputException.html

For example, you can check the detailed path by doing the following

class Class @JsonCreator constructor(@JsonProperty(value = "value", required = true) val value: String)

fun main() {
    val mapper = ObjectMapper()
    try {
        mapper.readValue<Class>("{}")
    } catch (e: MismatchedInputException) {
        println(e.pathReference)
    }
}

The example shows getting MismatchedInputException without kotlin-module, but you can get the same result using kogera.
https://github.com/ProjectMapK/jackson-module-kogera

[k163377]

I have tentatively incorporated this change for 2.16.

[mzeijen]

Replacing MissingKotlinParameterException with the more generic MismatchedInputException causes a problem for us, and I think it is the same as @remen indicated, namely it becomes harder to figure out what the failure type is. Except for parsing the message of the exception, there is no way to find out what the failure type is.
We also use the type of the exception to simplify the error so that we do not leak internals implementation details, so being able to figure out the type of failure is important to us.

If MissingKotlinParameterException has to be removed, maybe replace it with com.fasterxml.jackson.databind.exc.InvalidNullException, which to me seems to be an equivalent exception type. That way we can convert both to the same simplified exception indicating a missing or null field.

[cowtowncoder]

Quick note: perhaps it'd make sense to add a replacement subtype of MismatchedInputException -- I think the problem is not the idea of a custom exception type with clear semantics, but the current implementation that refers to a problematic type. WDYT @k163377 ?

[k163377]

Indeed, the biggest problem is the KParameter contained in the MissingKotlinParameterException, and if that can be removed, the goal is achieved.
From the points raised, I also understand that there is a specific use case.
For this reason, I agree that there should be a unique Exception.

What I am struggling with is whether to keep it simple and just revive it or make a few more changes.

First, the name MissingKotlinParameterException is inappropriate for what is thrown from the strictNullChecks process.
It occurs because an illegal null was entered, not because a parameter was missing.
Therefore, I think it should be renamed or separate exceptions should be defined for parameters and strictNullChecks.

The next is whether the superclass should be changed to InvalidNullException when reviving MissingKotlinParameterException.
From the perspective of expected usage, InvalidNullException seems to be a more appropriate parent class.

Personally, I am thinking of adding a new Exception named KotlinInvalidNullException that extends InvalidNullException.
However, I will not work on it for a while as I need to think about it some more (I will work on it before 2.16).

WDYT @cowtowncoder ?

[k163377]

@cowtowncoder I am sorry to bother you, but here is a reminder.

[cowtowncoder]

@k163377 Sorry, thought I had added comment here. Thanks for reminder.

So, yes, it all makes sense as explained, including use of src/main/java/com/fasterxml/jackson/databind/exc/InvalidNullException.java. I assume it could even be used as-is, but if Kotlin-specific sub-class makes more sense, sure, why not? (for more information or such).

[k163377]

@cowtowncoder
I have started to consider kotlin-module's own exception which inherits from InvalidNullException.
I would like to change the implementation of databind and would like to discuss.

Regarding InvalidNullException, is it conceivable to set the DeserializationContext to nullable?
I think JsonParser of MismatchedInputException is nullable, but if I inherit InvalidNullException, I get NullPointerException below.
https://github.com/FasterXML/jackson-databind/blob/98ad11248303f93864343d634fe0897c7baff036/src/main/java/com/fasterxml/jackson/databind/exc/InvalidNullException.java#L33

I plan to implement the following converter in the future to speed up the null check (strictNullCheck) for values in a Collection.
I would like to use KotlinInvalidNullException in this converter as well, but we cannot access DeserializationContext from the context of this process.
So I want to make DeserializationContext nullable.
https://github.com/ProjectMapK/jackson-module-kogera/blob/6b249f6d8b43f3bd70761425775ca8a1ba0952ed/src/main/kotlin/io/github/projectmapk/jackson/module/kogera/deser/Converters.kt#L22-L26

[cowtowncoder]

Yes I would be ok to get PR that allows DeserializationContext to be null for constructor.
Ideally this wouldn't be the case (it really should only be called from places where DeserializationContext is available) but if not then it is not strictly required from implementation perspective.

[k163377]

Sorry to change my previous statement, but I will not be making this change in 2.16 and will continue to consider it in 2.17.
The reason is that the MissingKotlinParameterException may no longer be used in this process due to the strictNullCheck improvement.

The discrepancy between the MissingKotlinParameterException error name and the strictNullCheck process was one of the main motivations for this change.
On the other hand, if the use from strictNullCheck is eliminated, then this name is appropriate.

With the imminent release of 2.16 and not enough time to establish an overall policy, I would prefer to postpone a decision.

[cowtowncoder]

Sounds reasonable. About the only is that unless already done, maybe marking MissingKotlinParameterException as deprecated for 2.16 would make sense?

[k163377]

@cowtowncoder
I have submitted a PR. Can you please confirm that it is as you have imagined?
#720

[cowtowncoder]

@k163377 Yes, that looks good to me. Thanks!

[Jerbell]

I'm a bit late, but just wanted to chime in and let you know my preference would be a replacement exception which contains the missing parameter name (as a couple of others have suggested).