IN PROGRESS - issue BATCH-569: Add RetryOperationsInterceptor with stateful retry

Fix issue with final callback value - broken integration build

spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptor.java

@@ -16,9 +16,6 @@
 
 package org.springframework.batch.repeat.interceptor;
 
-import java.util.ArrayList;
-import java.util.List;
-
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
 import org.springframework.aop.ProxyMethodInvocation;
@@ -33,8 +30,9 @@ import org.springframework.util.Assert;
 /**
  * A {@link MethodInterceptor} that can be used to automatically repeat calls to
  * a method on a service. The injected {@link RepeatOperations} is used to
- * control the completion of the loop. By default it will repeat until the
- * target method returns null. Be careful when injecting a bespoke
+ * control the completion of the loop. Independent of the completion policy in
+ * the {@link RepeatOperations} the loop will repeat until the target method
+ * returns null or false. Be careful when injecting a bespoke
  * {@link RepeatOperations} that the loop will actually terminate, because the
  * default policy for a vanilla {@link RepeatTemplate} will never complete if
  * the return type of the target method is void (the value returned is always
@@ -65,7 +63,14 @@ public class RepeatOperationsInterceptor implements MethodInterceptor {
 	 */
 	public Object invoke(final MethodInvocation invocation) throws Throwable {
 
-		final List results = new ArrayList();
+		final ResultHolder result = new ResultHolder();
+		// Cache void return value if intercepted method returns void
+		final boolean voidReturnType = Void.TYPE.equals(invocation.getMethod().getReturnType());
+		if (voidReturnType) {
+			// This will be ignored anyway, but we want it to be non-null for
+			// convenience of checking that there is a result.
+			result.setValue(new Object());
+		}
 
 		try {
 			repeatOperations.iterate(new RepeatCallback() {
@@ -82,19 +87,17 @@ public class RepeatOperationsInterceptor implements MethodInterceptor {
 									"MethodInvocation of the wrong type detected - this should not happen with Spring AOP, so please raise an issue if you see this exception");
 						}
 
-						Object result = clone.proceed();
-						if (clone.getMethod().getReturnType().equals(Void.TYPE)) {
-							results.clear();
-							results.add(result);
+						Object value = clone.proceed();
+						if (voidReturnType) {
 							return ExitStatus.CONTINUABLE;
 						}
-						if (!isComplete(result)) {
-							// We only save the last non-null result
-							results.clear();
-							results.add(result);
+						if (!isComplete(value)) {
+							// Save the last result
+							result.setValue(value);
 							return ExitStatus.CONTINUABLE;
 						}
 						else {
+							result.setFinalValue(value);
 							return ExitStatus.FINISHED;
 						}
 					}
@@ -110,16 +113,13 @@ public class RepeatOperationsInterceptor implements MethodInterceptor {
 
 			});
 		}
-		catch (RepeatOperationsInterceptorException e) {
-			// Unwrap and re-throw any nasty errors
-			throw e.getCause();
-		}
 		catch (Throwable t) {
+			// The repeat exception should be unwrapped by the template
 			throw t;
 		}
 
-		if (!results.isEmpty()) {
-			return results.get(0);
+		if (result.isReady()) {
+			return result.getValue();
 		}
 
 		// No result means something weird happened
@@ -152,4 +152,52 @@ public class RepeatOperationsInterceptor implements MethodInterceptor {
 		}
 	}
 
+	/**
+	 * Simple wrapper object for the result from a method invocation.
+	 * 
+	 * @author Dave Syer
+	 * 
+	 */
+	private static class ResultHolder {
+		private Object value = null;
+
+		private boolean ready = false;
+
+		/**
+		 * Public setter for the Object.
+		 * @param value the value to set
+		 */
+		public void setValue(Object value) {
+			this.ready = true;
+			this.value = value;
+		}
+
+		/**
+		 * @param value
+		 */
+		public void setFinalValue(Object value) {
+			if (ready) {
+				// Only set the value the last time if the last time was also
+				// the first time
+				return;
+			}
+			setValue(value);
+		}
+
+		/**
+		 * Public getter for the Object.
+		 * @return the value
+		 */
+		public Object getValue() {
+			return value;
+		}
+
+		/**
+		 * @return true if a value has been set
+		 */
+		public boolean isReady() {
+			return ready;
+		}
+	}
+
 }

spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptorTests.java

@@ -46,8 +46,7 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		super.setUp();
 		interceptor = new RepeatOperationsInterceptor();
 		target = new ServiceImpl();
-		ProxyFactory factory = new ProxyFactory(RepeatOperations.class
-				.getClassLoader());
+		ProxyFactory factory = new ProxyFactory(RepeatOperations.class.getClassLoader());
 		factory.setInterfaces(new Class[] { Service.class });
 		factory.setTarget(target);
 		service = (Service) factory.getProxy();
@@ -59,14 +58,22 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		assertEquals(3, target.count);
 	}
 
+	public void testCompleteOnFirstInvocation() throws Exception {
+		((Advised) service).addAdvice(interceptor);
+		target.setMaxService(0);
+		service.service();
+		assertEquals(1, target.count);
+	}
+
 	public void testSetTemplate() throws Exception {
 		final List calls = new ArrayList();
 		interceptor.setRepeatOperations(new RepeatOperations() {
 			public ExitStatus iterate(RepeatCallback callback) {
 				try {
 					Object result = callback.doInIteration(null);
-				calls.add(result);
-				} catch (Exception e) {
+					calls.add(result);
+				}
+				catch (Exception e) {
 					throw new RepeatException("Encountered exception in repeat.", e);
 				}
 				return ExitStatus.CONTINUABLE;
@@ -77,6 +84,25 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		assertEquals(1, calls.size());
 	}
 
+	public void testCallbackNotExecuted() throws Exception {
+		final List calls = new ArrayList();
+		interceptor.setRepeatOperations(new RepeatOperations() {
+			public ExitStatus iterate(RepeatCallback callback) {
+				calls.add(null);
+				return ExitStatus.FINISHED;
+			}
+		});
+		((Advised) service).addAdvice(interceptor);
+		try {
+			service.service();
+			fail("Expected IllegalStateException");
+		} catch (IllegalStateException e) {
+			String message = e.getMessage();
+			assertTrue("Wrong exception message: "+message, message.toLowerCase().indexOf("no result available")>=0);
+		}
+		assertEquals(1, calls.size());
+	}
+
 	public void testVoidServiceSunnyDay() throws Exception {
 		((Advised) service).addAdvice(interceptor);
 		RepeatTemplate template = new RepeatTemplate();
@@ -93,7 +119,8 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		try {
 			service.exception();
 			fail("Expected RuntimeException");
-		} catch (RuntimeException e) {
+		}
+		catch (RuntimeException e) {
 			assertEquals("Duh", e.getMessage().substring(0, 3));
 		}
 	}
@@ -102,12 +129,32 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		((Advised) service).addAdvice(interceptor);
 		try {
 			service.error();
-			fail("Expected BatchException");
-		} catch (Error e) {
+			fail("Expected Error");
+		}
+		catch (Error e) {
 			assertEquals("Duh", e.getMessage().substring(0, 3));
 		}
 	}
 
+	public void testCallbackWithBoolean() throws Exception {
+		RepeatTemplate template = new RepeatTemplate();
+		// N.B. the default completion policy results in an infinite loop, so we
+		// need to set the chunk size.
+		template.setCompletionPolicy(new SimpleCompletionPolicy(2));
+		interceptor.setRepeatOperations(template);
+		((Advised) service).addAdvice(interceptor);
+		assertTrue(service.isContinuable());
+		assertEquals(2, target.count);
+	}
+
+	public void testCallbackWithBooleanReturningFalseFirstTime() throws Exception {
+		target.setComplete(true);
+		((Advised) service).addAdvice(interceptor);
+		// Complete without repeat when boolean return value is false
+		assertFalse(service.isContinuable());
+		assertEquals(1, target.count);
+	}
+
 	public void testInterceptorChainWithRetry() throws Exception {
 		((Advised) service).addAdvice(interceptor);
 		final List list = new ArrayList();
@@ -129,7 +176,12 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		try {
 			interceptor.invoke(new MethodInvocation() {
 				public Method getMethod() {
-					return null;
+					try {
+						return Object.class.getMethod("toString", new Class[0]);
+					}
+					catch (Exception e) {
+						throw new RuntimeException(e);
+					}
 				}
 
 				public Object[] getArguments() {
@@ -149,10 +201,10 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 				}
 			});
 			fail("IllegalStateException expected");
-		} catch (IllegalStateException e) {
-			assertTrue("Exception message should contain MethodInvocation: "
-					+ e.getMessage(), e.getMessage()
-					.indexOf("MethodInvocation") >= 0);
+		}
+		catch (IllegalStateException e) {
+			assertTrue("Exception message should contain MethodInvocation: " + e.getMessage(), e.getMessage().indexOf(
+					"MethodInvocation") >= 0);
 		}
 	}
 
@@ -164,20 +216,39 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		Object exception() throws Exception;
 
 		Object error() throws Exception;
+
+		boolean isContinuable() throws Exception;
 	}
 
 	private static class ServiceImpl implements Service {
 		private int count = 0;
 
+		private boolean complete;
+
+		private int maxService = 2;
+		
+		/**
+		 * Public setter for the maximum number of times to call service().
+		 * @param maxService the maxService to set
+		 */
+		public void setMaxService(int maxService) {
+			this.maxService = maxService;
+		}
+
 		public Object service() throws Exception {
 			count++;
-			if (count <= 2) {
+			if (count <= maxService) {
 				return new Integer(count);
-			} else {
+			}
+			else {
 				return null;
 			}
 		}
 
+		public void setComplete(boolean complete) {
+			this.complete = complete;
+		}
+
 		public void alternate() throws Exception {
 			count++;
 		}
@@ -187,7 +258,12 @@ public class RepeatOperationsInterceptorTests extends TestCase {
 		}
 
 		public Object error() throws Exception {
-			throw new Error("Duh! Stupid.");
+			throw new Error("Duh! Stupid error.");
+		}
+
+		public boolean isContinuable() throws Exception {
+			count++;
+			return !complete;
 		}
 	}