From f9cf1a30b702aa481c7291a919bd7790341b34ad Mon Sep 17 00:00:00 2001 From: wrongwrong Date: Thu, 20 Mar 2025 13:19:32 +0900 Subject: [PATCH 1/3] Add test cases --- .../module/kotlin/test/github/GitHub922.kt | 157 +++++++++++++++++- 1 file changed, 150 insertions(+), 7 deletions(-) diff --git a/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/GitHub922.kt b/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/GitHub922.kt index 9150504bf..472fcf842 100644 --- a/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/GitHub922.kt +++ b/src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/GitHub922.kt @@ -1,13 +1,25 @@ package com.fasterxml.jackson.module.kotlin.test.github +import com.fasterxml.jackson.annotation.JsonProperty +import com.fasterxml.jackson.annotation.OptBoolean import com.fasterxml.jackson.databind.BeanDescription import com.fasterxml.jackson.databind.ObjectMapper import com.fasterxml.jackson.module.kotlin.KotlinFeature +import com.fasterxml.jackson.module.kotlin.defaultMapper import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper +import kotlin.reflect.full.memberProperties import kotlin.test.Test +import kotlin.test.assertEquals import kotlin.test.assertTrue class GitHub922 { + companion object { + val nullToEmptyMapper = jacksonObjectMapper { + enable(KotlinFeature.NullToEmptyCollection) + enable(KotlinFeature.NullToEmptyMap) + } + } + private inline fun ObjectMapper.introspectSerialization(): BeanDescription = serializationConfig.introspect(serializationConfig.constructType(T::class.java)) @@ -19,14 +31,145 @@ class GitHub922 { @Test fun `nullToEmpty does not override specification by Java annotation`() { - val mapper = jacksonObjectMapper { - enable(KotlinFeature.NullToEmptyCollection) - enable(KotlinFeature.NullToEmptyMap) - } + val defaultDesc = defaultMapper.introspectDeserialization() + + assertTrue(defaultDesc.isRequired("list")) + assertTrue(defaultDesc.isRequired("map")) + + val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization() + + assertTrue(nullToEmptyDesc.isRequired("list")) + assertTrue(nullToEmptyDesc.isRequired("map")) + } + + data class RequiredCollectionsDto1( + @JsonProperty(required = true) val list: List, + @JsonProperty(required = true) val map: Map + ) - val desc = mapper.introspectDeserialization() + data class RequiredCollectionsDto2( + @JsonProperty(isRequired = OptBoolean.TRUE) val list: List, + @JsonProperty(isRequired = OptBoolean.TRUE) val map: Map + ) + + @Test + fun `nullToEmpty does not override specification by annotation`() { + val defaultDesc1 = defaultMapper.introspectDeserialization() + + assertTrue(defaultDesc1.isRequired("list")) + assertTrue(defaultDesc1.isRequired("map")) + + val nullToEmptyDesc1 = nullToEmptyMapper.introspectDeserialization() + + assertTrue(nullToEmptyDesc1.isRequired("list")) + assertTrue(nullToEmptyDesc1.isRequired("map")) + + val defaultDesc2 = defaultMapper.introspectDeserialization() + + assertTrue(defaultDesc2.isRequired("list")) + assertTrue(defaultDesc2.isRequired("map")) + + val nullToEmptyDesc2 = nullToEmptyMapper.introspectDeserialization() + + assertTrue(nullToEmptyDesc2.isRequired("list")) + assertTrue(nullToEmptyDesc2.isRequired("map")) + } + + data class CollectionsDto(val list: List, val map: Map) + + @Test + fun `nullToEmpty does not affect for serialization`() { + val defaultDesc = defaultMapper.introspectSerialization() - assertTrue(desc.isRequired("list")) - assertTrue(desc.isRequired("map")) + assertTrue(defaultDesc.isRequired("list")) + assertTrue(defaultDesc.isRequired("map")) + + val nullToEmptyDesc = nullToEmptyMapper.introspectSerialization() + + assertTrue(nullToEmptyDesc.isRequired("list")) + assertTrue(nullToEmptyDesc.isRequired("map")) + } + + class SetterCollectionsDto { + lateinit var list: List + lateinit var map: Map + } + + @Test + fun `nullToEmpty does not affect for setter`() { + val defaultDesc = defaultMapper.introspectDeserialization() + + assertTrue(defaultDesc.isRequired("list")) + assertTrue(defaultDesc.isRequired("map")) + + val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization() + + assertTrue(nullToEmptyDesc.isRequired("list")) + assertTrue(nullToEmptyDesc.isRequired("map")) + } + + class FieldCollectionsDto { + @JvmField + var list: List = emptyList() + @JvmField + var map: Map = emptyMap() + } + + @Test + fun `nullToEmpty does not affect for field`() { + val defaultDesc = defaultMapper.introspectDeserialization() + + assertTrue(defaultDesc.isRequired("list")) + assertTrue(defaultDesc.isRequired("map")) + + val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization() + + assertTrue(nullToEmptyDesc.isRequired("list")) + assertTrue(nullToEmptyDesc.isRequired("map")) + } + + // isRequired_required_nullability_expected + @Suppress("PropertyName") + data class IsRequiredDto( + // region: isRequired takes precedence + @JsonProperty(isRequired = OptBoolean.FALSE, required = false) + val FALSE_false_nullable_false: String?, + @JsonProperty(isRequired = OptBoolean.FALSE, required = false) + val FALSE_false_nonNull_false: String, + @JsonProperty(isRequired = OptBoolean.FALSE, required = true) + val FALSE_true_nullable_false: String?, + @JsonProperty(isRequired = OptBoolean.FALSE, required = true) + val FALSE_true_nonNull_false: String, + @JsonProperty(isRequired = OptBoolean.TRUE, required = false) + val TRUE_false_nullable_true: String?, + @JsonProperty(isRequired = OptBoolean.TRUE, required = false) + val TRUE_false_nonNull_true: String, + @JsonProperty(isRequired = OptBoolean.TRUE, required = true) + val TRUE_true_nullable_true: String?, + @JsonProperty(isRequired = OptBoolean.TRUE, required = true) + val TRUE_true_nonNull_true: String, + // endregion + // region: If isRequired is the default, only overrides by required = true will work. + @JsonProperty(isRequired = OptBoolean.DEFAULT, required = false) + val DEFAULT_false_nullable_false: String?, + @JsonProperty(isRequired = OptBoolean.DEFAULT, required = false) + val DEFAULT_false_nonNull_true: String, + @JsonProperty(isRequired = OptBoolean.DEFAULT, required = true) + val DEFAULT_true_nullable_true: String?, + @JsonProperty(isRequired = OptBoolean.DEFAULT, required = true) + val DEFAULT_true_nonNull_true: String, + // endregion + ) + + @Test + fun `JsonProperty properly overrides required`() { + val desc = defaultMapper.introspectDeserialization() + + IsRequiredDto::class.memberProperties.forEach { prop -> + val name = prop.name + val expected = name.split("_").last().toBoolean() + + assertEquals(expected, desc.isRequired(name), name) + } } } From cc064732106347078077bc1ba3fd2a8c53823577 Mon Sep 17 00:00:00 2001 From: wrongwrong Date: Thu, 20 Mar 2025 14:53:57 +0900 Subject: [PATCH 2/3] Fixed handling of nullToEmpty options Fixed the problem that the nullToEmpty options overrides the specification by annotation and the nullToEmpty options affects other than parameters. This fixes #922 completely. Also, considerations regarding JsonProperty.isRequired have been added. --- .../kotlin/KotlinAnnotationIntrospector.kt | 79 ++++++++----------- 1 file changed, 35 insertions(+), 44 deletions(-) diff --git a/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt b/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt index 7efb65377..0054b3659 100644 --- a/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt +++ b/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinAnnotationIntrospector.kt @@ -1,6 +1,7 @@ package com.fasterxml.jackson.module.kotlin import com.fasterxml.jackson.annotation.JsonProperty +import com.fasterxml.jackson.annotation.OptBoolean import com.fasterxml.jackson.databind.DeserializationFeature import com.fasterxml.jackson.databind.JsonSerializer import com.fasterxml.jackson.databind.Module @@ -36,20 +37,27 @@ internal class KotlinAnnotationIntrospector( // 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 + // If a new isRequired is explicitly specified or the old required is true, those values take precedence. + // In other cases, override is done by KotlinModule. + private fun JsonProperty.forceRequiredByAnnotation(): Boolean? = when { + isRequired != OptBoolean.DEFAULT -> isRequired.asBoolean() + required -> true + else -> null + } + + private fun AccessibleObject.forceRequiredByAnnotation(): Boolean? = + getAnnotation(JsonProperty::class.java)?.forceRequiredByAnnotation() + override fun hasRequiredMarker( m: AnnotatedMember ): Boolean? = m.takeIf { it.member.declaringClass.isKotlinClass() }?.let { _ -> cache.javaMemberIsRequired(m) { try { - when { - nullToEmptyCollection && m.type.isCollectionLikeType -> false - nullToEmptyMap && m.type.isMapLikeType -> false - else -> when (m) { - is AnnotatedField -> m.hasRequiredMarker() - is AnnotatedMethod -> m.hasRequiredMarker() - is AnnotatedParameter -> m.hasRequiredMarker() - else -> null - } + when (m) { + is AnnotatedField -> m.hasRequiredMarker() + is AnnotatedMethod -> m.hasRequiredMarker() + is AnnotatedParameter -> m.hasRequiredMarker() + else -> null } } catch (_: UnsupportedOperationException) { null @@ -100,28 +108,9 @@ internal class KotlinAnnotationIntrospector( } private fun AnnotatedField.hasRequiredMarker(): Boolean? { - val byAnnotation = (member as Field).isRequiredByAnnotation() - val byNullability = (member as Field).kotlinProperty?.returnType?.isRequired() - - return requiredAnnotationOrNullability(byAnnotation, byNullability) - } - - private fun AccessibleObject.isRequiredByAnnotation(): Boolean? = annotations - .firstOrNull { it.annotationClass == JsonProperty::class } - ?.let { it as JsonProperty } - ?.required - - private fun requiredAnnotationOrNullability(byAnnotation: Boolean?, byNullability: Boolean?): Boolean? { - if (byAnnotation != null && byNullability != null) { - return byAnnotation || byNullability - } else if (byNullability != null) { - return byNullability - } - return byAnnotation - } - - private fun Method.isRequiredByAnnotation(): Boolean? { - return (this.annotations.firstOrNull { it.annotationClass.java == JsonProperty::class.java } as? JsonProperty)?.required + val field = member as Field + return field.forceRequiredByAnnotation() + ?: field.kotlinProperty?.returnType?.isRequired() } // Since Kotlin's property has the same Type for each field, getter, and setter, @@ -136,9 +125,7 @@ internal class KotlinAnnotationIntrospector( private fun AnnotatedMethod.getRequiredMarkerFromCorrespondingAccessor(): Boolean? { member.declaringClass.kotlin.declaredMemberProperties.forEach { kProperty -> if (kProperty.javaGetter == this.member || (kProperty as? KMutableProperty1)?.javaSetter == this.member) { - val byAnnotation = this.member.isRequiredByAnnotation() - val byNullability = kProperty.isRequiredByNullability() - return requiredAnnotationOrNullability(byAnnotation, byNullability) + return member.forceRequiredByAnnotation() ?: kProperty.isRequiredByNullability() } } return null @@ -146,10 +133,11 @@ internal class KotlinAnnotationIntrospector( // Is the member method a regular method of the data class or private fun Method.getRequiredMarkerFromAccessorLikeMethod(): Boolean? = cache.kotlinFromJava(this)?.let { func -> - val byAnnotation = this.isRequiredByAnnotation() - return when { - func.isGetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.returnType.isRequired()) - func.isSetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.valueParameters[0].isRequired()) + forceRequiredByAnnotation() ?: when { + func.isGetterLike() -> func.returnType.isRequired() + // If nullToEmpty could be supported for setters, + // a branch similar to AnnotatedParameter.hasRequiredMarker should be added. + func.isSetterLike() -> func.valueParameters[0].isRequired() else -> null } } @@ -157,12 +145,15 @@ internal class KotlinAnnotationIntrospector( private fun KFunction<*>.isGetterLike(): Boolean = parameters.size == 1 private fun KFunction<*>.isSetterLike(): Boolean = parameters.size == 2 && returnType == UNIT_TYPE - private fun AnnotatedParameter.hasRequiredMarker(): Boolean? { - val byAnnotation = this.getAnnotation(JsonProperty::class.java)?.required - val byNullability = cache.findKotlinParameter(this)?.isRequired() - - return requiredAnnotationOrNullability(byAnnotation, byNullability) - } + private fun AnnotatedParameter.hasRequiredMarker(): Boolean? = getAnnotation(JsonProperty::class.java) + ?.forceRequiredByAnnotation() + ?: run { + when { + nullToEmptyCollection && type.isCollectionLikeType -> false + nullToEmptyMap && type.isMapLikeType -> false + else -> cache.findKotlinParameter(this)?.isRequired() + } + } private fun AnnotatedMethod.findValueClassReturnType() = cache.findValueClassReturnType(this) From e723ad45b9226107d2d2fb59a469d72f73a46dbf Mon Sep 17 00:00:00 2001 From: wrongwrong Date: Thu, 20 Mar 2025 15:28:03 +0900 Subject: [PATCH 3/3] Update release notes wrt #929 --- release-notes/CREDITS-2.x | 1 + release-notes/VERSION-2.x | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/release-notes/CREDITS-2.x b/release-notes/CREDITS-2.x index b75d41aa4..35aa72212 100644 --- a/release-notes/CREDITS-2.x +++ b/release-notes/CREDITS-2.x @@ -21,6 +21,7 @@ Tatu Saloranta (@cowtowncoder) * #889: Upgrade kotlin dep to 1.9.25 (from 1.9.24) WrongWrong (@k163377) +* #929: Bug fixes to hasRequiredMarker and added isRequired considerations * #914: Add test case to serialize Nothing? (for #314) * #910: Add default KeyDeserializer for value class * #885: Performance improvement of strictNullChecks diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index dc58c1203..f162750b8 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -18,6 +18,18 @@ Co-maintainers: 2.19.0 (not yet released) +#929: Added consideration of `JsonProperty.isRequired` added in `2.19` in `hasRequiredMarker` processing. + Previously `JsonProperty.required` was defined as `Boolean` with default `false`, + so `KotlinModule` was forced to override it if the value was `false`. + This made it impossible for users to override the parsed result by `KotlinModule`. + The new `JsonProperty.isRequired` is defined with three values, including the default, + so `KotlinModule` can now respect user specifications. +#929: Fixed a problem with the `NullToEmptyCollection` and `NullToEmptyMap` options overriding annotated specifications + in the `hasRequiredMarker` process. +#929: Fixed a problem with the `NullToEmptyCollection` and `NullToEmptyMap` options being applied to non-parameters + in the `hasRequiredMarker` process. + They currently do not work for setters or fields and are not related to serialization, + but were being incorrectly applied to their `required` decisions. #910: A default `KeyDeserializer` for `value class` has been added. This eliminates the need to have a custom `KeyDeserializer` for each `value class` when using it as a key in a `Map`, if only simple boxing is needed. #889: Kotlin has been upgraded to 1.9.25.