search

FasterXML / jackson-module-kotlin

Module that adds support for serialization/deserialization of Kotlin (http://kotlinlang.org) classes and data classes.
Apache License 2.0
1.12k stars 175 forks source link

Remove `kotlin-reflect` and replace it with `kotlinx-metadata-jvm` #450

Open k163377 opened 3 years ago

k163377 commented 3 years ago

Use case

It makes it easier to use jackson-module-kotlin in resource-limited environments.

Describe the solution you'd like

Remove kotlin-reflect and replace it with kotlinx-metadata-jvm.

Additional context

The reason for creating this issue is that, as mentioned in this spring-framework issue, kotlin-reflect consumes a lot of resources, and there is a topic to replace it with kotlinx-metadata-jvm.

I've started prototyping it, and I'd be happy to discuss how to implement it in this issue if it seems to work.

Also, if you are interested in this change, please send me a :+1: (I would personally like to know if there is any demand for this change).

dinomite commented 3 years ago

Interesting. This would mean that all reflection would be gone and Jackson's work would happen purely at compile time? that doesn't sound like it would work with databind, would this alter just the Kotlin module?

Mainly, I want to know more, this is new to me.

k163377 commented 3 years ago

The kotlinx-metadata-jvm only provides the ability to read information from Metadata annotations. The rest of the processing will be done via Java Reflection.

This means that only the Kotlin module needs to be modified. Also, not all decisions are made at compile time, as in code generation.

dinomite commented 3 years ago

Got it, so this would reduce the amount of reflection required when dealing with Kotlin objects?

k163377 commented 3 years ago

this would reduce the amount of reflection required when dealing with Kotlin objects?

I don't have a clear comparison, but it probably is.

I think the explanation of moshi-metadata-reflect is easy to understand. https://github.com/ZacSweers/MoshiX/tree/main/moshi-metadata-reflect#motivation

cowtowncoder commented 3 years ago

I don't know this well enough to give much input but explanation for moshi-metadata does sound compelling. Main question would just be how likely this would be to cause issues; would there be enough time to prove safety of the new approach for, say, 2.13 (definitely should not be introduced in a patch release).

I assume that trying to allow either one dynamically would not make much sense (since kotlin-reflect would still be added as a dependency). ... but holy mackerel, is that jar REALLY 2.8 MEGS in size?!?! (see https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-reflect/1.5.10). If so, I can definitely see why it'd be good to be rid of.

An alternative for solely reducing jar size could be to use shading to include parts needed here. But if wholesale replacement is possible, sure, why not?

k163377 commented 3 years ago

Main question would just be how likely this would be to cause issues; would there be enough time to prove safety of the new approach for, say, 2.13

I don't think it's possible to implement this in 2.13. First, the changes to complete this are throughout the module, so it will probably take a while to test it. Also, if call functions with default arguments without kotlin-reflect, that need to include #439 as a prerequisite.

is that jar REALLY 2.8 MEGS in size?!?!

I'm also surprised. Why is it so heavy? And it's heavier than version 1.3.x...

dinomite commented 3 years ago

I'll take another look at #439 this week…

sdeleuze commented 3 years ago

I am very interested if Jackson could stop depending on kotlin-reflect, that would allow me to do the same on Spring side.

cowtowncoder commented 3 years ago

Note that although I hope to speed up progress towards 2.13, that is not to say there isn't time to do bigger things just due to that timeline. I try to accommodate release timings to get in important useful fixes, if a bit of extra time is needed. But at least it would be great to get this in 2.x series (as opposed to only Jackson 3.0).

k163377 commented 3 years ago

@dinomite @cowtowncoder As a preliminary step to working on this change, I am planning to do some refactoring regarding readability and performance.

I apologize for the many reviews I have already asked for. I always appreciate your reviews.

k163377 commented 1 year ago

The project that solved this problem is now available. If you are interested, please give it a try. https://github.com/ProjectMapK/jackson-module-kogera

sdeleuze commented 1 year ago

Very interested on this since we would like to migrate Spring as well. I will give it a try on Spring side and provide a feedback.

cowtowncoder commented 1 year ago

As per my note on Spring issue: I would be interested in this. Perhaps this could proceed for Jackson 2.15? Do you think this could be done with a simple (?) PR @k163377 here? What would be the trade-offs wrt backwards compatibility?

/cc @dinomite @Spikhalskiy @viartemev

k163377 commented 1 year ago

@cowtowncoder

Do you think this could be done with a simple (?) PR

I think it would be a very complicated PR.

First of all, not only are there many changes to be made, but there are also some things (especially around error messages) that have not been done because they are too time-consuming to implement. I intend to prioritize adding/fixing features related to value class in the development of kogera, and will work on the these migrations later.

Secondly, I have not confirmed that there are no unintended destructive changes in kogera, and I don't think we have an agreement on what would be sufficient to confirm. If we migrate at 2.15.x, I think there is a risk of unintentionally breaking the behavior.

Finally, as I mentioned in my comment here, if we are only considering Spring, the migration timing should be after the Spring work. As for individual migration, I think there is no problem if you use kogera.

In conclusion, I personally feel that it is better to wait until 2.16.x or later for the migration process.

cowtowncoder commented 1 year ago

@k163377 that sounds reasonable.

Looking at Kogera I like the idea of trying out how it works, and perhaps (but only perhaps) its ideas could eventually replace current jackson-module-kotlin.

I have only one concern. README states:

Since the package/module name of jackson-module-kogera is the same as that of jackson-module-kotlin,...

I think it is a bad idea to do either of these, and I would suggest you change this; especially module name. It may make things slightly easier for migration but can cause major issues with transitive dependencies: for that reason I suggest you use unique Java package name and module name. This will save users with complicated dependency set from lots of unnecessary pain. Even if they need to change (at least) one import statement and use different module dependency.

sdeleuze commented 1 year ago

2.16.x looks like a good target and let us the time to experiment on Spring side.

k163377 commented 1 year ago

I think it is a bad idea to do either of these, and I would suggest you change this; especially module name.

Sorry, I forgot to reply. I will fix this when I move to the beta version.

cowtowncoder commented 1 year ago

Great @k163377! This sounds like a good new module and lets you experiment much faster than with existing Jackson Kotlin module. Plus change behavior in incompatible ways where it makes sense too (fix problems)

k163377 commented 1 year ago

I apologize for the delay in responding due to my busy schedule. I have released a version with corrected package names. https://github.com/ProjectMapK/jackson-module-kogera/releases/tag/2.15.2-beta0