Merge pull request 'Add commit status summary table to reduce query from commit status table' (#3245) from viceice/forgejo:feat/commit-status-summary into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3245
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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)
 		}

models/git/commit_status_summary.go

@@ -0,0 +1,88 @@
+// 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"`
+	TargetURL string                `xorm:"TEXT"`
+}
+
+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,
+			TargetURL: summary.TargetURL,
+		})
+	}
+	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,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, target_url").
+		Update(&CommitStatusSummary{
+			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,
+			TargetURL: state.TargetURL,
+		})
+		return err
+	}
+	return nil
+}

models/migrations/migrations.go

@@ -578,7 +578,12 @@ 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),
+	// v296 -> v297
+	NewMigration("Add missing field of commit status summary table", v1_23.AddCommitStatusSummary2),
 }
 
 // GetCurrentDBVersion returns the current db version

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))
+}

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))
+}

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"
@@ -15,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"
@@ -25,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))
 }
@@ -59,6 +90,7 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato
 		sha = commit.ID.String()
 	}
 
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
 		if err := git_model.NewCommitStatus(ctx, git_model.NewCommitStatusOptions{
 			Repo:         repo,
 			Creator:      creator,
@@ -68,13 +100,18 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato
 			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 err
+	}
+
 	defaultBranchCommit, err := gitRepo.GetBranchCommit(repo.DefaultBranch)
 	if err != nil {
 		return fmt.Errorf("GetBranchCommit[%s]: %w", repo.DefaultBranch, err)
 	}
 
 	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)
 		}
 	}
@@ -91,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,
+			}
 		}
 	}
 
@@ -114,8 +151,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(repo.ID, repo.DefaultBranch, results[i].State, results[i].TargetURL); 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)
 	}
@@ -123,8 +187,8 @@ 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 err := updateCommitStatusCache(ctx, repo.ID, repo.DefaultBranch, results[i].State); err != 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)
 				}
 			}

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)
 	})
 }