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

Deprecate "enclosing classes" search strategy for MergedAnnotations #28079

Closed
sbrannen opened this issue Feb 19, 2022 · 7 comments
Closed

Deprecate "enclosing classes" search strategy for MergedAnnotations #28079

sbrannen opened this issue Feb 19, 2022 · 7 comments
Assignees
@sbrannen
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone
6.0.0-M3

Comments

@sbrannen
Copy link
Member

sbrannen commented Feb 19, 2022 •
edited

Overview

The TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy for MergedAnnotations was originally introduced to support @Nested test classes in JUnit Jupiter.

However, while implementing #19930, we determined that the TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy unfortunately could not be used since it does not allow the user to control when to recurse up the enclosing class hierarchy. For example, this search strategy will automatically search on enclosing classes for static nested classes as well as for inner classes, when the user probably only wants one such category of "enclosing class" to be searched. Consequently, TestContextAnnotationUtils was introduced in the Spring TestContext Framework to address the shortcomings of the TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy.

Since this search strategy is unlikely to be useful to general users, the team should consider deprecating this search strategy in Spring Framework 6.0.

Related Issues
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Feb 19, 2022
@sbrannen sbrannen added this to the 5.3.17 milestone Feb 19, 2022
@sbrannen sbrannen self-assigned this Feb 19, 2022
@sbrannen sbrannen mentioned this issue Feb 19, 2022
Closed
@philwebb
Copy link
Member

@sbrannen We use this strategy in Spring Boot to find @ConstructorBinding annotations from nested classes. Can we reconsider deprecating it?

@snicoll snicoll reopened this Mar 11, 2022
@sbrannen
Copy link
Member Author

Hi @philwebb,

I saw that you once used it in Boot's ConfigurationPropertiesBean, but that no longer seems to be the case.

Where is TYPE_HIERARCHY_AND_ENCLOSING_CLASSES still used in Spring Boot?

@snicoll
Copy link
Member

snicoll commented Mar 12, 2022

Here is one: https://github.com/spring-projects/spring-boot/blob/de321b00b7d0f2c5c1c79a77e7241b43fbcd8313/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/context/properties/ConfigurationPropertiesReportEndpoint.java#L559

We don't on main as the semantic of @ConstructorBinding has evolved in such a way that it is no longer necessary to search it this way.

@sbrannen
Copy link
Member Author

Thanks for the link and explanation, @snicoll.

If the search strategy is only used for @ConstructorBinding against Framework 5.3.x and is no longer used in Boot 3.0+ (relying on Framework 6.0+), is there still an issue with having the search strategy deprecated in Framework 5.3.x and removed in 6.0?

@sbrannen sbrannen modified the milestones: 5.3.17, Triage Queue Mar 13, 2022
@snicoll
Copy link
Member

snicoll commented Mar 13, 2022 •
edited

I think so, yes. Our policy is to not rely on deprecated code unless absolutely necessary. Getting in this situation for the whole duration of the 2.x line is far from ideal and we'd probably copy the code to avoid using deprecated code in framework.

While I have the opportunity, I disagree with the opening statements. It may have been introduced for a very specific use case but once it becomes public API, we can't really argue that this is the only use. It sounds like an addition in TestContextAnnotationUtils is fixing the problem. It doesn't, at least for us.

@sbrannen sbrannen modified the milestones: Triage Queue, 5.3.17 Mar 15, 2022
@snicoll snicoll removed their assignment Mar 16, 2022
@sbrannen
Copy link
Member Author

For the 5.3.x line, the team has decided not to deprecate the TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy.

Instead, we will add notes to the documentation to increase awareness of how the search strategy behaves.

In addition, we will reconsider deprecation/removal of the search strategy in 6.0.x.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 16, 2022
@sbrannen
Revert "Deprecate "enclosing classes" search strategy for MergedAnnot…
c9cd53f 
…ations"

This reverts commit 5689395.

See spring-projectsgh-28079
@sbrannen sbrannen modified the milestones: 5.3.17, 6.0.0-M4 Mar 16, 2022
@sbrannen sbrannen changed the title Deprecate "enclosing classes" search strategy for MergedAnnotations Consider deprecating "enclosing classes" search strategy for MergedAnnotations Mar 16, 2022
snicoll added a commit to spring-projects/spring-boot that referenced this issue Mar 16, 2022
@snicoll
Revert unnecessary suppress warnings
b97a3ae 
See spring-projects/spring-framework#28079
@sbrannen sbrannen changed the title Consider deprecating "enclosing classes" search strategy for MergedAnnotations Deprecate "enclosing classes" search strategy for MergedAnnotations Mar 16, 2022
@sbrannen sbrannen modified the milestones: 6.0.0-M4, 6.0.0-M3 Mar 16, 2022
@sbrannen
Copy link
Member Author

Team Decision: we have decided to deprecate the TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy in 6.0 M3, allowing consumers of 6.0 milestones and release candidates to provide feedback before potentially completely removing it and/or providing an alternate mechanism for achieving the same goal prior to 6.0 GA.

sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 16, 2022
@sbrannen
Add warning to "enclosing classes" search strategy for MergedAnnotations
29d9828 
This commit adds a warning to the Javadoc for the
TYPE_HIERARCHY_AND_ENCLOSING_CLASSES search strategy in
MergedAnnotations with regard to the scope of the search
algorithm.

See spring-projectsgh-28079
@sbrannen sbrannen mentioned this issue Mar 21, 2022
Closed
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 24, 2022
@sbrannen
Introduce predicate for searching enclosing classes in MergedAnnotations
1fe394f 
Due to the deprecation of the TYPE_HIERARCHY_AND_ENCLOSING_CLASSES
search strategy (see spring-projectsgh-28079), this commit introduces a way for users
to provide a Predicate<Class<?>> that is used to decide when the
enclosing class for the class supplied to the predicate should be
searched.

This gives the user complete control over the "enclosing classes"
aspect of the search algorithm in MergedAnnotations.

- To achieve the same behavior as TYPE_HIERARCHY_AND_ENCLOSING_CLASSES,
  a user can provide `clazz -> true` as the predicate.

- To limit the enclosing class search to inner classes, a user can
  provide `ClassUtils::isInnerClass` as the predicate.

- To limit the enclosing class search to static nested classes, a user
  can provide `ClassUtils::isStaticClass` as the predicate.

- For more advanced use cases, the user can provide a custom predicate.

For example, the following performs a search on MyInnerClass within the
entire type hierarchy and enclosing class hierarchy of that class.

MergedAnnotations mergedAnnotations =
   MergedAnnotations.search(TYPE_HIERARCHY)
      .withEnclosingClasses(ClassUtils::isInnerClass)
      .from(MyInnerClass.class);

In addition, TestContextAnnotationUtils in spring-test has been
revised to use this new feature where feasible.

Closes spring-projectsgh-28207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbrannen sbrannen

Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Milestone
6.0.0-M3
Development

No branches or pull requests
3 participants
@sbrannen @snicoll @philwebb