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

Improve strictNullChecks performance #44

Merged
merged 7 commits into from
Jan 18, 2023
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public enum class KotlinFeature(internal val enabledByDefault: Boolean) {
*
* With this disabled, the default, collections which are typed to disallow null members (e.g. `List<String>`)
* may contain null values after deserialization.
* Enabling it protects against this but has significant performance impact.
* Enabling it protects against this but has performance impact.
*/
StrictNullChecks(enabledByDefault = false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public class KotlinModule private constructor(
val cache = ReflectionCache(reflectionCacheSize)

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

if (singletonSupport) {
Expand All @@ -74,7 +74,7 @@ public class KotlinModule private constructor(
context.insertAnnotationIntrospector(
KotlinAnnotationIntrospector(context, nullToEmptyCollection, nullToEmptyMap, cache)
)
context.appendAnnotationIntrospector(KotlinNamesAnnotationIntrospector(this, cache))
context.appendAnnotationIntrospector(KotlinNamesAnnotationIntrospector(this, strictNullChecks, cache))

context.addDeserializers(KotlinDeserializers())
context.addKeyDeserializers(KotlinKeyDeserializers)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import com.fasterxml.jackson.databind.introspect.AnnotatedMember
import com.fasterxml.jackson.databind.introspect.AnnotatedMethod
import com.fasterxml.jackson.databind.introspect.AnnotatedParameter
import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector
import com.fasterxml.jackson.module.kotlin.deser.StrictNullChecksConverter
import com.fasterxml.jackson.module.kotlin.deser.ValueClassUnboxConverter
import com.fasterxml.jackson.module.kotlin.deser.value_instantiator.creator.ValueParameter
import kotlinx.metadata.Flag
import kotlinx.metadata.KmClass
import kotlinx.metadata.KmClassifier
Expand All @@ -26,6 +28,7 @@ import java.lang.reflect.Modifier

internal class KotlinNamesAnnotationIntrospector(
val module: KotlinModule,
private val strictNullChecks: Boolean,
private val cache: ReflectionCache
) : NopAnnotationIntrospector() {
// since 2.4
Expand Down Expand Up @@ -103,19 +106,51 @@ internal class KotlinNamesAnnotationIntrospector(
.takeIf { ann.annotated.isPrimarilyConstructorOf(kmClass) && !hasCreator(declaringClass, kmClass) }
}

private fun getValueParameter(a: AnnotatedParameter): ValueParameter? =
cache.valueCreatorFromJava(a.owner)?.let { it.valueParameters[a.index] }

// returns Converter when the argument on Java is an unboxed value class
override fun findDeserializationConverter(a: Annotated): Any? = (a as? AnnotatedParameter)?.let { param ->
cache.valueCreatorFromJava(param.owner)?.let { creator ->
(creator.valueParameters[param.index].type.classifier as? KmClassifier.Class)?.let { classifier ->
runCatching { classifier.name.reconstructClass() }
.getOrNull()
?.takeIf { it.isUnboxableValueClass() && it != param.rawType }
?.let { ValueClassUnboxConverter(it) }
getValueParameter(param)?.let { valueParameter ->
val rawType = a.rawType

valueParameter.createValueClassUnboxConverterOrNull(rawType) ?: run {
if (strictNullChecks) {
valueParameter.createStrictNullChecksConverterOrNull(rawType)
} else {
null
}
}
}
}
}

private fun ValueParameter.createValueClassUnboxConverterOrNull(rawType: Class<*>): ValueClassUnboxConverter<*>? {
return (this.type.classifier as? KmClassifier.Class)?.let { classifier ->
runCatching { classifier.name.reconstructClass() }
.getOrNull()
?.takeIf { it.isUnboxableValueClass() && it != rawType }
?.let { ValueClassUnboxConverter(it) }
}
}

// If the collection type argument cannot be obtained, treat it as nullable
// @see com.fasterxml.jackson.module.kotlin._ported.test.StrictNullChecksTest#testListOfGenericWithNullValue
private fun ValueParameter.isNullishTypeAt(index: Int) = arguments.getOrNull(index)?.isNullable ?: true

private fun ValueParameter.createStrictNullChecksConverterOrNull(rawType: Class<*>): StrictNullChecksConverter<*>? {
@Suppress("UNCHECKED_CAST")
return when {
Array::class.java.isAssignableFrom(rawType) && !this.isNullishTypeAt(0) ->
StrictNullChecksConverter.ForArray(rawType as Class<Array<*>>, this)
Iterable::class.java.isAssignableFrom(rawType) && !this.isNullishTypeAt(0) ->
StrictNullChecksConverter.ForIterable(rawType as Class<Iterable<*>>, this)
Map::class.java.isAssignableFrom(rawType) && !this.isNullishTypeAt(1) ->
StrictNullChecksConverter.ForMapValue(rawType as Class<Map<*, *>>, this)
else -> null
}
}

private fun Constructor<*>.isPrimarilyConstructorOf(kmClass: KmClass): Boolean = kmClass.findKmConstructor(this)
?.let { !Flag.Constructor.IS_SECONDARY(it.flags) || kmClass.constructors.size == 1 }
?: false
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package com.fasterxml.jackson.module.kotlin.deser

import com.fasterxml.jackson.databind.JavaType
import com.fasterxml.jackson.databind.type.TypeFactory
import com.fasterxml.jackson.databind.util.Converter
import com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException
import com.fasterxml.jackson.module.kotlin.deser.value_instantiator.creator.ValueParameter

internal class ValueClassUnboxConverter<T : Any>(private val valueClass: Class<T>) : Converter<T, Any?> {
private val unboxMethod = valueClass.getDeclaredMethod("unbox-impl").apply {
if (!this.isAccessible) this.isAccessible = true
}
private val outType = unboxMethod.returnType

override fun convert(value: T): Any? = unboxMethod.invoke(value)

override fun getInputType(typeFactory: TypeFactory): JavaType = typeFactory.constructType(valueClass)
override fun getOutputType(typeFactory: TypeFactory): JavaType = typeFactory.constructType(outType)
}

internal sealed class StrictNullChecksConverter<T : Any> : Converter<T, T> {
protected abstract val clazz: Class<T>
protected abstract val valueParameter: ValueParameter

protected abstract fun getValues(value: T): Iterator<*>

override fun convert(value: T): T {
getValues(value).forEach {
if (it == null) {
throw MissingKotlinParameterException(
valueParameter,
null,
"A null value was entered for the parameter ${valueParameter.name}."
)
}
}

return value
}

override fun getInputType(typeFactory: TypeFactory): JavaType = typeFactory.constructType(clazz)
override fun getOutputType(typeFactory: TypeFactory): JavaType = typeFactory.constructType(clazz)

class ForIterable(
override val clazz: Class<Iterable<*>>,
override val valueParameter: ValueParameter
) : StrictNullChecksConverter<Iterable<*>>() {
override fun getValues(value: Iterable<*>): Iterator<*> = value.iterator()
}

class ForArray(
override val clazz: Class<Array<*>>,
override val valueParameter: ValueParameter
) : StrictNullChecksConverter<Array<*>>() {
override fun getValues(value: Array<*>): Iterator<*> = value.iterator()
}

class ForMapValue(
override val clazz: Class<Map<*, *>>,
override val valueParameter: ValueParameter
) : StrictNullChecksConverter<Map<*, *>>() {
override fun getValues(value: Map<*, *>): Iterator<*> = value.values.iterator()
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import com.fasterxml.jackson.databind.deser.std.StdValueInstantiator
import com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException
import com.fasterxml.jackson.module.kotlin.ReflectionCache
import com.fasterxml.jackson.module.kotlin.deser.value_instantiator.creator.ValueCreator
import com.fasterxml.jackson.module.kotlin.deser.value_instantiator.creator.ValueParameter

private fun JsonMappingException.wrapWithPath(refFrom: Any?, refFieldName: String) =
JsonMappingException.wrapWithPath(this, refFrom, refFieldName)
Expand All @@ -24,41 +23,11 @@ internal class KotlinValueInstantiator(
private val cache: ReflectionCache,
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean,
private val strictNullChecks: Boolean
private val nullIsSameAsDefault: Boolean
) : StdValueInstantiator(src) {
// If the collection type argument cannot be obtained, treat it as nullable
// @see com.fasterxml.jackson.module.kotlin._ported.test.StrictNullChecksTest#testListOfGenericWithNullValue
private fun ValueParameter.isNullishTypeAt(index: Int) = arguments.getOrNull(index)?.isNullable ?: true

private fun JavaType.requireEmptyValue() =
(nullToEmptyCollection && this.isCollectionLikeType) || (nullToEmptyMap && this.isMapLikeType)

private fun strictNullCheck(
ctxt: DeserializationContext,
jsonProp: SettableBeanProperty,
paramDef: ValueParameter,
paramVal: Any
) {
// If an error occurs, Argument.name is always non-null
// @see com.fasterxml.jackson.module.kotlin.deser.value_instantiator.creator.Argument
when {
paramVal is Collection<*> && !paramDef.isNullishTypeAt(0) && paramVal.any { it == null } ->
"collection" to paramDef.arguments[0].name!!
paramVal is Array<*> && !paramDef.isNullishTypeAt(0) && paramVal.any { it == null } ->
"array" to paramDef.arguments[0].name!!
paramVal is Map<*, *> && !paramDef.isNullishTypeAt(1) && paramVal.values.any { it == null } ->
"map" to paramDef.arguments[1].name!!
else -> null
}?.let { (paramType, itemType) ->
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)
}
}

override fun createFromObjectWith(
ctxt: DeserializationContext,
props: Array<out SettableBeanProperty>,
Expand Down Expand Up @@ -106,8 +75,6 @@ internal class KotlinValueInstantiator(
).wrapWithPath(this.valueClass, jsonProp.name)
}
}
} else {
if (strictNullChecks) strictNullCheck(ctxt, jsonProp, paramDef, paramVal)
}

bucket[idx] = paramVal
Expand All @@ -124,8 +91,7 @@ internal class KotlinInstantiators(
private val cache: ReflectionCache,
private val nullToEmptyCollection: Boolean,
private val nullToEmptyMap: Boolean,
private val nullIsSameAsDefault: Boolean,
private val strictNullChecks: Boolean
private val nullIsSameAsDefault: Boolean
) : ValueInstantiators {
override fun findValueInstantiator(
deserConfig: DeserializationConfig,
Expand All @@ -139,8 +105,7 @@ internal class KotlinInstantiators(
cache,
nullToEmptyCollection,
nullToEmptyMap,
nullIsSameAsDefault,
strictNullChecks
nullIsSameAsDefault
)
} else {
// TODO: return defaultInstantiator and let default method parameters and nullability go unused?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ class KotlinInstantiatorsTest {
ReflectionCache(10),
nullToEmptyCollection = false,
nullToEmptyMap = false,
nullIsSameAsDefault = false,
strictNullChecks = false
nullIsSameAsDefault = false
)

@Test
Expand Down