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

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

Closed
elab opened this issue Jan 3, 2020 · 16 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Milestone

Comments

@elab
Copy link

elab commented Jan 3, 2020

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 3, 2020
@elab
Copy link
Author

elab commented Jan 5, 2020

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:

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:

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

  1. 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:

// 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
Copy link
Author

elab commented Jan 8, 2020

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

(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
Copy link
Author

elab commented Jan 8, 2020

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
Copy link
Member

sbrannen commented Jan 9, 2020

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

Have you followed the instructions outlined in Building from Source?

@elab
Copy link
Author

elab commented Jan 9, 2020

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

@sbrannen
Copy link
Member

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 added a commit to elab/spring-framework that referenced this issue Feb 22, 2020
(in accordance with the logic and all the javadoc and inline comments)

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 22, 2020
Merge settings from all relevant @transactional annotations in
inheritance tree (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 23, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 23, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 23, 2020
elab added a commit to elab/spring-framework that referenced this issue Feb 23, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 23, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Feb 27, 2020
elab added a commit to elab/spring-framework that referenced this issue Feb 27, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Mar 10, 2020
spring-projects#24291 :

- base DAO class
- concrete DAO classes, which declare different transaction managers and
inherit transactional methods from the base DAO class
elab added a commit to elab/spring-framework that referenced this issue Mar 10, 2020
elab added a commit to elab/spring-framework that referenced this issue Mar 10, 2020
Merge transactional attributes from all relevant definition places,
according to their priority (instead of returning the first found one).

See spring-projectsgh-24291
elab added a commit to elab/spring-framework that referenced this issue Mar 10, 2020
spring-projects#24291 :

- base DAO class
- concrete DAO classes, which declare different transaction managers and
inherit transactional methods from the base DAO class
@elab
Copy link
Author

elab commented Mar 10, 2020

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 added a commit to elab/spring-framework that referenced this issue Mar 10, 2020
spring-projects#24291 :

- base DAO class
- concrete DAO classes, which declare different transaction managers and
inherit transactional methods from the base DAO class
@elab
Copy link
Author

elab commented Apr 23, 2020

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

}

@rstoyanchev rstoyanchev added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Nov 10, 2021
@kurenchuksergey
Copy link

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

@elab
Copy link
Author

elab commented Apr 10, 2022

Did you think about rewriting primary transaction attributes by any override (In your code it is secondaryObj).

Could you give an example?

@kurenchuksergey
Copy link

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

@elab
Copy link
Author

elab commented Apr 10, 2022

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() ?

@kurenchuksergey
Copy link

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() ?

I think it still should be readOnly = false behaviour, because it is default value for annotation.

@elab
Copy link
Author

elab commented Apr 30, 2023

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

@jhoeller jhoeller self-assigned this Aug 23, 2023
@jhoeller jhoeller added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 23, 2023
@jhoeller jhoeller added this to the 6.1.0-RC1 milestone Aug 23, 2023
@jhoeller jhoeller modified the milestones: 6.1.0-M5, 6.1.0-RC1 Sep 12, 2023
@jhoeller jhoeller changed the title Inherited @Transactional methods use wrong TransactionManager Inherited @Transactional methods should be able to use class-level TransactionManager Sep 27, 2023
@jhoeller jhoeller modified the milestones: 6.1.0-RC1, 6.2.x Sep 27, 2023
@jhoeller
Copy link
Contributor

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

@sbrannen sbrannen changed the title Inherited @Transactional methods should be able to use class-level TransactionManager Inherited @Transactional methods should be able to use class-level TransactionManager Jan 22, 2024
@snicoll snicoll self-assigned this Feb 1, 2024
@bclozel bclozel changed the title Inherited @Transactional methods should be able to use class-level TransactionManager Inherited @Transactional methods should be able to use class-level TransactionManager Feb 14, 2024
@jhoeller jhoeller changed the title Inherited @Transactional methods should be able to use class-level TransactionManager Inherited @Transactional methods should be able to use class-level TransactionManager qualifier from concrete class Mar 6, 2024
@jhoeller jhoeller modified the milestones: 6.2.x, 6.2.0-M1 Mar 6, 2024
@jhoeller
Copy link
Contributor

jhoeller commented Mar 6, 2024

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

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

No branches or pull requests

7 participants