From 497438a318299cebe2500e6be2616c14fdfdcc88 Mon Sep 17 00:00:00 2001 From: Mahmoud Ben Hassine Date: Tue, 18 May 2021 15:21:23 +0200 Subject: [PATCH] Add assertions to reject null values in JobParameter Resolves #3913 --- .../batch/core/JobParameter.java | 76 ++++++++----------- .../batch/core/JobParametersBuilder.java | 36 ++++----- .../core/DefaultJobKeyGeneratorTests.java | 13 +--- .../batch/core/JobParameterTests.java | 29 +------ .../batch/core/JobParametersBuilderTests.java | 14 ---- .../batch/core/JobParametersTests.java | 28 ------- .../DefaultJobParametersConverterTests.java | 17 ----- .../batch/core/job/SimpleJobTests.java | 23 ------ .../dao/AbstractJobInstanceDaoTests.java | 20 +---- 9 files changed, 55 insertions(+), 201 deletions(-) diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/JobParameter.java b/spring-batch-core/src/main/java/org/springframework/batch/core/JobParameter.java index 199e64c8bd..5d89f2904e 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/JobParameter.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/JobParameter.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2013 the original author or authors. + * Copyright 2006-2021 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. @@ -19,6 +19,9 @@ import java.io.Serializable; import java.util.Date; +import org.springframework.lang.NonNull; +import org.springframework.util.Assert; + /** * Domain representation of a parameter to a batch job. Only the following types * can be parameters: String, Long, Date, and Double. The identifying flag is @@ -28,10 +31,10 @@ * @author Lucas Ward * @author Dave Syer * @author Michael Minella + * @author Mahmoud Ben Hassine * @since 2.0 * */ -@SuppressWarnings("serial") public class JobParameter implements Serializable { private final Object parameter; @@ -42,61 +45,57 @@ public class JobParameter implements Serializable { /** * Construct a new JobParameter as a String. - * @param parameter {@link String} instance. + * @param parameter {@link String} instance. Must not be {@code null}. * @param identifying true if JobParameter should be identifying. */ - public JobParameter(String parameter, boolean identifying) { - this.parameter = parameter; - parameterType = ParameterType.STRING; - this.identifying = identifying; + public JobParameter(@NonNull String parameter, boolean identifying) { + this(parameter, identifying, ParameterType.STRING); } /** * Construct a new JobParameter as a Long. * - * @param parameter {@link Long} instance. + * @param parameter {@link Long} instance. Must not be {@code null}. * @param identifying true if JobParameter should be identifying. */ - public JobParameter(Long parameter, boolean identifying) { - this.parameter = parameter; - parameterType = ParameterType.LONG; - this.identifying = identifying; + public JobParameter(@NonNull Long parameter, boolean identifying) { + this(parameter, identifying, ParameterType.LONG); } /** * Construct a new JobParameter as a Date. * - * @param parameter {@link Date} instance. + * @param parameter {@link Date} instance. Must not be {@code null}. * @param identifying true if JobParameter should be identifying. */ - public JobParameter(Date parameter, boolean identifying) { - this.parameter = parameter; - parameterType = ParameterType.DATE; - this.identifying = identifying; + public JobParameter(@NonNull Date parameter, boolean identifying) { + this(parameter, identifying, ParameterType.DATE); } /** * Construct a new JobParameter as a Double. * - * @param parameter {@link Double} instance. + * @param parameter {@link Double} instance. Must not be {@code null}. * @param identifying true if JobParameter should be identifying. */ - public JobParameter(Double parameter, boolean identifying) { + public JobParameter(@NonNull Double parameter, boolean identifying) { + this(parameter, identifying, ParameterType.DOUBLE); + } + + private JobParameter(Object parameter, boolean identifying, ParameterType parameterType) { + Assert.notNull(parameter, "parameter must not be null"); this.parameter = parameter; - parameterType = ParameterType.DOUBLE; + this.parameterType = parameterType; this.identifying = identifying; } - /** * Construct a new JobParameter as a String. * * @param parameter {@link String} instance. */ public JobParameter(String parameter) { - this.parameter = parameter; - parameterType = ParameterType.STRING; - this.identifying = true; + this(parameter, true); } /** @@ -105,9 +104,7 @@ public JobParameter(String parameter) { * @param parameter {@link Long} instance. */ public JobParameter(Long parameter) { - this.parameter = parameter; - parameterType = ParameterType.LONG; - this.identifying = true; + this(parameter, true); } /** @@ -116,9 +113,7 @@ public JobParameter(Long parameter) { * @param parameter {@link Date} instance. */ public JobParameter(Date parameter) { - this.parameter = parameter; - parameterType = ParameterType.DATE; - this.identifying = true; + this(parameter, true); } /** @@ -127,9 +122,7 @@ public JobParameter(Date parameter) { * @param parameter {@link Double} instance. */ public JobParameter(Double parameter) { - this.parameter = parameter; - parameterType = ParameterType.DOUBLE; - this.identifying = true; + this(parameter, true); } public boolean isIdentifying() { @@ -140,13 +133,7 @@ public boolean isIdentifying() { * @return the value contained within this JobParameter. */ public Object getValue() { - - if (parameter != null && parameter.getClass().isInstance(Date.class)) { - return new Date(((Date) parameter).getTime()); - } - else { - return parameter; - } + return parameter; } /** @@ -158,7 +145,7 @@ public ParameterType getType() { @Override public boolean equals(Object obj) { - if (obj instanceof JobParameter == false) { + if (!(obj instanceof JobParameter)) { return false; } @@ -167,18 +154,17 @@ public boolean equals(Object obj) { } JobParameter rhs = (JobParameter) obj; - return parameter==null ? rhs.parameter==null && parameterType==rhs.parameterType: parameter.equals(rhs.parameter); + return parameterType == rhs.parameterType && parameter.equals(rhs.parameter); } @Override public String toString() { - return parameter == null ? null : (parameterType == ParameterType.DATE ? "" + ((Date) parameter).getTime() - : parameter.toString()); + return parameterType == ParameterType.DATE ? "" + ((Date) parameter).getTime() : parameter.toString(); } @Override public int hashCode() { - return 7 + 21 * (parameter == null ? parameterType.hashCode() : parameter.hashCode()); + return 7 + 21 * parameter.hashCode(); } /** diff --git a/spring-batch-core/src/main/java/org/springframework/batch/core/JobParametersBuilder.java b/spring-batch-core/src/main/java/org/springframework/batch/core/JobParametersBuilder.java index 6b194fb645..298c847571 100644 --- a/spring-batch-core/src/main/java/org/springframework/batch/core/JobParametersBuilder.java +++ b/spring-batch-core/src/main/java/org/springframework/batch/core/JobParametersBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2019 the original author or authors. + * Copyright 2006-2021 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. @@ -23,6 +23,8 @@ import java.util.Properties; import org.springframework.batch.core.explore.JobExplorer; +import org.springframework.lang.NonNull; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** @@ -103,10 +105,10 @@ public JobParametersBuilder(JobParameters jobParameters, JobExplorer jobExplorer * Add a new identifying String parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @return a reference to this object. */ - public JobParametersBuilder addString(String key, String parameter) { + public JobParametersBuilder addString(String key, @NonNull String parameter) { this.parameterMap.put(key, new JobParameter(parameter, true)); return this; } @@ -115,11 +117,11 @@ public JobParametersBuilder addString(String key, String parameter) { * Add a new String parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @param identifying - indicates if the parameter is used as part of identifying a job instance * @return a reference to this object. */ - public JobParametersBuilder addString(String key, String parameter, boolean identifying) { + public JobParametersBuilder addString(String key, @NonNull String parameter, boolean identifying) { this.parameterMap.put(key, new JobParameter(parameter, identifying)); return this; } @@ -128,10 +130,10 @@ public JobParametersBuilder addString(String key, String parameter, boolean iden * Add a new identifying {@link Date} parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @return a reference to this object. */ - public JobParametersBuilder addDate(String key, Date parameter) { + public JobParametersBuilder addDate(String key, @NonNull Date parameter) { this.parameterMap.put(key, new JobParameter(parameter, true)); return this; } @@ -140,11 +142,11 @@ public JobParametersBuilder addDate(String key, Date parameter) { * Add a new {@link Date} parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @param identifying - indicates if the parameter is used as part of identifying a job instance * @return a reference to this object. */ - public JobParametersBuilder addDate(String key, Date parameter, boolean identifying) { + public JobParametersBuilder addDate(String key, @NonNull Date parameter, boolean identifying) { this.parameterMap.put(key, new JobParameter(parameter, identifying)); return this; } @@ -153,10 +155,10 @@ public JobParametersBuilder addDate(String key, Date parameter, boolean identify * Add a new identifying Long parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @return a reference to this object. */ - public JobParametersBuilder addLong(String key, Long parameter) { + public JobParametersBuilder addLong(String key, @NonNull Long parameter) { this.parameterMap.put(key, new JobParameter(parameter, true)); return this; } @@ -165,11 +167,11 @@ public JobParametersBuilder addLong(String key, Long parameter) { * Add a new Long parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @param identifying - indicates if the parameter is used as part of identifying a job instance * @return a reference to this object. */ - public JobParametersBuilder addLong(String key, Long parameter, boolean identifying) { + public JobParametersBuilder addLong(String key, @NonNull Long parameter, boolean identifying) { this.parameterMap.put(key, new JobParameter(parameter, identifying)); return this; } @@ -178,10 +180,10 @@ public JobParametersBuilder addLong(String key, Long parameter, boolean identify * Add a new identifying Double parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @return a reference to this object. */ - public JobParametersBuilder addDouble(String key, Double parameter) { + public JobParametersBuilder addDouble(String key, @NonNull Double parameter) { this.parameterMap.put(key, new JobParameter(parameter, true)); return this; } @@ -190,11 +192,11 @@ public JobParametersBuilder addDouble(String key, Double parameter) { * Add a new Double parameter for the given key. * * @param key - parameter accessor. - * @param parameter - runtime parameter + * @param parameter - runtime parameter. Must not be {@code null}. * @param identifying - indicates if the parameter is used as part of identifying a job instance * @return a reference to this object. */ - public JobParametersBuilder addDouble(String key, Double parameter, boolean identifying) { + public JobParametersBuilder addDouble(String key, @NonNull Double parameter, boolean identifying) { this.parameterMap.put(key, new JobParameter(parameter, identifying)); return this; } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/DefaultJobKeyGeneratorTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/DefaultJobKeyGeneratorTests.java index c76415335f..f01b42543c 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/DefaultJobKeyGeneratorTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/DefaultJobKeyGeneratorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2013 the original author or authors. + * Copyright 2013-2021 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. @@ -54,17 +54,6 @@ public void testCreateJobKey() { assertEquals(32, key.length()); } - @Test - public void testCreateJobKeyWithNullParameter() { - JobParameters jobParameters1 = new JobParametersBuilder().addString( - "foo", "bar").addString("bar", null).toJobParameters(); - JobParameters jobParameters2 = new JobParametersBuilder().addString( - "foo", "bar").addString("bar", "").toJobParameters(); - String key1 = jobKeyGenerator.generateKey(jobParameters1); - String key2 = jobKeyGenerator.generateKey(jobParameters2); - assertEquals(key1, key2); - } - @Test public void testCreateJobKeyOrdering() { JobParameters jobParameters1 = new JobParametersBuilder().addString( diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParameterTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParameterTests.java index a3bab89508..2fdf5bbefe 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParameterTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParameterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2013 the original author or authors. + * Copyright 2008-2021 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. @@ -37,10 +37,9 @@ public void testStringParameter(){ assertEquals("test", jobParameter.getValue()); } - @Test + @Test(expected = IllegalArgumentException.class) public void testNullStringParameter(){ jobParameter = new JobParameter((String)null, true); - assertEquals(null, jobParameter.getValue()); } @Test @@ -62,10 +61,9 @@ public void testDateParameter(){ assertEquals(new Date(0L), jobParameter.getValue()); } - @Test + @Test(expected = IllegalArgumentException.class) public void testNullDateParameter(){ jobParameter = new JobParameter((Date)null, true); - assertEquals(null, jobParameter.getValue()); } @Test @@ -89,25 +87,4 @@ public void testHashcode(){ assertEquals(testParameter.hashCode(), jobParameter.hashCode()); } - @Test - public void testEqualsWithNull(){ - jobParameter = new JobParameter((String)null, true); - JobParameter testParameter = new JobParameter((String)null, true); - assertTrue(jobParameter.equals(testParameter)); - } - - @Test - public void testEqualsWithNullAndDifferentType(){ - jobParameter = new JobParameter((String)null, true); - JobParameter testParameter = new JobParameter((Date)null, true); - assertFalse(jobParameter.equals(testParameter)); - } - - @Test - public void testHashcodeWithNull(){ - jobParameter = new JobParameter((String)null, true); - JobParameter testParameter = new JobParameter((String)null, true); - assertEquals(testParameter.hashCode(), jobParameter.hashCode()); - } - } \ No newline at end of file diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersBuilderTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersBuilderTests.java index f26d603669..221f56aabe 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersBuilderTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersBuilderTests.java @@ -120,20 +120,6 @@ public void testToJobRuntimeParameters(){ assertEquals("string value", parameters.getString("STRING")); } - @Test - public void testNullRuntimeParameters(){ - this.parametersBuilder.addDate("SCHEDULE_DATE", null); - this.parametersBuilder.addLong("LONG", null); - this.parametersBuilder.addString("STRING", null); - this.parametersBuilder.addDouble("DOUBLE", null); - - JobParameters parameters = this.parametersBuilder.toJobParameters(); - assertNull(parameters.getDate("SCHEDULE_DATE")); - assertNull(parameters.getLong("LONG")); - assertNull(parameters.getString("STRING")); - assertNull(parameters.getLong("DOUBLE")); - } - @Test public void testCopy(){ this.parametersBuilder.addString("STRING", "string value"); diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersTests.java index 335cb42eaa..12196bf302 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/JobParametersTests.java @@ -74,12 +74,6 @@ public void testGetString() { assertEquals("value2", parameters.getString("string.key2")); } - @Test - public void testGetNullString() { - parameters = new JobParameters(Collections.singletonMap("string.key1", new JobParameter((String) null, true))); - assertNull(parameters.getDate("string.key1")); - } - @Test public void testGetLong() { assertEquals(1L, parameters.getLong("long.key1").longValue()); @@ -98,18 +92,6 @@ public void testGetDate() { assertEquals(date2, parameters.getDate("date.key2")); } - @Test - public void testGetNullDate() { - parameters = new JobParameters(Collections.singletonMap("date.key1", new JobParameter((Date)null, true))); - assertNull(parameters.getDate("date.key1")); - } - - @Test - public void testGetEmptyLong() { - parameters = new JobParameters(Collections.singletonMap("long1", new JobParameter((Long)null, true))); - assertNull(parameters.getLong("long1")); - } - @Test public void testGetMissingLong() { assertNull(parameters.getLong("missing.long1")); @@ -231,14 +213,4 @@ public void testDateReturnsNullWhenKeyDoesntExit(){ assertNull(new JobParameters().getDate("keythatdoesntexist")); } - @Test - public void testToPropertiesWithNullValue() { - Map parameterMap = new HashMap<>(); - Long value = null; - parameterMap.put("nullkey", new JobParameter(value)); - JobParameters jobParameters = new JobParameters(parameterMap); - - Properties properties = jobParameters.toProperties(); - assertEquals("", properties.get("nullkey")); - } } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/converter/DefaultJobParametersConverterTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/converter/DefaultJobParametersConverterTests.java index 3c34fc770f..c6c996593b 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/converter/DefaultJobParametersConverterTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/converter/DefaultJobParametersConverterTests.java @@ -331,21 +331,4 @@ public void testNullArgs() { private boolean contains(String str, String searchStr) { return str.indexOf(searchStr) != -1; } - - @Test - public void testGetPropertiesWithNullValues() throws Exception { - - JobParameters parameters = new JobParametersBuilder().addDate("schedule.date", null) - .addString("job.key", null).addLong("vendor.id", null).addDouble("double.key", null) - .toJobParameters(); - - Properties props = factory.getProperties(parameters); - assertNotNull(props); - - final String NOT_FOUND = "NOT FOUND"; - assertEquals(NOT_FOUND, props.getProperty("schedule.date", NOT_FOUND)); - assertEquals(NOT_FOUND, props.getProperty("job.key", NOT_FOUND)); - assertEquals(NOT_FOUND, props.getProperty("vendor.id", NOT_FOUND)); - assertEquals(NOT_FOUND, props.getProperty("double.key", NOT_FOUND)); - } } diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/job/SimpleJobTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/job/SimpleJobTests.java index 6e1224bc58..fc9eb5a51b 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/job/SimpleJobTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/job/SimpleJobTests.java @@ -411,29 +411,6 @@ public void testRestart() throws Exception { assertFalse(step2.passedInStepContext.isEmpty()); } - @Test - public void testRestartWithNullParameter() throws Exception { - - JobParameters jobParameters = new JobParametersBuilder().addString("foo", null).toJobParameters(); - jobExecution = jobRepository.createJobExecution(job.getName(), jobParameters); - jobInstance = jobExecution.getJobInstance(); - - step1.setAllowStartIfComplete(true); - final RuntimeException exception = new RuntimeException("Foo!"); - step2.setProcessException(exception); - - job.execute(jobExecution); - Throwable e = jobExecution.getAllFailureExceptions().get(0); - assertSame(exception, e); - - jobExecution = jobRepository.createJobExecution(job.getName(), jobParameters); - job.execute(jobExecution); - e = jobExecution.getAllFailureExceptions().get(0); - assertSame(exception, e); - assertTrue(step1.passedInStepContext.isEmpty()); - assertFalse(step2.passedInStepContext.isEmpty()); - } - @Test public void testInterruptWithListener() throws Exception { step1.setProcessException(new JobInterruptedException("job interrupted!")); diff --git a/spring-batch-core/src/test/java/org/springframework/batch/core/repository/dao/AbstractJobInstanceDaoTests.java b/spring-batch-core/src/test/java/org/springframework/batch/core/repository/dao/AbstractJobInstanceDaoTests.java index 87b32711b8..4c07590927 100644 --- a/spring-batch-core/src/test/java/org/springframework/batch/core/repository/dao/AbstractJobInstanceDaoTests.java +++ b/spring-batch-core/src/test/java/org/springframework/batch/core/repository/dao/AbstractJobInstanceDaoTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2019 the original author or authors. + * Copyright 2008-2021 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,24 +67,6 @@ public void testCreateAndRetrieve() throws Exception { assertEquals(fooJob, retrievedInstance.getJobName()); } - /* - * Create and retrieve a job instance. - */ - @Transactional - @Test - public void testCreateAndRetrieveWithNullParameter() throws Exception { - - JobParameters jobParameters = new JobParametersBuilder().addString("foo", null).toJobParameters(); - - JobInstance fooInstance = dao.createJobInstance(fooJob, jobParameters); - assertNotNull(fooInstance.getId()); - assertEquals(fooJob, fooInstance.getJobName()); - - JobInstance retrievedInstance = dao.getJobInstance(fooJob, jobParameters); - assertEquals(fooInstance, retrievedInstance); - assertEquals(fooJob, retrievedInstance.getJobName()); - } - /* * Create and retrieve a job instance. */