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

DecerializationProblemHandler is ignored when the input is invalid null in KotlinValueInstantiator #875

Open
k163377 opened this issue Jan 5, 2025 · 2 comments
Labels

Comments

@k163377
Copy link
Contributor

k163377 commented Jan 5, 2025

This is an issue regarding the following comment.
#806 (comment)

Currently KotlinValueInstantiator.createFromObjectWith throws an exception directly if the input is an illegal null.
https://github.com/FasterXML/jackson-module-kotlin/blob/9d4ad6a5e85bc5eb58f69700199c7cee776d2f86/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

On the other hand, databind calls DeserializationContext.handleInstantiationProblem(DeserializationProblemHandler.handleInstantiationProblem).
https://github.com/FasterXML/jackson-databind/blob/f624751d3c4c13405c8d07051b8954a691f265f7/src/main/java/com/fasterxml/jackson/databind/deser/std/StdValueInstantiator.java#L284-L295

Therefore, KotlinValueInstantiator should be modified as follows

  1. If paramVal is an illegal null, call DeserializationContext.handleInstantiationProblem
    • If not handled or if it is a custom error, throw and exit
  2. If the result of the call of 1 was a value, continue deserialization using that value

Note that the error caused by strictNullCheck is assumed to be excluded from the correction because the process will be eliminated from KotlinValueInstantiator when the #719 correction is made.

@k163377 k163377 added the bug label Jan 5, 2025
@k163377
Copy link
Contributor Author

k163377 commented Jan 5, 2025

@cowtowncoder
The StdValueInstantiator calls handleInstantiationProblem for errors in the creator call (_withArgsCreator.call) after reading arguments.
On the other hand, the above proposal calls handleInstantiationProblem for errors in each argument.
Is this the expected usage?

Also, I am not familiar with DeserializationProblemHandler, so if there are any problems with the above modified policy, I would appreciate your comments.

@cowtowncoder
Copy link
Member

@k163377 handleInstantiationProblem() should be called once per value, and not once per each argument (although of course there may be recursive call due to nesting, but those if any would be by different ValueInstantiator).

If this call does not throw exception (i.e. regular fail), resulting value should be used as the full deserialized value and usually would not be further processed (although I suppose in some cases further validation could be applied).

But if I understand your description correctly, I think it makes sense and is the right way to go: the benefit of handleInstantiationProblem() (and related methods) is that it calls method in DeserializationProblemHandler (if one configured), to let calling application do some limited form of error recovery -- either to work around some known issues or possibly just collecting multiple failures and failing only after collecting multiple ones.

Does this make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants