Delete related PullAutoMerge and ReviewState on User/Repo Deletion (#19649)

* delete pullautomerges on repo/user deletion
* delete reviewstates on repo/user deletion
* optimize automerhe code
* add index to reviewstate

models/db/error.go

@@ -42,3 +42,18 @@ func IsErrSSHDisabled(err error) bool {
 func (err ErrSSHDisabled) Error() string {
 	return "SSH is disabled"
 }
+
+// ErrNotExist represents a non-exist error.
+type ErrNotExist struct {
+	ID int64
+}
+
+// IsErrNotExist checks if an error is an ErrNotExist
+func IsErrNotExist(err error) bool {
+	_, ok := err.(ErrNotExist)
+	return ok
+}
+
+func (err ErrNotExist) Error() string {
+	return fmt.Sprintf("record does not exist [id: %d]", err.ID)
+}

models/error.go

@@ -13,21 +13,6 @@ import (
 	"code.gitea.io/gitea/modules/git"
 )
 
-// ErrNotExist represents a non-exist error.
-type ErrNotExist struct {
-	ID int64
-}
-
-// IsErrNotExist checks if an error is an ErrNotExist
-func IsErrNotExist(err error) bool {
-	_, ok := err.(ErrNotExist)
-	return ok
-}
-
-func (err ErrNotExist) Error() string {
-	return fmt.Sprintf("record does not exist [id: %d]", err.ID)
-}
-
 // ErrUserOwnRepos represents a "UserOwnRepos" kind of error.
 type ErrUserOwnRepos struct {
 	UID int64

models/issue_comment.go

@@ -1360,6 +1360,28 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *Pul
 	return
 }
 
+// CreateAutoMergeComment is a internal function, only use it for CommentTypePRScheduledToAutoMerge and CommentTypePRUnScheduledToAutoMerge CommentTypes
+func CreateAutoMergeComment(ctx context.Context, typ CommentType, pr *PullRequest, doer *user_model.User) (comment *Comment, err error) {
+	if typ != CommentTypePRScheduledToAutoMerge && typ != CommentTypePRUnScheduledToAutoMerge {
+		return nil, fmt.Errorf("comment type %d cannot be used to create an auto merge comment", typ)
+	}
+	if err = pr.LoadIssueCtx(ctx); err != nil {
+		return
+	}
+
+	if err = pr.LoadBaseRepoCtx(ctx); err != nil {
+		return
+	}
+
+	comment, err = CreateCommentCtx(ctx, &CreateCommentOptions{
+		Type:  typ,
+		Doer:  doer,
+		Repo:  pr.BaseRepo,
+		Issue: pr.Issue,
+	})
+	return
+}
+
 // getCommitsFromRepo get commit IDs from repo in between oldCommitID and newCommitID
 // isForcePush will be true if oldCommit isn't on the branch
 // Commit on baseBranch will skip

models/issue_tracked_time.go

@@ -251,7 +251,7 @@ func DeleteIssueUserTimes(issue *Issue, user *user_model.User) error {
 		return err
 	}
 	if removedTime == 0 {
-		return ErrNotExist{}
+		return db.ErrNotExist{}
 	}
 
 	if err := issue.LoadRepo(ctx); err != nil {
@@ -311,7 +311,7 @@ func deleteTimes(e db.Engine, opts FindTrackedTimesOptions) (removedTime int64,
 
 func deleteTime(e db.Engine, t *TrackedTime) error {
 	if t.Deleted {
-		return ErrNotExist{ID: t.ID}
+		return db.ErrNotExist{ID: t.ID}
 	}
 	t.Deleted = true
 	_, err := e.ID(t.ID).Cols("deleted").Update(t)
@@ -325,7 +325,7 @@ func GetTrackedTimeByID(id int64) (*TrackedTime, error) {
 	if err != nil {
 		return nil, err
 	} else if !has {
-		return nil, ErrNotExist{ID: id}
+		return nil, db.ErrNotExist{ID: id}
 	}
 	return time, nil
 }

models/migrations/v215.go

@@ -15,7 +15,7 @@ func addReviewViewedFiles(x *xorm.Engine) error {
 	type ReviewState struct {
 		ID           int64                       `xorm:"pk autoincr"`
 		UserID       int64                       `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
-		PullID       int64                       `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"`
+		PullID       int64                       `xorm:"NOT NULL INDEX UNIQUE(pull_commit_user) DEFAULT 0"`
 		CommitSHA    string                      `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"`
 		UpdatedFiles map[string]pull.ViewedState `xorm:"NOT NULL LONGTEXT JSON"`
 		UpdatedUnix  timeutil.TimeStamp          `xorm:"updated"`

models/notification.go

@@ -825,7 +825,7 @@ func getNotificationByID(e db.Engine, notificationID int64) (*Notification, erro
 	}
 
 	if !ok {
-		return nil, ErrNotExist{ID: notificationID}
+		return nil, db.ErrNotExist{ID: notificationID}
 	}
 
 	return notification, nil

models/pull.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models/db"
+	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
@@ -96,6 +97,25 @@ func init() {
 	db.RegisterModel(new(PullRequest))
 }
 
+func deletePullsByBaseRepoID(sess db.Engine, repoID int64) error {
+	deleteCond := builder.Select("id").From("pull_request").Where(builder.Eq{"pull_request.base_repo_id": repoID})
+
+	// Delete scheduled auto merges
+	if _, err := sess.In("pull_id", deleteCond).
+		Delete(&pull_model.AutoMerge{}); err != nil {
+		return err
+	}
+
+	// Delete review states
+	if _, err := sess.In("pull_id", deleteCond).
+		Delete(&pull_model.ReviewState{}); err != nil {
+		return err
+	}
+
+	_, err := sess.Delete(&PullRequest{BaseRepoID: repoID})
+	return err
+}
+
 // MustHeadUserName returns the HeadRepo's username if failed return blank
 func (pr *PullRequest) MustHeadUserName() string {
 	if err := pr.LoadHeadRepo(); err != nil {

models/pull/automerge.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"fmt"
 
-	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
@@ -59,21 +58,12 @@ func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pullID int64,
 		return ErrAlreadyScheduledToAutoMerge{PullID: pullID}
 	}
 
-	if _, err := db.GetEngine(ctx).Insert(&AutoMerge{
+	_, err := db.GetEngine(ctx).Insert(&AutoMerge{
 		DoerID:     doer.ID,
 		PullID:     pullID,
 		MergeStyle: style,
 		Message:    message,
-	}); err != nil {
-		return err
-	}
-
-	pr, err := models.GetPullRequestByID(ctx, pullID)
-	if err != nil {
-		return err
-	}
-
-	_, err = createAutoMergeComment(ctx, models.CommentTypePRScheduledToAutoMerge, pr, doer)
+	})
 	return err
 }
 
@@ -94,50 +84,15 @@ func GetScheduledMergeByPullID(ctx context.Context, pullID int64) (bool, *AutoMe
 	return true, scheduledPRM, nil
 }
 
-// RemoveScheduledAutoMerge cancels a previously scheduled pull request
-func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pullID int64, comment bool) error {
-	return db.WithTx(func(ctx context.Context) error {
-		exist, scheduledPRM, err := GetScheduledMergeByPullID(ctx, pullID)
-		if err != nil {
-			return err
-		} else if !exist {
-			return models.ErrNotExist{ID: pullID}
-		}
-
-		if _, err := db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{}); err != nil {
-			return err
-		}
-
-		// if pull got merged we don't need to add "auto-merge canceled comment"
-		if !comment || doer == nil {
-			return nil
-		}
-
-		pr, err := models.GetPullRequestByID(ctx, pullID)
-		if err != nil {
-			return err
-		}
-
-		_, err = createAutoMergeComment(ctx, models.CommentTypePRUnScheduledToAutoMerge, pr, doer)
+// DeleteScheduledAutoMerge delete a scheduled pull request
+func DeleteScheduledAutoMerge(ctx context.Context, pullID int64) error {
+	exist, scheduledPRM, err := GetScheduledMergeByPullID(ctx, pullID)
+	if err != nil {
 		return err
-	}, ctx)
-}
-
-// createAutoMergeComment is a internal function, only use it for CommentTypePRScheduledToAutoMerge and CommentTypePRUnScheduledToAutoMerge CommentTypes
-func createAutoMergeComment(ctx context.Context, typ models.CommentType, pr *models.PullRequest, doer *user_model.User) (comment *models.Comment, err error) {
-	if err = pr.LoadIssueCtx(ctx); err != nil {
-		return
+	} else if !exist {
+		return db.ErrNotExist{ID: pullID}
 	}
 
-	if err = pr.LoadBaseRepoCtx(ctx); err != nil {
-		return
-	}
-
-	comment, err = models.CreateCommentCtx(ctx, &models.CreateCommentOptions{
-		Type:  typ,
-		Doer:  doer,
-		Repo:  pr.BaseRepo,
-		Issue: pr.Issue,
-	})
-	return
+	_, err = db.GetEngine(ctx).ID(scheduledPRM.ID).Delete(&AutoMerge{})
+	return err
 }

models/pull/review_state.go

@@ -38,10 +38,10 @@ func (viewedState ViewedState) String() string {
 type ReviewState struct {
 	ID           int64                  `xorm:"pk autoincr"`
 	UserID       int64                  `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
-	PullID       int64                  `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"`   // Which PR was the review on?
-	CommitSHA    string                 `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review?
-	UpdatedFiles map[string]ViewedState `xorm:"NOT NULL LONGTEXT JSON"`                        // Stores for each of the changed files of a PR whether they have been viewed, changed since last viewed, or not viewed
-	UpdatedUnix  timeutil.TimeStamp     `xorm:"updated"`                                       // Is an accurate indicator of the order of commits as we do not expect it to be possible to make reviews on previous commits
+	PullID       int64                  `xorm:"NOT NULL INDEX UNIQUE(pull_commit_user) DEFAULT 0"` // Which PR was the review on?
+	CommitSHA    string                 `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"`     // Which commit was the head commit for the review?
+	UpdatedFiles map[string]ViewedState `xorm:"NOT NULL LONGTEXT JSON"`                            // Stores for each of the changed files of a PR whether they have been viewed, changed since last viewed, or not viewed
+	UpdatedUnix  timeutil.TimeStamp     `xorm:"updated"`                                           // Is an accurate indicator of the order of commits as we do not expect it to be possible to make reviews on previous commits
 }
 
 func init() {

models/repo.go

@@ -704,7 +704,6 @@ func DeleteRepository(doer *user_model.User, uid, repoID int64) error {
 		&Notification{RepoID: repoID},
 		&ProtectedBranch{RepoID: repoID},
 		&ProtectedTag{RepoID: repoID},
-		&PullRequest{BaseRepoID: repoID},
 		&repo_model.PushMirror{RepoID: repoID},
 		&Release{RepoID: repoID},
 		&repo_model.RepoIndexerStatus{RepoID: repoID},
@@ -723,6 +722,11 @@ func DeleteRepository(doer *user_model.User, uid, repoID int64) error {
 		return err
 	}
 
+	// Delete Pulls and related objects
+	if err := deletePullsByBaseRepoID(sess, repoID); err != nil {
+		return err
+	}
+
 	// Delete Issues and related objects
 	var attachmentPaths []string
 	if attachmentPaths, err = deleteIssuesByRepoID(sess, repoID); err != nil {

models/user.go

@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/organization"
+	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
@@ -82,6 +83,8 @@ func DeleteUser(ctx context.Context, u *user_model.User) (err error) {
 		&Collaboration{UserID: u.ID},
 		&Stopwatch{UserID: u.ID},
 		&user_model.Setting{UserID: u.ID},
+		&pull_model.AutoMerge{DoerID: u.ID},
+		&pull_model.ReviewState{UserID: u.ID},
 	); err != nil {
 		return fmt.Errorf("deleteBeans: %v", err)
 	}

routers/api/v1/notify/threads.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/convert"
 )
@@ -102,7 +103,7 @@ func ReadThread(ctx *context.APIContext) {
 func getThread(ctx *context.APIContext) *models.Notification {
 	n, err := models.GetNotificationByID(ctx.ParamsInt64(":id"))
 	if err != nil {
-		if models.IsErrNotExist(err) {
+		if db.IsErrNotExist(err) {
 			ctx.Error(http.StatusNotFound, "GetNotificationByID", err)
 		} else {
 			ctx.InternalServerError(err)

routers/api/v1/repo/issue_tracked_time.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/context"
@@ -281,7 +282,7 @@ func ResetIssueTime(ctx *context.APIContext) {
 
 	err = models.DeleteIssueUserTimes(issue, ctx.Doer)
 	if err != nil {
-		if models.IsErrNotExist(err) {
+		if db.IsErrNotExist(err) {
 			ctx.Error(http.StatusNotFound, "DeleteIssueUserTimes", err)
 		} else {
 			ctx.Error(http.StatusInternalServerError, "DeleteIssueUserTimes", err)
@@ -352,7 +353,7 @@ func DeleteTime(ctx *context.APIContext) {
 
 	time, err := models.GetTrackedTimeByID(ctx.ParamsInt64(":id"))
 	if err != nil {
-		if models.IsErrNotExist(err) {
+		if db.IsErrNotExist(err) {
 			ctx.NotFound(err)
 			return
 		}

routers/api/v1/repo/pull.go

@@ -1208,7 +1208,7 @@ func CancelScheduledAutoMerge(ctx *context.APIContext) {
 		}
 	}
 
-	if err := pull_model.RemoveScheduledAutoMerge(ctx, ctx.Doer, pull.ID, true); err != nil {
+	if err := automerge.RemoveScheduledAutoMerge(ctx, ctx.Doer, pull); err != nil {
 		ctx.InternalServerError(err)
 	} else {
 		ctx.Status(http.StatusNoContent)

routers/web/repo/issue_timetrack.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
@@ -63,7 +64,7 @@ func DeleteTime(c *context.Context) {
 
 	t, err := models.GetTrackedTimeByID(c.ParamsInt64(":timeid"))
 	if err != nil {
-		if models.IsErrNotExist(err) {
+		if db.IsErrNotExist(err) {
 			c.NotFound("time not found", err)
 			return
 		}

services/automerge/automerge.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
@@ -61,17 +62,38 @@ func addToQueue(pr *models.PullRequest, sha string) {
 
 // ScheduleAutoMerge if schedule is false and no error, pull can be merged directly
 func ScheduleAutoMerge(ctx context.Context, doer *user_model.User, pull *models.PullRequest, style repo_model.MergeStyle, message string) (scheduled bool, err error) {
-	lastCommitStatus, err := pull_service.GetPullRequestCommitStatusState(ctx, pull)
-	if err != nil {
-		return false, err
-	}
+	err = db.WithTx(func(ctx context.Context) error {
+		lastCommitStatus, err := pull_service.GetPullRequestCommitStatusState(ctx, pull)
+		if err != nil {
+			return err
+		}
 
-	// we don't need to schedule
-	if lastCommitStatus.IsSuccess() {
-		return false, nil
-	}
+		// we don't need to schedule
+		if lastCommitStatus.IsSuccess() {
+			return nil
+		}
+
+		if err := pull_model.ScheduleAutoMerge(ctx, doer, pull.ID, style, message); err != nil {
+			return err
+		}
+		scheduled = true
+
+		_, err = models.CreateAutoMergeComment(ctx, models.CommentTypePRScheduledToAutoMerge, pull, doer)
+		return err
+	}, ctx)
+	return
+}
 
-	return true, pull_model.ScheduleAutoMerge(ctx, doer, pull.ID, style, message)
+// RemoveScheduledAutoMerge cancels a previously scheduled pull request
+func RemoveScheduledAutoMerge(ctx context.Context, doer *user_model.User, pull *models.PullRequest) error {
+	return db.WithTx(func(ctx context.Context) error {
+		if err := pull_model.DeleteScheduledAutoMerge(ctx, pull.ID); err != nil {
+			return err
+		}
+
+		_, err := models.CreateAutoMergeComment(ctx, models.CommentTypePRUnScheduledToAutoMerge, pull, doer)
+		return err
+	}, ctx)
 }
 
 // MergeScheduledPullRequest merges a previously scheduled pull request when all checks succeeded

services/pull/merge.go

@@ -141,7 +141,7 @@ func Merge(ctx context.Context, pr *models.PullRequest, doer *user_model.User, b
 	defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID))
 
 	// Removing an auto merge pull and ignore if not exist
-	if err := pull_model.RemoveScheduledAutoMerge(db.DefaultContext, doer, pr.ID, false); err != nil && !models.IsErrNotExist(err) {
+	if err := pull_model.DeleteScheduledAutoMerge(db.DefaultContext, pr.ID); err != nil && !db.IsErrNotExist(err) {
 		return err
 	}