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

fixes #585 there is no way to support this kind of feature right now #601

Merged

Conversation

szymonprz
Copy link
Contributor

@szymonprz szymonprz commented Nov 5, 2022

Cleaned up tests for null to default feature as they can now clearly show what is supported and what is not. There is no way to provide default value for primitive fields which are not set as nullable, because the primitives are initialized to their defaults (0, 0.0, false etc.) and there is no possibility to distinguish if the value was initialized as a default primitive value or it was provided by user in content (in the first case we can use default parameter value but in the latter no)

…ight now

Cleaned up tests for null to default feature as they can now clearly show what is supported and what is not.
There is no way to provide default value for primitive fields which are not set as nullable, because the primitives are initialized to their defaults (0, 0.0, false etc.) and there is no possibility to distinguish if the value was initialized as a default primitive value or it was provided by user in content (in the first case we can use default parameter value but in the latter no)
@szymonprz
Copy link
Contributor Author

@cowtowncoder this pr only adds more test to clarify how this whole feature works. It could be possible to support expected behaviour from #585 but we need to know if the value from the content is set to null. Reading the value from content using this buffer is not sufficient because we will get default values for primitives. Do you know if there is such api which we can use to check if the value of the prop from content is null without transforming it to any kind of primitive?

There is also an option to not support this kind of a feature because if you expect to get some nulls in the content then you need to mark those fields as nullable to get default value and then the defaults will work as intended.

@cowtowncoder
Copy link
Member

@szymonprz Not sure what

Do you know if there is such api which we can use to check if the value of the prop from content is null without transforming it to any kind of primitive?

exactly means. About all functionality for content is available via JsonParser (or derived structures like intermediate JsonNode) -- maybe with exception of handlers for constructors (but they are not exposed to deserializers).

I guess I can merge this PR now.

@cowtowncoder cowtowncoder merged commit 04731af into FasterXML:2.13 Nov 6, 2022
@szymonprz szymonprz deleted the cleanup-tests-for-null-to-default branch November 7, 2022 08:01
@szymonprz
Copy link
Contributor Author

The problem lies in those lines https://github.com/FasterXML/jackson-module-kotlin/blob/2.14/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt#L54
https://github.com/FasterXML/jackson-module-kotlin/blob/2.14/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt#L61
because we're using some kind of a buffer to read parameterValues and those values are already initialized with default primitive values. So to make this feature working, we need to somehow retrieve the null token from the content to check if the value is present and whether it's not a default primitive. So you suggest to use JsonParser to extract the null from the content? How then we can have a JsonParser instance in KotlinValueInstantiator to use it?

@cowtowncoder
Copy link
Member

No, I don't think JsonParser would necessarily have the needed state any more. I was just saying that all/most information must (have) come from it first, then buffered in other places if stream is advanced.

ValueInstiator would likely need to hold on to information, but I don't really know the implementation here.

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

Successfully merging this pull request may close these issues.

None yet

2 participants