Skip to content

Align TransactionManagementConfigurer support in TestContext framework with production #24869

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

Closed
friscoMad opened this issue Apr 6, 2020 · 6 comments
Assignees
Labels
in: test Issues in the test module type: enhancement A general enhancement
Milestone

Comments

@friscoMad
Copy link

friscoMad commented Apr 6, 2020

Affects: Spring Framework 4.3.0+

When @Primary support was added for spring-test in 677a321, the lookup was added before TransactionManagementConfigurer lookup and not after as it should have been, given that TransactionManagementConfigurer should take higher precedence. In fact maybe it should be the first check even before the beansOfTypeIncludingAncestors lookup as the configurer can construct a TransactionManager that is not a registered bean (a registered bean could be in the context that is not the one that the configurer will return).

Steps to reproduce

Configure a context with a @Primary TransactionManager and a TransactionManagementConfigurer.

Current result

Test uses the @Primary annotated bean.

Expected result

Test should use the bean provided by TransactionManagementConfigurer.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 6, 2020
@sbrannen sbrannen added the in: test Issues in the test module label Apr 6, 2020
@sbrannen sbrannen changed the title Spring-Test does not honor TransactionManagementConfigurer where there is a @Primary TransactionManager TestContext framework does not honor TransactionManagementConfigurer when there is a @Primary TransactionManager Apr 6, 2020
sbrannen added a commit that referenced this issue Apr 6, 2020
This commit introduces tests for the status quo in production support
for multiple transaction managers registered as @primary and via the
TransactionManagementConfigurer API.

See gh-24869
sbrannen added a commit that referenced this issue Apr 6, 2020
This commit introduces an integration test for the status quo in the
Spring TestContext Framework (TCF) for multiple transaction managers
registered as @primary and via the TransactionManagementConfigurer API.

In Spring Framework 5.3 we will revise the transaction manager lookup in
TestContextTransactionUtils so that the transaction manager configured
via the TransactionManagementConfigurer API is favored over a @primary
transaction manager.

See gh-24869
@sbrannen
Copy link
Member

sbrannen commented Apr 6, 2020

Thank you for bringing this to our attention! 👍

There is indeed a discrepancy between the transaction manager lookup algorithm in production code and the algorithm used within the Spring TestContext Framework (TCF).

I introduced tests for the status quo for production code in 144b0e1 and for the TCF in a5498ba.

In Spring Framework 5.3 we will revise the transaction manager lookup in TestContextTransactionUtils so that a transaction manager configured via the TransactionManagementConfigurer API is favored over a @Primary transaction manager.

@sbrannen sbrannen self-assigned this Apr 6, 2020
@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 6, 2020
@sbrannen sbrannen added this to the 5.3 M1 milestone Apr 6, 2020
@friscoMad
Copy link
Author

Thank you for such fast reply.
I know that this situation is a fringe case (I have several configurations in my classpath that I can not remove) and probably not a lot of projects are using TransactionManagementConfigurer
I did some more test after the report to confirm that in production it does work as it should and I think that the lookup is here and has this priority:

  1. Qualified
  2. TransactionManagementConfigurer
  3. Regular bean search (@primary or single one)

@sbrannen
Copy link
Member

sbrannen commented May 9, 2020

This has been addressed in 613bd3b.

Feel free to try it out in an upcoming 5.3 M1 snapshot.

@friscoMad
Copy link
Author

I will try as soon as posible but while the change does fix my initial report and my use case, it still does not replicate production transaction manager lookup, at least by my understanding looking at production code, the TransactionManagementConfigurer is the top priority unless the annotation is qualified, so by my understanding the code block in lines 187-191 (single transaction manager bean) should be moved after TransactionManagementConfigurer block

@sbrannen
Copy link
Member

Reopening to align with production behavior.

@sbrannen sbrannen reopened this May 11, 2020
@sbrannen sbrannen changed the title TestContext framework does not honor TransactionManagementConfigurer when there is a @Primary TransactionManager Align TransactionManagementConfigurer support in TestContext framework with production May 11, 2020
@sbrannen
Copy link
Member

@friscoMad, thanks for the feedback.

it still does not replicate production transaction manager lookup, at least by my understanding looking at production code, the TransactionManagementConfigurer is the top priority unless the annotation is qualified.

According to the Javadoc for TransactionManagementConfigurer.annotationDrivenTransactionManager():

it is important that the PlatformTransactionManager instance is managed as a Spring bean within the container since most PlatformTransactionManager implementations take advantage of Spring lifecycle callbacks such as InitializingBean and BeanFactoryAware.

Thus, although the scenario you have described is highly discouraged, it is technically possible and supported in production arrangements.

I have therefore further refined the transaction manager lookup in the TestContext framework in 715e8c9.

kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
Prior to this commit, the Spring TestContext Framework (TCF) favored a
@primary transaction manger over one configured via the
TransactionManagementConfigurer API, and this contradicts the behavior
in production Spring applications.

This commit aligns the transaction manger lookup within the TCF so that
a transaction manger configured via the TransactionManagementConfigurer
API is properly favored over a @primary transaction manager.

Closes spring-projectsgh-24869
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
This commit picks up where 613bd3b
left off by ensuring that a transaction manager configured via the
TransactionManagementConfigurer API takes precedence over any
transaction manager configured as a bean in the ApplicationContext
unless @transactional is configured with a qualifier for the explicit
transaction manager to use in tests.

Closes spring-projectsgh-24869
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants