Inherited @Transactional methods should be able to use class-level TransactionManager qualifier from concrete class

[elab]

Setup: In a multi-DB environment, there is an abstract Service/DAO class with @Transactional methods common to all entities from all DBs. Then there are multiple concrete Service/DAO beans, one per DB, each with specific transactionManager in the class-level @Transactional annotation.

Problem: When calling a base-class method via some DB-specific bean, a wrong transactionManager is used (always the @Primary one). The worst is that no exceptions are thrown, no error messages printed in log. The entities are simply silently e.g. not saved/updated in DB. Or, I afraid, they could even be saved to wrong DB.

The problem has already been described at least in #14011, #14295, #17080. Related issue is also #11839. But all the issues have been closed (as outdated) without a solution.

There were also some workarounds: https://stackoverflow.com/questions/51087660/dynamic-selection-of-transactionmanager-spring-boot, https://www.tirasa.net/en/blog/dynamic-springs-at-transactional, also #14295. But they all are not working (anymore).

How can it be solved? What do you, guys, do in such situation? Am I missing something obvious?
Thank you for your thoughts.

[elab]

I couldn't find a reliable way to work around (e.g. defining an own TransactionInterceptor was practically impossible for me due to many unclear interdependencies in Spring code; would be glad to see a working example though). Therefore I don't see any other way as to make changes to the Spring codebase, to the place where transaction attributes are computed.

It's all about the class AbstractFallbackTransactionAttributeSource, method computeTransactionAttribute. There are two implementation of them:

• in spring-framework/spring-tx (@jhoeller), important pieces of javadoc: on class
• in spring-data-commons for Spring Data project (@olivergierke @odrotbohm), important pieces of javadoc: common note, on class, also see Make AbstractFallbackTransactionAttributeSource.computeTransactionAttribute(…) protected [SPR-13246] #17837

Unfortunately both implementation have diverged a little. I see some refactorings done on the method in the spring-tx, which were not merged into spring-data-commons.

So I will concentrate on the spring-tx implementation for now.

1. The first thing (which was already mentioned by @nachogil in @Transactional annotation does not work with inheritance and qualifiers [SPR-9375] #14011 and by @rsanwal in Doc: Inheritance of @Transactional and lookup behaviour of AbstractFallbackTransactionAttributeSource [SPR-12474] #17080) is to fix these lines to refer to the targetClass instead of the declaringClass, in accordance with the logic and all the javadoc and inline comments:

old version:

spring-framework/spring-tx/src/main/java/org/springframework/transaction/interceptor/AbstractFallbackTransactionAttributeSource.java

Lines 167 to 168 in c8ef49c

	// Second try is the transaction attribute on the target class.
	txAttr = findTransactionAttribute(specificMethod.getDeclaringClass());

new version:

 // Second try is the transaction attribute on the target class. 
 txAttr = findTransactionAttribute(targetClass);

Update 2020-03-10: Now I think that the "declaring class" is correct and rather the javadoc/comments should be fixed than the code. But the "targetClass" could/should be considered as well as the next step.

2. The second and the most important thing (inspired by @mattinger in @Transactional with txManager name is cumbersome with base dao classes [SPR-9661] #14295, also see problem description in @Transactional inheritance [SPR-7180] #11839) is to merge settings from all relevant @Transactional annotations instead of returning the first found one:

old version:

spring-framework/spring-tx/src/main/java/org/springframework/transaction/interceptor/AbstractFallbackTransactionAttributeSource.java

Lines 161 to 186 in c8ef49c

	// First try is the method in the target class.
	TransactionAttribute txAttr = findTransactionAttribute(specificMethod);
	if (txAttr != null) {
	return txAttr;
	}
	// Second try is the transaction attribute on the target class.
	txAttr = findTransactionAttribute(specificMethod.getDeclaringClass());
	if (txAttr != null && ClassUtils.isUserLevelMethod(method)) {
	return txAttr;
	}
	if (specificMethod != method) {
	// Fallback is to look at the original method.
	txAttr = findTransactionAttribute(method);
	if (txAttr != null) {
	return txAttr;
	}
	// Last fallback is the class of the original method.
	txAttr = findTransactionAttribute(method.getDeclaringClass());
	if (txAttr != null && ClassUtils.isUserLevelMethod(method)) {
	return txAttr;
	}
	}
	return null;

new version:

// First try is the method in the target class.
TransactionAttribute txAttr = findTransactionAttribute(specificMethod);

// Second try is the transaction attribute on the target class.
txAttr = merge(txAttr, findTransactionAttribute(targetClass));

if (specificMethod != method) {
	// Fallback is to look at the original method.
	txAttr = merge(txAttr, findTransactionAttribute(method));

	// Last fallback is the class of the original method.
	txAttr = merge(txAttr, findTransactionAttribute(method.getDeclaringClass()));
}

return txAttr;

The ClassUtils.isUserLevelMethod(method) check from the original code should not be needed anymore, as this has previously prevented ignoring a user-made annotation (as opposed to a generated one in CGLIB/Proxy class), but now all the annotations are taken into consideration, no one is ignored.

the merge method (perhaps there is also another way of merging properties using some Spring reflection tools I'm not aware of):

/** Set empty properties of "primary" object from "secondary" object. */  
@Nullable
private TransactionAttribute merge(@Nullable TransactionAttribute primary, @Nullable TransactionAttribute secondary) {
	if (primary == null) return secondary;
	if (secondary == null) return primary;

	if (primary instanceof DefaultTransactionAttribute && secondary instanceof DefaultTransactionAttribute) {
		DefaultTransactionAttribute p = (DefaultTransactionAttribute) primary;
		DefaultTransactionAttribute s = (DefaultTransactionAttribute) secondary;

		if (p.getQualifier() == null) p.setQualifier(s.getQualifier());
		if (p.getDescriptor() == null) p.setDescriptor(s.getDescriptor());
		if (p.getName() == null) p.setName(s.getName());
		// other DefaultTransactionAttribute properties are always set (implicitly or explicitly):
		// propagationBehavior = PROPAGATION_REQUIRED, isolationLevel = ISOLATION_DEFAULT, timeout = TIMEOUT_DEFAULT, readOnly = false
	}
	if (primary instanceof RuleBasedTransactionAttribute && secondary instanceof RuleBasedTransactionAttribute) {
		RuleBasedTransactionAttribute p = (RuleBasedTransactionAttribute) primary;
		RuleBasedTransactionAttribute s = (RuleBasedTransactionAttribute) secondary;
		
		if (p.getRollbackRules() == null) p.setRollbackRules(s.getRollbackRules());
	}
	return primary;
}

---

The whole should ideally be refactored to take a list of suppliers of annotation-places (method in target class, target class, method in declaring class, declaring class), ordered by priority, as parameters. Or just one parameter, defining what has a higher priority: annotations in the declaring class (case for spring-data, as this is the user defined interface of the generated target class) or annotations in the target class (general case for all user-defined classes). Such an implementation could then be reused in both Spring projects: spring-data (which requires another priorities, see #17837) and spring-tx.

This call from spring-data implementation

// Ignore CGLIB subclasses - introspect the actual user class.
Class<?> userClass = ProxyUtils.getUserClass(targetClass);

should probably be added as well (userClass is then our new targetClass) for a reusable implementation. Not sure, which impact it would have on the logic in tx-transaction module though.

---

Possibly related issue about @Transactional semantics with interfaces and CGLIB proxy: #23538.

---

Unfortunately I currently have some troubles with setting up a development environment for Spring projects. Could someone try/review the proposed changes and give a feedback?

[elab]

OK, I slightly improved the merge method (see below) and created a patched version of the original Maven artifact spring-tx 5.2.3.RELEASE (which is used by the SpringBoot 2.2.4.RELEASE):

• original: https://repo1.maven.org/maven2/org/springframework/spring-tx/5.2.3.RELEASE/
• patched: https://repo1.maven.org/maven2/com/labun/spring-tx/5.2.3.RELEASE.patched/

(patches for 5.2.1 and 5.2.2 are available there as well)

The only changed source file is org.springframework.transaction.interceptor.AbstractFallbackTransactionAttributeSource -- you can verify it by comparing source jars by content.
The only changed binary files are AbstractFallbackTransactionAttributeSource.class and AbstractFallbackTransactionAttributeSource$1.class (only two bytes in the latter, obviously due to a changed offset position in the main class, where this anonymous class is created) -- you can verify it by comparing main jars by content. Compiled with AdoptOpenJDK 1.8.0_222 / Maven 3.6.2.
Javadoc jar remained unchanged for now.

Usage with Maven:

Inspect the dependency hierarchy in your project, identify the root Spring artifact which triggers the dependency on spring-tx. In my case, it's spring-boot-starter-data-jpa. Then exclude the original spring-tx dependency and add the patched one:

<dependency>
    <groupId>org.springframework.boot</groupId>
    <artifactId>spring-boot-starter-data-jpa</artifactId>
    <exclusions>
        <exclusion>
            <groupId>org.springframework</groupId>
            <artifactId>spring-tx</artifactId>
        </exclusion>
    </exclusions>
</dependency>
<!-- spring-tx 5.2.3.RELEASE is used by SpringBoot 2.2.4.RELEASE: -->
<dependency>
    <groupId>com.labun</groupId>
    <artifactId>spring-tx</artifactId>
    <version>5.2.3.RELEASE.patched</version>
</dependency>

By enabling transactions related logging

logging.level.org.springframework.transaction: trace

you can see the lines like (printed by AnnotationTransactionAttributeSource which inherits from AbstractFallbackTransactionAttributeSource)

Adding transactional method '<your class>.<your method>' with attribute: PROPAGATION_REQUIRED,ISOLATION_DEFAULT; '<your transaction manager'

and control that the attributes are as required.

You can also enable trace logging just for the AnnotationTransactionAttributeSource:

logging.level.org.springframework.transaction.annotation.AnnotationTransactionAttributeSource: trace

---

This works well for me so far for the persistence setup described at the beginning, i.e. user-defined DAO/service classes, no Spring Data interface based generated proxies. A solution for Spring Data has to be implemented yet. Feedback is welcome :)

---

And here is the new mergemethod:

/** Set empty properties of "primary" object from "secondary" object. */  
@Nullable
private TransactionAttribute merge(@Nullable TransactionAttribute primary, @Nullable TransactionAttribute secondary) {
	if (primary == null) return secondary;
	if (secondary == null) return primary;

	if (primary instanceof DefaultTransactionAttribute && secondary instanceof DefaultTransactionAttribute) {
		DefaultTransactionAttribute p = (DefaultTransactionAttribute) primary;
		DefaultTransactionAttribute s = (DefaultTransactionAttribute) secondary;

		if (p.getQualifier() == null || p.getQualifier().isEmpty()) p.setQualifier(s.getQualifier());
		if (p.getDescriptor() == null || p.getDescriptor().isEmpty()) p.setDescriptor(s.getDescriptor());
		if (p.getName() == null || p.getName().isEmpty()) p.setName(s.getName());
		// the following properties have default values in DefaultTransactionDefinition;
		// we cannot distinguish here, whether these values have been set explicitly or implicitly;
		// but it seems to be logical to handle default values like empty values
		if (p.getPropagationBehavior() == PROPAGATION_REQUIRED) p.setPropagationBehavior(s.getPropagationBehavior());
		if (p.getIsolationLevel() == ISOLATION_DEFAULT) p.setIsolationLevel(s.getIsolationLevel());
		if (p.getTimeout() == TIMEOUT_DEFAULT) p.setTimeout(s.getTimeout());
		if (p.isReadOnly() == false) p.setReadOnly(s.isReadOnly());
	}
	if (primary instanceof RuleBasedTransactionAttribute && secondary instanceof RuleBasedTransactionAttribute) {
		RuleBasedTransactionAttribute p = (RuleBasedTransactionAttribute) primary;
		RuleBasedTransactionAttribute s = (RuleBasedTransactionAttribute) secondary;

		if (p.getRollbackRules() == null || p.getRollbackRules().isEmpty()) p.setRollbackRules(s.getRollbackRules());
	}
	return primary;
}

[elab]

As I had troubles to setup IDE for the (whole) spring-framework project, the patch has been created directly from the spring-tx maven artifact sources. So, unfortunately, cannot currently do a pull request and a unit test :(

[sbrannen]

As I had troubles to setup IDE for the (whole) spring-framework project,

Have you followed the instructions outlined in Building from Source?

[elab]

@sbrannen Obviously, not enough... I will certainly give it another try, when I'll have more spare time (perhaps, second half of January).

[sbrannen]

OK. The instructions should be correct for both IntelliJ and Eclipse. I personally updated the Eclipse instructions very recently.

So if you run into instructions that do not work, please let us know so that we can update the documentation.