From 6495741238163e4d682fc00a84ecb675f6db41cb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Apr 2024 09:41:50 +0800 Subject: [PATCH 1/4] Add commit status summary table to reduce query from commit status table (#30223) This PR adds a new table named commit status summary to reduce queries from the commit status table. After this change, commit status summary table will be used for the final result, commit status table will be for details. --------- Co-authored-by: Jason Song --- models/git/commit_status.go | 21 ++--- models/git/commit_status_summary.go | 84 +++++++++++++++++++ models/migrations/migrations.go | 3 + models/migrations/v1_23/v295.go | 18 ++++ .../repository/commitstatus/commitstatus.go | 48 +++++++++-- tests/integration/pull_status_test.go | 7 ++ 6 files changed, 162 insertions(+), 19 deletions(-) create mode 100644 models/git/commit_status_summary.go create mode 100644 models/migrations/v1_23/v295.go diff --git a/models/git/commit_status.go b/models/git/commit_status.go index bec819348..3f2172e50 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -257,30 +257,27 @@ func GetLatestCommitStatus(ctx context.Context, repoID int64, sha string, listOp } // GetLatestCommitStatusForPairs returns all statuses with a unique context for a given list of repo-sha pairs -func GetLatestCommitStatusForPairs(ctx context.Context, repoIDsToLatestCommitSHAs map[int64]string, listOptions db.ListOptions) (map[int64][]*CommitStatus, error) { +func GetLatestCommitStatusForPairs(ctx context.Context, repoSHAs []RepoSHA) (map[int64][]*CommitStatus, error) { type result struct { Index int64 RepoID int64 + SHA string } - results := make([]result, 0, len(repoIDsToLatestCommitSHAs)) + results := make([]result, 0, len(repoSHAs)) getBase := func() *xorm.Session { return db.GetEngine(ctx).Table(&CommitStatus{}) } // Create a disjunction of conditions for each repoID and SHA pair - conds := make([]builder.Cond, 0, len(repoIDsToLatestCommitSHAs)) - for repoID, sha := range repoIDsToLatestCommitSHAs { - conds = append(conds, builder.Eq{"repo_id": repoID, "sha": sha}) + conds := make([]builder.Cond, 0, len(repoSHAs)) + for _, repoSHA := range repoSHAs { + conds = append(conds, builder.Eq{"repo_id": repoSHA.RepoID, "sha": repoSHA.SHA}) } sess := getBase().Where(builder.Or(conds...)). - Select("max( `index` ) as `index`, repo_id"). - GroupBy("context_hash, repo_id").OrderBy("max( `index` ) desc") - - if !listOptions.IsListAll() { - sess = db.SetSessionPagination(sess, &listOptions) - } + Select("max( `index` ) as `index`, repo_id, sha"). + GroupBy("context_hash, repo_id, sha").OrderBy("max( `index` ) desc") err := sess.Find(&results) if err != nil { @@ -297,7 +294,7 @@ func GetLatestCommitStatusForPairs(ctx context.Context, repoIDsToLatestCommitSHA cond := builder.Eq{ "`index`": result.Index, "repo_id": result.RepoID, - "sha": repoIDsToLatestCommitSHAs[result.RepoID], + "sha": result.SHA, } conds = append(conds, cond) } diff --git a/models/git/commit_status_summary.go b/models/git/commit_status_summary.go new file mode 100644 index 000000000..01674e943 --- /dev/null +++ b/models/git/commit_status_summary.go @@ -0,0 +1,84 @@ +// Copyright 2024 Gitea. All rights reserved. +// SPDX-License-Identifier: MIT + +package git + +import ( + "context" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/modules/setting" + api "code.gitea.io/gitea/modules/structs" + + "xorm.io/builder" +) + +// CommitStatusSummary holds the latest commit Status of a single Commit +type CommitStatusSummary struct { + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_id_sha)"` + SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"` + State api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` +} + +func init() { + db.RegisterModel(new(CommitStatusSummary)) +} + +type RepoSHA struct { + RepoID int64 + SHA string +} + +func GetLatestCommitStatusForRepoAndSHAs(ctx context.Context, repoSHAs []RepoSHA) ([]*CommitStatus, error) { + cond := builder.NewCond() + for _, rs := range repoSHAs { + cond = cond.Or(builder.Eq{"repo_id": rs.RepoID, "sha": rs.SHA}) + } + + var summaries []CommitStatusSummary + if err := db.GetEngine(ctx).Where(cond).Find(&summaries); err != nil { + return nil, err + } + + commitStatuses := make([]*CommitStatus, 0, len(repoSHAs)) + for _, summary := range summaries { + commitStatuses = append(commitStatuses, &CommitStatus{ + RepoID: summary.RepoID, + SHA: summary.SHA, + State: summary.State, + }) + } + return commitStatuses, nil +} + +func UpdateCommitStatusSummary(ctx context.Context, repoID int64, sha string) error { + commitStatuses, _, err := GetLatestCommitStatus(ctx, repoID, sha, db.ListOptionsAll) + if err != nil { + return err + } + state := CalcCommitStatus(commitStatuses) + // mysql will return 0 when update a record which state hasn't been changed which behaviour is different from other database, + // so we need to use insert in on duplicate + if setting.Database.Type.IsMySQL() { + _, err := db.GetEngine(ctx).Exec("INSERT INTO commit_status_summary (repo_id,sha,state) VALUES (?,?,?) ON DUPLICATE KEY UPDATE state=?", + repoID, sha, state.State, state.State) + return err + } + + if cnt, err := db.GetEngine(ctx).Where("repo_id=? AND sha=?", repoID, sha). + Cols("state"). + Update(&CommitStatusSummary{ + State: state.State, + }); err != nil { + return err + } else if cnt == 0 { + _, err = db.GetEngine(ctx).Insert(&CommitStatusSummary{ + RepoID: repoID, + SHA: sha, + State: state.State, + }) + return err + } + return nil +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 88a34bd4a..18f79f778 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -578,7 +578,10 @@ var migrations = []Migration{ // Gitea 1.22.0 ends at 294 + // v294 -> v295 NewMigration("Add unique index for project issue table", v1_23.AddUniqueIndexForProjectIssue), + // v295 -> v296 + NewMigration("Add commit status summary table", v1_23.AddCommitStatusSummary), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_23/v295.go b/models/migrations/v1_23/v295.go new file mode 100644 index 000000000..9a2003cfc --- /dev/null +++ b/models/migrations/v1_23/v295.go @@ -0,0 +1,18 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_23 //nolint + +import "xorm.io/xorm" + +func AddCommitStatusSummary(x *xorm.Engine) error { + type CommitStatusSummary struct { + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_id_sha)"` + SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"` + State string `xorm:"VARCHAR(7) NOT NULL"` + } + // there is no migrations because if there is no data on this table, it will fall back to get data + // from commit status + return x.Sync2(new(CommitStatusSummary)) +} diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index 0bb738e2a..2be218081 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -7,6 +7,7 @@ import ( "context" "crypto/sha256" "fmt" + "slices" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -59,13 +60,19 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato sha = commit.ID.String() } - if err := git_model.NewCommitStatus(ctx, git_model.NewCommitStatusOptions{ - Repo: repo, - Creator: creator, - SHA: commit.ID, - CommitStatus: status, + if err := db.WithTx(ctx, func(ctx context.Context) error { + if err := git_model.NewCommitStatus(ctx, git_model.NewCommitStatusOptions{ + Repo: repo, + Creator: creator, + SHA: commit.ID, + CommitStatus: status, + }); err != nil { + return fmt.Errorf("NewCommitStatus[repo_id: %d, user_id: %d, sha: %s]: %w", repo.ID, creator.ID, sha, err) + } + + return git_model.UpdateCommitStatusSummary(ctx, repo.ID, commit.ID.String()) }); err != nil { - return fmt.Errorf("NewCommitStatus[repo_id: %d, user_id: %d, sha: %s]: %w", repo.ID, creator.ID, sha, err) + return err } defaultBranchCommit, err := gitRepo.GetBranchCommit(repo.DefaultBranch) @@ -114,8 +121,35 @@ func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Rep return nil, fmt.Errorf("FindBranchesByRepoAndBranchName: %v", err) } + var repoSHAs []git_model.RepoSHA + for id, sha := range repoIDsToLatestCommitSHAs { + repoSHAs = append(repoSHAs, git_model.RepoSHA{RepoID: id, SHA: sha}) + } + + summaryResults, err := git_model.GetLatestCommitStatusForRepoAndSHAs(ctx, repoSHAs) + if err != nil { + return nil, fmt.Errorf("GetLatestCommitStatusForRepoAndSHAs: %v", err) + } + + for _, summary := range summaryResults { + for i, repo := range repos { + if repo.ID == summary.RepoID { + results[i] = summary + _ = slices.DeleteFunc(repoSHAs, func(repoSHA git_model.RepoSHA) bool { + return repoSHA.RepoID == repo.ID + }) + if results[i].State != "" { + if err := updateCommitStatusCache(ctx, repo.ID, repo.DefaultBranch, results[i].State); err != nil { + log.Error("updateCommitStatusCache[%d:%s] failed: %v", repo.ID, repo.DefaultBranch, err) + } + } + break + } + } + } + // call the database O(1) times to get the commit statuses for all repos - repoToItsLatestCommitStatuses, err := git_model.GetLatestCommitStatusForPairs(ctx, repoIDsToLatestCommitSHAs, db.ListOptionsAll) + repoToItsLatestCommitStatuses, err := git_model.GetLatestCommitStatusForPairs(ctx, repoSHAs) if err != nil { return nil, fmt.Errorf("GetLatestCommitStatusForPairs: %v", err) } diff --git a/tests/integration/pull_status_test.go b/tests/integration/pull_status_test.go index 26c99e644..bb7098e42 100644 --- a/tests/integration/pull_status_test.go +++ b/tests/integration/pull_status_test.go @@ -12,6 +12,9 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + git_model "code.gitea.io/gitea/models/git" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" api "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" @@ -90,6 +93,10 @@ func TestPullCreate_CommitStatus(t *testing.T) { assert.True(t, ok) assert.Contains(t, cls, statesIcons[status]) } + + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: "user1", Name: "repo1"}) + css := unittest.AssertExistsAndLoadBean(t, &git_model.CommitStatusSummary{RepoID: repo1.ID, SHA: commitID}) + assert.EqualValues(t, api.CommitStatusWarning, css.State) }) } From f393ccf745f1395e2850e7623ec37ac476dde87f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Apr 2024 19:02:42 +0800 Subject: [PATCH 2/4] Fix commit status cache which missed target_url (#30426) Fix #30421 --------- Co-authored-by: Jason Song --- .../repository/commitstatus/commitstatus.go | 54 ++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index 2be218081..cbe9d4cef 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/services/automerge" @@ -26,12 +27,41 @@ func getCacheKey(repoID int64, brancheName string) string { return fmt.Sprintf("commit_status:%x", hashBytes) } -func updateCommitStatusCache(ctx context.Context, repoID int64, branchName string, status api.CommitStatusState) error { - c := cache.GetCache() - return c.Put(getCacheKey(repoID, branchName), string(status), 3*24*60) +type commitStatusCacheValue struct { + State string `json:"state"` + TargetURL string `json:"target_url"` } -func deleteCommitStatusCache(ctx context.Context, repoID int64, branchName string) error { +func getCommitStatusCache(repoID int64, branchName string) *commitStatusCacheValue { + c := cache.GetCache() + statusStr, ok := c.Get(getCacheKey(repoID, branchName)).(string) + if ok && statusStr != "" { + var cv commitStatusCacheValue + err := json.Unmarshal([]byte(statusStr), &cv) + if err == nil && cv.State != "" { + return &cv + } + if err != nil { + log.Warn("getCommitStatusCache: json.Unmarshal failed: %v", err) + } + } + return nil +} + +func updateCommitStatusCache(repoID int64, branchName string, state api.CommitStatusState, targetURL string) error { + c := cache.GetCache() + bs, err := json.Marshal(commitStatusCacheValue{ + State: state.String(), + TargetURL: targetURL, + }) + if err != nil { + log.Warn("updateCommitStatusCache: json.Marshal failed: %v", err) + return nil + } + return c.Put(getCacheKey(repoID, branchName), string(bs), 3*24*60) +} + +func deleteCommitStatusCache(repoID int64, branchName string) error { c := cache.GetCache() return c.Delete(getCacheKey(repoID, branchName)) } @@ -81,7 +111,7 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato } if commit.ID.String() == defaultBranchCommit.ID.String() { // since one commit status updated, the combined commit status should be invalid - if err := deleteCommitStatusCache(ctx, repo.ID, repo.DefaultBranch); err != nil { + if err := deleteCommitStatusCache(repo.ID, repo.DefaultBranch); err != nil { log.Error("deleteCommitStatusCache[%d:%s] failed: %v", repo.ID, repo.DefaultBranch, err) } } @@ -98,12 +128,12 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato // FindReposLastestCommitStatuses loading repository default branch latest combinded commit status with cache func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Repository) ([]*git_model.CommitStatus, error) { results := make([]*git_model.CommitStatus, len(repos)) - c := cache.GetCache() - for i, repo := range repos { - status, ok := c.Get(getCacheKey(repo.ID, repo.DefaultBranch)).(string) - if ok && status != "" { - results[i] = &git_model.CommitStatus{State: api.CommitStatusState(status)} + if cv := getCommitStatusCache(repo.ID, repo.DefaultBranch); cv != nil { + results[i] = &git_model.CommitStatus{ + State: api.CommitStatusState(cv.State), + TargetURL: cv.TargetURL, + } } } @@ -139,7 +169,7 @@ func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Rep return repoSHA.RepoID == repo.ID }) if results[i].State != "" { - if err := updateCommitStatusCache(ctx, repo.ID, repo.DefaultBranch, results[i].State); err != nil { + if err := updateCommitStatusCache(repo.ID, repo.DefaultBranch, results[i].State, results[i].TargetURL); err != nil { log.Error("updateCommitStatusCache[%d:%s] failed: %v", repo.ID, repo.DefaultBranch, err) } } @@ -158,7 +188,7 @@ func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Rep if results[i] == nil { results[i] = git_model.CalcCommitStatus(repoToItsLatestCommitStatuses[repo.ID]) if results[i] != nil { - if err := updateCommitStatusCache(ctx, repo.ID, repo.DefaultBranch, results[i].State); err != nil { + if err := updateCommitStatusCache(repo.ID, repo.DefaultBranch, results[i].State, results[i].TargetURL); err != nil { log.Error("updateCommitStatusCache[%d:%s] failed: %v", repo.ID, repo.DefaultBranch, err) } } From c0d22d55c90fbdb3ffea9c394ff9bc7f6e6a264f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 14 Apr 2024 20:16:54 +0800 Subject: [PATCH 3/4] Fix commitstatus summary (#30431) The target_url is necessary for the UI, but missed in commit_status_summary table. This PR fix it. --------- Co-authored-by: silverwind Co-authored-by: delvh Co-authored-by: Giteabot --- models/git/commit_status_summary.go | 32 ++++++++++++++++------------- models/migrations/migrations.go | 2 ++ models/migrations/v1_23/v296.go | 16 +++++++++++++++ 3 files changed, 36 insertions(+), 14 deletions(-) create mode 100644 models/migrations/v1_23/v296.go diff --git a/models/git/commit_status_summary.go b/models/git/commit_status_summary.go index 01674e943..7603e7aa6 100644 --- a/models/git/commit_status_summary.go +++ b/models/git/commit_status_summary.go @@ -15,10 +15,11 @@ import ( // CommitStatusSummary holds the latest commit Status of a single Commit type CommitStatusSummary struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_id_sha)"` - SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"` - State api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_id_sha)"` + SHA string `xorm:"VARCHAR(64) NOT NULL INDEX UNIQUE(repo_id_sha)"` + State api.CommitStatusState `xorm:"VARCHAR(7) NOT NULL"` + TargetURL string `xorm:"TEXT"` } func init() { @@ -44,9 +45,10 @@ func GetLatestCommitStatusForRepoAndSHAs(ctx context.Context, repoSHAs []RepoSHA commitStatuses := make([]*CommitStatus, 0, len(repoSHAs)) for _, summary := range summaries { commitStatuses = append(commitStatuses, &CommitStatus{ - RepoID: summary.RepoID, - SHA: summary.SHA, - State: summary.State, + RepoID: summary.RepoID, + SHA: summary.SHA, + State: summary.State, + TargetURL: summary.TargetURL, }) } return commitStatuses, nil @@ -61,22 +63,24 @@ func UpdateCommitStatusSummary(ctx context.Context, repoID int64, sha string) er // mysql will return 0 when update a record which state hasn't been changed which behaviour is different from other database, // so we need to use insert in on duplicate if setting.Database.Type.IsMySQL() { - _, err := db.GetEngine(ctx).Exec("INSERT INTO commit_status_summary (repo_id,sha,state) VALUES (?,?,?) ON DUPLICATE KEY UPDATE state=?", - repoID, sha, state.State, state.State) + _, err := db.GetEngine(ctx).Exec("INSERT INTO commit_status_summary (repo_id,sha,state,target_url) VALUES (?,?,?,?) ON DUPLICATE KEY UPDATE state=?", + repoID, sha, state.State, state.TargetURL, state.State) return err } if cnt, err := db.GetEngine(ctx).Where("repo_id=? AND sha=?", repoID, sha). - Cols("state"). + Cols("state, target_url"). Update(&CommitStatusSummary{ - State: state.State, + State: state.State, + TargetURL: state.TargetURL, }); err != nil { return err } else if cnt == 0 { _, err = db.GetEngine(ctx).Insert(&CommitStatusSummary{ - RepoID: repoID, - SHA: sha, - State: state.State, + RepoID: repoID, + SHA: sha, + State: state.State, + TargetURL: state.TargetURL, }) return err } diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 18f79f778..338aebb11 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -582,6 +582,8 @@ var migrations = []Migration{ NewMigration("Add unique index for project issue table", v1_23.AddUniqueIndexForProjectIssue), // v295 -> v296 NewMigration("Add commit status summary table", v1_23.AddCommitStatusSummary), + // v296 -> v297 + NewMigration("Add missing field of commit status summary table", v1_23.AddCommitStatusSummary2), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v1_23/v296.go b/models/migrations/v1_23/v296.go new file mode 100644 index 000000000..495ae2ab2 --- /dev/null +++ b/models/migrations/v1_23/v296.go @@ -0,0 +1,16 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_23 //nolint + +import "xorm.io/xorm" + +func AddCommitStatusSummary2(x *xorm.Engine) error { + type CommitStatusSummary struct { + ID int64 `xorm:"pk autoincr"` + TargetURL string `xorm:"TEXT"` + } + // there is no migrations because if there is no data on this table, it will fall back to get data + // from commit status + return x.Sync(new(CommitStatusSummary)) +} From 8e9c296014bdd178f0fdb8436820e594d18e007f Mon Sep 17 00:00:00 2001 From: Michael Kriese Date: Tue, 16 Apr 2024 12:02:01 +0200 Subject: [PATCH 4/4] fix: don't update cache for empty state --- services/repository/commitstatus/commitstatus.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index cbe9d4cef..bd40a75dc 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -187,7 +187,7 @@ func FindReposLastestCommitStatuses(ctx context.Context, repos []*repo_model.Rep for i, repo := range repos { if results[i] == nil { results[i] = git_model.CalcCommitStatus(repoToItsLatestCommitStatuses[repo.ID]) - if results[i] != nil { + if results[i] != nil && results[i].State != "" { if err := updateCommitStatusCache(repo.ID, repo.DefaultBranch, results[i].State, results[i].TargetURL); err != nil { log.Error("updateCommitStatusCache[%d:%s] failed: %v", repo.ID, repo.DefaultBranch, err) }