Skip to content

On the priority of JsonCreator and Constructor #514

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

Open
k163377 opened this issue Nov 3, 2021 · 9 comments
Open

On the priority of JsonCreator and Constructor #514

k163377 opened this issue Nov 3, 2021 · 9 comments
Labels

Comments

@k163377
Copy link
Contributor

k163377 commented Nov 3, 2021

If a Constructor and a JsonCreator are defined that take the same type as an argument, as shown below, I felt that the JsonCreator would take precedence.

data class ByFactory(val value: String) {
    companion object {
        @JvmStatic
        @JsonCreator
        fun creator(value: String) = ByFactory("$value by factory.")
    }
}

On the other hand, the following test shows that Constructor is given priority.

// java.lang.AssertionError: 
// Expected :ByFactory(value=aaa by factory.)
// Actual   :ByFactory(value=aaa)
assertEquals(ByFactory("aaa by factory."), jacksonObjectMapper().readValue("\"aaa\""))

Which takes precedence, the JsonCreator or the Constructor?
Also, if there is a document that shows what priority is given to JsonCreator as Jackson, it would be helpful.

@dinomite
Copy link
Member

dinomite commented Nov 3, 2021

Oh, well that's pretty interesting—I, too, would expect the explicit annotation to win. This is likely controlled in jackson-databind, so @cowtowncoder might know of the top of his head.

@k163377
Copy link
Contributor Author

k163377 commented Nov 23, 2021

I also have a question about the behavior when defining multiple JsonCreators.
As shown in the following test, when I define a JsonCreator in both Constructor and Factory, no error occurs.

data class D1 @JsonCreator constructor(val foo: Int, val bar: String) {
    @JsonCreator constructor(foo: Int, bar: Int) : this(foo, bar.toString())
}

data class D2(val foo: Int, val bar: String) {
    @JsonCreator constructor(foo: Int, bar: Int) : this(foo, bar.toString())

    companion object {
        @JvmStatic
        @JsonCreator
        fun creator(foo: Int, bar: String) = D2(foo, bar)
    }
}

data class D3(val foo: Int, val bar: String) {
    companion object {
        @JvmStatic
        @JsonCreator
        fun creator(foo: Int, bar: String) = D3(foo, bar)

        @JvmStatic
        @JsonCreator
        fun creator(foo: Int, bar: Int) = D3(foo, bar.toString())
    }
}

class CreatorTest {
    val mapper = jacksonObjectMapper()
    val src = "{\"foo\":1,\"bar\":2}"

    @Test
    fun t1() {
        val expected = D1(1, "2")

        // com.firstxml.jackson.databind.exc.InvalidDefinitionException is thrown.
        assertEquals(expected, mapper.readValue(src))
    }

    @Test
    fun t2() {
        val expected = D2(1, "2")

        // no errors thrown.
        assertEquals(expected, mapper.readValue(src))
    }

    @Test
    fun t3() {
        val expected = D3(1, "2")

        // com.firstxml.jackson.databind.exc.InvalidDefinitionException is thrown.
        assertEquals(expected, mapper.readValue(src))
    }
}

This appears to be caused by a similar cause.

@k163377
Copy link
Contributor Author

k163377 commented Jan 2, 2022

@cowtowncoder Could you please take a look at this question?

@k163377
Copy link
Contributor Author

k163377 commented Jan 3, 2022

@dinomite
Fixing this issue will be easier if #510 can be merged.
I would appreciate it if you could review it if possible.

@cowtowncoder
Copy link
Member

I wish I remembered original thinking on precedence of @JsonCreator for static factory method vs constructor, but I do not remember it. At this point the answer is "whatever jackson-databind for vanilla Java does" -- there is code in there that separately collects candidates for both, and then code that picks up "winner".

I suspect there is at least one unit test that tests this, too, if anyone has time to go and have a look.

@dinomite
Copy link
Member

dinomite commented Jan 4, 2022

@k163377 Looking at #510

@nealeu
Copy link

nealeu commented Jan 11, 2022

Here's another example of this being weird:

interface Value<out T: Any> {
    val raw: T

    @JsonValue
    fun toExternalForm() = raw
}

data class Id(override val raw: String) : Value<String> {
    companion object {
        @JvmStatic
        @JsonCreator
        fun create(raw: String): Id {
            // We never get here. Debugging shows that the constructor is called, but if
            throw IllegalStateException()
        }
    }
}

data class Id2(override val raw: String) : Value<String> {
    companion object {
        @JvmStatic  // but no@JsonCreator
        fun create(raw: String): Id2 {
                return Id2(raw)
        }
    }
}

class JsonTest {

    @Test
    fun `Id doesn't call @JsonCreate method`() {
        val mapper = jacksonObjectMapper()
        val event = Id(UUID.randomUUID().toString())
        val json: String = mapper.writeValueAsString(event)

        assertEquals(json.length, 38)

        assertEquals(event, mapper.readValue(json, Id::class.java))
    }

    @Test
    fun `but Id2 fails without @JsonCreate`() {
        val mapper = jacksonObjectMapper()
        val event = Id2(UUID.randomUUID().toString())
        val json = mapper.writeValueAsString(event)

        val root = mapper.readValue(json, Id2::class.java)

    }
}

@nealeu
Copy link

nealeu commented Jan 20, 2022

Note on the above if it helps anyone.

The following works for Kotlin data classes

    @JvmRecord
    data class Id
        @JsonCreator(mode = DELEGATING)
        constructor(override val raw: String) : Value<String> {

        companion object {
            fun mint() = Id(UUID.randomUUID().toString())
        }
    }

@nealeu
Copy link

nealeu commented Jan 28, 2022

FYI. Despite the above workaround, we've gone a different route for our Kotlin data classes.

We've had some PRs merged into https://github.com/bertilmuth/moonwlker so that it now supports value types like the above data class without any annotations.

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

No branches or pull requests

4 participants