From bc9e97fe66b0cc44720e69e9fcd8755514fdc729 Mon Sep 17 00:00:00 2001 From: dsyer Date: Tue, 2 Jun 2009 11:11:13 +0000 Subject: [PATCH] BATCH-1264: null pointer if transaction attributes partially specified --- .../xml/StepParserStepFactoryBean.java | 151 ++++++++++++------ .../xml/StepParserStepFactoryBeanTests.java | 14 ++ .../src/main/resources/jobs/taskletJob.xml | 4 +- 3 files changed, 117 insertions(+), 52 deletions(-) diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBean.java b/spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBean.java index 534e4e7ff..bd80d9cfa 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBean.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBean.java @@ -142,30 +142,33 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { */ public final Object getObject() throws Exception { if (hasChunkElement) { - Assert.isNull(tasklet, "Step [" + name - + "] has both a element and a 'ref' attribute referencing a Tasklet."); + Assert + .isNull( + tasklet, + "Step [" + + name + + "] has both a element and a 'ref' attribute referencing a Tasklet."); if (isFaultTolerant()) { FaultTolerantStepFactoryBean fb = new FaultTolerantStepFactoryBean(); configureSimple(fb); configureFaultTolerant(fb); return fb.getObject(); - } - else { + } else { validateSimpleStep(); SimpleStepFactoryBean fb = new SimpleStepFactoryBean(); configureSimple(fb); return fb.getObject(); } - } - else if (tasklet != null) { + } else if (tasklet != null) { TaskletStep ts = new TaskletStep(); configureTaskletStep(ts); return ts; - } - else { - throw new IllegalStateException("Step [" + name - + "] has neither a element nor a 'ref' attribute referencing a Tasklet."); + } else { + throw new IllegalStateException( + "Step [" + + name + + "] has neither a element nor a 'ref' attribute referencing a Tasklet."); } } @@ -256,6 +259,7 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { } } + @SuppressWarnings("serial") private void configureTaskletStep(TaskletStep ts) { if (name != null) { ts.setName(name); @@ -281,14 +285,22 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { for (StepListener listener : listeners) { newListeners[i++] = (StepExecutionListener) listener; } - ts.setStepExecutionListeners((StepExecutionListener[]) newListeners); + ts + .setStepExecutionListeners((StepExecutionListener[]) newListeners); } if (transactionTimeout >= 0 || propagation != null || isolation != null) { DefaultTransactionAttribute attribute = new DefaultTransactionAttribute(); - attribute.setPropagationBehavior(propagation.value()); - attribute.setIsolationLevel(isolation.value()); - attribute.setTimeout(transactionTimeout); - ts.setTransactionAttribute(new DefaultTransactionAttribute(attribute) { + if (propagation != null) { + attribute.setPropagationBehavior(propagation.value()); + } + if (isolation != null) { + attribute.setIsolationLevel(isolation.value()); + } + if (transactionTimeout >= 0) { + attribute.setTimeout(transactionTimeout); + } + ts.setTransactionAttribute(new DefaultTransactionAttribute( + attribute) { /** * Ignore the default behaviour and rollback on all exceptions @@ -307,9 +319,12 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { private void validateSimpleStep() { PropertyNamePair[] notPermitted = new PropertyNamePair[] { new PropertyNamePair(retryListeners, "retry-listeners"), - new PropertyNamePair(skippableExceptionClasses, "skippable-exception-classes"), - new PropertyNamePair(retryableExceptionClasses, "retryable-exception-classes"), - new PropertyNamePair(fatalExceptionClasses, "fatal-exception-classes") }; + new PropertyNamePair(skippableExceptionClasses, + "skippable-exception-classes"), + new PropertyNamePair(retryableExceptionClasses, + "retryable-exception-classes"), + new PropertyNamePair(fatalExceptionClasses, + "fatal-exception-classes") }; List wrong = new ArrayList(); for (PropertyNamePair field : notPermitted) { @@ -318,10 +333,17 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { } } if (!wrong.isEmpty()) { - throw new IllegalArgumentException("The field" + (wrong.size() > 1 ? "s " : " ") + wrong - + (wrong.size() == 1 ? " is" : " are") + " not permitted on the simple step [" + name + "]. " - + (wrong.size() == 1 ? "It" : "They") + " can only be specified for fault-tolerant " - + "configurations providing skip-limit, retry-limit, or cache-capacity"); + throw new IllegalArgumentException( + "The field" + + (wrong.size() > 1 ? "s " : " ") + + wrong + + (wrong.size() == 1 ? " is" : " are") + + " not permitted on the simple step [" + + name + + "]. " + + (wrong.size() == 1 ? "It" : "They") + + " can only be specified for fault-tolerant " + + "configurations providing skip-limit, retry-limit, or cache-capacity"); } } @@ -346,7 +368,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { } private boolean isFaultTolerant() { - return isPositive(skipLimit) || isPositive(retryLimit) || isPositive(cacheCapacity) + return isPositive(skipLimit) || isPositive(retryLimit) + || isPositive(cacheCapacity) || isTrue(isReaderTransactionalQueue); } @@ -390,7 +413,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * Public setter for the flag to indicate that the step should be replayed * on a restart, even if successful the first time. * - * @param allowStartIfComplete the shouldAllowStartIfComplete to set + * @param allowStartIfComplete + * the shouldAllowStartIfComplete to set */ public void setAllowStartIfComplete(boolean allowStartIfComplete) { this.allowStartIfComplete = allowStartIfComplete; @@ -439,9 +463,11 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { } /** - * @param transactionManager the transaction manager to set + * @param transactionManager + * the transaction manager to set */ - public void setTransactionManager(PlatformTransactionManager transactionManager) { + public void setTransactionManager( + PlatformTransactionManager transactionManager) { this.transactionManager = transactionManager; } @@ -454,7 +480,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * {@link StepListener} can be used, and will then receive callbacks at the * appropriate stage in the step. * - * @param listeners an array of listeners + * @param listeners + * an array of listeners */ public void setListeners(StepListener[] listeners) { this.listeners = listeners; @@ -464,28 +491,33 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * Exception classes that may not cause a rollback if encountered in the * right place. * - * @param noRollbackExceptionClasses the noRollbackExceptionClasses to set + * @param noRollbackExceptionClasses + * the noRollbackExceptionClasses to set */ - public void setNoRollbackExceptionClasses(Collection> noRollbackExceptionClasses) { + public void setNoRollbackExceptionClasses( + Collection> noRollbackExceptionClasses) { this.noRollbackExceptionClasses = noRollbackExceptionClasses; } /** - * @param transactionTimeout the transactionTimeout to set + * @param transactionTimeout + * the transactionTimeout to set */ public void setTransactionTimeout(int transactionTimeout) { this.transactionTimeout = transactionTimeout; } /** - * @param isolation the isolation to set + * @param isolation + * the isolation to set */ public void setIsolation(Isolation isolation) { this.isolation = isolation; } /** - * @param propagation the propagation to set + * @param propagation + * the propagation to set */ public void setPropagation(Propagation propagation) { this.propagation = propagation; @@ -506,8 +538,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * this many failures in a single transaction. Defaults to the value in the * {@link MapRetryContextCache}.
* - * @param cacheCapacity the cache capacity to set (greater than 0 else - * ignored) + * @param cacheCapacity + * the cache capacity to set (greater than 0 else ignored) */ public void setCacheCapacity(int cacheCapacity) { this.cacheCapacity = cacheCapacity; @@ -519,7 +551,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * complete. Defaults to a {@link SimpleCompletionPolicy} with chunk size * equal to the commitInterval property. * - * @param chunkCompletionPolicy the chunkCompletionPolicy to set + * @param chunkCompletionPolicy + * the chunkCompletionPolicy to set */ public void setChunkCompletionPolicy(CompletionPolicy chunkCompletionPolicy) { this.chunkCompletionPolicy = chunkCompletionPolicy; @@ -529,7 +562,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * Set the commit interval. Either set this or the chunkCompletionPolicy but * not both. * - * @param commitInterval 1 by default + * @param commitInterval + * 1 by default */ public void setCommitInterval(int commitInterval) { this.commitInterval = commitInterval; @@ -540,7 +574,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * so that items are re-presented after a rollback. The default is false and * readers are assumed to be forward-only. * - * @param isReaderTransactionalQueue the value of the flag + * @param isReaderTransactionalQueue + * the value of the flag */ public void setIsReaderTransactionalQueue(boolean isReaderTransactionalQueue) { this.isReaderTransactionalQueue = isReaderTransactionalQueue; @@ -551,7 +586,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * limit. Note this limit includes the initial attempt to process the item, * therefore retryLimit == 1 by default. * - * @param retryLimit the retry limit to set, must be greater or equal to 1. + * @param retryLimit + * the retry limit to set, must be greater or equal to 1. */ public void setRetryLimit(int retryLimit) { this.retryLimit = retryLimit; @@ -564,7 +600,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * zero then all exceptions will be propagated from the chunk and cause the * step to abort. * - * @param skipLimit the value to set. Default is 0 (never skip). + * @param skipLimit + * the value to set. Default is 0 (never skip). */ public void setSkipLimit(int skipLimit) { this.skipLimit = skipLimit; @@ -574,7 +611,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * Public setter for the {@link TaskExecutor}. If this is set, then it will * be used to execute the chunk processing inside the {@link Step}. * - * @param taskExecutor the taskExecutor to set + * @param taskExecutor + * the taskExecutor to set */ public void setTaskExecutor(TaskExecutor taskExecutor) { this.taskExecutor = taskExecutor; @@ -586,28 +624,33 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * overwhelmed. Defaults to * {@link TaskExecutorRepeatTemplate#DEFAULT_THROTTLE_LIMIT}. * - * @param throttleLimit the throttle limit to set. + * @param throttleLimit + * the throttle limit to set. */ public void setThrottleLimit(Integer throttleLimit) { this.throttleLimit = throttleLimit; } /** - * @param itemReader the {@link ItemReader} to set + * @param itemReader + * the {@link ItemReader} to set */ public void setItemReader(ItemReader itemReader) { this.itemReader = itemReader; } /** - * @param itemProcessor the {@link ItemProcessor} to set + * @param itemProcessor + * the {@link ItemProcessor} to set */ - public void setItemProcessor(ItemProcessor itemProcessor) { + public void setItemProcessor( + ItemProcessor itemProcessor) { this.itemProcessor = itemProcessor; } /** - * @param itemWriter the {@link ItemWriter} to set + * @param itemWriter + * the {@link ItemWriter} to set */ public void setItemWriter(ItemWriter itemWriter) { this.itemWriter = itemWriter; @@ -620,7 +663,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { /** * Public setter for the {@link RetryListener}s. * - * @param retryListeners the {@link RetryListener}s to set + * @param retryListeners + * the {@link RetryListener}s to set */ public void setRetryListeners(RetryListener... retryListeners) { this.retryListeners = retryListeners; @@ -633,16 +677,19 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * * @param exceptionClasses */ - public void setSkippableExceptionClasses(Collection> exceptionClasses) { + public void setSkippableExceptionClasses( + Collection> exceptionClasses) { this.skippableExceptionClasses = exceptionClasses; } /** * Public setter for exception classes that will retry the item when raised. * - * @param retryableExceptionClasses the retryableExceptionClasses to set + * @param retryableExceptionClasses + * the retryableExceptionClasses to set */ - public void setRetryableExceptionClasses(Collection> retryableExceptionClasses) { + public void setRetryableExceptionClasses( + Collection> retryableExceptionClasses) { this.retryableExceptionClasses = retryableExceptionClasses; } @@ -651,7 +698,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * * @param fatalExceptionClasses */ - public void setFatalExceptionClasses(Collection> fatalExceptionClasses) { + public void setFatalExceptionClasses( + Collection> fatalExceptionClasses) { this.fatalExceptionClasses = fatalExceptionClasses; } @@ -660,7 +708,8 @@ class StepParserStepFactoryBean implements FactoryBean, BeanNameAware { * {@link ItemStream} can be used, and will then receive callbacks at the * appropriate stage in the step. * - * @param streams an array of listeners + * @param streams + * an array of listeners */ public void setStreams(ItemStream[] streams) { this.streams = streams; diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBeanTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBeanTests.java index 5ab9aec0c..341d0d99d 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBeanTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/StepParserStepFactoryBeanTests.java @@ -85,6 +85,20 @@ public class StepParserStepFactoryBeanTests { assertTrue(tasklet instanceof DummyTasklet); } + @Test + public void testTaskletStepMissingIsolation() throws Exception { + StepParserStepFactoryBean fb = new StepParserStepFactoryBean(); + fb.setBeanName("step1"); + fb.setJobRepository(new JobRepositorySupport()); + fb.setTasklet(new DummyTasklet()); + fb.setTransactionManager(new ResourcelessTransactionManager()); + fb.setPropagation(Propagation.REQUIRED); + Object step = fb.getObject(); + assertTrue(step instanceof TaskletStep); + Object tasklet = ReflectionTestUtils.getField(step, "tasklet"); + assertTrue(tasklet instanceof DummyTasklet); + } + @Test(expected = IllegalStateException.class) public void testSimpleStepAll() throws Exception { StepParserStepFactoryBean fb = new StepParserStepFactoryBean(); diff --git a/spring-batch-samples/src/main/resources/jobs/taskletJob.xml b/spring-batch-samples/src/main/resources/jobs/taskletJob.xml index 8a3679790..ee02a6ae2 100644 --- a/spring-batch-samples/src/main/resources/jobs/taskletJob.xml +++ b/spring-batch-samples/src/main/resources/jobs/taskletJob.xml @@ -17,7 +17,9 @@ - + + +