Skip to content

Commit 819f4b2

Browse files
k163377k163377
authored
Merge pull request #929 from k163377/922-intr
Bug fixes to `hasRequiredMarker` and added `isRequired` considerations
2 parents 8fe864e + e723ad4 commit 819f4b2

File tree

4 files changed

+198
-51
lines changed

4 files changed

+198
-51
lines changed

‎release-notes/CREDITS-2.x

+1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ Tatu Saloranta (@cowtowncoder)
2121
* #889: Upgrade kotlin dep to 1.9.25 (from 1.9.24)
2222

2323
WrongWrong (@k163377)
24+
* #929: Bug fixes to hasRequiredMarker and added isRequired considerations
2425
* #914: Add test case to serialize Nothing? (for #314)
2526
* #910: Add default KeyDeserializer for value class
2627
* #885: Performance improvement of strictNullChecks

‎release-notes/VERSION-2.x

+12
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,18 @@ Co-maintainers:
1818

1919
2.19.0 (not yet released)
2020

21+
#929: Added consideration of `JsonProperty.isRequired` added in `2.19` in `hasRequiredMarker` processing.
22+
Previously `JsonProperty.required` was defined as `Boolean` with default `false`,
23+
so `KotlinModule` was forced to override it if the value was `false`.
24+
This made it impossible for users to override the parsed result by `KotlinModule`.
25+
The new `JsonProperty.isRequired` is defined with three values, including the default,
26+
so `KotlinModule` can now respect user specifications.
27+
#929: Fixed a problem with the `NullToEmptyCollection` and `NullToEmptyMap` options overriding annotated specifications
28+
in the `hasRequiredMarker` process.
29+
#929: Fixed a problem with the `NullToEmptyCollection` and `NullToEmptyMap` options being applied to non-parameters
30+
in the `hasRequiredMarker` process.
31+
They currently do not work for setters or fields and are not related to serialization,
32+
but were being incorrectly applied to their `required` decisions.
2133
#910: A default `KeyDeserializer` for `value class` has been added.
2234
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.
2335
#889: Kotlin has been upgraded to 1.9.25.

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

+35-44
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.fasterxml.jackson.module.kotlin
22

33
import com.fasterxml.jackson.annotation.JsonProperty
4+
import com.fasterxml.jackson.annotation.OptBoolean
45
import com.fasterxml.jackson.databind.DeserializationFeature
56
import com.fasterxml.jackson.databind.JsonSerializer
67
import com.fasterxml.jackson.databind.Module
@@ -36,20 +37,27 @@ internal class KotlinAnnotationIntrospector(
3637
// TODO: implement nullIsSameAsDefault flag, which represents when TRUE that if something has a default value, it can be passed a null to default it
3738
// this likely impacts this class to be accurate about what COULD be considered required
3839

40+
// If a new isRequired is explicitly specified or the old required is true, those values take precedence.
41+
// In other cases, override is done by KotlinModule.
42+
private fun JsonProperty.forceRequiredByAnnotation(): Boolean? = when {
43+
isRequired != OptBoolean.DEFAULT -> isRequired.asBoolean()
44+
required -> true
45+
else -> null
46+
}
47+
48+
private fun AccessibleObject.forceRequiredByAnnotation(): Boolean? =
49+
getAnnotation(JsonProperty::class.java)?.forceRequiredByAnnotation()
50+
3951
override fun hasRequiredMarker(
4052
m: AnnotatedMember
4153
): Boolean? = m.takeIf { it.member.declaringClass.isKotlinClass() }?.let { _ ->
4254
cache.javaMemberIsRequired(m) {
4355
try {
44-
when {
45-
nullToEmptyCollection && m.type.isCollectionLikeType -> false
46-
nullToEmptyMap && m.type.isMapLikeType -> false
47-
else -> when (m) {
48-
is AnnotatedField -> m.hasRequiredMarker()
49-
is AnnotatedMethod -> m.hasRequiredMarker()
50-
is AnnotatedParameter -> m.hasRequiredMarker()
51-
else -> null
52-
}
56+
when (m) {
57+
is AnnotatedField -> m.hasRequiredMarker()
58+
is AnnotatedMethod -> m.hasRequiredMarker()
59+
is AnnotatedParameter -> m.hasRequiredMarker()
60+
else -> null
5361
}
5462
} catch (_: UnsupportedOperationException) {
5563
null
@@ -100,28 +108,9 @@ internal class KotlinAnnotationIntrospector(
100108
}
101109

102110
private fun AnnotatedField.hasRequiredMarker(): Boolean? {
103-
val byAnnotation = (member as Field).isRequiredByAnnotation()
104-
val byNullability = (member as Field).kotlinProperty?.returnType?.isRequired()
105-
106-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
107-
}
108-
109-
private fun AccessibleObject.isRequiredByAnnotation(): Boolean? = annotations
110-
.firstOrNull { it.annotationClass == JsonProperty::class }
111-
?.let { it as JsonProperty }
112-
?.required
113-
114-
private fun requiredAnnotationOrNullability(byAnnotation: Boolean?, byNullability: Boolean?): Boolean? {
115-
if (byAnnotation != null && byNullability != null) {
116-
return byAnnotation || byNullability
117-
} else if (byNullability != null) {
118-
return byNullability
119-
}
120-
return byAnnotation
121-
}
122-
123-
private fun Method.isRequiredByAnnotation(): Boolean? {
124-
return (this.annotations.firstOrNull { it.annotationClass.java == JsonProperty::class.java } as? JsonProperty)?.required
111+
val field = member as Field
112+
return field.forceRequiredByAnnotation()
113+
?: field.kotlinProperty?.returnType?.isRequired()
125114
}
126115

127116
// Since Kotlin's property has the same Type for each field, getter, and setter,
@@ -136,33 +125,35 @@ internal class KotlinAnnotationIntrospector(
136125
private fun AnnotatedMethod.getRequiredMarkerFromCorrespondingAccessor(): Boolean? {
137126
member.declaringClass.kotlin.declaredMemberProperties.forEach { kProperty ->
138127
if (kProperty.javaGetter == this.member || (kProperty as? KMutableProperty1)?.javaSetter == this.member) {
139-
val byAnnotation = this.member.isRequiredByAnnotation()
140-
val byNullability = kProperty.isRequiredByNullability()
141-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
128+
return member.forceRequiredByAnnotation() ?: kProperty.isRequiredByNullability()
142129
}
143130
}
144131
return null
145132
}
146133

147134
// Is the member method a regular method of the data class or
148135
private fun Method.getRequiredMarkerFromAccessorLikeMethod(): Boolean? = cache.kotlinFromJava(this)?.let { func ->
149-
val byAnnotation = this.isRequiredByAnnotation()
150-
return when {
151-
func.isGetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.returnType.isRequired())
152-
func.isSetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.valueParameters[0].isRequired())
136+
forceRequiredByAnnotation() ?: when {
137+
func.isGetterLike() -> func.returnType.isRequired()
138+
// If nullToEmpty could be supported for setters,
139+
// a branch similar to AnnotatedParameter.hasRequiredMarker should be added.
140+
func.isSetterLike() -> func.valueParameters[0].isRequired()
153141
else -> null
154142
}
155143
}
156144

157145
private fun KFunction<*>.isGetterLike(): Boolean = parameters.size == 1
158146
private fun KFunction<*>.isSetterLike(): Boolean = parameters.size == 2 && returnType == UNIT_TYPE
159147

160-
private fun AnnotatedParameter.hasRequiredMarker(): Boolean? {
161-
val byAnnotation = this.getAnnotation(JsonProperty::class.java)?.required
162-
val byNullability = cache.findKotlinParameter(this)?.isRequired()
163-
164-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
165-
}
148+
private fun AnnotatedParameter.hasRequiredMarker(): Boolean? = getAnnotation(JsonProperty::class.java)
149+
?.forceRequiredByAnnotation()
150+
?: run {
151+
when {
152+
nullToEmptyCollection && type.isCollectionLikeType -> false
153+
nullToEmptyMap && type.isMapLikeType -> false
154+
else -> cache.findKotlinParameter(this)?.isRequired()
155+
}
156+
}
166157

167158
private fun AnnotatedMethod.findValueClassReturnType() = cache.findValueClassReturnType(this)
168159

‎src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/GitHub922.kt

+150-7
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,25 @@
11
package com.fasterxml.jackson.module.kotlin.test.github
22

3+
import com.fasterxml.jackson.annotation.JsonProperty
4+
import com.fasterxml.jackson.annotation.OptBoolean
35
import com.fasterxml.jackson.databind.BeanDescription
46
import com.fasterxml.jackson.databind.ObjectMapper
57
import com.fasterxml.jackson.module.kotlin.KotlinFeature
8+
import com.fasterxml.jackson.module.kotlin.defaultMapper
69
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
10+
import kotlin.reflect.full.memberProperties
711
import kotlin.test.Test
12+
import kotlin.test.assertEquals
813
import kotlin.test.assertTrue
914

1015
class GitHub922 {
16+
companion object {
17+
val nullToEmptyMapper = jacksonObjectMapper {
18+
enable(KotlinFeature.NullToEmptyCollection)
19+
enable(KotlinFeature.NullToEmptyMap)
20+
}
21+
}
22+
1123
private inline fun <reified T : Any> ObjectMapper.introspectSerialization(): BeanDescription =
1224
serializationConfig.introspect(serializationConfig.constructType(T::class.java))
1325

@@ -19,14 +31,145 @@ class GitHub922 {
1931

2032
@Test
2133
fun `nullToEmpty does not override specification by Java annotation`() {
22-
val mapper = jacksonObjectMapper {
23-
enable(KotlinFeature.NullToEmptyCollection)
24-
enable(KotlinFeature.NullToEmptyMap)
25-
}
34+
val defaultDesc = defaultMapper.introspectDeserialization<GitHub922RequiredCollectionsDtoJava>()
35+
36+
assertTrue(defaultDesc.isRequired("list"))
37+
assertTrue(defaultDesc.isRequired("map"))
38+
39+
val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<GitHub922RequiredCollectionsDtoJava>()
40+
41+
assertTrue(nullToEmptyDesc.isRequired("list"))
42+
assertTrue(nullToEmptyDesc.isRequired("map"))
43+
}
44+
45+
data class RequiredCollectionsDto1(
46+
@JsonProperty(required = true) val list: List<String>,
47+
@JsonProperty(required = true) val map: Map<String, String>
48+
)
2649

27-
val desc = mapper.introspectDeserialization<GitHub922RequiredCollectionsDtoJava>()
50+
data class RequiredCollectionsDto2(
51+
@JsonProperty(isRequired = OptBoolean.TRUE) val list: List<String>,
52+
@JsonProperty(isRequired = OptBoolean.TRUE) val map: Map<String, String>
53+
)
54+
55+
@Test
56+
fun `nullToEmpty does not override specification by annotation`() {
57+
val defaultDesc1 = defaultMapper.introspectDeserialization<RequiredCollectionsDto1>()
58+
59+
assertTrue(defaultDesc1.isRequired("list"))
60+
assertTrue(defaultDesc1.isRequired("map"))
61+
62+
val nullToEmptyDesc1 = nullToEmptyMapper.introspectDeserialization<RequiredCollectionsDto1>()
63+
64+
assertTrue(nullToEmptyDesc1.isRequired("list"))
65+
assertTrue(nullToEmptyDesc1.isRequired("map"))
66+
67+
val defaultDesc2 = defaultMapper.introspectDeserialization<RequiredCollectionsDto2>()
68+
69+
assertTrue(defaultDesc2.isRequired("list"))
70+
assertTrue(defaultDesc2.isRequired("map"))
71+
72+
val nullToEmptyDesc2 = nullToEmptyMapper.introspectDeserialization<RequiredCollectionsDto2>()
73+
74+
assertTrue(nullToEmptyDesc2.isRequired("list"))
75+
assertTrue(nullToEmptyDesc2.isRequired("map"))
76+
}
77+
78+
data class CollectionsDto(val list: List<String>, val map: Map<String, String>)
79+
80+
@Test
81+
fun `nullToEmpty does not affect for serialization`() {
82+
val defaultDesc = defaultMapper.introspectSerialization<CollectionsDto>()
2883

29-
assertTrue(desc.isRequired("list"))
30-
assertTrue(desc.isRequired("map"))
84+
assertTrue(defaultDesc.isRequired("list"))
85+
assertTrue(defaultDesc.isRequired("map"))
86+
87+
val nullToEmptyDesc = nullToEmptyMapper.introspectSerialization<CollectionsDto>()
88+
89+
assertTrue(nullToEmptyDesc.isRequired("list"))
90+
assertTrue(nullToEmptyDesc.isRequired("map"))
91+
}
92+
93+
class SetterCollectionsDto {
94+
lateinit var list: List<String>
95+
lateinit var map: Map<String, String>
96+
}
97+
98+
@Test
99+
fun `nullToEmpty does not affect for setter`() {
100+
val defaultDesc = defaultMapper.introspectDeserialization<SetterCollectionsDto>()
101+
102+
assertTrue(defaultDesc.isRequired("list"))
103+
assertTrue(defaultDesc.isRequired("map"))
104+
105+
val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<SetterCollectionsDto>()
106+
107+
assertTrue(nullToEmptyDesc.isRequired("list"))
108+
assertTrue(nullToEmptyDesc.isRequired("map"))
109+
}
110+
111+
class FieldCollectionsDto {
112+
@JvmField
113+
var list: List<String> = emptyList()
114+
@JvmField
115+
var map: Map<String, String> = emptyMap()
116+
}
117+
118+
@Test
119+
fun `nullToEmpty does not affect for field`() {
120+
val defaultDesc = defaultMapper.introspectDeserialization<FieldCollectionsDto>()
121+
122+
assertTrue(defaultDesc.isRequired("list"))
123+
assertTrue(defaultDesc.isRequired("map"))
124+
125+
val nullToEmptyDesc = nullToEmptyMapper.introspectDeserialization<FieldCollectionsDto>()
126+
127+
assertTrue(nullToEmptyDesc.isRequired("list"))
128+
assertTrue(nullToEmptyDesc.isRequired("map"))
129+
}
130+
131+
// isRequired_required_nullability_expected
132+
@Suppress("PropertyName")
133+
data class IsRequiredDto(
134+
// region: isRequired takes precedence
135+
@JsonProperty(isRequired = OptBoolean.FALSE, required = false)
136+
val FALSE_false_nullable_false: String?,
137+
@JsonProperty(isRequired = OptBoolean.FALSE, required = false)
138+
val FALSE_false_nonNull_false: String,
139+
@JsonProperty(isRequired = OptBoolean.FALSE, required = true)
140+
val FALSE_true_nullable_false: String?,
141+
@JsonProperty(isRequired = OptBoolean.FALSE, required = true)
142+
val FALSE_true_nonNull_false: String,
143+
@JsonProperty(isRequired = OptBoolean.TRUE, required = false)
144+
val TRUE_false_nullable_true: String?,
145+
@JsonProperty(isRequired = OptBoolean.TRUE, required = false)
146+
val TRUE_false_nonNull_true: String,
147+
@JsonProperty(isRequired = OptBoolean.TRUE, required = true)
148+
val TRUE_true_nullable_true: String?,
149+
@JsonProperty(isRequired = OptBoolean.TRUE, required = true)
150+
val TRUE_true_nonNull_true: String,
151+
// endregion
152+
// region: If isRequired is the default, only overrides by required = true will work.
153+
@JsonProperty(isRequired = OptBoolean.DEFAULT, required = false)
154+
val DEFAULT_false_nullable_false: String?,
155+
@JsonProperty(isRequired = OptBoolean.DEFAULT, required = false)
156+
val DEFAULT_false_nonNull_true: String,
157+
@JsonProperty(isRequired = OptBoolean.DEFAULT, required = true)
158+
val DEFAULT_true_nullable_true: String?,
159+
@JsonProperty(isRequired = OptBoolean.DEFAULT, required = true)
160+
val DEFAULT_true_nonNull_true: String,
161+
// endregion
162+
)
163+
164+
@Test
165+
fun `JsonProperty properly overrides required`() {
166+
val desc = defaultMapper.introspectDeserialization<IsRequiredDto>()
167+
168+
IsRequiredDto::class.memberProperties.forEach { prop ->
169+
val name = prop.name
170+
val expected = name.split("_").last().toBoolean()
171+
172+
assertEquals(expected, desc.isRequired(name), name)
173+
}
31174
}
32175
}

0 commit comments

Comments
 (0)