Skip to content

Fixed serialization/deserialization failure when the name of getter contains -. #451

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Fixed serialization/deserialization failure when the name of getter contains -. #451

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b16059c
add test cases
k163377 May 25, 2021
858e0c2
fix issue
k163377 May 25, 2021
b67bb9a
separation
k163377 May 27, 2021
5c97eab
fix name
k163377 May 27, 2021
e60a6fd
fix cond
k163377 May 28, 2021
55117c8
Modified to use the name of KProperty unconditionally.
k163377 May 28, 2021
378c3df
Ignore class that disappeared
dinomite May 28, 2021
460923f
add getter like method support
k163377 Jun 5, 2021
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
54 changes: 28 additions & 26 deletions src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinNamesAnnotationIntrospector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,38 +18,42 @@ import java.util.Locale
import kotlin.reflect.KClass
import kotlin.reflect.KFunction
import kotlin.reflect.KParameter
import kotlin.reflect.full.companionObject
import kotlin.reflect.full.declaredFunctions
import kotlin.reflect.full.memberProperties
import kotlin.reflect.full.primaryConstructor
import kotlin.reflect.full.*
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() {
// since 2.4
override fun findImplicitPropertyName(member: AnnotatedMember): String? {
if (member is AnnotatedMethod && member.isInlineClass()) {
if (member.name.startsWith("get") &&
member.name.contains('-') &&
member.parameterCount == 0) {
return member.name.substringAfter("get")
.replaceFirstChar { it.lowercase(Locale.getDefault()) }
.substringBefore('-')
} else if (member.name.startsWith("is") &&
member.name.contains('-') &&
member.parameterCount == 0) {
return member.name.substringAfter("is")
.replaceFirstChar { it.lowercase(Locale.getDefault()) }
.substringBefore('-')
}
} else if (member is AnnotatedParameter) {
return findKotlinParameterName(member)
}

return null
override fun findImplicitPropertyName(member: AnnotatedMember): String? = when (member) {
is AnnotatedMethod -> findImplicitPropertyNameFromKotlinPropertyIfNeeded(member)
is AnnotatedParameter -> findKotlinParameterName(member)
else -> null
}

// Since getter for value class (inline class) will be compiled into a different name such as "getFoo-${random}",
// use the property name in Kotlin in such a case.
private fun findImplicitPropertyNameFromKotlinPropertyIfNeeded(member: AnnotatedMethod): String? = member
.takeIf {
it.parameterCount == 0 &&
it.name.run { contains("-") && (startsWith("get") || startsWith("is")) }
}?.let { _ ->
val propertyNameFromGetter = member.name.let {
when {
it.startsWith("get") -> it.substringAfter("get")
it.startsWith("is") -> it.substringAfter("is")
else -> throw IllegalStateException("Should not get here.")
}
}.replaceFirstChar { it.lowercase(Locale.getDefault()) }
Copy link
Member

Choose a reason for hiding this comment

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

I haven't opened this in an IDE so could be wrong, but does this work if you use when (member.name) and drop the .let {}?

            val propertyNameFromGetter = when (member.name) {
                    it.startsWith("get") -> it.substringAfter("get")
                    it.startsWith("is") -> it.substringAfter("is")
                    else -> throw IllegalStateException("Should not get here.")
            }.replaceFirstChar { it.lowercase(Locale.getDefault()) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but it seemed that I could only write the following.

val propertyNameFromGetter = when  {
    member.name.startsWith("get") -> member.name.substringAfter("get")
    member.name.startsWith("is") -> member.name.substringAfter("is")
    else -> throw IllegalStateException("Should not get here.")
}.replaceFirstChar { it.lowercase(Locale.getDefault()) }

I thought it would be better to use let because member.name would be repeated many times.

Copy link
Member

Choose a reason for hiding this comment

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

Quick note: lower-casing may be incorrect here. Consider case of getURL() which should produce, I think, URL, as per Java Beans definition. But if code above simply lower-cases the first character, it'd return uRL.
(Jackson itself has bit of inconsistency, originally returning url for that case, lower-casing all leading upper-case chars; but then adding option for producing URL)

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
In this scene, if the name retrieved from the getter is different from the name of the property, the name of the property will be adopted.
In other words, even if the name retrieved from the getter is wrong, the name of the property (= expected value) will be adopted as a result.
For this reason, I thought the problem you pointed out would not occur here.

Copy link
Contributor Author

@k163377 k163377 May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed that the pattern where getter is named with @JvmGetter @get:JvmName doesn't cover some areas.
It looks like the property names should not be used in this pattern.

I will revisit this later as I don't have much more time right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#451 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure exactly how much Kotlin module changes things, but the way basic Jackson property inspection works is that it first gets all "implicit" (and explicit) names of accessors, then joins similarly named entries (by implicit name), then does renaming of grouped accessors.
In this scheme, then, if implicit name from getter does not match those of fields, they would infer different non-related properties.

I have been hoping to change handling to let modules (Kotlin and Scala mostly) customize logic more to essentially "start with fields" and use different merging logic. But right now ways to override things tend to have unexpected side-effects.

For this particular case, also, I suspect that if matching of accessors used case-insensitive handling (not trivial to do as retrofit but something that has been requested and seems useful), mismatch could be avoided. And like you say, could then just use name from field (implicit name or explicit annotation)/


member.declaringClass.kotlin.declaredMemberProperties.find { kProperty1 ->
kProperty1.javaGetter
?.let { it == member.member && kProperty1.name != propertyNameFromGetter }
?: false
}?.name
}

// since 2.11: 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
Expand Down Expand Up @@ -169,5 +173,3 @@ internal class KotlinNamesAnnotationIntrospector(val module: KotlinModule, val c
}
}
}

private fun AnnotatedMethod.isInlineClass() = declaringClass.declaredMethods.any { it.name.contains('-') }
38 changes: 38 additions & 0 deletions src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/github/Github356.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,30 @@ class TestGithub356 {
assertEquals("""{"inlineClassProperty":"bar"}""", mapper.writeValueAsString(original))
}

@Test
fun deserializeKebabInlineMember() {
val original = ClassWithKebabInlineMember(ValueClass("bar"))
assertEquals(original, mapper.readValue(mapper.writeValueAsString(original)))
}

@Test
fun serializeKebabInlineClass() {
val original = ClassWithKebabInlineMember(ValueClass("bar"))
assertEquals("""{"value-class-property":"bar"}""", mapper.writeValueAsString(original))
}

@Test
fun deserializeNamedInlineClass() {
val original = ClassWithNamedInlineMember(ValueClass("bar"))
assertEquals(original, mapper.readValue(mapper.writeValueAsString(original)))
}

@Test
fun serializeNamedInlineClass() {
val original = ClassWithNamedInlineMember(ValueClass("bar"))
assertEquals("""{"value-":"bar"}""", mapper.writeValueAsString(original))
}

@Test
fun deserializeValueClass() {
val original = ClassWithValueMember(ValueClass("bar"))
Expand Down Expand Up @@ -54,3 +78,17 @@ data class ClassWithValueMember(val valueClassProperty: ValueClass) {
fun build() = ClassWithValueMember(ValueClass(valueClassProperty))
}
}

@JsonDeserialize(builder = ClassWithKebabInlineMember.JacksonBuilder::class)
data class ClassWithKebabInlineMember(val `value-class-property`: ValueClass) {
data class JacksonBuilder constructor(val `value-class-property`: String) {
fun build() = ClassWithKebabInlineMember(ValueClass(`value-class-property`))
}
}

@JsonDeserialize(builder = ClassWithNamedInlineMember.JacksonBuilder::class)
data class ClassWithNamedInlineMember(@get:JvmName("getValue-") val `value-`: ValueClass) {
data class JacksonBuilder constructor(val `value-`: String) {
fun build() = ClassWithNamedInlineMember(ValueClass(`value-`))
}
}