Refactor deletion (#28610)

Introduce the new generic deletion methods
- `func DeleteByID[T any](ctx context.Context, id int64) (int64, error)`
- `func DeleteByIDs[T any](ctx context.Context, ids ...int64) error`
- `func Delete[T any](ctx context.Context, opts FindOptions) (int64,
error)`

So, we no longer need any specific deletion method and can just use
the generic ones instead.

Replacement of #28450

Closes #28450

---------

Co-authored-by: Lunny Xiao <[email protected]>

Author: delvh

models/actions/artifact.go

@@ -90,19 +90,6 @@ func getArtifactByNameAndPath(ctx context.Context, runID int64, name, fpath stri
 	return &art, nil
 }
 
-// GetArtifactByID returns an artifact by id
-func GetArtifactByID(ctx context.Context, id int64) (*ActionArtifact, error) {
-	var art ActionArtifact
-	has, err := db.GetEngine(ctx).ID(id).Get(&art)
-	if err != nil {
-		return nil, err
-	} else if !has {
-		return nil, util.ErrNotExist
-	}
-
-	return &art, nil
-}
-
 // UpdateArtifactByID updates an artifact by id
 func UpdateArtifactByID(ctx context.Context, id int64, art *ActionArtifact) error {
 	art.ID = id

models/actions/runner.go

@@ -254,7 +254,7 @@ func DeleteRunner(ctx context.Context, id int64) error {
 		return err
 	}
 
-	_, err := db.GetEngine(ctx).Delete(&ActionRunner{ID: id})
+	_, err := db.DeleteByID[ActionRunner](ctx, id)
 	return err
 }
 

models/asymkey/ssh_key.go

@@ -227,16 +227,6 @@ func UpdatePublicKeyUpdated(ctx context.Context, id int64) error {
 	return nil
 }
 
-// DeletePublicKeys does the actual key deletion but does not update authorized_keys file.
-func DeletePublicKeys(ctx context.Context, keyIDs ...int64) error {
-	if len(keyIDs) == 0 {
-		return nil
-	}
-
-	_, err := db.GetEngine(ctx).In("id", keyIDs).Delete(new(PublicKey))
-	return err
-}
-
 // PublicKeysAreExternallyManaged returns whether the provided KeyID represents an externally managed Key
 func PublicKeysAreExternallyManaged(ctx context.Context, keys []*PublicKey) ([]bool, error) {
 	sources := make([]*auth.Source, 0, 5)
@@ -322,8 +312,8 @@ func deleteKeysMarkedForDeletion(ctx context.Context, keys []string) (bool, erro
 			log.Error("SearchPublicKeyByContent: %v", err)
 			continue
 		}
-		if err = DeletePublicKeys(ctx, key.ID); err != nil {
-			log.Error("deletePublicKeys: %v", err)
+		if _, err = db.DeleteByID[PublicKey](ctx, key.ID); err != nil {
+			log.Error("DeleteByID[PublicKey]: %v", err)
 			continue
 		}
 		sshKeysNeedUpdate = true

models/db/context.go

@@ -175,7 +175,7 @@ func Exec(ctx context.Context, sqlAndArgs ...any) (sql.Result, error) {
 
 func Get[T any](ctx context.Context, cond builder.Cond) (object *T, exist bool, err error) {
 	if !cond.IsValid() {
-		return nil, false, ErrConditionRequired{}
+		panic("cond is invalid in db.Get(ctx, cond). This should not be possible.")
 	}
 
 	var bean T
@@ -201,7 +201,7 @@ func GetByID[T any](ctx context.Context, id int64) (object *T, exist bool, err e
 
 func Exist[T any](ctx context.Context, cond builder.Cond) (bool, error) {
 	if !cond.IsValid() {
-		return false, ErrConditionRequired{}
+		panic("cond is invalid in db.Exist(ctx, cond). This should not be possible.")
 	}
 
 	var bean T
@@ -213,16 +213,36 @@ func ExistByID[T any](ctx context.Context, id int64) (bool, error) {
 	return GetEngine(ctx).ID(id).NoAutoCondition().Exist(&bean)
 }
 
+// DeleteByID deletes the given bean with the given ID
+func DeleteByID[T any](ctx context.Context, id int64) (int64, error) {
+	var bean T
+	return GetEngine(ctx).ID(id).NoAutoCondition().NoAutoTime().Delete(&bean)
+}
+
+func DeleteByIDs[T any](ctx context.Context, ids ...int64) error {
+	if len(ids) == 0 {
+		return nil
+	}
+
+	var bean T
+	_, err := GetEngine(ctx).In("id", ids).NoAutoCondition().NoAutoTime().Delete(&bean)
+	return err
+}
+
+func Delete[T any](ctx context.Context, opts FindOptions) (int64, error) {
+	if opts == nil || !opts.ToConds().IsValid() {
+		panic("opts are empty or invalid in db.Delete(ctx, opts). This should not be possible.")
+	}
+
+	var bean T
+	return GetEngine(ctx).Where(opts.ToConds()).NoAutoCondition().NoAutoTime().Delete(&bean)
+}
+
 // DeleteByBean deletes all records according non-empty fields of the bean as conditions.
 func DeleteByBean(ctx context.Context, bean any) (int64, error) {
 	return GetEngine(ctx).Delete(bean)
 }
 
-// DeleteByID deletes the given bean with the given ID
-func DeleteByID(ctx context.Context, id int64, bean any) (int64, error) {
-	return GetEngine(ctx).ID(id).NoAutoCondition().NoAutoTime().Delete(bean)
-}
-
 // FindIDs finds the IDs for the given table name satisfying the given condition
 // By passing a different value than "id" for "idCol", you can query for foreign IDs, i.e. the repo IDs which satisfy the condition
 func FindIDs(ctx context.Context, tableName, idCol string, cond builder.Cond) ([]int64, error) {

models/db/error.go

@@ -72,21 +72,3 @@ func (err ErrNotExist) Error() string {
 func (err ErrNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
-
-// ErrConditionRequired represents an error which require condition.
-type ErrConditionRequired struct{}
-
-// IsErrConditionRequired checks if an error is an ErrConditionRequired
-func IsErrConditionRequired(err error) bool {
-	_, ok := err.(ErrConditionRequired)
-	return ok
-}
-
-func (err ErrConditionRequired) Error() string {
-	return "condition is required"
-}
-
-// Unwrap unwraps this as a ErrNotExist err
-func (err ErrConditionRequired) Unwrap() error {
-	return util.ErrInvalidArgument
-}

models/issues/issue_test.go

@@ -249,11 +249,11 @@ func TestIssue_InsertIssue(t *testing.T) {
 
 	// there are 5 issues and max index is 5 on repository 1, so this one should 6
 	issue := testInsertIssue(t, "my issue1", "special issue's comments?", 6)
-	_, err := db.GetEngine(db.DefaultContext).ID(issue.ID).Delete(new(issues_model.Issue))
+	_, err := db.DeleteByID[issues_model.Issue](db.DefaultContext, issue.ID)
 	assert.NoError(t, err)
 
 	issue = testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7)
-	_, err = db.GetEngine(db.DefaultContext).ID(issue.ID).Delete(new(issues_model.Issue))
+	_, err = db.DeleteByID[issues_model.Issue](db.DefaultContext, issue.ID)
 	assert.NoError(t, err)
 }
 

models/issues/label.go

@@ -256,7 +256,7 @@ func DeleteLabel(ctx context.Context, id, labelID int64) error {
 		return nil
 	}
 
-	if _, err = sess.ID(labelID).Delete(new(Label)); err != nil {
+	if _, err = db.DeleteByID[Label](ctx, labelID); err != nil {
 		return err
 	} else if _, err = sess.
 		Where("label_id = ?", labelID).

models/issues/milestone.go

@@ -289,9 +289,7 @@ func DeleteMilestoneByRepoID(ctx context.Context, repoID, id int64) error {
 	}
 	defer committer.Close()
 
-	sess := db.GetEngine(ctx)
-
-	if _, err = sess.ID(m.ID).Delete(new(Milestone)); err != nil {
+	if _, err = db.DeleteByID[Milestone](ctx, m.ID); err != nil {
 		return err
 	}
 
@@ -311,7 +309,7 @@ func DeleteMilestoneByRepoID(ctx context.Context, repoID, id int64) error {
 	repo.NumMilestones = int(numMilestones)
 	repo.NumClosedMilestones = int(numClosedMilestones)
 
-	if _, err = sess.ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil {
+	if _, err = db.GetEngine(ctx).ID(repo.ID).Cols("num_milestones, num_closed_milestones").Update(repo); err != nil {
 		return err
 	}
 

models/issues/review.go

@@ -446,7 +446,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, revi
 				continue
 			}
 
-			if _, err := sess.ID(teamReviewRequest.ID).NoAutoCondition().Delete(teamReviewRequest); err != nil {
+			if _, err := db.DeleteByID[Review](ctx, teamReviewRequest.ID); err != nil {
 				return nil, nil, err
 			}
 		}
@@ -869,7 +869,6 @@ func DeleteReview(ctx context.Context, r *Review) error {
 		return err
 	}
 	defer committer.Close()
-	sess := db.GetEngine(ctx)
 
 	if r.ID == 0 {
 		return fmt.Errorf("review is not allowed to be 0")
@@ -885,7 +884,7 @@ func DeleteReview(ctx context.Context, r *Review) error {
 		ReviewID: r.ID,
 	}
 
-	if _, err := sess.Where(opts.ToConds()).Delete(new(Comment)); err != nil {
+	if _, err := db.Delete[Comment](ctx, opts); err != nil {
 		return err
 	}
 
@@ -895,7 +894,7 @@ func DeleteReview(ctx context.Context, r *Review) error {
 		ReviewID: r.ID,
 	}
 
-	if _, err := sess.Where(opts.ToConds()).Delete(new(Comment)); err != nil {
+	if _, err := db.Delete[Comment](ctx, opts); err != nil {
 		return err
 	}
 
@@ -905,11 +904,11 @@ func DeleteReview(ctx context.Context, r *Review) error {
 		ReviewID: r.ID,
 	}
 
-	if _, err := sess.Where(opts.ToConds()).Delete(new(Comment)); err != nil {
+	if _, err := db.Delete[Comment](ctx, opts); err != nil {
 		return err
 	}
 
-	if _, err := sess.ID(r.ID).Delete(new(Review)); err != nil {
+	if _, err := db.DeleteByID[Review](ctx, r.ID); err != nil {
 		return err
 	}
 

models/org.go

@@ -57,7 +57,7 @@ func RemoveOrgUser(ctx context.Context, orgID, userID int64) error {
 	}
 	defer committer.Close()
 
-	if _, err := db.GetEngine(ctx).ID(ou.ID).Delete(ou); err != nil {
+	if _, err := db.DeleteByID[organization.OrgUser](ctx, ou.ID); err != nil {
 		return err
 	} else if _, err = db.Exec(ctx, "UPDATE `user` SET num_members=num_members-1 WHERE id=?", orgID); err != nil {
 		return err