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

Upgrade to Kotlin 1.5.0 #441

Merged
merged 11 commits into from
May 25, 2021
Merged

Upgrade to Kotlin 1.5.0 #441

merged 11 commits into from
May 25, 2021

Conversation

dinomite
Copy link
Member

@dinomite dinomite commented May 7, 2021

No description provided.

@cowtowncoder
Copy link
Member

Sounds good to me? Are there likely downsides?

Also, I wonder if there's any chance we could try to test Kotlin module against 1.4.x and 1.5 in separate project -- assuming both could work? I assume 1.4.x should still work in short term.
More generally it'd be great to figure out effective version ranges, document them in Jackson Kotlin module release notes.

@dinomite
Copy link
Member Author

Kotlin releases are pretty solid…but they have made a few API changes in 1.5.0.

@cowtowncoder How hard is it to cut a snapshot release once I get this fixed up?

@cowtowncoder
Copy link
Member

@dinomite snapshots are easy, just mvn clean deploy from branch.
But our Travis CI/CD setup may even push these automatically, looking at .travis.yml.

@dinomite
Copy link
Member Author

@cowtowncoder Oh, I meant a snapshot published to Sonatype, is that do-able? I'd like that so we can get it used in some real-world cases.

@cowtowncoder
Copy link
Member

Yes, that's what mvn deploy does (as opposed to mvn install). Not sure if you need separate credentials with access... probably yes. So I can do it, and Travis job can (since I created the token being passed).

@dinomite
Copy link
Member Author

Ahh, I think I had removed the .travis.yml thinking it was unused (and maybe having some trouble with Travis? I don't recall). Re-added it and put this branch in the ones allowed to be built do see if that'll make a snapshot build.

@dinomite
Copy link
Member Author

The build is failing because of a binary change—the overidden deserialize() methods in KotlinDeserializers.kt have changed (e.g. from deserialize-w2LRezQ to deserialize-Iymvxus). Those methods are, as I understand it, generated by the Kotlin compiler (hence the random chars postfix) and actually invoked by a bridge method of the interface so they're safe to ignore.

I'll look more into this but would appreciate any ProTips™ others can offer.

@cowtowncoder
Copy link
Member

That looks like a failure due to recently added binary-compatibility checker plug-in. Not sure if that is legit or not -- could be the case where old variant should have been left with @Deprecated, in addition to adding a new variant.

@miensol
Copy link

miensol commented May 12, 2021

We were testing using Kotlin 1.5.0 with Jackson and found an issue related to type reification and TypeReference<T> usage.

In kotlin 1.4.x the following code worked

inline fun <reified T> jsonList(noinline matcher: (List<T>) -> Unit) = json(matcher, object : TypeReference<List<T>>() {})
fun <T : Any> json(matcher: (T) -> Unit, typeReference: TypeReference<T>): ResultMatcher {
      return ResultMatcher {
        val value: T? = mapper.readValue(it.response.contentAsByteArray, typeReference)
        MatcherAssert.assertThat(value, Matchers.notNullValue())
        matcher(value!!)
    }
}

And now it fails on mapper.readValue(it.response.contentAsByteArray, typeReference) with

Unrecognized Type: [null]
java.lang.IllegalArgumentException: Unrecognized Type: [null]
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1266)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromWildcard(TypeFactory.java:1533)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1263)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromParamType(TypeFactory.java:1480)
	at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1250)
	at com.fasterxml.jackson.databind.type.TypeFactory.constructType(TypeFactory.java:670)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3292)

I'm aware that this might not be related to jackson-module-kotlin. I wonder though, does this module use TypeReference<T>?

@cowtowncoder
Copy link
Member

@miensol Reproduction of the issue with TypeReference would be great, as minimal as possible. You are right that it is not necessarily clear where the issue is.

@dinomite
Copy link
Member Author

dinomite commented May 13, 2021

Re: ABI breakage that I mentioned previously—It's because name mangling suffix generation changed in Kotlin 1.4.30 (k-m-k currently uses 1.4.21 and this PR brings it up to 1.5.0).

I think the real solution here is to make the objects in KotlinDeserializers internal so that they aren't part of the public API of j-m-k. I'll do that shortly. This is an ABI breaking change, but those shouldn't be accessed outside of the module anyway.

Thanks to @marshallpierce for helping me look at the bytecode and finding the name mangling change.

@dinomite
Copy link
Member Author

@miensol We have one test that uses a TypeReference and it hasn't complained about the change to Kotlin 1.5.0. Would you write a PR against this branch (or just against 2.13, if that's easier) with a test for the issue you found?

@dinomite
Copy link
Member Author

@efenderbosch Since you did #419 would you check me here and make sure I'm not missing anything? I made all of the objects in KotlinDeserialzers.kt internal in 576fd81 because I don't think there is any reason why anything outside of j-m-k should need to refer to them.

@efenderbosch
Copy link

Actually, it might be a good idea to leave those unsigned number (de)serializers exposed (not internal). Otherwise people will likely have to write their own that's basically identical. Unfortunately, those (de)serializers only work in isolation, if a POKO has a UInt member, then you still need some sort of ctor/JsonCreator/Deserializer for that POKO that will take an Int and parse it as a UInt correctly.

@dinomite
Copy link
Member Author

@efenderbosch Ahh, ok, I feel like we might have talked about this at the time of that PR. That usage sounds pretty esoteric—do you think it'd be ok to leave them public, but not enforce API strictness to them (that is, they're public but we don't make guarantees that they'll be stable)?

@dinomite dinomite marked this pull request as ready for review May 20, 2021 18:07
@dinomite
Copy link
Member Author

Barring any complaints I'm going to merge this and make Jackson 2.13+ use Kotlin 1.5.0.

@dinomite dinomite merged commit 92b7816 into 2.13 May 25, 2021
@dinomite dinomite deleted the kotlin-1.5.0 branch May 25, 2021 09:43
@cowtowncoder
Copy link
Member

Cool! I am trying to speed up release process of 2.13 by dropping plans for more major features (... property introspection rewrite in particular).

@miensol
Copy link

miensol commented May 30, 2021

@miensol Reproduction of the issue with TypeReference would be great, as minimal as possible. You are right that it is not necessarily clear where the issue is.

I've prepared one here https://github.com/FasterXML/jackson-module-kotlin

So far it seems like a change in Kotlin.

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

Successfully merging this pull request may close these issues.

4 participants