From 46c8ebbe7e84f93b944a137cd75ec24b419e5da3 Mon Sep 17 00:00:00 2001 From: Mahmoud Ben Hassine Date: Thu, 16 Feb 2023 16:19:04 +0100 Subject: [PATCH] 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 --- .../batch/core/launch/JobOperator.java | 7 +- .../launch/support/SimpleJobOperator.java | 19 +-- .../batch/support/PropertiesConverter.java | 124 ++++++------------ .../support/PropertiesConverterTests.java | 116 ++++++---------- .../batch/test/MetaDataInstanceFactory.java | 4 +- 5 files changed, 85 insertions(+), 185 deletions(-) diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/launch/JobOperator.java b/spring-batch-core/src/main/java/org/springframework/batch/core/launch/JobOperator.java index f2369f04e..8f747458d 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/launch/JobOperator.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/launch/JobOperator.java @@ -91,7 +91,8 @@ public interface JobOperator { Set 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 @@ public interface JobOperator { /** * 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 diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/launch/support/SimpleJobOperator.java b/spring-batch-core/src/main/java/org/springframework/batch/core/launch/support/SimpleJobOperator.java index ca67fa9aa..b73811aa1 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/launch/support/SimpleJobOperator.java +++ b/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.StepLocator; 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 class SimpleJobOperator implements JobOperator, InitializingBean { Properties properties = this.jobParametersConverter.getProperties(jobExecution.getJobParameters()); - List keyValuePairs = new ArrayList<>(); - for (Map.Entry entry : properties.entrySet()) { - keyValuePairs.add(entry.getKey() + "=" + entry.getValue()); - } - return String.join(" ", keyValuePairs); + return PropertiesConverter.propertiesToString(properties); } /* @@ -319,14 +315,7 @@ public class SimpleJobOperator implements JobOperator, InitializingBean { 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)) { diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/PropertiesConverter.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/PropertiesConverter.java index 926b8ec4c..041026717 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/support/PropertiesConverter.java +++ b/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. + 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]); } - catch (IOException ex) { - throw new IllegalStateException( - "Error while trying to parse String to java.util.Properties," + " given String: " + properties); - } - 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); + List keyValuePairs = new ArrayList<>(); + for (Map.Entry entry : propertiesToParse.entrySet()) { + keyValuePairs.add(entry.getKey() + "=" + entry.getValue()); } - 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 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); - } - } - return value; - } - - private static boolean contains(String str, String searchStr) { - return str.indexOf(searchStr) != -1; + return String.join(LINE_SEPARATOR, keyValuePairs); } } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/support/PropertiesConverterTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/support/PropertiesConverterTests.java index 791373c62..43e537c79 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/support/PropertiesConverterTests.java +++ b/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() { + void testStringToPropertiesConversion() { + String stringToParse = "key1=value1\nkey2=value2"; + Properties expectedProperties = new Properties(); + expectedProperties.setProperty("key1", "value1"); + expectedProperties.setProperty("key2", "value2"); - Properties storedProps = new Properties(); - storedProps.setProperty("key1", "value1"); - storedProps.setProperty("key2", "value2"); + Properties props = PropertiesConverter.stringToProperties(stringToParse); - props = PropertiesConverter.stringToProperties(PropertiesConverter.propertiesToString(storedProps)); - - assertEquals(storedProps, props); + assertEquals(expectedProperties, props); } - /** - * Check that Properties can be comma delimited. - */ @Test - void testRegularConversionWithComma() { + void testPropertiesToStringConversion() { + Properties properties = new Properties(); + properties.setProperty("key1", "value1"); + properties.setProperty("key2", "value2"); - Properties storedProps = new Properties(); - storedProps.setProperty("key1", "value1"); - storedProps.setProperty("key2", "value2"); - - props = PropertiesConverter.stringToProperties("key1=value1,key2=value2"); - - assertEquals(storedProps, props); - } - - /** - * Check that Properties can be comma delimited with extra whitespace. - */ - @Test - void testRegularConversionWithCommaAndWhitespace() { - - Properties storedProps = new Properties(); - storedProps.setProperty("key1", "value1"); - storedProps.setProperty("key2", "value2"); - - props = PropertiesConverter.stringToProperties("key1=value1, key2=value2"); - - 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); + String value = PropertiesConverter.propertiesToString(properties); assertTrue(value.contains("key1=value1"), "Wrong value: " + value); assertTrue(value.contains("key2=value2"), "Wrong value: " + value); - assertEquals(1, StringUtils.countOccurrencesOf(value, ",")); + assertEquals(1, StringUtils.countOccurrencesOf(value, "\n")); } - /** - * Check that Properties can be newline delimited. - */ @Test - void testRegularConversionWithCommaAndNewline() { - + void testTwoWayRegularConversion() { Properties storedProps = new Properties(); storedProps.setProperty("key1", "value1"); storedProps.setProperty("key2", "value2"); - props = PropertiesConverter.stringToProperties("key1=value1\n key2=value2"); + Properties props = PropertiesConverter.stringToProperties(PropertiesConverter.propertiesToString(storedProps)); assertEquals(storedProps, props); } - /** - * 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 nullStringShouldNotBeAccepted() { + Assertions.assertThrows(IllegalArgumentException.class, () -> PropertiesConverter.stringToProperties(null)); } - /** - * Null or empty properties should be converted to empty String - */ @Test - void testPropertiesToStringNull() { - String string = PropertiesConverter.propertiesToString(null); - assertEquals("", string); + void emptyStringShouldBeConvertedToEmptyProperties() { + Properties properties = PropertiesConverter.stringToProperties(""); + Assertions.assertTrue(properties.isEmpty()); + } - string = PropertiesConverter.propertiesToString(new Properties()); + @Test + void nullPropertiesShouldNotBeAccepted() { + Assertions.assertThrows(IllegalArgumentException.class, () -> PropertiesConverter.propertiesToString(null)); + } + + @Test + void emptyPropertiesShouldBeConvertedToEmptyString() { + String string = PropertiesConverter.propertiesToString(new Properties()); assertEquals("", string); } diff --git a/spring-batch-test/src/main/java/org/springframework/batch/test/MetaDataInstanceFactory.java b/spring-batch-test/src/main/java/org/springframework/batch/test/MetaDataInstanceFactory.java index 94c2c5e4d..38411be19 100644 --- a/spring-batch-test/src/main/java/org/springframework/batch/test/MetaDataInstanceFactory.java +++ b/spring-batch-test/src/main/java/org/springframework/batch/test/MetaDataInstanceFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2021 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. @@ -112,7 +112,7 @@ public class MetaDataInstanceFactory { * @param jobName the name of the job * @param instanceId the Id of the {@link JobInstance} * @param executionId the id for the {@link JobExecution} - * @param jobParameters comma or new line separated name=value pairs + * @param jobParameters new line separated key=value pairs * @return a {@link JobExecution} */ public static JobExecution createJobExecution(String jobName, Long instanceId, Long executionId,