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

1、fix transcation invalid #4509

Merged
merged 6 commits into from
Aug 15, 2022
Merged

1、fix transcation invalid #4509

merged 6 commits into from
Aug 15, 2022

Conversation

ksice
Copy link
Contributor

@ksice ksice commented Aug 11, 2022

What's the purpose of this PR

Fix transaction invalid

sonar prompt bug is "assignNamespaceRoleToConsumer's" @transactional requirement is incompatible with the one for this method.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@github-actions
Copy link

github-actions bot commented Aug 11, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ksice
Copy link
Contributor Author

ksice commented Aug 11, 2022

I have read the CLA Document and I hereby sign the CLA

@nobodyiam
Copy link
Member

Thanks for the pull request. However, I'd like to learn more about what this change solves. Since there is no checked exception thrown for method assignNamespaceRoleToConsumer, I think adding rollbackFor or not is the same?

@ksice
Copy link
Contributor Author

ksice commented Aug 12, 2022

Thanks for the pull request. However, I'd like to learn more about what this change solves. Since there is no checked exception thrown for method assignNamespaceRoleToConsumer, I think adding rollbackFor or not is the same?

I think rollbackfor can not be added but the @transcation annotation should be added, otherwise the transaction of the "assignNamespace" method will be invalidated

@nobodyiam
Copy link
Member

Yes, but I think the @Transactional annotation is already added to public List<ConsumerRole> assignNamespaceRoleToConsumer(String token, String appId, String namespaceName, String env)?

@ksice
Copy link
Contributor Author

ksice commented Aug 14, 2022

Yes, but I think the @Transactional annotation is already added to public List<ConsumerRole> assignNamespaceRoleToConsumer(String token, String appId, String namespaceName, String env)?

yes,But what I mean is that this method assignNamespaceRoleToConsumer(String token, String appId, String namespaceName) needs to add @Transactional, @Transactional has several failure scenarios, one of which is
The @Transactional annotation does not take effect in similar method calls, so when assignNamespaceRoleToConsumer(String token, String appId, String namespaceName) is called, it will cause assignNamespaceRoleToConsumer(String token, String appId, String namespaceName, String env)
@Transactional fails, I find it not roll back when I locally called assignNamespaceRoleToConsumer(String token, String appId, String namespaceName) to throw an exception。
E.g
image

@nobodyiam
Copy link
Member

OK, then I think it's reasonable to add @Transactional to assignNamespaceRoleToConsumer(String token, String appId, String namespaceName).

@ksice
Copy link
Contributor Author

ksice commented Aug 15, 2022

OK, then I think it's reasonable to add @Transactional to assignNamespaceRoleToConsumer(String token, String appId, String namespaceName).

ok, I modified the commit

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit 82db184 into apolloconfig:master Aug 15, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants