jackson-module-kotlin: Remove MissingKotlinParameterException and replace with MismatchedInputException
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.
https://github.com/FasterXML/jackson-module-kotlin/issues/450
The same applies to resolving the following issue https://github.com/FasterXML/jackson-module-kotlin/issues/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.
https://github.com/ProjectMapK/jackson-module-kogera/pull/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.
About this issue
- Original URL
- State: open
- Created a year ago
- Comments: 21 (18 by maintainers)
Commits related to this issue
- Remove MissingKotlinParameterException and replace with MismatchedInputException https://github.com/FasterXML/jackson-module-kotlin/issues/617 — committed to ProjectMapK/jackson-module-kogera by k163377 a year ago
- Remove MissingKotlinParameterException and replace with MismatchedInputException fixes #617 — committed to k163377/jackson-module-kotlin by k163377 a year ago
- DeserializationContext changed to allow null wrt https://github.com/FasterXML/jackson-module-kotlin/issues/617#issuecomment-1783442855 — committed to k163377/jackson-databind by k163377 8 months ago
- DeserializationContext changed to allow null (#4179) wrt https://github.com/FasterXML/jackson-module-kotlin/issues/617#issuecomment-1783442855 — committed to FasterXML/jackson-databind by k163377 8 months ago
Would it be possible to keep
MissingKotlinParameterExceptionbut 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.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).
@cowtowncoder I have submitted a PR. Can you please confirm that it is as you have imagined? #720
Yes I would be ok to get PR that allows
DeserializationContextto benullfor constructor. Ideally this wouldn’t be the case (it really should only be called from places whereDeserializationContextis available) but if not then it is not strictly required from implementation perspective.Indeed, the biggest problem is the
KParametercontained in theMissingKotlinParameterException, 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 uniqueException.What I am struggling with is whether to keep it simple and just revive it or make a few more changes.
First, the name
MissingKotlinParameterExceptionis inappropriate for what isthrownfrom thestrictNullChecksprocess. It occurs because an illegalnullwas entered, not because a parameter was missing. Therefore, I think it should be renamed or separate exceptions should be defined for parameters andstrictNullChecks.The next is whether the superclass should be changed to
InvalidNullExceptionwhen revivingMissingKotlinParameterException. From the perspective of expected usage,InvalidNullExceptionseems to be a more appropriate parent class.Personally, I am thinking of adding a new
ExceptionnamedKotlinInvalidNullExceptionthat extendsInvalidNullException. 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 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.