From fb83e83e789097ff3197bb93781ea09be92d6869 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sat, 20 Jun 2015 01:21:39 +0200 Subject: [PATCH] Honor contract of @Repeatable in AnnotationUtils Prior to this commit, the implementation of getRepeatableAnnotation() in Spring's AnnotationUtils complied neither with the contract of getAnnotationsByType() nor with the contract of getDeclaredAnnotationsByType() as defined in AnnotatedElement in Java 8. Specifically, unexpected results can be encountered when using Spring's support for @Repeatable annotations: either annotations show up in the returned set in the wrong order, or annotations are returned in the set that should not even be found based on the semantics of @Repeatable. This commit remedies this problem by deprecating the existing getRepeatableAnnotation() methods and replacing them with new getRepeatableAnnotations() and getDeclaredRepeatableAnnotations() methods that comply with the contracts of Java's getAnnotationsByType() and getDeclaredAnnotationsByType(), respectively. Issue: SPR-13068 --- .../ScheduledAnnotationBeanPostProcessor.java | 2 +- .../core/annotation/AnnotationUtils.java | 146 ++++++++++++---- .../core/annotation/AnnotationUtilsTests.java | 157 +++++++++++++++--- .../jdbc/SqlScriptsTestExecutionListener.java | 4 +- 4 files changed, 251 insertions(+), 58 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java index 6131aba8ad..591ae6a350 100644 --- a/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java +++ b/spring-context/src/main/java/org/springframework/scheduling/annotation/ScheduledAnnotationBeanPostProcessor.java @@ -224,7 +224,7 @@ public class ScheduledAnnotationBeanPostProcessor implements BeanPostProcessor, @Override public void doWith(Method method) throws IllegalArgumentException, IllegalAccessException { for (Scheduled scheduled : - AnnotationUtils.getRepeatableAnnotation(method, Schedules.class, Scheduled.class)) { + AnnotationUtils.getRepeatableAnnotations(method, Schedules.class, Scheduled.class)) { processScheduled(scheduled, method, bean); annotatedMethods.add(method); } 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 7d16c256df..d6f2082c79 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 @@ -61,8 +61,9 @@ import org.springframework.util.StringUtils; * inheritance hierarchy of the given method ({@link #findAnnotation(Method, Class)}). * *

Terminology

- * The terms directly present and present have the same - * meanings as defined in the class-level Javadoc for {@link AnnotatedElement}. + * The terms directly present, indirectly present, and + * present have the same meanings as defined in the class-level + * Javadoc for {@link AnnotatedElement} (in Java 8). * *

An annotation is meta-present on an element if the annotation * is declared as a meta-annotation on some other annotation which is @@ -248,53 +249,125 @@ public abstract class AnnotationUtils { } /** - * Get the repeatable {@link Annotation}s of {@code annotationType} - * from the supplied {@link Method}, where such annotations are either - * present or meta-present on the method. - *

Handles both single annotations and annotations nested within a - * containing annotation. - *

Correctly handles bridge {@link Method Methods} generated by the compiler. - *

Meta-annotations will be searched if the annotation is not - * present on the supplied method. - * @param method the method to look for annotations on; never {@code null} - * @param containerAnnotationType the type of the container that holds the - * annotations; may be {@code null} if a container is not supported - * @param annotationType the annotation type to look for - * @return the annotations found or an empty set; never {@code null} + * Delegates to {@link #getRepeatableAnnotations}. * @since 4.0 - * @see org.springframework.core.BridgeMethodResolver#findBridgedMethod(Method) - * @see java.lang.annotation.Repeatable + * @deprecated As of Spring Framework 4.2, use {@link #getRepeatableAnnotations} + * or {@link #getDeclaredRepeatableAnnotations} instead. */ + @Deprecated public static Set getRepeatableAnnotation(Method method, Class containerAnnotationType, Class annotationType) { - - Method resolvedMethod = BridgeMethodResolver.findBridgedMethod(method); - return getRepeatableAnnotation((AnnotatedElement) resolvedMethod, containerAnnotationType, annotationType); + return getRepeatableAnnotations(method, containerAnnotationType, annotationType); } /** - * Get the repeatable {@link Annotation}s of {@code annotationType} - * from the supplied {@link AnnotatedElement}, where such annotations are - * either present or meta-present on the element. + * Delegates to {@link #getRepeatableAnnotations}. + * @since 4.0 + * @deprecated As of Spring Framework 4.2, use {@link #getRepeatableAnnotations} + * or {@link #getDeclaredRepeatableAnnotations} instead. + */ + @Deprecated + public static Set getRepeatableAnnotation(AnnotatedElement annotatedElement, + Class containerAnnotationType, Class annotationType) { + return getRepeatableAnnotations(annotatedElement, containerAnnotationType, annotationType); + } + + /** + * Get the repeatable {@linkplain Annotation annotations} of + * {@code annotationType} from the supplied {@link AnnotatedElement}, where + * such annotations are either present or meta-present + * on the element. + *

This method mimics the functionality of Java 8's + * {@link java.lang.reflect.AnnotatedElement#getAnnotationsByType(Class)} + * with additional support for meta-annotations. *

Handles both single annotations and annotations nested within a - * containing annotation. + * container annotation. + *

Correctly handles bridge methods generated by the + * compiler if the supplied element is a {@link Method}. *

Meta-annotations will be searched if the annotation is not * present on the supplied element. * @param annotatedElement the element to look for annotations on; never {@code null} - * @param containerAnnotationType the type of the container that holds the - * annotations; may be {@code null} if a container is not supported - * @param annotationType the annotation type to look for + * @param containerAnnotationType the type of the container that holds + * the annotations; may be {@code null} if a container is not supported + * @param annotationType the annotation type to look for; never {@code null} * @return the annotations found or an empty set; never {@code null} - * @since 4.0 + * @since 4.2 + * @see #getDeclaredRepeatableAnnotations(AnnotatedElement, Class, Class) + * @see org.springframework.core.BridgeMethodResolver#findBridgedMethod * @see java.lang.annotation.Repeatable + * @see java.lang.reflect.AnnotatedElement#getAnnotationsByType */ - public static Set getRepeatableAnnotation(AnnotatedElement annotatedElement, + public static Set getRepeatableAnnotations(AnnotatedElement annotatedElement, Class containerAnnotationType, Class annotationType) { + Set annotations = getDeclaredRepeatableAnnotations(annotatedElement, containerAnnotationType, annotationType); + if (!annotations.isEmpty()) { + return annotations; + } + + return getRepeatableAnnotations(annotatedElement, containerAnnotationType, annotationType, false); + } + + /** + * Get the declared repeatable {@linkplain Annotation annotations} + * of {@code annotationType} from the supplied {@link AnnotatedElement}, + * where such annotations are either directly present, + * indirectly present, or meta-present on the element. + *

This method mimics the functionality of Java 8's + * {@link java.lang.reflect.AnnotatedElement#getDeclaredAnnotationsByType(Class)} + * with additional support for meta-annotations. + *

Handles both single annotations and annotations nested within a + * container annotation. + *

Correctly handles bridge methods generated by the + * compiler if the supplied element is a {@link Method}. + *

Meta-annotations will be searched if the annotation is not + * present on the supplied element. + * @param annotatedElement the element to look for annotations on; never {@code null} + * @param containerAnnotationType the type of the container that holds + * the annotations; may be {@code null} if a container is not supported + * @param annotationType the annotation type to look for; never {@code null} + * @return the annotations found or an empty set; never {@code null} + * @since 4.2 + * @see #getRepeatableAnnotations(AnnotatedElement, Class, Class) + * @see org.springframework.core.BridgeMethodResolver#findBridgedMethod + * @see java.lang.annotation.Repeatable + * @see java.lang.reflect.AnnotatedElement#getDeclaredAnnotationsByType + */ + public static Set getDeclaredRepeatableAnnotations(AnnotatedElement annotatedElement, + Class containerAnnotationType, Class annotationType) { + return getRepeatableAnnotations(annotatedElement, containerAnnotationType, annotationType, true); + } + + /** + * Perform the actual work for {@link #getRepeatableAnnotations(AnnotatedElement, Class, Class)} + * and {@link #getDeclaredRepeatableAnnotations(AnnotatedElement, Class, Class)}. + *

Correctly handles bridge methods generated by the + * compiler if the supplied element is a {@link Method}. + *

Meta-annotations will be searched if the annotation is not + * present on the supplied element. + * + * @param annotatedElement the element to look for annotations on; never {@code null} + * @param containerAnnotationType the type of the container that holds + * the annotations; may be {@code null} if a container is not supported + * @param annotationType the annotation type to look for; never {@code null} + * @param declaredMode {@code true} if only declared annotations (i.e., + * directly or indirectly present) should be considered. + * @return the annotations found or an empty set; never {@code null} + * @since 4.2 + * @see org.springframework.core.BridgeMethodResolver#findBridgedMethod + * @see java.lang.annotation.Repeatable + */ + private static Set getRepeatableAnnotations(AnnotatedElement annotatedElement, + Class containerAnnotationType, Class annotationType, boolean declaredMode) { + + Assert.notNull(annotatedElement, "annotatedElement must not be null"); + Assert.notNull(annotationType, "annotationType must not be null"); + try { - if (annotatedElement.getAnnotations().length > 0) { - return new AnnotationCollector(containerAnnotationType, annotationType).getResult(annotatedElement); + if (annotatedElement instanceof Method) { + annotatedElement = BridgeMethodResolver.findBridgedMethod((Method) annotatedElement); } + return new AnnotationCollector(containerAnnotationType, annotationType, declaredMode).getResult(annotatedElement); } catch (Exception ex) { handleIntrospectionFailure(annotatedElement, ex); @@ -1596,16 +1669,20 @@ public abstract class AnnotationUtils { private final Class annotationType; + private final boolean declaredMode; + private final Set visited = new HashSet(); private final Set result = new LinkedHashSet(); - public AnnotationCollector(Class containerAnnotationType, Class annotationType) { + + AnnotationCollector(Class containerAnnotationType, Class annotationType, boolean declaredMode) { this.containerAnnotationType = containerAnnotationType; this.annotationType = annotationType; + this.declaredMode = declaredMode; } - public Set getResult(AnnotatedElement element) { + Set getResult(AnnotatedElement element) { process(element); return Collections.unmodifiableSet(this.result); } @@ -1614,7 +1691,8 @@ public abstract class AnnotationUtils { private void process(AnnotatedElement element) { if (this.visited.add(element)) { try { - for (Annotation ann : element.getAnnotations()) { + Annotation[] annotations = (declaredMode ? element.getDeclaredAnnotations() : element.getAnnotations()); + for (Annotation ann : annotations) { Class currentAnnotationType = ann.annotationType(); if (ObjectUtils.nullSafeEquals(this.annotationType, currentAnnotationType)) { this.result.add(synthesizeAnnotation((A) ann, element)); 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 94c1fb9d83..02fcc385c8 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 @@ -493,40 +493,130 @@ public class AnnotationUtilsTests { @Test public void findRepeatableAnnotationOnComposedAnnotation() { - Repeatable repeatable = findAnnotation(MyRepeatableMeta.class, Repeatable.class); + Repeatable repeatable = findAnnotation(MyRepeatableMeta1.class, Repeatable.class); assertNotNull(repeatable); assertEquals(MyRepeatableContainer.class, repeatable.value()); } @Test - public void getRepeatableFromMethod() throws Exception { + public void getRepeatableAnnotationsDeclaredOnMethod() throws Exception { Method method = InterfaceWithRepeated.class.getMethod("foo"); - Set annotations = getRepeatableAnnotation(method, MyRepeatableContainer.class, MyRepeatable.class); + Set annotations = getRepeatableAnnotations(method, MyRepeatableContainer.class, MyRepeatable.class); assertNotNull(annotations); List values = annotations.stream().map(MyRepeatable::value).collect(toList()); - assertThat(values, equalTo(Arrays.asList("a", "b", "c", "meta"))); + assertThat(values, is(Arrays.asList("A", "B", "C", "meta1"))); } @Test - public void getRepeatableWithMissingAttributeAliasDeclaration() throws Exception { + public void getRepeatableAnnotationsDeclaredOnClassWithMissingAttributeAliasDeclaration() throws Exception { exception.expect(AnnotationConfigurationException.class); exception.expectMessage(containsString("Attribute [value] in")); exception.expectMessage(containsString(BrokenContextConfig.class.getName())); exception.expectMessage(containsString("must be declared as an @AliasFor [locations]")); - getRepeatableAnnotation(BrokenConfigHierarchyTestCase.class, BrokenHierarchy.class, BrokenContextConfig.class); + getRepeatableAnnotations(BrokenConfigHierarchyTestCase.class, BrokenHierarchy.class, BrokenContextConfig.class); } @Test - public void getRepeatableWithAttributeAliases() throws Exception { - Set annotations = getRepeatableAnnotation(ConfigHierarchyTestCase.class, Hierarchy.class, - ContextConfig.class); + public void getRepeatableAnnotationsDeclaredOnClassWithAttributeAliases() throws Exception { + final List expectedLocations = Arrays.asList("A", "B"); + + Set annotations = getRepeatableAnnotations(ConfigHierarchyTestCase.class, Hierarchy.class, ContextConfig.class); assertNotNull(annotations); List locations = annotations.stream().map(ContextConfig::locations).collect(toList()); - assertThat(locations, equalTo(Arrays.asList("A", "B"))); + assertThat(locations, is(expectedLocations)); List values = annotations.stream().map(ContextConfig::value).collect(toList()); - assertThat(values, equalTo(Arrays.asList("A", "B"))); + assertThat(values, is(expectedLocations)); + } + + @Test + public void getRepeatableAnnotationsDeclaredOnClass() { + final List expectedValuesJava = Arrays.asList("A", "B", "C"); + final List expectedValuesSpring = Arrays.asList("A", "B", "C", "meta1"); + + // Java 8 + MyRepeatable[] array = MyRepeatableClass.class.getAnnotationsByType(MyRepeatable.class); + assertNotNull(array); + List values = stream(array).map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesJava)); + + // Spring + Set set = getRepeatableAnnotations(MyRepeatableClass.class, MyRepeatableContainer.class, MyRepeatable.class); + assertNotNull(set); + values = set.stream().map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesSpring)); + } + + @Test + public void getRepeatableAnnotationsDeclaredOnSuperclass() { + final Class clazz = SubMyRepeatableClass.class; + final List expectedValuesJava = Arrays.asList("A", "B", "C"); + final List expectedValuesSpring = Arrays.asList("A", "B", "C", "meta1"); + + // Java 8 + MyRepeatable[] array = clazz.getAnnotationsByType(MyRepeatable.class); + assertNotNull(array); + List values = stream(array).map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesJava)); + + // Spring + Set set = getRepeatableAnnotations(clazz, MyRepeatableContainer.class, MyRepeatable.class); + assertNotNull(set); + values = set.stream().map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesSpring)); + } + + @Test + public void getRepeatableAnnotationsDeclaredOnClassAndSuperclass() { + final Class clazz = SubMyRepeatableWithAdditionalLocalDeclarationsClass.class; + final List expectedValuesJava = Arrays.asList("X", "Y", "Z"); + final List expectedValuesSpring = Arrays.asList("X", "Y", "Z", "meta2"); + + // Java 8 + MyRepeatable[] array = clazz.getAnnotationsByType(MyRepeatable.class); + assertNotNull(array); + List values = stream(array).map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesJava)); + + // Spring + Set set = getRepeatableAnnotations(clazz, MyRepeatableContainer.class, MyRepeatable.class); + assertNotNull(set); + values = set.stream().map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesSpring)); + } + + @Test + public void getDeclaredRepeatableAnnotationsDeclaredOnClass() { + final List expectedValuesJava = Arrays.asList("A", "B", "C"); + final List expectedValuesSpring = Arrays.asList("A", "B", "C", "meta1"); + + // Java 8 + MyRepeatable[] array = MyRepeatableClass.class.getDeclaredAnnotationsByType(MyRepeatable.class); + assertNotNull(array); + List values = stream(array).map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesJava)); + + // Spring + Set set = getDeclaredRepeatableAnnotations(MyRepeatableClass.class, MyRepeatableContainer.class, MyRepeatable.class); + assertNotNull(set); + values = set.stream().map(MyRepeatable::value).collect(toList()); + assertThat(values, is(expectedValuesSpring)); + } + + @Test + public void getDeclaredRepeatableAnnotationsDeclaredOnSuperclass() { + final Class clazz = SubMyRepeatableClass.class; + + // Java 8 + MyRepeatable[] array = clazz.getDeclaredAnnotationsByType(MyRepeatable.class); + assertNotNull(array); + assertThat(array.length, is(0)); + + // Spring + Set set = getDeclaredRepeatableAnnotations(clazz, MyRepeatableContainer.class, MyRepeatable.class); + assertNotNull(set); + assertThat(set.size(), is(0)); } @Test @@ -915,6 +1005,8 @@ public class AnnotationUtilsTests { @Test public void synthesizeAnnotationWithAttributeAliasesInNestedAnnotations() throws Exception { + List expectedLocations = Arrays.asList("A", "B"); + Hierarchy hierarchy = ConfigHierarchyTestCase.class.getAnnotation(Hierarchy.class); assertNotNull(hierarchy); Hierarchy synthesizedHierarchy = synthesizeAnnotation(hierarchy); @@ -927,14 +1019,16 @@ public class AnnotationUtilsTests { Arrays.stream(configs).allMatch(c -> c instanceof SynthesizedAnnotation)); List locations = Arrays.stream(configs).map(ContextConfig::locations).collect(toList()); - assertThat(locations, equalTo(Arrays.asList("A", "B"))); + assertThat(locations, is(expectedLocations)); List values = Arrays.stream(configs).map(ContextConfig::value).collect(toList()); - assertThat(values, equalTo(Arrays.asList("A", "B"))); + assertThat(values, is(expectedLocations)); } @Test public void synthesizeAnnotationWithArrayOfAnnotations() throws Exception { + List expectedLocations = Arrays.asList("A", "B"); + Hierarchy hierarchy = ConfigHierarchyTestCase.class.getAnnotation(Hierarchy.class); assertNotNull(hierarchy); Hierarchy synthesizedHierarchy = synthesizeAnnotation(hierarchy); @@ -945,7 +1039,7 @@ public class AnnotationUtilsTests { ContextConfig[] configs = synthesizedHierarchy.value(); List locations = Arrays.stream(configs).map(ContextConfig::locations).collect(toList()); - assertThat(locations, equalTo(Arrays.asList("A", "B"))); + assertThat(locations, is(expectedLocations)); // Alter array returned from synthesized annotation configs[0] = contextConfig; @@ -953,7 +1047,7 @@ public class AnnotationUtilsTests { // Re-retrieve the array from the synthesized annotation configs = synthesizedHierarchy.value(); List values = Arrays.stream(configs).map(ContextConfig::value).collect(toList()); - assertThat(values, equalTo(Arrays.asList("A", "B"))); + assertThat(values, is(expectedLocations)); } @Test @@ -1231,18 +1325,39 @@ public class AnnotationUtilsTests { @Retention(RetentionPolicy.RUNTIME) @Inherited - @MyRepeatable("meta") - @interface MyRepeatableMeta { + @MyRepeatable("meta1") + @interface MyRepeatableMeta1 { } - public interface InterfaceWithRepeated { + @Retention(RetentionPolicy.RUNTIME) + @Inherited + @MyRepeatable("meta2") + @interface MyRepeatableMeta2 { + } - @MyRepeatable("a") - @MyRepeatableContainer({ @MyRepeatable("b"), @MyRepeatable("c") }) - @MyRepeatableMeta + interface InterfaceWithRepeated { + + @MyRepeatable("A") + @MyRepeatableContainer({ @MyRepeatable("B"), @MyRepeatable("C") }) + @MyRepeatableMeta1 void foo(); } + @MyRepeatable("A") + @MyRepeatableContainer({ @MyRepeatable("B"), @MyRepeatable("C") }) + @MyRepeatableMeta1 + static class MyRepeatableClass { + } + + static class SubMyRepeatableClass extends MyRepeatableClass { + } + + @MyRepeatable("X") + @MyRepeatableContainer({ @MyRepeatable("Y"), @MyRepeatable("Z") }) + @MyRepeatableMeta2 + static class SubMyRepeatableWithAdditionalLocalDeclarationsClass extends MyRepeatableClass { + } + enum RequestMethod { GET, POST } diff --git a/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java b/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java index 2201edcca7..d6d6a5c8f8 100644 --- a/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java +++ b/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java @@ -123,10 +123,10 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen private void executeSqlScripts(TestContext testContext, ExecutionPhase executionPhase) throws Exception { boolean classLevel = false; - Set sqlAnnotations = AnnotationUtils.getRepeatableAnnotation(testContext.getTestMethod(), SqlGroup.class, + Set sqlAnnotations = AnnotationUtils.getRepeatableAnnotations(testContext.getTestMethod(), SqlGroup.class, Sql.class); if (sqlAnnotations.isEmpty()) { - sqlAnnotations = AnnotationUtils.getRepeatableAnnotation(testContext.getTestClass(), SqlGroup.class, + sqlAnnotations = AnnotationUtils.getRepeatableAnnotations(testContext.getTestClass(), SqlGroup.class, Sql.class); if (!sqlAnnotations.isEmpty()) { classLevel = true;