Make JobParametersBuilder#getNextJobParameters consistent with CommandLineJobRunner and JobOperator

Before this commit, JobParametersBuilder#getNextJobParameters was checking
for restartability conditions. This is not only already done by the
JobLauncher, but also makes it inconsistent with the behaviour of
CommandLineJobRunner when used with "-next" option and
JobOperator#startNextInstance, which is starting the next instance in
sequence based on the incrementer without dealing with restartability.

This commit fixes the JobParametersBuilder#getNextJobParameters to behave
like the CommandLineJobRunner and JobOperator in regards to starting
the next instance.

Resolves BATCH-2711

Author: fmbenhassine

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

@@ -29,7 +29,7 @@
 /**
  * Helper class for creating {@link JobParameters}. Useful because all
  * {@link JobParameter} objects are immutable, and must be instantiated separately
- * to ensure typesafety. Once created, it can be used in the
+ * to ensure type safety. Once created, it can be used in the
  * same was a java.lang.StringBuilder (except, order is irrelevant), by adding
  * various parameter types and creating a valid {@link JobParameters} once
  * finished.<br>
@@ -239,66 +239,46 @@ public JobParametersBuilder addJobParameters(JobParameters jobParameters) {
 	/**
 	 * Initializes the {@link JobParameters} based on the state of the {@link Job}.  This
 	 * should be called after all parameters have been entered into the builder.
+	 * All parameters already set on this builder instance will be appended to
+	 * those retrieved from the job incrementer, overriding any with the same key (Same
+	 * behaviour as {@link org.springframework.batch.core.launch.support.CommandLineJobRunner}
+	 * with "-next" option and {@link org.springframework.batch.core.launch.JobOperator#startNextInstance(String)})
 	 *
 	 * @param job the job for which the {@link JobParameters} are being constructed.
 	 * @return a reference to this object.
 	 *
 	 * @since 4.0
 	 */
 	public JobParametersBuilder getNextJobParameters(Job job) {
-		if(this.jobExplorer == null) {
-			throw new IllegalStateException("A JobExplore is required to get next job parameters");
-		}
+		Assert.state(this.jobExplorer != null, "A JobExplorer is required to get next job parameters");
+		Assert.notNull(job, "Job must not be null");
+		Assert.notNull(job.getJobParametersIncrementer(), "No job parameters incrementer found for job=" + job.getName());
 
 		String name = job.getName();
-		JobParameters nextParameters = new JobParameters();
+		JobParameters nextParameters;
 		List<JobInstance> lastInstances = this.jobExplorer.getJobInstances(name, 0, 1);
 		JobParametersIncrementer incrementer = job.getJobParametersIncrementer();
 		if (lastInstances.isEmpty()) {
 			// Start from a completely clean sheet
-			if (incrementer != null) {
-				nextParameters = incrementer.getNext(new JobParameters());
-			}
+			nextParameters = incrementer.getNext(new JobParameters());
 		}
 		else {
-			List<JobExecution> previousExecutions = this.jobExplorer
-					.getJobExecutions(lastInstances.get(0));
-			JobExecution previousExecution = previousExecutions.get(0);
-			if (previousExecution == null) {
+			List<JobExecution> previousExecutions = this.jobExplorer.getJobExecutions(lastInstances.get(0));
+			if (previousExecutions.isEmpty()) {
 				// Normally this will not happen - an instance exists with no executions
-				if (incrementer != null) {
-					nextParameters = incrementer.getNext(new JobParameters());
-				}
-			}
-			else if (isStoppedOrFailed(previousExecution) && job.isRestartable()) {
-				// Retry a failed or stopped execution
-				nextParameters = previousExecution.getJobParameters();
-				// Non-identifying additional parameters can be removed to a retry
-				removeNonIdentifying(this.parameterMap);
+				nextParameters = incrementer.getNext(new JobParameters());
 			}
-			else if (incrementer != null) {
-				// New instance so increment the parameters if we can
+			else {
+				JobExecution previousExecution = previousExecutions.get(0);
 				nextParameters = incrementer.getNext(previousExecution.getJobParameters());
 			}
 		}
 
-		this.parameterMap = addJobParameters(nextParameters)
-								.toJobParameters()
-								.getParameters();
+		// start with parameters from the incrementer
+		Map<String, JobParameter> nextParametersMap = new HashMap<>(nextParameters.getParameters());
+		// append new parameters (overriding those with the same key)
+		nextParametersMap.putAll(this.parameterMap);
+		this.parameterMap = nextParametersMap;
 		return this;
 	}
-
-	private void removeNonIdentifying(Map<String, JobParameter> parameters) {
-		HashMap<String, JobParameter> copy = new HashMap<>(parameters);
-		for (Map.Entry<String, JobParameter> parameter : copy.entrySet()) {
-			if (!parameter.getValue().isIdentifying()) {
-				parameters.remove(parameter.getKey());
-			}
-		}
-	}
-
-	private boolean isStoppedOrFailed(JobExecution execution) {
-		BatchStatus status = execution.getStatus();
-		return (status == BatchStatus.STOPPED || status == BatchStatus.FAILED);
-	}
 }

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

@@ -21,10 +21,8 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
 
@@ -36,8 +34,8 @@
 import org.springframework.batch.core.Job;
 import org.springframework.batch.core.JobExecution;
 import org.springframework.batch.core.JobInstance;
-import org.springframework.batch.core.JobParameter;
 import org.springframework.batch.core.JobParameters;
+import org.springframework.batch.core.JobParametersBuilder;
 import org.springframework.batch.core.JobParametersIncrementer;
 import org.springframework.batch.core.configuration.JobLocator;
 import org.springframework.batch.core.converter.DefaultJobParametersConverter;
@@ -47,7 +45,6 @@
 import org.springframework.batch.core.launch.JobExecutionNotRunningException;
 import org.springframework.batch.core.launch.JobExecutionNotStoppedException;
 import org.springframework.batch.core.launch.JobLauncher;
-import org.springframework.batch.core.launch.JobParametersNotFoundException;
 import org.springframework.batch.core.launch.NoSuchJobException;
 import org.springframework.batch.core.repository.JobRepository;
 import org.springframework.beans.factory.BeanDefinitionStoreException;
@@ -356,10 +353,9 @@ int start(String jobPath, String jobIdentifier, String[] parameters, Set<String>
 			}
 
 			if (opts.contains("-next")) {
-				JobParameters nextParameters = getNextJobParameters(job);
-				Map<String, JobParameter> map = new HashMap<String, JobParameter>(nextParameters.getParameters());
-				map.putAll(jobParameters.getParameters());
-				jobParameters = new JobParameters(map);
+				jobParameters = new JobParametersBuilder(jobParameters, jobExplorer)
+						.getNextJobParameters(job)
+						.toJobParameters();
 			}
 
 			JobExecution jobExecution = launcher.run(job, jobParameters);
@@ -469,35 +465,6 @@ private Long getLongIdentifier(String jobIdentifier) {
 		}
 	}
 
-	/**
-	 * @param job the job that we need to find the next parameters for
-	 * @return the next job parameters if they can be located
-	 * @throws JobParametersNotFoundException if there is a problem
-	 */
-	private JobParameters getNextJobParameters(Job job) throws JobParametersNotFoundException {
-		String jobIdentifier = job.getName();
-		JobParameters jobParameters;
-		List<JobInstance> lastInstances = jobExplorer.getJobInstances(jobIdentifier, 0, 1);
-
-		JobParametersIncrementer incrementer = job.getJobParametersIncrementer();
-		if (incrementer == null) {
-			throw new JobParametersNotFoundException("No job parameters incrementer found for job=" + jobIdentifier);
-		}
-
-		if (lastInstances.isEmpty()) {
-			jobParameters = incrementer.getNext(new JobParameters());
-			if (jobParameters == null) {
-				throw new JobParametersNotFoundException("No bootstrap parameters found from incrementer for job="
-						+ jobIdentifier);
-			}
-		}
-		else {
-			List<JobExecution> lastExecutions = jobExplorer.getJobExecutions(lastInstances.get(0));
-			jobParameters = incrementer.getNext(lastExecutions.get(0).getJobParameters());
-		}
-		return jobParameters;
-	}
-
 	/**
 	 * Launch a batch job using a {@link CommandLineJobRunner}. Creates a new
 	 * Spring context for the job execution, and uses a common parent for all
@@ -515,7 +482,7 @@ private JobParameters getNextJobParameters(Job job) throws JobParametersNotFound
 	 * interpreted either as the name of the job or the id of the job execution
 	 * that failed.</li>
 	 * <li>-next: (optional) if the job has a {@link JobParametersIncrementer}
-	 * that can be used to launch the next in a sequence</li>
+	 * that can be used to launch the next instance in a sequence</li>
 	 * <li>jobPath: the xml application context containing a {@link Job}
 	 * <li>jobIdentifier: the bean id of the job or id of the failed execution
 	 * in the case of a restart.

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2013 the original author or authors.
+ * Copyright 2006-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,7 +31,7 @@
 import org.springframework.batch.core.JobExecution;
 import org.springframework.batch.core.JobInstance;
 import org.springframework.batch.core.JobParameters;
-import org.springframework.batch.core.JobParametersIncrementer;
+import org.springframework.batch.core.JobParametersBuilder;
 import org.springframework.batch.core.JobParametersInvalidException;
 import org.springframework.batch.core.Step;
 import org.springframework.batch.core.StepExecution;
@@ -45,7 +45,6 @@
 import org.springframework.batch.core.launch.JobInstanceAlreadyExistsException;
 import org.springframework.batch.core.launch.JobLauncher;
 import org.springframework.batch.core.launch.JobOperator;
-import org.springframework.batch.core.launch.JobParametersNotFoundException;
 import org.springframework.batch.core.launch.NoSuchJobException;
 import org.springframework.batch.core.launch.NoSuchJobExecutionException;
 import org.springframework.batch.core.launch.NoSuchJobInstanceException;
@@ -79,6 +78,7 @@
  * @author Dave Syer
  * @author Lucas Ward
  * @author Will Schipp
+ * @author Mahmoud Ben Hassine
  * @since 2.0
  */
 public class SimpleJobOperator implements JobOperator, InitializingBean {
@@ -334,30 +334,15 @@ public Long start(String jobName, String parameters) throws NoSuchJobException,
 	 * @see JobOperator#startNextInstance(String )
 	 */
 	@Override
-	public Long startNextInstance(String jobName) throws NoSuchJobException, JobParametersNotFoundException,
+	public Long startNextInstance(String jobName) throws NoSuchJobException,
 	UnexpectedJobExecutionException, JobParametersInvalidException {
 
 		logger.info("Locating parameters for next instance of job with name=" + jobName);
 
 		Job job = jobRegistry.getJob(jobName);
-		List<JobInstance> lastInstances = jobExplorer.getJobInstances(jobName, 0, 1);
-
-		JobParametersIncrementer incrementer = job.getJobParametersIncrementer();
-		if (incrementer == null) {
-			throw new JobParametersNotFoundException("No job parameters incrementer found for job=" + jobName);
-		}
-
-		JobParameters parameters;
-		if (lastInstances.isEmpty()) {
-			parameters = incrementer.getNext(new JobParameters());
-			if (parameters == null) {
-				throw new JobParametersNotFoundException("No bootstrap parameters found for job=" + jobName);
-			}
-		}
-		else {
-			List<JobExecution> lastExecutions = jobExplorer.getJobExecutions(lastInstances.get(0));
-			parameters = incrementer.getNext(lastExecutions.get(0).getJobParameters());
-		}
+		JobParameters parameters = new JobParametersBuilder(jobExplorer)
+				.getNextJobParameters(job)
+				.toJobParameters();
 
 		logger.info(String.format("Attempting to launch job with name=%s and parameters=%s", jobName, parameters));
 		try {

spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersBuilderTests.java

@@ -23,7 +23,9 @@
 import java.util.Properties;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import org.springframework.batch.core.explore.JobExplorer;
 import org.springframework.batch.core.job.SimpleJob;
@@ -56,6 +58,9 @@ public class JobParametersBuilderTests {
 
 	private Date date = new Date(System.currentTimeMillis());
 
+	@Rule
+	public ExpectedException expectedException = ExpectedException.none();
+
 	@Before
 	public void initialize() {
 		this.job = new SimpleJob("simpleJob");
@@ -198,9 +203,10 @@ public void testGetNextJobParametersFirstRun(){
 
 	@Test
 	public void testGetNextJobParametersNoIncrementer(){
+		expectedException.expect(IllegalArgumentException.class);
+		expectedException.expectMessage("No job parameters incrementer found for job=simpleJob");
 		initializeForNextJobParameters();
 		this.parametersBuilder.getNextJobParameters(this.job);
-		baseJobParametersVerify(this.parametersBuilder.toJobParameters(), 3);
 	}
 
 	@Test
@@ -226,14 +232,13 @@ public void testGetNextJobParametersRestartable(){
 		initializeForNextJobParameters();
 		this.parametersBuilder.addLong("NON_IDENTIFYING_LONG", new Long(1), false);
 		this.parametersBuilder.getNextJobParameters(this.job);
-		baseJobParametersVerify(this.parametersBuilder.toJobParameters(), 3);
+		baseJobParametersVerify(this.parametersBuilder.toJobParameters(), 5);
 	}
 
 	@Test
 	public void testGetNextJobParametersNoPreviousExecution(){
 		this.job.setJobParametersIncrementer(new RunIdIncrementer());
 		this.jobInstanceList.add(new JobInstance(1L, "simpleJobInstance"));
-		this.jobExecutionList.add(null);
 		when(this.jobExplorer.getJobInstances("simpleJob",0,1)).thenReturn(this.jobInstanceList);
 		when(this.jobExplorer.getJobExecutions(any())).thenReturn(this.jobExecutionList);
 		initializeForNextJobParameters();

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2013 the original author or authors.
+ * Copyright 2006-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -67,6 +67,7 @@
 /**
  * @author Dave Syer
  * @author Will Schipp
+ * @author Mahmoud Ben Hassine
  *
  */
 public class SimpleJobOperatorTests {
@@ -153,6 +154,7 @@ public void testMandatoryProperties() throws Exception {
 	 */
 	@Test
 	public void testStartNextInstanceSunnyDay() throws Exception {
+		jobParameters = new JobParameters();
 		JobInstance jobInstance = new JobInstance(321L, "foo");
 		when(jobExplorer.getJobInstances("foo", 0, 1)).thenReturn(Collections.singletonList(jobInstance));
 		when(jobExplorer.getJobExecutions(jobInstance)).thenReturn(Collections.singletonList(new JobExecution(jobInstance, new JobParameters())));