Bug fixes to hasRequiredMarker and added isRequired considerations

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

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.

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,33 +125,35 @@ 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
     }
 
     // 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
         }
     }
 
     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)
 

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 <reified T : Any> 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<GitHub922RequiredCollectionsDtoJava>()
+
+        assertTrue(defaultDesc.isRequired("list"))
+        assertTrue(defaultDesc.isRequired("map"))
+
+        val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<GitHub922RequiredCollectionsDtoJava>()
+
+        assertTrue(nullToEmptyDesc.isRequired("list"))
+        assertTrue(nullToEmptyDesc.isRequired("map"))
+    }
+
+    data class RequiredCollectionsDto1(
+        @JsonProperty(required = true) val list: List<String>,
+        @JsonProperty(required = true) val map: Map<String, String>
+    )
 
-        val desc = mapper.introspectDeserialization<GitHub922RequiredCollectionsDtoJava>()
+    data class RequiredCollectionsDto2(
+        @JsonProperty(isRequired = OptBoolean.TRUE) val list: List<String>,
+        @JsonProperty(isRequired = OptBoolean.TRUE) val map: Map<String, String>
+    )
+
+    @Test
+    fun `nullToEmpty does not override specification by annotation`() {
+        val defaultDesc1 = defaultMapper.introspectDeserialization<RequiredCollectionsDto1>()
+
+        assertTrue(defaultDesc1.isRequired("list"))
+        assertTrue(defaultDesc1.isRequired("map"))
+
+        val nullToEmptyDesc1 = nullToEmptyMapper.introspectDeserialization<RequiredCollectionsDto1>()
+
+        assertTrue(nullToEmptyDesc1.isRequired("list"))
+        assertTrue(nullToEmptyDesc1.isRequired("map"))
+
+        val defaultDesc2 = defaultMapper.introspectDeserialization<RequiredCollectionsDto2>()
+
+        assertTrue(defaultDesc2.isRequired("list"))
+        assertTrue(defaultDesc2.isRequired("map"))
+
+        val nullToEmptyDesc2 = nullToEmptyMapper.introspectDeserialization<RequiredCollectionsDto2>()
+
+        assertTrue(nullToEmptyDesc2.isRequired("list"))
+        assertTrue(nullToEmptyDesc2.isRequired("map"))
+    }
+
+    data class CollectionsDto(val list: List<String>, val map: Map<String, String>)
+
+    @Test
+    fun `nullToEmpty does not affect for serialization`() {
+        val defaultDesc = defaultMapper.introspectSerialization<CollectionsDto>()
 
-        assertTrue(desc.isRequired("list"))
-        assertTrue(desc.isRequired("map"))
+        assertTrue(defaultDesc.isRequired("list"))
+        assertTrue(defaultDesc.isRequired("map"))
+
+        val nullToEmptyDesc = nullToEmptyMapper.introspectSerialization<CollectionsDto>()
+
+        assertTrue(nullToEmptyDesc.isRequired("list"))
+        assertTrue(nullToEmptyDesc.isRequired("map"))
+    }
+
+    class SetterCollectionsDto {
+        lateinit var list: List<String>
+        lateinit var map: Map<String, String>
+    }
+
+    @Test
+    fun `nullToEmpty does not affect for setter`() {
+        val defaultDesc = defaultMapper.introspectDeserialization<SetterCollectionsDto>()
+
+        assertTrue(defaultDesc.isRequired("list"))
+        assertTrue(defaultDesc.isRequired("map"))
+
+        val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<SetterCollectionsDto>()
+
+        assertTrue(nullToEmptyDesc.isRequired("list"))
+        assertTrue(nullToEmptyDesc.isRequired("map"))
+    }
+
+    class FieldCollectionsDto {
+        @JvmField
+        var list: List<String> = emptyList()
+        @JvmField
+        var map: Map<String, String> = emptyMap()
+    }
+
+    @Test
+    fun `nullToEmpty does not affect for field`() {
+        val defaultDesc = defaultMapper.introspectDeserialization<FieldCollectionsDto>()
+
+        assertTrue(defaultDesc.isRequired("list"))
+        assertTrue(defaultDesc.isRequired("map"))
+
+        val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<FieldCollectionsDto>()
+
+        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>()
+
+        IsRequiredDto::class.memberProperties.forEach { prop ->
+            val name = prop.name
+            val expected = name.split("_").last().toBoolean()
+
+            assertEquals(expected, desc.isRequired(name), name)
+        }
     }
 }