Consistent throwing of BeanInstantiationException for factory methods, including a hint about circular references

Issue: SPR-12317

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;
 	}
 

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,
+	protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner) {
-			BeanFactory owner) {
+		return instantiateWithMethodInjection(bd, beanName, owner, null);
-
-		return instantiateWithMethodInjection(beanDefinition, beanName, owner, null, null);
 	}
 
 	@Override
-	protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, String beanName,
+	protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			BeanFactory owner, Constructor<?> ctor, Object[] args) {
+			Constructor<?> ctor, Object... args) {
 
-		// Must generate CGLIB subclass.
+		// Must generate CGLIB subclass...
-		return new CglibSubclassCreator(beanDefinition, owner).instantiate(ctor, args);
+		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(
+					throw new BeanInstantiationException(this.beanDefinition.getBeanClass(),
-							"Failed to invoke constructor for CGLIB enhanced subclass [%s]", subclass.getName()), ex);
+							"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;
 		}

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(
+	public BeanWrapper autowireConstructor(final String beanName, final RootBeanDefinition mbd,
-			final String beanName, final RootBeanDefinition mbd, Constructor<?>[] chosenCtors, final Object[] explicitArgs) {
+			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(
+				beanInstance = this.beanFactory.getInstantiationStrategy().instantiate(
-						mbd, beanName, beanFactory, factoryBean, factoryMethodToUse, argsToUse);
+						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.
+	 * <p>This method is also used for handling invocations of static factory methods.
 	 */
-	private int resolveConstructorArguments(
+	private int resolveConstructorArguments(String beanName, RootBeanDefinition mbd, BeanWrapper bw,
-			String beanName, RootBeanDefinition mbd, BeanWrapper bw,
 			ConstructorArgumentValues cargs, ConstructorArgumentValues resolvedValues) {
 
 		TypeConverter converter = (this.beanFactory.getCustomTypeConverter() != null ?

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 bd the bean definition
-	 * @param beanName name of the bean when it's created in this context.
+	 * @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 that
+	 * The name can be {@code null} if we're autowiring a bean which doesn't
-	 * doesn't belong to the factory.
+	 * belong to the factory.
-	 * @param owner owning BeanFactory
+	 * @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 bd the bean definition
-	 * @param beanName name of the bean when it's created in this context.
+	 * @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
+	 * The name can be {@code null} if we're autowiring a bean which doesn't
-	 * that doesn't belong to the factory.
+	 * belong to the factory.
-	 * @param owner owning BeanFactory
+	 * @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,
+	Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			Constructor<?> ctor, Object[] args) throws BeansException;
+			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 bd the bean definition
-	 * @param beanName name of the bean when it's created in this context.
+	 * @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
+	 * The name can be {@code null} if we're autowiring a bean which doesn't
-	 * that doesn't belong to the factory.
+	 * belong to the factory.
-	 * @param owner owning BeanFactory
+	 * @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 instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			Object factoryBean, Method factoryMethod, Object[] args) throws BeansException;
+			Object factoryBean, Method factoryMethod, Object... args) throws BeansException;
 
 }

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) {
+			synchronized (bd.constructorArgumentLock) {
-				constructorToUse = (Constructor<?>) beanDefinition.resolvedConstructorOrFactoryMethod;
+				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(
+	protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner) {
-			RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) {
+		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,
+	public Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			final Constructor<?> ctor, Object[] args) {
+			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<Object>() {
@@ -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,
+	protected Object instantiateWithMethodInjection(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			String beanName, BeanFactory owner, Constructor<?> ctor, Object[] args) {
+			Constructor<?> ctor, Object... args) {
 
-		throw new UnsupportedOperationException(
+		throw new UnsupportedOperationException("Method Injection not supported in SimpleInstantiationStrategy");
-				"Method Injection not supported in SimpleInstantiationStrategy");
 	}
 
 	@Override
-	public Object instantiate(RootBeanDefinition beanDefinition, String beanName, BeanFactory owner,
+	public Object instantiate(RootBeanDefinition bd, String beanName, BeanFactory owner,
-			Object factoryBean, final Method factoryMethod, Object[] args) {
+			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(
+			throw new BeanInstantiationException(factoryMethod.getReturnType(),
-					"Illegal arguments to factory method [" + factoryMethod + "]; " +
+					"Illegal arguments to factory method '" + factoryMethod.getName() + "'; " +
-					"args: " + StringUtils.arrayToCommaDelimitedString(args));
+					"args: " + StringUtils.arrayToCommaDelimitedString(args), ex);
 		}
 		catch (IllegalAccessException ex) {
-			throw new BeanDefinitionStoreException(
+			throw new BeanInstantiationException(factoryMethod.getReturnType(),
-					"Cannot access factory method [" + factoryMethod + "]; is it public?");
+					"Cannot access factory method '" + factoryMethod.getName() + "'; is it public?", ex);
 		}
 		catch (InvocationTargetException ex) {
-			throw new BeanDefinitionStoreException(
+			String msg = "Factory method '" + factoryMethod.getName() + "' threw exception";
-					"Factory method [" + factoryMethod + "] threw exception", ex.getTargetException());
+			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());
 		}
 	}
 

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 {
+	}
+
+}