Fix misuse of `TxContext` (#30061) (#30062)

Backport #30061 by @wolfogre

Help #29999, or its tests cannot pass.

Also, add some comments to clarify the usage of `TxContext`.

I don't check all usages of `TxContext` because there are too many
(almost 140+). It's a better idea to replace them with `WithTx` instead
of checking them one by one. However, that may be another refactoring
PR.

Co-authored-by: Jason Song <i@wolfogre.com>

diff --git a/models/db/context.go b/models/db/context.go
index 9f72b43555820b862b00d5edb9f21784dd1eeac2..fc1fecb0e3e042a46cd12d61364a48432e5a4232 100644 (file)
--- a/models/db/context.go
+++ b/models/db/context.go
@@ -120,6 +120,16 @@ func (c *halfCommitter) Close() error {
 
 // TxContext represents a transaction Context,
 // it will reuse the existing transaction in the parent context or create a new one.
+// Some tips to use:
+//
+//     1 It's always recommended to use `WithTx` in new code instead of `TxContext`, since `WithTx` will handle the transaction automatically.
+//     2. To maintain the old code which uses `TxContext`:
+//       a. Always call `Close()` before returning regardless of whether `Commit()` has been called.
+//       b. Always call `Commit()` before returning if there are no errors, even if the code did not change any data.
+//       c. Remember the `Committer` will be a halfCommitter when a transaction is being reused.
+//          So calling `Commit()` will do nothing, but calling `Close()` without calling `Commit()` will rollback the transaction.
+//          And all operations submitted by the caller stack will be rollbacked as well, not only the operations in the current function.
+//       d. It doesn't mean rollback is forbidden, but always do it only when there is an error, and you do want to rollback.
 func TxContext(parentCtx context.Context) (*Context, Committer, error) {
        if sess, ok := inTransaction(parentCtx); ok {
                return newContext(parentCtx, sess, true), &halfCommitter{committer: sess}, nil

diff --git a/models/issues/review.go b/models/issues/review.go
index 718708ec59ae718a700366a605131864b51f0997..c70bdc643b676ff42b14eca1f1ab2e004224b52b 100644 (file)
--- a/models/issues/review.go
+++ b/models/issues/review.go
@@ -621,7 +621,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 
        // skip it when reviewer hase been request to review
        if review != nil && review.Type == ReviewTypeRequest {
-               return nil, nil
+               return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
        }
 
        // if the reviewer is an official reviewer,