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

Duration support #689

Merged
merged 16 commits into from
Aug 6, 2023
Merged

Duration support #689

merged 16 commits into from
Aug 6, 2023

Conversation

kkurczewski
Copy link
Contributor

Issue: #683

Adds converter and deserializer which bridges conversions between kotlin.time.Duration and java.time.Duration.

In order to properly deserialize data class with duration following hacks are needed:

  • class need to have explicit static factory annotated with @JsonCreator, primary ctor won't cut it
  • in data class definition value-class getter has to be annotated with Duration converter

Notes:

  • later Kotlin version adds dedicated method kotlin.time.Duration.toJavaDuration() which can be used instead ISO parsing (in converter)
  • ...and adds dedicated extension method java.time.Duration.toKotlinDuration() which can be used instead ISO parsing (in deserializer)

@k163377
Copy link
Contributor

k163377 commented Jul 28, 2023

Sorry, but it seemed better to add a KotlinFeature as you originally suggested.
It seems counter-intuitive to have an error message saying that the JavaTime module is required when processing Kotlin Duration.

// data classes with kotlin.time.Duration field which is a value class
//
// @see DurationTests.`should deserialize Kotlin duration inside data class`
object JavaToKotlinDurationConverter : StdConverter<JavaDuration, KotlinDuration>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to implement a findDeserializationConverter that returns a Converter for an AnnotatedParameter?
I think it can be implemented by just adding Converter.

https://github.com/ProjectMapK/jackson-module-kogera/blob/ca6bfe5a1689ebef4dff795ae21ccbe7ca0248df/src/main/kotlin/io/github/projectmapk/jackson/module/kogera/annotation_introspector/KotlinFallbackAnnotationIntrospector.kt#L92

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can't do that without changes in Jackson core libs.

tl;dr; Jackson extracts parameter of type long and @JsonDeserialization annotation is only source of its real type (in Java-reflection land).

Why it happens

If we check ctors of tested class:

Meeting::class.constructors.forEach { println("kt ctor: $it") }
Meeting::class.java.declaredConstructors.forEach { println("java declared ctor: $it ${it.isSynthetic}") }

We will got:

kt ctor: fun <init>(java.time.Instant, kotlin.time.Duration): DurationTests.Meeting

java declared ctor: private DurationTests$Meeting(java.time.Instant,long) false
java declared ctor: public DurationTests$Meeting(java.time.Instant,long,kotlin.jvm.internal.DefaultConstructorMarker) true

Jackson looks only for Java ctors and removed synthetic ctors from lookup in FasterXML/jackson-databind#1005 hence we will see only:

DurationTests$Meeting(java.time.Instant,long) 

How we can restore original type

Theoretically, we can restore original types via Kotlin reflection but this will work only for synthetic ctor, following code:

Meeting::class.java.declaredConstructors.map { it.kotlinFunction }.forEach { println("restored kt ctor: $it") }

will return:

restored kt ctor: null
restored kt ctor: fun <init>(java.time.Instant, kotlin.time.Duration): DurationTests.Meeting

If we could collect that synthetic constructors (or just check for one with kotlin.jvm.internal.DefaultConstructorMarker param) then we can remove need of explicit @JsonCreator and @JsonDeserializer.

That being said, with current Jackson code we can't customize collection of constructors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since annotations are applied to the type after conversion in serialization, I mistakenly thought that this was also true for deserialization.
If this is the case, I think it is sufficient to implement only the deserializer, so I am very sorry, but please remove JavaToKotlinDurationConverter.

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 this is the case, I think it is sufficient to implement only the deserializer [...]

You mean this?
https://github.com/FasterXML/jackson-module-kotlin/pull/689/files#diff-0fe95cd6b35f4ae8bd509a735e78af8cd195263fdeeb9de7fdb30e1be802bb21R107

This won't cut the bill (at least in current code) because type we see from a ctor definition is a long hence we fall in wrong when-branch. Order in when clause doesn't matter.

so I am very sorry, but please remove JavaToKotlinDurationConverter.

If I remove converter I need to remove it from POJO in "should deserialize Kotlin duration inside data class" and test will fail:

com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `long` from String "PT1H30M": not a valid `long` value
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 44] (through reference chain: com.fasterxml.jackson.module.kotlin.test.DurationTests$Meeting["duration"])

Copy link
Contributor

@k163377 k163377 Aug 6, 2023

Choose a reason for hiding this comment

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

Sorry, I seem to be confused myself.
I have compiled a list of proposed fixes for deserialization, could you please check it out?

The following points have been improved

  • The KotlinDuration argument can now be deserialized without annotation
  • Simplified by using DelegatingDeserializer
  • Replaced Java -> Kotlin conversion with stdlib function

https://github.com/k163377/jackson-module-kotlin/compare/9709b83ebbc08ebbe39ad0931033b63de91bbe66..a0151d422267109a246c95c18242633b235c3ae7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The KotlinDuration argument can now be deserialized without annotation

Nice, thank you!

Simplified by using DelegatingDeserializer

Now I understand what you mean before, you wanted me to replace (remove) deserializer with converter like in case of serialization.

Replaced Java -> Kotlin conversion with stdlib function

Ah, missed that one. Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One minor thing, personally I would add guard condition to avoid return@let and use when at the end but I leave decision up to you.

@k163377 k163377 added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jul 28, 2023
@cowtowncoder cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Aug 1, 2023
@cowtowncoder
Copy link
Member

CLA received.

Adds converter and deserializer which bridges conversions between `kotlin.time.Duration` and `java.time.Duration`.

Notes:
* later Kotlin version adds dedicated method `kotlin.time.Duration.toJavaDuration()` which can be used instead ISO parsing (in converter)
* ...and adds dedicated extension method `java.time.Duration.toKotlinDuration()` which can be used instead ISO parsing (in deserializer)
In order to properly deserialize data class with duration following hacks are needed:
* class need to have explicit static factory annotated with ``@JsonCreator`, primary ctor won't cut it
* in data class definition value-class getter has to be annotated with Duration converter
@kkurczewski
Copy link
Contributor Author

kkurczewski commented Aug 3, 2023

Rebased (other Kotlin feature added which cause conflict)

Comment on lines 95 to 98
override fun findSerializer(am: Annotated): Any? = when ((am as? AnnotatedMethod)?.ktClass()) {
Duration::class -> KotlinToJavaDurationConverter.delegatingSerializer.takeIf { useJavaDurationConversion }
else -> null
} ?: super.findSerializer(am)
Copy link
Contributor

Choose a reason for hiding this comment

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

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 method is solution to nullable fields you mentioned here:

// Need to define in nullable due to bug related to value class
@field:JsonFormat(shape = JsonFormat.Shape.STRING)
val v2: KotlinDuration? = 1.toDuration(DurationUnit.HOURS)

If I comment line 96 then "should serialize Kotlin duration exactly as Java duration" test will fail:

Expected :{"plain":3600.000000000,"optPlain":3600.000000000,"shapeAnnotation":"PT1H","optShapeAnnotation":"PT1H"}
Actual   :{"plain":3600.000000000,"optPlain":3600.000000000,"shapeAnnotation":3600.000000000,"optShapeAnnotation":"PT1H"}

Though I can remove super call fallback if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I guess I misunderstood the function.

However, let me make it a policy to use only Converter as a way of doing things.
An example is pushed below.
https://github.com/k163377/jackson-module-kotlin/compare/a0151d422267109a246c95c18242633b235c3ae7..05d3622b670a016f0daf253d50a914203e7ce586

The policy of using only a Converter will only subdivide within existing conditions, thus reducing the load compared to adding a findSerializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, cherry-picked this commit.

Kotlin Duration is value class and as Jackson has limited support of such cases it falls under few limits:
* data classes with Duration needs to have explicit creator method
* Duration fields has to be annotated with explicit `@JsonDeserializer` annotation

See `DurationTests` for details.
@kkurczewski kkurczewski marked this pull request as ready for review August 5, 2023 06:36
@k163377
Copy link
Contributor

k163377 commented Aug 6, 2023

As for the code, it is LGTM, thanks for the contribution!

@k163377 k163377 merged commit b98c76b into FasterXML:2.16 Aug 6, 2023
k163377 added a commit to k163377/jackson-module-kotlin that referenced this pull request Aug 6, 2023
k163377 added a commit that referenced this pull request Aug 6, 2023
@kkurczewski kkurczewski deleted the duration-support branch August 6, 2023 15:54
k163377 added a commit to ProjectMapK/jackson-module-kogera that referenced this pull request Aug 26, 2023
k163377 added a commit that referenced this pull request Sep 15, 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.

3 participants