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

objectMapper.treeToValue is error-prone, should be either deprecated or fixed (or removed?) #496

Closed
Fuud opened this issue Aug 31, 2021 · 4 comments · Fixed by #497
Closed

Comments

@Fuud
Copy link
Contributor

Fuud commented Aug 31, 2021

Use case
There is old issue: #45

Main problem: ObjectMapper.treeToValue looks very similar to ObjectMapper.readValue but suffers from type erasure.
It is very simple to missuse it.
Current behaviour is because ObjectMapper.treeToValue(TreeNode, TypeReference) does not exist.
As suggested in FasterXML/jackson-databind#1294 we can use readValue or convertValue.

Describe the solution you'd like
Let's either:

  1. reimplement ObjectMapper.treeToValue in type-safe way
  2. deprecate it with descriptive message
@dinomite
Copy link
Member

Huh, I've never used treeToValue, I think the best fix would be to implement it using Tatu's suggestion in that ticket: FasterXML/jackson-databind#1294 (comment)

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 1, 2021

@Fuud why is this in Kotlin module? If it's about ObjectMapper, this belong under jackson-databind.

I am also not quite certain I understand why you claim treeToValue() is error-prone, compared to, say, readValue().
And I certainly would not want to deprecate the existing method, as that is convenient for vast majority of use cases where target type is not generic.

However: I'd be +1 for adding overloads where type can be passed as JavaType or TypeReference<T>: these would be rather easy to add one(s) for 2.13 at this point. I'd be happy to do that (or help merge a PR) if this is something something wants. All we need is an issue in jackson-databind asking for 1 or 2 overloads (might as well do both: one with JavaType is mandatory, TypeReference optional).

Note, also, that ObjectReader has 2 variants: JavaType taking one added in 2.13.

@Fuud
Copy link
Contributor Author

Fuud commented Sep 1, 2021

@cowtowncoder

There is no such problem in core jackson library because it is clean signature there: treeToValue

public <T> T treeToValue(TreeNode n, Class<T> valueType)

It is clear that type erasure will work in this case.

But jackson-module-kotlin brings extension functions
And without diving in implementations you cannot say which line is wrong:

import com.fasterxml.jackson.module.kotlin.convertValue
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import com.fasterxml.jackson.module.kotlin.readValue
import com.fasterxml.jackson.module.kotlin.treeToValue

data class Person(val name: String)
fun main() {
    val mapper = jacksonObjectMapper()
    val node = mapper.readTree("""[ {"name":"Alice"}, {"name" : "Bob"} ]""")


    val readValueResult: List<Person> = mapper.readValue(node.toString())
    val convertValueResult: List<Person> = mapper.convertValue(node)
    val treeToValueResult: List<Person> = mapper.treeToValue(node)!!


    println("readValueResult Classes: ")
    println(readValueResult.map { it::class.simpleName })

    println("convertValueResult Classes: ")
    println(convertValueResult.map { it::class.simpleName })

    println("treeToValueResult Classes: ")
    println(treeToValueResult.map { it::class.simpleName })

}
Click to see output output:
readValueResult Classes: 
[Person, Person]
convertValueResult Classes: 
[Person, Person]
treeToValueResult Classes: 
Exception in thread "main" java.lang.ClassCastException: java.util.LinkedHashMap cannot be cast to Person
  at MainKt.main(main.kt:42)
  at MainKt.main(main.kt)

@cowtowncoder
Copy link
Member

Ahhhh. Thank you @Fuud that makes sense. Thank you for clarifying!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants