Skip to content

Commit 88ac686

Browse files
k163377k163377
committed
Merge remote-tracking branch 'FasterXML/2.19'
2 parents 5543079 + 819f4b2 commit 88ac686

File tree

4 files changed

+197
-51
lines changed

4 files changed

+197
-51
lines changed

‎release-notes/CREDITS-2.x‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 12 additions & 0 deletions
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/tools/jackson/module/kotlin/KotlinAnnotationIntrospector.kt‎

Lines changed: 34 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package tools.jackson.module.kotlin
22

33
import com.fasterxml.jackson.annotation.JsonProperty
4+
import com.fasterxml.jackson.annotation.OptBoolean
45
import tools.jackson.databind.DeserializationFeature
56
import tools.jackson.databind.JacksonModule
67
import tools.jackson.databind.cfg.MapperConfig
@@ -42,21 +43,27 @@ internal class KotlinAnnotationIntrospector(
4243
// TODO: implement nullIsSameAsDefault flag, which represents when TRUE that if something has a default value, it can be passed a null to default it
4344
// this likely impacts this class to be accurate about what COULD be considered required
4445

46+
// If a new isRequired is explicitly specified or the old required is true, those values take precedence.
47+
// In other cases, override is done by KotlinModule.
48+
private fun JsonProperty.forceRequiredByAnnotation(): Boolean? = when {
49+
isRequired != OptBoolean.DEFAULT -> isRequired.asBoolean()
50+
required -> true
51+
else -> null
52+
}
53+
54+
private fun AccessibleObject.forceRequiredByAnnotation(): Boolean? =
55+
getAnnotation(JsonProperty::class.java)?.forceRequiredByAnnotation()
56+
4557
override fun hasRequiredMarker(
4658
cfg : MapperConfig<*>,
4759
m: AnnotatedMember
4860
): Boolean? = m.takeIf { it.member.declaringClass.isKotlinClass() }?.let { _ ->
4961
cache.javaMemberIsRequired(m) {
5062
try {
51-
when {
52-
nullToEmptyCollection && m.type.isCollectionLikeType -> false
53-
nullToEmptyMap && m.type.isMapLikeType -> false
54-
m.member.declaringClass.isKotlinClass() -> when (m) {
55-
is AnnotatedField -> m.hasRequiredMarker()
56-
is AnnotatedMethod -> m.hasRequiredMarker()
57-
is AnnotatedParameter -> m.hasRequiredMarker()
58-
else -> null
59-
}
63+
when (m) {
64+
is AnnotatedField -> m.hasRequiredMarker()
65+
is AnnotatedMethod -> m.hasRequiredMarker()
66+
is AnnotatedParameter -> m.hasRequiredMarker()
6067
else -> null
6168
}
6269
} catch (_: UnsupportedOperationException) {
@@ -108,28 +115,9 @@ internal class KotlinAnnotationIntrospector(
108115
}
109116

110117
private fun AnnotatedField.hasRequiredMarker(): Boolean? {
111-
val byAnnotation = (member as Field).isRequiredByAnnotation()
112-
val byNullability = (member as Field).kotlinProperty?.returnType?.isRequired()
113-
114-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
115-
}
116-
117-
private fun AccessibleObject.isRequiredByAnnotation(): Boolean? = annotations
118-
.firstOrNull { it.annotationClass == JsonProperty::class }
119-
?.let { it as JsonProperty }
120-
?.required
121-
122-
private fun requiredAnnotationOrNullability(byAnnotation: Boolean?, byNullability: Boolean?): Boolean? {
123-
if (byAnnotation != null && byNullability != null) {
124-
return byAnnotation || byNullability
125-
} else if (byNullability != null) {
126-
return byNullability
127-
}
128-
return byAnnotation
129-
}
130-
131-
private fun Method.isRequiredByAnnotation(): Boolean? {
132-
return (this.annotations.firstOrNull { it.annotationClass.java == JsonProperty::class.java } as? JsonProperty)?.required
118+
val field = member as Field
119+
return field.forceRequiredByAnnotation()
120+
?: field.kotlinProperty?.returnType?.isRequired()
133121
}
134122

135123
// Since Kotlin's property has the same Type for each field, getter, and setter,
@@ -144,33 +132,35 @@ internal class KotlinAnnotationIntrospector(
144132
private fun AnnotatedMethod.getRequiredMarkerFromCorrespondingAccessor(): Boolean? {
145133
member.declaringClass.kotlin.declaredMemberProperties.forEach { kProperty ->
146134
if (kProperty.javaGetter == this.member || (kProperty as? KMutableProperty1)?.javaSetter == this.member) {
147-
val byAnnotation = this.member.isRequiredByAnnotation()
148-
val byNullability = kProperty.isRequiredByNullability()
149-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
135+
return member.forceRequiredByAnnotation() ?: kProperty.isRequiredByNullability()
150136
}
151137
}
152138
return null
153139
}
154140

155141
// Is the member method a regular method of the data class or
156142
private fun Method.getRequiredMarkerFromAccessorLikeMethod(): Boolean? = cache.kotlinFromJava(this)?.let { func ->
157-
val byAnnotation = this.isRequiredByAnnotation()
158-
return when {
159-
func.isGetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.returnType.isRequired())
160-
func.isSetterLike() -> requiredAnnotationOrNullability(byAnnotation, func.valueParameters[0].isRequired())
143+
forceRequiredByAnnotation() ?: when {
144+
func.isGetterLike() -> func.returnType.isRequired()
145+
// If nullToEmpty could be supported for setters,
146+
// a branch similar to AnnotatedParameter.hasRequiredMarker should be added.
147+
func.isSetterLike() -> func.valueParameters[0].isRequired()
161148
else -> null
162149
}
163150
}
164151

165152
private fun KFunction<*>.isGetterLike(): Boolean = parameters.size == 1
166153
private fun KFunction<*>.isSetterLike(): Boolean = parameters.size == 2 && returnType == UNIT_TYPE
167154

168-
private fun AnnotatedParameter.hasRequiredMarker(): Boolean? {
169-
val byAnnotation = this.getAnnotation(JsonProperty::class.java)?.required
170-
val byNullability = cache.findKotlinParameter(this)?.isRequired()
171-
172-
return requiredAnnotationOrNullability(byAnnotation, byNullability)
173-
}
155+
private fun AnnotatedParameter.hasRequiredMarker(): Boolean? = getAnnotation(JsonProperty::class.java)
156+
?.forceRequiredByAnnotation()
157+
?: run {
158+
when {
159+
nullToEmptyCollection && type.isCollectionLikeType -> false
160+
nullToEmptyMap && type.isMapLikeType -> false
161+
else -> cache.findKotlinParameter(this)?.isRequired()
162+
}
163+
}
174164

175165
private fun AnnotatedMethod.findValueClassReturnType() = cache.findValueClassReturnType(this)
176166

‎src/test/kotlin/tools/jackson/module/kotlin/test/github/GitHub922.kt‎

Lines changed: 150 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,25 @@
11
package tools.jackson.module.kotlin.test.github
22

3+
import com.fasterxml.jackson.annotation.JsonProperty
4+
import com.fasterxml.jackson.annotation.OptBoolean
35
import tools.jackson.databind.BeanDescription
46
import tools.jackson.databind.ObjectMapper
57
import tools.jackson.module.kotlin.KotlinFeature
8+
import tools.jackson.module.kotlin.defaultMapper
69
import tools.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
_serializationContext().introspectBeanDescription(_serializationContext().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)