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.

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

Performance improvement of strictNullChecks #881

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 9 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,15 @@
<exclude>com.fasterxml.jackson.module.kotlin.KotlinModule#getSingletonSupport()</exclude>
<exclude>com.fasterxml.jackson.module.kotlin.SingletonSupport</exclude>
<!-- internal -->

<exclude>
com.fasterxml.jackson.module.kotlin.KotlinNamesAnnotationIntrospector#KotlinNamesAnnotationIntrospector(com.fasterxml.jackson.module.kotlin.ReflectionCache,boolean)
</exclude>
<exclude>
com.fasterxml.jackson.module.kotlin.KotlinValueInstantiator#KotlinValueInstantiator(com.fasterxml.jackson.databind.deser.std.StdValueInstantiator,com.fasterxml.jackson.module.kotlin.ReflectionCache,boolean,boolean,boolean,boolean)
</exclude>
<exclude>
com.fasterxml.jackson.module.kotlin.KotlinInstantiators#KotlinInstantiators(com.fasterxml.jackson.module.kotlin.ReflectionCache,boolean,boolean,boolean,boolean)
</exclude>
</excludes>
</parameter>
</configuration>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class KotlinModule private constructor(

val cache = ReflectionCache(reflectionCacheSize)

context.addValueInstantiators(KotlinInstantiators(cache, nullToEmptyCollection, nullToEmptyMap, nullIsSameAsDefault, strictNullChecks))
context.addValueInstantiators(KotlinInstantiators(cache, nullToEmptyCollection, nullToEmptyMap, nullIsSameAsDefault))

if (singletonSupport) {
context.addBeanDeserializerModifier(KotlinBeanDeserializerModifier)
Expand All @@ -109,7 +109,9 @@ class KotlinModule private constructor(
nullIsSameAsDefault,
useJavaDurationConversion
))
context.appendAnnotationIntrospector(KotlinNamesAnnotationIntrospector(cache, kotlinPropertyNameAsImplicitName))
context.appendAnnotationIntrospector(
KotlinNamesAnnotationIntrospector(cache, strictNullChecks, kotlinPropertyNameAsImplicitName)
)

context.addDeserializers(KotlinDeserializers(cache, useJavaDurationConversion))
context.addKeyDeserializers(KotlinKeyDeserializers)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.fasterxml.jackson.module.kotlin

import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.annotation.JsonSetter
import com.fasterxml.jackson.annotation.Nulls
import com.fasterxml.jackson.databind.JavaType
import com.fasterxml.jackson.databind.cfg.MapperConfig
import com.fasterxml.jackson.databind.introspect.Annotated
Expand All @@ -12,8 +14,10 @@ import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector
import com.fasterxml.jackson.databind.introspect.PotentialCreator
import java.lang.reflect.Constructor
import java.util.Locale
import kotlin.collections.getOrNull
import kotlin.reflect.KClass
import kotlin.reflect.KFunction
import kotlin.reflect.KParameter
import kotlin.reflect.full.hasAnnotation
import kotlin.reflect.full.memberProperties
import kotlin.reflect.full.primaryConstructor
Expand All @@ -22,6 +26,7 @@ import kotlin.reflect.jvm.javaType

internal class KotlinNamesAnnotationIntrospector(
private val cache: ReflectionCache,
private val strictNullChecks: Boolean,
private val kotlinPropertyNameAsImplicitName: Boolean
) : NopAnnotationIntrospector() {
private fun getterNameFromJava(member: AnnotatedMethod): String? {
Expand Down Expand Up @@ -73,16 +78,26 @@ internal class KotlinNamesAnnotationIntrospector(
}

override fun refineDeserializationType(config: MapperConfig<*>, a: Annotated, baseType: JavaType): JavaType =
(a as? AnnotatedParameter)?.let { _ ->
cache.findKotlinParameter(a)?.let { param ->
val rawType = a.rawType
(param.type.classifier as? KClass<*>)
?.java
?.takeIf { it.isUnboxableValueClass() && it != rawType }
?.let { config.constructType(it) }
}
findKotlinParameter(a)?.let { param ->
val rawType = a.rawType
(param.type.classifier as? KClass<*>)
?.java
?.takeIf { it.isUnboxableValueClass() && it != rawType }
?.let { config.constructType(it) }
} ?: baseType

override fun findSetterInfo(ann: Annotated): JsonSetter.Value = ann.takeIf { strictNullChecks }
?.let { _ ->
findKotlinParameter(ann)?.let { param ->
if (param.requireStrictNullCheck(ann.type)) {
JsonSetter.Value.forContentNulls(Nulls.FAIL)
} else {
null
}
}
}
?: super.findSetterInfo(ann)

override fun findDefaultCreator(
config: MapperConfig<*>,
valueClass: AnnotatedClass,
Expand All @@ -106,8 +121,18 @@ internal class KotlinNamesAnnotationIntrospector(
}

private fun findKotlinParameterName(param: AnnotatedParameter): String? = cache.findKotlinParameter(param)?.name

private fun findKotlinParameter(param: Annotated) = (param as? AnnotatedParameter)
?.let { cache.findKotlinParameter(it) }
}

private fun KParameter.markedNonNullAt(index: Int) = type.arguments.getOrNull(index)?.type?.isMarkedNullable == false

private fun KParameter.requireStrictNullCheck(type: JavaType): Boolean =
((type.isArrayType || type.isCollectionLikeType) && this.markedNonNullAt(0)) ||
(type.isMapLikeType && this.markedNonNullAt(1))


// If it is not a Kotlin class or an Enum, Creator is not used
private fun AnnotatedClass.creatableKotlinClass(): KClass<*>? = annotated
.takeIf { it.isKotlinClass() && !it.isEnum }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ import com.fasterxml.jackson.databind.deser.ValueInstantiators
import com.fasterxml.jackson.databind.deser.impl.PropertyValueBuffer
import com.fasterxml.jackson.databind.deser.std.StdValueInstantiator
import java.lang.reflect.TypeVariable
import kotlin.reflect.KParameter
import kotlin.reflect.KType
import kotlin.reflect.KTypeProjection
import kotlin.reflect.jvm.javaType

internal class KotlinValueInstantiator(
Expand All @@ -23,15 +21,12 @@ internal class KotlinValueInstantiator(
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean,
private val strictNullChecks: Boolean
) : StdValueInstantiator(src) {
private fun JavaType.requireEmptyValue() =
(nullToEmptyCollection && this.isCollectionLikeType) || (nullToEmptyMap && this.isMapLikeType)

private fun KType.isGenericTypeVar() = javaType is TypeVariable<*>

private fun List<KTypeProjection>.markedNonNullAt(index: Int) = getOrNull(index)?.type?.isMarkedNullable == false

// If the argument is a value class that wraps nullable and non-null,
// and the input is explicit null, the value class is instantiated with null as input.
private fun requireValueClassSpecialNullValue(
Expand Down Expand Up @@ -101,34 +96,6 @@ internal class KotlinValueInstantiator(
).wrapWithPath(this.valueClass, jsonProp.name)
}
}
} else if (strictNullChecks) {
val arguments = paramType.arguments

var paramTypeStr: String? = null
var itemType: KType? = null

if (propType.isCollectionLikeType && arguments.markedNonNullAt(0) && (paramVal as Collection<*>).any { it == null }) {
paramTypeStr = "collection"
itemType = arguments[0].type
}

if (propType.isMapLikeType && arguments.markedNonNullAt(1) && (paramVal as Map<*, *>).any { it.value == null }) {
paramTypeStr = "map"
itemType = arguments[1].type
}

if (propType.isArrayType && arguments.markedNonNullAt(0) && (paramVal as Array<*>).any { it == null }) {
paramTypeStr = "array"
itemType = arguments[0].type
}

if (paramTypeStr != null && itemType != null) {
throw MissingKotlinParameterException(
parameter = paramDef,
processor = ctxt.parser,
msg = "Instantiation of $itemType $paramType failed for JSON property ${jsonProp.name} due to null value in a $paramType that does not allow null values"
).wrapWithPath(this.valueClass, jsonProp.name)
}
}

bucket[paramDef] = paramVal
Expand All @@ -147,7 +114,6 @@ internal class KotlinInstantiators(
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean,
private val strictNullChecks: Boolean
) : ValueInstantiators {
override fun findValueInstantiator(
deserConfig: DeserializationConfig,
Expand All @@ -162,7 +128,6 @@ internal class KotlinInstantiators(
nullToEmptyCollection,
nullToEmptyMap,
nullIsSameAsDefault,
strictNullChecks
)
} else {
// TODO: return defaultInstantiator and let default method parameters and nullability go unused? or die with exception:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ class KotlinInstantiatorsTest {
nullToEmptyCollection = false,
nullToEmptyMap = false,
nullIsSameAsDefault = false,
strictNullChecks = false
)

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package com.fasterxml.jackson.module.kotlin.kogeraIntegration.deser

import com.fasterxml.jackson.annotation.JsonSetter
import com.fasterxml.jackson.annotation.Nulls
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.exc.InvalidNullException
import com.fasterxml.jackson.module.kotlin.KotlinFeature
import com.fasterxml.jackson.module.kotlin.KotlinModule
import com.fasterxml.jackson.module.kotlin.readValue
import org.junit.jupiter.api.Assertions
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows

class StrictNullChecksTest {
val mapper: ObjectMapper = ObjectMapper()
.registerModule(
KotlinModule.Builder()
.enable(KotlinFeature.StrictNullChecks)
.build()
)

class ArrayWrapper(val value: Array<Int>)
data class ListWrapper(val value: List<Int>)
data class MapWrapper(val value: Map<String, Int>)

@Nested
inner class NonNullInput {
@Test
fun array() {
val expected = ArrayWrapper(arrayOf(1))
val src = mapper.writeValueAsString(expected)
val result = mapper.readValue<ArrayWrapper>(src)

Assertions.assertArrayEquals(expected.value, result.value)
}

@Test
fun list() {
val expected = ListWrapper(listOf(1))
val src = mapper.writeValueAsString(expected)
val result = mapper.readValue<ListWrapper>(src)

Assertions.assertEquals(expected, result)
}

@Test
fun map() {
val expected = MapWrapper(mapOf("foo" to 1))
val src = mapper.writeValueAsString(expected)
val result = mapper.readValue<MapWrapper>(src)

Assertions.assertEquals(expected, result)
}
}

data class AnyWrapper(val value: Any)

@Nested
inner class NullInput {
@Test
fun array() {
val src = mapper.writeValueAsString(AnyWrapper(arrayOf<Int?>(null)))
assertThrows<InvalidNullException> { mapper.readValue<ArrayWrapper>(src) }
}

@Test
fun list() {
val src = mapper.writeValueAsString(AnyWrapper(arrayOf<Int?>(null)))
assertThrows<InvalidNullException> { mapper.readValue<ListWrapper>(src) }
}

@Test
fun map() {
val src = mapper.writeValueAsString(AnyWrapper(mapOf("foo" to null)))
assertThrows<InvalidNullException> { mapper.readValue<MapWrapper>(src) }
}
}

class ContentNullsSkipArrayWrapper(@JsonSetter(contentNulls = Nulls.SKIP) val value: Array<Int>)
data class ContentNullsSkipListWrapper(@JsonSetter(contentNulls = Nulls.SKIP) val value: List<Int>)
data class ContentNullsSkipMapWrapper(@JsonSetter(contentNulls = Nulls.SKIP) val value: Map<String, Int>)

@Nested
inner class CustomByAnnotationTest {
@Test
fun array() {
val expected = ContentNullsSkipArrayWrapper(emptyArray())
val src = mapper.writeValueAsString(AnyWrapper(arrayOf<Int?>(null)))
val result = mapper.readValue<ContentNullsSkipArrayWrapper>(src)

Assertions.assertArrayEquals(expected.value, result.value)
}

@Test
fun list() {
val expected = ContentNullsSkipListWrapper(emptyList())
val src = mapper.writeValueAsString(AnyWrapper(listOf<Int?>(null)))
val result = mapper.readValue<ContentNullsSkipListWrapper>(src)

Assertions.assertEquals(expected, result)
}

@Test
fun map() {
val expected = ContentNullsSkipMapWrapper(emptyMap())
val src = mapper.writeValueAsString(AnyWrapper(mapOf("foo" to null)))
val result = mapper.readValue<ContentNullsSkipMapWrapper>(src)

Assertions.assertEquals(expected, result)
}
}

class AnnotatedArrayWrapper(@JsonSetter(nulls = Nulls.SKIP) val value: Array<Int> = emptyArray())
data class AnnotatedListWrapper(@JsonSetter(nulls = Nulls.SKIP) val value: List<Int> = emptyList())
data class AnnotatedMapWrapper(@JsonSetter(nulls = Nulls.SKIP) val value: Map<String, Int> = emptyMap())

// If Default is specified by annotation, it is not overridden.
@Nested
inner class AnnotatedNullInput {
@Test
fun array() {
val src = mapper.writeValueAsString(AnyWrapper(arrayOf<Int?>(null)))
assertThrows<InvalidNullException> { mapper.readValue<AnnotatedArrayWrapper>(src) }
}

@Test
fun list() {
val src = mapper.writeValueAsString(AnyWrapper(arrayOf<Int?>(null)))
assertThrows<InvalidNullException> { mapper.readValue<AnnotatedListWrapper>(src) }
}

@Test
fun map() {
val src = mapper.writeValueAsString(AnyWrapper(mapOf("foo" to null)))
assertThrows<InvalidNullException> { mapper.readValue<AnnotatedMapWrapper>(src) }
}
}
}
Loading