From 1d30bf83a0d6f303ce38526286b90172c3c596c6 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 26 Feb 2014 02:29:30 +0100 Subject: [PATCH] Favor 'local' annotations over inherited ones Prior to this commit, the implementations of findAnnotation() in AnnotationUtils and getAnnotationAttributes() in AnnotatedElementUtils favored inherited annotations and inherited composed annotations over composed annotations that are declared closer to the starting class passed to these methods. This commit addresses this issue as follows: - Refactored AnnotationUtils to use getDeclaredAnnotation() and getDeclaredAnnotations() instead of getAnnotation() and getAnnotations() where appropriate. - AnnotatedElementUtils.doProcess() supports a traverseClassHierarchy flag to control whether the class hierarchy should be traversed, using getDeclaredAnnotations() instead of getAnnotations() if the flag is true. - Overhauled Javadoc in AnnotatedElementUtils. Issue: SPR-11475 --- .../annotation/AnnotatedElementUtils.java | 157 +++++++++++++----- .../core/annotation/AnnotationUtils.java | 4 +- .../AnnotatedElementUtilsTests.java | 115 ++++++++++--- .../core/annotation/AnnotationUtilsTests.java | 20 +-- 4 files changed, 217 insertions(+), 79 deletions(-) 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 75966d98ee2..8494c0f51d2 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 @@ -26,6 +26,8 @@ import java.util.Set; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import static org.springframework.core.annotation.AnnotationUtils.*; + /** * Utility class used to collect all annotation values including those declared on * meta-annotations. @@ -39,14 +41,16 @@ public class AnnotatedElementUtils { public static Set getMetaAnnotationTypes(AnnotatedElement element, String annotationType) { final Set types = new LinkedHashSet(); - process(element, annotationType, new Processor() { + process(element, annotationType, true, new Processor() { + @Override - public Object process(Annotation annotation, int depth) { - if (depth > 0) { + public Object process(Annotation annotation, int metaDepth) { + if (metaDepth > 0) { types.add(annotation.annotationType().getName()); } return null; } + @Override public void postProcess(Annotation annotation, Object result) { } @@ -55,14 +59,16 @@ public class AnnotatedElementUtils { } public static boolean hasMetaAnnotationTypes(AnnotatedElement element, String annotationType) { - return Boolean.TRUE.equals(process(element, annotationType, new Processor() { + return Boolean.TRUE.equals(process(element, annotationType, true, new Processor() { + @Override - public Boolean process(Annotation annotation, int depth) { - if (depth > 0) { - return true; + public Boolean process(Annotation annotation, int metaDepth) { + if (metaDepth > 0) { + return Boolean.TRUE; } return null; } + @Override public void postProcess(Annotation annotation, Boolean result) { } @@ -70,11 +76,13 @@ public class AnnotatedElementUtils { } public static boolean isAnnotated(AnnotatedElement element, String annotationType) { - return Boolean.TRUE.equals(process(element, annotationType, new Processor() { + return Boolean.TRUE.equals(process(element, annotationType, true, new Processor() { + @Override - public Boolean process(Annotation annotation, int depth) { - return true; + public Boolean process(Annotation annotation, int metaDepth) { + return Boolean.TRUE; } + @Override public void postProcess(Annotation annotation, Boolean result) { } @@ -85,20 +93,21 @@ public class AnnotatedElementUtils { return getAnnotationAttributes(element, annotationType, false, false); } - public static AnnotationAttributes getAnnotationAttributes( - AnnotatedElement element, String annotationType, + public static AnnotationAttributes getAnnotationAttributes(AnnotatedElement element, String annotationType, final boolean classValuesAsString, final boolean nestedAnnotationsAsMap) { - return process(element, annotationType, new Processor() { + return process(element, annotationType, true, new Processor() { + @Override - public AnnotationAttributes process(Annotation annotation, int depth) { + public AnnotationAttributes process(Annotation annotation, int metaDepth) { return AnnotationUtils.getAnnotationAttributes(annotation, classValuesAsString, nestedAnnotationsAsMap); } + @Override public void postProcess(Annotation annotation, AnnotationAttributes result) { for (String key : result.keySet()) { - if (!"value".equals(key)) { - Object value = AnnotationUtils.getValue(annotation, key); + if (!VALUE.equals(key)) { + Object value = getValue(annotation, key); if (value != null) { result.put(key, value); } @@ -108,28 +117,33 @@ public class AnnotatedElementUtils { }); } - public static MultiValueMap getAllAnnotationAttributes( - AnnotatedElement element, final String annotationType, - final boolean classValuesAsString, final boolean nestedAnnotationsAsMap) { + public static MultiValueMap getAllAnnotationAttributes(AnnotatedElement element, + final String annotationType) { + return getAllAnnotationAttributes(element, annotationType, false, false); + } + + public static MultiValueMap getAllAnnotationAttributes(AnnotatedElement element, + final String annotationType, final boolean classValuesAsString, final boolean nestedAnnotationsAsMap) { final MultiValueMap attributes = new LinkedMultiValueMap(); - process(element, annotationType, new Processor() { + process(element, annotationType, false, new Processor() { + @Override - public Void process(Annotation annotation, int depth) { + public Void process(Annotation annotation, int metaDepth) { if (annotation.annotationType().getName().equals(annotationType)) { - for (Map.Entry entry : - AnnotationUtils.getAnnotationAttributes( - annotation, classValuesAsString, nestedAnnotationsAsMap).entrySet()) { + for (Map.Entry entry : AnnotationUtils.getAnnotationAttributes(annotation, + classValuesAsString, nestedAnnotationsAsMap).entrySet()) { attributes.add(entry.getKey(), entry.getValue()); } } return null; } + @Override public void postProcess(Annotation annotation, Void result) { for (String key : attributes.keySet()) { - if (!"value".equals(key)) { - Object value = AnnotationUtils.getValue(annotation, key); + if (!VALUE.equals(key)) { + Object value = getValue(annotation, key); if (value != null) { attributes.add(key, value); } @@ -141,45 +155,91 @@ public class AnnotatedElementUtils { } /** - * Process all annotations of the specified annotation type and recursively all - * meta-annotations on the specified type. + * Process all annotations of the specified {@code annotationType} and + * recursively all meta-annotations on the specified {@code element}. + * + *

If the {@code traverseClassHierarchy} flag is {@code true} and the sought + * annotation is neither directly present on the given element nor + * present on the given element as a meta-annotation, then the algorithm will + * recursively search through the class hierarchy of the given element. + * * @param element the annotated element - * @param annotationType the annotation type to find. Only items of the specified - * type or meta-annotations of the specified type will be processed. - * @param processor the processor + * @param annotationType the annotation type to find + * @param traverseClassHierarchy whether or not to traverse up the class + * hierarchy recursively + * @param processor the processor to delegate to * @return the result of the processor */ - private static T process(AnnotatedElement element, String annotationType, Processor processor) { - return doProcess(element, annotationType, processor, new HashSet(), 0); + private static T process(AnnotatedElement element, String annotationType, boolean traverseClassHierarchy, + Processor processor) { + return doProcess(element, annotationType, traverseClassHierarchy, processor, new HashSet(), 0); } - private static T doProcess(AnnotatedElement element, String annotationType, - Processor processor, Set visited, int depth) { + /** + * Perform the search algorithm for the {@link #process} method, avoiding + * endless recursion by tracking which annotated elements have already been + * visited. + * + *

The {@code metaDepth} parameter represents the depth of the annotation + * relative to the initial element. For example, an annotation that is + * present on the element will have a depth of 0; a meta-annotation + * will have a depth of 1; and a meta-meta-annotation will have a depth of 2. + * + * @param element the annotated element + * @param annotationType the annotation type to find + * @param traverseClassHierarchy whether or not to traverse up the class + * hierarchy recursively + * @param processor the processor to delegate to + * @param visited the set of annotated elements that have already been visited + * @param metaDepth the depth of the annotation relative to the initial element + * @return the result of the processor + */ + private static T doProcess(AnnotatedElement element, String annotationType, boolean traverseClassHierarchy, + Processor processor, Set visited, int metaDepth) { if (visited.add(element)) { - for (Annotation annotation : element.getAnnotations()) { - if (annotation.annotationType().getName().equals(annotationType) || depth > 0) { - T result = processor.process(annotation, depth); + + Annotation[] annotations = traverseClassHierarchy ? element.getDeclaredAnnotations() + : element.getAnnotations(); + + for (Annotation annotation : annotations) { + if (annotation.annotationType().getName().equals(annotationType) || metaDepth > 0) { + T result = processor.process(annotation, metaDepth); if (result != null) { return result; } - result = doProcess(annotation.annotationType(), annotationType, processor, visited, depth + 1); + result = doProcess(annotation.annotationType(), annotationType, traverseClassHierarchy, processor, + visited, metaDepth + 1); if (result != null) { processor.postProcess(annotation, result); return result; } } } - for (Annotation annotation : element.getAnnotations()) { - if (!AnnotationUtils.isInJavaLangAnnotationPackage(annotation)) { - T result = doProcess(annotation.annotationType(), annotationType, processor, visited, depth); + + for (Annotation annotation : annotations) { + if (!isInJavaLangAnnotationPackage(annotation)) { + T result = doProcess(annotation.annotationType(), annotationType, traverseClassHierarchy, + processor, visited, metaDepth); if (result != null) { processor.postProcess(annotation, result); return result; } } } + + if (traverseClassHierarchy && element instanceof Class) { + Class superclass = ((Class) element).getSuperclass(); + if (superclass != null && !superclass.equals(Object.class)) { + T result = doProcess(superclass, annotationType, traverseClassHierarchy, processor, visited, + metaDepth); + if (result != null) { + return result; + } + } + } } + return null; } @@ -192,13 +252,18 @@ public class AnnotatedElementUtils { /** * Called to process the annotation. + * + *

The {@code metaDepth} parameter represents the depth of the + * annotation relative to the initial element. For example, an annotation + * that is present on the element will have a depth of 0; a + * meta-annotation will have a depth of 1; and a meta-meta-annotation + * will have a depth of 2. + * * @param annotation the annotation to process - * @param depth the depth of the annotation relative to the initial match. - * For example, a matched annotation will have a depth of 0, a meta-annotation - * 1 and a meta-meta-annotation 2 + * @param metaDepth the depth of the annotation relative to the initial element * @return the result of the processing or {@code null} to continue */ - T process(Annotation annotation, int depth); + T process(Annotation annotation, int metaDepth); void postProcess(Annotation annotation, T result); } 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 d3e51b7c639..3061648a25e 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 @@ -278,7 +278,7 @@ public abstract class AnnotationUtils { Set visited) { Assert.notNull(clazz, "Class must not be null"); - A annotation = clazz.getAnnotation(annotationType); + A annotation = clazz.getDeclaredAnnotation(annotationType); if (annotation != null) { return annotation; } @@ -288,7 +288,7 @@ public abstract class AnnotationUtils { return annotation; } } - for (Annotation ann : clazz.getAnnotations()) { + for (Annotation ann : clazz.getDeclaredAnnotations()) { if (!isInJavaLangAnnotationPackage(ann) && visited.add(ann)) { annotation = findAnnotation(ann.annotationType(), annotationType, visited); if (annotation != null) { 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 4b8df68c962..2aa3a0abd3b 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 @@ -22,10 +22,13 @@ import java.lang.annotation.Inherited; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.util.Arrays; import org.junit.Test; +import org.springframework.util.MultiValueMap; import static org.junit.Assert.*; +import static org.springframework.core.annotation.AnnotatedElementUtils.*; /** * Unit tests for {@link AnnotatedElementUtils}. @@ -35,37 +38,82 @@ import static org.junit.Assert.*; */ public class AnnotatedElementUtilsTests { + @Test + public void getAllAnnotationAttributesOnClassWithLocalAnnotation() { + MultiValueMap attributes = getAllAnnotationAttributes(TxConfig.class, + Transactional.class.getName()); + assertNotNull("Annotation attributes map for @Transactional on TxConfig", attributes); + assertEquals("value for TxConfig.", Arrays.asList("TxConfig"), attributes.get("value")); + } + + /** + * If the "value" entry contains both "DerivedTxConfig" AND "TxConfig", then + * the algorithm is accidentally picking up shadowed annotations of the same + * type within the class hierarchy. Such undesirable behavior would cause the + * logic in {@link org.springframework.context.annotation.ProfileCondition} + * to fail. + * + * @see org.springframework.core.env.EnvironmentIntegrationTests#mostSpecificDerivedClassDrivesEnvironment_withDevEnvAndDerivedDevConfigClass + */ + @Test + public void getAllAnnotationAttributesOnClassWithLocalAnnotationThatShadowsAnnotationFromSuperclass() { + MultiValueMap attributes = getAllAnnotationAttributes(DerivedTxConfig.class, + Transactional.class.getName()); + assertNotNull("Annotation attributes map for @Transactional on DerivedTxConfig", attributes); + assertEquals("value for DerivedTxConfig.", Arrays.asList("DerivedTxConfig"), attributes.get("value")); + } + + /** + * Note: this functionality is required by {@link org.springframework.context.annotation.ProfileCondition}. + * + * @see org.springframework.core.env.EnvironmentIntegrationTests + */ + @Test + public void getAllAnnotationAttributesOnClassWithMultipleComposedAnnotations() { + MultiValueMap attributes = getAllAnnotationAttributes(TxFromMultipleComposedAnnotations.class, + Transactional.class.getName()); + assertNotNull("Annotation attributes map for @Transactional on TxFromMultipleComposedAnnotations", attributes); + assertEquals("value for TxFromMultipleComposedAnnotations.", Arrays.asList("TxComposed1", "TxComposed2"), + attributes.get("value")); + } + + @Test + public void getAnnotationAttributesOnClassWithLocalAnnotation() { + AnnotationAttributes attributes = getAnnotationAttributes(TxConfig.class, Transactional.class.getName()); + assertNotNull("Annotation attributes for @Transactional on TxConfig", attributes); + assertEquals("value for TxConfig.", "TxConfig", attributes.getString("value")); + } + + @Test + public void getAnnotationAttributesOnClassWithLocalAnnotationThatShadowsAnnotationFromSuperclass() { + AnnotationAttributes attributes = getAnnotationAttributes(DerivedTxConfig.class, Transactional.class.getName()); + assertNotNull("Annotation attributes for @Transactional on DerivedTxConfig", attributes); + assertEquals("value for DerivedTxConfig.", "DerivedTxConfig", attributes.getString("value")); + } + @Test public void getAnnotationAttributesOnMetaCycleAnnotatedClassWithMissingTargetMetaAnnotation() { - AnnotationAttributes attributes = AnnotatedElementUtils.getAnnotationAttributes(MetaCycleAnnotatedClass.class, + AnnotationAttributes attributes = getAnnotationAttributes(MetaCycleAnnotatedClass.class, Transactional.class.getName()); assertNull("Should not find annotation attributes for @Transactional on MetaCycleAnnotatedClass", attributes); } @Test public void getAnnotationAttributesFavorsInheritedAnnotationsOverMoreLocallyDeclaredComposedAnnotations() { - AnnotationAttributes attributes = AnnotatedElementUtils.getAnnotationAttributes( - SubSubClassWithInheritedAnnotation.class, Transactional.class.getName()); - assertNotNull(attributes); - - // By inspecting SubSubClassWithInheritedAnnotation, one might expect that the - // readOnly flag should be true, since the immediate superclass is annotated with - // @Composed2; however, with the current implementation the readOnly flag will be - // false since @Transactional is declared as @Inherited. - assertFalse("readOnly flag for SubSubClassWithInheritedAnnotation", attributes.getBoolean("readOnly")); + AnnotationAttributes attributes = getAnnotationAttributes(SubSubClassWithInheritedAnnotation.class, + Transactional.class.getName()); + assertNotNull("AnnotationAttributes for @Transactional on SubSubClassWithInheritedAnnotation", attributes); + assertEquals("readOnly flag for SubSubClassWithInheritedAnnotation.", true, attributes.getBoolean("readOnly")); } @Test public void getAnnotationAttributesFavorsInheritedComposedAnnotationsOverMoreLocallyDeclaredComposedAnnotations() { - AnnotationAttributes attributes = AnnotatedElementUtils.getAnnotationAttributes( - SubSubClassWithInheritedComposedAnnotation.class, Transactional.class.getName()); - assertNotNull(attributes); - - // By inspecting SubSubClassWithInheritedComposedAnnotation, one might expect that - // the readOnly flag should be true, since the immediate superclass is annotated - // with @Composed2; however, with the current implementation the readOnly flag - // will be false since @Composed1 is declared as @Inherited. - assertFalse("readOnly flag", attributes.getBoolean("readOnly")); + AnnotationAttributes attributes = getAnnotationAttributes(SubSubClassWithInheritedComposedAnnotation.class, + Transactional.class.getName()); + assertNotNull("AnnotationAttributtes for @Transactional on SubSubClassWithInheritedComposedAnnotation.", + attributes); + assertEquals("readOnly flag for SubSubClassWithInheritedComposedAnnotation.", true, + attributes.getBoolean("readOnly")); } @@ -96,12 +144,16 @@ public class AnnotatedElementUtilsTests { static class MetaCycleAnnotatedClass { } + // ------------------------------------------------------------------------- + @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.TYPE) @Documented @Inherited @interface Transactional { + String value() default ""; + boolean readOnly() default false; } @@ -120,6 +172,18 @@ public class AnnotatedElementUtilsTests { @interface Composed2 { } + @Transactional("TxComposed1") + @Retention(RetentionPolicy.RUNTIME) + @interface TxComposed1 { + } + + @Transactional("TxComposed2") + @Retention(RetentionPolicy.RUNTIME) + @interface TxComposed2 { + } + + // ------------------------------------------------------------------------- + @Transactional static class ClassWithInheritedAnnotation { } @@ -142,4 +206,17 @@ public class AnnotatedElementUtilsTests { static class SubSubClassWithInheritedComposedAnnotation extends SubClassWithInheritedComposedAnnotation { } + @Transactional("TxConfig") + static class TxConfig { + } + + @Transactional("DerivedTxConfig") + static class DerivedTxConfig extends TxConfig { + } + + @TxComposed1 + @TxComposed2 + static class TxFromMultipleComposedAnnotations { + } + } 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 e613a619314..a03d4085e80 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 @@ -111,30 +111,26 @@ public class AnnotationUtilsTests { assertEquals("meta1", component.value()); } + /** + * @since 4.0.3 + */ @Test public void findAnnotationFavorsInheritedAnnotationsOverMoreLocallyDeclaredComposedAnnotations() { Transactional transactional = AnnotationUtils.findAnnotation(SubSubClassWithInheritedAnnotation.class, Transactional.class); assertNotNull(transactional); - - // By inspecting SubSubClassWithInheritedAnnotation, one might expect that the - // readOnly flag should be true, since the immediate superclass is annotated with - // @Composed2; however, with the current implementation the readOnly flag will be - // false since @Transactional is declared as @Inherited. - assertFalse("readOnly flag for SubSubClassWithInheritedAnnotation", transactional.readOnly()); + assertTrue("readOnly flag for SubSubClassWithInheritedAnnotation", transactional.readOnly()); } + /** + * @since 4.0.3 + */ @Test public void findAnnotationFavorsInheritedComposedAnnotationsOverMoreLocallyDeclaredComposedAnnotations() { Component component = AnnotationUtils.findAnnotation(SubSubClassWithInheritedMetaAnnotation.class, Component.class); assertNotNull(component); - - // By inspecting SubSubClassWithInheritedMetaAnnotation, one might expect that - // "meta2" should be found, since the immediate superclass is annotated with - // @Meta2; however, with the current implementation "meta1" will be found since - // @Meta1 is declared as @Inherited. - assertEquals("meta1", component.value()); + assertEquals("meta2", component.value()); } @Test