Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support type-safe transaction rollback rules #28098

Closed
hduyyg opened this issue Feb 23, 2022 · 4 comments
Closed

Support type-safe transaction rollback rules #28098

hduyyg opened this issue Feb 23, 2022 · 4 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Milestone

Comments

@hduyyg
Copy link

hduyyg commented Feb 23, 2022

Source code in question:

private int getDepth(Class<?> exceptionClass, int depth) {
	if (exceptionClass.getName().contains(this.exceptionName)) {
		// Found it!
		return depth;
	}
	// If we've gone as far as we can go and haven't found it...
	if (exceptionClass == Throwable.class) {
		return -1;
	}
	return getDepth(exceptionClass.getSuperclass(), depth + 1);
}

test code:

public class CustomException extends Exception {
}

public class CustomExceptionX extends Exception {
}

@Override
@Transactional(rollbackFor = CustomException.class)
public void testTransaction() throws Exception {
    taskMapper.softDeleteById(1, "test");
    if (1 == 1) {
        throw new CustomExceptionX();
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 23, 2022
@snicoll snicoll changed the title Bug: RollbackRuleAttribute.getDepth() use contains instead of equals RollbackRuleAttribute.getDepth() use contains instead of equals Feb 23, 2022
@rstoyanchev rstoyanchev added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Feb 25, 2022
@sbrannen sbrannen changed the title RollbackRuleAttribute.getDepth() use contains instead of equals RollbackRuleAttribute.getDepth() uses contains() instead of equals() Feb 28, 2022
@sbrannen sbrannen self-assigned this Feb 28, 2022
@sbrannen
Copy link
Member

sbrannen commented Feb 28, 2022

This is by design and was originally implemented using contains() for use in XML configuration files where users often specified the simple name of a custom exception type instead of the fully qualified class name.

You can see examples of this in the reference docs.

<tx:advice id="txAdvice" transaction-manager="txManager">
  <tx:attributes>
    <tx:method name="get*" read-only="true" rollback-for="NoProductInStockException"/>
    <tx:method name="*"/>
  </tx:attributes>
</tx:advice>

With your proposal to use equals() instead of contains(), configuration like that would no longer work since NoProductInStockException would only ever be equal to the fully qualified class name if the NoProductInStockException was declared as a top-level class in the default package -- which is rather unlikely.

Please take note of the Javadoc for RollbackRuleAttribute as well:

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 unusual exception names such as "BaseBusinessException" there's no need to use a fully package-qualified name.

Similar documentation exists for the rollbackForClassName attribute in @Transactional.


The Javadoc for the RollbackRuleAttribute(Class) constructor states the following,

Create a new instance of the RollbackRuleAttribute class.

This is the preferred way to construct a rollback rule that matches the supplied Exception class, its subclasses, and its nested classes.

However, that last sentence is not honored in the current implementation, since the type information (supplied via the Class reference) is not taken into account in the implementation of getDepth(...).

@sbrannen sbrannen added this to the Triage Queue milestone Feb 28, 2022
@sbrannen sbrannen removed their assignment Feb 28, 2022
@sbrannen
Copy link
Member

sbrannen commented Mar 1, 2022

Correction to my previous statement. The Javadoc for the RollbackRuleAttribute(Class) constructor is mostly correct.

However, the documentation for rollback rules can be improved to warn that unintentional matches may arise if the name of a thrown exception contains the name of a registered exception type.

I am therefore repurposing this issue to improve the documentation.

@sbrannen sbrannen added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 1, 2022
@sbrannen sbrannen modified the milestones: Triage Queue, 5.3.17 Mar 1, 2022
@sbrannen sbrannen self-assigned this Mar 1, 2022
@sbrannen sbrannen changed the title RollbackRuleAttribute.getDepth() uses contains() instead of equals() Transaction rollback rules may result in unintentional matches for similarly named exceptions Mar 1, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 1, 2022
@hduyyg
Copy link
Author

hduyyg commented Mar 2, 2022

I think this rollback rule is fallible and needs more precise matching rules。
A lot of people don't really read documents。

@sbrannen
Copy link
Member

sbrannen commented Mar 2, 2022

I think this rollback rule is fallible and needs more precise matching rules。

I agree with you. I've raised #28125 to improve the documentation for 5.3.x.

And we'll use this issue to improve the behavior in 6.0.

Specifically:

  • If an exception pattern is supplied as a String -- for example, in XML configuration or via @Transactional(rollbackForClassName = "example.CustomException") -- the existing contains() logic will continue to be used.
  • If a concrete exception type is supplied as a Class reference -- for example, via @Transactional(rollbackFor = example.CustomException.class) -- new logic will be implemented which honors the supplied type information, thereby avoiding an unintentional match against example.CustomException2 when example.CustomException (without the 2) was supplied as the exception type.

@sbrannen sbrannen added type: enhancement A general enhancement and removed type: documentation A documentation task labels Mar 2, 2022
@sbrannen sbrannen changed the title Transaction rollback rules may result in unintentional matches for similarly named exceptions Use exception type information instead of pattern matching for transaction rollback rules Mar 2, 2022
@sbrannen sbrannen changed the title Use exception type information instead of pattern matching for transaction rollback rules Support type-safe transaction rollback rules Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants