FasterXML / jackson-module-kotlin

Module that adds support for serialization/deserialization of Kotlin (http://kotlinlang.org) classes and data classes.
Apache License 2.0
1.12k stars 176 forks source link

Boolean property name starting with 'is' not serialized/deserialized properly #80

Closed tbvh closed 4 years ago

tbvh commented 7 years ago

Hi, I'm experiencing issues with a boolean property which name starts with 'is'. While the getter doesn't get prefixed by kotlin, it seems that jackson still strips away 'is' as though it was a prefix. The resulting json doesn't contain the 'is', and deserialization fails.

This test illustrates the problem:

class SerializationTest {
    @Test
    fun testIsBool() {
        val mapper = ObjectMapper()
        mapper.registerModule(KotlinModule())

        val example = IsBoolExample(true)
        val json = mapper.writeValueAsString(example) 
        // json contains: {"trueOrFalse":true}

        val deserialized = mapper.readValue(json, IsBoolExample::class.java)

        Assert.assertEquals(example.isTrueOrFalse, deserialized.isTrueOrFalse)
    }
}
class IsBoolExample(val isTrueOrFalse: Boolean)

This fails on mapper.readValue(..) with:

com.fasterxml.jackson.databind.JsonMappingException: Can not construct instance of com.bol.service.reptile.spring.IsBoolExample: no suitable constructor found, can not deserialize from Object value (missing default constructor or creator, or perhaps need to add/enable type information?) at [Source: {"trueOrFalse":true}; line: 1, column: 2]

tbvh commented 7 years ago

Note that the following test succeeds as expected:

class SerializationTest {
    @Test
    fun testBool() {
        val mapper = ObjectMapper()
        mapper.registerModule(KotlinModule())

        val example = BoolExample(true)
        val json = mapper.writeValueAsString(example)
        // json contains: {"trueOrFalse":true}

        val deserialized = mapper.readValue(json, BoolExample::class.java)

        Assert.assertEquals(example.trueOrFalse, deserialized.trueOrFalse)
    }
}
class BoolExample(val trueOrFalse: Boolean)

And finally, if another field is present (so a constructor can be matched) and FAIL_ON_UNKNOWN_PROPERTIES is false, then the boolean parameter stays (defaults to) false.

class SerializationTest {
    @Test
    fun testStrIsBoolExample() {
        val mapper = ObjectMapper()
        mapper.registerModule(KotlinModule())
        mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)

        val example = StrIsBoolExample(true, "example")
        val json = mapper.writeValueAsString(example)
        // json contains: {"str":"example","trueOrFalse":true}

        val deserialized = mapper.readValue(json, StrIsBoolExample::class.java)

        Assert.assertEquals(example.isTrueOrFalse, deserialized.isTrueOrFalse)
    }
}
class StrIsBoolExample(val isTrueOrFalse: Boolean,val str: String)

Here the assert fails.

hizhengfu commented 7 years ago

Encountered the same problem, It was also found that pId was serialized to pid

DavidRigglemanININ commented 6 years ago

I'm seeing this issue as well. One thing I noticed is that if I flip the val to a var, then it works as expected. So I'm guessing the issue is that the val generates a Java getter but not the setter, and that's what's causing the problem here.

apatrida commented 6 years ago

Jackson expects JavaBean naming patterns, Kotlin does not always generate things that way and therefore creates an unwinnable situation. I'm still looking at options, but there is a workaround for some cases.

This works by adding a JsonProperty annotation:

  @Test
    fun testGithub80_FailOnBoolWith_Is() {
        val mapper = jacksonObjectMapper()

        val example = IsBoolExample(true)
        val json = mapper.writeValueAsString(example)
        // json contains: {"trueOrFalse":true}

        val deserialized = mapper.readValue<IsBoolExample>(json)

        assertEquals(example.isTrueOrFalse, deserialized.isTrueOrFalse)
    }

    class IsBoolExample(@JsonProperty("trueOrFalse") val isTrueOrFalse: Boolean)
raderio commented 6 years ago

If in Kotlin I have class IsBoolExample(val isTrueOrFalse: Boolean) I expect to have isTrueOrFalse in json, not trueOrFalse, if I need trueOrFalse I will have trueOrFalse property in class.

So I think it will be ok to add @JsonProperty with values for boolean properties by default by jackson-module-kotlin, or at least to be configurable by property in module.

jdorleans commented 6 years ago

I have a similar problem for Boolean property when its name starts with is*. In my case, it is already a var and it doesn't work as mentioned above. The only trivial way I found was to add @JsonProperty, since I want its name to be recognised as it is and not configure the module to ignore it. Here's my example:

class Product {
  var isNew: Boolean? = null
}
...
val map = mapOf("isNew" to true) // Deserialized Product JSON in a Map<String, Any?>
mapper.convertValue(map, Product::class.java) // FAIL 

The error message says property isNew is not recognised and the only known property is called: new.

dynamics3 commented 6 years ago

We have encountered this thing as well. When we receive json that has a boolean, which starts with "is", then it fails on jacksonObjectMapper().readValue<TestData>(jsonString) throwing UnrecognizedPropertyException. Example: data class TestData(val isOk: Boolean = false)

The funny thing is that when another property is added to data class, then it works flawlessly, e.g. data class TestData(val addedProp: String = "", val isOk: Boolean = false).

Adding @get:JsonProperty("isOk") annotation fixes the issue. However, it would be great if the appropriate solution is added.

ahmetgeymen commented 6 years ago

I have also encountered with this issue with my String property. I overcame with using annotation @get:JsonProperty("isSomething"). Though I think we should not use any annotation, Jackson should serialize properly.

See also: (https://github.com/FasterXML/jackson-module-kotlin/issues/52)

sschuberth commented 6 years ago

Another incarnation of the problem seems to be that @JsonIgnore does not work on such properties. For code

@JsonIgnore
val isWholeProjectExcluded = true

the property still gets serialized (as "wholeProjectExcluded").

A work-around is to write the code as

val isWholeProjectExcluded
        @JsonIgnore
        get() = true

or shorter, as @Tapac points out, as

@get:JsonIgnore
val isWholeProjectExcluded = true
tjsnell commented 5 years ago

Just wasted a bunch of time tracking this down. Any plans to fix it?

Tapac commented 5 years ago

@sschuberth, you can use get as annotation target (see more here) @get:JsonIgnore val isWholeProjectExcluded = true

sschuberth commented 5 years ago

Thanks @Tapac, will use that!

brokenthorn commented 5 years ago

We have encountered this thing as well. When we receive json that has a boolean, which starts with "is", then it fails on jacksonObjectMapper().readValue<TestData>(jsonString) throwing UnrecognizedPropertyException. Example: data class TestData(val isOk: Boolean = false)

The funny thing is that when another property is added to data class, then it works flawlessly, e.g. data class TestData(val addedProp: String = "", val isOk: Boolean = false).

Adding @JsonProperty("isOk") annotation fixes the issue. However, it would be great if the appropriate solution is added.

@JsonProperty does not work for me.

My code actually looks like this:

@JsonProperty("isSubstance")
val isSubstance: Boolean = false,

and it still serializes as substance.

raderio commented 5 years ago

@brokenthorn you need to use @get:JsonProperty("isSubstance")

dynamics3 commented 5 years ago

We have encountered this thing as well. When we receive json that has a boolean, which starts with "is", then it fails on jacksonObjectMapper().readValue<TestData>(jsonString) throwing UnrecognizedPropertyException. Example: data class TestData(val isOk: Boolean = false) The funny thing is that when another property is added to data class, then it works flawlessly, e.g. data class TestData(val addedProp: String = "", val isOk: Boolean = false). Adding @JsonProperty("isOk") annotation fixes the issue. However, it would be great if the appropriate solution is added.

@JsonProperty does not work for me.

My code actually looks like this:

@JsonProperty("isSubstance")
val isSubstance: Boolean = false,

and it still serializes as substance.

As @raderio has already mentioned, you need to use @get:JsonPropert("isSubstance"). Sorry about that, I have edited my comment as well to reflect this. Somehow, made a mistake when wrote that post. Good luck with your code!!!

milosmns commented 5 years ago

But why is this happening? There is no straight answer. Booleans even get compiled to fields in Java prefixed with "is". There is absolutely no reason to strip the "is", please fix this.

brokenthorn commented 5 years ago

But why is this happening? There is no straight answer. Booleans even get compiled to fields in Java prefixed with "is". There is absolutely no reason to strip the "is", please fix this.

Totally agree. This seems very inconsistent and makes implementing serialization from other platforms that follow a more expected format, harder.

daliborfilus commented 5 years ago

Oh my...

cowtowncoder commented 5 years ago

Not sure why this would happen with Kotlin module, but core Jackson databind ONLY considers "is"-prefix for getters and setters -- field names (and constructor parameter names if available) are used as-is and do not infer any special handling.

DavidRigglemanININ commented 5 years ago

Could it be that a kotlin val/var has a auto generated getter/setter for Java compatibility purposes that is prefixed with "is"? For example, if I have a kotlin property "done", I believe it will generate a getter/setter where you can call isDone() from Java.

brokenthorn commented 5 years ago

Could it be that a kotlin val/var has a auto generated getter/setter for Java compatibility purposes that is prefixed with "is"? For example, if I have a kotlin property "done", I believe it will generate a getter/setter where you can call isDone() from Java.

Makes no sense then to remove the is part when something like isDone is defined in Kotlin, not Java.

DavidRigglemanININ commented 5 years ago

So I did a deep dive and figured out what was going on. My general assumption was mostly correct in that it was an issue with the generated getter/setters, although just in a slightly different way than I originally thought. The way that Kotlin works is that if you have a val boolean field isTrueOrFalse, Kotlin will generate a getter (and a setter if it's a var). For booleans, Kotlin appears to generate the getter/setters with the same name (it seems to skip adding the "is" prefix if the boolean field already is prefixed by "is"; interestingly, the same behavior does not appear to happen in non boolean getters where the field name starts with get). So now we are left with having both a field and method for a given property. Now inside the Jackson data bind code (so not part of kotlin module), there is logic to get rid of the field (BeanSerializaerFactory.removeIgnorableTypes), leaving only the method left. Now there is code later on to find the implicit property name. But because it's a method and not a field, it calls BeanUtil.okNameForIsGetter. This logic in turn strips out the "is" prefix because it's working under the standard Java naming convention for beans where the underlying field does not have the "is" but the getter does. So where does this leave us? I'm not sure what the best solution may be, but here's one proposal I'm currently testing locally that only requires changes to this module code. In KotlinModule.kt, update the code to contain the following

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 javaConstructorIsCreatorAnnotated = LRUMap<AnnotatedConstructor, Boolean>(reflectionCacheSize, reflectionCacheSize)
    private val kotlinGeneratedBooleanMethod = LRUMap<AnnotatedMethod, Boolean>(reflectionCacheSize, reflectionCacheSize)

    fun kotlinFromJava(key: Class<Any>): KClass<Any> = javaClassToKotlin.get(key) ?: key.kotlin.let { javaClassToKotlin.putIfAbsent(key, it) ?: it }
    fun kotlinFromJava(key: Constructor<Any>): KFunction<Any>? = javaConstructorToKotlin.get(key) ?: key.kotlinFunction?.let { javaConstructorToKotlin.putIfAbsent(key, it) ?: it }
    fun kotlinFromJava(key: Method): KFunction<*>? = javaMethodToKotlin.get(key) ?: key.kotlinFunction?.let { javaMethodToKotlin.putIfAbsent(key, it) ?: it }
    fun checkConstructorIsCreatorAnnotated(key: AnnotatedConstructor, calc: (AnnotatedConstructor) -> Boolean): Boolean = javaConstructorIsCreatorAnnotated.get(key) ?: calc(key).let { javaConstructorIsCreatorAnnotated.putIfAbsent(key, it) ?: it }
    fun isKotlinGeneratedBooleanMethod(key: AnnotatedMethod, calc: (AnnotatedMethod) -> Boolean): Boolean = kotlinGeneratedBooleanMethod.get(key) ?: calc(key).let { kotlinGeneratedBooleanMethod.putIfAbsent(key, it) ?: it }
}

...

override fun findImplicitPropertyName(member: AnnotatedMember): String? {
        if (member is AnnotatedParameter) {
            return findKotlinParameterName(member)
        } else if (member is AnnotatedMethod) {
            if (member.declaringClass.isKotlinClass() && (member.rawReturnType == Boolean::class.java || member.rawReturnType == java.lang.Boolean::class.java)) {
                if (cache.isKotlinGeneratedBooleanMethod(member, { it.declaringClass.declaredFields.any { f -> f.name == member.name } })) {
                    return member.name
                }
            }
        }
        return null
    }

What I am doing here is basically checking if there is a field with the same name as the method, then use the method name (without any name mangling). I've limited it to kotlin, boolean properties, and backed with a cache to keep the impact as small as possible and also for performance reasons since the check involves reflection.

Note that my solution will ignore JsonProperty annotations explicitly setting it to a different name. However, from my testing, I've found that JsonProperty doesn't even work if you have a "isTrueOrFalse" property name with JsonProperty("foo") annotation (serializing works but deserializing fails). If you did want to honor the property, you could add a check of !member.hasAnnotation(JsonProperty::class.java) right at the beginning of if(cache.isKotlinGeneratedBooleanMethod...

adhesivee commented 5 years ago

When will this be fixed? I still have this issue with 2.9.9 and is causing some unexpected behavior.

CraftingGamerTom commented 5 years ago

For me it doesn't seem to be an issue with "is.." rather an issue with the primitive type 'boolean'. Jackson serializes 'Boolean" types okay

garydwatson commented 5 years ago

Not sure if this is relevant, but I could get the serialization to work properly with the following code....

data class Kitten( val name: String, @get:JvmName("getIs_hairless") val is_hairless: Boolean

)

json looks like this {"name": "friskers", "is_hairless": true}

serialization works in both directions.

breun commented 5 years ago

@garydwatson Using @get:JsonProperty("is_hairless") instead of @get:JvmName("getIs_hairless") as suggested earlier makes a bit more sense to me.

ElbowBaggins commented 5 years ago

@breun That really depends on what angle you're trying to take to fix it.

Basically the issue here is that Jackson is having difficulty grappling with how Kotlin generates the JVM code for boolean vals in data classes. For example, if you have a property isHairless, Kotlin detects this and generates a getter called isHairless(). If you change it from a val to a var you also get a setHairless() which I reckon is why changing it to a var also 'solves' this problem, as Jackson recognizes this pairing as a single property and the @JsonProperty annotation can then work as expected. As a side note, if you simply name the property hairless you will get a getHairless() (and setHairless() in the var case). So it appears that the Kotlin compiler only produces isSomething() getters if and only if the property is ALSO named isSomething. Presumably to keep use-sites in both Java and Kotlin fairly similar looking.

Since a val provides no setter but you can set the value, @JsonProperty almost certainly must be looking at the named constructor parameter (called isHairless). It then sees a getter called isHairless() which it believes to be a completely separate property called hairless.

So it seems the true issue is that the name processing Jackson does for getters and setters isn't also occurring for the named constructor parameters (which is pretty fair when you look at it from a Java-language-centric point of view) so this 'connection', if you will, between the constructor parameter and its generated getter.

Fixing this with @get:JsonProperty("isHairless") doesn't actually solve this problem, it just renames the improperly named property in the response.

Fixing this with @get:JvmName("getIsHairless") causes Jackson to actually detect that the constructor parameter and getter refer to the same property because it forces the getter to have the provided name.

Basically, the named constructor parameter handling, the getter/setter handling, and Kotlin's own generation of getters and setters specifically for boolean values have all come together to cause a very unique problem.

Basically, this is effectively what's happening: You've written

data class Something(
  @JsonProperty("isHere")
  val isHere: bool
)

which is "expanding" to (yes I know that's not how it works, this is an illustration)

data class Something(
  @param:JsonProperty("isHere")
  @get:JsonProperty("here")
  val isHere: bool
)

but it should be

data class Something(
  @param:JsonProperty("isHere")
  @get:JsonProperty("isHere")
  val isHere: bool
)

This seems like a pretty clear-cut bug with Jackson, either because they missed this detail or Kotlin's behavior with regards to automatic getter/setter naming has changed since this was originally implemented.

breun commented 5 years ago

If you are familiar with the implementation details, then indeed maybe @get:JvmName makes more sense, but at least @get:JsonProperty("is_hairless") reads like "the JSON property is called is_hairless", which is true, while that JVM name just looks plain weird if you don't know what's going on under the hood.

cowtowncoder commented 5 years ago

@ElbowBaggins this is an interesting read: I assume logic is documented by Kotlin language authors but I wonder if that was available for linking? Knowing exact rules would make it easier to make Jackson core support this variatio better: modules can configure many aspects, but there's fine balances in how to split responsibilities. I suspect it'd be possible to use a combination of MapperFeature(s) and AnnotationIntrospector override(s). Or maybe even just former... "is-getter" detection is not easily overridable yet but its implementation is not terribly complicated. Timeline-wise solving this issue is something that would be great for Jackson 2.11 (assuming changes are needed to some of APIs).

Oh and just to make sure above makes sense: currently Jackson core knows absolutely nothing about Kotlin's representation of things -- it just follows the way Java Beans would name things. Adding little bit of knowledge (but without changing semantics for non-Kotlin use) would be the ideal change from my POV.

ElbowBaggins commented 4 years ago

@cowtowncoder Sorry for the late reply, but they've documented this here.

https://kotlinlang.org/docs/reference/java-to-kotlin-interop.html

Interestingly, this does not appear in their documentation for properties, only in the interop section.

cowtowncoder commented 4 years ago

@ElbowBaggins thanks!

cowtowncoder commented 4 years ago

Merged @LipatovAndrey 's fix -- will be in 2.10.1

And also contemplating actually adding databind MapperFeature in 2.11, to allow this out of the box (possibly to further improve in 3.0).

FabrizioBilleciUNICT commented 4 years ago

@get:PropertyName("isSomething") var isSomething works also for me.

LifeIsStrange commented 4 years ago

@cowtowncoder I thought this would be fixed out of the box for jackson-module-kotlin 2.11 but I am using the latest stable version through implementation("com.fasterxml.jackson.module:jackson-module-kotlin") so I believe I should have 2.11 on my system and am still hitting this issue. (yes I manually solve it through @get:JsonProperty)

MaksimRT commented 4 years ago

@cowtowncoder I thought this would be fixed out of the box for jackson-module-kotlin 2.11 but I am using the latest stable version through implementation("com.fasterxml.jackson.module:jackson-module-kotlin") so I believe I should have 2.11 on my system and am still hitting this issue. (yes I manually solve it through @get:JsonProperty)

Have you tried to add jackson-databind module as well?)

It will cause an issue without it(

        <dependency>
            <groupId>com.fasterxml.jackson.module</groupId>
            <artifactId>jackson-module-kotlin</artifactId>
            <version>${jackson-module-kotlin.version}</version>
        </dependency>
        <dependency>
            <groupId>com.fasterxml.jackson.core</groupId>
            <artifactId>jackson-databind</artifactId>
            <version>${jackson-databind.version}</version>
        </dependency>
cowtowncoder commented 4 years ago

Yes, both Kotlin module and jackson-databind would need to be 2.11(.2).