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

Improve serialization support for value class #659

Merged
merged 8 commits into from
Apr 4, 2023

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Apr 2, 2023

@k163377 k163377 changed the title 【WIP】Improved serialization support for value class 【WIP】Improve serialization support for value class Apr 2, 2023
@k163377 k163377 changed the title 【WIP】Improve serialization support for value class Improve serialization support for value class Apr 4, 2023
@k163377 k163377 marked this pull request as ready for review April 4, 2023 13:30
@@ -59,12 +58,12 @@ class GitHub524 {

class SerializeByAnnotation(@get:JsonSerialize(using = Serializer::class) val foo: HasSerializer = HasSerializer(1))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the Serializer did not work because of a mismatch in the return type from the getter.
With the change in this PR to use Converter, the types will now match and the Serializer will work.

// Decompile result for this getter, the return type is not HasSerializer due to unboxing
@JsonSerialize(using = Serializer.class)
@NotNull
public final Integer getFoo-Q01F06U() {

// Determine if the unbox result of value class is nullAable
// @see findNullSerializer
private fun KClass<*>.requireRebox(): Boolean =
this.memberProperties.first { it.javaField != null }.returnType.isMarkedNullable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will eliminate the bug described in the following comment.
#618 (comment)

@k163377
Copy link
Contributor Author

k163377 commented Apr 4, 2023

@cowtowncoder
How should this PR be handled?
No one seems to exist who can review this.

Personally, I think it is safe to merge the existing value class since the tests on serialization have been successful.

@cowtowncoder
Copy link
Member

@cowtowncoder How should this PR be handled? No one seems to exist who can review this.

Personally, I think it is safe to merge the existing value class since the tests on serialization have been successful.

You can decide whether to merge it in 2.15 (there is still time), or wait until 2.15.0 is out and merge into 2.16 (once branch exists). I don't have a strong opinion.

I'll cc @dinomite nor @viartemev just in case they might have some feedback; but in absence of other reviewers you are free to decide on how to proceed, basically.

@k163377
Copy link
Contributor Author

k163377 commented Apr 4, 2023

OK, merge immediately.

@k163377 k163377 merged commit 849e1dd into FasterXML:2.15 Apr 4, 2023
@k163377 k163377 deleted the fix-value-class-ser branch April 4, 2023 15:11
k163377 added a commit to k163377/jackson-module-kotlin that referenced this pull request Apr 4, 2023
k163377 added a commit that referenced this pull request Apr 4, 2023
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