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

Kotlin 1.4 support #356

Closed
efenderbosch opened this issue Aug 18, 2020 · 15 comments
Closed

Kotlin 1.4 support #356

efenderbosch opened this issue Aug 18, 2020 · 15 comments
Milestone

Comments

@efenderbosch
Copy link

Maybe this can be just a meta-issue to collect issues w/ Kotlin 1.4.

First thing I noticed is that inline classes no longer serialize properly.

w/ Kotlin 1.3:

{
  "type" : "SHOPPING_CART",
  "url" : "http://example.com"
}

w/ Kotlin 1.4:

{
  "type" : "SHOPPING_CART",
  "url-6j4v_q1g" : "http://example.com"
}
@daviddenton
Copy link
Contributor

Is there a plan around Kotlin 1.4.0 support? Will it be released as part of 2.12 or will it be held off to v3?

I've speculatively upgraded on the 2.12 branch and have created 357 . All the tests pass on that PR, so it looks like there is at maybe some missing coverage around the issue mentioned by @efenderbosch above?

@efenderbosch
Copy link
Author

efenderbosch commented Aug 18, 2020

I did some digging and I think that KotlinNamesAnnotationIntrospector::findImplicitPropertyName needs another branch to handle these mangled getter names.

if (member is AnnotatedMethod) {
    // pseudo code
    if (member.name is like "getFooBar-junk") {
        find fooBar member on the Kotlin class
        if the member's Kotlin class has "box-impl" or "unbox-impl" methods, then it is an inline class
        if it is an inline class, then return "fooBar"
    }
}

@efenderbosch
Copy link
Author

efenderbosch commented Aug 18, 2020

If there is missing coverage, this simple test might be enough.

class InlineClassTest {

    @Test
    fun test() {
        val mapper = ObjectMapper().registerModule(KotlinModule())
        val original = SomeOtherClass(MyInlineClass("bar"))
        val json = mapper.writeValueAsString(original)
        val deserialized = mapper.readValue(json, SomeOtherClass::class.java)
        assertThat(deserialized, equalTo(original))
    }
}

inline class MyInlineClass(val inner: String)

@JsonDeserialize(builder = SomeOtherClass.JacksonBuilder::class)
data class SomeOtherClass(val fooBar: MyInlineClass) {
    data class JacksonBuilder private constructor(val fooBar: String) {
        fun build() = SomeOtherClass(MyInlineClass(fooBar))
    }
}

Passes w/ Kotlin 1.3. Currently fails w/ Kotlin 1.4 with a MissingKotlinParameterException since it expects {"fooBar":"bar"} but finds {"fooBar-dSSZTOk":"bar"}

@cowtowncoder
Copy link
Member

Since Jackson 3.x is still ways out (no good idea of when focus should move back), I think that support for 2.12 would make sense (or 2.13, there will be minor 2.x versions past 2.12).

@andmoos
Copy link

andmoos commented Aug 24, 2020

Having hit this issue today myself I found that as workaround @JvmField annotations effectively disable the mangling.

@eugenio1590
Copy link

any progress here?

@dinomite
Copy link
Member

I created a kotlin-1.4 branch and this PR (I cherry picked @daviddenton's commits because I was stymied by Git) #374

@dinomite
Copy link
Member

@efenderbosch I added a (failing) test for that issue to the branch.

@dinomite
Copy link
Member

Well, it should be failing. Looks like I have more messing around to do with CircleCI

@dinomite
Copy link
Member

Ok, failing now; I wasn't following Maven Surefire's redundancy rules

@dinomite
Copy link
Member

I think we should move ahead with updating the module to Kotlin 1.4.10 in spite of this bug. There is a test for this issue on the kotlin-1.4 branch which is help to anyone who would like to give a shot at fixing this bug.

elektro-wolle added a commit to elektro-wolle/jackson-module-kotlin that referenced this issue Oct 12, 2020
@elektro-wolle
Copy link
Contributor

I've added simple demangling code via the above commit. Not sure, if this is enough (but all tests are green).

@cowtowncoder
Copy link
Member

One quick note: I released 2.12.0-rc1 yesterday. Will probably take at least over end of October to find out regressions, so final 2.12.0 will not go out before November 2020. But if this change is to be done for 2.12, would be good to coordinate.
(not saying it has to, could go in 2.13, but definitely sounds like something to do in a minor release, not patch, and timing of 2.12 seems plausible).

@dinomite dinomite mentioned this issue Oct 13, 2020
dinomite added a commit that referenced this issue Oct 13, 2020
dinomite pushed a commit that referenced this issue Oct 13, 2020
dinomite added a commit that referenced this issue Oct 13, 2020
@dinomite
Copy link
Member

@elektro-wolle I used your commit in this PR #383

@dinomite dinomite added this to the 2.12.0 milestone Oct 13, 2020
@dinomite
Copy link
Member

The 2.12 branch is now on Kotlin 1.4.10. Any new problems can be filed as new issues.

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

No branches or pull requests

7 participants