Duration support #689

kkurczewski · 2023-07-17T14:54:24Z

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 · 2023-07-28T15:59:13Z

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.

src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/DurationTests.kt

src/main/kotlin/com/fasterxml/jackson/module/kotlin/Converters.kt

src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/DurationTests.kt

k163377 · 2023-07-28T18:09:35Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/Converters.kt

+// 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>() {


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

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.

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.

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"])

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

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.

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.

cowtowncoder · 2023-08-01T15:01:23Z

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 · 2023-08-03T16:49:10Z

Rebased (other Kotlin feature added which cause conflict)

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinModule.kt

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt

k163377 · 2023-08-04T17:03:11Z

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt

+    override fun findSerializer(am: Annotated): Any? = when ((am as? AnnotatedMethod)?.ktClass()) {
+        Duration::class -> KotlinToJavaDurationConverter.delegatingSerializer.takeIf { useJavaDurationConversion }
+        else -> null
+    } ?: super.findSerializer(am)


This appears unnecessary since findSerializationConverter is implemented.
https://github.com/ProjectMapK/jackson-module-kogera/blob/2.15.2-beta1/src/main/kotlin/io/github/projectmapk/jackson/module/kogera/annotation_introspector/KotlinFallbackAnnotationIntrospector.kt

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.

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.

Okay, cherry-picked this commit.

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinFeature.kt

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.

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinFeature.kt

k163377 · 2023-08-06T09:29:12Z

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

Co-authored-by: wrongwrong <[email protected]>

Update release notes wrt #689

from FasterXML/jackson-module-kotlin#689

kkurczewski mentioned this pull request Jul 17, 2023

Enable implicit conversion from kotlin.time.Duration to java.time.Duration #683

Closed

k163377 reviewed Jul 28, 2023

View reviewed changes

k163377 added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jul 28, 2023

cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Aug 1, 2023

This was referenced Aug 3, 2023

Use synthetic functions of kotlin jvm inline class FasterXML/jackson-databind#4066

Closed

Support for value classes #693

Closed

kkurczewski added 7 commits August 3, 2023 18:44

Use conversion functions for Duration from stdlib-jdk8

a1ba894

Cleanup

efeb17f

Serialize Kotlin Duration in same way as Java

58ca164

Remarks

bc2a0b3

Add feature toggle for Duration parsing

79d8e4e

kkurczewski force-pushed the duration-support branch from 1e4a09e to 79d8e4e Compare August 3, 2023 16:48

k163377 reviewed Aug 4, 2023

View reviewed changes

kkurczewski added 2 commits August 5, 2023 07:49

Remarks

9709b83

kkurczewski requested a review from k163377 August 5, 2023 06:17

kkurczewski marked this pull request as ready for review August 5, 2023 06:36

k163377 and others added 6 commits August 6, 2023 14:44

Fixing converter usage

7161b2b

Modified to use DelegatingDeserializer

a0151d4

Add test cases involving mixins

bdc58b4

Update feature java doc

4fba300

Fixed conversion to be done by converter only

e650766

Add test for deserializing correct units from format annotation

f2e51b4

k163377 reviewed Aug 6, 2023

View reviewed changes

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinFeature.kt Show resolved Hide resolved

Add related issue reference in docs

73bf682

Co-authored-by: wrongwrong <[email protected]>

kkurczewski requested a review from k163377 August 6, 2023 11:40

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

Update release notes wrt FasterXML#689

5a7cf23

k163377 mentioned this pull request Aug 6, 2023

Update release notes wrt #689 #697

Merged

k163377 added a commit that referenced this pull request Aug 6, 2023

Merge pull request #697 from k163377/update-release-notes

9730a1b

Update release notes wrt #689

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

Porting Kotlin Duration Support

58a1276

from FasterXML/jackson-module-kotlin#689

k163377 mentioned this pull request Aug 26, 2023

Porting Kotlin Duration Support ProjectMapK/jackson-module-kogera#131

Merged

k163377 added a commit that referenced this pull request Sep 15, 2023

Fix for merge error in #689

f5428f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Duration support #689

Duration support #689

kkurczewski commented Jul 17, 2023

k163377 commented Jul 28, 2023

k163377 Jul 28, 2023

kkurczewski Jul 30, 2023

k163377 Aug 4, 2023

kkurczewski Aug 5, 2023

k163377 Aug 6, 2023 •

edited

Loading

kkurczewski Aug 6, 2023

kkurczewski Aug 6, 2023

cowtowncoder commented Aug 1, 2023

kkurczewski commented Aug 3, 2023 •

edited

Loading

k163377 Aug 4, 2023

kkurczewski Aug 5, 2023

k163377 Aug 6, 2023

kkurczewski Aug 6, 2023

k163377 commented Aug 6, 2023

Duration support #689

Duration support #689

Conversation

kkurczewski commented Jul 17, 2023

k163377 commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Why it happens

How we can restore original type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k163377 Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Aug 1, 2023

kkurczewski commented Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k163377 commented Aug 6, 2023

k163377 Aug 6, 2023 •

edited

Loading

kkurczewski commented Aug 3, 2023 •

edited

Loading