Spring-framework: Inherited @Transactional methods use wrong TransactionManager

[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 #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 #14011 and by @rsanwal in #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:
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._

1. 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 @Transactional annotations 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?

[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.

[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:

1. specific method; (this is just the proper name of the former "method in the target class")
2. declaring class of the specific method; (added again)
3. target class; (added in the first version of the patch)
4. method in the declaring class/interface;
5. 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

[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;
    }

}