ConfigurationClassEnhancer explicitly handles non-interceptable FactoryBeans

Issue: SPR-15275

spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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,7 +17,10 @@
 package org.springframework.context.annotation;
 
 import java.lang.reflect.Field;
+import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.Proxy;
 import java.util.Arrays;
 
 import org.apache.commons.logging.Log;
@@ -330,12 +333,11 @@ class ConfigurationClassEnhancer {
 					factoryContainsBean(beanFactory, beanName)) {
 				Object factoryBean = beanFactory.getBean(BeanFactory.FACTORY_BEAN_PREFIX + beanName);
 				if (factoryBean instanceof ScopedProxyFactoryBean) {
-					// Pass through - scoped proxy factory beans are a special case and should not
+					// Scoped proxy factory beans are a special case and should not be further proxied
-					// be further proxied
 				}
 				else {
 					// It is a candidate FactoryBean - go ahead with enhancement
-					return enhanceFactoryBean(factoryBean, beanFactory, beanName);
+					return enhanceFactoryBean(factoryBean, beanMethod.getReturnType(), beanFactory, beanName);
 				}
 			}
 
@@ -346,68 +348,88 @@ class ConfigurationClassEnhancer {
 				if (logger.isWarnEnabled() &&
 						BeanFactoryPostProcessor.class.isAssignableFrom(beanMethod.getReturnType())) {
 					logger.warn(String.format("@Bean method %s.%s is non-static and returns an object " +
-							"assignable to Spring's BeanFactoryPostProcessor interface. This will " +
+									"assignable to Spring's BeanFactoryPostProcessor interface. This will " +
-							"result in a failure to process annotations such as @Autowired, " +
+									"result in a failure to process annotations such as @Autowired, " +
-							"@Resource and @PostConstruct within the method's declaring " +
+									"@Resource and @PostConstruct within the method's declaring " +
-							"@Configuration class. Add the 'static' modifier to this method to avoid " +
+									"@Configuration class. Add the 'static' modifier to this method to avoid " +
-							"these container lifecycle issues; see @Bean javadoc for complete details.",
+									"these container lifecycle issues; see @Bean javadoc for complete details.",
 							beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName()));
 				}
 				return cglibMethodProxy.invokeSuper(enhancedConfigInstance, beanMethodArgs);
 			}
-			else {
+
-				// The user (i.e. not the factory) is requesting this bean through a call to
+			return obtainBeanInstanceFromFactory(beanMethod, beanMethodArgs, beanFactory, beanName);
-				// the bean method, direct or indirect. The bean may have already been marked
+		}
-				// as 'in creation' in certain autowiring scenarios; if so, temporarily set
+
-				// the in-creation status to false in order to avoid an exception.
+		private Object obtainBeanInstanceFromFactory(Method beanMethod, Object[] beanMethodArgs,
-				boolean alreadyInCreation = beanFactory.isCurrentlyInCreation(beanName);
+				ConfigurableBeanFactory beanFactory, String beanName) {
-				try {
+
-					if (alreadyInCreation) {
+			// The user (i.e. not the factory) is requesting this bean through a call to
-						beanFactory.setCurrentlyInCreation(beanName, false);
+			// the bean method, direct or indirect. The bean may have already been marked
-					}
+			// as 'in creation' in certain autowiring scenarios; if so, temporarily set
-					boolean useArgs = !ObjectUtils.isEmpty(beanMethodArgs);
+			// the in-creation status to false in order to avoid an exception.
-					if (useArgs && beanFactory.isSingleton(beanName)) {
+			boolean alreadyInCreation = beanFactory.isCurrentlyInCreation(beanName);
-						// Stubbed null arguments just for reference purposes,
+			try {
-						// expecting them to be autowired for regular singleton references?
+				if (alreadyInCreation) {
-						// A safe assumption since @Bean singleton arguments cannot be optional...
+					beanFactory.setCurrentlyInCreation(beanName, false);
-						for (Object arg : beanMethodArgs) {
+				}
-							if (arg == null) {
+				boolean useArgs = !ObjectUtils.isEmpty(beanMethodArgs);
-								useArgs = false;
+				if (useArgs && beanFactory.isSingleton(beanName)) {
-								break;
+					// Stubbed null arguments just for reference purposes,
-							}
+					// expecting them to be autowired for regular singleton references?
+					// A safe assumption since @Bean singleton arguments cannot be optional...
+					for (Object arg : beanMethodArgs) {
+						if (arg == null) {
+							useArgs = false;
+							break;
 						}
 					}
-					Object beanInstance = (useArgs ? beanFactory.getBean(beanName, beanMethodArgs) :
+				}
-							beanFactory.getBean(beanName));
+				Object beanInstance = (useArgs ? beanFactory.getBean(beanName, beanMethodArgs) :
-					if (beanInstance != null && !ClassUtils.isAssignableValue(beanMethod.getReturnType(), beanInstance)) {
+						beanFactory.getBean(beanName));
-						String msg = String.format("@Bean method %s.%s called as a bean reference " +
+				if (beanInstance != null && !ClassUtils.isAssignableValue(beanMethod.getReturnType(), beanInstance)) {
-									"for type [%s] but overridden by non-compatible bean instance of type [%s].",
+					String msg = String.format("@Bean method %s.%s called as a bean reference " +
-									beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName(),
+								"for type [%s] but overridden by non-compatible bean instance of type [%s].",
-									beanMethod.getReturnType().getName(), beanInstance.getClass().getName());
+								beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName(),
-						try {
+								beanMethod.getReturnType().getName(), beanInstance.getClass().getName());
-							BeanDefinition beanDefinition = beanFactory.getMergedBeanDefinition(beanName);
+					try {
-							msg += " Overriding bean of same name declared in: " + beanDefinition.getResourceDescription();
+						BeanDefinition beanDefinition = beanFactory.getMergedBeanDefinition(beanName);
-						}
+						msg += " Overriding bean of same name declared in: " + beanDefinition.getResourceDescription();
-						catch (NoSuchBeanDefinitionException ex) {
-							// Ignore - simply no detailed message then.
-						}
-						throw new IllegalStateException(msg);
 					}
-					Method currentlyInvoked = SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod();
+					catch (NoSuchBeanDefinitionException ex) {
-					if (currentlyInvoked != null) {
+						// Ignore - simply no detailed message then.
-						String outerBeanName = BeanAnnotationHelper.determineBeanNameFor(currentlyInvoked);
-						beanFactory.registerDependentBean(beanName, outerBeanName);
 					}
-					return beanInstance;
+					throw new IllegalStateException(msg);
 				}
-				finally {
+				Method currentlyInvoked = SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod();
-					if (alreadyInCreation) {
+				if (currentlyInvoked != null) {
-						beanFactory.setCurrentlyInCreation(beanName, true);
+					String outerBeanName = BeanAnnotationHelper.determineBeanNameFor(currentlyInvoked);
-					}
+					beanFactory.registerDependentBean(beanName, outerBeanName);
+				}
+				return beanInstance;
+			}
+			finally {
+				if (alreadyInCreation) {
+					beanFactory.setCurrentlyInCreation(beanName, true);
 				}
 			}
 		}
 
+		@Override
+		public boolean isMatch(Method candidateMethod) {
+			return BeanAnnotationHelper.isBeanAnnotated(candidateMethod);
+		}
+
+		private ConfigurableBeanFactory getBeanFactory(Object enhancedConfigInstance) {
+			Field field = ReflectionUtils.findField(enhancedConfigInstance.getClass(), BEAN_FACTORY_FIELD);
+			Assert.state(field != null, "Unable to find generated bean factory field");
+			Object beanFactory = ReflectionUtils.getField(field, enhancedConfigInstance);
+			Assert.state(beanFactory != null, "BeanFactory has not been injected into @Configuration class");
+			Assert.state(beanFactory instanceof ConfigurableBeanFactory,
+					"Injected BeanFactory is not a ConfigurableBeanFactory");
+			return (ConfigurableBeanFactory) beanFactory;
+		}
+
 		/**
 		 * Check the BeanFactory to see whether the bean named <var>beanName</var> already
 		 * exists. Accounts for the fact that the requested bean may be "in creation", i.e.:
@@ -444,8 +466,60 @@ class ConfigurationClassEnhancer {
 		 * instance directly. If a FactoryBean instance is fetched through the container via &-dereferencing,
 		 * it will not be proxied. This too is aligned with the way XML configuration works.
 		 */
-		private Object enhanceFactoryBean(final Object factoryBean, final ConfigurableBeanFactory beanFactory,
+		private Object enhanceFactoryBean(final Object factoryBean, Class<?> exposedType,
-				final String beanName) {
+				final ConfigurableBeanFactory beanFactory, final String beanName) {
+
+			try {
+				Class<?> clazz = factoryBean.getClass();
+				boolean finalClass = Modifier.isFinal(clazz.getModifiers());
+				boolean finalMethod = Modifier.isFinal(clazz.getMethod("getObject").getModifiers());
+				if (finalClass || finalMethod) {
+					if (exposedType.isInterface()) {
+						if (logger.isDebugEnabled()) {
+							logger.debug("Creating interface proxy for FactoryBean '" + beanName + "' of type [" +
+									clazz.getName() + "] for use within another @Bean method because its " +
+									(finalClass ? "implementation class" : "getObject() method") +
+									" is final: Otherwise a getObject() call would not be routed to the factory.");
+						}
+						return createInterfaceProxyForFactoryBean(factoryBean, exposedType, beanFactory, beanName);
+					}
+					else {
+						if (logger.isInfoEnabled()) {
+							logger.info("Unable to proxy FactoryBean '" + beanName + "' of type [" +
+									clazz.getName() + "] for use within another @Bean method because its " +
+									(finalClass ? "implementation class" : "getObject() method") +
+									" is final: A getObject() call will NOT be routed to the factory. " +
+									"Consider declaring the return type as a FactoryBean interface.");
+						}
+						return factoryBean;
+					}
+				}
+			}
+			catch (NoSuchMethodException ex) {
+				// No getObject() method -> shouldn't happen, but as long as nobody is trying to call it...
+			}
+
+			return createCglibProxyForFactoryBean(factoryBean, beanFactory, beanName);
+		}
+
+		private Object createInterfaceProxyForFactoryBean(final Object factoryBean, Class<?> interfaceType,
+				final ConfigurableBeanFactory beanFactory, final String beanName) {
+
+			return Proxy.newProxyInstance(
+					factoryBean.getClass().getClassLoader(), new Class<?>[] {interfaceType},
+					new InvocationHandler() {
+						@Override
+						public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+							if (method.getName().equals("getObject") && args == null) {
+								return beanFactory.getBean(beanName);
+							}
+							return ReflectionUtils.invokeMethod(method, factoryBean, args);
+						}
+					});
+		}
+
+		private Object createCglibProxyForFactoryBean(final Object factoryBean,
+				final ConfigurableBeanFactory beanFactory, final String beanName) {
 
 			Enhancer enhancer = new Enhancer();
 			enhancer.setSuperclass(factoryBean.getClass());
@@ -489,21 +563,6 @@ class ConfigurationClassEnhancer {
 
 			return fbProxy;
 		}
-
-		private ConfigurableBeanFactory getBeanFactory(Object enhancedConfigInstance) {
-			Field field = ReflectionUtils.findField(enhancedConfigInstance.getClass(), BEAN_FACTORY_FIELD);
-			Assert.state(field != null, "Unable to find generated bean factory field");
-			Object beanFactory = ReflectionUtils.getField(field, enhancedConfigInstance);
-			Assert.state(beanFactory != null, "BeanFactory has not been injected into @Configuration class");
-			Assert.state(beanFactory instanceof ConfigurableBeanFactory,
-					"Injected BeanFactory is not a ConfigurableBeanFactory");
-			return (ConfigurableBeanFactory) beanFactory;
-		}
-
-		@Override
-		public boolean isMatch(Method candidateMethod) {
-			return BeanAnnotationHelper.isBeanAnnotated(candidateMethod);
-		}
 	}
 
 }

spring-context/src/test/java/org/springframework/context/annotation/Spr15275Tests.java

@@ -0,0 +1,250 @@
+/*
+ * Copyright 2002-2017 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.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.context.annotation;
+
+import org.junit.Test;
+
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.beans.factory.config.AbstractFactoryBean;
+import org.springframework.context.ApplicationContext;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author Juergen Hoeller
+ */
+public class Spr15275Tests {
+
+	@Test
+	public void testWithFactoryBean() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithFactoryBean.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		assertSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+	@Test
+	public void testWithAbstractFactoryBean() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithAbstractFactoryBean.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		assertSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+	@Test
+	public void testWithAbstractFactoryBeanForInterface() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithAbstractFactoryBeanForInterface.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		assertSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+	@Test
+	public void testWithAbstractFactoryBeanAsReturnType() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithAbstractFactoryBeanAsReturnType.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		assertSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+	@Test
+	public void testWithFinalFactoryBean() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithFinalFactoryBean.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		assertSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+	@Test
+	public void testWithFinalFactoryBeanAsReturnType() {
+		ApplicationContext context = new AnnotationConfigApplicationContext(ConfigWithFinalFactoryBeanAsReturnType.class);
+		assertEquals("x", context.getBean(Bar.class).foo.toString());
+		// not same due to fallback to raw FinalFactoryBean instance with repeated getObject() invocations
+		assertNotSame(context.getBean(FooInterface.class), context.getBean(Bar.class).foo);
+	}
+
+
+	@Configuration
+	protected static class ConfigWithFactoryBean {
+
+		@Bean
+		public FactoryBean<Foo> foo() {
+			return new FactoryBean<Foo>() {
+				@Override
+				public Foo getObject() {
+					return new Foo("x");
+				}
+				@Override
+				public Class<?> getObjectType() {
+					return Foo.class;
+				}
+			};
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	@Configuration
+	protected static class ConfigWithAbstractFactoryBean {
+
+		@Bean
+		public FactoryBean<Foo> foo() {
+			return new AbstractFactoryBean<Foo>() {
+				@Override
+				public Foo createInstance() {
+					return new Foo("x");
+				}
+				@Override
+				public Class<?> getObjectType() {
+					return Foo.class;
+				}
+			};
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	@Configuration
+	protected static class ConfigWithAbstractFactoryBeanForInterface {
+
+		@Bean
+		public FactoryBean<FooInterface> foo() {
+			return new AbstractFactoryBean<FooInterface>() {
+				@Override
+				public FooInterface createInstance() {
+					return new Foo("x");
+				}
+				@Override
+				public Class<?> getObjectType() {
+					return FooInterface.class;
+				}
+			};
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	@Configuration
+	protected static class ConfigWithAbstractFactoryBeanAsReturnType {
+
+		@Bean
+		public AbstractFactoryBean<FooInterface> foo() {
+			return new AbstractFactoryBean<FooInterface>() {
+				@Override
+				public FooInterface createInstance() {
+					return new Foo("x");
+				}
+				@Override
+				public Class<?> getObjectType() {
+					return Foo.class;
+				}
+			};
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	@Configuration
+	protected static class ConfigWithFinalFactoryBean {
+
+		@Bean
+		public FactoryBean<FooInterface> foo() {
+			return new FinalFactoryBean();
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	@Configuration
+	protected static class ConfigWithFinalFactoryBeanAsReturnType {
+
+		@Bean
+		public FinalFactoryBean foo() {
+			return new FinalFactoryBean();
+		}
+
+		@Bean
+		public Bar bar() throws Exception {
+			assertTrue(foo().isSingleton());
+			return new Bar(foo().getObject());
+		}
+	}
+
+
+	private static final class FinalFactoryBean implements FactoryBean<FooInterface> {
+
+		@Override
+		public Foo getObject() {
+			return new Foo("x");
+		}
+
+		@Override
+		public Class<?> getObjectType() {
+			return FooInterface.class;
+		}
+	};
+
+
+	protected interface FooInterface {
+	}
+
+
+	protected static class Foo implements FooInterface {
+
+		private final String value;
+
+		public Foo(String value) {
+			this.value = value;
+		}
+
+		@Override
+		public String toString() {
+			return this.value;
+		}
+	}
+
+
+	protected static class Bar {
+
+		public final FooInterface foo;
+
+		public Bar(FooInterface foo) {
+			this.foo = foo;
+		}
+	}
+
+}