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.11k stars 175 forks source link

Kotlin `master` (3.0) test failures -- due to `MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS` default change; `SORT_PROPERTIES_ALPHABETICALLY` #807

Closed cowtowncoder closed 1 month ago

cowtowncoder commented 1 month ago

So, Kotlin module unit tests on master fail -- now that we have dependent builds, builds are triggered on jackson-databind changes. Looking at fail message, I think it has same root cause as Scala fails:

https://github.com/FasterXML/jackson-module-scala/issues/678

and similar solutions (either change tests not to rely on forcing setting of final fields; or change MapperFeature setting used by mapper).

cowtowncoder commented 1 month ago

cc @k163377 @JooHyukKim I hope above is enough to solve issues.

JooHyukKim commented 1 month ago

Changing MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS default fixes the issue (tested locally).

If I try connecting the dots, we have two options.

~~- Quick and easy solution : just flip ALLOW_FINAL_FIELDS_AS_MUTATORS on Kotlin-module side, suffer long term. ~~ - Possible concern, users who use ObjectMapper for both java and Kotlin classes should also know about it? - More-work-needed solution : Make #805 work (if I understood correctly, would solve this issue)

cowtowncoder commented 1 month ago

I don't think #805 matters here: the problem is that some tests were only working if Jackson is allowed to force (Via reflection) setting of final field values (isn't it interesting this ever works?! It is likely required for JDK serialization functionality).

So I think tests in question are likely wrong: jackson-databind had one or two as well, but in those cases it was unintentional.

By more work I meant investigating tests themselves to see why they need this setting, and if the tests might be buggy, in a way. Not so much that more work is needed on non-test code.

I don't think users really should enable this MapperFeature, fwtw; it will probably not work in future with later JDKs. So turning it off just exposes possibly bad unit tests.

JooHyukKim commented 1 month ago

(either change tests not to rely on forcing setting of final fields; or change MapperFeature setting used by mapper).

Ah okay, I understand now what it meant 👍🏼

cowtowncoder commented 1 month ago

Yea it's, an odd setting. Had to be added because code was already setting final fields without intending (I had not filtered out final fields :) -- someone pointed this out to me, thinking it was a Feature... not a bug ! )

cowtowncoder commented 1 month ago

Noting that MapperFeature.SORT_PROPERTIES_ALPHABETICALLY default change contributes to fails as well.

JooHyukKim commented 1 month ago

I think we could probably go ahead and fix the tests via changing configuration defaults? @cowtowncoder WDYT?

cowtowncoder commented 1 month ago

@JooHyukKim yes, I think that'd be the simplest immediate solution.

JooHyukKim commented 1 month ago

filed #808.

cowtowncoder commented 4 weeks ago

Note: we are almost good, but there's now #813 for Kotlin 2.0 issues of some kind wrt master branch.