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

Resolve type variable recursively in GenericTypeResolver #30079

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Mar 8, 2023

@quaff quaff marked this pull request as draft March 8, 2023 05:02
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 8, 2023
@quaff quaff marked this pull request as ready for review March 8, 2023 07:12
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Mar 8, 2023
@sbrannen sbrannen requested a review from jhoeller March 8, 2023 11:00
@sbrannen sbrannen changed the title Improve GenericTypeResolver to resolve type variable recursively Resolve type variable recursively in GenericTypeResolver Mar 8, 2023
@quaff
Copy link
Contributor Author

quaff commented Jun 20, 2023

@jhoeller Could you review it?

@quaff
Copy link
Contributor Author

quaff commented Jun 30, 2023

Rebased to main, ready for review.

@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 29, 2023
@snicoll snicoll self-assigned this Sep 29, 2023
@snicoll snicoll added this to the 6.1.0-RC1 milestone Sep 29, 2023
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @quaff - This looks promising as it fixes another issue that was reported against generics handling.

Unfortunately, the build is broken with those changes, see https://ge.spring.io/s/kktsevdqhmgqc/tests/overview?outcome=FAILED.

@quaff
Copy link
Contributor Author

quaff commented Oct 7, 2023

Thanks for the PR @quaff - This looks promising as it fixes another issue that was reported against generics handling.

Unfortunately, the build is broken with those changes, see https://ge.spring.io/s/kktsevdqhmgqc/tests/overview?outcome=FAILED.

The failed tests should adapt changes on ResolvableType.java, I've updated the commit.

@snicoll
Copy link
Member

snicoll commented Oct 10, 2023

The failed tests should adapt changes on ResolvableType.java, I've updated the commit.

With those changes in place, you had to change some assertions that looks invalid to me now. For instance, event listening that accepts a String as input would be considered if the type is EntityWrapper<?>. Same for an event that accepts an EntityWrapper<?> would now accept a List<?>.

This may be something that wrong elsewhere that the fix reveals but changing the assertion isn't what I had in mind.

Given the impact this has so late, I've moved this one for 6.2. If you have time to review the above, that would be much appreciated.

@quaff
Copy link
Contributor Author

quaff commented Oct 11, 2023

The failed tests should adapt changes on ResolvableType.java, I've updated the commit.

With those changes in place, you had to change some assertions that looks invalid to me now. For instance, event listening that accepts a String as input would be considered if the type is EntityWrapper<?>. Same for an event that accepts an EntityWrapper<?> would now accept a List<?>.

This may be something that wrong elsewhere that the fix reveals but changing the assertion isn't what I had in mind.

Given the impact this has so late, I've moved this one for 6.2. If you have time to review the above, that would be much appreciated.

I've fixed that, please review. @snicoll

@quaff

This comment was marked as resolved.

@quaff quaff requested a review from snicoll October 25, 2023 02:17
@snicoll

This comment was marked as resolved.

@snicoll
Copy link
Member

snicoll commented Nov 29, 2023

@quaff with #31690 (comment) some code that we didn't intend to have has been reverted. If you have time to rebase, it would be much appreciated.

@quaff
Copy link
Contributor Author

quaff commented Nov 30, 2023

@quaff with #31690 (comment) some code that we didn't intend to have has been reverted. If you have time to rebase, it would be much appreciated.

Done.

@jhoeller jhoeller removed their assignment Dec 22, 2023
@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.0-M1 Feb 5, 2024
@jhoeller jhoeller merged commit e788aeb into spring-projects:main Feb 15, 2024
2 checks passed
jhoeller added a commit that referenced this pull request Feb 15, 2024
@kilink

This comment was marked as resolved.

@kilink

This comment was marked as resolved.

@kilink

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants