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

bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect #5092

Merged
merged 9 commits into from
Nov 23, 2022

Conversation

zhuyoufeng
Copy link
Contributor

@zhuyoufeng zhuyoufeng commented Nov 18, 2022

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

This PR is used to ensure springboot create correct transaction manager for different ORM framework.

The original code shows that this configuration SeataTCCFenceAutoConfiguration must be executed after DataSourceTransactionManagerAutoConfiguration, but for springboot, different transaction manager for different ORM framework. this DataSourceTransactionManagerAutoConfiguration will create DataSourceTransactionManager which works for jdbc related ORM, not work for JPA.

  • JPA: JpaTransactionManager
  • Hibernate: HibernateTransactionManager
  • JDBC: DataSourceTransactionManager (actually the configuration DataSourceTransactionManagerAutoConfiguration is implemented with Ordered.LOWEST_PRECEDENCE)

pic1

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Removed this config. It will be work because this config SeataTCCFenceAutoConfiguration already set the "ConditionalOnBean" with "org.springframework.transaction.PlatformTransactionManager". And the order is correct now.

pic2

Ⅴ. Special notes for reviews

…ate correct transaction manager for different orm framework.
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #5092 (c0d4f8d) into develop (1966c11) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5092      +/-   ##
=============================================
- Coverage      49.25%   49.23%   -0.02%     
+ Complexity      4131     4130       -1     
=============================================
  Files            737      737              
  Lines          26188    26188              
  Branches        3234     3234              
=============================================
- Hits           12898    12894       -4     
- Misses         11913    11915       +2     
- Partials        1377     1379       +2     
Impacted Files Coverage Δ
.../autoconfigure/SeataTCCFenceAutoConfiguration.java 0.00% <ø> (ø)
...n/src/main/java/io/seata/common/util/IdWorker.java 77.08% <0.00%> (-6.25%) ⬇️
...erver/storage/file/session/FileSessionManager.java 49.04% <0.00%> (-0.64%) ⬇️

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM please register pr to develop.md https://github.com/seata/seata/tree/develop/changes

@funky-eyes funky-eyes added this to the 1.6.0 milestone Nov 21, 2022
@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. module/seata-spring-boot-starter seata-spring-boot-starter module labels Nov 21, 2022
@wangliang181230
Copy link
Contributor

问下,在修改前,会报什么样的错误?麻烦提供下异常日志信息吧。

@zhuyoufeng
Copy link
Contributor Author

zhuyoufeng commented Nov 21, 2022

@wangliang181230
这个不会报错,只是使用JPA的时候,事务无法提交,因为 SeataTCCFenceAutoConfiguration 中AutoConfigureAfter 的使用 使springboot 创建 transactionmanager 的顺序发生了变化,使用JPA的时候创建出来的transactionmanager不正确。这边是我测试的例子:https://github.com/zhuyoufeng/seata-study

11

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhuyoufeng
Copy link
Contributor Author

zhuyoufeng commented Nov 22, 2022

@wangliang181230
我明白你的意思了,如果单纯这样改动的话,ConfigurationClassBeanDefinitionReader.loadBeanDefinitionsForConfigurationClass 会因为 @ConditionalOnBean(type = {"javax.sql.DataSource", "org.springframework.transaction.PlatformTransactionManager"}) 这个条件不成立 而将 SeataTCCFenceAutoConfiguration 整个配置skip掉。我的想法是 将 @ConditionalOnBean(type = {"javax.sql.DataSource", "org.springframework.transaction.PlatformTransactionManager"}) 这个条件去除,因为springboot在创建 tccFenceConfig 的时候,会自根据依赖关系先创建 datasourcetransactionmanager,在成功之后再调用 SeataTCCFenceAutoConfiguration 创建 TCCFenceConfig

image

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

我的想法是 将 @ConditionalOnBean(type = {"javax.sql.DataSource", "org.springframework.transaction.PlatformTransactionManager"}) 这个条件去除,因为 springboot 在创建 tccFenceConfig 的时候,会自根据依赖关系先创建 datasourcetransactionmanager,在成功之后再调用 SeataTCCFenceAutoConfiguration 创建 TCCFenceConfig

@zhuyoufeng 这样不行,只有TM没有数据源的服务,会启动失败。
我建议,添加一个 resources/META-INF/spring-autoconfigure-metadata.properties
内容如下:

org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration=
org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration.AutoConfigureBefore=org.springframework.boot.autoconfigure.jdbc.DataSourceTransactionManagerAutoConfiguration

强制将 jpa 的auto 调整到 jdbc 的前面。

我在你的测试代码里亲测有效。

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

另外,建议去jpa的开源项目里提个issue。推荐他们添加该文件。
或直接推荐他们添加 @AutoConfigureBefore(DataSourceTransactionManagerAutoConfiguration.class)

@zhuyoufeng
Copy link
Contributor Author

zhuyoufeng commented Nov 22, 2022

这个不正确吧,没有datasource,transactionmanager也不会创建成功啊。我依然认为你不应该判断是否存在bean,或可以判断conditiononclass,因为强制这样控制顺序,破坏的是加载顺序

并且,个人认为开发者是在引入seata之后出现的transactionmanager不正确的情况,不应该由开发人员去处理其加载顺序。

@wangliang181230
Copy link
Contributor

图片

经过测试,只使用TM功能的,是启动不了的。

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

这个不正确吧,没有datasource,transactionmanager也不会创建成功啊。我依然认为你不应该判断是否存在bean,或可以判断conditiononclass,因为强制这样控制顺序,破坏的是加载顺序

并且,个人认为开发者是在引入seata之后出现的transactionmanager不正确的情况,不应该由开发人员去处理其加载顺序。

对于seata自身来说,tcc的auto类的定义是没有问题的,它的确是依赖于bean,而不是class。因为有class但用户并没有创建bean时,这个tcc的auto也不应生效,所以不推荐改成OnClass。

正如你说的,jpa的加载顺序因为引入了seata,而破坏了jpa与jdbc的加载顺序。但也不应该因为引入了jpa而破坏了seata与jdbc(或transactionManager的bean创建者)的加载顺序吧?

目前最明确的问题的确是jpa没有显式定义其加载顺序before于jdbc,而导致的问题,并不是因为引入了seata而破坏了其加载顺序,因为seata自身的定义并没有问题,这有点把责任推给了seata的感觉。
对于jpa自身的auto类来说,它的确应该before于jdbc的,否则jpa将无法正常使用。所以应该是调整jpa的顺序before于jdbc.

@zhuyoufeng
Copy link
Contributor Author

DataSourceTransactionManagerAutoConfiguration 中已经使用了 AutoConfigureOrder 控制顺序了,只是这个顺序被seata的starter破坏了,你可以试一下你们 1.4.2 的版本有没有这个问题。

如果你认为没必要,可以 reject。

@wangliang181230
Copy link
Contributor

经过测试,使用你的解决方案,tcc的auto,在你的项目中的确没有生效了。验证了前面的疑问。

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

DataSourceTransactionManagerAutoConfiguration 中已经使用了 AutoConfigureOrder 控制顺序了,只是这个顺序被seata的starter破坏了,你可以试一下你们 1.4.2 的版本有没有这个问题。

因为1.4.2还没有tcc的auto类。

如果你认为没必要,可以 reject。

并不是没必要,seata1.5.2和jpa的确存在冲突,是需要寻找最佳解决方案的。目前正在跟你讨论呢。

@wangliang181230
Copy link
Contributor

在你的测试代码里,经过几种尝试,还是添加 spring-autoconfigure-metadata.properties 能够解决所有问题。

你是否有其他的解决方案?

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

我们先罗列一下需要解决的问题吧,这样比较清晰一些。
以你的例子作为测试项目:https://github.com/zhuyoufeng/seata-study

  1. seata在只使用TM,没有 dataSource 的项目中,要能够正常启动。
  2. seata的 SeataTCCFenceAutoConfiguration 在存在 dataSourcetransactionManager 的bean时,应生效。
  3. 业务端使用的 transactionManager 应是 jpa 创建的。
  4. TCCFenceConfig 加载的 transactionManager 也应是 jpa 创建的。

@zhuyoufeng
Copy link
Contributor Author

我的想法还是移除 @ConditionalOnBean(type = {"javax.sql.DataSource", "org.springframework.transaction.PlatformTransactionManager"}) 这个代码,其他的暂时没有。

理由:

  1. 虽然增加 resources/META-INF/spring-autoconfigure-metadata.properties 可以解决问题,我个人还是觉得这样破坏了原来的加载顺序。
  2. 我个人觉得datasource和TM是seata工作的保证,如果项目没有存在正确的datasource 和 TM,是不是确实应该报错来告诉开发人员项目不正确。(也可能我对seata的了解还不够)

@wangliang181230
Copy link
Contributor

@zhuyoufeng 你认为的原来的加载顺序,就是jpa在jdbc之前啊。

@wangliang181230
Copy link
Contributor

我的想法还是移除 @ConditionalOnBean(type = {"javax.sql.DataSource", "org.springframework.transaction.PlatformTransactionManager"}) 这个代码,其他的暂时没有。

理由:

1. 虽然增加 resources/META-INF/spring-autoconfigure-metadata.properties 可以解决问题,我个人还是觉得这样破坏了原来的加载顺序。

2. 我个人觉得datasource和TM是seata工作的保证,如果项目没有存在正确的datasource 和 TM,是不是确实应该报错来告诉开发人员项目不正确。(也可能我对seata的了解还不够)

部分公司的数据源都是在下层dao服务层的,上层是提供API的,API层只使用seata的TM,并不使用RM。

@zhuyoufeng zhuyoufeng closed this Nov 22, 2022
@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

我们先罗列一下需要解决的问题吧,这样比较清晰一些。
以你的例子作为测试项目:https://github.com/zhuyoufeng/seata-study

1. seata在只使用TM,没有 `dataSource` 的项目中,要能够正常启动。
2. seata的 `SeataTCCFenceAutoConfiguration` 在存在 `dataSource` 和 `transactionManager` 的bean时,应生效。
3. 业务端使用的 `transactionManager` 应是 jpa 创建的。
4. `TCCFenceConfig` 加载的 `transactionManager` 也应是 jpa 创建的。

@slievrly @a364176773
基于以上几个需考虑的问题,两位再review一下代码吧。

然后再看下,我提供的建议:

我建议,添加一个 resources/META-INF/spring-autoconfigure-metadata.properties 内容如下:

org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration=
org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration.AutoConfigureBefore=org.springframework.boot.autoconfigure.jdbc.DataSourceTransactionManagerAutoConfiguration

强制将 jpa 的auto 调整到 jdbc 的前面。

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 22, 2022

@zhuyoufeng 想到个好办法。

  1. @ConditionalOnBean 改成 @ConditionalOnClass
  2. 依赖的 dataSourcetransactionManager 前面加 @Autowired(required = false) ,让 TccFenceConfig 的bean继续创建。
  3. @AutoConfigureAfter继续根据你的PR来,移除那两个auto.
  4. TccFenceConfig 里,如果 dataSourcetransactionManager 不存在,那么里面的逻辑都为空。然后在这种情况下还被使用时,直接在逻辑前抛异常。

这样的话就不需要等jpa那修改,也能够兼容了。

@zhuyoufeng
Copy link
Contributor Author

@zhuyoufeng 想到个好办法。

  1. @ConditionalOnBean 改成 @ConditionalOnClass

  2. 依赖的 dataSourcetransactionManager 前面加 @Autowired(required = false) ,让 TccFenceConfig 的bean继续创建。

  3. @AutoConfigureAfter继续根据你的PR来,移除那两个auto.

  4. TccFenceConfig 里,如果 dataSourcetransactionManager 不存在,那么里面的逻辑都为空。然后在这种情况下还被使用时,直接在逻辑前抛异常。

这样的话就不需要等jpa那修改,也能够兼容了。

我试试

@zhuyoufeng
Copy link
Contributor Author

@wangliang181230
我想应该这样可行,不应该特别指定after在某个transactionmanager 的 autoconfiguration之后,只要确保这个config在 TransactionAutoConfiguration 之后执行即可。你看下是否可行。

@AutoConfigureAfter({SeataCoreAutoConfiguration.class, TransactionAutoConfiguration.class})

image

@wangliang181230
Copy link
Contributor

昨天在 springboot 那边提了 issue,然后经过他们的解答,发现我一直误解了。原来auto的order默认值是0。

所以,这个BUG的解决方案是在 SeataTCCFenceAutoConfiguration 上加 @AutoConfigureOrder(Ordered.LOWEST_PRECEDENCE) 就行了。不用这么麻烦了。

麻烦再改一版本吧。

@zhuyoufeng
Copy link
Contributor Author

@wangliang181230 Done.

@wangliang181230
Copy link
Contributor

wangliang181230 commented Nov 23, 2022

@zhuyoufeng 不好意思,刚看了一下你上次提交的那个,也是可以的。

麻烦两个都加着吧。

  1. @AutoConfigureOrder(Ordered.LOWEST_PRECEDENCE)
  2. after 于 TransactionAutoConfiguration.class

这样更保险。

@wangliang181230 wangliang181230 changed the title bugfix: remove "DataSourceTransactionManagerAutoConfiguration" to cre… bugfix: remove "DataSourceTransactionManagerAutoConfiguration" to create correct transaction manager for different orm framework Nov 23, 2022
@wangliang181230 wangliang181230 changed the title bugfix: remove "DataSourceTransactionManagerAutoConfiguration" to create correct transaction manager for different orm framework bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect Nov 23, 2022
@wangliang181230 wangliang181230 changed the title bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect Nov 23, 2022
@wangliang181230 wangliang181230 changed the title bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect bugfix: fix the order of transaction manager creation not correct Nov 23, 2022
Copy link
Contributor

@wangliang181230 wangliang181230 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wangliang181230 wangliang181230 changed the title bugfix: fix the order of transaction manager creation not correct bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect Nov 23, 2022
@zhuyoufeng
Copy link
Contributor Author

@wangliang181230 好了,你看下。

@wangliang181230
Copy link
Contributor

@wangliang181230 好了,你看下。

嗯,可以了,稍后我merge下。

@wangliang181230
Copy link
Contributor

1.6.0 快发布了,到时候你可以升级上去。

@wangliang181230 wangliang181230 merged commit cda49cc into apache:develop Nov 23, 2022
@zhuyoufeng
Copy link
Contributor Author

1.6.0 快发布了,到时候你可以升级上去。

好的,谢谢

@wangliang181230
Copy link
Contributor

1.6.0-RC1 候选版本先发布了,你可以先试用看看。

@zhuyoufeng
Copy link
Contributor Author

1.6.0-RC1 候选版本先发布了,你可以先试用看看。

👌

zw201913 pushed a commit to zw201913/seata that referenced this pull request Dec 12, 2022
…o 1114_for_5073

* '1114_for_5073' of https://github.com/zw201913/seata:
  bugfix: hikari datasource auto proxy fail (apache#5134)
  bugfix: rollback active xa connection fail (apache#5131)
  optimize: support oracle on delete tccfence logs  (apache#5124)
  feature: support passing `contextPath` parameter to Nacos client (apache#5111)
  bugfix:NPE caused when there is no @GlobalTransactional annotation on the RM side  (apache#5109)
  bugfix: Druid disable oracle implicit cache (apache#5098)
  bugfix: fix access key loss after server restart (apache#5097)
  optimize: remove druid dependency in ConnectionProxy (apache#5104)
  bugfix:fix ClassNotFoundException during the ZK unit test (apache#5101)
  bugfix: fix when seata and jpa are used together, their AutoConfiguration order is incorrect (apache#5092)
  optimize: lock priority attempts to insert (apache#4681)
  bugfix: update join condition placeholder param error (apache#5052)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/seata-spring-boot-starter seata-spring-boot-starter module type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants