Fix parameters parsing in JobOperator and MetaDataInstanceFactory

Before this commit, the parsing of job parameters in
JobOperator#start and MetaDataInstanceFactory#createJobExecution
was accepting the comma separated key=value pairs format,
which is incompatible with the new job parameters format
introduced in v5.

This commit updates the contract as well as the implementation
of those APIs to be compatible with v5.

Resolves #4253
Resolves #4301

Author: fmbenhassine

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

@@ -91,7 +91,8 @@ default JobInstance getJobInstance(String jobName, JobParameters jobParameters)
 	Set<Long> getRunningExecutions(String jobName) throws NoSuchJobException;
 
 	/**
-	 * Get the {@link JobParameters} as an easily readable String.
+	 * Get the {@link JobParameters} as a human readable String (new line separated
+	 * key=value pairs).
 	 * @param executionId the id of an existing {@link JobExecution}
 	 * @return the job parameters that were used to launch the associated instance
 	 * @throws NoSuchJobExecutionException if the id was not associated with any
@@ -102,8 +103,8 @@ default JobInstance getJobInstance(String jobName, JobParameters jobParameters)
 	/**
 	 * Start a new instance of a job with the parameters specified.
 	 * @param jobName the name of the {@link Job} to launch
-	 * @param parameters the parameters to launch it with (comma or newline separated
-	 * name=value pairs)
+	 * @param parameters the parameters to launch it with (new line separated key=value
+	 * pairs)
 	 * @return the id of the {@link JobExecution} that is launched
 	 * @throws NoSuchJobException if there is no {@link Job} with the specified name
 	 * @throws JobInstanceAlreadyExistsException if a job instance with this name and

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2022 the original author or authors.
+ * Copyright 2006-2023 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.
@@ -60,9 +60,9 @@
 import org.springframework.batch.core.step.tasklet.StoppableTasklet;
 import org.springframework.batch.core.step.tasklet.Tasklet;
 import org.springframework.batch.core.step.tasklet.TaskletStep;
+import org.springframework.batch.support.PropertiesConverter;
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.lang.Nullable;
-import org.springframework.transaction.annotation.Transactional;
 import org.springframework.util.Assert;
 
 /**
@@ -222,11 +222,7 @@ public String getParameters(long executionId) throws NoSuchJobExecutionException
 
 		Properties properties = this.jobParametersConverter.getProperties(jobExecution.getJobParameters());
 
-		List<String> keyValuePairs = new ArrayList<>();
-		for (Map.Entry<Object, Object> entry : properties.entrySet()) {
-			keyValuePairs.add(entry.getKey() + "=" + entry.getValue());
-		}
-		return String.join(" ", keyValuePairs);
+		return PropertiesConverter.propertiesToString(properties);
 	}
 
 	/*
@@ -319,14 +315,7 @@ public Long start(String jobName, String parameters)
 			logger.info("Checking status of job with name=" + jobName);
 		}
 
-		Properties properties = new Properties();
-		if (!parameters.isEmpty()) {
-			String[] keyValuePairs = parameters.split(" ");
-			for (String string : keyValuePairs) {
-				String[] keyValuePair = string.split("=");
-				properties.setProperty(keyValuePair[0], keyValuePair[1]);
-			}
-		}
+		Properties properties = PropertiesConverter.stringToProperties(parameters);
 		JobParameters jobParameters = jobParametersConverter.getJobParameters(properties);
 
 		if (jobRepository.isJobInstanceExists(jobName, jobParameters)) {

spring-batch-infrastructure/src/main/java/org/springframework/batch/support/PropertiesConverter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2007 the original author or authors.
+ * Copyright 2006-2023 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.
@@ -16,122 +16,74 @@
 
 package org.springframework.batch.support;
 
-import java.io.IOException;
-import java.io.StringReader;
-import java.io.StringWriter;
-import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 import java.util.Properties;
 
-import org.springframework.util.DefaultPropertiesPersister;
-import org.springframework.util.PropertiesPersister;
+import org.springframework.lang.NonNull;
+import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
- * Utility to convert a Properties object to a String and back. Ideally this utility
- * should have been used to convert to string in order to convert that string back to a
- * Properties Object. Attempting to convert a string obtained by calling
- * Properties.toString() will return an invalid Properties object. The format of
- * Properties is that used by {@link PropertiesPersister} from the Spring Core, so a
- * String in the correct format for a Spring property editor is fine (key=value pairs
- * separated by new lines).
+ * Utility to convert a Properties object to a String and back. The format of properties
+ * is new line separated key=value pairs.
  *
  * @author Lucas Ward
  * @author Dave Syer
- * @see PropertiesPersister
+ * @author Mahmoud Ben Hassine
  */
 public final class PropertiesConverter {
 
-	private static final PropertiesPersister propertiesPersister = new DefaultPropertiesPersister();
-
-	private static final String LINE_SEPARATOR = System.getProperty("line.separator");
+	private static final String LINE_SEPARATOR = "\n";
 
 	// prevents the class from being instantiated
 	private PropertiesConverter() {
 	}
 
 	/**
-	 * Parse a String to a Properties object. If string is null, an empty Properties
-	 * object will be returned. The input String is a set of name=value pairs, delimited
-	 * by either newline or comma (for brevity). If the input String contains a newline it
-	 * is assumed that the separator is newline, otherwise comma.
-	 * @param stringToParse String to parse.
-	 * @return Properties parsed from each string.
-	 * @see PropertiesPersister
+	 * Parse a String to a Properties object. If string is empty, an empty Properties
+	 * object will be returned. The input String should be a set of key=value pairs,
+	 * separated by a new line.
+	 * @param stringToParse String to parse. Must not be {@code null}.
+	 * @return Properties parsed from each key=value pair.
 	 */
-	public static Properties stringToProperties(String stringToParse) {
-
-		if (stringToParse == null) {
+	public static Properties stringToProperties(@NonNull String stringToParse) {
+		Assert.notNull(stringToParse, "stringToParse must not be null");
+		if (!StringUtils.hasText(stringToParse)) {
 			return new Properties();
 		}
-
-		if (!contains(stringToParse, "\n")) {
-			stringToParse = StringUtils
-					.arrayToDelimitedString(StringUtils.commaDelimitedListToStringArray(stringToParse), "\n");
-		}
-
-		StringReader stringReader = new StringReader(stringToParse);
-
 		Properties properties = new Properties();
-
-		try {
-			propertiesPersister.load(properties, stringReader);
-			// Exception is only thrown by StringReader after it is closed,
-			// so never in this case.
-		}
-		catch (IOException ex) {
-			throw new IllegalStateException(
-					"Error while trying to parse String to java.util.Properties," + " given String: " + properties);
+		String[] keyValuePairs = stringToParse.split(LINE_SEPARATOR);
+		for (String string : keyValuePairs) {
+			if (!string.contains("=")) {
+				throw new IllegalArgumentException(string + "is not a valid key=value pair");
+			}
+			String[] keyValuePair = string.split("=");
+			properties.setProperty(keyValuePair[0], keyValuePair[1]);
 		}
-
 		return properties;
 	}
 
 	/**
-	 * Convert Properties object to String. This is only necessary for compatibility with
-	 * converting the String back to a properties object. If an empty properties object is
-	 * passed in, a blank string is returned, otherwise it's string representation is
-	 * returned.
-	 * @param propertiesToParse contains the properties be converted.
-	 * @return String representation of properties object
+	 * Convert a Properties object to a String. This is only necessary for compatibility
+	 * with converting the String back to a properties object. If an empty properties
+	 * object is passed in, a blank string is returned, otherwise it's string
+	 * representation is returned.
+	 * @param propertiesToParse contains the properties to be converted. Must not be
+	 * {@code null}.
+	 * @return String representation of the properties object
 	 */
-	public static String propertiesToString(Properties propertiesToParse) {
-
-		// If properties is empty, return a blank string.
-		if (propertiesToParse == null || propertiesToParse.size() == 0) {
+	public static String propertiesToString(@NonNull Properties propertiesToParse) {
+		Assert.notNull(propertiesToParse, "propertiesToParse must not be null");
+		if (propertiesToParse.isEmpty()) {
 			return "";
 		}
-
-		StringWriter stringWriter = new StringWriter();
-
-		try {
-			propertiesPersister.store(propertiesToParse, stringWriter, null);
-		}
-		catch (IOException ex) {
-			// Exception is never thrown by StringWriter
-			throw new IllegalStateException("Error while trying to convert properties to string");
-		}
-
-		// If the value is short enough (and doesn't contain commas), convert to
-		// comma-separated...
-		String value = stringWriter.toString();
-		if (value.length() < 160) {
-			List<String> list = Arrays
-					.asList(StringUtils.delimitedListToStringArray(value, LINE_SEPARATOR, LINE_SEPARATOR));
-			String shortValue = StringUtils.collectionToCommaDelimitedString(list.subList(1, list.size()));
-			int count = StringUtils.countOccurrencesOf(shortValue, ",");
-			if (count == list.size() - 2) {
-				value = shortValue;
-			}
-			if (value.endsWith(",")) {
-				value = value.substring(0, value.length() - 1);
-			}
+		List<String> keyValuePairs = new ArrayList<>();
+		for (Map.Entry<Object, Object> entry : propertiesToParse.entrySet()) {
+			keyValuePairs.add(entry.getKey() + "=" + entry.getValue());
 		}
-		return value;
-	}
-
-	private static boolean contains(String str, String searchStr) {
-		return str.indexOf(searchStr) != -1;
+		return String.join(LINE_SEPARATOR, keyValuePairs);
 	}
 
 }

spring-batch-infrastructure/src/test/java/org/springframework/batch/support/PropertiesConverterTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2006-2022 the original author or authors.
+ * Copyright 2006-2023 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.
@@ -16,121 +16,79 @@
 
 package org.springframework.batch.support;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.util.Properties;
 
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+
 import org.springframework.util.StringUtils;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 /**
- * Unit tests for {@link PropertiesConverter}
+ * Unit tests for {@link PropertiesConverter}.
  *
  * @author Robert Kasanicky
+ * @author Mahmoud Ben Hassine
  */
 class PropertiesConverterTests {
 
-	// convenience attributes for storing results of conversions
-	private Properties props = null;
-
-	/**
-	 * Check that Properties can be converted to String and back correctly.
-	 */
 	@Test
-	void testTwoWayRegularConversion() {
-
-		Properties storedProps = new Properties();
-		storedProps.setProperty("key1", "value1");
-		storedProps.setProperty("key2", "value2");
+	void testStringToPropertiesConversion() {
+		String stringToParse = "key1=value1\nkey2=value2";
+		Properties expectedProperties = new Properties();
+		expectedProperties.setProperty("key1", "value1");
+		expectedProperties.setProperty("key2", "value2");
 
-		props = PropertiesConverter.stringToProperties(PropertiesConverter.propertiesToString(storedProps));
+		Properties props = PropertiesConverter.stringToProperties(stringToParse);
 
-		assertEquals(storedProps, props);
+		assertEquals(expectedProperties, props);
 	}
 
-	/**
-	 * Check that Properties can be comma delimited.
-	 */
 	@Test
-	void testRegularConversionWithComma() {
-
-		Properties storedProps = new Properties();
-		storedProps.setProperty("key1", "value1");
-		storedProps.setProperty("key2", "value2");
+	void testPropertiesToStringConversion() {
+		Properties properties = new Properties();
+		properties.setProperty("key1", "value1");
+		properties.setProperty("key2", "value2");
 
-		props = PropertiesConverter.stringToProperties("key1=value1,key2=value2");
+		String value = PropertiesConverter.propertiesToString(properties);
 
-		assertEquals(storedProps, props);
+		assertTrue(value.contains("key1=value1"), "Wrong value: " + value);
+		assertTrue(value.contains("key2=value2"), "Wrong value: " + value);
+		assertEquals(1, StringUtils.countOccurrencesOf(value, "\n"));
 	}
 
-	/**
-	 * Check that Properties can be comma delimited with extra whitespace.
-	 */
 	@Test
-	void testRegularConversionWithCommaAndWhitespace() {
-
+	void testTwoWayRegularConversion() {
 		Properties storedProps = new Properties();
 		storedProps.setProperty("key1", "value1");
 		storedProps.setProperty("key2", "value2");
 
-		props = PropertiesConverter.stringToProperties("key1=value1, key2=value2");
+		Properties props = PropertiesConverter.stringToProperties(PropertiesConverter.propertiesToString(storedProps));
 
 		assertEquals(storedProps, props);
 	}
 
-	/**
-	 * Check that Properties can be comma delimited with extra whitespace.
-	 */
 	@Test
-	void testShortConversionWithCommas() {
-
-		Properties storedProps = new Properties();
-		storedProps.setProperty("key1", "value1");
-		storedProps.setProperty("key2", "value2");
-
-		String value = PropertiesConverter.propertiesToString(storedProps);
-
-		assertTrue(value.contains("key1=value1"), "Wrong value: " + value);
-		assertTrue(value.contains("key2=value2"), "Wrong value: " + value);
-		assertEquals(1, StringUtils.countOccurrencesOf(value, ","));
+	void nullStringShouldNotBeAccepted() {
+		Assertions.assertThrows(IllegalArgumentException.class, () -> PropertiesConverter.stringToProperties(null));
 	}
 
-	/**
-	 * Check that Properties can be newline delimited.
-	 */
 	@Test
-	void testRegularConversionWithCommaAndNewline() {
-
-		Properties storedProps = new Properties();
-		storedProps.setProperty("key1", "value1");
-		storedProps.setProperty("key2", "value2");
-
-		props = PropertiesConverter.stringToProperties("key1=value1\n key2=value2");
-
-		assertEquals(storedProps, props);
+	void emptyStringShouldBeConvertedToEmptyProperties() {
+		Properties properties = PropertiesConverter.stringToProperties("");
+		Assertions.assertTrue(properties.isEmpty());
 	}
 
-	/**
-	 * Null String should be converted to empty Properties
-	 */
 	@Test
-	void testStringToPropertiesNull() {
-		props = PropertiesConverter.stringToProperties(null);
-		assertNotNull(props);
-		assertEquals(0, props.size(), "properties are empty");
+	void nullPropertiesShouldNotBeAccepted() {
+		Assertions.assertThrows(IllegalArgumentException.class, () -> PropertiesConverter.propertiesToString(null));
 	}
 
-	/**
-	 * Null or empty properties should be converted to empty String
-	 */
 	@Test
-	void testPropertiesToStringNull() {
-		String string = PropertiesConverter.propertiesToString(null);
-		assertEquals("", string);
-
-		string = PropertiesConverter.propertiesToString(new Properties());
+	void emptyPropertiesShouldBeConvertedToEmptyString() {
+		String string = PropertiesConverter.propertiesToString(new Properties());
 		assertEquals("", string);
 	}