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

Improvements to serialization of value class. #527

Merged
merged 7 commits into from
Jan 1, 2022

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Dec 18, 2021

Fixed a problem where the Serializer set for the value class was not working if it was unboxed by the getter.
This also fixes #524.

Additional contexts.

1

This fix is a destructive change.
If a Serializer is already set for a value class, it may change the response.

2

This fix adds reflection to all getters, which will reduce serialization performance.

3

Support for the JsonSerialize annotation could not be added.
The reasons as follows.

  • I did not know the proper way to find the JsonSerialize annotation.
    • jackson-module-kotlin seems to handle not only getter, but also annotations set to constructor parameter and so on.
  • I did not know the correspondence between the fields that can be set in the JsonSerialize annotation and the contents returned by findSerializer.

@k163377 k163377 changed the title Fixes to the way the Serializer for value class works in all cases. Improvements to serialization of value class. Dec 18, 2021
}

// Perform proper serialization even if the value wrapped by the value class is null.
override fun findNullSerializer(am: Annotated) = findSerializer(am)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work as expected? Call should only be made if the value class itself is null; not for wrapped value. I may be wrong since I am not familiar with types here, just suggest validation of all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the property itself is null, the return value from the java getter will be in an unboxed state, so the ValueClassBoxSerializer will not be used.
This is verified by the following comparison.
https://github.com/FasterXML/jackson-module-kotlin/pull/527/files#diff-7c6b0a8c396b759fd146df36577c7a8fb3cbe94406e6912a729f6e028413411bR49-R51

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. As long as unit tests cover expected behavior it's probably fine.

One other note: "null serializer" is only ever used for writing null value or placeholder, and never anything more advanced. So one typically returns sort of special constant value serializing implementation, instead of delegating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cowtowncoder @tatu-at-datastax
Sorry if I'm not reading this correctly.

The findNullSerializer returns a serializer for values that are non-null in Kotlin.
I meant to add a comment to explain that.

I think it might have been better to have a private function named something like findSerializerForValueClass and call the same function in findSerializer/findNullSerializer.
If you have any comments or implementations that are easier to understand, please let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is simply this: method findNullSerializer is called by Jackson only EVER when there is a literal JVM-level null to serialize. Regular serializers are NEVER given null values to serialize; they are not expected to (have to) do null checks, nor know what to serialize.

In that sense it seems/seemed odd that implementation would delegate to findSerializer(): I am not sure why this is done. It seems rather like it should not be defined at all.

But then again Kotlin module sometimes overrides things I do not expect it to, partially since null handling in Kotlin is quite different from plain Java.

I'll let you and Kotlin module owners decide here what to do; I just try to give input from Jackson databind side to help develop things :)

@dinomite dinomite self-assigned this Dec 21, 2021
@dinomite dinomite merged commit eb7a12a into FasterXML:2.13 Jan 1, 2022
@k163377 k163377 deleted the github-524/fix branch January 2, 2022 01:29
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.

4 participants