optimize: Prohibit registration of TCC resources with the same name #6091

PleaseGiveMeTheCoke · 2023-11-28T12:52:42Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

Check when registering TCC resources, if a resource with the same name already exists, determine if it is a parent-child relationship, if not, prevent the application from starting by throwing an exception.

注册TCC资源时进行检查，如果已经存在相同名称的资源，判断是否是父子类关系，如果不是，通过抛出异常的方式阻止应用启动

Ⅱ. Does this pull request fix one issue?

#6088

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2023-11-28T13:01:50Z

Codecov Report

Merging #6091 (f6d5517) into 2.x (5563492) will decrease coverage by 0.03%.
The diff coverage is 29.16%.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #6091      +/-   ##
============================================
- Coverage     48.94%   48.91%   -0.03%     
- Complexity     4775     4779       +4     
============================================
  Files           913      914       +1     
  Lines         31710    31728      +18     
  Branches       3827     3829       +2     
============================================
  Hits          15521    15521              
- Misses        14653    14661       +8     
- Partials       1536     1546      +10

Files	Coverage Δ
.../common/exception/RepeatRegistrationException.java	`0.00% <0.00%> (ø)`
.../main/java/io/seata/rm/tcc/TCCResourceManager.java	`15.00% <38.88%> (+3.23%)`	⬆️

... and 6 files with indirect coverage changes

funky-eyes

LGTM

leizhiyuan · 2023-11-29T09:24:33Z

please resolve conflicts

wangliang181230 · 2023-11-30T09:46:31Z

tcc/src/main/java/io/seata/rm/tcc/TCCResourceManager.java

+            Class<?> newResourceClass = newResource.getTargetBean().getClass();
+            Class<?> oldResourceClass = oldResource.getTargetBean().getClass();
+            if (!newResourceClass.isAssignableFrom(oldResourceClass)
+                    && !oldResourceClass.isAssignableFrom(newResourceClass)) {


只要存在相同的resourceId，是不是可以直接抛出异常？同一个 TCCResource 应该不会注册多次吧？
如果担心重复注册时误判导致抛出该异常，那你可以判断一下新老资源是否为同一个实例。

Is it possible to throw an exception as long as the same resourceId exists? The same TCCResource should not be registered multiple times, right？
If you are worried about duplicate registration, just check if the registered resource and the new resource are the same object.

只要存在相同的resourceId，是不是可以直接抛出异常？同一个 TCCResource 应该不会注册多次吧？如果担心重复注册时误判导致抛出该异常，那你可以判断一下新老资源是否为同一个实例。

Is it possible to throw an exception as long as the same resourceId exists? The same TCCResource should not be registered multiple times, right？ If you are worried about duplicate registration, just check if the registered resource and the new resource are the same object.

@funky-eyes 之前说过有可能在父子类的情况下注册多次

@funky-eyes said before it's possible to register multiple times in the case of parent-child classes

只判断是否父子关系，是无法判断注册进来的资源内部的属性。如果两个资源的属性不一致，那么它们的逻辑也很可能是不一样的，很可能是用户自己重复定义了 resourceId。

Only determining whether there is a parent-child relationship cannot determine the internal attributes of the registered resources.If the value of there fields are inconsistent, their logic is also likely to be different, and it is likely that the user has repeatedly defined the resourceId.

wangliang181230 · 2023-11-30T09:55:09Z

tcc/src/main/java/io/seata/rm/tcc/TCCResourceManager.java

+            Class<?> oldResourceClass = oldResource.getTargetBean().getClass();
+            if (!newResourceClass.isAssignableFrom(oldResourceClass)
+                    && !oldResourceClass.isAssignableFrom(newResourceClass)) {
+                throw new ShouldNeverHappenException(String.format("Same TCC resource name between class <%s> and class <%s>, should be unique",


异常类型换一个吧。ShouldNeverHappenException 只有在 前置操作未执行过 时才使用。
但这里明显不是因为前置操作未执行引起的。

Change the exception type. The ShouldNeverHappenException is only used when the preceding operation has not been executed.

Change the exception type. The ShouldNeverHappenException is only used when the preceding operation has not been executed.

异常类型换一个吧。ShouldNeverHappenException 只有在 前置操作未执行过 时才使用。但这里明显不是因为前置操作未执行引起的。

我查看了几处使用示例，比如
I looked at a couple of example uses, such as

看起来好像这个异常普遍用在发生系统规则之外情况的时候？或者说你觉得哪种异常更适合被用在这里呢
It seems like this exception is commonly used when something happens outside of the system rules? Or which exception do you think would be more appropriate to use here?

你可以自定义一个异常类。

You can customize an exception class.

wangliang181230 · 2023-12-01T04:20:14Z

tcc/src/main/java/io/seata/rm/tcc/TCCResourceManager.java

+        if (oldResource != null) {
+            Class<?> newResourceClass = newResource.getTargetBean().getClass();
+            Class<?> oldResourceClass = oldResource.getTargetBean().getClass();
+            if (newResourceClass != oldResourceClass) {


the targetBean, not the targetBeanClass.

wangliang181230

LGTM

slievrly

From the perspective of internal practical experience, share my suggestions:

The resource name should be globally unique.
Annotations to the interface, do not allow multiple implementation classes, single responsibility.

PleaseGiveMeTheCoke · 2023-12-04T06:24:59Z

From the perspective of internal practical experience, share my suggestions:

The resource name should be globally unique.

Annotations to the interface, do not allow multiple implementation classes, single responsibility.

你的意思是将定义TCC资源的注解直接移动到接口上而不是某个方法上吗？
Do you mean moving the annotations defining the TCC resources directly to the interface instead of to a method?

pengten · 2023-12-05T06:43:18Z

LGTM

tcc/src/main/java/io/seata/rm/tcc/TCCResourceManager.java

common/src/main/java/io/seata/common/exception/RepeatRegistrationException.java

wt-better · 2023-12-21T12:49:54Z

LGTM

wt-better · 2023-12-24T06:16:13Z

There are some conflicting files, please fix.

codecov-commenter · 2023-12-24T15:29:37Z

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (e590de5) 48.95% compared to head (852f05c) 48.98%.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #6091      +/-   ##
============================================
+ Coverage     48.95%   48.98%   +0.02%     
- Complexity     4778     4781       +3     
============================================
  Files           915      916       +1     
  Lines         31892    31910      +18     
  Branches       3853     3855       +2     
============================================
+ Hits          15613    15631      +18     
+ Misses        14720    14718       -2     
- Partials       1559     1561       +2

Files	Coverage Δ
.../common/exception/RepeatRegistrationException.java	`0.00% <0.00%> (ø)`
.../main/java/io/seata/rm/tcc/TCCResourceManager.java	`15.00% <38.88%> (+3.23%)`	⬆️

... and 3 files with indirect coverage changes

PleaseGiveMeTheCoke · 2023-12-24T15:30:34Z

There are some conflicting files, please fix.

Thanks for the heads up, I have resolved the conflict

funky-eyes · 2024-03-02T13:41:34Z

I can't merge this pr due to another conflict, please fix it.

PleaseGiveMeTheCoke marked this pull request as ready for review November 28, 2023 12:56

funky-eyes added this to the 2.1.0 milestone Nov 29, 2023

funky-eyes added mode: TCC TCC transaction mode module/tcc tcc module labels Nov 29, 2023

PleaseGiveMeTheCoke changed the title ~~bugfix: Prohibit registration of TCC resources with the same name~~ optimize: Prohibit registration of TCC resources with the same name Nov 29, 2023

funky-eyes previously approved these changes Nov 29, 2023

View reviewed changes

leizhiyuan self-requested a review November 29, 2023 09:24

wangliang181230 reviewed Nov 30, 2023

View reviewed changes

wangliang181230 reviewed Dec 1, 2023

View reviewed changes

wangliang181230 previously approved these changes Dec 1, 2023

View reviewed changes

slievrly reviewed Dec 1, 2023

View reviewed changes

wt-better reviewed Dec 14, 2023

View reviewed changes

tcc/src/main/java/io/seata/rm/tcc/TCCResourceManager.java Outdated Show resolved Hide resolved

funky-eyes reviewed Dec 21, 2023

View reviewed changes

common/src/main/java/io/seata/common/exception/RepeatRegistrationException.java Outdated Show resolved Hide resolved

PleaseGiveMeTheCoke dismissed stale reviews from wangliang181230 and funky-eyes via d137200 December 23, 2023 07:27

PleaseGiveMeTheCoke closed this Mar 4, 2024

PleaseGiveMeTheCoke force-pushed the 2.x branch from 852f05c to 13a5ada Compare March 4, 2024 15:37

PleaseGiveMeTheCoke mentioned this pull request Mar 5, 2024

optimize: Prohibit registration of TCC resources with the same name #6391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize: Prohibit registration of TCC resources with the same name #6091

optimize: Prohibit registration of TCC resources with the same name #6091

PleaseGiveMeTheCoke commented Nov 28, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

funky-eyes left a comment

leizhiyuan commented Nov 29, 2023

wangliang181230 Nov 30, 2023 •

edited

Loading

PleaseGiveMeTheCoke Nov 30, 2023 •

edited by wangliang181230

Loading

wangliang181230 Nov 30, 2023

wangliang181230 Nov 30, 2023 •

edited

Loading

PleaseGiveMeTheCoke Nov 30, 2023

wangliang181230 Nov 30, 2023 •

edited

Loading

wangliang181230 Dec 1, 2023

wangliang181230 left a comment

slievrly left a comment

PleaseGiveMeTheCoke commented Dec 4, 2023 •

edited

Loading

pengten commented Dec 5, 2023

wt-better commented Dec 21, 2023

wt-better commented Dec 24, 2023

codecov-commenter commented Dec 24, 2023 •

edited

Loading

PleaseGiveMeTheCoke commented Dec 24, 2023

funky-eyes commented Mar 2, 2024

optimize: Prohibit registration of TCC resources with the same name #6091

optimize: Prohibit registration of TCC resources with the same name #6091

Conversation

PleaseGiveMeTheCoke commented Nov 28, 2023

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

funky-eyes left a comment

Choose a reason for hiding this comment

leizhiyuan commented Nov 29, 2023

wangliang181230 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

PleaseGiveMeTheCoke Nov 30, 2023 • edited by wangliang181230 Loading

Choose a reason for hiding this comment

wangliang181230 Nov 30, 2023

Choose a reason for hiding this comment

wangliang181230 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

PleaseGiveMeTheCoke Nov 30, 2023

Choose a reason for hiding this comment

wangliang181230 Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

wangliang181230 Dec 1, 2023

Choose a reason for hiding this comment

wangliang181230 left a comment

Choose a reason for hiding this comment

slievrly left a comment

Choose a reason for hiding this comment

PleaseGiveMeTheCoke commented Dec 4, 2023 • edited Loading

pengten commented Dec 5, 2023

wt-better commented Dec 21, 2023

wt-better commented Dec 24, 2023

codecov-commenter commented Dec 24, 2023 • edited Loading

Codecov Report

PleaseGiveMeTheCoke commented Dec 24, 2023

funky-eyes commented Mar 2, 2024

codecov bot commented Nov 28, 2023 •

edited

Loading

wangliang181230 Nov 30, 2023 •

edited

Loading

PleaseGiveMeTheCoke Nov 30, 2023 •

edited by wangliang181230

Loading

wangliang181230 Nov 30, 2023 •

edited

Loading

wangliang181230 Nov 30, 2023 •

edited

Loading

PleaseGiveMeTheCoke commented Dec 4, 2023 •

edited

Loading

codecov-commenter commented Dec 24, 2023 •

edited

Loading