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.

Sign up for GitHub

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

Jump to bottom

ImportsContextCustomizer does not support AliasFor #34917

Closed
wants to merge 2 commits into from
Closed

ImportsContextCustomizer does not support AliasFor #34917

wants to merge 2 commits into from

Conversation

lmartelli
Copy link
Contributor

Fixes #34854

Use MergedAnnotations in order to support @AliasFor.

I used plain Predicate instead of AnnotationFilter because it is much simpler and generic.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 8, 2023
@lmartelli lmartelli force-pushed the ImportsContextCustomizer-support-AliasFor branch 4 times, most recently from bb59448 to 8c75138 Compare April 9, 2023 18:49
@lmartelli lmartelli force-pushed the ImportsContextCustomizer-support-AliasFor branch from 8c75138 to 3a2e2cd Compare April 9, 2023 18:57
wilkinsona
wilkinsona requested changes Apr 11, 2023
View reviewed changes
Copy link
Member

@wilkinsona wilkinsona 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 proposal, @lmartelli. We try to minimise the scope of changes that we make, particularly in maintenance releases. With this in mind, these changes are too broad. For example, the move from AnnotationFilter to Predicate is not necessary to fix the problem. We also don't use var in Spring Boot's code. Would you like to rework your proposal so that it only makes the changes that are required to fix the problem and nothing more?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 11, 2023
@lmartelli
Copy link
Contributor Author

Would you like to rework your proposal so that it only makes the changes that are required to fix the problem and nothing more?

Sure, I'll do that.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 11, 2023
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 11, 2023
@lmartelli
Copy link
Contributor Author

Would you like to rework your proposal so that it only makes the changes that are required to fix the problem and nothing more?

Sure, I'll do that.

@wilkinsona But note that we still have to remove ImportsContextCustomizer.AnnotationFilter and use the one from org.springframework.core.annotation.AnnotationFilter instead since that's what org.springframework.core.annotation.MergedAnnotations.Search#withAnnotationFilter() wants.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 14, 2023
@lmartelli
Copy link
Contributor Author

@wilkinsona is the new version OK ?

@wilkinsona wilkinsona mentioned this pull request Apr 28, 2023
Closed
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Apr 28, 2023
@wilkinsona wilkinsona added this to the 2.7.x milestone Apr 28, 2023
@snicoll snicoll changed the title ImportsContextCustomizer supports AliasFor ImportsContextCustomizer does not support AliasFor May 10, 2023
@snicoll snicoll self-assigned this May 10, 2023
@snicoll
Copy link
Member

snicoll commented May 10, 2023

This still looks quite a large change to me and I am not sure the use of stream and reduce is something we want. The MergedAnnotations API you've used doesn't exist in 2.7.x and we need to be able to rebase the change there. Can you please limit your change to support aliases and only that? We can refine the code further as a separate step.

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label May 10, 2023
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jun 1, 2023
@snicoll
Copy link
Member

snicoll commented Jun 12, 2023

@lmartelli do you want to review your contribution based on the suggestion above? If no, no problem and we can close this and reopen the related issue.

@lmartelli
Copy link
Contributor Author

@lmartelli do you want to review your contribution based on the suggestion above? If no, no problem and we can close this and reopen the related issue.

Yes, I will do that. I just need to find a little time.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 14, 2023
@philwebb philwebb mentioned this pull request Jul 4, 2023
Closed
philwebb pushed a commit that referenced this pull request Jul 4, 2023
@philwebb
Test that ImportsContextCustomizer can support @AliasFor
9baf23c 
Add test to ensure that `ImportsContextCustomizer` can support
`@AliasFor` annotations.

See gh-34917
philwebb added a commit that referenced this pull request Jul 4, 2023
@philwebb
Use MergedAnnotations for @AliasFor support
5d0640a 
Update `ImportsContextCustomizer` to use `MergedAnnotations` so
that `@AliasFor` can be supported.

See gh-34917
@philwebb philwebb closed this in b7ad85c Jul 4, 2023
@philwebb philwebb modified the milestones: 2.7.x, 2.7.14 Jul 4, 2023
@philwebb philwebb mentioned this pull request Jul 4, 2023
Closed
@philwebb
Copy link
Member

philwebb commented Jul 4, 2023

Thanks very much @lmartelli, I've taken the tests from this commit and merged them into 2.7 along with a minimal fix. I've also taken the changes you made to ImportsContextCustomizer and applied them with a little polish to main (see #36211).

Thanks again!

@Netyyyy Netyyyy mentioned this pull request Aug 10, 2023
Closed
@Netyyyy Netyyyy mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkinsona wilkinsona wilkinsona requested changes

Assignees

@snicoll snicoll

Labels
for: merge-with-amendments Needs some changes when we merge status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Milestone
2.7.14
Development

Successfully merging this pull request may close these issues.

ImportsContextCustomizer does not support AliasFor
5 participants
@lmartelli @snicoll @philwebb @wilkinsona @spring-projects-issues