IN PROGRESS - issue BATCH-572: Retryable exceptions cannot be skippable

StatefulRetryStepFactoryBean functionality all moved into SkipLimitStepFactoryBean (the former should be removed). TODO: take some more care with exception types that overlap between retry / skip.

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

@@ -8,6 +8,8 @@ import java.util.List;
 
 import junit.framework.TestCase;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.batch.core.JobExecution;
 import org.springframework.batch.core.JobInstance;
 import org.springframework.batch.core.JobParameters;
@@ -34,6 +36,8 @@ import org.springframework.util.StringUtils;
  */
 public class SkipLimitStepFactoryBeanTests extends TestCase {
 
+	protected final Log logger = LogFactory.getLog(getClass());
+
 	private SkipLimitStepFactoryBean factory = new SkipLimitStepFactoryBean();
 
 	private Class[] skippableExceptions = new Class[] { SkippableException.class, SkippableRuntimeException.class };
@@ -73,11 +77,10 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 		StepExecution stepExecution = new StepExecution(step.getName(), jobExecution);
 		step.execute(stepExecution);
 
-		
 		assertEquals(2, stepExecution.getSkipCount());
 		assertEquals(1, stepExecution.getReadSkipCount().intValue());
 		assertEquals(1, stepExecution.getWriteSkipCount().intValue());
-		
+
 		// only write exception caused rollback
 		assertEquals(1, stepExecution.getRollbackCount().intValue());
 
@@ -192,11 +195,13 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 		assertEquals(1, stepExecution.getWriteSkipCount().intValue());
 
 		// writer did not skip "2" as it never made it to writer, only "4" did
+		assertFalse(reader.processed.contains("2"));
 		assertTrue(reader.processed.contains("4"));
 
-		// failure on "4" tripped the skip limit so we never write anything
-		// ("1" was written but rolled back)
-		List expectedOutput = Arrays.asList(StringUtils.commaDelimitedListToStringArray(""));
+		// failure on "5" tripped the skip limit but "4" failed on write and was skipped and 
+		// RepeatSynchronizationManager.setCompleteOnly() was called in the retry policy to
+		// aggressively commit after a recovery ("1" was written at that point)
+		List expectedOutput = Arrays.asList(StringUtils.commaDelimitedListToStringArray("1"));
 		assertEquals(expectedOutput, writer.written);
 
 	}
@@ -289,6 +294,8 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 	 */
 	private static class SkipReaderStub implements ItemReader {
 
+		protected final Log logger = LogFactory.getLog(getClass());
+
 		private final String[] items;
 
 		private Collection processed = new ArrayList();
@@ -311,22 +318,27 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 		public Object read() throws Exception, UnexpectedInputException, NoWorkFoundException, ParseException {
 			counter++;
 			if (counter >= items.length) {
+				logger.debug("Returning null at count=" + counter);
 				return null;
 			}
 			String item = items[counter];
 			if (failures.contains(item)) {
+				logger.debug("Throwing exception for [" + item + "] at count=" + counter);
 				throw new SkippableException("exception in reader");
 			}
 			processed.add(item);
+			logger.debug("Returning [" + item + "] at count=" + counter);
 			return item;
 		}
 
 		public void mark() throws MarkFailedException {
+			logger.debug("Marked at count=" + counter);
 			marked = counter;
 		}
 
 		public void reset() throws ResetFailedException {
 			counter = marked;
+			logger.debug("Reset at count=" + counter);
 		}
 
 	}
@@ -336,9 +348,11 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 	 */
 	private static class SkipWriterStub implements ItemWriter {
 
-		List written = new ArrayList();
+		protected final Log logger = LogFactory.getLog(getClass());
 
-		int flushIndex = -1;
+		private List written = new ArrayList();
+
+		private int flushIndex = -1;
 
 		private final Collection failures;
 
@@ -355,7 +369,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 
 		public void clear() throws ClearFailedException {
 			for (int i = flushIndex + 1; i < written.size(); i++) {
-				written.remove(i);
+				written.remove(written.size()-1);
 			}
 		}
 
@@ -365,6 +379,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 
 		public void write(Object item) throws Exception {
 			if (failures.contains(item)) {
+				logger.debug("Throwing write exception on [" + item + "]");
 				throw new SkippableRuntimeException("exception in writer");
 			}
 			written.add(item);

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

@@ -89,6 +89,7 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		factory.setItemWriter(processor);
 		factory.setJobRepository(repository);
 		factory.setTransactionManager(new ResourcelessTransactionManager());
+		factory.setRetryableExceptionClasses(new Class[] { Exception.class });
 
 		JobSupport job = new JobSupport("jobName");
 		job.setRestartable(true);
@@ -107,7 +108,15 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		assertTrue(factory.getObject() instanceof Step);
 	}
 
-	public void testSuccessfulRetry() throws Exception {
+	/**
+	 * N.B. this doesn't really test retry, since the retry is only on write
+	 * failures, but it does test that read errors are re-presented for another
+	 * try when the retryLimit is high enough (it is used to build an exception
+	 * handler).
+	 * 
+	 * @throws Exception
+	 */
+	public void testSuccessfulRetryWithReadFailure() throws Exception {
 		List items = TransactionAwareProxyFactory.createTransactionalList();
 		items.addAll(Arrays.asList(new String[] { "a", "b", "c" }));
 		ItemReader provider = new ListItemReader(items) {
@@ -122,6 +131,7 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		};
 		factory.setItemReader(provider);
 		factory.setRetryLimit(10);
+		factory.setSkippableExceptionClasses(new Class[0]);
 		AbstractStep step = (AbstractStep) factory.getObject();
 
 		StepExecution stepExecution = new StepExecution(step.getName(), jobExecution);
@@ -129,7 +139,7 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 
 		assertEquals(0, stepExecution.getSkipCount());
 
-		// b is processed twice, plus 1, plus c, plus the null at end
+		// b is processed twice, plus a, plus c, plus the null at end
 		assertEquals(5, count);
 	}
 
@@ -201,7 +211,8 @@ public class StatefulRetryStepFactoryBeanTests extends TestCase {
 		assertEquals(2, recovered.size());
 		assertEquals(2, stepExecution.getSkipCount());
 		assertEquals(2, stepExecution.getWriteSkipCount().intValue());
-		// each item once, plus 5 failed retries each for b and d, plus the null terminator
+		// each item once, plus 5 failed retries each for b and d, plus the null
+		// terminator
 		assertEquals(17, count);
 	}
 }