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.

Sign up for GitHub

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

Jump to bottom

Duration support #689

Merged
merged 16 commits into from
Aug 6, 2023
+337 −25
Merged

Duration support #689

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cf5281c
Handle `kotlin.time.Duration` as `java.time.Duration`
kkurczewski Jul 17, 2023
7a61b3f
Add Duration tests for data class
kkurczewski Jul 17, 2023
a1ba894
Use conversion functions for Duration from stdlib-jdk8
kkurczewski Jul 30, 2023
efeb17f
Cleanup
kkurczewski Jul 30, 2023
58ca164
Serialize Kotlin Duration in same way as Java
kkurczewski Jul 30, 2023
bc2a0b3
Remarks
kkurczewski Jul 30, 2023
79d8e4e
Add feature toggle for Duration parsing
kkurczewski Aug 3, 2023
b56e139
Add doc note about current limitations
kkurczewski Aug 5, 2023
9709b83
Remarks
kkurczewski Aug 5, 2023
7161b2b
Fixing converter usage
k163377 Aug 6, 2023
a0151d4
Modified to use DelegatingDeserializer
k163377 Aug 6, 2023
bdc58b4
Add test cases involving mixins
kkurczewski Aug 6, 2023
4fba300
Update feature java doc
kkurczewski Aug 6, 2023
e650766
Fixed conversion to be done by converter only
k163377 Aug 6, 2023
f2e51b4
Add test for deserializing correct units from format annotation
kkurczewski Aug 6, 2023
73bf682
Add related issue reference in docs
kkurczewski Aug 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@
<version>${version.kotlin}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib-jdk8</artifactId>
<version>${version.kotlin}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-reflect</artifactId>
Expand All @@ -121,6 +127,12 @@
<artifactId>jackson-dataformat-xml</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<!-- needed for kotlin.time.Duration converter test -->
<groupId>com.fasterxml.jackson.datatype</groupId>
<artifactId>jackson-datatype-jsr310</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
18 changes: 18 additions & 0 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/Converters.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import com.fasterxml.jackson.databind.ser.std.StdDelegatingSerializer
import com.fasterxml.jackson.databind.type.TypeFactory
import com.fasterxml.jackson.databind.util.StdConverter
import kotlin.reflect.KClass
import kotlin.time.toJavaDuration
import java.time.Duration as JavaDuration
import kotlin.time.Duration as KotlinDuration

internal class SequenceToIteratorConverter(private val input: JavaType) : StdConverter<Sequence<*>, Iterator<*>>() {
override fun convert(value: Sequence<*>): Iterator<*> = value.iterator()
Expand All @@ -16,6 +19,21 @@ internal class SequenceToIteratorConverter(private val input: JavaType) : StdCon
?: typeFactory.constructType(Iterator::class.java)
}

internal object KotlinToJavaDurationConverter : StdConverter<KotlinDuration, JavaDuration>() {
override fun convert(value: KotlinDuration) = value.toJavaDuration()

val delegatingSerializer: StdDelegatingSerializer by lazy { StdDelegatingSerializer(this) }
}

/**
* Currently it is not possible to deduce type of [kotlin.time.Duration] fields therefore explicit annotation is needed on fields in order to properly deserialize POJO.
*
* @see [com.fasterxml.jackson.module.kotlin.test.DurationTests]
*/
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 •
edited
Loading

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.

override fun convert(value: JavaDuration) = KotlinDuration.parseIsoString(value.toString())
}

// S is nullable because value corresponds to a nullable value class
// @see KotlinNamesAnnotationIntrospector.findNullSerializer
internal class ValueClassBoxConverter<S : Any?, D : Any>(
Expand Down
50 changes: 31 additions & 19 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import com.fasterxml.jackson.databind.Module
import com.fasterxml.jackson.databind.cfg.MapperConfig
import com.fasterxml.jackson.databind.introspect.*
import com.fasterxml.jackson.databind.jsontype.NamedType
import com.fasterxml.jackson.databind.ser.std.StdSerializer
import com.fasterxml.jackson.databind.util.Converter
import java.lang.reflect.AccessibleObject
import java.lang.reflect.Constructor
Expand All @@ -23,13 +22,17 @@ import kotlin.reflect.full.createType
import kotlin.reflect.full.declaredMemberProperties
import kotlin.reflect.full.memberProperties
import kotlin.reflect.jvm.*
import kotlin.time.Duration


internal class KotlinAnnotationIntrospector(private val context: Module.SetupContext,
private val cache: ReflectionCache,
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean) : NopAnnotationIntrospector() {
internal class KotlinAnnotationIntrospector(
private val context: Module.SetupContext,
private val cache: ReflectionCache,
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean,
private val useJavaDurationConversion: Boolean,
) : NopAnnotationIntrospector() {

// TODO: implement nullIsSameAsDefault flag, which represents when TRUE that if something has a default value, it can be passed a null to default it
// this likely impacts this class to be accurate about what COULD be considered required
Expand Down Expand Up @@ -66,11 +69,14 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon

override fun findSerializationConverter(a: Annotated): Converter<*, *>? = when (a) {
// Find a converter to handle the case where the getter returns an unboxed value from the value class.
is AnnotatedMethod -> cache.findValueClassReturnType(a)
?.let { cache.getValueClassBoxConverter(a.rawReturnType, it) }
is AnnotatedClass -> a
.takeIf { Sequence::class.java.isAssignableFrom(it.rawType) }
?.let { SequenceToIteratorConverter(it.type) }
is AnnotatedMethod -> a.ktClass()?.let { cache.getValueClassBoxConverter(a.rawReturnType, it) }
is AnnotatedClass -> lookupKotlinTypeConverter(a)
else -> null
}

private fun lookupKotlinTypeConverter(a: AnnotatedClass) = when {
Sequence::class.java.isAssignableFrom(a.rawType) -> SequenceToIteratorConverter(a.type)
Duration::class.java.isAssignableFrom(a.rawType) -> KotlinToJavaDurationConverter.takeIf { useJavaDurationConversion }
k163377 marked this conversation as resolved.
Show resolved Hide resolved
else -> null
}

Expand All @@ -81,11 +87,15 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon

// Perform proper serialization even if the value wrapped by the value class is null.
// If value is a non-null object type, it must not be reboxing.
override fun findNullSerializer(am: Annotated): JsonSerializer<*>? = (am as? AnnotatedMethod)?.let { _ ->
cache.findValueClassReturnType(am)
?.takeIf { it.requireRebox() }
?.let { cache.getValueClassBoxConverter(am.rawReturnType, it).delegatingSerializer }
}
override fun findNullSerializer(am: Annotated): JsonSerializer<*>? = (am as? AnnotatedMethod)
?.ktClass()
?.takeIf { it.requireRebox() }
?.let { cache.getValueClassBoxConverter(am.rawReturnType, it).delegatingSerializer }

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.

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

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.


/**
* Subclasses can be detected automatically for sealed classes, since all possible subclasses are known
Expand All @@ -102,7 +112,7 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon

private fun AnnotatedField.hasRequiredMarker(): Boolean? {
val byAnnotation = (member as Field).isRequiredByAnnotation()
val byNullability = (member as Field).kotlinProperty?.returnType?.isRequired()
val byNullability = (member as Field).kotlinProperty?.returnType?.isRequired()

return requiredAnnotationOrNullability(byAnnotation, byNullability)
}
Expand All @@ -122,7 +132,7 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon
}

private fun Method.isRequiredByAnnotation(): Boolean? {
return (this.annotations.firstOrNull { it.annotationClass.java == JsonProperty::class.java } as? JsonProperty)?.required
return (this.annotations.firstOrNull { it.annotationClass.java == JsonProperty::class.java } as? JsonProperty)?.required
}

// Since Kotlin's property has the same Type for each field, getter, and setter,
Expand Down Expand Up @@ -171,12 +181,14 @@ internal class KotlinAnnotationIntrospector(private val context: Module.SetupCon
return requiredAnnotationOrNullability(byAnnotation, byNullability)
}

private fun AnnotatedMethod.ktClass() = cache.findValueClassReturnType(this)
k163377 marked this conversation as resolved.
Show resolved Hide resolved

private fun KFunction<*>.isConstructorParameterRequired(index: Int): Boolean {
return isParameterRequired(index)
}

private fun KFunction<*>.isMethodParameterRequired(index: Int): Boolean {
return isParameterRequired(index+1)
return isParameterRequired(index + 1)
}

private fun KFunction<*>.isParameterRequired(index: Int): Boolean {
Expand Down
15 changes: 13 additions & 2 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinDeserializers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,21 @@ import com.fasterxml.jackson.databind.JavaType
import com.fasterxml.jackson.databind.JsonDeserializer
import com.fasterxml.jackson.databind.deser.Deserializers
import com.fasterxml.jackson.databind.deser.std.StdDeserializer
import kotlin.time.toKotlinDuration
import java.time.Duration as JavaDuration
import kotlin.time.Duration as KotlinDuration

object SequenceDeserializer : StdDeserializer<Sequence<*>>(Sequence::class.java) {
override fun deserialize(p: JsonParser, ctxt: DeserializationContext): Sequence<*> {
return ctxt.readValue(p, List::class.java).asSequence()
}
}

internal object DurationDeserializer : StdDeserializer<KotlinDuration>(KotlinDuration::class.java) {
override fun deserialize(p: JsonParser, ctxt: DeserializationContext) = ctxt
.readValue(p, JavaDuration::class.java)?.toKotlinDuration()
}

object RegexDeserializer : StdDeserializer<Regex>(Regex::class.java) {
override fun deserialize(p: JsonParser, ctxt: DeserializationContext): Regex {
val node = ctxt.readTree(p)
Expand Down Expand Up @@ -81,11 +89,13 @@ object ULongDeserializer : StdDeserializer<ULong>(ULong::class.java) {
)
}

internal class KotlinDeserializers : Deserializers.Base() {
internal class KotlinDeserializers(
private val useJavaDurationConversion: Boolean,
) : Deserializers.Base() {
override fun findBeanDeserializer(
type: JavaType,
config: DeserializationConfig?,
beanDesc: BeanDescription?
beanDesc: BeanDescription?,
): JsonDeserializer<*>? {
return when {
type.isInterface && type.rawClass == Sequence::class.java -> SequenceDeserializer
Expand All @@ -94,6 +104,7 @@ internal class KotlinDeserializers : Deserializers.Base() {
type.rawClass == UShort::class.java -> UShortDeserializer
type.rawClass == UInt::class.java -> UIntDeserializer
type.rawClass == ULong::class.java -> ULongDeserializer
type.rawClass == KotlinDuration::class.java -> DurationDeserializer.takeIf { useJavaDurationConversion }
else -> null
}
}
Expand Down
9 changes: 8 additions & 1 deletion src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinFeature.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@ enum class KotlinFeature(private val enabledByDefault: Boolean) {
* In addition, the adjustment of behavior using get:JvmName is disabled.
* Note also that this feature does not apply to setters.
*/
KotlinPropertyNameAsImplicitName(enabledByDefault = false);
KotlinPropertyNameAsImplicitName(enabledByDefault = false),

/**
* This feature represents whether to handle [kotlin.time.Duration] using [java.time.Duration] as conversion bridge.
*
* This allows use Kotlin Duration type with [com.fasterxml.jackson.datatype.jsr310.JavaTimeModule].
k163377 marked this conversation as resolved.
Show resolved Hide resolved
*/
UseJavaDurationConversion(enabledByDefault = false);

internal val bitSet: BitSet = (1 shl ordinal).toBitSet()

Expand Down
20 changes: 16 additions & 4 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.fasterxml.jackson.module.kotlin.KotlinFeature.NullToEmptyCollection
import com.fasterxml.jackson.module.kotlin.KotlinFeature.NullToEmptyMap
import com.fasterxml.jackson.module.kotlin.KotlinFeature.StrictNullChecks
import com.fasterxml.jackson.module.kotlin.KotlinFeature.KotlinPropertyNameAsImplicitName
import com.fasterxml.jackson.module.kotlin.KotlinFeature.UseJavaDurationConversion
import com.fasterxml.jackson.module.kotlin.SingletonSupport.CANONICALIZE
import com.fasterxml.jackson.module.kotlin.SingletonSupport.DISABLED
import java.util.*
Expand All @@ -33,6 +34,8 @@ fun Class<*>.isKotlinClass(): Boolean {
* the default, collections which are typed to disallow null members
* (e.g. List<String>) may contain null values after deserialization. Enabling it
* protects against this but has significant performance impact.
* @param useJavaDurationConversion Default: false. Whether to use [java.time.Duration] as a bridge for [kotlin.time.Duration].
* This allows use Kotlin Duration type with [com.fasterxml.jackson.datatype.jsr310.JavaTimeModule].
*/
class KotlinModule @Deprecated(
level = DeprecationLevel.WARNING,
Expand All @@ -55,7 +58,8 @@ class KotlinModule @Deprecated(
val nullIsSameAsDefault: Boolean = false,
val singletonSupport: SingletonSupport = DISABLED,
val strictNullChecks: Boolean = false,
val useKotlinPropertyNameForGetter: Boolean = false
val useKotlinPropertyNameForGetter: Boolean = false,
private val useJavaDurationConversion: Boolean = false,
k163377 marked this conversation as resolved.
Show resolved Hide resolved
) : SimpleModule(KotlinModule::class.java.name, PackageVersion.VERSION) {
init {
if (!KotlinVersion.CURRENT.isAtLeast(1, 5)) {
Expand Down Expand Up @@ -105,7 +109,8 @@ class KotlinModule @Deprecated(
else -> DISABLED
},
builder.isEnabled(StrictNullChecks),
builder.isEnabled(KotlinPropertyNameAsImplicitName)
builder.isEnabled(KotlinPropertyNameAsImplicitName),
builder.isEnabled(UseJavaDurationConversion),
)

companion object {
Expand All @@ -132,7 +137,14 @@ class KotlinModule @Deprecated(
}
}

context.insertAnnotationIntrospector(KotlinAnnotationIntrospector(context, cache, nullToEmptyCollection, nullToEmptyMap, nullIsSameAsDefault))
context.insertAnnotationIntrospector(KotlinAnnotationIntrospector(
context,
cache,
nullToEmptyCollection,
nullToEmptyMap,
nullIsSameAsDefault,
useJavaDurationConversion
))
context.appendAnnotationIntrospector(
KotlinNamesAnnotationIntrospector(
this,
Expand All @@ -141,7 +153,7 @@ class KotlinModule @Deprecated(
useKotlinPropertyNameForGetter)
)

context.addDeserializers(KotlinDeserializers())
context.addDeserializers(KotlinDeserializers(useJavaDurationConversion))
context.addKeyDeserializers(KotlinKeyDeserializers)
context.addSerializers(KotlinSerializers())
context.addKeySerializers(KotlinKeySerializers())
Expand Down
Loading