From c38cd7454238a2fc3563fbe65b2880bbd8724d85 Mon Sep 17 00:00:00 2001 From: Akshay Dubey <38462415+itsAkshayDubey@users.noreply.github.com> Date: Sat, 24 Jun 2023 14:02:29 +0000 Subject: [PATCH 1/2] Fail fast if job name does not exist See gh-36060 --- .../batch/JobLauncherApplicationRunner.java | 31 ++++++++++++++----- .../batch/BatchAutoConfigurationTests.java | 12 +++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java index 76b3e955d20..dd6baf6865c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java @@ -24,6 +24,8 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Properties; +import javax.annotation.PostConstruct; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -66,6 +68,7 @@ import org.springframework.util.StringUtils; * @author Jean-Pierre Bergamin * @author Mahmoud Ben Hassine * @author Stephane Nicoll + * @author Akshay Dubey * @since 2.3.0 */ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, ApplicationEventPublisherAware { @@ -111,6 +114,18 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, this.jobRepository = jobRepository; } + @PostConstruct + public void validate() { + if (StringUtils.hasText(this.jobNames)) { + String[] jobsToRun = this.jobNames.split(","); + for(String jobName: jobsToRun) { + if (!isLocalJob(jobName) && !isRegisteredJob(jobName)) { + throw new IllegalArgumentException("No job instances were found for job name [" + jobName + "]"); + } + } + } + } + public void setOrder(int order) { this.order = order; } @@ -161,6 +176,14 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, executeRegisteredJobs(jobParameters); } + private boolean isLocalJob(String jobName) { + return this.jobs.stream().anyMatch((job) -> job.getName().equals(jobName)); + } + + private boolean isRegisteredJob(String jobName) { + return this.jobRegistry != null && this.jobRegistry.getJobNames().contains(jobName); + } + private void executeLocalJobs(JobParameters jobParameters) throws JobExecutionException { for (Job job : this.jobs) { if (StringUtils.hasText(this.jobNames)) { @@ -178,16 +201,10 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, if (this.jobRegistry != null && StringUtils.hasText(this.jobNames)) { String[] jobsToRun = this.jobNames.split(","); for (String jobName : jobsToRun) { - try { + if(isRegisteredJob(jobName) && !isLocalJob(jobName)) { Job job = this.jobRegistry.getJob(jobName); - if (this.jobs.contains(job)) { - continue; - } execute(job, jobParameters); } - catch (NoSuchJobException ex) { - logger.debug(LogMessage.format("No job found in registry for job name: %s", jobName)); - } } } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java index 6b1062e41b6..a25e7190b5a 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java @@ -80,6 +80,7 @@ import static org.mockito.Mockito.mock; * @author Stephane Nicoll * @author Vedran Pavic * @author Kazuki Shimizu + * @author Akshay Dubey */ @ExtendWith(OutputCaptureExtension.class) class BatchAutoConfigurationTests { @@ -374,6 +375,17 @@ class BatchAutoConfigurationTests { .hasBean("customInitializer")); } + @Test + void testNonConfiguredJobThrowsException() { + this.contextRunner + .withUserConfiguration(NamedJobConfigurationWithLocalJob.class, EmbeddedDataSourceConfiguration.class) + .withPropertyValues("spring.batch.job.names:discreteLocalJob,nonConfiguredJob") + .run((context) -> { + assertThat(context).hasFailed().getFailure().getRootCause() + .hasMessageContaining("nonConfiguredJob"); + }); + } + @Configuration(proxyBeanMethods = false) protected static class BatchDataSourceConfiguration { From 854c162966cd5606f91951e7a42a4093f82302df Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Mon, 24 Jul 2023 15:42:32 +0200 Subject: [PATCH 2/2] Polish "Fail fast if job name does not exist" See gh-36060 --- .../batch/JobLauncherApplicationRunner.java | 28 +++++----- .../batch/BatchAutoConfigurationTests.java | 56 ++++++++++++++++--- 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java index dd6baf6865c..ae28d64a7ae 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JobLauncherApplicationRunner.java @@ -24,8 +24,6 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Properties; -import javax.annotation.PostConstruct; - import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -43,11 +41,11 @@ import org.springframework.batch.core.converter.JobParametersConverter; import org.springframework.batch.core.explore.JobExplorer; 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.JobExecutionAlreadyRunningException; import org.springframework.batch.core.repository.JobInstanceAlreadyCompleteException; import org.springframework.batch.core.repository.JobRepository; import org.springframework.batch.core.repository.JobRestartException; +import org.springframework.beans.factory.InitializingBean; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.ApplicationArguments; import org.springframework.boot.ApplicationRunner; @@ -71,7 +69,8 @@ import org.springframework.util.StringUtils; * @author Akshay Dubey * @since 2.3.0 */ -public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, ApplicationEventPublisherAware { +public class JobLauncherApplicationRunner + implements ApplicationRunner, InitializingBean, Ordered, ApplicationEventPublisherAware { /** * The default order for the command line runner. @@ -114,15 +113,14 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, this.jobRepository = jobRepository; } - @PostConstruct - public void validate() { + @Override + public void afterPropertiesSet() { if (StringUtils.hasText(this.jobNames)) { - String[] jobsToRun = this.jobNames.split(","); - for(String jobName: jobsToRun) { + for (String jobName : jobsToRun()) { if (!isLocalJob(jobName) && !isRegisteredJob(jobName)) { - throw new IllegalArgumentException("No job instances were found for job name [" + jobName + "]"); + throw new IllegalArgumentException("No job found with name '" + jobName + "'"); } - } + } } } @@ -187,7 +185,7 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, private void executeLocalJobs(JobParameters jobParameters) throws JobExecutionException { for (Job job : this.jobs) { if (StringUtils.hasText(this.jobNames)) { - String[] jobsToRun = this.jobNames.split(","); + String[] jobsToRun = jobsToRun(); if (!PatternMatchUtils.simpleMatch(jobsToRun, job.getName())) { logger.debug(LogMessage.format("Skipped job: %s", job.getName())); continue; @@ -199,9 +197,9 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, private void executeRegisteredJobs(JobParameters jobParameters) throws JobExecutionException { if (this.jobRegistry != null && StringUtils.hasText(this.jobNames)) { - String[] jobsToRun = this.jobNames.split(","); + String[] jobsToRun = jobsToRun(); for (String jobName : jobsToRun) { - if(isRegisteredJob(jobName) && !isLocalJob(jobName)) { + if (!isLocalJob(jobName)) { Job job = this.jobRegistry.getJob(jobName); execute(job, jobParameters); } @@ -263,4 +261,8 @@ public class JobLauncherApplicationRunner implements ApplicationRunner, Ordered, return new JobParameters(merged); } + private String[] jobsToRun() { + return this.jobNames.split(","); + } + } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java index a25e7190b5a..6da759a819a 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationTests.java @@ -16,6 +16,7 @@ package org.springframework.boot.autoconfigure.batch; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -71,6 +72,8 @@ import org.springframework.transaction.PlatformTransactionManager; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; /** @@ -80,7 +83,6 @@ import static org.mockito.Mockito.mock; * @author Stephane Nicoll * @author Vedran Pavic * @author Kazuki Shimizu - * @author Akshay Dubey */ @ExtendWith(OutputCaptureExtension.class) class BatchAutoConfigurationTests { @@ -376,14 +378,50 @@ class BatchAutoConfigurationTests { } @Test - void testNonConfiguredJobThrowsException() { - this.contextRunner - .withUserConfiguration(NamedJobConfigurationWithLocalJob.class, EmbeddedDataSourceConfiguration.class) - .withPropertyValues("spring.batch.job.names:discreteLocalJob,nonConfiguredJob") - .run((context) -> { - assertThat(context).hasFailed().getFailure().getRootCause() - .hasMessageContaining("nonConfiguredJob"); - }); + void whenTheUserDefinesAJobNameAsJobInstanceValidates() { + JobLauncherApplicationRunner runner = createInstance("another"); + runner.setJobs(Collections.singletonList(mockJob("test"))); + runner.setJobNames("test"); + runner.afterPropertiesSet(); + } + + @Test + void whenTheUserDefinesAJobNameAsRegisteredJobValidates() { + JobLauncherApplicationRunner runner = createInstance("test"); + runner.setJobNames("test"); + runner.afterPropertiesSet(); + } + + @Test + void whenTheUserDefinesAJobNameThatDoesNotExistWithJobInstancesFailsFast() { + JobLauncherApplicationRunner runner = createInstance(); + runner.setJobs(Arrays.asList(mockJob("one"), mockJob("two"))); + runner.setJobNames("three"); + assertThatIllegalArgumentException().isThrownBy(runner::afterPropertiesSet) + .withMessage("No job found with name 'three'"); + } + + @Test + void whenTheUserDefinesAJobNameThatDoesNotExistWithRegisteredJobFailsFast() { + JobLauncherApplicationRunner runner = createInstance("one", "two"); + runner.setJobNames("three"); + assertThatIllegalArgumentException().isThrownBy(runner::afterPropertiesSet) + .withMessage("No job found with name 'three'"); + } + + private JobLauncherApplicationRunner createInstance(String... registeredJobNames) { + JobLauncherApplicationRunner runner = new JobLauncherApplicationRunner(mock(JobLauncher.class), + mock(JobExplorer.class), mock(JobRepository.class)); + JobRegistry jobRegistry = mock(JobRegistry.class); + given(jobRegistry.getJobNames()).willReturn(Arrays.asList(registeredJobNames)); + runner.setJobRegistry(jobRegistry); + return runner; + } + + private Job mockJob(String name) { + Job job = mock(Job.class); + given(job.getName()).willReturn(name); + return job; } @Configuration(proxyBeanMethods = false)