spring-projects/spring-batch

Add ability to customize the job parameters converter in the default batch configuration

micheljung opened this issue · 4 comments

micheljung commented

Hi, I think DefaultBatchConfiguration has some issues that need to be addressed:

The documentation suggests that it should be extended

Quote:

A typical usage of this class is as follows:

  @Configuration
  public class MyJobConfiguration extends DefaultBatchConfiguration {
 
      @Bean
      public Job job(JobRepository jobRepository) {
          return new JobBuilder("myJob", jobRepository)
                  // define job flow as needed
                  .build();
      }
 
  }
  1. This suggests to extend DefaultBatchConfiguration. But following Composition over Inheritance, you would rather want to import it.
  2. The only reason to extend DefaultBatchConfiguration should probably be to override e. g. getTaskExecutor(), allowing you to customize the Batch infrastructure. But then again, following Composition over Inheritance, it might be better to let the user provide a factory bean and/or a customizer.
  3. While it already uses factory beans like JobRepositoryFactoryBean, these are hard coded and can neither be changed nor customized by the user.
  4. The code above suggests defining job beans in your subclass of DefaultBatchConfiguration. This mixes two things that, IMO, should not be mixed: Batch infrastructure configuration and job configurations. What if you want to have multiple classes that define jobs, would you then extend DefaultBatchConfiguration multiple times? No, because if you do that, from which class will the beans be taken? Imagine overriding getTaskExecutor() in multiple subclasses: only one will win. So as a user, you most likely want to subclass DefaultBatchConfiguration at most once and put jobs into separate configs. Not everyone might realize this. Therefore, I think the doc shouldn't suggest mixing job config with infrastructure.

It makes auto configuration difficult for Spring Boot

Even if customizable bean factories were provided, it would still not work well with Spring Boot AutoConfiguration where users expect beans to be replaceable. See spring-projects/spring-boot#40040
If the suggested solution works, this point might be invalid. Still, maybe it's something to be considered.

I'm interested in reading your thoughts. When I find the time, I would also like to provide suggestions. Unfortunately, I can't right now.

micheljung commented

Case in point: I need to use JsonJobParametersConverter instead of DefaultJobParametersConverter. This is configured in JobOperatorFactoryBean. But in DefaultBatchConfiguration, the factory is hard coded and it doesn't call setJobParametersConverter. My only option is to override this whole method. I can't even safely call super.jobOperator() because it returns a JobOperator, not a SimpleJobOperator :)

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/configuration/support/DefaultBatchConfiguration.java

Lines 231 to 247 in 425134c

@Bean
public JobOperator jobOperator(JobRepository jobRepository, JobExplorer jobExplorer, JobRegistry jobRegistry,
JobLauncher jobLauncher) throws BatchConfigurationException {
JobOperatorFactoryBean jobOperatorFactoryBean = new JobOperatorFactoryBean();
jobOperatorFactoryBean.setTransactionManager(getTransactionManager());
jobOperatorFactoryBean.setJobRepository(jobRepository);
jobOperatorFactoryBean.setJobExplorer(jobExplorer);
jobOperatorFactoryBean.setJobRegistry(jobRegistry);
jobOperatorFactoryBean.setJobLauncher(jobLauncher);
try {
jobOperatorFactoryBean.afterPropertiesSet();
return jobOperatorFactoryBean.getObject();
}
catch (Exception e) {
throw new BatchConfigurationException("Unable to configure the default job operator", e);
}
}

fmbenhassine commented

Thank you for sharing your thoughts! I wish we had such great feedback when we worked on #3942 (which I recommend you to check if you want to understand the rationale behind the design choices in v5).

  1. This suggests to extend DefaultBatchConfiguration. But following Composition over Inheritance, you would rather want to import it.
  2. The only reason to extend DefaultBatchConfiguration should probably be to override e. g. getTaskExecutor(), allowing you to customize the Batch infrastructure. But then again, following Composition over Inheritance, it might be better to let the user provide a factory bean and/or a customizer.

The idea of defining default infrastructure beans in a base class is not new, it is actually similar to several classes with same purpose across the portfolio (like AbstractDispatcherServletInitializer in Spring MVC and AbstractSecurityWebApplicationInitializer in Spring Security). Composition over Inheritance really applies to business services in application code to make them swappable. In this context, we are actually writing some configuration code that we inherit from a base class and override if needed). That said, you can still @Import the default batch configuration class instead of extending it.

  1. While it already uses factory beans like JobRepositoryFactoryBean, these are hard coded and can neither be changed nor customized by the user.

Factory beans should not be leaked in the API, otherwise this would be a design mistake IMHO. They are nothing more than nice to have components to ease the configuration of infrastructure beans. As a batch user, I should be able to define an infrastructure bean (a JobRepositoryfor instance) and choose to use a factory bean or not.

  1. The code above suggests defining job beans in your subclass of DefaultBatchConfiguration. This mixes two things that, IMO, should not be mixed: Batch infrastructure configuration and job configurations.

That's a fair point. I always recommend to separate infrastructure beans from business beans. A good example is the hello world tutorial, where infrastructure beans are defined in a separate class and imported in the job configuration class. The example you referred to can be refactored as such too, but one could argue that it is ok to inherit infrastructure beans from a base class (which is considered a separate class).

It makes auto configuration difficult for Spring Boot

I understand and agree. This is a known limitation, I will add a comment on spring-projects/spring-boot#40040 to mention that this is currently missing in Spring Boot. Currently, I can define a datasource and it will be picked up and set on the job repository (ie I don't have to redefine the job repository). In the same way, I should be able to define a task executor to use in the job launcher without having to redefine a job launcher.

Hopefully I answered all your questions.

fmbenhassine commented

Case in point: I need to use JsonJobParametersConverter instead of DefaultJobParametersConverter. This is configured in JobOperatorFactoryBean. But in DefaultBatchConfiguration, the factory is hard coded and it doesn't call setJobParametersConverter.

This is a valid issue. Similar to the ExecutionContextSerializer, I should be able to define a bean of type JobParametersConverter and get it configured on top level infrastructure beans where appropriate. I will plan the addition in a minor release.