diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/JobRepositorySupport.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/JobRepositorySupport.java index 585b6367a..2a60a36ea 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/JobRepositorySupport.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/JobRepositorySupport.java @@ -61,12 +61,10 @@ public class JobRepositorySupport implements JobRepository { } public StepExecution getLastStepExecution(JobInstance jobInstance, Step step) { - // TODO Auto-generated method stub return null; } public int getStepExecutionCount(JobInstance jobInstance, Step step) { - // TODO Auto-generated method stub return 0; } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemOrientedStepTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemOrientedStepTests.java index 3f1d12f43..e48e7dc67 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemOrientedStepTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/ItemOrientedStepTests.java @@ -360,7 +360,6 @@ public class ItemOrientedStepTests extends TestCase { } public void update(ExecutionContext executionContext) { - // TODO Auto-generated method stub executionContext.putString("foo", "bar"); } }; diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/MockItemReader.java b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/MockItemReader.java index 549b64604..b796ed440 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/MockItemReader.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/step/item/MockItemReader.java @@ -55,7 +55,6 @@ public class MockItemReader implements ItemReader { } public Object getKey(Object item) { - // TODO Auto-generated method stub return null; } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStream.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStream.java index 87fc4c4e0..49c48e55a 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStream.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ItemStream.java @@ -48,7 +48,7 @@ public interface ItemStream { * If any resources are needed for the stream to operate they need to be destroyed here. Once this method has been * called all other methods (except open) may throw an exception. * - * @param executionContext TODO + * @param executionContext the current execution context in case it is needed */ void close(ExecutionContext executionContext) throws ItemStreamException; } 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 a96ce31e3..a7fc1394c 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 @@ -29,7 +29,6 @@ import org.springframework.batch.repeat.RepeatException; import org.springframework.batch.repeat.RepeatOperations; import org.springframework.batch.repeat.support.RepeatTemplate; import org.springframework.util.Assert; -import org.springframework.util.ClassUtils; /** * A {@link MethodInterceptor} that can be used to automatically repeat calls to @@ -83,12 +82,12 @@ 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"); } - // N.B. discards return value if there is one + Object result = clone.proceed(); if (clone.getMethod().getReturnType().equals(Void.TYPE)) { - clone.proceed(); + results.clear(); + results.add(result); return ExitStatus.CONTINUABLE; } - Object result = clone.proceed(); if (!isComplete(result)) { // We only save the last non-null result results.clear(); @@ -123,12 +122,9 @@ public class RepeatOperationsInterceptor implements MethodInterceptor { return results.get(0); } - Class returnType = invocation.getMethod().getReturnType(); - Object defaultValue = null; - if (ClassUtils.isPrimitiveOrWrapper(returnType)) { - defaultValue = getDefaultForPrimitiveType(returnType); - } - return defaultValue; + // No result means something weird happened + throw new IllegalStateException("No result available for attempted repeat call to " + invocation + + ". The invocation was never called, so maybe there is a problem with the completion policy?"); } /** @@ -156,42 +152,4 @@ public class RepeatOperationsInterceptor implements MethodInterceptor { } } - /** - * Set up a default return value for primitive types (all basically "0"). - * @param returnType the desired primitive type - * @return a value to use as the default return value if recovery path is - * taken - */ - // TODO: cache these values. - private Object getDefaultForPrimitiveType(Class returnType) { - if (returnType.equals(Boolean.TYPE)) { - return Boolean.FALSE; - } - else if (returnType.equals(Byte.TYPE)) { - return Byte.valueOf("0"); - } - else if (returnType.equals(Character.TYPE)) { - return Character.valueOf('0'); - } - else if (returnType.equals(Short.TYPE)) { - return Short.valueOf("0"); - } - else if (returnType.equals(Integer.TYPE)) { - return Integer.valueOf('0'); - } - else if (returnType.equals(Long.TYPE)) { - return Long.valueOf('0'); - } - else if (returnType.equals(Float.TYPE)) { - return Float.valueOf('0'); - } - else if (returnType.equals(Double.TYPE)) { - return Double.valueOf('0'); - } - else if (returnType.equals(Void.TYPE)) { - return null; - } - throw new IllegalStateException("Primitive type with no default: " + returnType); - } - } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java index fac591fbc..1d8f3ba85 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/callback/RecoveryRetryCallback.java @@ -21,7 +21,6 @@ import org.springframework.batch.item.ItemWriter; import org.springframework.batch.retry.RecoveryCallback; import org.springframework.batch.retry.RetryCallback; import org.springframework.batch.retry.RetryContext; -import org.springframework.batch.retry.RetryException; import org.springframework.batch.retry.RetryPolicy; import org.springframework.batch.retry.policy.RecoveryCallbackRetryPolicy; @@ -112,11 +111,10 @@ public class RecoveryRetryCallback implements RetryCallback { } public Object doWithRetry(RetryContext context) throws Throwable { - if (!context.isExhaustedOnly()) { - return callback.doWithRetry(context); - } - // TODO: is this necessary? - throw new RetryException("Recovery path requested in retry callback."); + return callback.doWithRetry(context); + // N.B. code used to check here for isExhaustedOnly and throw exception. + // This is unnecessary because the callback could just throw the + // exception itself if it wants to go to the recovery path. } public Object getItem() { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java index beb035346..245c306cc 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/interceptor/StatefulRetryOperationsInterceptor.java @@ -172,6 +172,10 @@ public class StatefulRetryOperationsInterceptor implements MethodInterceptor { if (recoverer != null) { return recoverer.recover(item, context.getLastThrowable()); } + // TODO: This sucks big time because the method invocation almost + // certainly does not return an object of this type. It would be + // better to return null (but then method invocations that return + // primitive values would barf). return item; } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java index 9b51765c5..08d5570c4 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/retry/policy/CompositeRetryPolicy.java @@ -65,17 +65,27 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy { /** * Delegate to the policies that were in operation when the context was - * created. + * created. If any of them fails to close the exception is propagated (and + * those later in the chain are closed before re-throwing). * * @see org.springframework.batch.retry.RetryPolicy#close(org.springframework.batch.retry.RetryContext) */ public void close(RetryContext context) { RetryContext[] contexts = ((CompositeRetryContext) context).contexts; RetryPolicy[] policies = ((CompositeRetryContext) context).policies; - // TODO: throw some sort of composite exception if any of the close - // methods fail? + RuntimeException exception = null; for (int i = 0; i < contexts.length; i++) { - policies[i].close(contexts[i]); + try { + policies[i].close(contexts[i]); + } + catch (RuntimeException e) { + if (exception==null) { + exception = e; + } + } + } + if (exception!=null) { + throw exception; } } @@ -83,7 +93,8 @@ public class CompositeRetryPolicy extends AbstractStatelessRetryPolicy { * Creates a new context that copies the existing policies and keeps a list * of the contexts from each one. * - * @see org.springframework.batch.retry.RetryPolicy#open(org.springframework.batch.retry.RetryCallback, RetryContext) + * @see org.springframework.batch.retry.RetryPolicy#open(org.springframework.batch.retry.RetryCallback, + * RetryContext) */ public RetryContext open(RetryCallback callback, RetryContext parent) { List list = new ArrayList(); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/file/FlatFileItemReaderAdvancedTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/file/FlatFileItemReaderAdvancedTests.java index 0cd427654..014075668 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/file/FlatFileItemReaderAdvancedTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/file/FlatFileItemReaderAdvancedTests.java @@ -106,8 +106,6 @@ public class FlatFileItemReaderAdvancedTests extends TestCase { // we should now process all records after first commit point assertEquals("[testLine3]", reader.read().toString()); - // TODO update and assert ExecutionContext - } /** 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 fe97dee24..8566fff15 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 @@ -29,6 +29,7 @@ import org.springframework.aop.framework.Advised; import org.springframework.aop.framework.ProxyFactory; import org.springframework.batch.repeat.ExitStatus; import org.springframework.batch.repeat.RepeatCallback; +import org.springframework.batch.repeat.RepeatException; import org.springframework.batch.repeat.RepeatOperations; import org.springframework.batch.repeat.policy.SimpleCompletionPolicy; import org.springframework.batch.repeat.support.RepeatTemplate; @@ -62,8 +63,12 @@ public class RepeatOperationsInterceptorTests extends TestCase { final List calls = new ArrayList(); interceptor.setRepeatOperations(new RepeatOperations() { public ExitStatus iterate(RepeatCallback callback) { - Object result = "1"; + try { + Object result = callback.doInIteration(null); calls.add(result); + } catch (Exception e) { + throw new RepeatException("Encountered exception in repeat.", e); + } return ExitStatus.CONTINUABLE; } }); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/AsynchronousRepeatTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/AsynchronousRepeatTests.java index fa5ac5818..7d549f915 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/AsynchronousRepeatTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/AsynchronousRepeatTests.java @@ -67,6 +67,7 @@ public class AsynchronousRepeatTests extends AbstractTradeBatchTests { TaskExecutorRepeatTemplate jobTemplate = new TaskExecutorRepeatTemplate(); final RepeatTemplate stepTemplate = new RepeatTemplate(); SimpleAsyncTaskExecutor taskExecutor = new SimpleAsyncTaskExecutor(); + taskExecutor.setConcurrencyLimit(2); jobTemplate.setTaskExecutor(taskExecutor); final String threadName = Thread.currentThread().getName(); @@ -92,9 +93,9 @@ public class AsynchronousRepeatTests extends AbstractTradeBatchTests { // Thread.sleep(500); assertEquals(NUMBER_OF_ITEMS, processor.count); // Because of the throttling and queueing internally to a TaskExecutor, - // more than one thread will be used - the number used is (as of writing) - // one less than the throttle limit of the template. - // TODO: see if we can get it to use only one thread? + // more than one thread will be used - the number used is the + // concurrency limit in the task executor, plus 1. + // System.err.println(threadNames); assertTrue(threadNames.size() >= 1); } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/SimpleRepeatTemplateTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/SimpleRepeatTemplateTests.java index b9ec5e561..2e959e39c 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/SimpleRepeatTemplateTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/repeat/support/SimpleRepeatTemplateTests.java @@ -262,9 +262,6 @@ public class SimpleRepeatTemplateTests extends AbstractTradeBatchTests { }) { public ExitStatus doInIteration(RepeatContext context) throws Exception { count++; - // TODO parameter is rewritten and then compared to value it has - // just been assigned - context = RepeatSynchronizationManager.getContext(); assertSame(context, RepeatSynchronizationManager.getContext()); return super.doInIteration(context); } @@ -307,9 +304,6 @@ public class SimpleRepeatTemplateTests extends AbstractTradeBatchTests { }) { public ExitStatus doInIteration(RepeatContext context) throws Exception { count++; - // TODO parameter is rewritten and then compared to value it has - // just been assigned - context = RepeatSynchronizationManager.getContext(); assertSame(context, RepeatSynchronizationManager.getContext()); super.doInIteration(context); return ExitStatus.CONTINUABLE; diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java index e12f434bf..22dfea6e6 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/retry/policy/CompositeRetryPolicyTests.java @@ -80,9 +80,6 @@ public class CompositeRetryPolicyTests extends TestCase { policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() { public void close(RetryContext context) { list.add("1"); - // TODO: test that all close methods are called if this - // happens... - // throw new RuntimeException("Pah!"); } }, new MockRetryPolicySupport() { public void close(RetryContext context) { @@ -95,6 +92,30 @@ public class CompositeRetryPolicyTests extends TestCase { assertEquals(2, list.size()); } + public void testExceptionOnPoliciesClose() throws Exception { + final List list = new ArrayList(); + CompositeRetryPolicy policy = new CompositeRetryPolicy(); + policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport() { + public void close(RetryContext context) { + list.add("1"); + throw new RuntimeException("Pah!"); + } + }, new MockRetryPolicySupport() { + public void close(RetryContext context) { + list.add("2"); + } + } }); + RetryContext context = policy.open(null, null); + assertNotNull(context); + try { + policy.close(context); + fail("Expected RuntimeException"); + } catch (RuntimeException e) { + assertEquals("Pah!", e.getMessage()); + } + assertEquals(2, list.size()); + } + public void testRetryCount() throws Exception { CompositeRetryPolicy policy = new CompositeRetryPolicy(); policy.setPolicies(new RetryPolicy[] { new MockRetryPolicySupport(), new MockRetryPolicySupport() });