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

Add KotlinPropertyNameAsImplicitName option #686

Merged
merged 5 commits into from
Jul 28, 2023
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
Expand Up @@ -42,7 +42,23 @@ enum class KotlinFeature(private val enabledByDefault: Boolean) {
* may contain null values after deserialization.
* Enabling it protects against this but has significant performance impact.
*/
StrictNullChecks(enabledByDefault = false);
StrictNullChecks(enabledByDefault = false),

/**
* By enabling this feature, the property name on Kotlin will be used as the getter name.
*
* By default, the name based on the getter name on the JVM is used as the accessor name.
* This name may be different from the parameter/field name, in which case serialization results
* may be incorrect or annotations may malfunction.
* See [jackson-module-kotlin#630] for details.
*
* By enabling this feature, such malfunctions will not occur.
*
* On the other hand, enabling this option increases the amount of reflection processing,
* which may result in performance degradation for both serialization and deserialization.
* In addition, the adjustment of behavior using get:JvmName is disabled.
*/
UseKotlinPropertyNameForGetter(enabledByDefault = false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cowtowncoder
I would like to get a review as I am not sure about the option name and description.

Copy link
Member

Choose a reason for hiding this comment

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

One quick question: is "property" first-class concept in Kotlin? If not, maybe it'd be "field", but I assume it is.

But I guess I am not sure what "use property name for getter" actually means... I mean, getter in Jackson specifically means method use to access value of a logical property to serialize. It has name specified by class file, and does not change.
Name of logical property does change tho. So maybe naming here is bit confusing, at least to me.

Copy link
Contributor Author

@k163377 k163377 Jul 17, 2023

Choose a reason for hiding this comment

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

is "property" first-class concept in Kotlin?

I think we can call it a first-class concept in this regard.
On Kotlin, fields and accessors are defined for properties.
https://kotlinlang.org/docs/properties.html

So maybe naming here is bit confusing, at least to me.

I don't know if this is a good name, but at least this is how I would describe the internal behavior directly.

If the emphasis is on naming on Jackson, would it be UseKotrinPropertyNameForGetterImplicitName?
However, it doesn't seem like a good name since we may not use the findImplicitPropertyName function forever.
Also, the name seems too long compared to the traditional KotlinFeature.

Copy link
Member

Choose a reason for hiding this comment

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

I just did not understand what "for getter" means. But yes, if it changes "implicit [property] name" in context of serialization, I can see how "....ForGetterImplicitName" could work.

Does this not affect deserialization? Otherwise just something like "UseKotlinNameAsImplicitName" would make more sense to me.

Copy link
Contributor Author

@k163377 k163377 Jul 21, 2023

Choose a reason for hiding this comment

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

I just did not understand what "for getter" means.

I wanted to express that it does not affect parameters, fields, or setters, but only getters.
Currently we are already using names on Kotlin for parameter names, but that is not affected by this option.

Does this not affect deserialization?

If there are annotations for deserialization on the getter, it will affect the behavior, but basically it should only affect serialization.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I think referring to serialization makes more sense because getter really is physical thing (Method) used to access a property for purpose of (de)serialization. So you can't really change name of getter, but you change name of property for (de)serialization. Although, yes, obtained from getter method.

Still, ultimately this is your decision: I just offer my suggestions. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed for correction.
aa7217f


internal val bitSet: BitSet = (1 shl ordinal).toBitSet()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.fasterxml.jackson.module.kotlin.KotlinFeature.NullIsSameAsDefault
import com.fasterxml.jackson.module.kotlin.KotlinFeature.NullToEmptyCollection
import com.fasterxml.jackson.module.kotlin.KotlinFeature.NullToEmptyMap
import com.fasterxml.jackson.module.kotlin.KotlinFeature.StrictNullChecks
import com.fasterxml.jackson.module.kotlin.KotlinFeature.UseKotlinPropertyNameForGetter
import com.fasterxml.jackson.module.kotlin.SingletonSupport.CANONICALIZE
import com.fasterxml.jackson.module.kotlin.SingletonSupport.DISABLED
import java.util.*
Expand Down Expand Up @@ -53,7 +54,8 @@ class KotlinModule @Deprecated(
val nullToEmptyMap: Boolean = false,
val nullIsSameAsDefault: Boolean = false,
val singletonSupport: SingletonSupport = DISABLED,
val strictNullChecks: Boolean = false
val strictNullChecks: Boolean = false,
val useKotlinPropertyNameForGetter: Boolean = false
) : SimpleModule(KotlinModule::class.java.name, PackageVersion.VERSION) {
init {
if (!KotlinVersion.CURRENT.isAtLeast(1, 5)) {
Expand Down Expand Up @@ -102,7 +104,8 @@ class KotlinModule @Deprecated(
builder.isEnabled(KotlinFeature.SingletonSupport) -> CANONICALIZE
else -> DISABLED
},
builder.isEnabled(StrictNullChecks)
builder.isEnabled(StrictNullChecks),
builder.isEnabled(UseKotlinPropertyNameForGetter)
)

companion object {
Expand Down Expand Up @@ -130,7 +133,13 @@ class KotlinModule @Deprecated(
}

context.insertAnnotationIntrospector(KotlinAnnotationIntrospector(context, cache, nullToEmptyCollection, nullToEmptyMap, nullIsSameAsDefault))
context.appendAnnotationIntrospector(KotlinNamesAnnotationIntrospector(this, cache, ignoredClassesForImplyingJsonCreator))
context.appendAnnotationIntrospector(
KotlinNamesAnnotationIntrospector(
this,
cache,
ignoredClassesForImplyingJsonCreator,
useKotlinPropertyNameForGetter)
)

context.addDeserializers(KotlinDeserializers())
context.addKeyDeserializers(KotlinKeyDeserializers)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,35 +24,57 @@ import kotlin.reflect.full.hasAnnotation
import kotlin.reflect.full.memberProperties
import kotlin.reflect.full.primaryConstructor
import kotlin.reflect.jvm.internal.KotlinReflectionInternalError
import kotlin.reflect.jvm.javaGetter
import kotlin.reflect.jvm.javaType
import kotlin.reflect.jvm.kotlinFunction

internal class KotlinNamesAnnotationIntrospector(val module: KotlinModule, val cache: ReflectionCache, val ignoredClassesForImplyingJsonCreator: Set<KClass<*>>) : NopAnnotationIntrospector() {
internal class KotlinNamesAnnotationIntrospector(
val module: KotlinModule,
val cache: ReflectionCache,
val ignoredClassesForImplyingJsonCreator: Set<KClass<*>>,
val useKotlinPropertyNameForGetter: Boolean
) : NopAnnotationIntrospector() {
private fun getterNameFromJava(member: AnnotatedMethod): String? {
val name = member.name

// The reason for truncating after `-` is to truncate the random suffix
// given after the value class accessor name.
return when {
name.startsWith("get") -> name.takeIf { it.contains("-") }?.let { _ ->
name.substringAfter("get")
.replaceFirstChar { it.lowercase(Locale.getDefault()) }
.substringBefore('-')
}
// since 2.15: support Kotlin's way of handling "isXxx" backed properties where
// logical property name needs to remain "isXxx" and not become "xxx" as with Java Beans
// (see https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html and
// https://github.com/FasterXML/jackson-databind/issues/2527 and
// https://github.com/FasterXML/jackson-module-kotlin/issues/340
// for details)
name.startsWith("is") -> if (name.contains("-")) name.substringAfter("-") else name
else -> null
}
}

private fun getterNameFromKotlin(member: AnnotatedMethod): String? {
val getter = member.member

return member.member.declaringClass.takeIf { it.isKotlinClass() }?.let { clazz ->
clazz.kotlin.memberProperties.find { it.javaGetter == getter }
?.let { it.name }
}
}

// since 2.4
override fun findImplicitPropertyName(member: AnnotatedMember): String? {
if (!member.declaringClass.isKotlinClass()) return null

val name = member.name

return when (member) {
is AnnotatedMethod -> if (member.parameterCount == 0) {
// The reason for truncating after `-` is to truncate the random suffix
// given after the value class accessor name.
when {
name.startsWith("get") -> name.takeIf { it.contains("-") }?.let { _ ->
name.substringAfter("get")
.replaceFirstChar { it.lowercase(Locale.getDefault()) }
.substringBefore('-')
}
// since 2.15: support Kotlin's way of handling "isXxx" backed properties where
// logical property name needs to remain "isXxx" and not become "xxx" as with Java Beans
// (see https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html and
// https://github.com/FasterXML/jackson-databind/issues/2527 and
// https://github.com/FasterXML/jackson-module-kotlin/issues/340
// for details)
name.startsWith("is") -> if (name.contains("-")) name.substringAfter("-") else name
else -> null
}
if (useKotlinPropertyNameForGetter) {
// Fall back to default if it is a getter-like function
getterNameFromKotlin(member) ?: getterNameFromJava(member)
} else getterNameFromJava(member)
} else null
is AnnotatedParameter -> findKotlinParameterName(member)
else -> null
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.fasterxml.jackson.module.kotlin.test.github

import com.fasterxml.jackson.annotation.JsonProperty
import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.module.kotlin.KotlinFeature
import com.fasterxml.jackson.module.kotlin.KotlinModule
import org.junit.Test
import kotlin.test.assertEquals

class Github630 {
private val mapper = ObjectMapper()
.registerModule(KotlinModule.Builder().enable(KotlinFeature.UseKotlinPropertyNameForGetter).build())!!

data class Dto(
// from #570, #603
val FOO: Int = 0,
val bAr: Int = 0,
@JsonProperty("b")
val BAZ: Int = 0,
@JsonProperty("q")
val qUx: Int = 0,
// from #71
internal val quux: Int = 0,
// from #434
val `corge-corge`: Int = 0,
// additional
@get:JvmName("aaa")
val grault: Int = 0
)

@Test
fun test() {
val dto = Dto()

assertEquals(
"""{"FOO":0,"bAr":0,"b":0,"q":0,"quux":0,"corge-corge":0,"grault":0}""",
mapper.writeValueAsString(dto)
)
}
}