BATCH-1739: fix all retry/skip inconsistencies *except* with late bin…

…ding of limits

spring-batch-core/.settings/com.springsource.sts.config.flow.prefs

@@ -1,6 +1,10 @@
-#Tue Mar 15 13:48:56 GMT 2011
+#Wed May 04 11:15:42 BST 2011
 //com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/JobExecutionListenerMethodAttributeParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element type\="job">\n<structure end\="995" endstart\="989" start\="511" startend\="586"/>\n<bounds height\="118" width\="77" x\="15" y\="17"/>\n</element>\n</graph>
 //com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/JobRepositoryDefaultParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph/>
+//com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentRetryableLateBindingStepFactoryBeanParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="StepModelElement" type\="step">\n<structure end\="771" endstart\="764" start\="510" startend\="600"/>\n<bounds height\="34" width\="95" x\="106" y\="19"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1011" endstart\="1005" start\="774" startend\="788"/>\n<bounds height\="128" width\="77" x\="15" y\="19"/>\n</element>\n</graph>
+//com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentRetryableStepFactoryBeanParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="StepModelElement" type\="step">\n<structure end\="700" endstart\="693" start\="439" startend\="529"/>\n<bounds height\="34" width\="95" x\="106" y\="19"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="955" endstart\="949" start\="703" startend\="769"/>\n<bounds height\="128" width\="77" x\="15" y\="19"/>\n</element>\n</graph>
+//com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentSkippableLateBindingStepFactoryBeanParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="StepModelElement" type\="step">\n<structure end\="771" endstart\="764" start\="510" startend\="600"/>\n<bounds height\="34" width\="95" x\="106" y\="19"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1006" endstart\="1000" start\="774" startend\="788"/>\n<bounds height\="128" width\="77" x\="15" y\="19"/>\n</element>\n</graph>
+//com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentSkippableStepFactoryBeanParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="StepModelElement" type\="step">\n<structure end\="700" endstart\="693" start\="439" startend\="529"/>\n<bounds height\="34" width\="95" x\="106" y\="19"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="953" endstart\="947" start\="703" startend\="769"/>\n<bounds height\="128" width\="77" x\="15" y\="19"/>\n</element>\n</graph>
 //com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/PartitionStepParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="713" endstart\="707" start\="510" startend\="525"/>\n<bounds height\="128" width\="84" x\="15" y\="17"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="884" endstart\="878" start\="716" startend\="731"/>\n<bounds height\="172" width\="84" x\="207" y\="17"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1042" endstart\="1036" start\="887" startend\="902"/>\n<bounds height\="128" width\="84" x\="111" y\="17"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1295" endstart\="1289" start\="1045" startend\="1060"/>\n<bounds height\="128" width\="84" x\="303" y\="17"/>\n</element>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1412" endstart\="1406" start\="1298" startend\="1313"/>\n<bounds height\="128" width\="84" x\="399" y\="17"/>\n</element>\n</graph>
 //com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/StepListenerInStepParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element clazz\="JobModelElement" type\="job">\n<structure end\="1832" endstart\="1826" start\="510" startend\="524"/>\n<bounds height\="268" width\="77" x\="15" y\="17"/>\n</element>\n</graph>
 //com.springsource.sts.config.flow.coordinates\:http\://www.springframework.org/schema/batch\:/spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/StepListenerMethodAttributeParserTests-context.xml=<?xml version\="1.0" encoding\="UTF-8"?>\n<graph>\n<element type\="job">\n<structure end\="854" endstart\="848" start\="511" startend\="525"/>\n<bounds height\="118" width\="77" x\="15" y\="17"/>\n</element>\n</graph>

spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/ChunkElementParser.java

@@ -124,11 +124,11 @@ protected void parse(Element element, AbstractBeanDefinition bd, ParserContext p
 		ManagedMap skippableExceptions = handleExceptionElement(element, parserContext, "skippable-exception-classes");
 		boolean hasSkipPolicy = false;
 		if (StringUtils.hasText(skipLimit)) {
-			if (skippableExceptions == null) {
-				parserContext.getReaderContext().error(
-						"The <chunk/> element must have skippable-exceptions if a skip-limit is specified.", element);
-			}
 			if (skipLimit.startsWith("#")) {
+				if (skippableExceptions == null) {
+					parserContext.getReaderContext().error(
+							"The <chunk/> element must have skippable-exceptions if a skip-limit is specified.", element);
+				}
 				// It's a late binding expression, so we need step scope...
 				BeanDefinitionBuilder skipPolicy = BeanDefinitionBuilder
 						.genericBeanDefinition(LimitCheckingItemSkipPolicy.class);
@@ -139,6 +139,10 @@ protected void parse(Element element, AbstractBeanDefinition bd, ParserContext p
 				hasSkipPolicy = true;
 			}
 			else {
+				if (skippableExceptions == null) {
+					skippableExceptions = new ManagedMap();
+					skippableExceptions.setMergeEnabled(true);
+				}
 				propertyValues.addPropertyValue("skipLimit", skipLimit);
 			}
 		}
@@ -155,11 +159,11 @@ protected void parse(Element element, AbstractBeanDefinition bd, ParserContext p
 		ManagedMap retryableExceptions = handleExceptionElement(element, parserContext, "retryable-exception-classes");
 		boolean hasRetryPolicy = false;
 		if (StringUtils.hasText(retryLimit)) {
-			if (retryableExceptions == null) {
-				parserContext.getReaderContext().error(
-						"The <chunk/> element must have retryable-exceptions if a retry-limit is specified.", element);
-			}
 			if (retryLimit.startsWith("#")) {
+				if (retryableExceptions == null) {
+					parserContext.getReaderContext().error(
+							"The <chunk/> element must have retryable-exceptions if a retry-limit is specified.", element);
+				}
 				// It's a late binding expression, so we need step scope...
 				BeanDefinitionBuilder retryPolicy = BeanDefinitionBuilder
 						.genericBeanDefinition(SimpleRetryPolicy.class);
@@ -170,6 +174,10 @@ protected void parse(Element element, AbstractBeanDefinition bd, ParserContext p
 				hasRetryPolicy = true;
 			}
 			else {
+				if (retryableExceptions == null) {
+					retryableExceptions = new ManagedMap();
+					retryableExceptions.setMergeEnabled(true);
+				}
 				propertyValues.addPropertyValue("retryLimit", retryLimit);
 			}
 		}

spring-batch-core/src/main/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBean.java

@@ -59,6 +59,7 @@
 import org.springframework.transaction.TransactionException;
 import org.springframework.transaction.interceptor.DefaultTransactionAttribute;
 import org.springframework.transaction.interceptor.TransactionAttribute;
+import org.springframework.util.Assert;
 
 /**
  * Factory bean for step that provides options for configuring skip behaviour.
@@ -393,9 +394,13 @@ protected SimpleChunkProvider<T> configureChunkProvider() {
 	 */
 	protected SkipPolicy createSkipPolicy() {
 		SkipPolicy skipPolicy = this.skipPolicy;
-		LimitCheckingItemSkipPolicy limitCheckingItemSkipPolicy = new LimitCheckingItemSkipPolicy(skipLimit,
-				getSkippableExceptionClasses());
+		Map<Class<? extends Throwable>, Boolean> map = new HashMap<Class<? extends Throwable>, Boolean>(
+				skippableExceptionClasses);
+		map.put(ForceRollbackForWriteSkipException.class, true);
+		LimitCheckingItemSkipPolicy limitCheckingItemSkipPolicy = new LimitCheckingItemSkipPolicy(skipLimit, map);
 		if (skipPolicy == null) {
+			Assert.state(!(skippableExceptionClasses.isEmpty() && skipLimit > 0),
+					"If a skip limit is provided then skippable exceptions must also be specified");
 			skipPolicy = limitCheckingItemSkipPolicy;
 		}
 		else if (limitCheckingItemSkipPolicy != null) {
@@ -429,16 +434,6 @@ protected SimpleChunkProcessor<T, S> configureChunkProcessor() {
 
 	}
 
-	/**
-	 * @return
-	 */
-	private Map<Class<? extends Throwable>, Boolean> getSkippableExceptionClasses() {
-		Map<Class<? extends Throwable>, Boolean> map = new HashMap<Class<? extends Throwable>, Boolean>(
-				skippableExceptionClasses);
-		map.put(ForceRollbackForWriteSkipException.class, true);
-		return map;
-	}
-
 	/**
 	 * @return fully configured retry template for item processing phase.
 	 */
@@ -453,6 +448,8 @@ private BatchRetryTemplate configureRetry() {
 		simpleRetryPolicy = new SimpleRetryPolicy(retryLimit, map);
 
 		if (retryPolicy == null) {
+			Assert.state(!(retryableExceptionClasses.isEmpty() && retryLimit > 0),
+					"If a retry limit is provided then retryable exceptions must also be specified");
 			retryPolicy = simpleRetryPolicy;
 		}
 		else if ((!retryableExceptionClasses.isEmpty() && retryLimit > 0)) {

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/ChunkElementParserTests.java

@@ -38,7 +38,6 @@
 import org.springframework.batch.retry.policy.SimpleRetryPolicy;
 import org.springframework.beans.PropertyAccessorUtils;
 import org.springframework.beans.factory.BeanCreationException;
-import org.springframework.beans.factory.parsing.BeanDefinitionParsingException;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.context.support.ClassPathXmlApplicationContext;
@@ -91,8 +90,8 @@ public void testIllegalSkipAndRetryAttributes() throws Exception {
 				"org/springframework/batch/core/configuration/xml/ChunkElementIllegalSkipAndRetryAttributeParserTests-context.xml");
 		Step step = (Step) context.getBean("s1", Step.class);
 		assertNotNull("Step not parsed", step);
-		fail("Expected BeanDefinitionParsingException");
-		} catch (BeanDefinitionParsingException e) {
+		fail("Expected BeanCreationException");
+		} catch (BeanCreationException e) {
 			// expected
 		}
 	}

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/ParentStepFactoryBeanParserTests.java

@@ -18,6 +18,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import org.junit.Ignore;
 import org.junit.Test;
 import org.springframework.batch.core.Step;
 import org.springframework.batch.core.step.item.FaultTolerantChunkProcessor;
@@ -53,4 +54,39 @@ public void testSkippableAttributes() throws Exception {
 		assertTrue("Wrong processor type", chunkProcessor instanceof FaultTolerantChunkProcessor<?,?>);
 	}
 
+	@Test
+	public void testRetryableAttributes() throws Exception {
+		ConfigurableApplicationContext context = new ClassPathXmlApplicationContext(
+				"org/springframework/batch/core/configuration/xml/ParentRetryableStepFactoryBeanParserTests-context.xml");
+		Object step = context.getBean("s1", Step.class);
+		assertNotNull("Step not parsed", step);
+		Object tasklet = ReflectionTestUtils.getField(step, "tasklet");
+		Object chunkProcessor = ReflectionTestUtils.getField(tasklet, "chunkProcessor");
+		assertTrue("Wrong processor type", chunkProcessor instanceof FaultTolerantChunkProcessor<?,?>);
+	}
+
+	@Test
+	@Ignore // Fix this BATCH-1739
+	public void testRetryableLateBindingAttributes() throws Exception {
+		ConfigurableApplicationContext context = new ClassPathXmlApplicationContext(
+				"org/springframework/batch/core/configuration/xml/ParentRetryableLateBindingStepFactoryBeanParserTests-context.xml");
+		Object step = context.getBean("s1", Step.class);
+		assertNotNull("Step not parsed", step);
+		Object tasklet = ReflectionTestUtils.getField(step, "tasklet");
+		Object chunkProcessor = ReflectionTestUtils.getField(tasklet, "chunkProcessor");
+		assertTrue("Wrong processor type", chunkProcessor instanceof FaultTolerantChunkProcessor<?,?>);
+	}
+
+	@Test
+	@Ignore // Fix this BATCH-1739
+	public void testSkippableLateBindingAttributes() throws Exception {
+		ConfigurableApplicationContext context = new ClassPathXmlApplicationContext(
+				"org/springframework/batch/core/configuration/xml/ParentSkippableLateBindingStepFactoryBeanParserTests-context.xml");
+		Object step = context.getBean("s1", Step.class);
+		assertNotNull("Step not parsed", step);
+		Object tasklet = ReflectionTestUtils.getField(step, "tasklet");
+		Object chunkProcessor = ReflectionTestUtils.getField(tasklet, "chunkProcessor");
+		assertTrue("Wrong processor type", chunkProcessor instanceof FaultTolerantChunkProcessor<?,?>);
+	}
+
 }

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBeanTests.java

@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertTrue;
 
 import java.util.HashMap;
+import java.util.Map;
 
 import org.junit.Test;
 import org.springframework.aop.framework.Advised;
@@ -226,8 +227,10 @@ public void testFaultTolerantStep() throws Exception {
 		fb.setSkipLimit(100);
 		fb.setThrottleLimit(10);
 		fb.setRetryListeners(new RetryListenerSupport());
-		fb.setSkippableExceptionClasses(new HashMap<Class<? extends Throwable>, Boolean>());
-		fb.setRetryableExceptionClasses(new HashMap<Class<? extends Throwable>, Boolean>());
+		@SuppressWarnings("unchecked")
+		Map<Class<? extends Throwable>, Boolean> exceptionMap = getExceptionMap(Exception.class);
+		fb.setSkippableExceptionClasses(exceptionMap);
+		fb.setRetryableExceptionClasses(exceptionMap);
 
 		Object step = fb.getObject();
 		assertTrue(step instanceof TaskletStep);
@@ -297,4 +300,12 @@ public void testFlowStep() throws Exception {
 		assertTrue(handler instanceof SimpleFlow);
 	}
 
+	private Map<Class<? extends Throwable>, Boolean> getExceptionMap(Class<? extends Throwable>... args) {
+		Map<Class<? extends Throwable>, Boolean> map = new HashMap<Class<? extends Throwable>, Boolean>();
+		for (Class<? extends Throwable> arg : args) {
+			map.put(arg, true);
+		}
+		return map;
+	}
+
 }

spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanRollbackTests.java

@@ -149,7 +149,7 @@ public void testReaderAttributesOverrideSkippableNoRollback() throws Exception {
 		reader.setExceptionType(SkippableException.class);
 
 		// No skips by default
-		factory.setSkippableExceptionClasses(getExceptionMap());
+		factory.setSkippableExceptionClasses(getExceptionMap(RuntimeException.class));
 		// But this one is explicit in the tx-attrs so it should be skipped
 		factory.setNoRollbackExceptionClasses(getExceptionList(SkippableException.class));
 

spring-batch-core/src/test/java/org/springframework/batch/core/step/item/FaultTolerantStepFactoryBeanTests.java

@@ -127,7 +127,7 @@ public void testNonSkippableExceptionOnRead() throws Exception {
 		reader.setFailures("2");
 
 		// nothing is skippable
-		factory.setSkippableExceptionClasses(getExceptionMap());
+		factory.setSkippableExceptionClasses(getExceptionMap(NonExistentException.class));
 
 		Step step = (Step) factory.getObject();
 
@@ -144,7 +144,7 @@ public void testNonSkippableExceptionOnRead() throws Exception {
 	@Test
 	public void testNonSkippableException() throws Exception {
 		// nothing is skippable
-		factory.setSkippableExceptionClasses(getExceptionMap());
+		factory.setSkippableExceptionClasses(getExceptionMap(NonExistentException.class));
 		factory.setCommitInterval(1);
 
 		// no failures on read
@@ -1066,5 +1066,9 @@ private Map<Class<? extends Throwable>, Boolean> getExceptionMap(Class<? extends
 		}
 		return map;
 	}
+
+	public static class NonExistentException extends Exception {
+
+	}
 
 }

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentRetryableLateBindingStepFactoryBeanParserTests-context.xml

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans:beans xmlns="http://www.springframework.org/schema/batch" xmlns:beans="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://www.springframework.org/schema/batch http://www.springframework.org/schema/batch/spring-batch-2.1.xsd
+	http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">
+
+	<beans:import resource="common-context.xml" />
+
+	<step id="stepParent" abstract="true" xmlns="http://www.springframework.org/schema/batch">
+		<tasklet>
+			<chunk>
+				<retryable-exception-classes>
+					<include class="java.lang.Exception" />
+				</retryable-exception-classes>
+			</chunk>
+		</tasklet>
+	</step>
+
+	<job id="job">
+		<step id="s1" parent="stepParent">
+			<tasklet>
+				<chunk reader="reader" writer="writer" processor="processor" commit-interval="1"
+					retry-limit="#{jobParameters['retry.interval']}"/>
+			</tasklet>
+		</step>
+	</job>
+
+</beans:beans>

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentRetryableStepFactoryBeanParserTests-context.xml

@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://www.springframework.org/schema/batch http://www.springframework.org/schema/batch/spring-batch-2.1.xsd
+	http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">
+
+	<import resource="common-context.xml" />
+
+	<step id="stepParent" abstract="true" xmlns="http://www.springframework.org/schema/batch">
+		<tasklet>
+			<chunk>
+				<retryable-exception-classes>
+					<include class="java.lang.Exception" />
+				</retryable-exception-classes>
+			</chunk>
+		</tasklet>
+	</step>
+
+	<job id="job" xmlns="http://www.springframework.org/schema/batch">
+		<step id="s1" parent="stepParent">
+			<tasklet>
+				<chunk reader="reader" writer="writer" processor="processor" commit-interval="5" retry-limit="1" />
+			</tasklet>
+		</step>
+	</job>
+
+</beans>

spring-batch-core/src/test/resources/org/springframework/batch/core/configuration/xml/ParentSkippableLateBindingStepFactoryBeanParserTests-context.xml

@@ -0,0 +1,28 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans:beans xmlns="http://www.springframework.org/schema/batch" xmlns:beans="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xsi:schemaLocation="http://www.springframework.org/schema/batch http://www.springframework.org/schema/batch/spring-batch-2.1.xsd
+	http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">
+
+	<beans:import resource="common-context.xml" />
+
+	<step id="stepParent" abstract="true" xmlns="http://www.springframework.org/schema/batch">
+		<tasklet>
+			<chunk>
+				<skippable-exception-classes>
+					<include class="java.lang.Exception" />
+				</skippable-exception-classes>
+			</chunk>
+		</tasklet>
+	</step>
+
+	<job id="job">
+		<step id="s1" parent="stepParent">
+			<tasklet>
+				<chunk reader="reader" writer="writer" processor="processor" commit-interval="1"
+					skip-limit="#{jobParameters['skip.limit']}"/>
+			</tasklet>
+		</step>
+	</job>
+
+</beans:beans>