RESOLVED - issue BATCH-565: StatefulRetryStepFactoryBean ignores skip configuration

Added additional protected getter so the skip policy can be determined from base class

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

@@ -133,16 +133,16 @@ public class SimpleStepFactoryBean extends AbstractStepFactoryBean {
 	}
 
 	/**
-	 * Public getter for the ItemProcessor.
-	 * @return the itemProcessor
+	 * Public getter for the ItemHandler.
+	 * @return the ItemHandler
 	 */
 	protected ItemHandler getItemHandler() {
 		return itemHandler;
 	}
 
 	/**
-	 * Public setter for the ItemProcessor.
-	 * @param itemHandler the itemProcessor to set
+	 * Public setter for the ItemHandler.
+	 * @param itemHandler the ItemHandler to set
 	 */
 	protected void setItemHandler(ItemHandler itemHandler) {
 		this.itemHandler = itemHandler;

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

@@ -4,6 +4,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
+import org.springframework.batch.core.step.skip.ItemSkipPolicy;
 import org.springframework.batch.core.step.skip.LimitCheckingItemSkipPolicy;
 import org.springframework.batch.core.step.skip.NeverSkipItemSkipPolicy;
 import org.springframework.batch.core.step.skip.SkipLimitExceededException;
@@ -37,6 +38,8 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 
 	private int skipCacheCapacity = 1024;
 
+	private ItemSkipPolicy itemSkipPolicy;
+
 	/**
 	 * Public setter for a limit that determines skip policy. If this value is
 	 * positive then an exception in chunk processing will cause the item to be
@@ -96,6 +99,14 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 	protected ItemKeyGenerator getItemKeyGenerator() {
 		return itemKeyGenerator;
 	}
+	
+	/**
+	 * Protected getter for the {@link ItemSkipPolicy}.
+	 * @return the itemSkipPolicy
+	 */
+	protected ItemSkipPolicy getItemSkipPolicy() {
+		return itemSkipPolicy;
+	}
 
 	/**
 	 * Public setter for the capacity of the skipped item cache. If a large
@@ -134,9 +145,10 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 			addFatalExceptionIfMissing(SkipLimitExceededException.class);
 			List fatalExceptionList = Arrays.asList(fatalExceptionClasses);
 
-			LimitCheckingItemSkipPolicy skipPolicy = new LimitCheckingItemSkipPolicy(skipLimit, Arrays
+			LimitCheckingItemSkipPolicy limitCheckingSkipPolicy = new LimitCheckingItemSkipPolicy(skipLimit, Arrays
 					.asList(skippableExceptionClasses), fatalExceptionList);
-			itemHandler.setItemSkipPolicy(skipPolicy);
+			itemHandler.setItemSkipPolicy(limitCheckingSkipPolicy);
+			this.itemSkipPolicy = limitCheckingSkipPolicy;
 			SimpleLimitExceptionHandler exceptionHandler = new SimpleLimitExceptionHandler(skipLimit);
 			exceptionHandler.setExceptionClasses(skippableExceptionClasses);
 			exceptionHandler.setFatalExceptionClasses(fatalExceptionClasses);

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

@@ -142,6 +142,7 @@ public class StatefulRetryStepFactoryBean extends SkipLimitStepFactoryBean {
 
 			StatefulRetryItemHandler itemHandler = new StatefulRetryItemHandler(getItemReader(), getItemWriter(),
 					retryTemplate, getItemKeyGenerator(), itemRecoverer);
+			itemHandler.setItemSkipPolicy(getItemSkipPolicy());
 
 			step.setItemHandler(itemHandler);
 

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

@@ -25,7 +25,9 @@ import org.springframework.batch.item.NoWorkFoundException;
 import org.springframework.batch.item.ParseException;
 import org.springframework.batch.item.ResetFailedException;
 import org.springframework.batch.item.UnexpectedInputException;
+import org.springframework.batch.item.support.ListItemReader;
 import org.springframework.batch.support.transaction.ResourcelessTransactionManager;
+import org.springframework.batch.support.transaction.TransactionAwareProxyFactory;
 import org.springframework.util.StringUtils;
 
 /**
@@ -47,6 +49,8 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 
 	JobExecution jobExecution;
 
+	protected int count;
+
 	protected void setUp() throws Exception {
 		factory.setJobRepository(new JobRepositorySupport());
 		factory.setTransactionManager(new ResourcelessTransactionManager());
@@ -222,6 +226,32 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 
 	}
 
+	public void testDefaultSkipPolicy() throws Exception {
+		factory.setSkippableExceptionClasses(new Class[] {Exception.class});
+		factory.setSkipLimit(1);
+		List items = TransactionAwareProxyFactory.createTransactionalList();
+		items.addAll(Arrays.asList(new String[] { "a", "b", "c" }));
+		ItemReader provider = new ListItemReader(items) {
+			public Object read() {
+				Object item = super.read();
+				count++;
+				if ("b".equals(item)) {
+					throw new RuntimeException("Read error - planned failure.");
+				}
+				return item;
+			}
+		};
+		factory.setItemReader(provider);
+		AbstractStep step = (AbstractStep) factory.getObject();
+
+		StepExecution stepExecution = new StepExecution(step, jobExecution);
+		step.execute(stepExecution);
+		
+		assertEquals(1, stepExecution.getSkipCount());
+		// b is processed once and skipped, plus 1, plus c, plus the null at end
+		assertEquals(4, count);
+	}
+
 	/**
 	 * Simple item reader that supports skip functionality.
 	 */

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

@@ -53,6 +53,8 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 	private List recovered = new ArrayList();
 
 	private List processed = new ArrayList();
+	
+	int count = 0;	
 
 	private SimpleJobRepository repository = new SimpleJobRepository(new MapJobInstanceDao(), new MapJobExecutionDao(),
 			new MapStepExecutionDao());
@@ -109,20 +111,51 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		List items = TransactionAwareProxyFactory.createTransactionalList();
 		items.addAll(Arrays.asList(new String[] { "a", "b", "c" }));
 		ItemReader provider = new ListItemReader(items) {
-			int count = 0;
 			public Object read() {
+				Object item = super.read();
 				count++;
 				if (count == 2) {
 					throw new RuntimeException("Temporary error - retry for success.");
 				}
-				return super.read();
+				return item;
 			}
 		};
 		factory.setItemReader(provider);
 		factory.setRetryLimit(10);
 		AbstractStep step = (AbstractStep) factory.getObject();
 
-		step.execute(new StepExecution(step, jobExecution));
+		StepExecution stepExecution = new StepExecution(step, jobExecution);
+		step.execute(stepExecution);
+		
+		assertEquals(0, stepExecution.getSkipCount());
+		// b is processed twice, plus 1, plus c, plus the null at end
+		assertEquals(5, count);
 	}
 	
+	public void testSkipAndRetry() throws Exception {
+		factory.setSkippableExceptionClasses(new Class[] {Exception.class});
+		factory.setSkipLimit(1);
+		List items = TransactionAwareProxyFactory.createTransactionalList();
+		items.addAll(Arrays.asList(new String[] { "a", "b", "c" }));
+		ItemReader provider = new ListItemReader(items) {
+			public Object read() {
+				Object item = super.read();
+				count++;
+				if ("b".equals(item)) {
+					throw new RuntimeException("Read error - planned but skippable.");
+				}
+				return item;
+			}
+		};
+		factory.setItemReader(provider);
+		factory.setRetryLimit(10);
+		AbstractStep step = (AbstractStep) factory.getObject();
+
+		StepExecution stepExecution = new StepExecution(step, jobExecution);
+		step.execute(stepExecution);
+		
+		assertEquals(1, stepExecution.getSkipCount());
+		// b is processed once and skipped, plus 1, plus c, plus the null at end
+		assertEquals(4, count);
+	}
 }

spring-batch-core/src/test/resources/log4j.properties

@@ -6,8 +6,8 @@ log4j.appender.stdout.layout.ConversionPattern=%d %p [%c] - <%m>%n
 
 log4j.category.org.apache.activemq=ERROR
 log4j.category.org.springframework.batch=DEBUG
-log4j.category.org.springframework.transaction=INFO
+# log4j.category.org.springframework.transaction=INFO
 
-log4j.category.org.hibernate.SQL=DEBUG
+# log4j.category.org.hibernate.SQL=DEBUG
 # for debugging datasource initialization
 # log4j.category.test.jdbc=DEBUG