diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/StepScope.java b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/StepScope.java index 40d1868ee..383e50b71 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/StepScope.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/StepScope.java @@ -17,9 +17,7 @@ package org.springframework.batch.core.scope; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.aop.framework.autoproxy.AutoProxyUtils; import org.springframework.batch.core.scope.util.PlaceholderProxyFactoryBean; -import org.springframework.batch.core.scope.util.StepContextFactory; import org.springframework.beans.BeanWrapper; import org.springframework.beans.BeansException; import org.springframework.beans.factory.ObjectFactory; @@ -31,7 +29,6 @@ import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.beans.factory.config.Scope; import org.springframework.beans.factory.support.BeanDefinitionReaderUtils; import org.springframework.beans.factory.support.BeanDefinitionRegistry; -import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.core.Ordered; import org.springframework.util.Assert; import org.springframework.util.StringValueResolver; @@ -214,7 +211,7 @@ public class StepScope implements Scope, BeanFactoryPostProcessor, Ordered { public void setName(String name) { this.name = name; } - + /** * Wrap a target bean definition in a proxy that defers initialization until * after the {@link StepContext} is available. Amounts to adding @@ -237,7 +234,7 @@ public class StepScope implements Scope, BeanFactoryPostProcessor, Ordered { // TODO: detect presence of Spring 3.0 and use ScopedPoxyUtils instead // Create the scoped proxy... - BeanDefinitionHolder proxyHolder = createScopedProxy(new BeanDefinitionHolder(definition, beanName), registry, + BeanDefinitionHolder proxyHolder = PlaceholderProxyFactoryBean.createScopedProxy(new BeanDefinitionHolder(definition, beanName), registry, proxyTargetClass); // ...and register it under the original target name registry.registerBeanDefinition(beanName, proxyHolder.getBeanDefinition()); @@ -246,44 +243,6 @@ public class StepScope implements Scope, BeanFactoryPostProcessor, Ordered { } - private static BeanDefinitionHolder createScopedProxy(BeanDefinitionHolder definition, - BeanDefinitionRegistry registry, boolean proxyTargetClass) { - - String originalBeanName = definition.getBeanName(); - BeanDefinition targetDefinition = definition.getBeanDefinition(); - - // Create a proxy definition for the original bean name, - // "hiding" the target bean in an internal target definition. - RootBeanDefinition proxyDefinition = new RootBeanDefinition(PlaceholderProxyFactoryBean.class); - proxyDefinition.getConstructorArgumentValues().addGenericArgumentValue(new StepContextFactory()); - proxyDefinition.setOriginatingBeanDefinition(definition.getBeanDefinition()); - proxyDefinition.setSource(definition.getSource()); - proxyDefinition.setRole(BeanDefinition.ROLE_INFRASTRUCTURE); - - String targetBeanName = "lazyBindingProxy." + originalBeanName; - proxyDefinition.getPropertyValues().addPropertyValue("targetBeanName", targetBeanName); - - if (proxyTargetClass) { - targetDefinition.setAttribute(AutoProxyUtils.PRESERVE_TARGET_CLASS_ATTRIBUTE, Boolean.TRUE); - // ProxyFactoryBean's "proxyTargetClass" default is TRUE, so we - // don't need to set it explicitly here. - } - else { - proxyDefinition.getPropertyValues().addPropertyValue("proxyTargetClass", Boolean.FALSE); - } - - proxyDefinition.setAutowireCandidate(targetDefinition.isAutowireCandidate()); - // The target bean should be ignored in favor of the proxy. - targetDefinition.setAutowireCandidate(false); - - // Register the target bean as separate bean in the factory. - registry.registerBeanDefinition(targetBeanName, targetDefinition); - - // Return the scoped proxy definition as primary bean definition - // (potentially an inner bean). - return new BeanDefinitionHolder(proxyDefinition, originalBeanName, definition.getAliases()); - } - /** * Helper class to scan a bean definition hierarchy and force the use of * auto-proxy for step scoped beans. diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderProxyFactoryBean.java b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderProxyFactoryBean.java index e5f3693d0..a5f393903 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderProxyFactoryBean.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderProxyFactoryBean.java @@ -5,6 +5,7 @@ import java.lang.reflect.Modifier; import org.springframework.aop.framework.AopInfrastructureBean; import org.springframework.aop.framework.ProxyConfig; import org.springframework.aop.framework.ProxyFactory; +import org.springframework.aop.framework.autoproxy.AutoProxyUtils; import org.springframework.aop.scope.DefaultScopedObject; import org.springframework.aop.scope.ScopedObject; import org.springframework.aop.scope.ScopedProxyFactoryBean; @@ -13,7 +14,11 @@ import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.BeanFactoryAware; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.FactoryBeanNotInitializedException; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.config.BeanDefinitionHolder; import org.springframework.beans.factory.config.ConfigurableBeanFactory; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; +import org.springframework.beans.factory.support.RootBeanDefinition; import org.springframework.util.ClassUtils; /** @@ -80,8 +85,8 @@ public class PlaceholderProxyFactoryBean extends ProxyConfig implements FactoryB pf.addAdvice(new DelegatingIntroductionInterceptor(scopedObject)); // Add the AopInfrastructureBean marker to indicate that the scoped - // proxy - // itself is not subject to auto-proxying! Only its target bean is. + // proxy itself is not subject to auto-proxying! Only its target bean + // is. pf.addInterface(AopInfrastructureBean.class); this.proxy = pf.getProxy(cbf.getBeanClassLoader()); @@ -107,4 +112,55 @@ public class PlaceholderProxyFactoryBean extends ProxyConfig implements FactoryB public boolean isSingleton() { return true; } + + /** + * Convenience method to create a {@link BeanDefinition} for a target + * wrapped in a placeholder tarrget source, able to defer binding of + * placeholders until the bean is used. + * + * @param definition a target bean definition + * @param registry a {@link BeanDefinitionRegistry} + * @param proxyTargetClass true if we need to use CGlib to create the + * proxies + * @return a {@link BeanDefinitionHolder} for a + * {@link PlaceholderProxyFactoryBean} + */ + public static BeanDefinitionHolder createScopedProxy(BeanDefinitionHolder definition, + BeanDefinitionRegistry registry, boolean proxyTargetClass) { + + String originalBeanName = definition.getBeanName(); + BeanDefinition targetDefinition = definition.getBeanDefinition(); + + // Create a proxy definition for the original bean name, + // "hiding" the target bean in an internal target definition. + RootBeanDefinition proxyDefinition = new RootBeanDefinition(PlaceholderProxyFactoryBean.class); + proxyDefinition.getConstructorArgumentValues().addGenericArgumentValue(new StepContextFactory()); + proxyDefinition.setOriginatingBeanDefinition(definition.getBeanDefinition()); + proxyDefinition.setSource(definition.getSource()); + proxyDefinition.setRole(BeanDefinition.ROLE_INFRASTRUCTURE); + + String targetBeanName = "lazyBindingProxy." + originalBeanName; + proxyDefinition.getPropertyValues().addPropertyValue("targetBeanName", targetBeanName); + + if (proxyTargetClass) { + targetDefinition.setAttribute(AutoProxyUtils.PRESERVE_TARGET_CLASS_ATTRIBUTE, Boolean.TRUE); + // ProxyFactoryBean's "proxyTargetClass" default is TRUE, so we + // don't need to set it explicitly here. + } + else { + proxyDefinition.getPropertyValues().addPropertyValue("proxyTargetClass", Boolean.FALSE); + } + + proxyDefinition.setAutowireCandidate(targetDefinition.isAutowireCandidate()); + // The target bean should be ignored in favor of the proxy. + targetDefinition.setAutowireCandidate(false); + + // Register the target bean as separate bean in the factory. + registry.registerBeanDefinition(targetBeanName, targetDefinition); + + // Return the scoped proxy definition as primary bean definition + // (potentially an inner bean). + return new BeanDefinitionHolder(proxyDefinition, originalBeanName, definition.getAliases()); + } + } diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderTargetSource.java b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderTargetSource.java index faf968af3..6b56879a2 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderTargetSource.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/scope/util/PlaceholderTargetSource.java @@ -17,14 +17,15 @@ package org.springframework.batch.core.scope.util; import org.springframework.aop.TargetSource; import org.springframework.aop.target.SimpleBeanTargetSource; +import org.springframework.batch.core.scope.StepScope; import org.springframework.beans.BeanWrapper; import org.springframework.beans.BeanWrapperImpl; import org.springframework.beans.BeansException; import org.springframework.beans.TypeConverter; import org.springframework.beans.TypeMismatchException; import org.springframework.beans.factory.InitializingBean; -import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.BeanDefinitionVisitor; +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; import org.springframework.beans.factory.config.TypedStringValue; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.GenericBeanDefinition; @@ -57,10 +58,10 @@ public class PlaceholderTargetSource extends SimpleBeanTargetSource implements I private static final String PLACEHOLDER_SUFFIX = "}"; - private volatile boolean active = false; - private ContextFactory contextFactory; + private StepScope scope = new StepScope(); + /** * Public setter for the context factory. Used to construct the context root * whenever placeholders are replaced in a bean definition. @@ -93,10 +94,8 @@ public class PlaceholderTargetSource extends SimpleBeanTargetSource implements I final TypeConverter typeConverter = listableBeanFactory.getTypeConverter(); - // Try to prevent other threads from using this TypeConverter - active = true; - - listableBeanFactory.setTypeConverter(new TypeConverter() { + DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory(listableBeanFactory); + beanFactory.setTypeConverter(new TypeConverter() { @SuppressWarnings("unchecked") public Object convertIfNecessary(Object value, Class requiredType, MethodParameter methodParam) throws TypeMismatchException { @@ -127,8 +126,8 @@ public class PlaceholderTargetSource extends SimpleBeanTargetSource implements I * come back when getBean() is called later on */ String targetBeanName = getTargetBeanName(); - BeanDefinition originalDefinition = listableBeanFactory.getMergedBeanDefinition(getTargetBeanName()); - GenericBeanDefinition beanDefinition = new GenericBeanDefinition(originalDefinition); + GenericBeanDefinition beanDefinition = new GenericBeanDefinition(listableBeanFactory + .getMergedBeanDefinition(targetBeanName)); logger.debug("Rehydrating scoped target: [" + targetBeanName + "]"); BeanDefinitionVisitor visitor = new BeanDefinitionVisitor(new StringValueResolver() { @@ -156,18 +155,27 @@ public class PlaceholderTargetSource extends SimpleBeanTargetSource implements I }; - listableBeanFactory.registerBeanDefinition(beanName, beanDefinition); + String beanScope = beanDefinition.getScope(); + /* + * The scope for the bean really ought to be "step" if we get this + * far, but we'll be cautious anyway and avoid a potential issue + * with trying to replace the default scopes (which is not allowed) + */ + if (!beanScope.equals(ConfigurableListableBeanFactory.SCOPE_PROTOTYPE) + && !beanScope.equals(ConfigurableListableBeanFactory.SCOPE_SINGLETON)) { + // Need this otherwise there will be no step scope available. + beanFactory.registerScope(beanScope, scope); + } + beanFactory.registerBeanDefinition(beanName, beanDefinition); // Make the replacements before the target is hydrated visitor.visitBeanDefinition(beanDefinition); - return listableBeanFactory.getBean(beanName); + return beanFactory.getBean(beanName); } finally { - listableBeanFactory.removeBeanDefinition(beanName); - // Replace the original type converter. TODO: does this cause - // problems if in fact it was null to start with? - listableBeanFactory.setTypeConverter(typeConverter); - active = false; + beanFactory.destroySingletons(); + beanFactory = null; + // Anything else we can do to clean it up? } } @@ -179,13 +187,11 @@ public class PlaceholderTargetSource extends SimpleBeanTargetSource implements I */ private Object convertFromContext(String key, Class requiredType) { Object result = null; - if (active) { - BeanWrapper wrapper = new BeanWrapperImpl(contextFactory.getContext()); - if (wrapper.isReadableProperty(key)) { - Object property = wrapper.getPropertyValue(key); - if (requiredType.isAssignableFrom(property.getClass())) { - result = property; - } + BeanWrapper wrapper = new BeanWrapperImpl(contextFactory.getContext()); + if (wrapper.isReadableProperty(key)) { + Object property = wrapper.getPropertyValue(key); + if (requiredType.isAssignableFrom(property.getClass())) { + result = property; } } return result; diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/JobStartupRunner.java b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/JobStartupRunner.java new file mode 100644 index 000000000..c02a5ed38 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/JobStartupRunner.java @@ -0,0 +1,24 @@ +package org.springframework.batch.core.scope; + +import javax.annotation.PostConstruct; + +import org.springframework.batch.core.JobExecution; +import org.springframework.batch.core.Step; +import org.springframework.batch.core.StepExecution; + +public class JobStartupRunner { + + private Step step; + + public void setStep(Step step) { + this.step = step; + } + + @PostConstruct + public void launch() throws Exception { + StepExecution stepExecution = new StepExecution("step", new JobExecution(1L), 0L); + step.execute(stepExecution); + // expect no errors + } + +} diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopePlaceholderIntegrationTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopePlaceholderIntegrationTests.java index ee6ec6580..fb1440cda 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopePlaceholderIntegrationTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopePlaceholderIntegrationTests.java @@ -79,6 +79,10 @@ public class StepScopePlaceholderIntegrationTests implements BeanFactoryAware { @Test public void testSimpleProperty() throws Exception { assertEquals("bar", simple.getName()); + // Once the step context is set up it should be baked into the proxies + // so changing it now should have no effect + stepExecution.getExecutionContext().put("foo", "wrong!"); + assertEquals("bar", simple.getName()); } @Test diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests.java new file mode 100644 index 000000000..fab5e2a69 --- /dev/null +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests.java @@ -0,0 +1,17 @@ +package org.springframework.batch.core.scope; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; + + +@ContextConfiguration +@RunWith(SpringJUnit4ClassRunner.class) +public class StepScopeStartupIntegrationTests { + + @Test + public void testScopedProxyDuringStartup() throws Exception { + } + +} diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/TestStep.java b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/TestStep.java index f5ba9ad31..e8d7d8fb6 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/scope/TestStep.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/scope/TestStep.java @@ -24,6 +24,12 @@ public class TestStep implements Step { public void execute(StepExecution stepExecution) throws JobInterruptedException { context = StepSynchronizationManager.getContext(); + setContextFromCollaborator(); + stepExecution.getExecutionContext().put("foo", "changed but it shouldn't affect the collaborator"); + setContextFromCollaborator(); + } + + private void setContextFromCollaborator() { if (context != null) { context.setAttribute("collaborator", collaborator.getName()); } diff --git a/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeIntegrationTests-context.xml b/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeIntegrationTests-context.xml index 95608805a..138af872c 100644 --- a/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeIntegrationTests-context.xml +++ b/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeIntegrationTests-context.xml @@ -5,7 +5,7 @@ xsi:schemaLocation=" http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd http://www.springframework.org/schema/aop http://www.springframework.org/schema/aop/spring-aop.xsd - http://www.springframework.org/schema/tx http://www.springframework.org/schema/tx/spring-tx.xsd"> + http://www.springframework.org/schema/tx http://www.springframework.org/schema/tx/spring-tx.xsd"> @@ -40,4 +40,5 @@ + \ No newline at end of file diff --git a/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests-context.xml b/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests-context.xml new file mode 100644 index 000000000..8d7bc6089 --- /dev/null +++ b/spring-batch-core/src/test/resources/org/springframework/batch/core/scope/StepScopeStartupIntegrationTests-context.xml @@ -0,0 +1,31 @@ + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file