Changes in constructor invocation and argument management

.github/workflows/main.yml

@@ -27,7 +27,8 @@ jobs:
       fail-fast: false
       matrix:
         java_version: ['8', '11', '17', '21']
-        kotlin_version: ['1.7.22', '1.8.22', '1.9.21']
+        # kotlin-reflect 1.8.2x has a bug and some tests fail, so we are downgrading to 1.8.10.
+        kotlin_version: ['1.7.22', '1.8.10', '1.9.21']
         os: ['ubuntu-20.04']
     env:
       JAVA_OPTS: "-XX:+TieredCompilation -XX:TieredStopAtLevel=1"

pom.xml

@@ -237,6 +237,9 @@
                           <exclude>com.fasterxml.jackson.module.kotlin.KotlinModule#serialVersionUID</exclude>
                 <!-- internal -->
                           <exclude>com.fasterxml.jackson.module.kotlin.KotlinNamesAnnotationIntrospector</exclude>
+                          <exclude>com.fasterxml.jackson.module.kotlin.ValueCreator</exclude>
+                          <exclude>com.fasterxml.jackson.module.kotlin.ConstructorValueCreator</exclude>
+                          <exclude>com.fasterxml.jackson.module.kotlin.MethodValueCreator</exclude>
                         </excludes>
                     </parameter>
                 </configuration>

release-notes/CREDITS-2.x

@@ -18,6 +18,7 @@ Contributors:
 # 2.17.0 (not yet released)
 
 WrongWrong (@k163377)
+* #755: Changes in constructor invocation and argument management.
 * #752: Fix KDoc for KotlinModule.
 * #751: Marked useKotlinPropertyNameForGetter as deprecated.
 * #747: Improved performance related to KotlinModule initialization and setupModule.

release-notes/VERSION-2.x

@@ -18,6 +18,8 @@ Co-maintainers:
 
 2.17.0 (not yet released)
 
+#755: Changes in constructor invocation and argument management.
+ This change degrades performance in cases where the constructor is called without default arguments, but improves performance in other cases.
 #751: The KotlinModule#useKotlinPropertyNameForGetter property was deprecated because it differed from the name of the KotlinFeature.
  Please use KotlinModule#kotlinPropertyNameAsImplicitName from now on.
 #747: Improved performance related to KotlinModule initialization and setupModule.

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ArgumentBucket.kt

@@ -0,0 +1,68 @@
+package com.fasterxml.jackson.module.kotlin
+
+import kotlin.reflect.KParameter
+
+internal class BucketGenerator private constructor(paramSize: Int, instanceParameter: KParameter?, instance: Any?) {
+    private val originalParameters = arrayOfNulls<KParameter>(paramSize)
+    private val originalArguments = arrayOfNulls<Any?>(paramSize)
+    private val initialCount: Int
+
+    init {
+        if (instance != null) {
+            originalParameters[0] = instanceParameter
+            originalArguments[0] = instance
+            initialCount = 1
+        } else {
+            initialCount = 0
+        }
+    }
+
+    fun generate(): ArgumentBucket = ArgumentBucket(
+        parameters = originalParameters.clone(),
+        arguments = originalArguments.clone(),
+        count = initialCount
+    )
+
+    companion object {
+        fun forConstructor(paramSize: Int): BucketGenerator = BucketGenerator(paramSize, null, null)
+
+        fun forMethod(paramSize: Int, instanceParameter: KParameter, instance: Any): BucketGenerator =
+            BucketGenerator(paramSize, instanceParameter, instance)
+    }
+}
+
+internal class ArgumentBucket(
+    private val parameters: Array<KParameter?>,
+    val arguments: Array<Any?>,
+    private var count: Int
+) : Map<KParameter, Any?> {
+    operator fun set(key: KParameter, value: Any?) {
+        arguments[key.index] = value
+        parameters[key.index] = key
+
+        // Multiple calls are not checked because internally no calls are made more than once per argument.
+        count++
+    }
+
+    val isFullInitialized: Boolean get() = count == arguments.size
+
+    private class Entry(override val key: KParameter, override val value: Any?) : Map.Entry<KParameter, Any?>
+
+    override val entries: Set<Map.Entry<KParameter, Any?>>
+        get() = parameters.mapNotNull { key -> key?.let { Entry(it, arguments[it.index]) } }.toSet()
+    override val keys: Set<KParameter>
+        get() = parameters.filterNotNull().toSet()
+    override val size: Int
+        get() = count
+    override val values: Collection<Any?>
+        get() = keys.map { arguments[it.index] }
+
+    override fun isEmpty(): Boolean = this.size == 0
+
+    // Skip the check here, as it is only called after the check for containsKey.
+    override fun get(key: KParameter): Any? = arguments[key.index]
+
+    override fun containsValue(value: Any?): Boolean = keys.any { arguments[it.index] == value }
+
+    override fun containsKey(key: KParameter): Boolean = parameters.any { it?.index == key.index }
+}

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ConstructorValueCreator.kt

@@ -5,6 +5,7 @@ import kotlin.reflect.jvm.isAccessible
 
 internal class ConstructorValueCreator<T>(override val callable: KFunction<T>) : ValueCreator<T>() {
     override val accessible: Boolean = callable.isAccessible
+    override val bucketGenerator: BucketGenerator = BucketGenerator.forConstructor(callable.parameters.size)
 
     init {
         // To prevent the call from failing, save the initial value and then rewrite the flag.

src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinValueInstantiator.kt

@@ -42,24 +42,7 @@ internal class KotlinValueInstantiator(
         val valueCreator: ValueCreator<*> = cache.valueCreatorFromJava(_withArgsCreator)
             ?: return super.createFromObjectWith(ctxt, props, buffer)
 
-        val propCount: Int
-        var numCallableParameters: Int
-        val callableParameters: Array<KParameter?>
-        val jsonParamValueList: Array<Any?>
-
-        if (valueCreator is MethodValueCreator) {
-            propCount = props.size + 1
-            numCallableParameters = 1
-            callableParameters = arrayOfNulls<KParameter>(propCount)
-                .apply { this[0] = valueCreator.instanceParameter }
-            jsonParamValueList = arrayOfNulls<Any>(propCount)
-                .apply { this[0] = valueCreator.companionObjectInstance }
-        } else {
-            propCount = props.size
-            numCallableParameters = 0
-            callableParameters = arrayOfNulls(propCount)
-            jsonParamValueList = arrayOfNulls(propCount)
-        }
+        val bucket = valueCreator.generateBucket()
 
         valueCreator.valueParameters.forEachIndexed { idx, paramDef ->
             val jsonProp = props[idx]
@@ -131,26 +114,12 @@ internal class KotlinValueInstantiator(
                 }
             }
 
-            jsonParamValueList[numCallableParameters] = paramVal
-            callableParameters[numCallableParameters] = paramDef
-            numCallableParameters++
+            bucket[paramDef] = paramVal
         }
 
-        return if (numCallableParameters == jsonParamValueList.size && valueCreator is ConstructorValueCreator) {
-            // we didn't do anything special with default parameters, do a normal call
-            super.createFromObjectWith(ctxt, jsonParamValueList)
-        } else {
-            valueCreator.checkAccessibility(ctxt)
-
-            val callableParametersByName = linkedMapOf<KParameter, Any?>()
-            callableParameters.mapIndexed { idx, paramDef ->
-                if (paramDef != null) {
-                    callableParametersByName[paramDef] = jsonParamValueList[idx]
-                }
-            }
-            valueCreator.callBy(callableParametersByName)
-        }
+        valueCreator.checkAccessibility(ctxt)
 
+        return valueCreator.callBy(bucket)
     }
 
     private fun SettableBeanProperty.hasInjectableValueId(): Boolean = injectableValueId != null

src/main/kotlin/com/fasterxml/jackson/module/kotlin/MethodValueCreator.kt

@@ -9,9 +9,11 @@ import kotlin.reflect.jvm.isAccessible
 internal class MethodValueCreator<T> private constructor(
     override val callable: KFunction<T>,
     override val accessible: Boolean,
-    val companionObjectInstance: Any
+    companionObjectInstance: Any
 ) : ValueCreator<T>() {
-    val instanceParameter: KParameter = callable.instanceParameter!!
+    override val bucketGenerator: BucketGenerator = callable.parameters.let {
+        BucketGenerator.forMethod(it.size, it[0], companionObjectInstance)
+    }
 
     companion object {
         fun <T> of(callable: KFunction<T>): MethodValueCreator<T>? {

src/main/kotlin/com/fasterxml/jackson/module/kotlin/ValueCreator.kt

@@ -21,6 +21,10 @@ internal sealed class ValueCreator<T> {
      */
     protected abstract val accessible: Boolean
 
+    protected abstract val bucketGenerator: BucketGenerator
+
+    fun generateBucket(): ArgumentBucket = bucketGenerator.generate()
+
     /**
      * ValueParameters of the KFunction to be called.
      */
@@ -45,5 +49,9 @@ internal sealed class ValueCreator<T> {
     /**
      * Function call with default values enabled.
      */
-    fun callBy(args: Map<KParameter, Any?>): T = callable.callBy(args)
+    fun callBy(args: ArgumentBucket): T = if (args.isFullInitialized) {
+        callable.call(*args.arguments)
+    } else {
+        callable.callBy(args)
+    }
 }

src/test/kotlin/com/fasterxml/jackson/module/kotlin/ArgumentBucketTest.kt

@@ -0,0 +1,86 @@
+package com.fasterxml.jackson.module.kotlin
+
+import com.fasterxml.jackson.annotation.JsonCreator
+import org.junit.Ignore
+import org.junit.experimental.runners.Enclosed
+import org.junit.runner.RunWith
+import kotlin.reflect.KFunction
+import kotlin.reflect.full.functions
+import kotlin.reflect.full.hasAnnotation
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+
+@RunWith(Enclosed::class)
+class ArgumentBucketTest {
+    class Normal {
+        @Ignore
+        data class Constructor(val foo: String, val bar: String)
+
+        @Test
+        fun constructorTest() {
+            val function: KFunction<*> = ::Constructor
+            val params = function.parameters
+            val generator = BucketGenerator.forConstructor(params.size)
+            val bucket = generator.generate()
+
+            assertTrue(bucket.isEmpty())
+            assertEquals(0, bucket.size)
+            assertFalse(bucket.isFullInitialized)
+
+            bucket[params[0]] = "foo"
+
+            assertFalse(bucket.isEmpty())
+            assertEquals(1, bucket.size)
+            assertFalse(bucket.isFullInitialized)
+            assertEquals("foo", bucket[params[0]])
+
+            bucket[params[1]] = "bar"
+
+            assertFalse(bucket.isEmpty())
+            assertEquals(2, bucket.size)
+            assertTrue(bucket.isFullInitialized)
+            assertEquals("bar", bucket[params[1]])
+        }
+
+        @Ignore
+        data class Method(val foo: String, val bar: String) {
+            companion object {
+                @JvmStatic
+                @JsonCreator
+                fun of(foo: String, bar: String): Method = Method(foo, bar)
+            }
+        }
+
+        @Test
+        fun methodTest() {
+            val function: KFunction<*> = Method.Companion::class.functions.first { it.hasAnnotation<JsonCreator>() }
+            val params = function.parameters
+            val generator = BucketGenerator.forMethod(params.size, params[0], Method)
+            val bucket = generator.generate()
+
+            assertFalse(bucket.isEmpty())
+            assertEquals(1, bucket.size)
+            assertEquals(Method.Companion, bucket[params[0]])
+            assertFalse(bucket.isFullInitialized)
+
+            bucket[params[1]] = "foo"
+
+            assertFalse(bucket.isEmpty())
+            assertEquals(2, bucket.size)
+            assertFalse(bucket.isFullInitialized)
+            assertEquals("foo", bucket[params[1]])
+
+            bucket[params[2]] = "bar"
+
+            assertFalse(bucket.isEmpty())
+            assertEquals(3, bucket.size)
+            assertTrue(bucket.isFullInitialized)
+            assertEquals("bar", bucket[params[2]])
+        }
+    }
+
+    // After support, add a case with a value class.
+}

src/test/kotlin/com/fasterxml/jackson/module/kotlin/TestCommons.kt

@@ -5,10 +5,28 @@ import com.fasterxml.jackson.core.util.DefaultIndenter
 import com.fasterxml.jackson.core.util.DefaultPrettyPrinter
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.databind.ObjectWriter
+import kotlin.reflect.KParameter
+import kotlin.reflect.full.memberProperties
+import kotlin.reflect.full.primaryConstructor
+import kotlin.test.assertEquals
 
 // This `printer` is used to match the output from Jackson to the newline char of the source code.
 // If this is removed, comparisons will fail in a Windows-like platform.
 val LF_PRINTER: PrettyPrinter =
     DefaultPrettyPrinter().withObjectIndenter(DefaultIndenter().withLinefeed("\n"))
 
 fun ObjectMapper.testPrettyWriter(): ObjectWriter = this.writer().with(LF_PRINTER)
+internal val defaultMapper = jacksonObjectMapper()
+
+internal inline fun <reified T : Any> callPrimaryConstructor(mapper: (KParameter) -> Any? = { it.name }): T =
+    T::class.primaryConstructor!!.run {
+        val args = parameters.associateWith { mapper(it) }
+        callBy(args)
+    }
+
+// Function for comparing non-data classes.
+internal inline fun <reified T : Any> assertReflectEquals(expected: T, actual: T) {
+    T::class.memberProperties.forEach {
+        assertEquals(it.get(expected), it.get(actual))
+    }
+}