RESOLVED - issue BATCH-485: SkippableExceptionClasses should be inverted

http://jira.springframework.org/browse/BATCH-485

SkipLimitStepFactoryBean now declares setter for fatal exceptions that are never skipped and immediately rethrown by exception handlerRESOLVED - issue BATCH-485: SkippableExceptionClasses should be inverted 
http://jira.springframework.org/browse/BATCH-485

SkipLimitStepFactoryBean now declares setter for fatal exceptions that are never skipped and immediately rethrown by exception handler.
LimitCheckingItemSkipPolicy and SimpleLimitExceptionHandler were modified to recognize fatal exceptions.

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

@@ -7,15 +7,28 @@ import org.springframework.batch.core.step.skip.NeverSkipItemSkipPolicy;
 import org.springframework.batch.repeat.exception.SimpleLimitExceptionHandler;
 
 /**
- * Factory bean for step that allows configuring skip limit.
+ * Factory bean for step that provides options for configuring skip behavior.
+ * User can set {@link #skipLimit} to set how many exceptions of
+ * {@link #skippableExceptionClasses} types are tolerated.
+ * {@link #fatalExceptionClasses} will cause immediate termination of job - they
+ * are treated as higher priority than {@link #skippableExceptionClasses}, so
+ * the two lists don't need to be exclusive.
+ * 
+ * @see SimpleStepFactoryBean
+ * @see StatefulRetryStepFactoryBean
+ * 
+ * @author Dave Syer
+ * @author Robert Kasanicky
  * 
  */
 public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 
 	private int skipLimit = 0;
-	
-	private Class[] skippableExceptionClasses = new Class[]{ Exception.class };
-	
+
+	private Class[] skippableExceptionClasses = new Class[] { Exception.class };
+
+	private Class[] fatalExceptionClasses = new Class[] { Error.class };
+
 	/**
 	 * 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
@@ -28,11 +41,11 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 	public void setSkipLimit(int skipLimit) {
 		this.skipLimit = skipLimit;
 	}
-	
+
 	/**
 	 * Public setter for exception classes that when raised won't crash the job
-	 * but will result in transaction rollback and the item which handling caused
-	 * the exception will be skipped.
+	 * but will result in transaction rollback and the item which handling
+	 * caused the exception will be skipped.
 	 * 
 	 * @param skippableExceptionClasses defaults to <code>Exception</code>
 	 */
@@ -41,12 +54,21 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 	}
 
 	/**
-	 * Uses the {@link #skipLimit} value to configure item handler and
-	 * and exception handler.
+	 * Public setter for exception classes that should cause immediate failure.
+	 * 
+	 * @param fatalExceptionClasses {@link Error} by default
+	 */
+	public void setFatalExceptionClasses(Class[] fatalExceptionClasses) {
+		this.fatalExceptionClasses = fatalExceptionClasses;
+	}
+
+	/**
+	 * Uses the {@link #skipLimit} value to configure item handler and and
+	 * exception handler.
 	 */
 	protected void applyConfiguration(ItemOrientedStep step) {
 		super.applyConfiguration(step);
-		
+
 		ItemSkipPolicyItemHandler itemHandler = new ItemSkipPolicyItemHandler(getItemReader(), getItemWriter());
 
 		if (skipLimit > 0) {
@@ -55,10 +77,12 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 			 * to absorb exceptions at the step level because the failed items
 			 * will never re-appear after a rollback.
 			 */
-			itemHandler.setItemSkipPolicy(new LimitCheckingItemSkipPolicy(skipLimit, Arrays.asList(skippableExceptionClasses)));
+			itemHandler.setItemSkipPolicy(new LimitCheckingItemSkipPolicy(skipLimit, Arrays
+					.asList(skippableExceptionClasses), Arrays.asList(fatalExceptionClasses)));
 			SimpleLimitExceptionHandler exceptionHandler = new SimpleLimitExceptionHandler();
 			exceptionHandler.setLimit(skipLimit);
 			exceptionHandler.setExceptionClasses(skippableExceptionClasses);
+			exceptionHandler.setFatalExceptionClasses(fatalExceptionClasses);
 			setExceptionHandler(exceptionHandler);
 			getStepOperations().setExceptionHandler(getExceptionHandler());
 		}
@@ -66,8 +90,8 @@ public class SkipLimitStepFactoryBean extends SimpleStepFactoryBean {
 			// This is the default in ItemOrientedStep anyway...
 			itemHandler.setItemSkipPolicy(new NeverSkipItemSkipPolicy());
 		}
-		
+
 		step.setItemHandler(itemHandler);
 	}
-	
+
 }

spring-batch-core/src/main/java/org/springframework/batch/core/step/skip/LimitCheckingItemSkipPolicy.java

@@ -45,12 +45,16 @@ import org.springframework.batch.support.SubclassExceptionClassifier;
  * likely want to skip, but a {@link FileNotFoundException} should cause
  * immediate termination of the {@link Step}. Because it would be impossible
  * for a general purpose policy to determine all the types of exceptions that
- * should be skipped from those that shouldn't, a list must be passed in, with
- * all of the exceptions that are 'skippable'
+ * should be skipped from those that shouldn't, two lists must be passed in,
+ * with all of the exceptions that are 'fatal' and 'skippable'. The two lists
+ * are not enforced to be exclusive, they are prioritized instead - exceptions
+ * that are fatal will never be skipped, regardless whether the exception can
+ * also be classified as skippable.
  * </p>
  * 
  * @author Ben Hale
  * @author Lucas Ward
+ * @author Robert Kasanicky
  */
 public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy {
 
@@ -59,15 +63,33 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy {
 	 */
 	private static final String SKIP = "skip";
 
+	/**
+	 * Label for classifying fatal exceptions - these are never skipped.
+	 */
+	private static final String NEVER_SKIP = "neverSkip";
+
 	private final int skipLimit;
 
 	private ExceptionClassifier exceptionClassifier;
 
+	/**
+	 * Convenience constructor that assumes all exception types are skippable
+	 * and none are fatal.
+	 * @param skipLimit the number of exceptions allowed to skip
+	 */
 	public LimitCheckingItemSkipPolicy(int skipLimit) {
-		this(skipLimit, Collections.singletonList(Exception.class));
+		this(skipLimit, Collections.singletonList(Exception.class), Collections.EMPTY_LIST);
 	}
 
-	public LimitCheckingItemSkipPolicy(int skipLimit, List skippableExceptions) {
+	/**
+	 * 
+	 * @param skipLimit the number of skippable exceptions that are allowed to
+	 * be skipped
+	 * @param skippableExceptions exception classes that can be skipped
+	 * (non-critical)
+	 * @param fatalExceptions exception classes that should never be skipped
+	 */
+	public LimitCheckingItemSkipPolicy(int skipLimit, List skippableExceptions, List fatalExceptions) {
 		this.skipLimit = skipLimit;
 		SubclassExceptionClassifier exceptionClassifier = new SubclassExceptionClassifier();
 		Map typeMap = new HashMap();
@@ -75,6 +97,10 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy {
 			Class throwable = (Class) iterator.next();
 			typeMap.put(throwable, SKIP);
 		}
+		for (Iterator iterator = fatalExceptions.iterator(); iterator.hasNext();) {
+			Class throwable = (Class) iterator.next();
+			typeMap.put(throwable, NEVER_SKIP);
+		}
 		exceptionClassifier.setTypeMap(typeMap);
 		this.exceptionClassifier = exceptionClassifier;
 	}
@@ -82,12 +108,16 @@ public class LimitCheckingItemSkipPolicy implements ItemSkipPolicy {
 	/**
 	 * Given the provided exception and skip count, determine whether or not
 	 * processing should continue for the given exception. If the exception is
-	 * not within the list of 'skippable exceptions', false will be returned. If
-	 * the exception is within the list, and {@link StepExecution} skipCount is
-	 * greater than the skipLimit, then a {@link SkipLimitExceededException}
-	 * will be thrown.
+	 * not within the list of 'skippable exceptions' or belongs to the list of
+	 * 'fatal exceptions', false will be returned. If the exception is within
+	 * the skippable list (and not in the fatal list), and {@link StepExecution}
+	 * skipCount is greater than the skipLimit, then a
+	 * {@link SkipLimitExceededException} will be thrown.
 	 */
 	public boolean shouldSkip(Throwable t, int skipCount) {
+		if (exceptionClassifier.classify(t).equals(NEVER_SKIP)) {
+			return false;
+		}
 		if (exceptionClassifier.classify(t).equals(SKIP)) {
 			if (skipCount < skipLimit) {
 				return true;

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

@@ -17,6 +17,7 @@ package org.springframework.batch.core.step.item;
 
 import java.io.FileNotFoundException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 import junit.framework.TestCase;
@@ -38,8 +39,9 @@ public class SkipLimitReadFailurePolicyTests extends TestCase {
 		
 		List skippableExceptions = new ArrayList();
 		skippableExceptions.add(FlatFileParseException.class);
+		List fatalExceptions = Collections.EMPTY_LIST;
 		
-		failurePolicy = new LimitCheckingItemSkipPolicy(1, skippableExceptions);
+		failurePolicy = new LimitCheckingItemSkipPolicy(1, skippableExceptions, fatalExceptions);
 	}
 	
 	public void testLimitExceed(){		

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

@@ -34,8 +34,10 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 
 	SkipLimitStepFactoryBean tested = new SkipLimitStepFactoryBean();
 
-	// TODO checked exceptions are wrapped as RepeatExceptions by the chunkOperations#exceptionHandler
-	Class[] skippableExceptions = new Class[] { RepeatException.class, SkippableException.class, SkippableRuntimeException.class };
+	// TODO checked exceptions are wrapped as RepeatExceptions by the
+	// chunkOperations#exceptionHandler
+	Class[] skippableExceptions = new Class[] { RepeatException.class, SkippableException.class,
+			SkippableRuntimeException.class };
 
 	final int SKIP_LIMIT = 2;
 
@@ -61,8 +63,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 	}
 
 	/**
-	 * 
-	 * @throws Exception
+	 * Check items causing errors are skipped as expected.
 	 */
 	public void testSkip() throws Exception {
 		ItemOrientedStep step = (ItemOrientedStep) tested.getObject();
@@ -71,7 +72,7 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 		step.execute(stepExecution);
 
 		assertEquals(2, stepExecution.getSkipCount());
-		
+
 		assertTrue(reader.skipped.contains("2"));
 		assertTrue(reader.skipped.contains("4"));
 		// writer did not skip "2" as it never made it to writer, only "4" did
@@ -84,7 +85,31 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 					.contains(expectedOutput[i]));
 		}
 		assertTrue(writer.written.size() == expectedOutput.length);
-		
+
+	}
+
+	/**
+	 * Fatal exception should cause immediate termination regardless of other
+	 * skip settings (note the fatal exception is also classified as skippable).
+	 */
+	public void testFatalException() throws Exception {
+		tested.setFatalExceptionClasses(new Class[] { FatalRuntimeException.class });
+		tested.setItemWriter(new SkipWriterStub() {
+			public void write(Object item) {
+				throw new FatalRuntimeException("Ouch!");
+			}
+		});
+
+		ItemOrientedStep step = (ItemOrientedStep) tested.getObject();
+		StepExecution stepExecution = new StepExecution(step, jobExecution);
+
+		try {
+			step.execute(stepExecution);
+			fail();
+		}
+		catch (FatalRuntimeException expected) {
+			assertTrue(expected.getMessage().equals("Ouch!"));
+		}
 	}
 
 	/**
@@ -175,4 +200,10 @@ public class SkipLimitStepFactoryBeanTests extends TestCase {
 		}
 	}
 
+	private static class FatalRuntimeException extends SkippableRuntimeException {
+		public FatalRuntimeException(String message) {
+			super(message);
+		}
+	}
+
 }

spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/exception/SimpleLimitExceptionHandler.java

@@ -22,23 +22,35 @@ import org.springframework.batch.repeat.RepeatContext;
 import org.springframework.batch.support.ExceptionClassifierSupport;
 
 /**
- * Simple implementation of exception handler which looks for one exception
- * type. If it is found then a counter is incremented and the a limit is checked
- * to determine if it has been exceeded and the Throwable should be re-thrown.
+ * Simple implementation of exception handler which looks for given exception
+ * types. If one of the types is found then a counter is incremented and the
+ * limit is checked to determine if it has been exceeded and the Throwable
+ * should be re-thrown. Also allows to specify list of 'fatal' exceptions that
+ * are never subject to counting, but are immediately re-thrown. The fatal list
+ * has higher priority so the two lists needn't be exclusive.
  * 
  * @author Dave Syer
+ * @author Robert Kasanicky
  */
 public class SimpleLimitExceptionHandler implements ExceptionHandler {
 
 	/**
-	 * Name of exception classifier key for the nominated exception type.
+	 * Name of exception classifier key for the nominated exception types.
 	 */
 	private static final String TX_INVALID = "TX_INVALID";
 
+	/**
+	 * Name of exception classifier key for the fatal exception types (not
+	 * counted, immediately rethrown).
+	 */
+	private static final String FATAL = "FATAL";
+
 	private RethrowOnThresholdExceptionHandler delegate = new RethrowOnThresholdExceptionHandler();
 
 	private Class[] exceptionClasses = new Class[] { Exception.class };
 
+	private Class[] fatalExceptionClasses = new Class[] { Error.class };
+
 	/**
 	 * Flag to indicate the the exception counters should be shared between
 	 * sibling contexts in a nested batch (i.e. inner loop). Default is false.
@@ -68,6 +80,11 @@ public class SimpleLimitExceptionHandler implements ExceptionHandler {
 		super();
 		delegate.setExceptionClassifier(new ExceptionClassifierSupport() {
 			public Object classify(Throwable throwable) {
+				for (int i = 0; i < fatalExceptionClasses.length; i++) {
+					if (fatalExceptionClasses[i].isAssignableFrom(throwable.getClass())) {
+						return FATAL;
+					}
+				}
 				for (int i = 0; i < exceptionClasses.length; i++) {
 					if (exceptionClasses[i].isAssignableFrom(throwable.getClass())) {
 						return TX_INVALID;
@@ -103,20 +120,30 @@ public class SimpleLimitExceptionHandler implements ExceptionHandler {
 			{
 				put(ExceptionClassifierSupport.DEFAULT, new Integer(0));
 				put(TX_INVALID, new Integer(limit));
+				put(FATAL, new Integer(0));
 			}
 		});
 	}
 
 	/**
-	 * Setter for the Throwable exceptionClasses that this handler counts. Defaults to
-	 * {@link Exception}. If more exceptionClasses are specified handler uses single
-	 * counter that is incremented when one of the recognized exception
-	 * exceptionClasses is handled.
-	 * 
-	 * @param type
+	 * Setter for the Throwable exceptionClasses that this handler counts.
+	 * Defaults to {@link Exception}. If more exceptionClasses are specified
+	 * handler uses single counter that is incremented when one of the
+	 * recognized exception exceptionClasses is handled.
 	 */
 	public void setExceptionClasses(Class[] classes) {
 		this.exceptionClasses = classes;
 	}
 
+	/**
+	 * Setter for the Throwable exceptionClasses that shouldn't be counted, but
+	 * rethrown immediately. This list has higher priority than
+	 * {@link #exceptionClasses}.
+	 * 
+	 * @param fatalExceptionClasses defaults to {@link Error}
+	 */
+	public void setFatalExceptionClasses(Class[] fatalExceptionClasses) {
+		this.fatalExceptionClasses = fatalExceptionClasses;
+	}
+
 }