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.

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

Fix #340 #641

Merged
merged 4 commits into from
Mar 15, 2023
Merged

Fix #340 #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
80d289f
Modified the name of isGetter to be changed by findImplicitPropertyName
k163377 Mar 4, 2023
e99ea39
Modify fixed test case and move package
k163377 Mar 4, 2023
24e2968
Added test case for when PropertyNamingStrategy is specified
k163377 Mar 4, 2023
8ada33f
Added test case for isSetter to work.
k163377 Mar 4, 2023
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: 27 additions & 27 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 @@ -29,34 +29,34 @@ 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? = when (member) {
is AnnotatedMethod -> if (member.name.contains('-') && member.parameterCount == 0) {
when {
member.name.startsWith("get") -> member.name.substringAfter("get")
member.name.startsWith("is") -> member.name.substringAfter("is")
else -> null
}?.replaceFirstChar { it.lowercase(Locale.getDefault()) }?.substringBefore('-')
} else null
is AnnotatedParameter -> findKotlinParameterName(member)
else -> null
}

// 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
// https://github.com/FasterXML/jackson-databind/issues/2527
// for details)
override fun findRenameByField(config: MapperConfig<*>,
field: AnnotatedField,
implName: PropertyName): PropertyName? {
val origSimple = implName.simpleName
if (field.declaringClass.isKotlinClass() && origSimple.startsWith("is")) {
val mangledName: String? = BeanUtil.stdManglePropertyName(origSimple, 2)
if ((mangledName != null) && !mangledName.equals(origSimple)) {
return PropertyName.construct(mangledName)
}
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 { _ ->
Copy link
Member

Choose a reason for hiding this comment

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

Ok: I think you should ONLY need to consider isXxx() case, given that getXxx() should work by default.

The intent of "implicit name" was originally to allow exposing names for constructor parameters, because names were not included in byte code, unlike with methods and fields. So if there is no addition "implicit" name, underlying method/field name is used. It seems better (at least conceptually) to only override cases where there is difference.

Otherwise, I think that use of implicit name route may be better than rename using findRenameByField... if it works as well I see no reason for other case.

However: use of findRenameByField() may make sense for a different case: case where property name starts with capital letter. Intent with this method is that instead of starting with getters, setters and fields all to unify names, instead start with field name. This tends to work better for Kotlin, Scala etc where developers specify property name as "plain", similar to it being field (and in fact being added as Field) and getter/setter name is created based on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok: I think you should ONLY need to consider isXxx() case, given that getXxx() should work by default.

This branching only applies when the getter name contains -, so I believe the behavior is as you say.
The reason for this is as per the comments on lines 39 and 40.

I think that use of implicit name route may be better than rename using findRenameByField.

I don't see how using findRenameByField would avoid the problem (#340) of confusion when two properties, isFoo and foo, are defined (this is the main problem I wanted to solve).

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Ok, that - part I somehow missed although explicitly stated. Still, what I was trying to say is that I think you are right in preferring implicit name production over findRenameByFiedl() for cases you are thinking of solving.

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
}
} else null
is AnnotatedParameter -> findKotlinParameterName(member)
else -> null
}
return null
}

@Suppress("UNCHECKED_CAST")
Expand Down
52 changes: 34 additions & 18 deletions src/test/kotlin/com/fasterxml/jackson/module/kotlin/test/ParameterNameTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,27 @@ class TestJacksonWithKotlin {
val primaryAddress: String
val wrongName: Boolean
val createdDt: Date
val isName: Boolean

fun validate(
nameField: String = name,
ageField: Int = age,
addressField: String = primaryAddress,
wrongNameField: Boolean = wrongName,
createDtField: Date = createdDt
createDtField: Date = createdDt,
isNameField: Boolean = isName,
) {
assertThat(nameField, equalTo("Frank"))
assertThat(ageField, equalTo(30))
assertThat(addressField, equalTo("something here"))
assertThat(wrongNameField, equalTo(true))
assertThat(createDtField, equalTo(Date(1477419948000)))
assertThat(isNameField, equalTo(false))
}
}

private val normalCasedJson = """{"name":"Frank","age":30,"primaryAddress":"something here","renamed":true,"createdDt":"2016-10-25T18:25:48.000+00:00"}"""
private val pascalCasedJson = """{"Name":"Frank","Age":30,"PrimaryAddress":"something here","Renamed":true,"CreatedDt":"2016-10-25T18:25:48.000+00:00"}"""
private val normalCasedJson = """{"name":"Frank","age":30,"primaryAddress":"something here","renamed":true,"createdDt":"2016-10-25T18:25:48.000+00:00","isName":false}"""
private val pascalCasedJson = """{"Name":"Frank","Age":30,"PrimaryAddress":"something here","Renamed":true,"CreatedDt":"2016-10-25T18:25:48.000+00:00","IsName":false}"""

private val normalCasedMapper = jacksonObjectMapper()
.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false)
Expand All @@ -65,6 +68,7 @@ class TestJacksonWithKotlin {

override var primaryAddress: String = ""
override var createdDt: Date = Date()
override val isName: Boolean = false
}

@Test fun NoFailWithDefaultAndSpecificConstructor() {
Expand All @@ -79,7 +83,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
val renamed: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields {
@JsonIgnore
override val wrongName = renamed // here for the test validation only
Expand All @@ -97,7 +102,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
val renamed: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields {
@JsonIgnore
override val wrongName = renamed // here for the test validation only
Expand All @@ -121,7 +127,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
@JsonProperty("renamed") override val wrongName: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields

@Test fun testDataClassWithExplicitJsonCreatorAndJsonProperty() {
Expand All @@ -141,7 +148,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
@JsonProperty("renamed") override val wrongName: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields

@Test fun testNormalClassWithJsonCreator() {
Expand All @@ -155,7 +163,8 @@ class TestJacksonWithKotlin {
private class StateObjectWithPartialFieldsInConstructor(
override val name: String,
override val age: Int,
override val primaryAddress: String
override val primaryAddress: String,
override val isName: Boolean
) : TestFields {
@JsonProperty("renamed") override var wrongName: Boolean = false
override var createdDt: Date by Delegates.notNull()
Expand All @@ -176,7 +185,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
@JsonProperty("renamed") override val wrongName: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields

@Test fun testDataClassWithNonFieldParametersInConstructor() {
Expand Down Expand Up @@ -207,7 +217,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
override val wrongName: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields {
var factoryUsed: Boolean = false
companion object {
Expand All @@ -216,9 +227,10 @@ class TestJacksonWithKotlin {
@JsonProperty("age") age: Int,
@JsonProperty("primaryAddress") primaryAddress: String,
@JsonProperty("renamed") wrongName: Boolean,
@JsonProperty("createdDt") createdDt: Date
@JsonProperty("createdDt") createdDt: Date,
@JsonProperty("isName") isName: Boolean
): StateObjectWithFactory {
val obj = StateObjectWithFactory(nameThing, age, primaryAddress, wrongName, createdDt)
val obj = StateObjectWithFactory(nameThing, age, primaryAddress, wrongName, createdDt, isName)
obj.factoryUsed = true
return obj
}
Expand All @@ -236,17 +248,19 @@ class TestJacksonWithKotlin {
val age: Int,
val primaryAddress: String,
val renamed: Boolean,
val createdDt: Date
val createdDt: Date,
val isName: Boolean
) {
companion object {
@JvmStatic @JsonCreator fun create(
name: String,
age: Int,
primaryAddress: String,
renamed: Boolean,
createdDt: Date
createdDt: Date,
isName: Boolean
): StateObjectWithFactoryNoParamAnnotations {
return StateObjectWithFactoryNoParamAnnotations(name, age, primaryAddress, renamed, createdDt)
return StateObjectWithFactoryNoParamAnnotations(name, age, primaryAddress, renamed, createdDt, isName)
}
}
}
Expand All @@ -266,7 +280,8 @@ class TestJacksonWithKotlin {
override val age: Int,
override val primaryAddress: String,
override val wrongName: Boolean,
override val createdDt: Date
override val createdDt: Date,
override val isName: Boolean
) : TestFields {
var factoryUsed: Boolean = false
private companion object Named {
Expand All @@ -275,9 +290,10 @@ class TestJacksonWithKotlin {
@JsonProperty("age") age: Int,
@JsonProperty("primaryAddress") primaryAddress: String,
@JsonProperty("renamed") wrongName: Boolean,
@JsonProperty("createdDt") createdDt: Date
@JsonProperty("createdDt") createdDt: Date,
@JsonProperty("isName") isName: Boolean
): StateObjectWithFactoryOnNamedCompanion {
val obj = StateObjectWithFactoryOnNamedCompanion(nameThing, age, primaryAddress, wrongName, createdDt)
val obj = StateObjectWithFactoryOnNamedCompanion(nameThing, age, primaryAddress, wrongName, createdDt, isName)
obj.factoryUsed = true
return obj
}
Expand Down
25 changes: 18 additions & 7 deletions ...e/kotlin/test/github/failing/Github340.kt → ...on/module/kotlin/test/github/Github340.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
package com.fasterxml.jackson.module.kotlin.test.github.failing
package com.fasterxml.jackson.module.kotlin.test.github

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import com.fasterxml.jackson.module.kotlin.kotlinModule
import com.fasterxml.jackson.module.kotlin.readValue
import com.fasterxml.jackson.module.kotlin.test.expectFailure
import org.junit.Test
import kotlin.test.assertEquals

Expand All @@ -21,15 +20,27 @@ class OwnerRequestTest {

@Test
fun testDeserHit340() {
expectFailure<UnrecognizedPropertyException>("GitHub #340 has been fixed!") {
val value: IsField = jackson.readValue(json)
assertEquals("Got a foo", value.foo)
}
val value: IsField = jackson.readValue(json)
// Fixed
assertEquals("Got a foo", value.foo)
}

@Test
fun testDeserWithoutIssue() {
val value: NoIsField = jackson.readValue(json)
assertEquals("Got a foo", value.foo)
}

// A test case for isSetter to work, added with the fix for this issue.
class IsSetter {
lateinit var isFoo: String
}

@Test
fun isSetterTest() {
val json = """{"isFoo":"bar"}"""
val isSetter: IsSetter = jackson.readValue(json)

assertEquals("bar", isSetter.isFoo)
}
}