-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Fix @Transactional
invalid cases
#4551
Conversation
@@ -41,7 +41,7 @@ List<Audit> find(String owner, String entity, String op) { | |||
} | |||
|
|||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no need to add a transaction here, because if the execution fails, the database will not be updated successfully, which is equivalent to automatically rolling back the transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes,If there is only one database update I think it can be removed,But I found that he originally added transcation so didn't remove it
@@ -51,7 +51,7 @@ void audit(String entityName, Long entityId, Audit.OP op, String owner) { | |||
} | |||
|
|||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. I don't think there is any need to add transaction annotations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes,i think is so
Codecov Report
@@ Coverage Diff @@
## master #4551 +/- ##
============================================
- Coverage 53.30% 53.27% -0.04%
Complexity 2710 2710
============================================
Files 495 495
Lines 15438 15438
Branches 1599 1599
============================================
- Hits 8229 8224 -5
- Misses 6648 6655 +7
+ Partials 561 559 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What's the purpose of this PR
Fix
@Transactional
invalid,@transcation has some invalid cases.E.G.
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.