From 129c05bcff4bcd99b3139577debb2391a1dfe099 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Sat, 31 Mar 2018 15:12:02 +0200 Subject: [PATCH] Comprehensively cache annotated methods for interfaces and superclasses Issue: SPR-16675 --- .../annotation/AnnotatedElementUtils.java | 144 +++++++++--------- .../core/annotation/AnnotationUtils.java | 87 +++++++---- .../core/annotation/AnnotationUtilsTests.java | 27 +--- .../MapAnnotationAttributeExtractorTests.java | 45 +++--- 4 files changed, 158 insertions(+), 145 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 41b8303241..9e86e3f464 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 @@ -1050,17 +1050,48 @@ public class AnnotatedElementUtils { try { // Locally declared annotations (ignoring @Inherited) Annotation[] annotations = element.getDeclaredAnnotations(); - List aggregatedResults = (processor.aggregates() ? new ArrayList<>() : null); + if (annotations.length > 0) { + List aggregatedResults = (processor.aggregates() ? new ArrayList<>() : null); - // Search in local annotations - for (Annotation annotation : annotations) { - Class currentAnnotationType = annotation.annotationType(); - if (!AnnotationUtils.isInJavaLangAnnotationPackage(currentAnnotationType)) { - if (currentAnnotationType == annotationType || - currentAnnotationType.getName().equals(annotationName) || - processor.alwaysProcesses()) { - T result = processor.process(element, annotation, metaDepth); + // Search in local annotations + for (Annotation annotation : annotations) { + Class currentAnnotationType = annotation.annotationType(); + if (!AnnotationUtils.isInJavaLangAnnotationPackage(currentAnnotationType)) { + if (currentAnnotationType == annotationType || + currentAnnotationType.getName().equals(annotationName) || + processor.alwaysProcesses()) { + T result = processor.process(element, annotation, metaDepth); + if (result != null) { + if (aggregatedResults != null && metaDepth == 0) { + aggregatedResults.add(result); + } + else { + return result; + } + } + } + // Repeatable annotations in container? + else if (currentAnnotationType == containerType) { + for (Annotation contained : getRawAnnotationsFromContainer(element, annotation)) { + T result = processor.process(element, contained, metaDepth); + if (aggregatedResults != null && result != null) { + // No need to post-process since repeatable annotations within a + // container cannot be composed annotations. + aggregatedResults.add(result); + } + } + } + } + } + + // Recursively search in meta-annotations + for (Annotation annotation : annotations) { + Class currentAnnotationType = annotation.annotationType(); + if (hasSearchableMetaAnnotations(currentAnnotationType, annotationType, annotationName)) { + T result = searchWithFindSemantics(currentAnnotationType, annotationType, annotationName, + containerType, processor, visited, metaDepth + 1); if (result != null) { + processor.postProcess(currentAnnotationType, annotation, result); if (aggregatedResults != null && metaDepth == 0) { aggregatedResults.add(result); } @@ -1069,43 +1100,14 @@ public class AnnotatedElementUtils { } } } - // Repeatable annotations in container? - else if (currentAnnotationType == containerType) { - for (Annotation contained : getRawAnnotationsFromContainer(element, annotation)) { - T result = processor.process(element, contained, metaDepth); - if (aggregatedResults != null && result != null) { - // No need to post-process since repeatable annotations within a - // container cannot be composed annotations. - aggregatedResults.add(result); - } - } - } } - } - // Recursively search in meta-annotations - for (Annotation annotation : annotations) { - Class currentAnnotationType = annotation.annotationType(); - if (hasSearchableMetaAnnotations(currentAnnotationType, annotationType, annotationName)) { - T result = searchWithFindSemantics(currentAnnotationType, annotationType, annotationName, - containerType, processor, visited, metaDepth + 1); - if (result != null) { - processor.postProcess(currentAnnotationType, annotation, result); - if (aggregatedResults != null && metaDepth == 0) { - aggregatedResults.add(result); - } - else { - return result; - } - } + if (!CollectionUtils.isEmpty(aggregatedResults)) { + // Prepend to support top-down ordering within class hierarchies + processor.getAggregatedResults().addAll(0, aggregatedResults); } } - if (!CollectionUtils.isEmpty(aggregatedResults)) { - // Prepend to support top-down ordering within class hierarchies - processor.getAggregatedResults().addAll(0, aggregatedResults); - } - if (element instanceof Method) { Method method = (Method) element; T result; @@ -1123,8 +1125,8 @@ public class AnnotatedElementUtils { // Search on methods in interfaces declared locally Class[] ifcs = method.getDeclaringClass().getInterfaces(); if (ifcs.length > 0) { - result = searchOnInterfaces(method, annotationType, annotationName, containerType, - processor, visited, metaDepth, ifcs); + result = searchOnInterfaces(method, annotationType, annotationName, + containerType, processor, visited, metaDepth, ifcs); if (result != null) { return result; } @@ -1137,23 +1139,23 @@ public class AnnotatedElementUtils { if (clazz == null || Object.class == clazz) { break; } - - try { - Method equivalentMethod = clazz.getDeclaredMethod(method.getName(), method.getParameterTypes()); - Method resolvedEquivalentMethod = BridgeMethodResolver.findBridgedMethod(equivalentMethod); - result = searchWithFindSemantics(resolvedEquivalentMethod, annotationType, annotationName, - containerType, processor, visited, metaDepth); - if (result != null) { - return result; + Set annotatedMethods = AnnotationUtils.getAnnotatedMethodsInBaseType(clazz); + if (!annotatedMethods.isEmpty()) { + for (Method annotatedMethod : annotatedMethods) { + if (annotatedMethod.getName().equals(method.getName()) && + Arrays.equals(annotatedMethod.getParameterTypes(), method.getParameterTypes())) { + Method resolvedSuperMethod = BridgeMethodResolver.findBridgedMethod(annotatedMethod); + result = searchWithFindSemantics(resolvedSuperMethod, annotationType, annotationName, + containerType, processor, visited, metaDepth); + if (result != null) { + return result; + } + } } } - catch (NoSuchMethodException ex) { - // No equivalent method found - } - // Search on interfaces declared on superclass - result = searchOnInterfaces(method, annotationType, annotationName, containerType, processor, - visited, metaDepth, clazz.getInterfaces()); + result = searchOnInterfaces(method, annotationType, annotationName, + containerType, processor, visited, metaDepth, clazz.getInterfaces()); if (result != null) { return result; } @@ -1164,8 +1166,8 @@ public class AnnotatedElementUtils { // Search on interfaces for (Class ifc : clazz.getInterfaces()) { - T result = searchWithFindSemantics(ifc, annotationType, annotationName, containerType, - processor, visited, metaDepth); + T result = searchWithFindSemantics(ifc, annotationType, annotationName, + containerType, processor, visited, metaDepth); if (result != null) { return result; } @@ -1174,8 +1176,8 @@ public class AnnotatedElementUtils { // Search on superclass Class superclass = clazz.getSuperclass(); if (superclass != null && Object.class != superclass) { - T result = searchWithFindSemantics(superclass, annotationType, annotationName, containerType, - processor, visited, metaDepth); + T result = searchWithFindSemantics(superclass, annotationType, annotationName, + containerType, processor, visited, metaDepth); if (result != null) { return result; } @@ -1195,18 +1197,18 @@ public class AnnotatedElementUtils { Processor processor, Set visited, int metaDepth, Class[] ifcs) { for (Class ifc : ifcs) { - if (AnnotationUtils.isInterfaceWithAnnotatedMethods(ifc)) { - try { - Method equivalentMethod = ifc.getMethod(method.getName(), method.getParameterTypes()); - T result = searchWithFindSemantics(equivalentMethod, annotationType, annotationName, containerType, - processor, visited, metaDepth); - if (result != null) { - return result; + Set annotatedMethods = AnnotationUtils.getAnnotatedMethodsInBaseType(ifc); + if (!annotatedMethods.isEmpty()) { + for (Method annotatedMethod : annotatedMethods) { + if (annotatedMethod.getName().equals(method.getName()) && + Arrays.equals(annotatedMethod.getParameterTypes(), method.getParameterTypes())) { + T result = searchWithFindSemantics(annotatedMethod, annotationType, annotationName, + containerType, processor, visited, metaDepth); + if (result != null) { + return result; + } } } - catch (NoSuchMethodException ex) { - // Skip this interface - it doesn't have the 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 29b7d63c0a..238f11c407 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 @@ -26,6 +26,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashMap; @@ -122,7 +123,7 @@ public abstract class AnnotationUtils { private static final Map metaPresentCache = new ConcurrentReferenceHashMap<>(256); - private static final Map, Boolean> annotatedInterfaceCache = + private static final Map, Set> annotatedBaseTypeCache = new ConcurrentReferenceHashMap<>(256); private static final Map, Boolean> synthesizableCache = @@ -539,7 +540,6 @@ public abstract class AnnotationUtils { if (result == null) { Method resolvedMethod = BridgeMethodResolver.findBridgedMethod(method); result = findAnnotation((AnnotatedElement) resolvedMethod, annotationType); - if (result == null) { result = searchOnInterfaces(method, annotationType, method.getDeclaringClass().getInterfaces()); } @@ -574,48 +574,63 @@ public abstract class AnnotationUtils { @Nullable private static A searchOnInterfaces(Method method, Class annotationType, Class... ifcs) { - A annotation = null; for (Class ifc : ifcs) { - if (isInterfaceWithAnnotatedMethods(ifc)) { - try { - Method equivalentMethod = ifc.getMethod(method.getName(), method.getParameterTypes()); - annotation = getAnnotation(equivalentMethod, annotationType); - } - catch (NoSuchMethodException ex) { - // Skip this interface - it doesn't have the method... - } - if (annotation != null) { - break; + Set annotatedMethods = getAnnotatedMethodsInBaseType(ifc); + if (!annotatedMethods.isEmpty()) { + for (Method annotatedMethod : annotatedMethods) { + if (annotatedMethod.getName().equals(method.getName()) && + Arrays.equals(annotatedMethod.getParameterTypes(), method.getParameterTypes())) { + A annotation = getAnnotation(annotatedMethod, annotationType); + if (annotation != null) { + return annotation; + } + } } } } - return annotation; + return null; } - static boolean isInterfaceWithAnnotatedMethods(Class ifc) { - if (ClassUtils.isJavaLanguageInterface(ifc)) { - return false; + static Set getAnnotatedMethodsInBaseType(Class baseType) { + if (ClassUtils.isJavaLanguageInterface(baseType)) { + return Collections.emptySet(); } - Boolean found = annotatedInterfaceCache.get(ifc); - if (found != null) { - return found; + Set annotatedMethods = annotatedBaseTypeCache.get(baseType); + if (annotatedMethods != null) { + return annotatedMethods; } - found = Boolean.FALSE; - for (Method ifcMethod : ifc.getMethods()) { + Method[] methods = (baseType.isInterface() ? baseType.getMethods() : baseType.getDeclaredMethods()); + for (Method baseMethod : methods) { try { - Annotation[] anns = ifcMethod.getAnnotations(); - if (anns.length > 1 || (anns.length == 1 && anns[0].annotationType() != Nullable.class)) { - found = Boolean.TRUE; - break; + if (hasSearchableAnnotations(baseMethod)) { + if (annotatedMethods == null) { + annotatedMethods = new HashSet<>(); + } + annotatedMethods.add(baseMethod); } } catch (Throwable ex) { - handleIntrospectionFailure(ifcMethod, ex); + handleIntrospectionFailure(baseMethod, ex); } } - annotatedInterfaceCache.put(ifc, found); - return found; + if (annotatedMethods == null) { + annotatedMethods = Collections.emptySet(); + } + annotatedBaseTypeCache.put(baseType, annotatedMethods); + return annotatedMethods; + } + + private static boolean hasSearchableAnnotations(Method ifcMethod) { + Annotation[] anns = ifcMethod.getAnnotations(); + if (anns.length == 0) { + return false; + } + if (anns.length == 1) { + Class annType = anns[0].annotationType(); + return (annType != Nullable.class && annType != Deprecated.class); + } + return true; } /** @@ -1875,6 +1890,20 @@ public abstract class AnnotationUtils { } } + /** + * Clear the internal annotation metadata cache. + * @since 4.3.15 + */ + public static void clearCache() { + findAnnotationCache.clear(); + metaPresentCache.clear(); + annotatedBaseTypeCache.clear(); + synthesizableCache.clear(); + attributeAliasesCache.clear(); + attributeMethodsCache.clear(); + aliasDescriptorCache.clear(); + } + /** * Cache key for the AnnotatedElement cache. 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 1d720552ea..9bc555947d 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 @@ -22,7 +22,6 @@ import java.lang.annotation.Repeatable; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.Collections; import java.util.List; @@ -39,7 +38,6 @@ import org.springframework.core.annotation.subpackage.NonPublicAnnotatedClass; import org.springframework.lang.Nullable; import org.springframework.stereotype.Component; import org.springframework.util.ClassUtils; -import org.springframework.util.ReflectionUtils; import static java.util.Arrays.*; import static java.util.stream.Collectors.*; @@ -64,23 +62,8 @@ public class AnnotationUtilsTests { @Before - public void clearCachesBeforeTests() { - clearCaches(); - } - - static void clearCaches() { - clearCache("findAnnotationCache", "annotatedInterfaceCache", "metaPresentCache", "synthesizableCache", - "attributeAliasesCache", "attributeMethodsCache", "aliasDescriptorCache"); - } - - static void clearCache(String... cacheNames) { - stream(cacheNames).forEach(cacheName -> getCache(cacheName).clear()); - } - - static Map getCache(String cacheName) { - Field field = ReflectionUtils.findField(AnnotationUtils.class, cacheName); - ReflectionUtils.makeAccessible(field); - return (Map) ReflectionUtils.getField(field, null); + public void clearCacheBeforeTests() { + AnnotationUtils.clearCache(); } @@ -1544,9 +1527,9 @@ public class AnnotationUtilsTests { @Test public void interfaceWithAnnotatedMethods() { - assertFalse(AnnotationUtils.isInterfaceWithAnnotatedMethods(NonAnnotatedInterface.class)); - assertTrue(AnnotationUtils.isInterfaceWithAnnotatedMethods(AnnotatedInterface.class)); - assertFalse(AnnotationUtils.isInterfaceWithAnnotatedMethods(NullableAnnotatedInterface.class)); + assertTrue(AnnotationUtils.getAnnotatedMethodsInBaseType(NonAnnotatedInterface.class).isEmpty()); + assertFalse(AnnotationUtils.getAnnotatedMethodsInBaseType(AnnotatedInterface.class).isEmpty()); + assertTrue(AnnotationUtils.getAnnotatedMethodsInBaseType(NullableAnnotatedInterface.class).isEmpty()); } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/MapAnnotationAttributeExtractorTests.java b/spring-core/src/test/java/org/springframework/core/annotation/MapAnnotationAttributeExtractorTests.java index 9470bf1636..3fa9039637 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/MapAnnotationAttributeExtractorTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/MapAnnotationAttributeExtractorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ package org.springframework.core.annotation; import java.lang.annotation.Annotation; +import java.lang.reflect.Field; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; @@ -41,13 +42,20 @@ import static org.springframework.core.annotation.AnnotationUtilsTests.*; @SuppressWarnings("serial") public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnnotationAttributeExtractorTestCase { - @Before - public void clearCachesBeforeTests() { - AnnotationUtilsTests.clearCaches(); + @Override + protected AnnotationAttributeExtractor createExtractorFor(Class clazz, String expected, Class annotationType) { + Map attributes = Collections.singletonMap(expected, expected); + return new MapAnnotationAttributeExtractor(attributes, annotationType, clazz); } + @Before + public void clearCacheBeforeTests() { + AnnotationUtils.clearCache(); + } + + @Test - public void enrichAndValidateAttributesWithImplicitAliasesAndMinimalAttributes() { + public void enrichAndValidateAttributesWithImplicitAliasesAndMinimalAttributes() throws Exception { Map attributes = new HashMap<>(); Map expectedAttributes = new HashMap() {{ put("groovyScript", ""); @@ -64,7 +72,7 @@ public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnno } @Test - public void enrichAndValidateAttributesWithImplicitAliases() { + public void enrichAndValidateAttributesWithImplicitAliases() throws Exception { Map attributes = new HashMap() {{ put("groovyScript", "groovy!"); }}; @@ -85,7 +93,6 @@ public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnno @Test public void enrichAndValidateAttributesWithSingleElementThatOverridesAnArray() { - // @formatter:off Map attributes = new HashMap() {{ // Intentionally storing 'value' as a single String instead of an array. // put("value", asArray("/foo")); @@ -99,7 +106,6 @@ public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnno put("name", "test"); put("method", new RequestMethod[0]); }}; - // @formatter:on MapAnnotationAttributeExtractor extractor = new MapAnnotationAttributeExtractor(attributes, WebMapping.class, null); Map enriched = extractor.getSource(); @@ -109,18 +115,17 @@ public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnno } @SuppressWarnings("unchecked") - private void assertEnrichAndValidateAttributes(Map sourceAttributes, Map expected) { + private void assertEnrichAndValidateAttributes(Map sourceAttributes, Map expected) throws Exception { Class annotationType = ImplicitAliasesContextConfig.class; - // Since the ordering of attribute methods returned by the JVM is - // non-deterministic, we have to rig the attributeAliasesCache in AnnotationUtils - // so that the tests consistently fail in case enrichAndValidateAttributes() is - // buggy. - // - // Otherwise, these tests would intermittently pass even for an invalid - // implementation. + // Since the ordering of attribute methods returned by the JVM is non-deterministic, + // we have to rig the attributeAliasesCache in AnnotationUtils so that the tests + // consistently fail in case enrichAndValidateAttributes() is buggy. + // Otherwise, these tests would intermittently pass even for an invalid implementation. + Field cacheField = AnnotationUtils.class.getDeclaredField("attributeAliasesCache"); + cacheField.setAccessible(true); Map, MultiValueMap> attributeAliasesCache = - (Map, MultiValueMap>) AnnotationUtilsTests.getCache("attributeAliasesCache"); + (Map, MultiValueMap>) cacheField.get(null); // Declare aliases in an order that will cause enrichAndValidateAttributes() to // fail unless it considers all aliases in the set of implicit aliases. @@ -141,10 +146,4 @@ public class MapAnnotationAttributeExtractorTests extends AbstractAliasAwareAnno expected.forEach((attr, expectedValue) -> assertThat("for attribute '" + attr + "'", enriched.get(attr), is(expectedValue))); } - @Override - protected AnnotationAttributeExtractor createExtractorFor(Class clazz, String expected, Class annotationType) { - Map attributes = Collections.singletonMap(expected, expected); - return new MapAnnotationAttributeExtractor(attributes, annotationType, clazz); - } - }