-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Only avoid Records fields detection for deserialization #3894
Only avoid Records fields detection for deserialization #3894
Conversation
src/main/java/com/fasterxml/jackson/databind/introspect/POJOPropertiesCollector.java
Show resolved
Hide resolved
@@ -440,7 +440,7 @@ protected void collectAll() | |||
|
|||
// 15-Jan-2023, tatu: [databind#3736] Let's avoid detecting fields of Records | |||
// altogether (unless we find a good reason to detect them) | |||
if (!isRecordType()) { | |||
if (!isRecordType() || _forSerialization) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. I don't think this should be done, even if it handles this particular problem, it seems like a hack...
But let me think about this bit more.
I guess the idea is that doing this would "pull in" getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you @yihtserns -- it's not so much for pulling in getters, but for alternate visibility.
I think this is necessary, then. I am bit worried about possible side-effects of doing this (wrt forcing access to Field
s but since it's only for serialization perhaps it's not a problem).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the problem in #3897 taken into account, or affected by this PR? Hope not 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've did test this against that issue, the other day - apparently they're unrelated. 😮💨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've did test this against that issue,
@yihtserns Great! ✌🏼✌🏼
If you have the test code used, maybe we can merge it to 2.15, under failing?
If you don't have time for it, you can just share it here and I can include it in #3899
As per my comment I feel uneasy about exposing fields for Records. It seems to me that the issue here wrt usage is that Records DO NOT use Bean getter/setter notion, and users should follow that naming notation. So... I don't know. It's a confusion situation. I really wish JDK team hadn't switched from Bean convention since this is a major hassle -- and has been for years, actually. I understand it's bit more typing and all but for compatibility leaving getters as accessors would have been so much better for everyone involved. |
fdee4a0
to
6777448
Compare
Updated the PR 1st comment with the 3 currently known use cases.
|
I think I have the problem with a slightly different use case, in case that helps. class Foo(val bar: Int) {
val isZero get() = bar == 0
} We serialize these classes with |
@fleiber unless that Kotlin class actually produces Record classes, then they're unrelated - you'd need to open a new issue for that use case. |
Ok let's do this: we can still re-consider if necessary before 2.15.1 release, but for now this seems sensible. |
Refines #3737 - the reason why the fields are needed is because this kind of config needs it:
And currently, we have 3 use cases that uses that config:
getXXX
methods.For the 2nd use case, there's an alternative solution. Not so lucky for 1st & 3rd use cases.