Skip to content

Migrate legacy reflection properties to use MethodHandles, take 2 #5090

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

Open
wants to merge 19 commits into
base: 3.x
Choose a base branch
from

Conversation

stevenschlansker
Copy link

I'm pretty sure I figured out the main reason the previous attempt (#5046) did not work - in order to read modules from databind, we still have to call setAccessible(true) which was happening too late due to the virtual method e.g. BeanPropertyWriter.fixAccess happening after constructor finishes.

Work around this by using a lazy memoized Setter class. Also ends up helping to simplify some serialization wonkiness.

Fixes #2083

@stevenschlansker stevenschlansker force-pushed the methodhandle-hacking branch 3 times, most recently from fb6aa3f to 9d9b7d7 Compare April 11, 2025 04:25
@stevenschlansker
Copy link
Author

Sorry the first attempt did not go so well, but here is try #2. Instead of changing any of the rules around set accessible, defer using the method / constructors / etc until first-use.
It's unfortunate that the fixAccess / setAccessible is done after the constructor finishes...

I've tested this works with jackson-dataformat-xml jackson-jakarta-rs-providers. Working on one test failure so far in jackson-modules-base still.

@stevenschlansker
Copy link
Author

Ok, with FasterXML/jackson-modules-base#295, jackson-modules-base builds too :)

@stevenschlansker stevenschlansker force-pushed the methodhandle-hacking branch 5 times, most recently from c10f758 to 2b57c71 Compare April 11, 2025 14:59
@cowtowncoder
Copy link
Member

@stevenschlansker I think I will merge this post rc3 (which I hope to release maybe this weekend).
May be able to merge smaller pieces first, will see if some changes could be applied separately.

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 12, 2025

Ok made small change (Exception arg to Throwable) into 3.x, merged to this PR.

@stevenschlansker
Copy link
Author

Thanks - please let me know if there's any additional changes needed or validation I should perform.

@stevenschlansker
Copy link
Author

@cowtowncoder , while working on this, I ran into the BeanPropertyWriter.fixAccess and AnnotatedMember.fixAccess which is unfortunate in that it mutates state after the constructor completes. I am not sure if it is feasible, but it would be nice if we could push that decision up to the constructor - then more state can be final and we could get rid of the lazy memoized setter, for example

@cowtowncoder
Copy link
Member

@stevenschlansker Sounds like a good idea at high level. Unfortunately I don't have more context on this (as in, haven't worked in those aspects lately) so say much about feasibility.

I'll try to get #5094 in tonight, to be done with bigger messing up of introspection, to stabilize things.
(#5093 and #5094 are meant to allow reducing introspection but realized that on short term they won't... but lay some foundation for reduction as next steps. Not directly related to your suggestion, just a sidenote on concurrent changes in related area).

@cowtowncoder
Copy link
Member

@stevenschlansker Not sure how much (if at all) this helps, but with #5143 I removed java.io.Serializable from SettableBeanProperty -- so I think UnreflectHandleSupplier and SetterHolder no longer need to be JDK serializable either.

What else do we need to help with issue you mentioned wrt BeanPropertyWriter.fixAccess()?

_throwAsJacksonE(ctxt.getParser(), e, value);
return null;
}
}

class SetterHolder extends UnreflectHandleSupplier {
private static final long serialVersionUID = 1L;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost certainly no longer needed (I removed most implements java.io.Serializable cases in #5143)

@cowtowncoder
Copy link
Member

@stevenschlansker Hoping to merge this in right before 3.0.0-rc4 -- anything you want to change before merging? (we can still do follow-ups of course)

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.

2 participants