Stop using Constants utility in DefaultTransactionDefinition

See gh-30851

spring-tx/src/main/java/org/springframework/transaction/support/AbstractPlatformTransactionManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2021 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -473,11 +473,10 @@ public abstract class AbstractPlatformTransactionManager implements PlatformTran
 			if (definition.getIsolationLevel() != TransactionDefinition.ISOLATION_DEFAULT) {
 				Integer currentIsolationLevel = TransactionSynchronizationManager.getCurrentTransactionIsolationLevel();
 				if (currentIsolationLevel == null || currentIsolationLevel != definition.getIsolationLevel()) {
-					Constants isoConstants = DefaultTransactionDefinition.constants;
 					throw new IllegalTransactionStateException("Participating transaction with definition [" +
 							definition + "] specifies isolation level which is incompatible with existing transaction: " +
 							(currentIsolationLevel != null ?
-									isoConstants.toCode(currentIsolationLevel, DefaultTransactionDefinition.PREFIX_ISOLATION) :
+									DefaultTransactionDefinition.getIsolationLevelName(currentIsolationLevel) :
 									"(unknown)"));
 				}
 			}

spring-tx/src/main/java/org/springframework/transaction/support/DefaultTransactionDefinition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -17,10 +17,11 @@
 package org.springframework.transaction.support;
 
 import java.io.Serializable;
+import java.util.Map;
 
-import org.springframework.core.Constants;
 import org.springframework.lang.Nullable;
 import org.springframework.transaction.TransactionDefinition;
+import org.springframework.util.Assert;
 
 /**
  * Default implementation of the {@link TransactionDefinition} interface,
@@ -31,6 +32,7 @@ import org.springframework.transaction.TransactionDefinition;
  * {@link org.springframework.transaction.interceptor.DefaultTransactionAttribute}.
  *
  * @author Juergen Hoeller
+ * @author Sam Brannen
  * @since 08.05.2003
  */
 @SuppressWarnings("serial")
@@ -49,8 +51,31 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	public static final String READ_ONLY_MARKER = "readOnly";
 
 
-	/** Constants instance for TransactionDefinition. */
+	/**
-	static final Constants constants = new Constants(TransactionDefinition.class);
+	 * Map of constant names to constant values for the propagation constants
+	 * defined in {@link TransactionDefinition}.
+	 */
+	static final Map<String, Integer> propagationConstants = Map.of(
+			"PROPAGATION_REQUIRED", TransactionDefinition.PROPAGATION_REQUIRED,
+			"PROPAGATION_SUPPORTS", TransactionDefinition.PROPAGATION_SUPPORTS,
+			"PROPAGATION_MANDATORY", TransactionDefinition.PROPAGATION_MANDATORY,
+			"PROPAGATION_REQUIRES_NEW", TransactionDefinition.PROPAGATION_REQUIRES_NEW,
+			"PROPAGATION_NOT_SUPPORTED", TransactionDefinition.PROPAGATION_NOT_SUPPORTED,
+			"PROPAGATION_NEVER", TransactionDefinition.PROPAGATION_NEVER,
+			"PROPAGATION_NESTED", TransactionDefinition.PROPAGATION_NESTED
+		);
+
+	/**
+	 * Map of constant names to constant values for the isolation constants
+	 * defined in {@link TransactionDefinition}.
+	 */
+	static final Map<String, Integer> isolationConstants = Map.of(
+			"ISOLATION_DEFAULT", TransactionDefinition.ISOLATION_DEFAULT,
+			"ISOLATION_READ_UNCOMMITTED", TransactionDefinition.ISOLATION_READ_UNCOMMITTED,
+			"ISOLATION_READ_COMMITTED", TransactionDefinition.ISOLATION_READ_COMMITTED,
+			"ISOLATION_REPEATABLE_READ", TransactionDefinition.ISOLATION_REPEATABLE_READ,
+			"ISOLATION_SERIALIZABLE", TransactionDefinition.ISOLATION_SERIALIZABLE
+		);
 
 	private int propagationBehavior = PROPAGATION_REQUIRED;
 
@@ -108,7 +133,7 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 
 	/**
 	 * Set the propagation behavior by the name of the corresponding constant in
-	 * TransactionDefinition, e.g. "PROPAGATION_REQUIRED".
+	 * {@link TransactionDefinition} &mdash; for example, {@code "PROPAGATION_REQUIRED"}.
 	 * @param constantName name of the constant
 	 * @throws IllegalArgumentException if the supplied value is not resolvable
 	 * to one of the {@code PROPAGATION_} constants or is {@code null}
@@ -116,10 +141,10 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	 * @see #PROPAGATION_REQUIRED
 	 */
 	public final void setPropagationBehaviorName(String constantName) throws IllegalArgumentException {
-		if (!constantName.startsWith(PREFIX_PROPAGATION)) {
+		Assert.hasText(constantName, "'constantName' must not be null or blank");
-			throw new IllegalArgumentException("Only propagation constants allowed");
+		Integer propagationBehavior = propagationConstants.get(constantName);
-		}
+		Assert.notNull(propagationBehavior, "Only propagation behavior constants allowed");
-		setPropagationBehavior(constants.asNumber(constantName).intValue());
+		this.propagationBehavior = propagationBehavior;
 	}
 
 	/**
@@ -138,9 +163,8 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	 * @see #PROPAGATION_REQUIRED
 	 */
 	public final void setPropagationBehavior(int propagationBehavior) {
-		if (!constants.getValues(PREFIX_PROPAGATION).contains(propagationBehavior)) {
+		Assert.isTrue(propagationConstants.containsValue(propagationBehavior),
-			throw new IllegalArgumentException("Only values of propagation constants allowed");
+				"Only values of propagation constants allowed");
-		}
 		this.propagationBehavior = propagationBehavior;
 	}
 
@@ -151,7 +175,7 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 
 	/**
 	 * Set the isolation level by the name of the corresponding constant in
-	 * TransactionDefinition, e.g. "ISOLATION_DEFAULT".
+	 * {@link TransactionDefinition} &mdash; for example, {@code "ISOLATION_DEFAULT"}.
 	 * @param constantName name of the constant
 	 * @throws IllegalArgumentException if the supplied value is not resolvable
 	 * to one of the {@code ISOLATION_} constants or is {@code null}
@@ -159,10 +183,10 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	 * @see #ISOLATION_DEFAULT
 	 */
 	public final void setIsolationLevelName(String constantName) throws IllegalArgumentException {
-		if (!constantName.startsWith(PREFIX_ISOLATION)) {
+		Assert.hasText(constantName, "'constantName' must not be null or blank");
-			throw new IllegalArgumentException("Only isolation constants allowed");
+		Integer isolationLevel = isolationConstants.get(constantName);
-		}
+		Assert.notNull(isolationLevel, "Only isolation constants allowed");
-		setIsolationLevel(constants.asNumber(constantName).intValue());
+		this.isolationLevel = isolationLevel;
 	}
 
 	/**
@@ -181,9 +205,8 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	 * @see #ISOLATION_DEFAULT
 	 */
 	public final void setIsolationLevel(int isolationLevel) {
-		if (!constants.getValues(PREFIX_ISOLATION).contains(isolationLevel)) {
+		Assert.isTrue(isolationConstants.containsValue(isolationLevel),
-			throw new IllegalArgumentException("Only values of isolation constants allowed");
+				"Only values of isolation constants allowed");
-		}
 		this.isolationLevel = isolationLevel;
 	}
 
@@ -294,9 +317,9 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 	 */
 	protected final StringBuilder getDefinitionDescription() {
 		StringBuilder result = new StringBuilder();
-		result.append(constants.toCode(this.propagationBehavior, PREFIX_PROPAGATION));
+		result.append(getPropagationBehaviorName(this.propagationBehavior));
 		result.append(',');
-		result.append(constants.toCode(this.isolationLevel, PREFIX_ISOLATION));
+		result.append(getIsolationLevelName(this.isolationLevel));
 		if (this.timeout != TIMEOUT_DEFAULT) {
 			result.append(',');
 			result.append(PREFIX_TIMEOUT).append(this.timeout);
@@ -308,4 +331,22 @@ public class DefaultTransactionDefinition implements TransactionDefinition, Seri
 		return result;
 	}
 
+	private static String getPropagationBehaviorName(int propagationBehavior) {
+		for (Map.Entry<String, Integer> entry : propagationConstants.entrySet()) {
+			if (entry.getValue().equals(propagationBehavior)) {
+				return entry.getKey();
+			}
+		}
+		throw new IllegalArgumentException("Unsupported propagation behavior: " + propagationBehavior);
+	}
+
+	static String getIsolationLevelName(int isolationLevel) {
+		for (Map.Entry<String, Integer> entry : isolationConstants.entrySet()) {
+			if (entry.getValue().equals(isolationLevel)) {
+				return entry.getKey();
+			}
+		}
+		throw new IllegalArgumentException("Unsupported isolation level: " + isolationLevel);
+	}
+
 }

spring-tx/src/test/java/org/springframework/transaction/support/TestTransactionManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2023 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.
@@ -14,10 +14,10 @@
  * limitations under the License.
  */
 
-package org.springframework.transaction;
+package org.springframework.transaction.support;
 
-import org.springframework.transaction.support.AbstractPlatformTransactionManager;
+import org.springframework.transaction.CannotCreateTransactionException;
-import org.springframework.transaction.support.DefaultTransactionStatus;
+import org.springframework.transaction.TransactionDefinition;
 
 /**
  * @author Juergen Hoeller

spring-tx/src/test/java/org/springframework/transaction/support/TransactionSupportTests.java

@@ -14,22 +14,37 @@
  * limitations under the License.
  */
 
-package org.springframework.transaction;
+package org.springframework.transaction.support;
+
+import java.lang.reflect.Field;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 
-import org.springframework.transaction.support.DefaultTransactionDefinition;
+import org.springframework.transaction.IllegalTransactionStateException;
-import org.springframework.transaction.support.DefaultTransactionStatus;
+import org.springframework.transaction.MockCallbackPreferringTransactionManager;
-import org.springframework.transaction.support.TransactionCallbackWithoutResult;
+import org.springframework.transaction.PlatformTransactionManager;
-import org.springframework.transaction.support.TransactionSynchronizationManager;
+import org.springframework.transaction.TransactionDefinition;
-import org.springframework.transaction.support.TransactionTemplate;
+import org.springframework.transaction.TransactionStatus;
+import org.springframework.transaction.TransactionSystemException;
+import org.springframework.util.ReflectionUtils;
 
 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.assertj.core.api.Assertions.assertThatRuntimeException;
+import static org.springframework.transaction.TransactionDefinition.ISOLATION_REPEATABLE_READ;
+import static org.springframework.transaction.TransactionDefinition.ISOLATION_SERIALIZABLE;
+import static org.springframework.transaction.TransactionDefinition.PROPAGATION_MANDATORY;
+import static org.springframework.transaction.TransactionDefinition.PROPAGATION_REQUIRED;
+import static org.springframework.transaction.TransactionDefinition.PROPAGATION_SUPPORTS;
+import static org.springframework.transaction.support.DefaultTransactionDefinition.PREFIX_ISOLATION;
+import static org.springframework.transaction.support.DefaultTransactionDefinition.PREFIX_PROPAGATION;
 
 /**
  * @author Juergen Hoeller
@@ -48,33 +63,33 @@ class TransactionSupportTests {
 	void noExistingTransaction() {
 		PlatformTransactionManager tm = new TestTransactionManager(false, true);
 		DefaultTransactionStatus status1 = (DefaultTransactionStatus)
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_SUPPORTS));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_SUPPORTS));
 		assertThat(status1.hasTransaction()).as("Must not have transaction").isFalse();
 
 		DefaultTransactionStatus status2 = (DefaultTransactionStatus)
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_REQUIRED));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_REQUIRED));
 		assertThat(status2.hasTransaction()).as("Must have transaction").isTrue();
 		assertThat(status2.isNewTransaction()).as("Must be new transaction").isTrue();
 
 		assertThatExceptionOfType(IllegalTransactionStateException.class).isThrownBy(() ->
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_MANDATORY)));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_MANDATORY)));
 	}
 
 	@Test
 	void existingTransaction() {
 		PlatformTransactionManager tm = new TestTransactionManager(true, true);
 		DefaultTransactionStatus status1 = (DefaultTransactionStatus)
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_SUPPORTS));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_SUPPORTS));
 		assertThat(status1.getTransaction()).as("Must have transaction").isNotNull();
 		assertThat(status1.isNewTransaction()).as("Must not be new transaction").isFalse();
 
 		DefaultTransactionStatus status2 = (DefaultTransactionStatus)
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_REQUIRED));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_REQUIRED));
 		assertThat(status2.getTransaction()).as("Must have transaction").isNotNull();
 		assertThat(status2.isNewTransaction()).as("Must not be new transaction").isFalse();
 
 		DefaultTransactionStatus status3 = (DefaultTransactionStatus)
-				tm.getTransaction(new DefaultTransactionDefinition(TransactionDefinition.PROPAGATION_MANDATORY));
+				tm.getTransaction(new DefaultTransactionDefinition(PROPAGATION_MANDATORY));
 		assertThat(status3.getTransaction()).as("Must have transaction").isNotNull();
 		assertThat(status3.isNewTransaction()).as("Must not be new transaction").isFalse();
 	}
@@ -263,35 +278,101 @@ class TransactionSupportTests {
 		}
 
 		@Test
-		void setPropagationBehaviorName() {
+		void setPropagationBehaviorNameToUnsupportedValues() {
-			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehaviorName("TIMEOUT_DEFAULT"));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehaviorName(null));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehaviorName("   "));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehaviorName("bogus"));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehaviorName("ISOLATION_SERIALIZABLE"));
+		}
 
-			template.setPropagationBehaviorName("PROPAGATION_SUPPORTS");
+		/**
-			assertThat(template.getPropagationBehavior()).isEqualTo(TransactionDefinition.PROPAGATION_SUPPORTS);
+		 * Verify that the internal 'propagationConstants' map is properly configured
+		 * for all PROPAGATION_ constants defined in {@link TransactionDefinition}.
+		 */
+		@Test
+		void setPropagationBehaviorNameToAllSupportedValues() {
+			Set<Integer> uniqueValues = new HashSet<>();
+			streamPropagationConstants()
+					.forEach(name -> {
+						template.setPropagationBehaviorName(name);
+						int propagationBehavior = template.getPropagationBehavior();
+						int expected = DefaultTransactionDefinition.propagationConstants.get(name);
+						assertThat(propagationBehavior).isEqualTo(expected);
+						uniqueValues.add(propagationBehavior);
+					});
+			assertThat(uniqueValues).hasSize(DefaultTransactionDefinition.propagationConstants.size());
 		}
 
 		@Test
 		void setPropagationBehavior() {
 			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehavior(999));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setPropagationBehavior(ISOLATION_SERIALIZABLE));
 
-			template.setPropagationBehavior(TransactionDefinition.PROPAGATION_MANDATORY);
+			template.setPropagationBehavior(PROPAGATION_MANDATORY);
-			assertThat(template.getPropagationBehavior()).isEqualTo(TransactionDefinition.PROPAGATION_MANDATORY);
+			assertThat(template.getPropagationBehavior()).isEqualTo(PROPAGATION_MANDATORY);
 		}
 
 		@Test
-		void setIsolationLevelName() {
+		void setIsolationLevelNameToUnsupportedValues() {
-			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevelName("TIMEOUT_DEFAULT"));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevelName(null));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevelName("   "));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevelName("bogus"));
+			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevelName("PROPAGATION_MANDATORY"));
+		}
 
-			template.setIsolationLevelName("ISOLATION_SERIALIZABLE");
+		/**
-			assertThat(template.getIsolationLevel()).isEqualTo(TransactionDefinition.ISOLATION_SERIALIZABLE);
+		 * Verify that the internal 'isolationConstants' map is properly configured
+		 * for all ISOLATION_ constants defined in {@link TransactionDefinition}.
+		 */
+		@Test
+		void setIsolationLevelNameToAllSupportedValues() {
+			Set<Integer> uniqueValues = new HashSet<>();
+			streamIsolationConstants()
+					.forEach(name -> {
+						template.setIsolationLevelName(name);
+						int isolationLevel = template.getIsolationLevel();
+						int expected = DefaultTransactionDefinition.isolationConstants.get(name);
+						assertThat(isolationLevel).isEqualTo(expected);
+						uniqueValues.add(isolationLevel);
+					});
+			assertThat(uniqueValues).hasSize(DefaultTransactionDefinition.isolationConstants.size());
 		}
 
 		@Test
 		void setIsolationLevel() {
 			assertThatIllegalArgumentException().isThrownBy(() -> template.setIsolationLevel(999));
 
-			template.setIsolationLevel(TransactionDefinition.ISOLATION_REPEATABLE_READ);
+			template.setIsolationLevel(ISOLATION_REPEATABLE_READ);
-			assertThat(template.getIsolationLevel()).isEqualTo(TransactionDefinition.ISOLATION_REPEATABLE_READ);
+			assertThat(template.getIsolationLevel()).isEqualTo(ISOLATION_REPEATABLE_READ);
+		}
+
+		@Test
+		void getDefinitionDescription() {
+			assertThat(template.getDefinitionDescription()).asString()
+					.isEqualTo("PROPAGATION_REQUIRED,ISOLATION_DEFAULT");
+
+			template.setPropagationBehavior(PROPAGATION_MANDATORY);
+			template.setIsolationLevel(ISOLATION_REPEATABLE_READ);
+			template.setReadOnly(true);
+			template.setTimeout(42);
+			assertThat(template.getDefinitionDescription()).asString()
+					.isEqualTo("PROPAGATION_MANDATORY,ISOLATION_REPEATABLE_READ,timeout_42,readOnly");
+		}
+
+		private static Stream<String> streamPropagationConstants() {
+			return streamTransactionDefinitionConstants()
+					.filter(name -> name.startsWith(PREFIX_PROPAGATION));
+		}
+
+		private static Stream<String> streamIsolationConstants() {
+			return streamTransactionDefinitionConstants()
+					.filter(name -> name.startsWith(PREFIX_ISOLATION));
+		}
+
+		private static Stream<String> streamTransactionDefinitionConstants() {
+			return Arrays.stream(TransactionDefinition.class.getFields())
+					.filter(ReflectionUtils::isPublicStaticFinal)
+					.map(Field::getName);
 		}
 
 	}