From 7fb0ad37da156ae9a2c9c236eaa0f0e2a512c462 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 22 Feb 2017 15:32:04 +0100 Subject: [PATCH] ConfigurationClassEnhancer explicitly handles non-interceptable FactoryBeans Issue: SPR-15275 --- .../ConfigurationClassEnhancer.java | 199 +++++++++----- .../context/annotation/Spr15275Tests.java | 250 ++++++++++++++++++ 2 files changed, 379 insertions(+), 70 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/context/annotation/Spr15275Tests.java diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java index 5a1a1186f3f..df2ae5b6a7b 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java +++ b/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 - // be further proxied + // Scoped proxy factory beans are a special case and should not 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 " + - "result in a failure to process annotations such as @Autowired, " + - "@Resource and @PostConstruct within the method's declaring " + - "@Configuration class. Add the 'static' modifier to this method to avoid " + - "these container lifecycle issues; see @Bean javadoc for complete details.", + "assignable to Spring's BeanFactoryPostProcessor interface. This will " + + "result in a failure to process annotations such as @Autowired, " + + "@Resource and @PostConstruct within the method's declaring " + + "@Configuration class. Add the 'static' modifier to this method to avoid " + + "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 - // 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. - boolean alreadyInCreation = beanFactory.isCurrentlyInCreation(beanName); - try { - if (alreadyInCreation) { - beanFactory.setCurrentlyInCreation(beanName, false); - } - boolean useArgs = !ObjectUtils.isEmpty(beanMethodArgs); - if (useArgs && beanFactory.isSingleton(beanName)) { - // 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; - } + + return obtainBeanInstanceFromFactory(beanMethod, beanMethodArgs, beanFactory, beanName); + } + + private Object obtainBeanInstanceFromFactory(Method beanMethod, Object[] beanMethodArgs, + ConfigurableBeanFactory beanFactory, String beanName) { + + // The user (i.e. not the factory) is requesting this bean through a call to + // 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. + boolean alreadyInCreation = beanFactory.isCurrentlyInCreation(beanName); + try { + if (alreadyInCreation) { + beanFactory.setCurrentlyInCreation(beanName, false); + } + boolean useArgs = !ObjectUtils.isEmpty(beanMethodArgs); + if (useArgs && beanFactory.isSingleton(beanName)) { + // 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)); - if (beanInstance != null && !ClassUtils.isAssignableValue(beanMethod.getReturnType(), beanInstance)) { - String msg = String.format("@Bean method %s.%s called as a bean reference " + - "for type [%s] but overridden by non-compatible bean instance of type [%s].", - beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName(), - beanMethod.getReturnType().getName(), beanInstance.getClass().getName()); - try { - 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); + } + Object beanInstance = (useArgs ? beanFactory.getBean(beanName, beanMethodArgs) : + beanFactory.getBean(beanName)); + if (beanInstance != null && !ClassUtils.isAssignableValue(beanMethod.getReturnType(), beanInstance)) { + String msg = String.format("@Bean method %s.%s called as a bean reference " + + "for type [%s] but overridden by non-compatible bean instance of type [%s].", + beanMethod.getDeclaringClass().getSimpleName(), beanMethod.getName(), + beanMethod.getReturnType().getName(), beanInstance.getClass().getName()); + try { + BeanDefinition beanDefinition = beanFactory.getMergedBeanDefinition(beanName); + msg += " Overriding bean of same name declared in: " + beanDefinition.getResourceDescription(); } - Method currentlyInvoked = SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod(); - if (currentlyInvoked != null) { - String outerBeanName = BeanAnnotationHelper.determineBeanNameFor(currentlyInvoked); - beanFactory.registerDependentBean(beanName, outerBeanName); + catch (NoSuchBeanDefinitionException ex) { + // Ignore - simply no detailed message then. } - return beanInstance; + throw new IllegalStateException(msg); } - finally { - if (alreadyInCreation) { - beanFactory.setCurrentlyInCreation(beanName, true); - } + Method currentlyInvoked = SimpleInstantiationStrategy.getCurrentlyInvokedFactoryMethod(); + if (currentlyInvoked != null) { + 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 beanName 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, - final String beanName) { + private Object enhanceFactoryBean(final Object factoryBean, Class exposedType, + 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); - } } } diff --git a/spring-context/src/test/java/org/springframework/context/annotation/Spr15275Tests.java b/spring-context/src/test/java/org/springframework/context/annotation/Spr15275Tests.java new file mode 100644 index 00000000000..a15c79df00f --- /dev/null +++ b/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() { + return new FactoryBean() { + @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() { + return new AbstractFactoryBean() { + @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 foo() { + return new AbstractFactoryBean() { + @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 foo() { + return new AbstractFactoryBean() { + @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 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 { + + @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; + } + } + +}