From b3e5f86277e73c91990a85d23779509225085d63 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 3 Mar 2022 16:20:13 +0100 Subject: [PATCH 1/2] Polish rollback rule support --- .../interceptor/RollbackRuleAttribute.java | 78 +++++----- .../interceptor/MyRuntimeException.java | 8 +- .../RollbackRuleAttributeTests.java | 139 ++++++++++++------ .../RuleBasedTransactionAttributeTests.java | 32 ++-- .../TransactionAttributeEditorTests.java | 74 ++++------ 5 files changed, 188 insertions(+), 143 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java index 07a59cc113..4c3d4ec53c 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-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,13 +22,13 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** - * Rule determining whether or not a given exception (and any subclasses) - * should cause a rollback. + * Rule determining whether or not a given exception should cause a rollback. * *

Multiple such rules can be applied to determine whether a transaction * should commit or rollback after an exception has been thrown. * * @author Rod Johnson + * @author Sam Brannen * @since 09.04.2003 * @see NoRollbackRuleAttribute */ @@ -36,7 +36,7 @@ import org.springframework.util.Assert; public class RollbackRuleAttribute implements Serializable{ /** - * The {@link RollbackRuleAttribute rollback rule} for + * The {@linkplain RollbackRuleAttribute rollback rule} for * {@link RuntimeException RuntimeExceptions}. */ public static final RollbackRuleAttribute ROLLBACK_ON_RUNTIME_EXCEPTIONS = @@ -48,30 +48,31 @@ public class RollbackRuleAttribute implements Serializable{ * This way does multiple string comparisons, but how often do we decide * whether to roll back a transaction following an exception? */ - private final String exceptionName; + private final String exceptionPattern; /** - * Create a new instance of the {@code RollbackRuleAttribute} class. + * Create a new instance of the {@code RollbackRuleAttribute} class + * for the given {@code exceptionType}. *

This is the preferred way to construct a rollback rule that matches - * the supplied {@link Exception} class, its subclasses, and its nested classes. - * @param clazz throwable class; must be {@link Throwable} or a subclass + * the supplied exception type, its subclasses, and its nested classes. + * @param exceptionType exception type; must be {@link Throwable} or a subclass * of {@code Throwable} - * @throws IllegalArgumentException if the supplied {@code clazz} is + * @throws IllegalArgumentException if the supplied {@code exceptionType} is * not a {@code Throwable} type or is {@code null} */ - public RollbackRuleAttribute(Class clazz) { - Assert.notNull(clazz, "'clazz' cannot be null"); - if (!Throwable.class.isAssignableFrom(clazz)) { + public RollbackRuleAttribute(Class exceptionType) { + Assert.notNull(exceptionType, "'exceptionType' cannot be null"); + if (!Throwable.class.isAssignableFrom(exceptionType)) { throw new IllegalArgumentException( - "Cannot construct rollback rule from [" + clazz.getName() + "]: it's not a Throwable"); + "Cannot construct rollback rule from [" + exceptionType.getName() + "]: it's not a Throwable"); } - this.exceptionName = clazz.getName(); + this.exceptionPattern = exceptionType.getName(); } /** * Create a new instance of the {@code RollbackRuleAttribute} class - * for the given {@code exceptionName}. + * for the given {@code exceptionPattern}. *

This can be a substring, with no wildcard support at present. A value * of "ServletException" would match * {@code javax.servlet.ServletException} and subclasses, for example. @@ -79,40 +80,49 @@ public class RollbackRuleAttribute implements Serializable{ * whether to include package information (which is not mandatory). For * example, "Exception" will match nearly anything, and will probably hide * other rules. "java.lang.Exception" would be correct if "Exception" was - * meant to define a rule for all checked exceptions. With more unusual + * meant to define a rule for all checked exceptions. With more unique * exception names such as "BaseBusinessException" there's no need to use a * fully package-qualified name. - * @param exceptionName the exception name pattern; can also be a fully + * @param exceptionPattern the exception name pattern; can also be a fully * package-qualified class name - * @throws IllegalArgumentException if the supplied - * {@code exceptionName} is {@code null} or empty + * @throws IllegalArgumentException if the supplied {@code exceptionPattern} + * is {@code null} or empty */ - public RollbackRuleAttribute(String exceptionName) { - Assert.hasText(exceptionName, "'exceptionName' cannot be null or empty"); - this.exceptionName = exceptionName; + public RollbackRuleAttribute(String exceptionPattern) { + Assert.hasText(exceptionPattern, "'exceptionPattern' cannot be null or empty"); + this.exceptionPattern = exceptionPattern; } /** - * Return the pattern for the exception name. + * Get the configured exception name pattern that this rule uses for matching. + * @see #getDepth(Throwable) */ public String getExceptionName() { - return this.exceptionName; + return this.exceptionPattern; } /** - * Return the depth of the superclass matching. - *

{@code 0} means {@code ex} matches exactly. Returns - * {@code -1} if there is no match. Otherwise, returns depth with the - * lowest depth winning. + * Return the depth of the superclass matching, with the following semantics. + *

+ *

When comparing roll back rules that match against a given exception, a rule + * with a lower matching depth wins. For example, a direct match ({@code depth == 0}) + * wins over a match in the superclass hierarchy ({@code depth > 0}). */ - public int getDepth(Throwable ex) { - return getDepth(ex.getClass(), 0); + public int getDepth(Throwable exception) { + return getDepth(exception.getClass(), 0); } private int getDepth(Class exceptionClass, int depth) { - if (exceptionClass.getName().contains(this.exceptionName)) { + if (exceptionClass.getName().contains(this.exceptionPattern)) { // Found it! return depth; } @@ -133,17 +143,17 @@ public class RollbackRuleAttribute implements Serializable{ return false; } RollbackRuleAttribute rhs = (RollbackRuleAttribute) other; - return this.exceptionName.equals(rhs.exceptionName); + return this.exceptionPattern.equals(rhs.exceptionPattern); } @Override public int hashCode() { - return this.exceptionName.hashCode(); + return this.exceptionPattern.hashCode(); } @Override public String toString() { - return "RollbackRuleAttribute with pattern [" + this.exceptionName + "]"; + return "RollbackRuleAttribute with pattern [" + this.exceptionPattern + "]"; } } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/MyRuntimeException.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/MyRuntimeException.java index 80affe6bc2..5b512e1eb8 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/MyRuntimeException.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/MyRuntimeException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-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. @@ -25,7 +25,13 @@ import org.springframework.core.NestedRuntimeException; */ @SuppressWarnings("serial") class MyRuntimeException extends NestedRuntimeException { + + public MyRuntimeException() { + super(""); + } + public MyRuntimeException(String msg) { super(msg); } + } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/RollbackRuleAttributeTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/RollbackRuleAttributeTests.java index fd05ff6755..f210659af3 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/RollbackRuleAttributeTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/RollbackRuleAttributeTests.java @@ -18,6 +18,7 @@ package org.springframework.transaction.interceptor; import java.io.IOException; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.springframework.beans.FatalBeanException; @@ -36,65 +37,105 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException */ class RollbackRuleAttributeTests { - @Test - void constructorArgumentMustBeThrowableClassWithNonThrowableType() { - assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute(Object.class)); + @Nested + class ExceptionPatternTests { + + @Test + void constructorPreconditions() { + assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute((String) null)); + } + + @Test + void notFound() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(IOException.class.getName()); + assertThat(rr.getDepth(new MyRuntimeException())).isEqualTo(-1); + } + + @Test + void alwaysFoundForThrowable() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Throwable.class.getName()); + assertThat(rr.getDepth(new MyRuntimeException())).isGreaterThan(0); + assertThat(rr.getDepth(new IOException())).isGreaterThan(0); + assertThat(rr.getDepth(new FatalBeanException(null, null))).isGreaterThan(0); + assertThat(rr.getDepth(new RuntimeException())).isGreaterThan(0); + } + + @Test + void foundImmediatelyWhenDirectMatch() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class.getName()); + assertThat(rr.getDepth(new Exception())).isEqualTo(0); + } + + @Test + void foundImmediatelyWhenExceptionThrownIsNestedTypeOfRegisteredException() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(EnclosingException.class.getName()); + assertThat(rr.getDepth(new EnclosingException.NestedException())).isEqualTo(0); + } + + @Test + void foundImmediatelyWhenNameOfExceptionThrownStartsWithNameOfRegisteredException() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(MyException.class.getName()); + assertThat(rr.getDepth(new MyException2())).isEqualTo(0); + } + + @Test + void foundInSuperclassHierarchy() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class.getName()); + // Exception -> RuntimeException -> NestedRuntimeException -> MyRuntimeException + assertThat(rr.getDepth(new MyRuntimeException())).isEqualTo(3); + } + } - @Test - void constructorArgumentMustBeThrowableClassWithNullThrowableType() { - assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute((Class) null)); - } + @Nested + class ExceptionTypeTests { - @Test - void constructorArgumentMustBeStringWithNull() { - assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute((String) null)); - } + @Test + void constructorPreconditions() { + assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute(Object.class)); + assertThatIllegalArgumentException().isThrownBy(() -> new RollbackRuleAttribute((Class) null)); + } - @Test - void notFound() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(IOException.class); - assertThat(rr.getDepth(new MyRuntimeException(""))).isEqualTo(-1); - } + @Test + void notFound() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(IOException.class); + assertThat(rr.getDepth(new MyRuntimeException())).isEqualTo(-1); + } - @Test - void foundImmediatelyWithString() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class.getName()); - assertThat(rr.getDepth(new Exception())).isEqualTo(0); - } + @Test + void alwaysFoundForThrowable() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Throwable.class); + assertThat(rr.getDepth(new MyRuntimeException())).isGreaterThan(0); + assertThat(rr.getDepth(new IOException())).isGreaterThan(0); + assertThat(rr.getDepth(new FatalBeanException(null, null))).isGreaterThan(0); + assertThat(rr.getDepth(new RuntimeException())).isGreaterThan(0); + } - @Test - void foundImmediatelyWithClass() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class); - assertThat(rr.getDepth(new Exception())).isEqualTo(0); - } + @Test + void foundImmediatelyWhenDirectMatch() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class); + assertThat(rr.getDepth(new Exception())).isEqualTo(0); + } - @Test - void foundInSuperclassHierarchy() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class); - // Exception -> RuntimeException -> NestedRuntimeException -> MyRuntimeException - assertThat(rr.getDepth(new MyRuntimeException(""))).isEqualTo(3); - } + @Test + void foundImmediatelyWhenExceptionThrownIsNestedTypeOfRegisteredException() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(EnclosingException.class); + assertThat(rr.getDepth(new EnclosingException.NestedException())).isEqualTo(0); + } - @Test - void alwaysFoundForThrowable() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(Throwable.class); - assertThat(rr.getDepth(new MyRuntimeException(""))).isGreaterThan(0); - assertThat(rr.getDepth(new IOException())).isGreaterThan(0); - assertThat(rr.getDepth(new FatalBeanException(null, null))).isGreaterThan(0); - assertThat(rr.getDepth(new RuntimeException())).isGreaterThan(0); - } + @Test + void foundImmediatelyWhenNameOfExceptionThrownStartsWithNameOfRegisteredException() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(MyException.class); + assertThat(rr.getDepth(new MyException2())).isEqualTo(0); + } - @Test - void foundNestedExceptionInEnclosingException() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(EnclosingException.class); - assertThat(rr.getDepth(new EnclosingException.NestedException())).isEqualTo(0); - } + @Test + void foundInSuperclassHierarchy() { + RollbackRuleAttribute rr = new RollbackRuleAttribute(Exception.class); + // Exception -> RuntimeException -> NestedRuntimeException -> MyRuntimeException + assertThat(rr.getDepth(new MyRuntimeException())).isEqualTo(3); + } - @Test - void foundWhenNameOfExceptionThrownStartsWithTheNameOfTheRegisteredExceptionType() { - RollbackRuleAttribute rr = new RollbackRuleAttribute(MyException.class); - assertThat(rr.getDepth(new MyException2())).isEqualTo(0); } diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/RuleBasedTransactionAttributeTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/RuleBasedTransactionAttributeTests.java index 8aa9815e7c..3f4cffe2b8 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/RuleBasedTransactionAttributeTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/RuleBasedTransactionAttributeTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-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. @@ -35,13 +35,13 @@ import static org.assertj.core.api.Assertions.assertThat; * @author Chris Beams * @since 09.04.2003 */ -public class RuleBasedTransactionAttributeTests { +class RuleBasedTransactionAttributeTests { @Test - public void testDefaultRule() { + void defaultRule() { RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(); assertThat(rta.rollbackOn(new RuntimeException())).isTrue(); - assertThat(rta.rollbackOn(new MyRuntimeException(""))).isTrue(); + assertThat(rta.rollbackOn(new MyRuntimeException())).isTrue(); assertThat(rta.rollbackOn(new Exception())).isFalse(); assertThat(rta.rollbackOn(new IOException())).isFalse(); } @@ -50,20 +50,20 @@ public class RuleBasedTransactionAttributeTests { * Test one checked exception that should roll back. */ @Test - public void testRuleForRollbackOnChecked() { + void ruleForRollbackOnChecked() { List list = new ArrayList<>(); list.add(new RollbackRuleAttribute(IOException.class.getName())); RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(TransactionDefinition.PROPAGATION_REQUIRED, list); assertThat(rta.rollbackOn(new RuntimeException())).isTrue(); - assertThat(rta.rollbackOn(new MyRuntimeException(""))).isTrue(); + assertThat(rta.rollbackOn(new MyRuntimeException())).isTrue(); assertThat(rta.rollbackOn(new Exception())).isFalse(); // Check that default behaviour is overridden assertThat(rta.rollbackOn(new IOException())).isTrue(); } @Test - public void testRuleForCommitOnUnchecked() { + void ruleForCommitOnUnchecked() { List list = new ArrayList<>(); list.add(new NoRollbackRuleAttribute(MyRuntimeException.class.getName())); list.add(new RollbackRuleAttribute(IOException.class.getName())); @@ -71,14 +71,14 @@ public class RuleBasedTransactionAttributeTests { assertThat(rta.rollbackOn(new RuntimeException())).isTrue(); // Check default behaviour is overridden - assertThat(rta.rollbackOn(new MyRuntimeException(""))).isFalse(); + assertThat(rta.rollbackOn(new MyRuntimeException())).isFalse(); assertThat(rta.rollbackOn(new Exception())).isFalse(); // Check that default behaviour is overridden assertThat(rta.rollbackOn(new IOException())).isTrue(); } @Test - public void testRuleForSelectiveRollbackOnCheckedWithString() { + void ruleForSelectiveRollbackOnCheckedWithString() { List l = new ArrayList<>(); l.add(new RollbackRuleAttribute(java.rmi.RemoteException.class.getName())); RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(TransactionDefinition.PROPAGATION_REQUIRED, l); @@ -86,7 +86,7 @@ public class RuleBasedTransactionAttributeTests { } @Test - public void testRuleForSelectiveRollbackOnCheckedWithClass() { + void ruleForSelectiveRollbackOnCheckedWithClass() { List l = Collections.singletonList(new RollbackRuleAttribute(RemoteException.class)); RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(TransactionDefinition.PROPAGATION_REQUIRED, l); doTestRuleForSelectiveRollbackOnChecked(rta); @@ -105,7 +105,7 @@ public class RuleBasedTransactionAttributeTests { * when Exception prompts a rollback. */ @Test - public void testRuleForCommitOnSubclassOfChecked() { + void ruleForCommitOnSubclassOfChecked() { List list = new ArrayList<>(); // Note that it's important to ensure that we have this as // a FQN: otherwise it will match everything! @@ -120,20 +120,20 @@ public class RuleBasedTransactionAttributeTests { } @Test - public void testRollbackNever() { + void rollbackNever() { List list = new ArrayList<>(); list.add(new NoRollbackRuleAttribute("Throwable")); RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(TransactionDefinition.PROPAGATION_REQUIRED, list); assertThat(rta.rollbackOn(new Throwable())).isFalse(); assertThat(rta.rollbackOn(new RuntimeException())).isFalse(); - assertThat(rta.rollbackOn(new MyRuntimeException(""))).isFalse(); + assertThat(rta.rollbackOn(new MyRuntimeException())).isFalse(); assertThat(rta.rollbackOn(new Exception())).isFalse(); assertThat(rta.rollbackOn(new IOException())).isFalse(); } @Test - public void testToStringMatchesEditor() { + void toStringMatchesEditor() { List list = new ArrayList<>(); list.add(new NoRollbackRuleAttribute("Throwable")); RuleBasedTransactionAttribute rta = new RuleBasedTransactionAttribute(TransactionDefinition.PROPAGATION_REQUIRED, list); @@ -144,7 +144,7 @@ public class RuleBasedTransactionAttributeTests { assertThat(rta.rollbackOn(new Throwable())).isFalse(); assertThat(rta.rollbackOn(new RuntimeException())).isFalse(); - assertThat(rta.rollbackOn(new MyRuntimeException(""))).isFalse(); + assertThat(rta.rollbackOn(new MyRuntimeException())).isFalse(); assertThat(rta.rollbackOn(new Exception())).isFalse(); assertThat(rta.rollbackOn(new IOException())).isFalse(); } @@ -153,7 +153,7 @@ public class RuleBasedTransactionAttributeTests { * See this forum post. */ @Test - public void testConflictingRulesToDetermineExactContract() { + void conflictingRulesToDetermineExactContract() { List list = new ArrayList<>(); list.add(new NoRollbackRuleAttribute(MyBusinessWarningException.class)); list.add(new RollbackRuleAttribute(MyBusinessException.class)); diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionAttributeEditorTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionAttributeEditorTests.java index 4614f2648a..3bfd82bb67 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionAttributeEditorTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionAttributeEditorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-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. @@ -16,7 +16,6 @@ package org.springframework.transaction.interceptor; - import java.io.IOException; import org.junit.jupiter.api.Test; @@ -27,72 +26,65 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** - * Tests to check conversion from String to TransactionAttribute. + * Tests to check conversion from String to TransactionAttribute using + * a {@link TransactionAttributeEditor}. * * @author Rod Johnson * @author Juergen Hoeller * @author Chris Beams * @since 26.04.2003 */ -public class TransactionAttributeEditorTests { +class TransactionAttributeEditorTests { + + private final TransactionAttributeEditor pe = new TransactionAttributeEditor(); + @Test - public void testNull() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void nullText() { pe.setAsText(null); - TransactionAttribute ta = (TransactionAttribute) pe.getValue(); - assertThat(ta == null).isTrue(); + assertThat(pe.getValue()).isNull(); } @Test - public void testEmptyString() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void emptyString() { pe.setAsText(""); - TransactionAttribute ta = (TransactionAttribute) pe.getValue(); - assertThat(ta == null).isTrue(); + assertThat(pe.getValue()).isNull(); } @Test - public void testValidPropagationCodeOnly() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void validPropagationCodeOnly() { pe.setAsText("PROPAGATION_REQUIRED"); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); - assertThat(ta != null).isTrue(); - assertThat(ta.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRED).isTrue(); - assertThat(ta.getIsolationLevel() == TransactionDefinition.ISOLATION_DEFAULT).isTrue(); - boolean condition = !ta.isReadOnly(); - assertThat(condition).isTrue(); + assertThat(ta).isNotNull(); + assertThat(ta.getPropagationBehavior()).isEqualTo(TransactionDefinition.PROPAGATION_REQUIRED); + assertThat(ta.getIsolationLevel()).isEqualTo(TransactionDefinition.ISOLATION_DEFAULT); + assertThat(ta.isReadOnly()).isFalse(); } @Test - public void testInvalidPropagationCodeOnly() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void invalidPropagationCodeOnly() { // should have failed with bogus propagation code - assertThatIllegalArgumentException().isThrownBy(() -> - pe.setAsText("XXPROPAGATION_REQUIRED")); + assertThatIllegalArgumentException().isThrownBy(() -> pe.setAsText("XXPROPAGATION_REQUIRED")); } @Test - public void testValidPropagationCodeAndIsolationCode() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void validPropagationCodeAndIsolationCode() { pe.setAsText("PROPAGATION_REQUIRED, ISOLATION_READ_UNCOMMITTED"); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); - assertThat(ta != null).isTrue(); - assertThat(ta.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRED).isTrue(); - assertThat(ta.getIsolationLevel() == TransactionDefinition.ISOLATION_READ_UNCOMMITTED).isTrue(); + assertThat(ta).isNotNull(); + assertThat(ta.getPropagationBehavior()).isEqualTo(TransactionDefinition.PROPAGATION_REQUIRED); + assertThat(ta.getIsolationLevel()).isEqualTo(TransactionDefinition.ISOLATION_READ_UNCOMMITTED); } @Test - public void testValidPropagationAndIsolationCodesAndInvalidRollbackRule() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void validPropagationAndIsolationCodesAndInvalidRollbackRule() { // should fail with bogus rollback rule - assertThatIllegalArgumentException().isThrownBy(() -> - pe.setAsText("PROPAGATION_REQUIRED,ISOLATION_READ_UNCOMMITTED,XXX")); + assertThatIllegalArgumentException() + .isThrownBy(() -> pe.setAsText("PROPAGATION_REQUIRED,ISOLATION_READ_UNCOMMITTED,XXX")); } @Test - public void testValidPropagationCodeAndIsolationCodeAndRollbackRules1() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void validPropagationCodeAndIsolationCodeAndRollbackRules1() { pe.setAsText("PROPAGATION_MANDATORY,ISOLATION_REPEATABLE_READ,timeout_10,-IOException,+MyRuntimeException"); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); assertThat(ta).isNotNull(); @@ -104,13 +96,11 @@ public class TransactionAttributeEditorTests { assertThat(ta.rollbackOn(new Exception())).isFalse(); // Check for our bizarre customized rollback rules assertThat(ta.rollbackOn(new IOException())).isTrue(); - boolean condition = !ta.rollbackOn(new MyRuntimeException("")); - assertThat(condition).isTrue(); + assertThat(ta.rollbackOn(new MyRuntimeException())).isFalse(); } @Test - public void testValidPropagationCodeAndIsolationCodeAndRollbackRules2() { - TransactionAttributeEditor pe = new TransactionAttributeEditor(); + void validPropagationCodeAndIsolationCodeAndRollbackRules2() { pe.setAsText("+IOException,readOnly,ISOLATION_READ_COMMITTED,-MyRuntimeException,PROPAGATION_SUPPORTS"); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); assertThat(ta).isNotNull(); @@ -122,18 +112,17 @@ public class TransactionAttributeEditorTests { assertThat(ta.rollbackOn(new Exception())).isFalse(); // Check for our bizarre customized rollback rules assertThat(ta.rollbackOn(new IOException())).isFalse(); - assertThat(ta.rollbackOn(new MyRuntimeException(""))).isTrue(); + assertThat(ta.rollbackOn(new MyRuntimeException())).isTrue(); } @Test - public void testDefaultTransactionAttributeToString() { + void defaultTransactionAttributeToString() { DefaultTransactionAttribute source = new DefaultTransactionAttribute(); source.setPropagationBehavior(TransactionDefinition.PROPAGATION_SUPPORTS); source.setIsolationLevel(TransactionDefinition.ISOLATION_REPEATABLE_READ); source.setTimeout(10); source.setReadOnly(true); - TransactionAttributeEditor pe = new TransactionAttributeEditor(); pe.setAsText(source.toString()); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); assertThat(source).isEqualTo(ta); @@ -151,7 +140,7 @@ public class TransactionAttributeEditorTests { } @Test - public void testRuleBasedTransactionAttributeToString() { + void ruleBasedTransactionAttributeToString() { RuleBasedTransactionAttribute source = new RuleBasedTransactionAttribute(); source.setPropagationBehavior(TransactionDefinition.PROPAGATION_SUPPORTS); source.setIsolationLevel(TransactionDefinition.ISOLATION_REPEATABLE_READ); @@ -160,7 +149,6 @@ public class TransactionAttributeEditorTests { source.getRollbackRules().add(new RollbackRuleAttribute("IllegalArgumentException")); source.getRollbackRules().add(new NoRollbackRuleAttribute("IllegalStateException")); - TransactionAttributeEditor pe = new TransactionAttributeEditor(); pe.setAsText(source.toString()); TransactionAttribute ta = (TransactionAttribute) pe.getValue(); assertThat(source).isEqualTo(ta); From fa3130d71631f6de36a34d0a5cdf1a37bece3102 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 4 Mar 2022 16:39:11 +0100 Subject: [PATCH 2/2] Document that TX rollback rules may result in unintentional matches Closes gh-28125 --- .../transaction/annotation/Transactional.java | 105 +++++++++++----- .../interceptor/NoRollbackRuleAttribute.java | 25 ++-- .../interceptor/RollbackRuleAttribute.java | 47 ++++--- src/docs/asciidoc/data-access.adoc | 119 +++++++++++++----- 4 files changed, 209 insertions(+), 87 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/annotation/Transactional.java b/spring-tx/src/main/java/org/springframework/transaction/annotation/Transactional.java index 4253757222..935933af42 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/annotation/Transactional.java +++ b/spring-tx/src/main/java/org/springframework/transaction/annotation/Transactional.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-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. @@ -37,17 +37,57 @@ import org.springframework.transaction.TransactionDefinition; * Transaction Management * section of the reference manual. * - *

This annotation type is generally directly comparable to Spring's + *

This annotation is generally directly comparable to Spring's * {@link org.springframework.transaction.interceptor.RuleBasedTransactionAttribute} * class, and in fact {@link AnnotationTransactionAttributeSource} will directly - * convert the data to the latter class, so that Spring's transaction support code - * does not have to know about annotations. If no custom rollback rules apply, - * the transaction will roll back on {@link RuntimeException} and {@link Error} - * but not on checked exceptions. + * convert this annotation's attributes to properties in {@code RuleBasedTransactionAttribute}, + * so that Spring's transaction support code does not have to know about annotations. * - *

For specific information about the semantics of this annotation's attributes, - * consult the {@link org.springframework.transaction.TransactionDefinition} and - * {@link org.springframework.transaction.interceptor.TransactionAttribute} javadocs. + *

Attribute Semantics

+ * + *

If no custom rollback rules are configured in this annotation, the transaction + * will roll back on {@link RuntimeException} and {@link Error} but not on checked + * exceptions. + * + *

Rollback rules determine if a transaction should be rolled back when a given + * exception is thrown, and the rules are based on patterns. A pattern can be a + * fully qualified class name or a substring of a fully qualified class name for + * an exception type (which must be a subclass of {@code Throwable}), with no + * wildcard support at present. For example, a value of + * {@code "javax.servlet.ServletException"} or {@code "ServletException"} will + * match {@code javax.servlet.ServletException} and its subclasses. + * + *

Rollback rules may be configured via {@link #rollbackFor}/{@link #noRollbackFor} + * and {@link #rollbackForClassName}/{@link #noRollbackForClassName}, which allow + * patterns to be specified as {@link Class} references or {@linkplain String + * strings}, respectively. When an exception type is specified as a class reference + * its fully qualified name will be used as the pattern. Consequently, + * {@code @Transactional(rollbackFor = example.CustomException.class)} is equivalent + * to {@code @Transactional(rollbackForClassName = "example.CustomException")}. + * + *

WARNING: You must carefully consider how specific the pattern + * is and whether to include package information (which isn't mandatory). For example, + * {@code "Exception"} will match nearly anything and will probably hide other + * rules. {@code "java.lang.Exception"} would be correct if {@code "Exception"} + * were meant to define a rule for all checked exceptions. With more unique + * exception names such as {@code "BaseBusinessException"} there is likely no + * need to use the fully qualified class name for the exception pattern. Furthermore, + * rollback rules may result in unintentional matches for similarly named exceptions + * and nested classes. This is due to the fact that a thrown exception is considered + * to be a match for a given rollback rule if the name of thrown exception contains + * the exception pattern configured for the rollback rule. For example, given a + * rule configured to match on {@code com.example.CustomException}, that rule + * would match against an exception named + * {@code com.example.CustomExceptionV2} (an exception in the same package as + * {@code CustomException} but with an additional suffix) or an exception named + * {@code com.example.CustomException$AnotherException} + * (an exception declared as a nested class in {@code CustomException}). + * + *

For specific information about the semantics of other attributes in this + * annotation, consult the {@link org.springframework.transaction.TransactionDefinition} + * and {@link org.springframework.transaction.interceptor.TransactionAttribute} javadocs. + * + *

Transaction Management

* *

This annotation commonly works with thread-bound transactions managed by a * {@link org.springframework.transaction.PlatformTransactionManager}, exposing a @@ -167,37 +207,33 @@ public @interface Transactional { boolean readOnly() default false; /** - * Defines zero (0) or more exception {@link Class classes}, which must be + * Defines zero (0) or more exception {@linkplain Class classes}, which must be * subclasses of {@link Throwable}, indicating which exception types must cause * a transaction rollback. - *

By default, a transaction will be rolling back on {@link RuntimeException} + *

By default, a transaction will be rolled back on {@link RuntimeException} * and {@link Error} but not on checked exceptions (business exceptions). See * {@link org.springframework.transaction.interceptor.DefaultTransactionAttribute#rollbackOn(Throwable)} * for a detailed explanation. *

This is the preferred way to construct a rollback rule (in contrast to - * {@link #rollbackForClassName}), matching the exception class and its subclasses. - *

Similar to {@link org.springframework.transaction.interceptor.RollbackRuleAttribute#RollbackRuleAttribute(Class clazz)}. + * {@link #rollbackForClassName}), matching the exception type, its subclasses, + * and its nested classes. See the {@linkplain Transactional class-level javadocs} + * for further details on rollback rule semantics and warnings regarding possible + * unintentional matches. * @see #rollbackForClassName + * @see org.springframework.transaction.interceptor.RollbackRuleAttribute#RollbackRuleAttribute(Class) * @see org.springframework.transaction.interceptor.DefaultTransactionAttribute#rollbackOn(Throwable) */ Class[] rollbackFor() default {}; /** - * Defines zero (0) or more exception names (for exceptions which must be a + * Defines zero (0) or more exception name patterns (for exceptions which must be a * subclass of {@link Throwable}), indicating which exception types must cause * a transaction rollback. - *

This can be a substring of a fully qualified class name, with no wildcard - * support at present. For example, a value of {@code "ServletException"} would - * match {@code javax.servlet.ServletException} and its subclasses. - *

NB: Consider carefully how specific the pattern is and whether - * to include package information (which isn't mandatory). For example, - * {@code "Exception"} will match nearly anything and will probably hide other - * rules. {@code "java.lang.Exception"} would be correct if {@code "Exception"} - * were meant to define a rule for all checked exceptions. With more unusual - * {@link Exception} names such as {@code "BaseBusinessException"} there is no - * need to use a FQN. - *

Similar to {@link org.springframework.transaction.interceptor.RollbackRuleAttribute#RollbackRuleAttribute(String exceptionName)}. + *

See the {@linkplain Transactional class-level javadocs} for further details + * on rollback rule semantics, patterns, and warnings regarding possible + * unintentional matches. * @see #rollbackFor + * @see org.springframework.transaction.interceptor.RollbackRuleAttribute#RollbackRuleAttribute(String) * @see org.springframework.transaction.interceptor.DefaultTransactionAttribute#rollbackOn(Throwable) */ String[] rollbackForClassName() default {}; @@ -206,23 +242,26 @@ public @interface Transactional { * Defines zero (0) or more exception {@link Class Classes}, which must be * subclasses of {@link Throwable}, indicating which exception types must * not cause a transaction rollback. - *

This is the preferred way to construct a rollback rule (in contrast - * to {@link #noRollbackForClassName}), matching the exception class and - * its subclasses. - *

Similar to {@link org.springframework.transaction.interceptor.NoRollbackRuleAttribute#NoRollbackRuleAttribute(Class clazz)}. + *

This is the preferred way to construct a rollback rule (in contrast to + * {@link #noRollbackForClassName}), matching the exception type, its subclasses, + * and its nested classes. See the {@linkplain Transactional class-level javadocs} + * for further details on rollback rule semantics and warnings regarding possible + * unintentional matches. * @see #noRollbackForClassName + * @see org.springframework.transaction.interceptor.NoRollbackRuleAttribute#NoRollbackRuleAttribute(Class) * @see org.springframework.transaction.interceptor.DefaultTransactionAttribute#rollbackOn(Throwable) */ Class[] noRollbackFor() default {}; /** - * Defines zero (0) or more exception names (for exceptions which must be a + * Defines zero (0) or more exception name patterns (for exceptions which must be a * subclass of {@link Throwable}) indicating which exception types must not * cause a transaction rollback. - *

See the description of {@link #rollbackForClassName} for further - * information on how the specified names are treated. - *

Similar to {@link org.springframework.transaction.interceptor.NoRollbackRuleAttribute#NoRollbackRuleAttribute(String exceptionName)}. + *

See the {@linkplain Transactional class-level javadocs} for further details + * on rollback rule semantics, patterns, and warnings regarding possible + * unintentional matches. * @see #noRollbackFor + * @see org.springframework.transaction.interceptor.NoRollbackRuleAttribute#NoRollbackRuleAttribute(String) * @see org.springframework.transaction.interceptor.DefaultTransactionAttribute#rollbackOn(Throwable) */ String[] noRollbackForClassName() default {}; diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/NoRollbackRuleAttribute.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/NoRollbackRuleAttribute.java index a92e14b9ff..2282274a94 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/NoRollbackRuleAttribute.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/NoRollbackRuleAttribute.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-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. @@ -21,6 +21,7 @@ package org.springframework.transaction.interceptor; * to the {@code RollbackRuleAttribute} superclass. * * @author Rod Johnson + * @author Sam Brannen * @since 09.04.2003 */ @SuppressWarnings("serial") @@ -28,22 +29,28 @@ public class NoRollbackRuleAttribute extends RollbackRuleAttribute { /** * Create a new instance of the {@code NoRollbackRuleAttribute} class - * for the supplied {@link Throwable} class. - * @param clazz the {@code Throwable} class + * for the given {@code exceptionType}. + * @param exceptionType exception type; must be {@link Throwable} or a subclass + * of {@code Throwable} + * @throws IllegalArgumentException if the supplied {@code exceptionType} is + * not a {@code Throwable} type or is {@code null} * @see RollbackRuleAttribute#RollbackRuleAttribute(Class) */ - public NoRollbackRuleAttribute(Class clazz) { - super(clazz); + public NoRollbackRuleAttribute(Class exceptionType) { + super(exceptionType); } /** * Create a new instance of the {@code NoRollbackRuleAttribute} class - * for the supplied {@code exceptionName}. - * @param exceptionName the exception name pattern + * for the supplied {@code exceptionPattern}. + * @param exceptionPattern the exception name pattern; can also be a fully + * package-qualified class name + * @throws IllegalArgumentException if the supplied {@code exceptionPattern} + * is {@code null} or empty * @see RollbackRuleAttribute#RollbackRuleAttribute(String) */ - public NoRollbackRuleAttribute(String exceptionName) { - super(exceptionName); + public NoRollbackRuleAttribute(String exceptionPattern) { + super(exceptionPattern); } @Override diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java index 4c3d4ec53c..a643c4c9b1 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/RollbackRuleAttribute.java @@ -27,6 +27,22 @@ import org.springframework.util.Assert; *

Multiple such rules can be applied to determine whether a transaction * should commit or rollback after an exception has been thrown. * + *

Each rule is based on an exception pattern which can be a fully qualified + * class name or a substring of a fully qualified class name for an exception + * type (which must be a subclass of {@code Throwable}), with no wildcard support + * at present. For example, a value of {@code "javax.servlet.ServletException"} + * or {@code "ServletException"} would match {@code javax.servlet.ServletException} + * and its subclasses. + * + *

An exception pattern can be specified as a {@link Class} reference or a + * {@link String} in {@link #RollbackRuleAttribute(Class)} and + * {@link #RollbackRuleAttribute(String)}, respectively. When an exception type + * is specified as a class reference its fully qualified name will be used as the + * pattern. See the javadocs for + * {@link org.springframework.transaction.annotation.Transactional @Transactional} + * for further details on rollback rule semantics, patterns, and warnings regarding + * possible unintentional matches. + * * @author Rod Johnson * @author Sam Brannen * @since 09.04.2003 @@ -56,6 +72,10 @@ public class RollbackRuleAttribute implements Serializable{ * for the given {@code exceptionType}. *

This is the preferred way to construct a rollback rule that matches * the supplied exception type, its subclasses, and its nested classes. + *

See the javadocs for + * {@link org.springframework.transaction.annotation.Transactional @Transactional} + * for further details on rollback rule semantics, patterns, and warnings regarding + * possible unintentional matches. * @param exceptionType exception type; must be {@link Throwable} or a subclass * of {@code Throwable} * @throws IllegalArgumentException if the supplied {@code exceptionType} is @@ -73,16 +93,10 @@ public class RollbackRuleAttribute implements Serializable{ /** * Create a new instance of the {@code RollbackRuleAttribute} class * for the given {@code exceptionPattern}. - *

This can be a substring, with no wildcard support at present. A value - * of "ServletException" would match - * {@code javax.servlet.ServletException} and subclasses, for example. - *

NB: Consider carefully how specific the pattern is, and - * whether to include package information (which is not mandatory). For - * example, "Exception" will match nearly anything, and will probably hide - * other rules. "java.lang.Exception" would be correct if "Exception" was - * meant to define a rule for all checked exceptions. With more unique - * exception names such as "BaseBusinessException" there's no need to use a - * fully package-qualified name. + *

See the javadocs for + * {@link org.springframework.transaction.annotation.Transactional @Transactional} + * for further details on rollback rule semantics, patterns, and warnings regarding + * possible unintentional matches. * @param exceptionPattern the exception name pattern; can also be a fully * package-qualified class name * @throws IllegalArgumentException if the supplied {@code exceptionPattern} @@ -106,7 +120,7 @@ public class RollbackRuleAttribute implements Serializable{ * Return the depth of the superclass matching, with the following semantics. *