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

Refactor KVI #512

Merged
merged 7 commits into from
Jan 3, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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
@@ -0,0 +1,12 @@
package com.fasterxml.jackson.module.kotlin

import kotlin.reflect.KFunction
import kotlin.reflect.jvm.isAccessible

internal class ConstructorValueCreator<T>(override val callable: KFunction<T>) : ValueCreator<T>() {
override val accessible: Boolean = callable.isAccessible

init {
callable.isAccessible = true
}
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this override the above since this init block is lower? Is that the correct behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that it is difficult to know which comes first, initializing the property or rewriting isAccessible?
If I believe Intellij IDEA's point, it should work correctly, but if I value readability, should I rewrite it as follows?(Rewriting the accessible property was surprisingly expensive, so I've included a change to rewrite it only when necessary.)

override val accessible: Boolean = run {
    val initialAccessible = callable.isAccessible

    if (!initialAccessible) callable.isAccessible = true

    initialAccessible
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I was just a bit unsure whether that rewrite was intended 👍

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,15 @@ package com.fasterxml.jackson.module.kotlin
import com.fasterxml.jackson.databind.BeanDescription
import com.fasterxml.jackson.databind.DeserializationConfig
import com.fasterxml.jackson.databind.DeserializationContext
import com.fasterxml.jackson.databind.MapperFeature
import com.fasterxml.jackson.databind.deser.SettableBeanProperty
import com.fasterxml.jackson.databind.deser.ValueInstantiator
import com.fasterxml.jackson.databind.deser.ValueInstantiators
import com.fasterxml.jackson.databind.deser.impl.NullsAsEmptyProvider
import com.fasterxml.jackson.databind.deser.impl.PropertyValueBuffer
import com.fasterxml.jackson.databind.deser.std.StdValueInstantiator
import com.fasterxml.jackson.databind.introspect.AnnotatedConstructor
import com.fasterxml.jackson.databind.introspect.AnnotatedMethod
import java.lang.reflect.Constructor
import java.lang.reflect.Method
import java.lang.reflect.TypeVariable
import kotlin.reflect.KParameter
import kotlin.reflect.KType
import kotlin.reflect.full.extensionReceiverParameter
import kotlin.reflect.full.instanceParameter
import kotlin.reflect.full.valueParameters
import kotlin.reflect.jvm.isAccessible
import kotlin.reflect.jvm.javaType

internal class KotlinValueInstantiator(
Expand All @@ -31,62 +22,34 @@ internal class KotlinValueInstantiator(
private val nullIsSameAsDefault: Boolean,
private val strictNullChecks: Boolean
) : StdValueInstantiator(src) {
@Suppress("UNCHECKED_CAST")
override fun createFromObjectWith(
ctxt: DeserializationContext,
props: Array<out SettableBeanProperty>,
buffer: PropertyValueBuffer
): Any? {
val callable = when (_withArgsCreator) {
is AnnotatedConstructor -> cache.kotlinFromJava(_withArgsCreator.annotated as Constructor<Any>)
is AnnotatedMethod -> cache.kotlinFromJava(_withArgsCreator.annotated as Method)
else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")
} ?: return super.createFromObjectWith(
ctxt,
props,
buffer
) // we cannot reflect this method so do the default Java-ish behavior

if (callable.extensionReceiverParameter != null) {
// we shouldn't have an instance or receiver parameter and if we do, just go with default Java-ish behavior
return super.createFromObjectWith(ctxt, props, buffer)
}

val propCount = props.size + if (callable.instanceParameter != null) 1 else 0

var numCallableParameters = 0
val callableParameters = arrayOfNulls<KParameter>(propCount)
val jsonParamValueList = arrayOfNulls<Any>(propCount)

if (callable.instanceParameter != null) {
val possibleCompanion = callable.instanceParameter!!.type.erasedType().kotlin

if (!possibleCompanion.isCompanion) {
// abort, we have some unknown case here
return super.createFromObjectWith(ctxt, props, buffer)
}

// TODO: cache this lookup since the exception throwing/catching can be expensive
jsonParamValueList[numCallableParameters] = try {
possibleCompanion.objectInstance
} catch (ex: IllegalAccessException) {
// fallback for when an odd access exception happens through Kotlin reflection
val companionField = possibleCompanion.java.enclosingClass.fields.firstOrNull { it.type.kotlin.isCompanion }
?: throw ex
val accessible = companionField.isAccessible
if ((!accessible && ctxt.config.isEnabled(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS)) ||
(accessible && ctxt.config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS))
) {
companionField.isAccessible = true
}
companionField.get(null) ?: throw ex
}

callableParameters[numCallableParameters] = callable.instanceParameter
numCallableParameters++
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)
}

callable.valueParameters.forEachIndexed { idx, paramDef ->
valueCreator.valueParameters.forEachIndexed { idx, paramDef ->
val jsonProp = props[idx]
val isMissing = !buffer.hasParameter(jsonProp)

Expand Down Expand Up @@ -157,23 +120,19 @@ internal class KotlinValueInstantiator(
numCallableParameters++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing code, but I think this would be a bit clearer as numCallableParameters + 1, since we don't need to increment the value in place. That'll also allow numCallableParameters to be declared as a val

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

}

return if (numCallableParameters == jsonParamValueList.size && callable.instanceParameter == null) {
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 {
val accessible = callable.isAccessible
if ((!accessible && ctxt.config.isEnabled(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS)) ||
(accessible && ctxt.config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS))
) {
callable.isAccessible = true
}
valueCreator.checkAccessibility(ctxt)

val callableParametersByName = linkedMapOf<KParameter, Any?>()
callableParameters.mapIndexed { idx, paramDef ->
if (paramDef != null) {
callableParametersByName[paramDef] = jsonParamValueList[idx]
}
}
callable.callBy(callableParametersByName)
valueCreator.callBy(callableParametersByName)
}

}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.fasterxml.jackson.module.kotlin

import kotlin.reflect.KFunction
import kotlin.reflect.KParameter
import kotlin.reflect.full.extensionReceiverParameter
import kotlin.reflect.full.instanceParameter
import kotlin.reflect.jvm.isAccessible

internal class MethodValueCreator<T> private constructor(
override val callable: KFunction<T>,
override val accessible: Boolean,
val companionObjectInstance: Any
) : ValueCreator<T>() {
val instanceParameter: KParameter = callable.instanceParameter!!

companion object {
fun <T> of(callable: KFunction<T>): MethodValueCreator<T>? {
// we shouldn't have an instance or receiver parameter and if we do, just go with default Java-ish behavior
if (callable.extensionReceiverParameter != null) return null

val possibleCompanion = callable.instanceParameter!!.type.erasedType().kotlin

// abort, we have some unknown case here
if (!possibleCompanion.isCompanion) return null

val (companionObjectInstance: Any, accessible: Boolean) = try {
// throws ex
val instance = possibleCompanion.objectInstance!!
// If an instance of the companion object can be obtained, accessibility depends on the KFunction
instance to callable.isAccessible
} catch (ex: IllegalAccessException) {
// fallback for when an odd access exception happens through Kotlin reflection
possibleCompanion.java.enclosingClass.fields
.firstOrNull { it.type.kotlin.isCompanion }
?.let {
it.isAccessible = true

// If the instance of the companion object cannot be obtained, accessibility will always be false
it.get(null) to false
} ?: throw ex
}

return MethodValueCreator(callable, accessible, companionObjectInstance)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.fasterxml.jackson.module.kotlin
import com.fasterxml.jackson.databind.introspect.AnnotatedConstructor
import com.fasterxml.jackson.databind.introspect.AnnotatedMember
import com.fasterxml.jackson.databind.introspect.AnnotatedMethod
import com.fasterxml.jackson.databind.introspect.AnnotatedWithParams
import com.fasterxml.jackson.databind.util.LRUMap
import java.lang.reflect.Constructor
import java.lang.reflect.Method
Expand Down Expand Up @@ -35,6 +36,8 @@ internal class ReflectionCache(reflectionCacheSize: Int) {
private val javaClassToKotlin = LRUMap<Class<Any>, KClass<Any>>(reflectionCacheSize, reflectionCacheSize)
private val javaConstructorToKotlin = LRUMap<Constructor<Any>, KFunction<Any>>(reflectionCacheSize, reflectionCacheSize)
private val javaMethodToKotlin = LRUMap<Method, KFunction<*>>(reflectionCacheSize, reflectionCacheSize)
private val javaConstructorToValueCreator = LRUMap<Constructor<Any>, ConstructorValueCreator<*>>(reflectionCacheSize, reflectionCacheSize)
private val javaMethodToValueCreator = LRUMap<Method, MethodValueCreator<*>>(reflectionCacheSize, reflectionCacheSize)
private val javaConstructorIsCreatorAnnotated = LRUMap<AnnotatedConstructor, Boolean>(reflectionCacheSize, reflectionCacheSize)
private val javaMemberIsRequired = LRUMap<AnnotatedMember, BooleanTriState?>(reflectionCacheSize, reflectionCacheSize)
private val kotlinGeneratedMethod = LRUMap<AnnotatedMethod, Boolean>(reflectionCacheSize, reflectionCacheSize)
Expand All @@ -49,6 +52,37 @@ internal class ReflectionCache(reflectionCacheSize: Int) {
fun kotlinFromJava(key: Method): KFunction<*>? = javaMethodToKotlin.get(key)
?: key.kotlinFunction?.let { javaMethodToKotlin.putIfAbsent(key, it) ?: it }

/**
* return null if...
* - can't get kotlinFunction
* - contains extensionReceiverParameter
* - instance parameter is not companion object or can't get
*/
@Suppress("UNCHECKED_CAST")
fun valueCreatorFromJava(_withArgsCreator: AnnotatedWithParams): ValueCreator<*>? = when (_withArgsCreator) {
is AnnotatedConstructor -> {
val constructor = _withArgsCreator.annotated as Constructor<Any>

javaConstructorToValueCreator.get(constructor)
?: kotlinFromJava(constructor)?.let {
val value = ConstructorValueCreator(it)

javaConstructorToValueCreator.putIfAbsent(constructor, value) ?: value
}
k163377 marked this conversation as resolved.
Show resolved Hide resolved
}
is AnnotatedMethod -> {
val method = _withArgsCreator.annotated as Method

javaMethodToValueCreator.get(method)
?: kotlinFromJava(method)?.let {
val value = MethodValueCreator.of(it)

k163377 marked this conversation as resolved.
Show resolved Hide resolved
javaMethodToValueCreator.putIfAbsent(method, value) ?: value
}
}
else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")
} // we cannot reflect this method so do the default Java-ish behavior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the else -> throw prevent any further processing? Perhaps it should be else -> null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that the following process has been ported as is.

val callable = when (_withArgsCreator) {
is AnnotatedConstructor -> cache.kotlinFromJava(_withArgsCreator.annotated as Constructor<Any>)
is AnnotatedMethod -> cache.kotlinFromJava(_withArgsCreator.annotated as Method)
else -> throw IllegalStateException("Expected a constructor or method to create a Kotlin object, instead found ${_withArgsCreator.annotated.javaClass.name}")
} ?: return super.createFromObjectWith(
ctxt,
props,
buffer
) // we cannot reflect this method so do the default Java-ish behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. For posterity the null half of that ?: was moved to KotlinValueInstantiator:31


fun checkConstructorIsCreatorAnnotated(key: AnnotatedConstructor, calc: (AnnotatedConstructor) -> Boolean): Boolean = javaConstructorIsCreatorAnnotated.get(key)
?: calc(key).let { javaConstructorIsCreatorAnnotated.putIfAbsent(key, it) ?: it }

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

import com.fasterxml.jackson.databind.DeserializationContext
import com.fasterxml.jackson.databind.MapperFeature
import kotlin.reflect.KFunction
import kotlin.reflect.KParameter
import kotlin.reflect.full.valueParameters

/**
* A class that abstracts the creation of instances by calling KFunction.
* @see KotlinValueInstantiator
*/
internal sealed class ValueCreator<T> {
/**
* Function to be call.
*/
protected abstract val callable: KFunction<T>

/**
* Initial value for accessibility by reflection.
*/
protected abstract val accessible: Boolean

/**
* ValueParameters of the KFunction to be called.
*/
val valueParameters: List<KParameter> by lazy { callable.valueParameters }

/**
* Checking process to see if access from context is possible.
* @throws IllegalAccessException
*/
fun checkAccessibility(ctxt: DeserializationContext) {
if ((!accessible && ctxt.config.isEnabled(MapperFeature.CAN_OVERRIDE_ACCESS_MODIFIERS)) ||
(accessible && ctxt.config.isEnabled(MapperFeature.OVERRIDE_PUBLIC_ACCESS_MODIFIERS))) {
return
}

throw IllegalAccessException("Cannot access to function or companion object instance, target: $callable")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message here is a summary of what was originally individual content.
Should I follow the existing content and give a more specific message for each one?

Also, I couldn't think of any good error message content, so I would appreciate any suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by individual content

Copy link
Contributor Author

@k163377 k163377 Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember exactly what I was thinking when I left this comment because it's been so long since I created the PR...

I think I was probably worrying about the following two points.

  • Whether people who read this error message will be able to respond appropriately
  • Should I indicate specifically which of constructor, factory function, or companion object was inaccessible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, if that's possible I think it could prove to be quite helpful

}

/**
* Function call with default values enabled.
*/
fun callBy(args: Map<KParameter, Any?>): T = callable.callBy(args)
}