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

Broken Kotlin 1.4 support in 2.13.2 #556

Closed
Spikhalskiy opened this issue Mar 28, 2022 · 20 comments · Fixed by #557
Closed

Broken Kotlin 1.4 support in 2.13.2 #556

Spikhalskiy opened this issue Mar 28, 2022 · 20 comments · Fixed by #557
Labels
Milestone

Comments

@Spikhalskiy
Copy link
Contributor

Spikhalskiy commented Mar 28, 2022

Actual

Using Kotlin 1.4.32 and Jackson 2.13.2.20220324 with jackson-module-kotlin I'm getting an exception

Caused by: java.lang.NoSuchMethodError: 'boolean kotlin.reflect.KClass.isValue()'
	at com.fasterxml.jackson.module.kotlin.KotlinAnnotationIntrospector.findSerializer(KotlinAnnotationIntrospector.kt:88)
	at com.fasterxml.jackson.module.kotlin.KotlinAnnotationIntrospector.findSerializer(KotlinAnnotationIntrospector.kt:26)
	at com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair.findSerializer(AnnotationIntrospectorPair.java:334)
	at com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair.findSerializer(AnnotationIntrospectorPair.java:334)
	at com.fasterxml.jackson.databind.ser.BasicSerializerFactory.findSerializerFromAnnotation(BasicSerializerFactory.java:540)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory._constructWriter(BeanSerializerFactory.java:862)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.findBeanProperties(BeanSerializerFactory.java:630)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.constructBeanOrAddOnSerializer(BeanSerializerFactory.java:401)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.findBeanOrAddOnSerializer(BeanSerializerFactory.java:294)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory._createSerializer2(BeanSerializerFactory.java:239)
	at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.createSerializer(BeanSerializerFactory.java:173)
	at com.fasterxml.jackson.databind.SerializerProvider._createUntypedSerializer(SerializerProvider.java:1495)
	at com.fasterxml.jackson.databind.SerializerProvider._createAndCacheUntypedSerializer(SerializerProvider.java:1443)
	at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:544)
	at com.fasterxml.jackson.databind.SerializerProvider.findTypedValueSerializer(SerializerProvider.java:822)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:308)
	at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4568)
	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsBytes(ObjectMapper.java:3844)

The same code works fine with Jackson 2.13.1

The regression is introduced in #527
kotlin.reflect.KClass.isValue() was added in Kotlin 1.5 and not available in 1.4: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.reflect/-k-class/is-value.html

Expected

  • get Kotlin 1.4 support back (the last release of 1.4 was not that long time ago to consider it end of life https://github.com/JetBrains/kotlin/releases/tag/v1.4.32) or
  • explicitly state which versions of kotlin are supported by this module and that Kotlin 1.4 got dropped in 2.13.2. Jackson Kotlin Module's release notes are stating: "No changes since 2.13.1?" for 2.13.2.
@cowtowncoder
Copy link
Member

Hmmmh. I wonder if the expected baseline is documented anywhere? 2.13(.2) seems to depend on Kotlin 1.5.30.

I do agree that baseline changes should not occur in patches, just trying to figure out what the baseline is expected to be: and suggest adding explicit definition.

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Mar 29, 2022

No, there is no official published end-of-life schedule for Kotlin versions.

For our project, I decided that if the minor version got an update in the last two years - it's still alive. But it's just our own made-up criteria. 1.4 is still alive according to it, 1.3 is not.
Another criteria/clue that may be used is that Kotlin 1.5 (and 1.6) gradle plugins got fixes to work correctly on Java 16 while 1.4 never did.

But it's definitely a good idea to establish a watermark and test against minor versions above this watermark.
For example, we test Temporal Java/Kotlin SDK against 1.4, 1.5 and 1.6 separately. And especially because some internals change between versions, kotlin reflection code has to have kotlin version condition checks sometimes. So it needs to be tested that our reflection code doesn't work only on 1.5/1.6.

@cowtowncoder
Copy link
Member

While I am not the maintainer of this module, I think a PR to make 2.13(.3) work again would be accepted if (but only if) it could be done without reverting fix/change of #527.
I don't know if this is possible or not.

@Spikhalskiy
Copy link
Contributor Author

Well, there are no value classes in Kotlin 1.4. So, the check for isValue() should be just replaced to (isKotlin15OrHigher() && isValue()) and we should be fine without breaking the new functionality for value classes.

@cowtowncoder
Copy link
Member

Ok. Then a PR would be acceptable I think. I don't think I'll be able to do that but could probably code review and merge it.

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label Mar 29, 2022
@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Mar 29, 2022

It also looks like the project builds against 1.6.10 now in 2.14. It may be not the best approach. Kotlin mostly guarantees binary compatibility with runtimes one minor version lower than the kotlin version that was used for the build. So, the 2.14 branch being released today will be fine on 1.6.x, should be fine on 1.5.x, but no guarantees on 1.4.x.

See https://kotlinlang.org/docs/kotlin-evolution.html#evolving-the-binary-format for reference.

All binaries are backwards compatible, i.e. a newer compiler can read older binaries (e.g. 1.3 understands 1.0 through 1.2),

Older compilers reject binaries that rely on new features (e.g. a 1.0 compiler rejects binaries that use coroutines).

Preferably (but we can't guarantee it), the binary format is mostly forwards compatible with the next feature release, but not later ones (in the cases when new features are not used, e.g. 1.3 can understand most binaries from 1.4, but not 1.5).

It's a good idea to use the lowest version of Kotlin that is actually needed for the project. It looks like for this module right now it's 1.5.x. I was able to downgrade 2.14 branch to Kotlin 1.5.32 mostly successfully (with one failed TestJacksonWithKotlin.findingFactoryMethod3 test).

It would be great if this project may be migrated to build on 1.5.32 for the sake of binary backward compatibility.

@cowtowncoder
Copy link
Member

/cc @dinomite

@k163377
Copy link
Contributor

k163377 commented Mar 31, 2022

@cowtowncoder @Spikhalskiy

official published end-of-life schedule for Kotlin versions

Unless there is a policy change, the three major versions (i.e., down to 1.4 at this time) will be supported from JetBrains.
The rationale is the following reference

To give you more time for migration, we support the development for three previous language and API versions in addition to the latest stable one.
https://kotlinlang.org/docs/kotlin-evolution.html#compatibility-flags

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented May 17, 2022

Are there any plans for fixing this regression with Kotlin 1.4? We have to stick to 1.13.1 because of this.

@cowtowncoder
Copy link
Member

Alas, looks like both Kotlin maintainers are inactive at this point in time. :-(

Is there a PR that would fix this, for 2.13? Unfortunately, too, I just released 2.13.3 so would probably need a micro-patch for Kotlin (2.13.3.1) to make it quickly available.

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented May 22, 2022

It's been broken for 2 months already, we can wait for 2.13.4 or 2.14.0 just fine, no rush. The problem is that nobody with deep Kotlin background can really make a code review for my patch looks like...
Let me make sure that it at least actually works with Kotlin 1.4 with this patch, I will ping you in the PR.

@cowtowncoder
Copy link
Member

Sure, but I do not mind pushing 2.13.3.1 relatively soon to resolve the issue.
Apologies for slow follow up here; I'll have a look at the patch (I saw the other notification).

Also: assuming things go ok, would you have time and interest to become a maintainer? I feel that this would be helpful for the project, but I understand that you may have other commitments.
And regardless we should have multiple people to have effective code reviews. But one is better than none :)

@cowtowncoder cowtowncoder removed the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label May 23, 2022
@cowtowncoder cowtowncoder added this to the 2.13.4 milestone May 23, 2022
@cowtowncoder
Copy link
Member

Fixed via #557 -- backported in 2.13 for 2.13.4 or (if such micro-patch released), 2.13.3.1.

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented May 23, 2022

@cowtowncoder

assuming things go ok, would you have time and interest to become a maintainer?

I can help to keep this module alive and the community PRs moving for essential things. Especially because users of our project (Temporal) use Kotlin a lot and we rely on Jackson, it's important for us to see this module in a healthy state :)
This project has quite a bit of stale PRs with a lot of work done inside...
But I don't think I have the bandwidth to actively develop it myself.

@cowtowncoder
Copy link
Member

No problem; there is no real minimum definition for what co-author/maintainer would have to do.
To me keeping things moving, reviewing, merging things, helping is plenty.
So there is no expectation that -- should you choose to accept the nomination :) -- you would have to actively develop new features, for example.

@Spikhalskiy
Copy link
Contributor Author

@cowtowncoder Sure, sign me in, and let's keep it alive ;)

@cowtowncoder
Copy link
Member

Sounds good!

@IgnatBeresnev
Copy link

IgnatBeresnev commented Jun 10, 2022

Hi! Sorry for bumping this.

Is there a planned release date for 2.13.4, 2.13.3.1 or whatever the closest version with this fix is? :)

We updated our library to latest to get rid of jackson-databind#2816, but we can't drop support for 1.4 just yet, otherwise we might have problems with Gradle 6

Asking because we'll wait if it's around the corner, but we'll have to downgrade to 2.12 it if it's not yet planned :(

Update: it looks like #523 doesn't support 1.4 also, so fixing this issue doesn't really solve 1.4 support problem completely

@cowtowncoder
Copy link
Member

No firm plans for 2.13.4 release (it will take a while), due to there being few issues fixed.

If there is desire to get 2.13.3.1 out I could be persuaded to do that -- if so, please let me know. But I do not want to release something that would not help you (wrt #523) being incomplete.

@blackjets
Copy link

blackjets commented Jun 14, 2022

Hi there, the same trouble with jackson 2.13.3
Caused by: java.lang.NoSuchMethodError: 'boolean kotlin.reflect.KClass.isValue()'
at com.fasterxml.jackson.module.kotlin.KotlinAnnotationIntrospector.findSerializer(KotlinAnnotationIntrospector.kt:88)
at com.fasterxml.jackson.module.kotlin.KotlinAnnotationIntrospector.findSerializer(KotlinAnnotationIntrospector.kt:26)
at com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair.findSerializer(AnnotationIntrospectorPair.java:334)
at com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair.findSerializer(AnnotationIntrospectorPair.java:334)
at com.fasterxml.jackson.databind.ser.BasicSerializerFactory.findSerializerFromAnnotation(BasicSerializerFactory.java:540)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory._constructWriter(BeanSerializerFactory.java:862)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.findBeanProperties(BeanSerializerFactory.java:630)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.constructBeanOrAddOnSerializer(BeanSerializerFactory.java:401)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.findBeanOrAddOnSerializer(BeanSerializerFactory.java:294)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory._createSerializer2(BeanSerializerFactory.java:239)
at com.fasterxml.jackson.databind.ser.BeanSerializerFactory.createSerializer(BeanSerializerFactory.java:173)
at com.fasterxml.jackson.databind.SerializerProvider._createUntypedSerializer(SerializerProvider.java:1495)
at com.fasterxml.jackson.databind.SerializerProvider._createAndCacheUntypedSerializer(SerializerProvider.java:1443)
at com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:544)
at com.fasterxml.jackson.databind.SerializerProvider.findTypedValueSerializer(SerializerProvider.java:822)
at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:308)
at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4568)
at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3821)

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

Successfully merging a pull request may close this issue.

5 participants