Skip to content

Deprecate AnnotationFilter.NONE (since MergedAnnotations always filters java.lang.* annotations) #24932

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

Closed
neiser opened this issue Apr 19, 2020 · 4 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: documentation A documentation task
Milestone

Comments

@neiser
Copy link
Contributor

neiser commented Apr 19, 2020

The following test case

@Test
void deprecatedAnnotationShouldBeFoundIfFilterIsNone() {
    List<MergedAnnotation<Deprecated>> deprecatedAnnotations = MergedAnnotations.from(
            TestClass.class,
            MergedAnnotations.SearchStrategy.DIRECT,
            RepeatableContainers.standardRepeatables(),
            AnnotationFilter.NONE // That should include java.lang.* annotations?!
    ).stream(Deprecated.class).collect(Collectors.toList());
    assertThat(deprecatedAnnotations).hasSize(1); // fails!
}
@java.lang.Deprecated
private static class TestClass {
    // just for testing
}

fails although I've specified AnnotationFilter.NONE instead of the default AnnotationFilter.PLAIN filter option. I'd expect the annotation to be found (and thus the I expect the list to contain one element instead of being empty).

I suspect this is a bug since as a user of the MergedAnnotations API I expect that the AnnotationFilter.NONE returns all possible annotations, even if they're part of java.lang package.

I think I tracked the problem down to the following method
org.springframework.core.annotation.AnnotationsScanner#isIgnorable:

private static boolean isIgnorable(Class<?> annotationType) {
    return AnnotationFilter.PLAIN.matches(annotationType);
}

which is used when getting the list of annotations from the annotated element in org.springframework.core.annotation.AnnotationsScanner#getDeclaredAnnotations(java.lang.reflect.AnnotatedElement, boolean). However, I'm no expert for this part of the framework and I might be that I'm mistaken if that's the origin of the bug.

I don't know how to propose a fix for this and why this isIgnorable method is used, but I hope you agreed that this is a bug or at least unexpected behavior for using the filter option NONE.

Also note that this bug is usually hidden by a shortcut in for example org.springframework.core.annotation.AnnotationUtils#findAnnotation:

// Shortcut: directly present on the element, with no merging needed?
if (AnnotationFilter.PLAIN.matches(annotationType) ||
		AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotatedElement)) {
	return annotatedElement.getDeclaredAnnotation(annotationType);
}

So either the comment is not correct that this is just a shortcut but actually also finding annotations matching PLAIN, or the MergedAnnotations API has a bug, as explained above.

Background: I'm working on a Spring-based library and would like to find a bunch of annotations, some of them mergable, but some not. I wouldn't like to use different search utilities and always use the MergedAnnotations API, no matter what kind of annotation I'm looking for within my library.

Thanks a lot for investigating!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 19, 2020
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Apr 19, 2020
@sbrannen
Copy link
Member

Thanks for reporting the issue and for providing the test case.

I have confirmed that the test fails as claimed against master.

I have also confirmed that the cause of this behavior is that AnnotationsScanner.isIgnorable(Class<? extends Annotation>) returns true for java.lang.Deprecated, causing it to be removed from the results before the user-specified filter is even applied.

In other words, the application of AnnotationFilter.PLAIN (in isIgnorable()) preemptively overrides the user's choice of AnnotationFilter.NONE for such use cases.

@sbrannen sbrannen self-assigned this Apr 19, 2020
@sbrannen sbrannen added this to the 5.2.6 milestone Apr 19, 2020
@sbrannen
Copy link
Member

Tentatively slated for 5.2.6 for team discussion

@sbrannen sbrannen added status: pending-design-work Needs design work before any code can be developed and removed status: waiting-for-triage An issue we've not yet triaged or decided on for: team-attention labels Apr 20, 2020
@jhoeller jhoeller changed the title Using MergedAnnotations.from(..., AnnotationFilter.NONE) still filters java.lang.* annotations Deprecate AnnotationFilter.NONE (since MergedAnnotations always filters java.lang.* annotations) Apr 25, 2020
@jhoeller
Copy link
Contributor

We've decided to deprecate AnnotationFilter.NONE altogether since the MergedAnnotations API and its backing model have been designed for composable annotations in Spring's common component model, with a focus on attribute aliasing and meta-annotation relationships. By design, there is no support for retrieving plain Java annotations with this API; please use standard Java reflection or Spring's AnnotationUtils for simple annotation retrieval purposes.

As a consequence, I'm repurposing this ticket for the deprecation step and related documentation.

@jhoeller jhoeller added type: documentation A documentation task and removed status: pending-design-work Needs design work before any code can be developed labels Apr 25, 2020
@neiser
Copy link
Contributor Author

neiser commented May 3, 2020

@jhoeller @sbrannen Thanks for clarifying the situation of the reported problem. You're proposing to use the AnnotationUtils class for simple annotation retrieva. However, I'm searching now for a method in Spring Boot Core to find annotations matching PLAIN (thus ignored by Merged Annotatiosn API) which can appear multiple times and are supposed to be analyzed by the caller in order of appearance. As far as I can see, AnnotationUtils only offers findAnnotation to get the first appearance, but not all of them. That was actually the reason why I've started to use the Merged Annotations API: To find all declared annotations in a type hierarchy.

I'd appreciate a little hint on this from your side. Maybe I'm just missing a method? For the moment, I'm using code like this to "workaround" this:

    if (AnnotationFilter.PLAIN.matches(annotationType)) {
         // PLAIN annotations are ignored by merged annotations API, see
         // https://github.com/spring-projects/spring-framework/issues/24932
         return Arrays.stream(annotatedElement.getAnnotationsByType(annotationType));
    } else {
        return mergedAnnotations.stream(annotationType).map(MergedAnnotation::synthesize);
    }

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: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

4 participants