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

Only avoid Records fields detection for deserialization #3894

Merged
merged 1 commit into from
May 3, 2023

Conversation

yihtserns
Copy link
Contributor

@yihtserns yihtserns commented Apr 24, 2023

Refines #3737 - the reason why the fields are needed is because this kind of config needs it:

new ObjectMapper()
  .setVisibility(PropertyAccessor.GETTER, Visibility.NONE)
  .setVisibility(PropertyAccessor.FIELD, Visibility.ANY)

And currently, we have 3 use cases that uses that config:

Who Why
1 #3628 Easy way to ignore methods (that looks like a property) that comes from interface(s), and only serializing Record components.
2 #3724 (comment) Easy way (albeit overkill) way to exclude a method (that looks like a property) from being serialized, to avoid making test assertion complicated.
3 #3895, #3906 (comment) A way to ignore non-accessor getXXX methods.

For the 2nd use case, there's an alternative solution. Not so lucky for 1st & 3rd use cases.

@@ -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) {
Copy link
Member

@cowtowncoder cowtowncoder Apr 24, 2023

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.

Copy link
Member

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 Fields but since it's only for serialization perhaps it's not a problem).

Copy link
Member

@JooHyukKim JooHyukKim Apr 27, 2023

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 🥲

Copy link
Contributor Author

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. 😮‍💨

Copy link
Member

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

@cowtowncoder
Copy link
Member

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.
But then again I don't think added accesors would be found.

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.

@yihtserns yihtserns marked this pull request as ready for review April 26, 2023 17:05
@yihtserns
Copy link
Contributor Author

yihtserns commented Apr 27, 2023

Updated the PR 1st comment with the 3 currently known use cases.

  • The 2nd has alternative solution, so it can be ignored.
  • The 1st can be achieved via custom VisibilityChecker via custom AnnotationIntrospector - albeit quite a lot of code to write.
  • The 3rd has no alternative solution, and it seems to be a legit need.

@fleiber
Copy link

fleiber commented May 1, 2023

I think I have the problem with a slightly different use case, in case that helps.
The code is in Kotlin, where we have classes looking like:

class Foo(val bar: Int) {
      val isZero get() = bar == 0
}

We serialize these classes with setVisibility(PropertyAccessor.IS_GETTER, Visibility.NONE) to ignore the isXXX methods, and please note that we cannot use @JsonIgnore ("This annotation is not applicable to target 'member property without backing field or delegate'").
Starting with Jackson 2.15.0, the isZero property is serialized, then the deserialization fails.
The way we use the visibility feature looks fine to me, so I would say the 2.15 change does look like a regression.

@yihtserns
Copy link
Contributor Author

@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.

@fleiber
Copy link

fleiber commented May 1, 2023

@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.

Indeed, I missed the "record" part in the title... Sorry, let me create a new issue.
Edit: #3904

@cowtowncoder
Copy link
Member

Ok let's do this: we can still re-consider if necessary before 2.15.1 release, but for now this seems sensible.

@cowtowncoder cowtowncoder merged commit 329944a into FasterXML:2.15 May 3, 2023
@cowtowncoder cowtowncoder changed the title Only avoid Records fields detection for deserialization. Only avoid Records fields detection for deserialization May 3, 2023
cowtowncoder added a commit that referenced this pull request May 3, 2023
@cowtowncoder cowtowncoder added this to the 2.15.1 milestone May 3, 2023
@yihtserns yihtserns deleted the records-fields-4-serialize branch July 17, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants