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 master (3.0) test failures -- due to MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS default change; SORT_PROPERTIES_ALPHABETICALLY #807

Closed
cowtowncoder opened this issue Jun 15, 2024 · 10 comments
Labels
3.x Issue affecting/planned for Jackson 3.x

Comments

@cowtowncoder
Copy link
Member

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:

FasterXML/jackson-module-scala#678

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

@cowtowncoder cowtowncoder added the 3.x Issue affecting/planned for Jackson 3.x label Jun 15, 2024
@cowtowncoder
Copy link
Member Author

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

@JooHyukKim
Copy link
Member

JooHyukKim commented Jun 16, 2024

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
Copy link
Member Author

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
Copy link
Member

(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
Copy link
Member Author

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 cowtowncoder changed the title Kotlin master (3.0) test failures -- probably due to MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS default change Kotlin master (3.0) test failures -- due to MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS default change; SORT_PROPERTIES_ALPHABETICALLY Jun 18, 2024
@cowtowncoder
Copy link
Member Author

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

@JooHyukKim
Copy link
Member

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

@cowtowncoder
Copy link
Member Author

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

@JooHyukKim
Copy link
Member

filed #808.

cowtowncoder added a commit that referenced this issue Jun 22, 2024
@cowtowncoder
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issue affecting/planned for Jackson 3.x
Projects
None yet
Development

No branches or pull requests

2 participants