From 4a9e02cda6eae93d2265fc4527e203018d53014e Mon Sep 17 00:00:00 2001 From: dsyer Date: Tue, 13 May 2008 22:51:44 +0000 Subject: [PATCH] IN PROGRESS - issue BATCH-569: Add RetryOperationsInterceptor with stateful retry Fix issue with final callback value - broken integration build --- .../RepeatOperationsInterceptor.java | 88 +++++++++++---- .../RepeatOperationsInterceptorTests.java | 106 +++++++++++++++--- 2 files changed, 159 insertions(+), 35 deletions(-) diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptor.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptor.java index a7fc1394c..5dc0e5ce5 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptor.java +++ b/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; + } + } + } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptorTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptorTests.java index 8566fff15..7b10e551d 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/interceptor/RepeatOperationsInterceptorTests.java +++ b/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; } }