From 63496ee571519ea5c6c42c1baf52eba425575c0f Mon Sep 17 00:00:00 2001
From: robokaso <robokaso@2e8e966d-6415-0410-94b9-dc4e65435d03>
Date: Wed, 27 Feb 2008 13:34:04 +0000
Subject: [PATCH] RESOLVED - issue BATCH-321: Throw exception if attempt to
 create duplicate job instance occurs (based on job parameters)
 http://jira.springframework.org/browse/BATCH-321

repository now throws exception if JobInstance already exists for non-restartable job. Had to tweak sample junit launcher to allow parameter setting because HibernateJob is run twice.
---
 .../repository/SimpleJobRepository.java       | 39 ++++++++++---------
 .../execution/launch/SimpleJobTests.java      |  2 +
 .../SimpleJobRepositoryIntegrationTests.java  | 22 +++++++++++
 .../StepExecutorInterruptionTests.java        |  7 +++-
 .../sample/AbstractBatchLauncherTests.java    | 12 +++++-
 .../HibernateFailureJobFunctionalTests.java   |  4 ++
 6 files changed, 64 insertions(+), 22 deletions(-)

diff --git a/spring-batch-execution/src/main/java/org/springframework/batch/execution/repository/SimpleJobRepository.java b/spring-batch-execution/src/main/java/org/springframework/batch/execution/repository/SimpleJobRepository.java
index 86b27c026..d4e9c201a 100644
--- a/spring-batch-execution/src/main/java/org/springframework/batch/execution/repository/SimpleJobRepository.java
+++ b/spring-batch-execution/src/main/java/org/springframework/batch/execution/repository/SimpleJobRepository.java
@@ -87,10 +87,10 @@ public class SimpleJobRepository implements JobRepository {
 	 * <p>
 	 * There are two ways in which the method determines if a job should be
 	 * created or an existing one should be returned. The first is
-	 * restartability. The {@link Job} restartable property will be
-	 * checked first. If it is false, a new job will be created, regardless of
-	 * whether or not one exists. If it is true, the {@link JobInstanceDao} will
-	 * be checked to determine if the job already exists, if it does, it's steps
+	 * restartability. The {@link Job} restartable property will be checked
+	 * first. If it is false, a new job will be created, regardless of whether
+	 * or not one exists. If it is true, the {@link JobInstanceDao} will be
+	 * checked to determine if the job already exists, if it does, it's steps
 	 * will be populated (there must be at least 1) and a new
 	 * {@link JobExecution} will be returned. If no job instance is found, a new
 	 * one will be created.
@@ -106,9 +106,9 @@ public class SimpleJobRepository implements JobRepository {
 	 * <li>What happens then depends on how many existing job instances we
 	 * find:
 	 * <ul>
-	 * <li>If there are none, or the {@link Job} is marked restartable,
-	 * then we create a new {@link JobInstance}</li>
-	 * <li>If there is more than one and the {@link JobSupport} is not marked
+	 * <li>If there are none, or the {@link Job} is marked restartable, then we
+	 * create a new {@link JobInstance}</li>
+	 * <li>If there is more than one and the {@link Job} is not marked
 	 * as restartable, it is an error. This could be caused by a job whose
 	 * restartable flag has changed to be more strict (true not false)
 	 * <em>after</em> it has been executed at least once.</li>
@@ -136,6 +136,7 @@ public class SimpleJobRepository implements JobRepository {
 	 * JobInstance.getJobExecutionCount() is greater than Job.getStartLimit()
 	 * @throws JobExecutionAlreadyRunningException if a job execution is found
 	 * for the given {@link JobIdentifier} that is already running
+	 * @throws CannotRestartJobInstanceException 
 	 * 
 	 */
 	public JobExecution createJobExecution(Job job, JobParameters jobParameters)
@@ -147,19 +148,19 @@ public class SimpleJobRepository implements JobRepository {
 		List jobInstances = new ArrayList();
 		JobInstance jobInstance;
 
-		// Check if a job is restartable, if not, create and return a new job
-		if (job.isRestartable()) {
+		/*
+		 * Find all jobs matching the runtime information.
+		 * 
+		 * If this method is transactional, and the isolation level is
+		 * REPEATABLE_READ or better, another launcher trying to start the same
+		 * job in another thread or process will block until this transaction
+		 * has finished.
+		 */
 
-			/*
-			 * Find all jobs matching the runtime information.
-			 * 
-			 * Always do this if the job is restartable, then if this method is
-			 * transactional, and the isolation level is REPEATABLE_READ or
-			 * better, another launcher trying to start the same job in another
-			 * thread or process will block until this transaction has finished.
-			 */
-
-			jobInstances = jobInstanceDao.findJobInstances(job, jobParameters);
+		jobInstances = jobInstanceDao.findJobInstances(job, jobParameters);
+		
+		if ((jobInstances.size() > 0) && (job.isRestartable() == false)) {
+			throw new BatchRestartException("JobInstance already exists and is not restartable");
 		}
 
 		if (jobInstances.size() == 1) {
diff --git a/spring-batch-execution/src/test/java/org/springframework/batch/execution/launch/SimpleJobTests.java b/spring-batch-execution/src/test/java/org/springframework/batch/execution/launch/SimpleJobTests.java
index fda801f5b..06c30e73a 100644
--- a/spring-batch-execution/src/test/java/org/springframework/batch/execution/launch/SimpleJobTests.java
+++ b/spring-batch-execution/src/test/java/org/springframework/batch/execution/launch/SimpleJobTests.java
@@ -65,6 +65,8 @@ public class SimpleJobTests extends TestCase {
 	protected void setUp() throws Exception {
 		super.setUp();
 		job.setJobRepository(repository);
+		MapJobDao.clear();
+		MapStepDao.clear();
 	}
 
 	private AbstractStep getStep(String arg) throws Exception {
diff --git a/spring-batch-execution/src/test/java/org/springframework/batch/execution/repository/SimpleJobRepositoryIntegrationTests.java b/spring-batch-execution/src/test/java/org/springframework/batch/execution/repository/SimpleJobRepositoryIntegrationTests.java
index 263758c93..d41b4ba4a 100644
--- a/spring-batch-execution/src/test/java/org/springframework/batch/execution/repository/SimpleJobRepositoryIntegrationTests.java
+++ b/spring-batch-execution/src/test/java/org/springframework/batch/execution/repository/SimpleJobRepositoryIntegrationTests.java
@@ -6,6 +6,7 @@ import java.util.Map;
 
 import org.springframework.batch.core.domain.JobExecution;
 import org.springframework.batch.core.domain.JobParameters;
+import org.springframework.batch.core.repository.BatchRestartException;
 import org.springframework.batch.execution.job.JobSupport;
 import org.springframework.test.AbstractTransactionalDataSourceSpringContextTests;
 import org.springframework.util.ClassUtils;
@@ -90,4 +91,25 @@ public class SimpleJobRepositoryIntegrationTests extends AbstractTransactionalDa
 		assertEquals(firstExecution.getJobInstance(), secondExecution.getJobInstance());
 		assertEquals(job, secondExecution.getJobInstance().getJob());
 	}
+
+	/**
+	 * Non-restartable JobInstance can be run only once - attempt to run
+	 * existing non-restartable JobInstance causes error.
+	 */
+	public void testRunNonRestartableJobInstanceTwice() throws Exception {
+		JobSupport job = new JobSupport("nonRestartableJob");
+		job.setRestartable(false);
+		JobParameters jobParameters = new JobParameters();
+		
+		JobExecution firstExecution = jobRepository.createJobExecution(job, jobParameters);
+		jobRepository.saveOrUpdate(firstExecution);
+		
+		try {
+			jobRepository.createJobExecution(job, jobParameters);
+			fail();
+		}
+		catch (BatchRestartException e) {
+			// expected
+		}
+	}
 }
\ No newline at end of file
diff --git a/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/support/StepExecutorInterruptionTests.java b/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/support/StepExecutorInterruptionTests.java
index f4ad4a5db..0ca88f7ba 100644
--- a/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/support/StepExecutorInterruptionTests.java
+++ b/spring-batch-execution/src/test/java/org/springframework/batch/execution/step/support/StepExecutorInterruptionTests.java
@@ -53,12 +53,14 @@ public class StepExecutorInterruptionTests extends TestCase {
 	private ItemOrientedStep step;
 
 	public void setUp() throws Exception {
-
+		MapJobDao.clear();
+		MapStepDao.clear();
+		
 		jobRepository = new SimpleJobRepository(jobInstanceDao, jobExecutionDao, stepExecutionDao);
 
 		JobSupport jobConfiguration = new JobSupport();
 		step = new ItemOrientedStep();
-		step.setName("stepName");
+		step.setName("interruptedStep");
 		jobConfiguration.addStep(step);
 		jobConfiguration.setBeanName("testJob");
 		jobRepository.createJobExecution(jobConfiguration, new JobParameters());
@@ -71,6 +73,7 @@ public class StepExecutorInterruptionTests extends TestCase {
 		step.afterPropertiesSet();
 	}
 
+
 	public void testInterruptChunk() throws Exception {
 
 		JobExecution jobExecutionContext = new JobExecution(new JobInstance(new Long(0L), new JobParameters(), new JobSupport("testJob")));
diff --git a/spring-batch-samples/src/test/java/org/springframework/batch/sample/AbstractBatchLauncherTests.java b/spring-batch-samples/src/test/java/org/springframework/batch/sample/AbstractBatchLauncherTests.java
index ec5c27f97..d69173b2e 100644
--- a/spring-batch-samples/src/test/java/org/springframework/batch/sample/AbstractBatchLauncherTests.java
+++ b/spring-batch-samples/src/test/java/org/springframework/batch/sample/AbstractBatchLauncherTests.java
@@ -38,8 +38,14 @@ public abstract class AbstractBatchLauncherTests extends
 
 	private static final String CONTAINER_DEFINITION_LOCATION = "simple-container-definition.xml";
 
+	public AbstractBatchLauncherTests() {
+		setDependencyCheck(false);
+	}
+	
 	JobLauncher launcher;
 	private Job job;
+	
+	private JobParameters jobParameters = new JobParameters();
 
 	/*
 	 * (non-Javadoc)
@@ -74,11 +80,15 @@ public abstract class AbstractBatchLauncherTests extends
 		return job.getName();
 	}
 
+	public void setJobParameters(JobParameters jobParameters) {
+		this.jobParameters = jobParameters;
+	}
+
 	/**
 	 * @throws Exception
 	 * 
 	 */
 	public void testLaunchJob() throws Exception {
-		launcher.run(job, new JobParameters());
+		launcher.run(job, jobParameters);
 	}
 }
diff --git a/spring-batch-samples/src/test/java/org/springframework/batch/sample/HibernateFailureJobFunctionalTests.java b/spring-batch-samples/src/test/java/org/springframework/batch/sample/HibernateFailureJobFunctionalTests.java
index b6a192ec3..4409359ba 100644
--- a/spring-batch-samples/src/test/java/org/springframework/batch/sample/HibernateFailureJobFunctionalTests.java
+++ b/spring-batch-samples/src/test/java/org/springframework/batch/sample/HibernateFailureJobFunctionalTests.java
@@ -3,6 +3,8 @@ package org.springframework.batch.sample;
 import java.math.BigDecimal;
 import java.util.List;
 
+import org.springframework.batch.core.domain.JobParameters;
+import org.springframework.batch.core.domain.JobParametersBuilder;
 import org.springframework.batch.sample.dao.HibernateCreditWriter;
 import org.springframework.jdbc.UncategorizedSQLException;
 import org.springframework.orm.hibernate3.HibernateJdbcException;
@@ -48,6 +50,8 @@ public class HibernateFailureJobFunctionalTests extends
 	 * @see org.springframework.batch.sample.AbstractValidatingBatchLauncherTests#testLaunchJob()
 	 */
 	public void testLaunchJob() throws Exception {
+		JobParameters params = new JobParametersBuilder().addString("key", "failureJob").toJobParameters();
+		setJobParameters(params);
 		writer.setFailOnFlush(2);
 
 		int before = jdbcTemplate.queryForInt("SELECT COUNT(*) from CUSTOMER");