RESOLVED - issue BATCH-1411: Allow a Job to specify its required JobParameters

Move validation check to AbstractJob (and out of Job)

spring-batch-core/src/main/java/org/springframework/batch/core/Job.java

@@ -54,6 +54,4 @@ public interface Job {
 	 */
 	JobParametersIncrementer getJobParametersIncrementer();
 
-	void validate(JobParameters parameters) throws JobParametersInvalidException;
-
 }

spring-batch-core/src/main/java/org/springframework/batch/core/configuration/support/GroupAwareJob.java

@@ -17,9 +17,7 @@ package org.springframework.batch.core.configuration.support;
 
 import org.springframework.batch.core.Job;
 import org.springframework.batch.core.JobExecution;
-import org.springframework.batch.core.JobParameters;
 import org.springframework.batch.core.JobParametersIncrementer;
-import org.springframework.batch.core.JobParametersInvalidException;
 
 /**
  * A {@link Job} that can optionally prepend a group name to another job's name,
@@ -71,10 +69,6 @@ public class GroupAwareJob implements Job {
 		delegate.execute(execution);
 	}
 	
-	public void validate(JobParameters parameters) throws JobParametersInvalidException {
-		delegate.validate(parameters);
-	}
-
 	/**
 	 * Concatenates the group name and the delegate job name (joining with a
 	 * ".").

spring-batch-core/src/main/java/org/springframework/batch/core/job/AbstractJob.java

@@ -16,6 +16,8 @@
 
 package org.springframework.batch.core.job;
 
+import java.io.PrintWriter;
+import java.io.StringWriter;
 import java.util.Collection;
 import java.util.Date;
 
@@ -29,12 +31,12 @@ import org.springframework.batch.core.JobExecutionException;
 import org.springframework.batch.core.JobExecutionListener;
 import org.springframework.batch.core.JobInstance;
 import org.springframework.batch.core.JobInterruptedException;
-import org.springframework.batch.core.JobParameters;
 import org.springframework.batch.core.JobParametersIncrementer;
-import org.springframework.batch.core.JobParametersInvalidException;
 import org.springframework.batch.core.StartLimitExceededException;
 import org.springframework.batch.core.Step;
 import org.springframework.batch.core.StepExecution;
+import org.springframework.batch.core.launch.NoSuchJobException;
+import org.springframework.batch.core.launch.support.ExitCodeMapper;
 import org.springframework.batch.core.listener.CompositeJobExecutionListener;
 import org.springframework.batch.core.repository.JobRepository;
 import org.springframework.batch.core.repository.JobRestartException;
@@ -99,16 +101,6 @@ public abstract class AbstractJob implements Job, StepLocator, BeanNameAware, In
 		this.jobParametersValidator = jobParametersValidator;
 	}
 
-	/**
-	 * Delegates to the {@link #setJobParametersValidator validator} supplied
-	 * (defaults to just checking for null parameters).
-	 * 
-	 * @see Job#validate(JobParameters)
-	 */
-	public void validate(JobParameters parameters) throws JobParametersInvalidException {
-		jobParametersValidator.validate(parameters);
-	}
-
 	/**
 	 * Assert mandatory properties: {@link JobRepository}.
 	 * 
@@ -263,6 +255,8 @@ public abstract class AbstractJob implements Job, StepLocator, BeanNameAware, In
 
 		try {
 
+			jobParametersValidator.validate(execution.getJobInstance().getJobParameters());
+
 			if (execution.getStatus() != BatchStatus.STOPPING) {
 
 				execution.setStartTime(new Date());
@@ -291,21 +285,23 @@ public abstract class AbstractJob implements Job, StepLocator, BeanNameAware, In
 		}
 		catch (JobInterruptedException e) {
 			logger.error("Encountered interruption executing job", e);
-			execution.setExitStatus(ExitStatus.STOPPED);
+			execution.setExitStatus(getDefaultExitStatusForFailure(e));
 			execution.setStatus(BatchStatus.STOPPED);
 			execution.addFailureException(e);
 		}
 		catch (Throwable t) {
 			logger.error("Encountered fatal error executing job", t);
-			execution.setExitStatus(ExitStatus.FAILED);
+			execution.setExitStatus(getDefaultExitStatusForFailure(t));
 			execution.setStatus(BatchStatus.FAILED);
 			execution.addFailureException(t);
 		}
 		finally {
 
-			if (execution.getStepExecutions().isEmpty()) {
-				execution.setExitStatus(ExitStatus.NOOP
-						.addExitDescription("All steps already completed or no steps configured for this job."));
+			if (execution.getStatus().isLessThanOrEqualTo(BatchStatus.STOPPED)
+					&& execution.getStepExecutions().isEmpty()) {
+				ExitStatus exitStatus = execution.getExitStatus();
+				execution.setExitStatus(exitStatus.and(ExitStatus.NOOP
+						.addExitDescription("All steps already completed or no steps configured for this job.")));
 			}
 
 			execution.setEndTime(new Date());
@@ -458,6 +454,30 @@ public abstract class AbstractJob implements Job, StepLocator, BeanNameAware, In
 		}
 	}
 
+	/**
+	 * Default mapping from throwable to {@link ExitStatus}.
+	 * 
+	 * @param ex the cause of the failure
+	 * @return an {@link ExitStatus}
+	 */
+	private ExitStatus getDefaultExitStatusForFailure(Throwable ex) {
+		ExitStatus exitStatus;
+		if (ex instanceof JobInterruptedException || ex.getCause() instanceof JobInterruptedException) {
+			exitStatus = ExitStatus.STOPPED.addExitDescription(JobInterruptedException.class.getName());
+		}
+		else if (ex instanceof NoSuchJobException || ex.getCause() instanceof NoSuchJobException) {
+			exitStatus = new ExitStatus(ExitCodeMapper.NO_SUCH_JOB, ex.getClass().getName());
+		}
+		else {
+			StringWriter writer = new StringWriter();
+			ex.printStackTrace(new PrintWriter(writer));
+			String message = writer.toString();
+			exitStatus = ExitStatus.FAILED.addExitDescription(message);
+		}
+
+		return exitStatus;
+	}
+
 	private void updateStatus(JobExecution jobExecution, BatchStatus status) {
 		jobExecution.setStatus(status);
 		jobRepository.update(jobExecution);

spring-batch-core/src/main/java/org/springframework/batch/core/job/DefaultJobParametersValidator.java

@@ -3,6 +3,7 @@ package org.springframework.batch.core.job;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.Set;
 
 import org.springframework.batch.core.JobParameters;
 import org.springframework.batch.core.JobParametersInvalidException;
@@ -27,14 +28,19 @@ public class DefaultJobParametersValidator implements JobParametersValidator, In
 	 */
 	public void afterPropertiesSet() throws IllegalStateException {
 		for (String key : requiredKeys) {
-			Assert.state(!optionalKeys.contains(key), "Optional keys canot be required: "+key);
+			Assert.state(!optionalKeys.contains(key), "Optional keys canot be required: " + key);
 		}
 	}
 
 	/**
-	 * Check the parameters meet the specification provided.
+	 * Check the parameters meet the specification provided. If optional keys
+	 * are explicitly specified then all keys must be in that list, or in the
+	 * required list. Otherwise all keys that are specified as required must be
+	 * present.
 	 * 
 	 * @see JobParametersValidator#validate(JobParameters)
+	 * 
+	 * @throws JobParametersInvalidException if the parameters are not valid
 	 */
 	public void validate(JobParameters parameters) throws JobParametersInvalidException {
 
@@ -42,9 +48,28 @@ public class DefaultJobParametersValidator implements JobParametersValidator, In
 			throw new JobParametersInvalidException("The JobParameters can not be null");
 		}
 
+		Set<String> keys = parameters.getParameters().keySet();
+
+		// If there are explicit optional keys then all keys must be in that
+		// group, or in the required group.
+		if (!optionalKeys.isEmpty()) {
+
+			Collection<String> missingKeys = new HashSet<String>();
+			for (String key : keys) {
+				if (!optionalKeys.contains(key) && !requiredKeys.contains(key)) {
+					missingKeys.add(key);
+				}
+			}
+			if (!missingKeys.isEmpty()) {
+				throw new JobParametersInvalidException(
+						"The JobParameters contains keys that are not explicitly optional or required: " + missingKeys);
+			}
+
+		}
+
 		Collection<String> missingKeys = new HashSet<String>();
 		for (String key : requiredKeys) {
-			if (!parameters.getParameters().containsKey(key)) {
+			if (!keys.contains(key)) {
 				missingKeys.add(key);
 			}
 		}
@@ -55,18 +80,27 @@ public class DefaultJobParametersValidator implements JobParametersValidator, In
 	}
 
 	/**
-	 * The keys that are required in the parameters.
+	 * The keys that are required in the parameters. The default is empty,
+	 * meaning that all parameters are optional, unless optional keys are
+	 * explicitly specified.
 	 * 
 	 * @param requiredKeys the required key values
+	 * 
+	 * @see #setOptionalKeys(String[])
 	 */
 	public void setRequiredKeys(String[] requiredKeys) {
 		this.requiredKeys = new HashSet<String>(Arrays.asList(requiredKeys));
 	}
 
 	/**
-	 * The keys that are optional in the parameters.
+	 * The keys that are optional in the parameters. If any keys are explicitly
+	 * optional, then to be valid all other keys must be explicitly required.
+	 * The default is empty, meaning that all parameters that are not required
+	 * are optional.
 	 * 
 	 * @param optionalKeys the optional key values
+	 * 
+	 * @see #setRequiredKeys(String[])
 	 */
 	public void setOptionalKeys(String[] optionalKeys) {
 		this.optionalKeys = new HashSet<String>(Arrays.asList(optionalKeys));

spring-batch-core/src/main/java/org/springframework/batch/core/launch/support/SimpleJobLauncher.java

@@ -84,9 +84,6 @@ public class SimpleJobLauncher implements JobLauncher, InitializingBean {
 		Assert.notNull(job, "The Job must not be null.");
 		Assert.notNull(jobParameters, "The JobParameters must not be null.");
 
-		// Allow the job to veto the execution
-		job.validate(jobParameters);
-
 		final JobExecution jobExecution;
 		JobExecution lastExecution = jobRepository.getLastJobExecution(job.getName(), jobParameters);
 		if (lastExecution != null) {

spring-batch-core/src/test/java/org/springframework/batch/core/configuration/xml/SplitInterruptedJobParserTests.java

@@ -50,7 +50,7 @@ public class SplitInterruptedJobParserTests extends AbstractJobParserTests {
 		Thread.sleep(200L);
 		
 		assertEquals(BatchStatus.STOPPED, jobExecution.getStatus());
-		assertEquals(ExitStatus.STOPPED, jobExecution.getExitStatus());
+		assertEquals(ExitStatus.STOPPED.getExitCode(), jobExecution.getExitStatus().getExitCode());
 
 		assertTrue(stepNamesList.contains("stop"));
 

spring-batch-core/src/test/java/org/springframework/batch/core/job/DefaultJobParametersValidatorTests.java

@@ -14,6 +14,11 @@ public class DefaultJobParametersValidatorTests {
 		validator.validate(null);
 	}
 
+	@Test
+	public void testValidateNoRequiredValues() throws Exception {
+		validator.validate(new JobParametersBuilder().addString("name", "foo").toJobParameters());
+	}
+
 	@Test
 	public void testValidateRequiredValues() throws Exception {
 		validator.setRequiredKeys(new String[] { "name", "value" });
@@ -33,6 +38,19 @@ public class DefaultJobParametersValidatorTests {
 		validator.validate(new JobParameters());
 	}
 
+	@Test(expected = JobParametersInvalidException.class)
+	public void testValidateOptionalWithImplicitRequiredKey() throws Exception {
+		validator.setOptionalKeys(new String[] { "name", "value" });
+		validator.validate(new JobParametersBuilder().addString("foo", "bar").toJobParameters());
+	}
+
+	@Test
+	public void testValidateOptionalWithExplicitRequiredKey() throws Exception {
+		validator.setOptionalKeys(new String[] { "name", "value" });
+		validator.setRequiredKeys(new String[] { "foo" });
+		validator.validate(new JobParametersBuilder().addString("foo", "bar").toJobParameters());
+	}
+
 	@Test(expected = IllegalStateException.class)
 	public void testOptionalValuesAlsoRequired() throws Exception {
 		validator.setOptionalKeys(new String[] { "name", "value" });

spring-batch-core/src/test/java/org/springframework/batch/core/job/ExtendedAbstractJobTests.java

@@ -25,6 +25,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
 
+import org.junit.Before;
 import org.junit.Test;
 import org.springframework.batch.core.BatchStatus;
 import org.springframework.batch.core.JobExecution;
@@ -45,7 +46,16 @@ import org.springframework.batch.support.transaction.ResourcelessTransactionMana
  */
 public class ExtendedAbstractJobTests {
 
-	AbstractJob job = new StubJob("job");
+	private AbstractJob job;
+	private JobRepository jobRepository;
+	
+	@Before
+	public void setUp() throws Exception {
+		MapJobRepositoryFactoryBean factory = new MapJobRepositoryFactoryBean();
+		factory.setTransactionManager(new ResourcelessTransactionManager());
+		jobRepository = (JobRepository) factory.getObject();
+		job = new StubJob("job", jobRepository);
+	}
 
 	/**
 	 * Test method for
@@ -75,7 +85,7 @@ public class ExtendedAbstractJobTests {
 	 */
 	@Test
 	public void testSetBeanNameWithNullName() {
-		job = new StubJob(null);
+		job = new StubJob(null, null);
 		assertEquals(null, job.getName());
 		job.setBeanName("foo");
 		assertEquals("foo", job.getName());
@@ -111,26 +121,27 @@ public class ExtendedAbstractJobTests {
 		}
 	}
 	
-	@Test(expected=JobParametersInvalidException.class)
-	public void testValidatorWithNullParameters() throws Exception {
-		job.validate(null);
-	}
-
 	@Test
 	public void testValidatorWithNotNullParameters() throws Exception {
-		job.validate(new JobParameters());
+		JobExecution execution = jobRepository.createJobExecution("job", new JobParameters());
+		job.execute(execution);
 		// Should be free of side effects
 	}
 
-	@Test(expected=JobParametersInvalidException.class)
+	@Test
 	public void testSetValidator() throws Exception {
 		job.setJobParametersValidator(new DefaultJobParametersValidator() {
 			@Override
 			public void validate(JobParameters parameters) throws JobParametersInvalidException {
-				throw new JobParametersInvalidException("Expected");
+				throw new JobParametersInvalidException("FOO");
 			}
 		});
-		job.validate(new JobParameters());
+		JobExecution execution = jobRepository.createJobExecution("job", new JobParameters());
+		job.execute(execution);
+		assertEquals(BatchStatus.FAILED, execution.getStatus());
+		assertEquals("FOO", execution.getFailureExceptions().get(0).getMessage());
+		String description = execution.getExitStatus().getExitDescription();
+		assertTrue("Wrong description: "+description, description.contains("FOO"));
 	}
 
 	/**
@@ -183,9 +194,16 @@ public class ExtendedAbstractJobTests {
 	private static class StubJob extends AbstractJob {
 		/**
 		 * @param name
+		 * @param jobRepository 
 		 */
-		private StubJob(String name) {
+		private StubJob(String name, JobRepository jobRepository) {
 			super(name);
+			try {
+				setJobRepository(jobRepository);
+			}
+			catch (Exception e) {
+				throw new IllegalStateException(e);
+			}
 		}
 
 		/**

spring-batch-core/src/test/java/org/springframework/batch/core/job/JobSupport.java

@@ -21,9 +21,7 @@ import java.util.List;
 
 import org.springframework.batch.core.Job;
 import org.springframework.batch.core.JobExecution;
-import org.springframework.batch.core.JobParameters;
 import org.springframework.batch.core.JobParametersIncrementer;
-import org.springframework.batch.core.JobParametersInvalidException;
 import org.springframework.batch.core.Step;
 import org.springframework.batch.core.UnexpectedJobExecutionException;
 import org.springframework.beans.factory.BeanNameAware;
@@ -135,13 +133,6 @@ public class JobSupport implements BeanNameAware, Job {
 		return restartable;
 	}
 	
-	/**
-	 * @see Job#validate(JobParameters)
-	 */
-	public void validate(JobParameters parameters) throws JobParametersInvalidException {
-		
-	}
-
 	/*
 	 * (non-Javadoc)
 	 * 

spring-batch-core/src/test/java/org/springframework/batch/core/launch/SimpleJobLauncherTests.java

@@ -72,7 +72,10 @@ public class SimpleJobLauncherTests {
 
 	@Test
 	public void testRun() throws Exception {
+		run(ExitStatus.COMPLETED);
+	}
 
+	private void run(ExitStatus exitStatus) throws Exception {
 		JobExecution jobExecution = new JobExecution(null, null);
 
 		expect(jobRepository.getLastJobExecution(job.getName(), jobParameters)).andReturn(null);
@@ -80,10 +83,13 @@ public class SimpleJobLauncherTests {
 		replay(jobRepository);
 
 		jobLauncher.afterPropertiesSet();
-		jobLauncher.run(job, jobParameters);
-		assertEquals(ExitStatus.COMPLETED, jobExecution.getExitStatus());
-
-		verify(jobRepository);
+		try {
+			jobLauncher.run(job, jobParameters);
+		}
+		finally {
+			assertEquals(exitStatus, jobExecution.getExitStatus());
+			verify(jobRepository);
+		}
 	}
 
 	/*
@@ -142,7 +148,7 @@ public class SimpleJobLauncherTests {
 			}
 		};
 		try {
-			testRun();
+			run(ExitStatus.FAILED);
 			fail("Expected RuntimeException");
 		}
 		catch (RuntimeException e) {
@@ -159,7 +165,7 @@ public class SimpleJobLauncherTests {
 			}
 		};
 		try {
-			testRun();
+			run(ExitStatus.FAILED);
 			fail("Expected Error");
 		}
 		catch (RuntimeException e) {

spring-batch-core/src/test/java/org/springframework/batch/core/step/tasklet/TaskletStepTests.java

@@ -697,9 +697,7 @@ public class TaskletStepTests {
 
 		step.execute(stepExecution);
 		assertEquals(BatchStatus.UNKNOWN, stepExecution.getStatus());
-		String msg = stepExecution.getExitStatus().getExitDescription();
 		Throwable ex = stepExecution.getFailureExceptions().get(0);
-		msg = ex.getMessage();
 		// The original rollback failed because of this one:
 		assertEquals("Bar", ex.getMessage());
 	}
@@ -890,16 +888,10 @@ public class TaskletStepTests {
 
 		private boolean restoreFromCalled = false;
 
-		private boolean restoreFromCalledWithSomeContext = false;
-
 		public String read() throws Exception {
 			return "item";
 		}
 
-		public boolean isRestoreFromCalledWithSomeContext() {
-			return restoreFromCalledWithSomeContext;
-		}
-
 		public void update(ExecutionContext executionContext) {
 			getExecutionAttributesCalled = true;
 			executionContext.putString("spam", "bucket");
@@ -920,10 +912,6 @@ public class TaskletStepTests {
 		public void beforeStep(StepExecution stepExecution) {
 		}
 
-		public ExitStatus onErrorInStep(StepExecution stepExecution, Throwable e) {
-			return null;
-		}
-
 	}
 
 }

spring-batch-samples/src/test/java/org/springframework/batch/sample/support/JobSupport.java

@@ -21,9 +21,7 @@ import java.util.List;
 
 import org.springframework.batch.core.Job;
 import org.springframework.batch.core.JobExecution;
-import org.springframework.batch.core.JobParameters;
 import org.springframework.batch.core.JobParametersIncrementer;
-import org.springframework.batch.core.JobParametersInvalidException;
 import org.springframework.batch.core.Step;
 import org.springframework.batch.core.UnexpectedJobExecutionException;
 import org.springframework.beans.factory.BeanNameAware;
@@ -99,10 +97,6 @@ public class JobSupport implements BeanNameAware, Job {
 		return name;
 	}
 	
-	public void validate(JobParameters parameters) throws JobParametersInvalidException {
-		// no-op
-	}
-
 	public void setSteps(List<Step> steps) {
 		this.steps.clear();
 		this.steps.addAll(steps);