Skip to content

Refactor DeleteByID #28450

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions models/actions/artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion models/actions/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,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
}

Expand Down
14 changes: 2 additions & 12 deletions models/asymkey/ssh_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
30 changes: 25 additions & 5 deletions models/db/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
return 0, ErrConditionRequired{}
Copy link
Member

Choose a reason for hiding this comment

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

I felt this ErrConditionRequired strange.
If I'm going to use the Delete function and forget to add conditions, then it returns an error, I naturally think something is wrong with the database. Maybe the database is somehow disconnected or some other errors. So I probably will use if err != nil and then return the error to the upper function or log the error to remind the user end.
It's unlikely for every developer to check the ErrConditionRequired error. Because 1. it's cumbersome. 2. It's hard to notice because this error was only introduced last week and no one is familiar with it.
Then the error is shown on the user end. But actually, the programmer should be warned instead of the user end.
I prefer to delete this ErrConditionRequired and let the test code find out the problem. But maybe there is a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

panic.

}

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) {
Expand Down
4 changes: 2 additions & 2 deletions models/issues/issue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,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)
}

Expand Down
2 changes: 1 addition & 1 deletion models/issues/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 2 additions & 4 deletions models/issues/milestone.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
11 changes: 5 additions & 6 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -867,7 +867,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")
Expand All @@ -883,7 +882,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
}

Expand All @@ -893,7 +892,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
}

Expand All @@ -903,11 +902,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
}

Expand Down
2 changes: 1 addition & 1 deletion models/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 2 additions & 4 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,9 +344,7 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error
}
}

if _, err := db.DeleteByBean(ctx, &asymkey_model.DeployKey{
ID: key.ID,
}); err != nil {
if _, err := db.DeleteByID[asymkey_model.DeployKey](ctx, key.ID); err != nil {
return fmt.Errorf("delete deploy key [%d]: %w", key.ID, err)
}

Expand All @@ -355,7 +353,7 @@ func DeleteDeployKey(ctx context.Context, doer *user_model.User, id int64) error
if err != nil {
return err
} else if !has {
if err = asymkey_model.DeletePublicKeys(ctx, key.KeyID); err != nil {
if _, err = db.DeleteByID[asymkey_model.PublicKey](ctx, key.KeyID); err != nil {
return err
}
}
Expand Down
15 changes: 1 addition & 14 deletions models/repo/archiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,6 @@ func repoArchiverForRelativePath(relativePath string) (*RepoArchiver, error) {
}, nil
}

var delRepoArchiver = new(RepoArchiver)

// DeleteRepoArchiver delete archiver
func DeleteRepoArchiver(ctx context.Context, archiver *RepoArchiver) error {
_, err := db.GetEngine(ctx).ID(archiver.ID).Delete(delRepoArchiver)
return err
}

// GetRepoArchiver get an archiver
func GetRepoArchiver(ctx context.Context, repoID int64, tp git.ArchiveType, commitID string) (*RepoArchiver, error) {
var archiver RepoArchiver
Expand All @@ -100,12 +92,6 @@ func ExistsRepoArchiverWithStoragePath(ctx context.Context, storagePath string)
return db.GetEngine(ctx).Exist(archiver)
}

// AddRepoArchiver adds an archiver
func AddRepoArchiver(ctx context.Context, archiver *RepoArchiver) error {
_, err := db.GetEngine(ctx).Insert(archiver)
return err
}

// UpdateRepoArchiverStatus updates archiver's status
func UpdateRepoArchiverStatus(ctx context.Context, archiver *RepoArchiver) error {
_, err := db.GetEngine(ctx).ID(archiver.ID).Cols("status").Update(archiver)
Expand All @@ -114,6 +100,7 @@ func UpdateRepoArchiverStatus(ctx context.Context, archiver *RepoArchiver) error

// DeleteAllRepoArchives deletes all repo archives records
func DeleteAllRepoArchives(ctx context.Context) error {
// 1=1 to enforce delete all data, otherwise it will delete nothing
_, err := db.GetEngine(ctx).Where("1=1").Delete(new(RepoArchiver))
return err
}
Expand Down
22 changes: 3 additions & 19 deletions models/repo/pushmirror.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@ type PushMirror struct {
}

type PushMirrorOptions struct {
db.ListOptions
ID int64
RepoID int64
RemoteName string
}

func (opts *PushMirrorOptions) toConds() builder.Cond {
func (opts PushMirrorOptions) ToConds() builder.Cond {
cond := builder.NewCond()
if opts.RepoID > 0 {
cond = cond.And(builder.Eq{"repo_id": opts.RepoID})
Expand Down Expand Up @@ -75,12 +76,6 @@ func (m *PushMirror) GetRemoteName() string {
return m.RemoteName
}

// InsertPushMirror inserts a push-mirror to database
func InsertPushMirror(ctx context.Context, m *PushMirror) error {
_, err := db.GetEngine(ctx).Insert(m)
return err
}

// UpdatePushMirror updates the push-mirror
func UpdatePushMirror(ctx context.Context, m *PushMirror) error {
_, err := db.GetEngine(ctx).ID(m.ID).AllCols().Update(m)
Expand All @@ -95,23 +90,12 @@ func UpdatePushMirrorInterval(ctx context.Context, m *PushMirror) error {

func DeletePushMirrors(ctx context.Context, opts PushMirrorOptions) error {
if opts.RepoID > 0 {
_, err := db.GetEngine(ctx).Where(opts.toConds()).Delete(&PushMirror{})
_, err := db.Delete[PushMirror](ctx, opts)
return err
}
return util.NewInvalidArgumentErrorf("repoID required and must be set")
}

func GetPushMirror(ctx context.Context, opts PushMirrorOptions) (*PushMirror, error) {
mirror := &PushMirror{}
exist, err := db.GetEngine(ctx).Where(opts.toConds()).Get(mirror)
if err != nil {
return nil, err
} else if !exist {
return nil, ErrPushMirrorNotExist
}
return mirror, nil
}

// GetPushMirrorsByRepoID returns push-mirror information of a repository.
func GetPushMirrorsByRepoID(ctx context.Context, repoID int64, listOptions db.ListOptions) ([]*PushMirror, int64, error) {
sess := db.GetEngine(ctx).Where("repo_id = ?", repoID)
Expand Down
6 changes: 3 additions & 3 deletions models/repo/pushmirror_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,20 @@ func TestPushMirrorsIterate(t *testing.T) {

now := timeutil.TimeStampNow()

repo_model.InsertPushMirror(db.DefaultContext, &repo_model.PushMirror{
db.Insert(db.DefaultContext, &repo_model.PushMirror{
RemoteName: "test-1",
LastUpdateUnix: now,
Interval: 1,
})

long, _ := time.ParseDuration("24h")
repo_model.InsertPushMirror(db.DefaultContext, &repo_model.PushMirror{
db.Insert(db.DefaultContext, &repo_model.PushMirror{
RemoteName: "test-2",
LastUpdateUnix: now,
Interval: long,
})

repo_model.InsertPushMirror(db.DefaultContext, &repo_model.PushMirror{
db.Insert(db.DefaultContext, &repo_model.PushMirror{
RemoteName: "test-3",
LastUpdateUnix: now,
Interval: 0,
Expand Down
8 changes: 1 addition & 7 deletions models/repo/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -450,12 +450,6 @@ func SortReleases(rels []*Release) {
sort.Sort(sorter)
}

// DeleteReleaseByID deletes a release from database by given ID.
func DeleteReleaseByID(ctx context.Context, id int64) error {
_, err := db.GetEngine(ctx).ID(id).Delete(new(Release))
return err
}

// UpdateReleasesMigrationsByType updates all migrated repositories' releases from gitServiceType to replace originalAuthorID to posterID
func UpdateReleasesMigrationsByType(ctx context.Context, gitServiceType structs.GitServiceType, originalAuthorID string, posterID int64) error {
_, err := db.GetEngine(ctx).Table("release").
Expand Down Expand Up @@ -509,7 +503,7 @@ func PushUpdateDeleteTag(ctx context.Context, repo *Repository, tagName string)
return fmt.Errorf("GetRelease: %w", err)
}
if rel.IsTag {
if _, err = db.GetEngine(ctx).ID(rel.ID).Delete(new(Release)); err != nil {
if _, err = db.DeleteByID[Release](ctx, rel.ID); err != nil {
return fmt.Errorf("Delete: %w", err)
}
} else {
Expand Down
4 changes: 1 addition & 3 deletions models/repo/upload.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,7 @@ func DeleteUploads(ctx context.Context, uploads ...*Upload) (err error) {
for i := 0; i < len(uploads); i++ {
ids[i] = uploads[i].ID
}
if _, err = db.GetEngine(ctx).
In("id", ids).
Delete(new(Upload)); err != nil {
if err = db.DeleteByIDs[Upload](ctx, ids...); err != nil {
return fmt.Errorf("delete uploads: %w", err)
}

Expand Down
10 changes: 4 additions & 6 deletions models/repo/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,21 @@ func watchRepoMode(ctx context.Context, watch Watch, mode WatchMode) (err error)

watch.Mode = mode

e := db.GetEngine(ctx)

if !hadrec && needsrec {
watch.Mode = mode
if _, err = e.Insert(watch); err != nil {
if err = db.Insert(ctx, watch); err != nil {
return err
}
} else if needsrec {
watch.Mode = mode
if _, err := e.ID(watch.ID).AllCols().Update(watch); err != nil {
if _, err := db.GetEngine(ctx).ID(watch.ID).AllCols().Update(watch); err != nil {
return err
}
} else if _, err = e.Delete(Watch{ID: watch.ID}); err != nil {
} else if _, err = db.DeleteByID[Watch](ctx, watch.ID); err != nil {
return err
}
if repodiff != 0 {
_, err = e.Exec("UPDATE `repository` SET num_watches = num_watches + ? WHERE id = ?", repodiff, watch.RepoID)
_, err = db.GetEngine(ctx).Exec("UPDATE `repository` SET num_watches = num_watches + ? WHERE id = ?", repodiff, watch.RepoID)
}
return err
}
Expand Down
Loading