From 9333ed22f6ea6fae9909f6b6e6aa317c1b45ce82 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Fri, 4 Aug 2023 00:47:04 +0200 Subject: [PATCH 1/2] Avoid repeated FactoryBean targetType check See gh-30987 --- .../AbstractAutowireCapableBeanFactory.java | 33 ++++++++----------- .../factory/support/AbstractBeanFactory.java | 20 +---------- .../support/FactoryBeanRegistrySupport.java | 30 +++++++++++++++++ 3 files changed, 45 insertions(+), 38 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index 667969ed83d..7de716b44c9 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -837,16 +837,13 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac return result; } - ResolvableType beanType = - (mbd.hasBeanClass() ? ResolvableType.forClass(mbd.getBeanClass()) : ResolvableType.NONE); - - // For instance supplied beans try the target type and bean class + // For instance supplied beans, try the target type and bean class immediately if (mbd.getInstanceSupplier() != null) { result = getFactoryBeanGeneric(mbd.targetType); if (result.resolve() != null) { return result; } - result = getFactoryBeanGeneric(beanType); + result = getFactoryBeanGeneric(mbd.hasBeanClass() ? ResolvableType.forClass(mbd.getBeanClass()) : null); if (result.resolve() != null) { return result; } @@ -909,22 +906,20 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac return getTypeForFactoryBeanFromMethod(mbd.getBeanClass(), factoryMethodName); } - result = getFactoryBeanGeneric(mbd.targetType); - if (result.resolve() != null) { - return result; - } - result = getFactoryBeanGeneric(beanType); - if (result.resolve() != null) { - return result; + // For regular beans, try the target type and bean class as fallback + if (mbd.getInstanceSupplier() == null) { + result = getFactoryBeanGeneric(mbd.targetType); + if (result.resolve() != null) { + return result; + } + result = getFactoryBeanGeneric(mbd.hasBeanClass() ? ResolvableType.forClass(mbd.getBeanClass()) : null); + if (result.resolve() != null) { + return result; + } } - return ResolvableType.NONE; - } - private ResolvableType getFactoryBeanGeneric(@Nullable ResolvableType type) { - if (type == null) { - return ResolvableType.NONE; - } - return type.as(FactoryBean.class).getGeneric(); + // FactoryBean type not resolvable + return ResolvableType.NONE; } /** diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java index c0f9bb58689..57b9b8dffad 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractBeanFactory.java @@ -64,7 +64,6 @@ import org.springframework.beans.factory.config.DestructionAwareBeanPostProcesso import org.springframework.beans.factory.config.InstantiationAwareBeanPostProcessor; import org.springframework.beans.factory.config.Scope; import org.springframework.beans.factory.config.SmartInstantiationAwareBeanPostProcessor; -import org.springframework.core.AttributeAccessor; import org.springframework.core.DecoratingClassLoader; import org.springframework.core.NamedThreadLocal; import org.springframework.core.ResolvableType; @@ -1684,25 +1683,8 @@ public abstract class AbstractBeanFactory extends FactoryBeanRegistrySupport imp onSuppressedException(ex); } } - return ResolvableType.NONE; - } - /** - * Determine the bean type for a FactoryBean by inspecting its attributes for a - * {@link FactoryBean#OBJECT_TYPE_ATTRIBUTE} value. - * @param attributes the attributes to inspect - * @return a {@link ResolvableType} extracted from the attributes or - * {@code ResolvableType.NONE} - * @since 5.2 - */ - ResolvableType getTypeForFactoryBeanFromAttributes(AttributeAccessor attributes) { - Object attribute = attributes.getAttribute(FactoryBean.OBJECT_TYPE_ATTRIBUTE); - if (attribute instanceof ResolvableType resolvableType) { - return resolvableType; - } - if (attribute instanceof Class clazz) { - return ResolvableType.forClass(clazz); - } + // FactoryBean type not resolvable return ResolvableType.NONE; } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java index bfe58203905..72644917ea5 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/FactoryBeanRegistrySupport.java @@ -24,6 +24,8 @@ import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.BeanCurrentlyInCreationException; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.FactoryBeanNotInitializedException; +import org.springframework.core.AttributeAccessor; +import org.springframework.core.ResolvableType; import org.springframework.lang.Nullable; /** @@ -61,6 +63,34 @@ public abstract class FactoryBeanRegistrySupport extends DefaultSingletonBeanReg } } + /** + * Determine the bean type for a FactoryBean by inspecting its attributes for a + * {@link FactoryBean#OBJECT_TYPE_ATTRIBUTE} value. + * @param attributes the attributes to inspect + * @return a {@link ResolvableType} extracted from the attributes or + * {@code ResolvableType.NONE} + * @since 5.2 + */ + ResolvableType getTypeForFactoryBeanFromAttributes(AttributeAccessor attributes) { + Object attribute = attributes.getAttribute(FactoryBean.OBJECT_TYPE_ATTRIBUTE); + if (attribute instanceof ResolvableType resolvableType) { + return resolvableType; + } + if (attribute instanceof Class clazz) { + return ResolvableType.forClass(clazz); + } + return ResolvableType.NONE; + } + + /** + * Determine the FactoryBean object type from the given generic declaration. + * @param type the FactoryBean type + * @return the nested object type, or {@code NONE} if not resolvable + */ + ResolvableType getFactoryBeanGeneric(@Nullable ResolvableType type) { + return (type != null ? type.as(FactoryBean.class).getGeneric() : ResolvableType.NONE); + } + /** * Obtain an object to expose from the given FactoryBean, if available * in cached form. Quick check for minimal synchronization. From 7e6612a920219f2dd811f55ec0d6a1d282b15aee Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Fri, 4 Aug 2023 00:47:18 +0200 Subject: [PATCH 2/2] Sort multiple @Autowired methods on same bean class via ASM Closes gh-30359 --- .../AutowiredAnnotationBeanPostProcessor.java | 63 +++++++++++++++++-- ...wiredAnnotationBeanPostProcessorTests.java | 16 +++-- ...onfigurationClassBeanDefinitionReader.java | 6 +- .../annotation/ConfigurationClassParser.java | 5 +- 4 files changed, 71 insertions(+), 19 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java index da11e0203f5..a26094cd25d 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessor.java @@ -17,6 +17,7 @@ package org.springframework.beans.factory.annotation; import java.beans.PropertyDescriptor; +import java.io.IOException; import java.lang.annotation.Annotation; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; @@ -79,6 +80,10 @@ import org.springframework.core.PriorityOrdered; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.annotation.MergedAnnotation; import org.springframework.core.annotation.MergedAnnotations; +import org.springframework.core.type.AnnotationMetadata; +import org.springframework.core.type.MethodMetadata; +import org.springframework.core.type.classreading.MetadataReaderFactory; +import org.springframework.core.type.classreading.SimpleMetadataReaderFactory; import org.springframework.javapoet.ClassName; import org.springframework.javapoet.CodeBlock; import org.springframework.lang.Nullable; @@ -167,6 +172,9 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA @Nullable private ConfigurableListableBeanFactory beanFactory; + @Nullable + private MetadataReaderFactory metadataReaderFactory; + private final Set lookupMethodsChecked = Collections.newSetFromMap(new ConcurrentHashMap<>(256)); private final Map, Constructor[]> candidateConstructorsCache = new ConcurrentHashMap<>(256); @@ -271,6 +279,7 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA "AutowiredAnnotationBeanPostProcessor requires a ConfigurableListableBeanFactory: " + beanFactory); } this.beanFactory = clbf; + this.metadataReaderFactory = new SimpleMetadataReaderFactory(clbf.getBeanClassLoader()); } @@ -539,12 +548,11 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA return InjectionMetadata.EMPTY; } - List elements = new ArrayList<>(); + final List elements = new ArrayList<>(); Class targetClass = clazz; do { - final List currElements = new ArrayList<>(); - + final List fieldElements = new ArrayList<>(); ReflectionUtils.doWithLocalFields(targetClass, field -> { MergedAnnotation ann = findAutowiredAnnotation(field); if (ann != null) { @@ -555,10 +563,11 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA return; } boolean required = determineRequiredStatus(ann); - currElements.add(new AutowiredFieldElement(field, required)); + fieldElements.add(new AutowiredFieldElement(field, required)); } }); + final List methodElements = new ArrayList<>(); ReflectionUtils.doWithLocalMethods(targetClass, method -> { Method bridgedMethod = BridgeMethodResolver.findBridgedMethod(method); if (!BridgeMethodResolver.isVisibilityBridgeMethodPair(method, bridgedMethod)) { @@ -580,11 +589,12 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA } boolean required = determineRequiredStatus(ann); PropertyDescriptor pd = BeanUtils.findPropertyForMethod(bridgedMethod, clazz); - currElements.add(new AutowiredMethodElement(method, required, pd)); + methodElements.add(new AutowiredMethodElement(method, required, pd)); } }); - elements.addAll(0, currElements); + elements.addAll(0, sortMethodElements(methodElements, targetClass)); + elements.addAll(0, fieldElements); targetClass = targetClass.getSuperclass(); } while (targetClass != null && targetClass != Object.class); @@ -617,6 +627,47 @@ public class AutowiredAnnotationBeanPostProcessor implements SmartInstantiationA this.requiredParameterValue == ann.getBoolean(this.requiredParameterName)); } + /** + * Sort the method elements via ASM for deterministic declaration order if possible. + */ + private List sortMethodElements( + List methodElements, Class targetClass) { + + if (this.metadataReaderFactory != null && methodElements.size() > 1) { + // Try reading the class file via ASM for deterministic declaration order... + // Unfortunately, the JVM's standard reflection returns methods in arbitrary + // order, even between different runs of the same application on the same JVM. + try { + AnnotationMetadata asm = + this.metadataReaderFactory.getMetadataReader(targetClass.getName()).getAnnotationMetadata(); + Set asmMethods = asm.getAnnotatedMethods(Autowired.class.getName()); + if (asmMethods.size() >= methodElements.size()) { + List candidateMethods = new ArrayList<>(methodElements); + List selectedMethods = new ArrayList<>(asmMethods.size()); + for (MethodMetadata asmMethod : asmMethods) { + for (Iterator it = candidateMethods.iterator(); it.hasNext();) { + InjectionMetadata.InjectedElement element = it.next(); + if (element.getMember().getName().equals(asmMethod.getMethodName())) { + selectedMethods.add(element); + it.remove(); + break; + } + } + } + if (selectedMethods.size() == methodElements.size()) { + // All reflection-detected methods found in ASM method set -> proceed + return selectedMethods; + } + } + } + catch (IOException ex) { + logger.debug("Failed to read class file via ASM for determining @Autowired method order", ex); + // No worries, let's continue with the reflection metadata we started with... + } + } + return methodElements; + } + /** * Register the specified bean as dependent on the autowired beans. */ diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java index 3a1fb0a6139..289863e6d1c 100644 --- a/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java @@ -72,6 +72,7 @@ import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.core.annotation.Order; import org.springframework.core.testfixture.io.SerializationTestUtils; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; import org.springframework.util.ReflectionUtils; @@ -2605,13 +2606,12 @@ public class AutowiredAnnotationBeanPostProcessorTests { @Autowired(required = false) private TestBean testBean; - private TestBean testBean2; + TestBean testBean2; @Autowired public void setTestBean2(TestBean testBean2) { - if (this.testBean2 != null) { - throw new IllegalStateException("Already called"); - } + Assert.state(this.testBean != null, "Wrong initialization order"); + Assert.state(this.testBean2 == null, "Already called"); this.testBean2 = testBean2; } @@ -2643,9 +2643,8 @@ public class AutowiredAnnotationBeanPostProcessorTests { @Override @Autowired - @SuppressWarnings("deprecation") public void setTestBean2(TestBean testBean2) { - super.setTestBean2(testBean2); + this.testBean2 = testBean2; } @Autowired @@ -2661,6 +2660,7 @@ public class AutowiredAnnotationBeanPostProcessorTests { @Autowired protected void initBeanFactory(BeanFactory beanFactory) { + Assert.state(this.baseInjected, "Wrong initialization order"); this.beanFactory = beanFactory; } @@ -4084,9 +4084,7 @@ public class AutowiredAnnotationBeanPostProcessorTests { private RT obj; protected void setObj(RT obj) { - if (this.obj != null) { - throw new IllegalStateException("Already called"); - } + Assert.state(this.obj == null, "Already called"); this.obj = obj; } } diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 66df6f8bcfc..ae75eb1a017 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 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. @@ -314,8 +314,7 @@ class ConfigurationClassBeanDefinitionReader { // At this point, it's a top-level override (probably XML), just having been parsed // before configuration class processing kicks in... - if (this.registry instanceof DefaultListableBeanFactory dlbf && - !dlbf.isAllowBeanDefinitionOverriding()) { + if (this.registry instanceof DefaultListableBeanFactory dlbf && !dlbf.isAllowBeanDefinitionOverriding()) { throw new BeanDefinitionStoreException(beanMethod.getConfigurationClass().getResource().getDescription(), beanName, "@Bean definition illegally overridden by existing bean definition: " + existingBeanDef); } @@ -401,6 +400,7 @@ class ConfigurationClassBeanDefinitionReader { public ConfigurationClassBeanDefinition(RootBeanDefinition original, ConfigurationClass configClass, MethodMetadata beanMethodMetadata, String derivedBeanName) { + super(original); this.annotationMetadata = configClass.getMetadata(); this.factoryMethodMetadata = beanMethodMetadata; diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java index afcc46a27f2..12281caec79 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java @@ -403,11 +403,14 @@ class ConfigurationClassParser { this.metadataReaderFactory.getMetadataReader(original.getClassName()).getAnnotationMetadata(); Set asmMethods = asm.getAnnotatedMethods(Bean.class.getName()); if (asmMethods.size() >= beanMethods.size()) { + Set candidateMethods = new LinkedHashSet<>(beanMethods); Set selectedMethods = new LinkedHashSet<>(asmMethods.size()); for (MethodMetadata asmMethod : asmMethods) { - for (MethodMetadata beanMethod : beanMethods) { + for (Iterator it = candidateMethods.iterator(); it.hasNext();) { + MethodMetadata beanMethod = it.next(); if (beanMethod.getMethodName().equals(asmMethod.getMethodName())) { selectedMethods.add(beanMethod); + it.remove(); break; } }