Simplify configuring transactionManager with Java config

[mjiderhamn]

Simplify subclassing DefaultBatchConfigurer by just overriding getTransactionManager() and getDataSource().

[mjiderhamn]

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

[mminella]

This PR won't work as currently coded. Using the getters in the initialize method in this PR would require them to be final in order for them not to be overridden in a subclass and risk not returning the correct value but would defeat the purpose of having them available in the first place (since you want to override them). What is the use case for overriding getDataSource()?

[mjiderhamn]

Actually, there's no need for me to override the DataSource. The case I have is that I want to specifically set the PlatformTransactionManager (JTA for runtime and mock for tests).

Since transactionManager is private without a setter, and createJobRepository() accesses the transactionManager directly, that means I have to override createJobRepository(). And since I cannot access the private dataSource in my subclass, I had to override that attribute + setter in my subclass too. (All this just to control PlatformTransactionManager)

One option would have been to add a setter for transactionManager, but since setDataSource() writes to it unconditionally, that would mean the order of setters would be important.

This PR was an attempt at being as flexible as possible. If you have an idea of a more correct approach, I'm willing to contribute.

[mjiderhamn]

How about this instead?

KISS - removing the need to subclass for the transactionManager case, but allow access to the dataSource in case you chose to do so. Example use case for still subclassing is to setIsolationLevelForCreate() in an overridden createJobRepository().

[mjiderhamn]

@mminella Any chance to get this into the next version?

[mminella]

@mjiderhamn The plan for Batch 3.1 (which will begin development in earnest in a couple weeks) is to simplify the java config story across the framework. I'll be sure to consider this use case in some capacity then. FYI, it's targeted to be released in early December.

[mminella]

This needs some tests to validate the behavior. Otherwise this looks good.

[mminella]

This isn't needed and should be removed.

[fmbenhassine]

KISS - removing the need to subclass for the transactionManager case

If I understand correctly, the idea is that if I provide a transaction manager in my context, it would be used by Spring Batch thanks to autowiring via the new setter added in this PR.

This won't work as coded in this PR when I don't specify a bean of type BatchConfigurer (a DefaultBatchConfigurer or a subtype) in my context. Here is a failing test:

import javax.sql.DataSource;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import org.springframework.aop.Advisor;
import org.springframework.aop.TargetSource;
import org.springframework.aop.framework.Advised;
import org.springframework.batch.core.repository.JobRepository;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
import org.springframework.transaction.PlatformTransactionManager;
import org.springframework.transaction.interceptor.TransactionInterceptor;

@RunWith(MockitoJUnitRunner.class)
public class TransactionManagerConfigTests {

	@Mock
	protected static PlatformTransactionManager transactionManager;

	@Test
	public void testConfigurationWithCustomTransactionManager() throws Exception {
		AnnotationConfigApplicationContext applicationContext = new AnnotationConfigApplicationContext(BatchConfiguration.class);
		PlatformTransactionManager platformTransactionManager = applicationContext.getBean(PlatformTransactionManager.class);
		Assert.assertSame(transactionManager, platformTransactionManager);
		assertThatTransactionManagerIsSetOnJobRepository(applicationContext, transactionManager);
	}

	@EnableBatchProcessing
	public static class BatchConfiguration {

		@Bean
		public DataSource dataSource() {
			return new EmbeddedDatabaseBuilder()
					.addScript("classpath:org/springframework/batch/core/schema-drop-hsqldb.sql")
					.addScript("classpath:org/springframework/batch/core/schema-hsqldb.sql")
					.build();
		}

		@Bean
		public PlatformTransactionManager transactionManager() {
			return transactionManager;
		}
	}

	/*
	 * The transaction manager set on JobRepositoryFactoryBean in DefaultBatchConfigurer.createJobRepository
	 * ends up in the TransactionInterceptor advise applied to the (proxied) JobRepository.
	 * This method extracts the advise from the proxy and checks if the supplied
	 * transaction manager is the one used by the TransactionInterceptor advise.
	 */
	private void assertThatTransactionManagerIsSetOnJobRepository(AnnotationConfigApplicationContext applicationContext, PlatformTransactionManager transactionManager) throws Exception {
		JobRepository jobRepository = applicationContext.getBean(JobRepository.class);
		TargetSource targetSource = ((Advised) jobRepository).getTargetSource(); // proxy created in SimpleBatchConfiguration.createLazyProxy
		Advised target = (Advised) targetSource.getTarget(); // initial proxy created in AbstractJobRepositoryFactoryBean.initializeProxy
		Advisor[] advisors = target.getAdvisors();
		for (Advisor advisor : advisors) {
			if (advisor.getAdvice() instanceof TransactionInterceptor) {
				TransactionInterceptor transactionInterceptor = (TransactionInterceptor) advisor.getAdvice();
				Assert.assertEquals(transactionManager, transactionInterceptor.getTransactionManager());
			}
		}
	}
}

This test fails because it is expected that my custom transaction manager is used in the job repository, but a DataSourceTransactionManager is used instead. When I don't specify a BatchConfigurer, the DefaultBatchConfigurer is used but it is not declared as a bean, and hence the transaction manager is not autowired (we programmatically create the instance and call initialize). So the condition:

if(this.transactionManager == null) {
   this.transactionManager = new DataSourceTransactionManager(dataSource);
}

is true even if I have defined a transaction manager in my context and which is supposed to be autowired by the new setter.

I think the transaction manager should not be autowired. If the user wants to use a custom transaction manager, he has to provide a custom BatchConfigurer (to be consistent with Spring Cloud Task, see spring-cloud/spring-cloud-task#270). Subclassing the DefaultBatchConfigurer and overriding the getter should be sufficient (like the example here: #620), this approach is tested here: fmbenhassine@b6fa592 where all tests are passing (except the ones that are ignored because we don't want to address them).

[mminella]

Based on the comments @benas made as well as the PR he submitted to address this issue, I'm closing this PR. #634 should address the functionality discussed in this issue. Thank you for the contribution!