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.
Comment From: 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 #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.
- The first thing (which was already mentioned by @nachogil in #14011 and by @rsanwal in #17080) is to fix these lines to refer to the
targetClassinstead of thedeclaringClass, in accordance with the logic and all the javadoc and inline comments:
old version: https://github.com/spring-projects/spring-framework/blob/c8ef49cc8ecf7b4f7124e3e3987def5efb08cf9b/spring-tx/src/main/java/org/springframework/transaction/interceptor/AbstractFallbackTransactionAttributeSource.java#L167-L168
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.
- The second and the most important thing (inspired by @mattinger in #14295, also see problem description in #11839) is to merge settings from all relevant
@Transactionalannotations instead of returning the first found one:
old version: https://github.com/spring-projects/spring-framework/blob/c8ef49cc8ecf7b4f7124e3e3987def5efb08cf9b/spring-tx/src/main/java/org/springframework/transaction/interceptor/AbstractFallbackTransactionAttributeSource.java#L161-L186
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?
Comment From: 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 areAbstractFallbackTransactionAttributeSource.classandAbstractFallbackTransactionAttributeSource$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;
}
Comment From: 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 :(
Comment From: sbrannen
As I had troubles to setup IDE for the (whole) spring-framework project,
Have you followed the instructions outlined in Building from Source?
Comment From: elab
@sbrannen Obviously, not enough... I will certainly give it another try, when I'll have more spare time (perhaps, second half of January).
Comment From: 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.
Comment From: elab
The PR with patch and additional unit test for the initial use case is created.
I also added the "declaring class of the specific method" again to "priorities" (as it was in the original implementation), while also keeping the previously added "target class".
The new priorities are:
- specific method; (this is just the proper name of the former "method in the target class")
- declaring class of the specific method; (added again)
- target class; (added in the first version of the patch)
- method in the declaring class/interface;
- declaring class/interface.
As the new ("merging") behavior is not backward compatible, we would need some parameter to switch the implementation (TODO), e.g.
transaction-attribute-policy: {fallback, merge} # default: fallback
Comment From: elab
I finally managed to solve the problem without patching the spring-tx library (now, no need to create a patch for every new version of Spring):
The default "transactionAttributeSource" bean (defined in ProxyTransactionManagementConfiguration) will be replaced (1) by an instance of the own MergeAnnotationTransactionAttributeSource (2).
(1) AnnotationTransactionAttributeSourceReplacer
An important part was to implement the PriorityOrdered interface in the replacer, otherwise it was invoked too late (after instantiation of the "transactionAttributeSource" bean), see PostProcessorRegistrationDelegate#invokeBeanFactoryPostProcessors for details.
(Ordered interface would probably suffice, too.)
import org.springframework.beans.BeansException;
import org.springframework.beans.factory.config.InstantiationAwareBeanPostProcessor;
import org.springframework.core.PriorityOrdered;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.ProxyTransactionManagementConfiguration;
import org.springframework.transaction.interceptor.TransactionAttributeSource;
import lombok.extern.slf4j.Slf4j;
/**
* Replaces the default "transactionAttributeSource" bean (defined in {@link ProxyTransactionManagementConfiguration})
* with instance of {@link MergeAnnotationTransactionAttributeSource}.
*
* @author Eugen Labun
*/
@Slf4j
@Component
public class AnnotationTransactionAttributeSourceReplacer implements InstantiationAwareBeanPostProcessor, PriorityOrdered /*this is important*/ {
public AnnotationTransactionAttributeSourceReplacer() {
// to check that the replacer is created before instantiation of the "transactionAttributeSource" bean
log.trace("AnnotationTransactionAttributeSourceReplacer - constructor");
}
@Override
public Object postProcessBeforeInstantiation(Class<?> beanClass, String beanName) throws BeansException {
// log.trace("postProcessBeforeInstantiation - beanName: {}, beanClass: {}", beanName, beanClass);
if (beanName.equals("transactionAttributeSource") && TransactionAttributeSource.class.isAssignableFrom(beanClass)) {
log.debug("instantiating bean {} as {}", beanName, MergeAnnotationTransactionAttributeSource.class.getName());
return new MergeAnnotationTransactionAttributeSource();
} else {
return null;
}
}
@Override
public int getOrder() {
return 0;
}
}
(2) MergeAnnotationTransactionAttributeSource
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import org.springframework.aop.support.AopUtils;
import org.springframework.lang.Nullable;
import org.springframework.transaction.TransactionDefinition;
import org.springframework.transaction.annotation.AnnotationTransactionAttributeSource;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.interceptor.AbstractFallbackTransactionAttributeSource;
import org.springframework.transaction.interceptor.DefaultTransactionAttribute;
import org.springframework.transaction.interceptor.RuleBasedTransactionAttribute;
import org.springframework.transaction.interceptor.TransactionAttribute;
import org.springframework.util.ClassUtils;
import lombok.extern.slf4j.Slf4j;
/**
* Implements a merge policy for transaction attributes (see {@link Transactional} annotation)
* with following priorities (high to low):
* <ol>
* <li>specific method;
* <li>declaring class of the specific method;
* <li>target class;
* <li>method in the declaring class/interface;
* <li>declaring class/interface.
* </ol>
*
* <p>The merge policy means that all transaction attributes which are not
* explicitly set [1] on a specific definition place (see above) will be inherited
* from the place with the next lower priority.
*
* <p>On the contrary, the Spring default {@link AbstractFallbackTransactionAttributeSource} implements a fallback policy,
* where all attributes are read from the first found definition place (essentially in the above order), and all others are ignored.
*
* <p>See analysis in <a href="https://github.com/spring-projects/spring-framework/issues/24291">Inherited @Transactional methods use wrong TransactionManager</a>.
*
* <p>[1] If the value of an attribute is equal to its default value, the current implementation
* cannot distinguish, whether this value has been set explicitly or implicitly,
* and considers such attribute as "not explicitly set". Therefore it's currently impossible to override a non-default value with a default value.
*
* @author Eugen Labun
*/
@Slf4j
@SuppressWarnings("serial")
public class MergeAnnotationTransactionAttributeSource extends AnnotationTransactionAttributeSource {
public MergeAnnotationTransactionAttributeSource() {
log.info("MergeAnnotationTransactionAttributeSource constructor");
}
@Override
@Nullable
protected TransactionAttribute computeTransactionAttribute(Method method, @Nullable Class<?> targetClass) {
// Don't allow no-public methods as required.
if (allowPublicMethodsOnly() && !Modifier.isPublic(method.getModifiers())) {
return null;
}
// The method may be on an interface, but we also need attributes from the target class.
// If the target class is null, the method will be unchanged.
Method specificMethod = AopUtils.getMostSpecificMethod(method, targetClass);
// 1st priority is the specific method.
TransactionAttribute txAttr = findTransactionAttribute(specificMethod);
// 2nd priority is the declaring class of the specific method.
Class<?> declaringClass = specificMethod.getDeclaringClass();
boolean userLevelMethod = ClassUtils.isUserLevelMethod(method);
if (userLevelMethod) {
txAttr = merge(txAttr, findTransactionAttribute(declaringClass));
}
// 3rd priority is the target class
if (targetClass != null && !targetClass.equals(declaringClass) && userLevelMethod) {
txAttr = merge(txAttr, findTransactionAttribute(targetClass));
}
if (method != specificMethod) {
// 4th priority is the method in the declaring class/interface.
txAttr = merge(txAttr, findTransactionAttribute(method));
// 5th priority is the declaring class/interface.
txAttr = merge(txAttr, findTransactionAttribute(method.getDeclaringClass()));
}
return txAttr;
}
/**
* Set empty and default properties of "primary" object from "secondary" object.
* <p>Parameter objects should not be used after the call to this method,
* as they can be changed here or/and returned as a result.
*/
@Nullable
private TransactionAttribute merge(@Nullable TransactionAttribute primaryObj, @Nullable TransactionAttribute secondaryObj) {
if (primaryObj == null) {
return secondaryObj;
}
if (secondaryObj == null) {
return primaryObj;
}
if (primaryObj instanceof DefaultTransactionAttribute && secondaryObj instanceof DefaultTransactionAttribute) {
DefaultTransactionAttribute primary = (DefaultTransactionAttribute) primaryObj;
DefaultTransactionAttribute secondary = (DefaultTransactionAttribute) secondaryObj;
if (primary.getQualifier() == null || primary.getQualifier().isEmpty()) {
primary.setQualifier(secondary.getQualifier());
}
if (primary.getDescriptor() == null || primary.getDescriptor().isEmpty()) {
primary.setDescriptor(secondary.getDescriptor());
}
if (primary.getName() == null || primary.getName().isEmpty()) {
primary.setName(secondary.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 (primary.getPropagationBehavior() == TransactionDefinition.PROPAGATION_REQUIRED) {
primary.setPropagationBehavior(secondary.getPropagationBehavior());
}
if (primary.getIsolationLevel() == TransactionDefinition.ISOLATION_DEFAULT) {
primary.setIsolationLevel(secondary.getIsolationLevel());
}
if (primary.getTimeout() == TransactionDefinition.TIMEOUT_DEFAULT) {
primary.setTimeout(secondary.getTimeout());
}
if (!primary.isReadOnly()) {
primary.setReadOnly(secondary.isReadOnly());
}
}
if (primaryObj instanceof RuleBasedTransactionAttribute && secondaryObj instanceof RuleBasedTransactionAttribute) {
RuleBasedTransactionAttribute primary = (RuleBasedTransactionAttribute) primaryObj;
RuleBasedTransactionAttribute secondary = (RuleBasedTransactionAttribute) secondaryObj;
if (primary.getRollbackRules() == null || primary.getRollbackRules().isEmpty()) {
primary.setRollbackRules(secondary.getRollbackRules());
}
}
return primaryObj;
}
}
Comment From: kurenchuksergey
Hello. I found this issue, because I have met strange behaviour. I just have a simple structure
Class A {
@Transaction
void method()
}
@Transaction(readOnly = true)
class B exntend A {
}
And when I invoke 'method' on instance 'B', I still get transactionAttributes from the super class. Is it correct behaviour sbrannen ?
@elab Did you continue work on PR? Did you think about rewriting primary transaction attributes by any override (In your code it is secondaryObj).
Comment From: elab
Did you think about rewriting primary transaction attributes by any override (In your code it is secondaryObj).
Could you give an example?
Comment From: kurenchuksergey
Did you think about rewriting primary transaction attributes by any override (In your code it is secondaryObj).
Could you give an example?
Yes, of course. I will improve the example
Fistful DAO structures:
@Component
Class ReadOnlyRepository {
@Transaction(readOnly = true)
void read()
}
@Component
@Transaction(readOnly = false)
class MasterRepository exntend ReadOnlyRepository {
}
And I invokes read method on the MasterRepository instance:
applicationContext.get(MasterRepository.class).read() //like
then my attribute in TransactionAttribute still will be readOnly = true, but of course it isn't what I suspect when annotated MasterRepository by additional annotation
Comment From: elab
OK, I understand your thought.
What about
@Component
class ReadOnlyRepository {
@Transactional(readOnly = true)
void read()
}
@Component
@Transactional
class MasterRepository extends ReadOnlyRepository {
}
(I removed readOnly = false on the MasterRepository.)
Which value of TransactionAttribute.readOnly would you expect now for masterRepository.read() ?
Comment From: kurenchuksergey
OK, I understand your thought.
What about
``` @Component class ReadOnlyRepository { @Transactional(readOnly = true) void read() }
@Component @Transactional class MasterRepository extends ReadOnlyRepository { } ```
(I removed
readOnly = falseon the MasterRepository.)Which value of TransactionAttribute.readOnly would you expect now for masterRepository.read() ?
I think it still should be readOnly = false behaviour, because it is default value for annotation.
Comment From: elab
@kurenchuksergey Sorry for the delay.
The "override" behavior you wrote about is essentially how the standard Spring library works: The @Transactional annotation with the highest priority "overrides" all other occurrences of that annotation. All the transaction attributes (defined explicitly and defaults for others) will be taken from that first found annotation.
Note, that the highest priority, according to the logic in AbstractFallbackTransactionAttributeSource#computeTransactionAttribute, has the "specific method" (i.e. the implementation - ReadOnlyRepository#read in your example), and not the "target class" (i.e. the class whose instance has been used to invoke the method - MasterRepository in your example).
On the contrary, the MergeAnnotationTransactionAttributeSource discussed in this topic tries to "merge" the transactional attributes from multiple @Transactional annotations (ordered according to their defined priorities, see javadoc above). The highest priority still has the specific method. Thereafter, the annotation on the target class will be considered as well, but it has no impact in your example, as this annotation doesn't explicitly define additional attributes.
Comment From: jhoeller
It looks like we'll address both this and #23473 through a new @TransactionConfig annotation, analogous to our existing @CacheConfig, or similar global settings which are not trying to rely on class-level @Transactional declarations (which brings in the undesirable implicit transactional-for-all-public-methods semantic whereas we only try to have certain settings shared for methods which are explicitly marked as transactional).
Comment From: jhoeller
As a rather design-oriented measure, we are going to take type-level @Qualifier("...") values into account for transaction manager selection, effectively serving as a @Transactional("...") qualifier value for transaction definitions without such a specific annotation-declared value (for local as well as inherited methods). In contrast to transaction-specific qualifiers, this is considered an optional hint; if no transaction manager matches that qualifier, we proceed with default transaction manager selection.
Such a type-level bean qualifier can serve multiple purposes, e.g. with a value of "accountdb" it can be used for autowiring purposes (identifying the account repository) as well as transaction manager selection, as long as the target beans for autowiring as well as the associated transaction manager definitions declare the same qualifier value. While this technically also works against target bean names, it is recommended to use custom @Qualifier("...") values on the target beans as well. Qualifier values can be descriptive ("accountdb" on both a repository and a transaction manager) rather than id style ("accountRepository" and "accountTransactionManager"); after all, such qualifier values only need to be unique with a set of type-matching beans.
We do not recommend any use of type-level @Transactional definitions in such scenarios, and in particular no custom merging with method-level definitions. Individual transaction definitions are always clearer per method. If you'd like to avoid repetition of specific attributes beyond the transaction manager qualifier, consider defining your own @MyTransactional variant (meta-annotated with Spring's @Transactional and pre-defining some specific attribute values there).