From c9857702e40afb5aa13980420f15f307bacee9e1 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 27 Mar 2019 12:56:11 +0100 Subject: [PATCH] Refined retrieval of plain annotations through direct presence checks Shortcut checks apply for hasPlainJavaAnnotationsOnly types as well now. Closes gh-22685 --- spring-core/spring-core.gradle | 1 + .../annotation/AnnotatedElementUtils.java | 34 +++------- .../core/annotation/AnnotationFilter.java | 9 ++- .../core/annotation/AnnotationUtils.java | 40 ++++------- .../annotation/TypeMappedAnnotations.java | 18 ++--- .../AnnotatedElementUtilsTests.java | 67 +++++++++++++++---- .../annotation/AnnotationFilterTests.java | 7 +- .../core/annotation/AnnotationUtilsTests.java | 6 +- 8 files changed, 100 insertions(+), 82 deletions(-) diff --git a/spring-core/spring-core.gradle b/spring-core/spring-core.gradle index a38f986ed9..720d4e5cc0 100644 --- a/spring-core/spring-core.gradle +++ b/spring-core/spring-core.gradle @@ -85,6 +85,7 @@ dependencies { optional("io.netty:netty-buffer") testCompile("io.projectreactor:reactor-test") testCompile("org.apache.tomcat.embed:tomcat-embed-core:${tomcatVersion}") + testCompile("com.google.code.findbugs:jsr305:3.0.2") testCompile("org.xmlunit:xmlunit-matchers:2.6.2") testCompile("javax.xml.bind:jaxb-api:2.3.1") testCompile("com.fasterxml.woodstox:woodstox-core:5.2.0") { diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java index cfbeafa8d8..31c3c3adb3 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java @@ -200,15 +200,12 @@ public abstract class AnnotatedElementUtils { * @since 4.2.3 * @see #hasAnnotation(AnnotatedElement, Class) */ - public static boolean isAnnotated(AnnotatedElement element,Class annotationType) { - // Shortcut: directly present on the element, with no processing needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + public static boolean isAnnotated(AnnotatedElement element, Class annotationType) { + // Shortcut: directly present on the element, with no merging needed? + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { return element.isAnnotationPresent(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { - return false; - } // Exhaustive retrieval of merged annotations... return getAnnotations(element).isPresent(annotationType); } @@ -332,13 +329,10 @@ public abstract class AnnotatedElementUtils { @Nullable public static A getMergedAnnotation(AnnotatedElement element, Class annotationType) { // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { return element.getDeclaredAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { - return null; - } // Exhaustive retrieval of merged annotations... return getAnnotations(element) .get(annotationType, null, MergedAnnotationSelectors.firstDirectlyDeclared()) @@ -528,14 +522,11 @@ public abstract class AnnotatedElementUtils { * @see #isAnnotated(AnnotatedElement, Class) */ public static boolean hasAnnotation(AnnotatedElement element, Class annotationType) { - // Shortcut: directly present on the element, with no processing needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + // Shortcut: directly present on the element, with no merging needed? + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { return element.isAnnotationPresent(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { - return false; - } // Exhaustive retrieval of merged annotations... return findAnnotations(element).isPresent(annotationType); } @@ -633,13 +624,10 @@ public abstract class AnnotatedElementUtils { @Nullable public static A findMergedAnnotation(AnnotatedElement element, Class annotationType) { // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { return element.getDeclaredAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(element)) { - return null; - } // Exhaustive retrieval of merged annotations... return findAnnotations(element) .get(annotationType, null, MergedAnnotationSelectors.firstDirectlyDeclared()) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationFilter.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationFilter.java index b86a4b1de4..b0c8d32bcd 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationFilter.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationFilter.java @@ -28,17 +28,16 @@ import java.lang.annotation.Annotation; public interface AnnotationFilter { /** - * {@link AnnotationFilter} that matches annotations is in the - * {@code java.lang.*} or in the - * {@code org.springframework.lang.*} package. + * {@link AnnotationFilter} that matches annotations in the + * {@code java.lang.*} and {@code org.springframework.lang.*} packages. */ AnnotationFilter PLAIN = packages("java.lang", "org.springframework.lang"); /** * {@link AnnotationFilter} that matches annotations in the - * {@code java.lang.*} package. + * {@code java.*}/{@code javax.*} namespaces. */ - AnnotationFilter JAVA = packages("java.lang"); + AnnotationFilter JAVA = packages("java", "javax"); /** * {@link AnnotationFilter} that never matches and can be used when no diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java index 3de3a7eb8a..8a59547cb4 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationUtils.java @@ -210,13 +210,10 @@ public abstract class AnnotationUtils { @Nullable public static A getAnnotation(AnnotatedElement annotatedElement, Class annotationType) { // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotatedElement)) { return annotatedElement.getAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotatedElement)) { - return null; - } // Exhaustive retrieval of merged annotations... return MergedAnnotations.from(annotatedElement, SearchStrategy.INHERITED_ANNOTATIONS, RepeatableContainers.none(), AnnotationFilter.PLAIN) @@ -483,13 +480,10 @@ public abstract class AnnotationUtils { return null; } // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotatedElement)) { return annotatedElement.getDeclaredAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotatedElement)) { - return null; - } // Exhaustive retrieval of merged annotations... return MergedAnnotations.from(annotatedElement, SearchStrategy.INHERITED_ANNOTATIONS) .get(annotationType).withNonMergedAttributes() @@ -517,13 +511,10 @@ public abstract class AnnotationUtils { return null; } // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(method)) { return method.getDeclaredAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(method)) { - return null; - } // Exhaustive retrieval of merged annotations... return MergedAnnotations.from(method, SearchStrategy.EXHAUSTIVE) .get(annotationType).withNonMergedAttributes() @@ -558,13 +549,10 @@ public abstract class AnnotationUtils { return null; } // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType)) { + if (AnnotationFilter.PLAIN.matches(annotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(clazz)) { return clazz.getDeclaredAnnotation(annotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(clazz)) { - return null; - } // Exhaustive retrieval of merged annotations... return MergedAnnotations.from(clazz, SearchStrategy.EXHAUSTIVE) .get(annotationType).withNonMergedAttributes() @@ -710,17 +698,13 @@ public abstract class AnnotationUtils { return false; } // Shortcut: directly present on the element, with no merging needed? - if (AnnotationFilter.PLAIN.matches(annotationType) || - AnnotationFilter.PLAIN.matches(metaAnnotationType)) { + if (AnnotationFilter.PLAIN.matches(metaAnnotationType) || + AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotationType)) { return annotationType.isAnnotationPresent(metaAnnotationType); } - // Shortcut: no searchable annotations to be found on plain Java classes and core Spring types... - if (AnnotationsScanner.hasPlainJavaAnnotationsOnly(annotationType)) { - return false; - } // Exhaustive retrieval of merged annotations... - return (MergedAnnotations.from( - annotationType, SearchStrategy.INHERITED_ANNOTATIONS).isPresent(metaAnnotationType)); + return MergedAnnotations.from(annotationType, SearchStrategy.INHERITED_ANNOTATIONS, + RepeatableContainers.none(), AnnotationFilter.PLAIN).isPresent(metaAnnotationType); } /** diff --git a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotations.java b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotations.java index 3d869c4d50..1b091c3772 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotations.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotations.java @@ -40,7 +40,7 @@ import org.springframework.lang.Nullable; */ final class TypeMappedAnnotations implements MergedAnnotations { - private static final AnnotationFilter FILTER_ALL = annotationType -> true; + private static final AnnotationFilter FILTER_ALL = (annotationType -> true); private static final MergedAnnotations NONE = new TypeMappedAnnotations( null, new Annotation[0], RepeatableContainers.none(), FILTER_ALL); @@ -108,8 +108,8 @@ final class TypeMappedAnnotations implements MergedAnnotations { } @Override - public boolean isDirectlyPresent(@Nullable Class annotationType) { - if (annotationType == null || this.annotationFilter.matches(annotationType)) { + public boolean isDirectlyPresent(Class annotationType) { + if (this.annotationFilter.matches(annotationType)) { return false; } return Boolean.TRUE.equals(scan(annotationType, @@ -176,9 +176,7 @@ final class TypeMappedAnnotations implements MergedAnnotations { } @Override - public Stream> stream( - @Nullable Class annotationType) { - + public Stream> stream(Class annotationType) { if (this.annotationFilter == FILTER_ALL) { return Stream.empty(); } @@ -186,9 +184,7 @@ final class TypeMappedAnnotations implements MergedAnnotations { } @Override - public Stream> stream( - @Nullable String annotationType) { - + public Stream> stream(String annotationType) { if (this.annotationFilter == FILTER_ALL) { return Stream.empty(); } @@ -219,8 +215,7 @@ final class TypeMappedAnnotations implements MergedAnnotations { return spliterator(null); } - private Spliterator> spliterator( - @Nullable Object annotationType) { + private Spliterator> spliterator(@Nullable Object annotationType) { return new AggregatesSpliterator<>(annotationType, getAggregates()); } @@ -248,6 +243,7 @@ final class TypeMappedAnnotations implements MergedAnnotations { return null; } + static MergedAnnotations from(@Nullable AnnotatedElement element, SearchStrategy searchStrategy, RepeatableContainers repeatableContainers, AnnotationFilter annotationFilter) { diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java index 00bb15ed6b..ddbcb6d2f9 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java @@ -17,6 +17,7 @@ package org.springframework.core.annotation; import java.lang.annotation.Annotation; +import java.lang.annotation.Documented; import java.lang.annotation.ElementType; import java.lang.annotation.Inherited; import java.lang.annotation.Retention; @@ -28,6 +29,8 @@ import java.lang.reflect.Method; import java.util.Date; import java.util.List; import java.util.Set; +import javax.annotation.Nonnull; +import javax.annotation.ParametersAreNonnullByDefault; import javax.annotation.Resource; import org.junit.Ignore; @@ -36,6 +39,7 @@ import org.junit.Test; import org.junit.internal.ArrayComparisonFailure; import org.junit.rules.ExpectedException; +import org.springframework.lang.NonNullApi; import org.springframework.lang.Nullable; import org.springframework.stereotype.Component; import org.springframework.stereotype.Indexed; @@ -121,33 +125,70 @@ public class AnnotatedElementUtilsTests { @Test public void isAnnotatedOnNonAnnotatedClass() { + assertFalse(isAnnotated(NonAnnotatedClass.class, Transactional.class)); + } + + @Test + public void isAnnotatedOnClassWithMetaDepth() { + assertTrue(isAnnotated(TransactionalComponentClass.class, TransactionalComponent.class)); + assertFalse("isAnnotated() does not search the class hierarchy.", + isAnnotated(SubTransactionalComponentClass.class, TransactionalComponent.class)); + assertTrue(isAnnotated(TransactionalComponentClass.class, Transactional.class)); + assertTrue(isAnnotated(TransactionalComponentClass.class, Component.class)); + assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, Transactional.class)); + assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, Component.class)); + assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, ComposedTransactionalComponent.class)); + } + + @Test + public void isAnnotatedForPlainTypes() { + assertTrue(isAnnotated(Order.class, Documented.class)); + assertTrue(isAnnotated(NonNullApi.class, Documented.class)); + assertTrue(isAnnotated(NonNullApi.class, Nonnull.class)); + assertTrue(isAnnotated(ParametersAreNonnullByDefault.class, Nonnull.class)); + } + + @Test + public void isAnnotatedWithNameOnNonAnnotatedClass() { assertFalse(isAnnotated(NonAnnotatedClass.class, TX_NAME)); } @Test - public void isAnnotatedOnClassWithMetaDepth0() { + public void isAnnotatedWithNameOnClassWithMetaDepth() { assertTrue(isAnnotated(TransactionalComponentClass.class, TransactionalComponent.class.getName())); - } - - @Test - public void isAnnotatedOnSubclassWithMetaDepth0() { assertFalse("isAnnotated() does not search the class hierarchy.", isAnnotated(SubTransactionalComponentClass.class, TransactionalComponent.class.getName())); - } - - @Test - public void isAnnotatedOnClassWithMetaDepth1() { assertTrue(isAnnotated(TransactionalComponentClass.class, TX_NAME)); assertTrue(isAnnotated(TransactionalComponentClass.class, Component.class.getName())); - } - - @Test - public void isAnnotatedOnClassWithMetaDepth2() { assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, TX_NAME)); assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, Component.class.getName())); assertTrue(isAnnotated(ComposedTransactionalComponentClass.class, ComposedTransactionalComponent.class.getName())); } + @Test + public void hasAnnotationOnNonAnnotatedClass() { + assertFalse(hasAnnotation(NonAnnotatedClass.class, Transactional.class)); + } + + @Test + public void hasAnnotationOnClassWithMetaDepth() { + assertTrue(hasAnnotation(TransactionalComponentClass.class, TransactionalComponent.class)); + assertTrue(hasAnnotation(SubTransactionalComponentClass.class, TransactionalComponent.class)); + assertTrue(hasAnnotation(TransactionalComponentClass.class, Transactional.class)); + assertTrue(hasAnnotation(TransactionalComponentClass.class, Component.class)); + assertTrue(hasAnnotation(ComposedTransactionalComponentClass.class, Transactional.class)); + assertTrue(hasAnnotation(ComposedTransactionalComponentClass.class, Component.class)); + assertTrue(hasAnnotation(ComposedTransactionalComponentClass.class, ComposedTransactionalComponent.class)); + } + + @Test + public void hasAnnotationForPlainTypes() { + assertTrue(hasAnnotation(Order.class, Documented.class)); + assertTrue(hasAnnotation(NonNullApi.class, Documented.class)); + assertTrue(hasAnnotation(NonNullApi.class, Nonnull.class)); + assertTrue(hasAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)); + } + @Test public void getAllAnnotationAttributesOnNonAnnotatedClass() { assertNull(getAllAnnotationAttributes(NonAnnotatedClass.class, TX_NAME)); diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java index 2f2d0aaa47..5b9f9d97ca 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java @@ -18,7 +18,7 @@ package org.springframework.core.annotation; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; -import java.util.Arrays; +import javax.annotation.Nonnull; import org.junit.Test; @@ -82,6 +82,11 @@ public class AnnotationFilterTests { assertThat(AnnotationFilter.JAVA.matches(Retention.class)).isTrue(); } + @Test + public void javaWhenJavaxAnnotationReturnsTrue() { + assertThat(AnnotationFilter.JAVA.matches(Nonnull.class)).isTrue(); + } + @Test public void javaWhenSpringLangAnnotationReturnsFalse() { assertThat(AnnotationFilter.JAVA.matches(Nullable.class)).isFalse(); diff --git a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java index 31be5ccec9..a6a1616ec6 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java @@ -29,6 +29,8 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; +import javax.annotation.Nonnull; +import javax.annotation.ParametersAreNonnullByDefault; import org.junit.Before; import org.junit.Rule; @@ -438,9 +440,11 @@ public class AnnotationUtilsTests { } @Test - public void isAnnotationMetaPresentForJavaLangType() { + public void isAnnotationMetaPresentForPlainType() { assertTrue(isAnnotationMetaPresent(Order.class, Documented.class)); assertTrue(isAnnotationMetaPresent(NonNullApi.class, Documented.class)); + assertTrue(isAnnotationMetaPresent(NonNullApi.class, Nonnull.class)); + assertTrue(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Nonnull.class)); } @Test