From ca432309aaff61a5ec555551004778d379f6c3a0 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 11 Jan 2022 11:04:04 +0100 Subject: [PATCH] Polish "Add option to allow Spring Batch custom isolation levels" See gh-28859 --- .../batch/BasicBatchConfigurer.java | 6 +- .../autoconfigure/batch/BatchProperties.java | 60 ++++++++++++--- .../batch/JpaBatchConfigurer.java | 20 ++--- .../batch/BatchAutoConfigurationTests.java | 40 +++++++--- ...BatchAutoConfigurationWithoutJpaTests.java | 26 ++++--- .../batch/JobRepositoryTestingSupport.java | 76 ------------------- 6 files changed, 107 insertions(+), 121 deletions(-) delete mode 100644 spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/JobRepositoryTestingSupport.java diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BasicBatchConfigurer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BasicBatchConfigurer.java index 7d4aca944bf..dac961ecc25 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BasicBatchConfigurer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BasicBatchConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2022 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. @@ -26,6 +26,7 @@ import org.springframework.batch.core.launch.support.SimpleJobLauncher; import org.springframework.batch.core.repository.JobRepository; import org.springframework.batch.core.repository.support.JobRepositoryFactoryBean; import org.springframework.beans.factory.InitializingBean; +import org.springframework.boot.autoconfigure.batch.BatchProperties.Isolation; import org.springframework.boot.autoconfigure.transaction.TransactionManagerCustomizers; import org.springframework.boot.context.properties.PropertyMapper; import org.springframework.jdbc.datasource.DataSourceTransactionManager; @@ -139,7 +140,8 @@ public class BasicBatchConfigurer implements BatchConfigurer, InitializingBean { * @return the isolation level or {@code null} to use the default */ protected String determineIsolationLevel() { - return this.properties.getJdbc().getIsolationLevelForCreate(); + Isolation isolation = this.properties.getJdbc().getIsolationLevelForCreate(); + return (isolation != null) ? isolation.toIsolationName() : null; } protected PlatformTransactionManager createTransactionManager() { diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BatchProperties.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BatchProperties.java index a5e429daa36..efe9a140b2c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BatchProperties.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/BatchProperties.java @@ -66,6 +66,12 @@ public class BatchProperties { private static final String DEFAULT_SCHEMA_LOCATION = "classpath:org/springframework/" + "batch/core/schema-@@platform@@.sql"; + /** + * Transaction isolation level to use when creating job meta-data for new jobs. + * Auto-detected based on whether JPA is being used or not. + */ + private Isolation isolationLevelForCreate; + /** * Path to the SQL file to use to initialize the database schema. */ @@ -87,10 +93,13 @@ public class BatchProperties { */ private DatabaseInitializationMode initializeSchema = DatabaseInitializationMode.EMBEDDED; - /** - * Transaction isolation level to use when creating job meta-data for new jobs. - */ - private String isolationLevelForCreate; + public Isolation getIsolationLevelForCreate() { + return this.isolationLevelForCreate; + } + + public void setIsolationLevelForCreate(Isolation isolationLevelForCreate) { + this.isolationLevelForCreate = isolationLevelForCreate; + } public String getSchema() { return this.schema; @@ -124,12 +133,45 @@ public class BatchProperties { this.initializeSchema = initializeSchema; } - public String getIsolationLevelForCreate() { - return this.isolationLevelForCreate; - } + } - public void setIsolationLevelForCreate(String isolationLevelForCreate) { - this.isolationLevelForCreate = isolationLevelForCreate; + /** + * Available transaction isolation levels. + */ + public enum Isolation { + + /** + * Use the default isolation level of the underlying datastore. + */ + DEFAULT, + + /** + * Indicates that dirty reads, non-repeatable reads and phantom reads can occur. + */ + READ_UNCOMMITTED, + + /** + * Indicates that dirty reads are prevented; non-repeatable reads and phantom + * reads can occur. + */ + READ_COMMITTED, + + /** + * Indicates that dirty reads and non-repeatable reads are prevented; phantom + * reads can occur. + */ + REPEATABLE_READ, + + /** + * Indicate that dirty reads, non-repeatable reads and phantom reads are + * prevented. + */ + SERIALIZABLE; + + private static final String PREFIX = "ISOLATION_"; + + String toIsolationName() { + return PREFIX + name(); } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JpaBatchConfigurer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JpaBatchConfigurer.java index ebe67f1d2da..1d168debe10 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JpaBatchConfigurer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/batch/JpaBatchConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2022 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. @@ -22,6 +22,7 @@ import javax.sql.DataSource; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.boot.autoconfigure.batch.BatchProperties.Isolation; import org.springframework.boot.autoconfigure.transaction.TransactionManagerCustomizers; import org.springframework.orm.jpa.JpaTransactionManager; import org.springframework.transaction.PlatformTransactionManager; @@ -38,8 +39,6 @@ public class JpaBatchConfigurer extends BasicBatchConfigurer { private final EntityManagerFactory entityManagerFactory; - private final String isolationLevelForCreate; - /** * Create a new {@link BasicBatchConfigurer} instance. * @param properties the batch properties @@ -52,18 +51,19 @@ public class JpaBatchConfigurer extends BasicBatchConfigurer { TransactionManagerCustomizers transactionManagerCustomizers, EntityManagerFactory entityManagerFactory) { super(properties, dataSource, transactionManagerCustomizers); this.entityManagerFactory = entityManagerFactory; - this.isolationLevelForCreate = properties.getJdbc().getIsolationLevelForCreate(); } @Override protected String determineIsolationLevel() { - if (this.isolationLevelForCreate == null) { - logger.warn( - "JPA does not support custom isolation levels, so locks may not be taken when launching Jobs. Define spring.batch.jdbc.isolation-level-for-create property to force a custom isolation level."); - return "ISOLATION_DEFAULT"; + String name = super.determineIsolationLevel(); + if (name != null) { + return name; + } + else { + logger.warn("JPA does not support custom isolation levels, so locks may not be taken when launching Jobs. " + + "To silence this warning, set 'spring.batch.jdbc.isolation-level-for-create' to 'default'."); + return Isolation.DEFAULT.toIsolationName(); } - - return this.isolationLevelForCreate; } @Override 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 bf6e2f67e21..502df3940aa 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 @@ -23,6 +23,7 @@ import javax.persistence.EntityManagerFactory; import javax.sql.DataSource; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.batch.core.BatchStatus; import org.springframework.batch.core.Job; @@ -57,6 +58,8 @@ import org.springframework.boot.jdbc.init.DataSourceScriptDatabaseInitializer; import org.springframework.boot.sql.init.DatabaseInitializationMode; import org.springframework.boot.sql.init.DatabaseInitializationSettings; import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.boot.test.system.CapturedOutput; +import org.springframework.boot.test.system.OutputCaptureExtension; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Primary; @@ -78,6 +81,7 @@ import static org.mockito.Mockito.mock; * @author Vedran Pavic * @author Kazuki Shimizu */ +@ExtendWith(OutputCaptureExtension.class) class BatchAutoConfigurationTests { private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() @@ -208,8 +212,30 @@ class BatchAutoConfigurationTests { // level) assertThat(context.getBean(JobRepository.class).getLastJobExecution("job", new JobParameters())) .isNull(); - assertThat(context.getBean(JobRepository.class)) - .satisfies(JobRepositoryTestingSupport.isolationLevelRequirements("ISOLATION_DEFAULT")); + }); + } + + @Test + void testDefaultIsolationLevelWithJpaLogsWarning(CapturedOutput output) { + this.contextRunner.withUserConfiguration(TestConfiguration.class, EmbeddedDataSourceConfiguration.class, + HibernateJpaAutoConfiguration.class).run((context) -> { + assertThat(context.getBean(BasicBatchConfigurer.class).determineIsolationLevel()) + .isEqualTo("ISOLATION_DEFAULT"); + assertThat(output).contains("JPA does not support custom isolation levels") + .contains("set 'spring.batch.jdbc.isolation-level-for-create' to 'default'"); + }); + } + + @Test + void testCustomIsolationLevelWithJpaDoesNotLogWarning(CapturedOutput output) { + this.contextRunner.withPropertyValues("spring.batch.jdbc.isolation-level-for-create=default") + .withUserConfiguration(TestConfiguration.class, EmbeddedDataSourceConfiguration.class, + HibernateJpaAutoConfiguration.class) + .run((context) -> { + assertThat(context.getBean(BasicBatchConfigurer.class).determineIsolationLevel()) + .isEqualTo("ISOLATION_DEFAULT"); + assertThat(output).doesNotContain("JPA does not support custom isolation levels") + .doesNotContain("set 'spring.batch.jdbc.isolation-level-for-create' to 'default'"); }); } @@ -234,16 +260,6 @@ class BatchAutoConfigurationTests { }); } - @Test - void testCustomIsolationLevelForCreate() { - this.contextRunner - .withUserConfiguration(TestConfiguration.class, EmbeddedDataSourceConfiguration.class, - HibernateJpaAutoConfiguration.class) - .withPropertyValues("spring.batch.jdbc.isolation-level-for-create:ISOLATION_READ_COMMITTED") - .run((context) -> assertThat(context.getBean(JobRepository.class)) - .satisfies(JobRepositoryTestingSupport.isolationLevelRequirements("ISOLATION_READ_COMMITTED"))); - } - @Test void testCustomizeJpaTransactionManagerUsingProperties() { this.contextRunner diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationWithoutJpaTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationWithoutJpaTests.java index 06ac163c355..98b07f757ed 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationWithoutJpaTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/BatchAutoConfigurationWithoutJpaTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2022 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. @@ -27,7 +27,6 @@ import org.springframework.batch.core.launch.JobLauncher; import org.springframework.batch.core.repository.JobRepository; import org.springframework.boot.autoconfigure.AutoConfigurations; import org.springframework.boot.autoconfigure.TestAutoConfigurationPackage; -import org.springframework.boot.autoconfigure.batch.BatchProperties.Jdbc; import org.springframework.boot.autoconfigure.jdbc.EmbeddedDataSourceConfiguration; import org.springframework.boot.autoconfigure.orm.jpa.test.City; import org.springframework.boot.autoconfigure.transaction.TransactionAutoConfiguration; @@ -38,7 +37,6 @@ import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.transaction.PlatformTransactionManager; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.from; /** * Tests for {@link BatchAutoConfiguration} when JPA is not on the classpath. @@ -61,24 +59,19 @@ class BatchAutoConfigurationWithoutJpaTests { assertThat(context).hasSingleBean(PlatformTransactionManager.class); assertThat(context.getBean(PlatformTransactionManager.class).toString()) .contains("DataSourceTransactionManager"); - assertThat(context.getBean(BatchProperties.class).getJdbc()) - .returns("classpath:org/springframework/batch/core/schema-@@platform@@.sql", - from(Jdbc::getSchema)) - .returns(DatabaseInitializationMode.EMBEDDED, from(Jdbc::getInitializeSchema)) - .returns(null, from(Jdbc::getIsolationLevelForCreate)); + assertThat(context.getBean(BatchProperties.class).getJdbc().getInitializeSchema()) + .isEqualTo(DatabaseInitializationMode.EMBEDDED); + assertThat(context.getBean(BasicBatchConfigurer.class).determineIsolationLevel()).isNull(); assertThat(new JdbcTemplate(context.getBean(DataSource.class)) .queryForList("select * from BATCH_JOB_EXECUTION")).isEmpty(); assertThat(context.getBean(JobExplorer.class).findRunningJobExecutions("test")).isEmpty(); assertThat(context.getBean(JobRepository.class).getLastJobExecution("test", new JobParameters())) .isNull(); - - assertThat(context.getBean(JobRepository.class)).satisfies( - JobRepositoryTestingSupport.isolationLevelRequirements("ISOLATION_SERIALIZABLE")); }); } @Test - void jdbcWithCustomSettings() { + void jdbcWithCustomPrefix() { this.contextRunner.withUserConfiguration(DefaultConfiguration.class, EmbeddedDataSourceConfiguration.class) .withPropertyValues("spring.datasource.generate-unique-name=true", "spring.batch.jdbc.schema:classpath:batch/custom-schema-hsql.sql", @@ -92,6 +85,15 @@ class BatchAutoConfigurationWithoutJpaTests { }); } + @Test + void jdbcWithCustomIsolationLevel() { + this.contextRunner.withUserConfiguration(DefaultConfiguration.class, EmbeddedDataSourceConfiguration.class) + .withPropertyValues("spring.datasource.generate-unique-name=true", + "spring.batch.jdbc.isolation-level-for-create=read_committed") + .run((context) -> assertThat(context.getBean(BasicBatchConfigurer.class).determineIsolationLevel()) + .isEqualTo("ISOLATION_READ_COMMITTED")); + } + @EnableBatchProcessing @TestAutoConfigurationPackage(City.class) static class DefaultConfiguration { diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/JobRepositoryTestingSupport.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/JobRepositoryTestingSupport.java deleted file mode 100644 index cc14ca8194d..00000000000 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/batch/JobRepositoryTestingSupport.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright 2012-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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.boot.autoconfigure.batch; - -import java.util.Arrays; -import java.util.function.Consumer; -import java.util.stream.Stream; -import java.util.stream.Stream.Builder; - -import org.aopalliance.aop.Advice; -import org.assertj.core.api.InstanceOfAssertFactories; - -import org.springframework.aop.Advisor; -import org.springframework.aop.framework.Advised; -import org.springframework.aop.support.AopUtils; -import org.springframework.batch.core.repository.JobRepository; -import org.springframework.transaction.interceptor.TransactionAspectSupport; - -import static org.assertj.core.api.Assertions.as; -import static org.assertj.core.api.Assertions.assertThat; - -final class JobRepositoryTestingSupport { - - private JobRepositoryTestingSupport() { - - } - - static Consumer isolationLevelRequirements(String isolationLevel) { - return (jobRepository) -> - // jobRepository is proxied twice, the inner proxy has the transaction advice. - // This logic does not assume anything about proxy hierarchy, but it does about - // the advice itself. - assertThat(getTransactionAdvices(jobRepository)) - .anySatisfy((advice) -> assertThat(advice).extracting("transactionAttributeSource") - .extracting(Object::toString, as(InstanceOfAssertFactories.STRING)) - .contains("create*=PROPAGATION_REQUIRES_NEW," + isolationLevel) - .contains("getLastJobExecution*=PROPAGATION_REQUIRES_NEW," + isolationLevel)); - } - - private static Stream getTransactionAdvices(Object candidate) { - Builder builder = Stream.builder(); - getTransactionAdvices(candidate, builder); - return builder.build(); - } - - private static void getTransactionAdvices(Object candidate, Builder builder) { - try { - if (AopUtils.isAopProxy(candidate) && candidate instanceof Advised) { - Arrays.stream(((Advised) candidate).getAdvisors()).map(Advisor::getAdvice) - .filter(TransactionAspectSupport.class::isInstance).forEach(builder::add); - Object target = ((Advised) candidate).getTargetSource().getTarget(); - if (target != null) { - getTransactionAdvices(target, builder); - } - } - } - catch (Exception ex) { - throw new IllegalStateException("Failed to unwrap proxied object", ex); - } - } - -}