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

Refactor Bean property introspection #4532

Merged
merged 27 commits into from
May 18, 2024

Conversation

cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented May 17, 2024

This PR is for (roughly) first half of work for #4515.

What it does is rewrite Creator (constructor, factory-method) introspection to happen mostly in POJOPropertiesCollector (early, along with Field/Getter/Setter introspection); and rely less on BasicDeserializerFactory handling which occurs much later on in the process.

But this first PR does not eliminate redundancy yet, leaving BasicDeserializerFactory to do sort of double processing; old methods in POJOPropertiesCollector are left (mostly) as-is as well (but they will be removed ASAP after merging to 3.0).

@cowtowncoder
Copy link
Member Author

Some progress has been made, rewriting logic of POJOPropertiesCollector; leaving about 12 failures, not including Record-tests (more work for Records).

PropertyName implName = ctor.implicitName(i);
final boolean hasExplicit = (explName != null);

if (!hasExplicit) {
Copy link
Member

@JooHyukKim JooHyukKim May 18, 2024

Choose a reason for hiding this comment

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

This may be personal preference.

From here to line 766 seems like we can just end everything with...

if  explicit do explcit handling
else do implicit 

And not mix up two different handling. So that is easier to make changes per case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I am sure there are ways to clean things up: at first I am trying to replicate existing logic. So once things pass will focus more on clean up.

Exciting thing is that right now I am 1 test fail away from passing all Java 8 tests -- then need to tackle record handling.

And once that happens, then start making use of Creators detected: right now all I have changed is finding properties-based Creators to link properties in POJOPropertiesCollector, but BasicDeserializerFactory then re-scans everything. But that won't be necessary for long.

... and need to figure out at what point to merge things to master. Probably half-way through, to keep diff manageable.

But at least I am FINALLY making progress!

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to mention that this part of code will definitely be rewritten as right now it is strictly duplicating existing handling, but once everything works as well as it used to, can and will start simplifying & cleaning up.

@cowtowncoder cowtowncoder marked this pull request as ready for review May 18, 2024 23:04
@cowtowncoder
Copy link
Member Author

And with that, ALL tests pass! On to merging to 2.18, then try to tackle merge to master (3.0)

@cowtowncoder cowtowncoder merged commit e56bc0b into 2.18 May 18, 2024
7 checks passed
@cowtowncoder cowtowncoder deleted the feature/2.18/4515-rewrite-prop-introspection branch May 18, 2024 23:19
@JooHyukKim
Copy link
Member

Great work! @cowtowncoder 🔥

@cowtowncoder
Copy link
Member Author

Thanks @JooHyukKim ! Hope to continue with follow-up refactoring to remove Creator selection from BasicDeserializerFactory -- and then maybe improve logic in POJOPropertiesCollector by:

  1. Better support for actual annotation-less auto-detection (finally possible now)
  2. Add hook(s) for Kotlin and Scala module to indicate canonical constructor (or maybe just creator, allowing factory methods) to use, f.ex for Kotlin Data classes (and Scala has something similar too I think)

With (2) I'll need @pjfanning 's and @k163377 's help; but I am not quite there yet.
Hook(s) to call would probably be via AnnotationIntrospector, called from POJOPropertiesCollector "_addCreators()" method.

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