From ad62b2afb145499d384cb76c5f254113db99796c Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 21 Oct 2014 21:40:43 +0200 Subject: [PATCH] Consistent throwing of BeanInstantiationException for factory methods, including a hint about circular references Issue: SPR-12317 --- .../beans/BeanInstantiationException.java | 2 +- ...CglibSubclassingInstantiationStrategy.java | 30 ++++----- .../factory/support/ConstructorResolver.java | 19 +++--- .../support/InstantiationStrategy.java | 49 +++++++------- .../support/SimpleInstantiationStrategy.java | 63 ++++++++--------- .../ConfigurationClassPostProcessorTests.java | 67 ++++++++++++++++++- 6 files changed, 148 insertions(+), 82 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanInstantiationException.java b/spring-beans/src/main/java/org/springframework/beans/BeanInstantiationException.java index c4b7ff45f4..09190f6f12 100644 --- a/spring-beans/src/main/java/org/springframework/beans/BeanInstantiationException.java +++ b/spring-beans/src/main/java/org/springframework/beans/BeanInstantiationException.java @@ -45,7 +45,7 @@ public class BeanInstantiationException extends FatalBeanException { * @param cause the root cause */ public BeanInstantiationException(Class beanClass, String msg, Throwable cause) { - super("Could not instantiate bean class [" + beanClass.getName() + "]: " + msg, cause); + super("Failed to instantiate [" + beanClass.getName() + "]: " + msg, cause); this.beanClass = beanClass; } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/CglibSubclassingInstantiationStrategy.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/CglibSubclassingInstantiationStrategy.java index 8821bcab39..ed8d5a3e83 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/CglibSubclassingInstantiationStrategy.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/CglibSubclassingInstantiationStrategy.java @@ -68,18 +68,16 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt @Override - protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, String beanName, - BeanFactory owner) { - - return instantiateWithMethodInjection(beanDefinition, beanName, owner, null, null); + protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner) { + return instantiateWithMethodInjection(bd, beanName, owner, null); } @Override - protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, String beanName, - BeanFactory owner, Constructor ctor, Object[] args) { + protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner, + Constructor ctor, Object... args) { - // Must generate CGLIB subclass. - return new CglibSubclassCreator(beanDefinition, owner).instantiate(ctor, args); + // Must generate CGLIB subclass... + return new CglibSubclassCreator(bd, owner).instantiate(ctor, args); } @@ -110,7 +108,7 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt * Ignored if the {@code ctor} parameter is {@code null}. * @return new instance of the dynamically generated subclass */ - Object instantiate(Constructor ctor, Object[] args) { + public Object instantiate(Constructor ctor, Object... args) { Class subclass = createEnhancedSubclass(this.beanDefinition); Object instance; if (ctor == null) { @@ -122,8 +120,8 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt instance = enhancedSubclassConstructor.newInstance(args); } catch (Exception ex) { - throw new BeanInstantiationException(this.beanDefinition.getBeanClass(), String.format( - "Failed to invoke constructor for CGLIB enhanced subclass [%s]", subclass.getName()), ex); + throw new BeanInstantiationException(this.beanDefinition.getBeanClass(), + "Failed to invoke constructor for CGLIB enhanced subclass [" + subclass.getName() + "]", ex); } } // SPR-10785: set callbacks directly on the instance instead of in the @@ -159,11 +157,11 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt private final RootBeanDefinition beanDefinition; - CglibIdentitySupport(RootBeanDefinition beanDefinition) { + public CglibIdentitySupport(RootBeanDefinition beanDefinition) { this.beanDefinition = beanDefinition; } - RootBeanDefinition getBeanDefinition() { + public RootBeanDefinition getBeanDefinition() { return this.beanDefinition; } @@ -187,7 +185,7 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt private static final Log logger = LogFactory.getLog(MethodOverrideCallbackFilter.class); - MethodOverrideCallbackFilter(RootBeanDefinition beanDefinition) { + public MethodOverrideCallbackFilter(RootBeanDefinition beanDefinition) { super(beanDefinition); } @@ -220,7 +218,7 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt private final BeanFactory owner; - LookupOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) { + public LookupOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) { super(beanDefinition); this.owner = owner; } @@ -248,7 +246,7 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt private final BeanFactory owner; - ReplaceOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) { + public ReplaceOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) { super(beanDefinition); this.owner = owner; } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java index 29987b591b..5dc6184260 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java @@ -94,8 +94,8 @@ class ConstructorResolver { * or {@code null} if none (-> use constructor argument values from bean definition) * @return a BeanWrapper for the new instance */ - public BeanWrapper autowireConstructor( - final String beanName, final RootBeanDefinition mbd, Constructor[] chosenCtors, final Object[] explicitArgs) { + public BeanWrapper autowireConstructor(final String beanName, final RootBeanDefinition mbd, + Constructor[] chosenCtors, final Object[] explicitArgs) { BeanWrapperImpl bw = new BeanWrapperImpl(); this.beanFactory.initBeanWrapper(bw); @@ -275,7 +275,8 @@ class ConstructorResolver { return bw; } catch (Throwable ex) { - throw new BeanCreationException(mbd.getResourceDescription(), beanName, "Instantiation of bean failed", ex); + throw new BeanCreationException(mbd.getResourceDescription(), beanName, + "Bean instantiation via constructor failed", ex); } } @@ -587,8 +588,8 @@ class ConstructorResolver { }, beanFactory.getAccessControlContext()); } else { - beanInstance = beanFactory.getInstantiationStrategy().instantiate( - mbd, beanName, beanFactory, factoryBean, factoryMethodToUse, argsToUse); + beanInstance = this.beanFactory.getInstantiationStrategy().instantiate( + mbd, beanName, this.beanFactory, factoryBean, factoryMethodToUse, argsToUse); } if (beanInstance == null) { @@ -598,17 +599,17 @@ class ConstructorResolver { return bw; } catch (Throwable ex) { - throw new BeanCreationException(mbd.getResourceDescription(), beanName, "Instantiation of bean failed", ex); + throw new BeanCreationException(mbd.getResourceDescription(), beanName, + "Bean instantiation via factory method failed", ex); } } /** * Resolve the constructor arguments for this bean into the resolvedValues object. * This may involve looking up other beans. - * This method is also used for handling invocations of static factory methods. + *

This method is also used for handling invocations of static factory methods. */ - private int resolveConstructorArguments( - String beanName, RootBeanDefinition mbd, BeanWrapper bw, + private int resolveConstructorArguments(String beanName, RootBeanDefinition mbd, BeanWrapper bw, ConstructorArgumentValues cargs, ConstructorArgumentValues resolvedValues) { TypeConverter converter = (this.beanFactory.getCustomTypeConverter() != null ? diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/InstantiationStrategy.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/InstantiationStrategy.java index 884ee9c3d9..6aec7e8aa3 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/InstantiationStrategy.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/InstantiationStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 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. @@ -29,55 +29,56 @@ import org.springframework.beans.factory.BeanFactory; * including using CGLIB to create subclasses on the fly to support Method Injection. * * @author Rod Johnson + * @author Juergen Hoeller * @since 1.1 */ public interface InstantiationStrategy { /** * Return an instance of the bean with the given name in this factory. - * @param beanDefinition the bean definition - * @param beanName name of the bean when it's created in this context. - * The name can be {@code null} if we're autowiring a bean that - * doesn't belong to the factory. - * @param owner owning BeanFactory + * @param bd the bean definition + * @param beanName the name of the bean when it's created in this context. + * The name can be {@code null} if we're autowiring a bean which doesn't + * belong to the factory. + * @param owner the owning BeanFactory * @return a bean instance for this bean definition - * @throws BeansException if the instantiation failed + * @throws BeansException if the instantiation attempt failed */ - Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) + Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner) throws BeansException; /** * Return an instance of the bean with the given name in this factory, * creating it via the given constructor. - * @param beanDefinition the bean definition - * @param beanName name of the bean when it's created in this context. - * The name can be {@code null} if we're autowiring a bean - * that doesn't belong to the factory. - * @param owner owning BeanFactory + * @param bd the bean definition + * @param beanName the name of the bean when it's created in this context. + * The name can be {@code null} if we're autowiring a bean which doesn't + * belong to the factory. + * @param owner the owning BeanFactory * @param ctor the constructor to use * @param args the constructor arguments to apply * @return a bean instance for this bean definition - * @throws BeansException if the instantiation failed + * @throws BeansException if the instantiation attempt failed */ - Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner, - Constructor ctor, Object[] args) throws BeansException; + Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner, + Constructor ctor, Object... args) throws BeansException; /** * Return an instance of the bean with the given name in this factory, * creating it via the given factory method. - * @param beanDefinition bean definition - * @param beanName name of the bean when it's created in this context. - * The name can be {@code null} if we're autowiring a bean - * that doesn't belong to the factory. - * @param owner owning BeanFactory + * @param bd the bean definition + * @param beanName the name of the bean when it's created in this context. + * The name can be {@code null} if we're autowiring a bean which doesn't + * belong to the factory. + * @param owner the owning BeanFactory * @param factoryBean the factory bean instance to call the factory method on, * or {@code null} in case of a static factory method * @param factoryMethod the factory method to use * @param args the factory method arguments to apply * @return a bean instance for this bean definition - * @throws BeansException if the instantiation failed + * @throws BeansException if the instantiation attempt failed */ - Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner, - Object factoryBean, Method factoryMethod, Object[] args) throws BeansException; + Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner, + Object factoryBean, Method factoryMethod, Object... args) throws BeansException; } diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java index 26ef21270a..f878c57ed0 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/SimpleInstantiationStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 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. @@ -25,8 +25,8 @@ import java.security.PrivilegedExceptionAction; import org.springframework.beans.BeanInstantiationException; import org.springframework.beans.BeanUtils; -import org.springframework.beans.factory.BeanDefinitionStoreException; import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; @@ -56,14 +56,14 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { @Override - public Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) { + public Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner) { // Don't override the class with CGLIB if no overrides. - if (beanDefinition.getMethodOverrides().isEmpty()) { + if (bd.getMethodOverrides().isEmpty()) { Constructor constructorToUse; - synchronized (beanDefinition.constructorArgumentLock) { - constructorToUse = (Constructor) beanDefinition.resolvedConstructorOrFactoryMethod; + synchronized (bd.constructorArgumentLock) { + constructorToUse = (Constructor) bd.resolvedConstructorOrFactoryMethod; if (constructorToUse == null) { - final Class clazz = beanDefinition.getBeanClass(); + final Class clazz = bd.getBeanClass(); if (clazz.isInterface()) { throw new BeanInstantiationException(clazz, "Specified class is an interface"); } @@ -79,7 +79,7 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { else { constructorToUse = clazz.getDeclaredConstructor((Class[]) null); } - beanDefinition.resolvedConstructorOrFactoryMethod = constructorToUse; + bd.resolvedConstructorOrFactoryMethod = constructorToUse; } catch (Exception ex) { throw new BeanInstantiationException(clazz, "No default constructor found", ex); @@ -90,7 +90,7 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { } else { // Must generate CGLIB subclass. - return instantiateWithMethodInjection(beanDefinition, beanName, owner); + return instantiateWithMethodInjection(bd, beanName, owner); } } @@ -100,18 +100,15 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { * the Method Injection specified in the given RootBeanDefinition. * Instantiation should use a no-arg constructor. */ - protected Object instantiateWithMethodInjection( - RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) { - - throw new UnsupportedOperationException( - "Method Injection not supported in SimpleInstantiationStrategy"); + protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner) { + throw new UnsupportedOperationException("Method Injection not supported in SimpleInstantiationStrategy"); } @Override - public Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner, - final Constructor ctor, Object[] args) { + public Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner, + final Constructor ctor, Object... args) { - if (beanDefinition.getMethodOverrides().isEmpty()) { + if (bd.getMethodOverrides().isEmpty()) { if (System.getSecurityManager() != null) { // use own privileged to change accessibility (when security is on) AccessController.doPrivileged(new PrivilegedAction() { @@ -125,7 +122,7 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { return BeanUtils.instantiateClass(ctor, args); } else { - return instantiateWithMethodInjection(beanDefinition, beanName, owner, ctor, args); + return instantiateWithMethodInjection(bd, beanName, owner, ctor, args); } } @@ -135,16 +132,15 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { * the Method Injection specified in the given RootBeanDefinition. * Instantiation should use the given constructor and parameters. */ - protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, - String beanName, BeanFactory owner, Constructor ctor, Object[] args) { + protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner, + Constructor ctor, Object... args) { - throw new UnsupportedOperationException( - "Method Injection not supported in SimpleInstantiationStrategy"); + throw new UnsupportedOperationException("Method Injection not supported in SimpleInstantiationStrategy"); } @Override - public Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner, - Object factoryBean, final Method factoryMethod, Object[] args) { + public Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner, + Object factoryBean, final Method factoryMethod, Object... args) { try { if (System.getSecurityManager() != null) { @@ -175,17 +171,22 @@ public class SimpleInstantiationStrategy implements InstantiationStrategy { } } catch (IllegalArgumentException ex) { - throw new BeanDefinitionStoreException( - "Illegal arguments to factory method [" + factoryMethod + "]; " + - "args: " + StringUtils.arrayToCommaDelimitedString(args)); + throw new BeanInstantiationException(factoryMethod.getReturnType(), + "Illegal arguments to factory method '" + factoryMethod.getName() + "'; " + + "args: " + StringUtils.arrayToCommaDelimitedString(args), ex); } catch (IllegalAccessException ex) { - throw new BeanDefinitionStoreException( - "Cannot access factory method [" + factoryMethod + "]; is it public?"); + throw new BeanInstantiationException(factoryMethod.getReturnType(), + "Cannot access factory method '" + factoryMethod.getName() + "'; is it public?", ex); } catch (InvocationTargetException ex) { - throw new BeanDefinitionStoreException( - "Factory method [" + factoryMethod + "] threw exception", ex.getTargetException()); + String msg = "Factory method '" + factoryMethod.getName() + "' threw exception"; + if (bd.getFactoryBeanName() != null && owner instanceof ConfigurableBeanFactory && + ((ConfigurableBeanFactory) owner).isCurrentlyInCreation(bd.getFactoryBeanName())) { + msg = "Circular reference involving containing bean '" + bd.getFactoryBeanName() + "' - consider " + + "declaring the factory method as static for independence from its containing instance. " + msg; + } + throw new BeanInstantiationException(factoryMethod.getReturnType(), msg, ex.getTargetException()); } } diff --git a/spring-context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostProcessorTests.java b/spring-context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostProcessorTests.java index 1e502ac187..0a3b790228 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostProcessorTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/ConfigurationClassPostProcessorTests.java @@ -27,6 +27,7 @@ import org.junit.Test; import org.springframework.aop.scope.ScopedObject; import org.springframework.aop.scope.ScopedProxyUtils; +import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.beans.factory.annotation.Autowired; @@ -429,6 +430,34 @@ public class ConfigurationClassPostProcessorTests { beanFactory.preInstantiateSingletons(); } + @Test + public void testCircularDependency() { + AutowiredAnnotationBeanPostProcessor bpp = new AutowiredAnnotationBeanPostProcessor(); + bpp.setBeanFactory(beanFactory); + beanFactory.addBeanPostProcessor(bpp); + beanFactory.registerBeanDefinition("configClass1", new RootBeanDefinition(A.class)); + beanFactory.registerBeanDefinition("configClass2", new RootBeanDefinition(AStrich.class)); + new ConfigurationClassPostProcessor().postProcessBeanFactory(beanFactory); + try { + beanFactory.preInstantiateSingletons(); + fail("Should have thrown BeanCreationException"); + } + catch (BeanCreationException ex) { + assertTrue(ex.getMessage().contains("Circular reference")); + } + } + + @Test + public void testCircularDependencyWithApplicationContext() { + try { + new AnnotationConfigApplicationContext(A.class, AStrich.class); + fail("Should have thrown BeanCreationException"); + } + catch (BeanCreationException ex) { + assertTrue(ex.getMessage().contains("Circular reference")); + } + } + // ------------------------------------------------------------------------- @@ -849,5 +878,41 @@ public class ConfigurationClassPostProcessorTests { return new ServiceBean("message"); } } -} + @Configuration + public static class A { + + @Autowired(required=true) + Z z; + + @Bean + public B b() { + if (z == null) { + throw new NullPointerException("z is null"); + } + return new B(z); + } + } + + @Configuration + public static class AStrich { + + @Autowired + B b; + + @Bean + public Z z() { + return new Z(); + } + } + + public static class B { + + public B(Z z) { + } + } + + public static class Z { + } + +}