From 70327d6a92ac973e6c48a0f3086fa8dccdcb8899 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 14 Feb 2025 00:05:55 -0800 Subject: [PATCH] Improve commits list performance to reduce unnecessary database queries (#33528) When listing commits, Gitea attempts to retrieve the actual user based on the commit email. Querying users one by one from the database is inefficient. This PR optimizes the process by batch querying users by email, reducing the number of database queries. --- models/asymkey/gpg_key_commit_verification.go | 31 +++++++- models/git/commit_status.go | 38 +++++---- models/issues/comment.go | 5 +- models/user/user.go | 79 ++++++++++++++++--- routers/web/repo/blame.go | 7 +- routers/web/repo/commit.go | 27 +++++-- routers/web/repo/compare.go | 6 +- routers/web/repo/pull.go | 6 +- routers/web/repo/wiki.go | 9 ++- tests/integration/repo_commits_test.go | 41 ++++++++++ 10 files changed, 207 insertions(+), 42 deletions(-) diff --git a/models/asymkey/gpg_key_commit_verification.go b/models/asymkey/gpg_key_commit_verification.go index 9219a509df..04cbf20f19 100644 --- a/models/asymkey/gpg_key_commit_verification.go +++ b/models/asymkey/gpg_key_commit_verification.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -71,21 +72,41 @@ const ( ) // ParseCommitsWithSignature checks if signaute of commits are corresponding to users gpg keys. -func ParseCommitsWithSignature(ctx context.Context, oldCommits []*user_model.UserCommit, repoTrustModel repo_model.TrustModelType, isOwnerMemberCollaborator func(*user_model.User) (bool, error)) []*SignCommit { +func ParseCommitsWithSignature(ctx context.Context, oldCommits []*user_model.UserCommit, repoTrustModel repo_model.TrustModelType, isOwnerMemberCollaborator func(*user_model.User) (bool, error)) ([]*SignCommit, error) { newCommits := make([]*SignCommit, 0, len(oldCommits)) keyMap := map[string]bool{} + emails := make(container.Set[string]) for _, c := range oldCommits { + if c.Committer != nil { + emails.Add(c.Committer.Email) + } + } + + emailUsers, err := user_model.GetUsersByEmails(ctx, emails.Values()) + if err != nil { + return nil, err + } + + for _, c := range oldCommits { + committer, ok := emailUsers[c.Committer.Email] + if !ok && c.Committer != nil { + committer = &user_model.User{ + Name: c.Committer.Name, + Email: c.Committer.Email, + } + } + signCommit := &SignCommit{ UserCommit: c, - Verification: ParseCommitWithSignature(ctx, c.Commit), + Verification: parseCommitWithSignatureCommitter(ctx, c.Commit, committer), } _ = CalculateTrustStatus(signCommit.Verification, repoTrustModel, isOwnerMemberCollaborator, &keyMap) newCommits = append(newCommits, signCommit) } - return newCommits + return newCommits, nil } // ParseCommitWithSignature check if signature is good against keystore. @@ -113,6 +134,10 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific } } + return parseCommitWithSignatureCommitter(ctx, c, committer) +} + +func parseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, committer *user_model.User) *CommitVerification { // If no signature just report the committer if c.Signature == nil { return &CommitVerification{ diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 0579a41209..ad3624bbc9 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -497,7 +497,7 @@ type SignCommitWithStatuses struct { } // ParseCommitsWithStatus checks commits latest statuses and calculates its worst status state -func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.SignCommit, repo *repo_model.Repository) []*SignCommitWithStatuses { +func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.SignCommit, repo *repo_model.Repository) ([]*SignCommitWithStatuses, error) { newCommits := make([]*SignCommitWithStatuses, 0, len(oldCommits)) for _, c := range oldCommits { @@ -506,15 +506,14 @@ func ParseCommitsWithStatus(ctx context.Context, oldCommits []*asymkey_model.Sig } statuses, _, err := GetLatestCommitStatus(ctx, repo.ID, commit.ID.String(), db.ListOptions{}) if err != nil { - log.Error("GetLatestCommitStatus: %v", err) - } else { - commit.Statuses = statuses - commit.Status = CalcCommitStatus(statuses) + return nil, err } + commit.Statuses = statuses + commit.Status = CalcCommitStatus(statuses) newCommits = append(newCommits, commit) } - return newCommits + return newCommits, nil } // hashCommitStatusContext hash context @@ -523,18 +522,23 @@ func hashCommitStatusContext(context string) string { } // ConvertFromGitCommit converts git commits into SignCommitWithStatuses -func ConvertFromGitCommit(ctx context.Context, commits []*git.Commit, repo *repo_model.Repository) []*SignCommitWithStatuses { - return ParseCommitsWithStatus(ctx, - asymkey_model.ParseCommitsWithSignature( - ctx, - user_model.ValidateCommitsWithEmails(ctx, commits), - repo.GetTrustModel(), - func(user *user_model.User) (bool, error) { - return repo_model.IsOwnerMemberCollaborator(ctx, repo, user.ID) - }, - ), - repo, +func ConvertFromGitCommit(ctx context.Context, commits []*git.Commit, repo *repo_model.Repository) ([]*SignCommitWithStatuses, error) { + validatedCommits, err := user_model.ValidateCommitsWithEmails(ctx, commits) + if err != nil { + return nil, err + } + signedCommits, err := asymkey_model.ParseCommitsWithSignature( + ctx, + validatedCommits, + repo.GetTrustModel(), + func(user *user_model.User) (bool, error) { + return repo_model.IsOwnerMemberCollaborator(ctx, repo, user.ID) + }, ) + if err != nil { + return nil, err + } + return ParseCommitsWithStatus(ctx, signedCommits, repo) } // CommitStatusesHideActionsURL hide Gitea Actions urls diff --git a/models/issues/comment.go b/models/issues/comment.go index a248708820..49dd12ab01 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -802,7 +802,10 @@ func (c *Comment) LoadPushCommits(ctx context.Context) (err error) { } defer closer.Close() - c.Commits = git_model.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo) + c.Commits, err = git_model.ConvertFromGitCommit(ctx, gitRepo.GetCommitsFromIDs(data.CommitIDs), c.Issue.Repo) + if err != nil { + return err + } c.CommitsNum = int64(len(c.Commits)) } diff --git a/models/user/user.go b/models/user/user.go index 293c876957..12692e2ae1 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -1129,28 +1129,85 @@ func ValidateCommitWithEmail(ctx context.Context, c *git.Commit) *User { } // ValidateCommitsWithEmails checks if authors' e-mails of commits are corresponding to users. -func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) []*UserCommit { +func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) ([]*UserCommit, error) { var ( - emails = make(map[string]*User) newCommits = make([]*UserCommit, 0, len(oldCommits)) + emailSet = make(container.Set[string]) ) for _, c := range oldCommits { - var u *User if c.Author != nil { - if v, ok := emails[c.Author.Email]; !ok { - u, _ = GetUserByEmail(ctx, c.Author.Email) - emails[c.Author.Email] = u - } else { - u = v - } + emailSet.Add(c.Author.Email) } + } + + emailUserMap, err := GetUsersByEmails(ctx, emailSet.Values()) + if err != nil { + return nil, err + } + for _, c := range oldCommits { + user, ok := emailUserMap[c.Author.Email] + if !ok { + user = &User{ + Name: c.Author.Name, + Email: c.Author.Email, + } + } newCommits = append(newCommits, &UserCommit{ - User: u, + User: user, Commit: c, }) } - return newCommits + return newCommits, nil +} + +func GetUsersByEmails(ctx context.Context, emails []string) (map[string]*User, error) { + if len(emails) == 0 { + return nil, nil + } + + needCheckEmails := make(container.Set[string]) + needCheckUserNames := make(container.Set[string]) + for _, email := range emails { + if strings.HasSuffix(email, fmt.Sprintf("@%s", setting.Service.NoReplyAddress)) { + username := strings.TrimSuffix(email, fmt.Sprintf("@%s", setting.Service.NoReplyAddress)) + needCheckUserNames.Add(username) + } else { + needCheckEmails.Add(strings.ToLower(email)) + } + } + + emailAddresses := make([]*EmailAddress, 0, len(needCheckEmails)) + if err := db.GetEngine(ctx).In("lower_email", needCheckEmails.Values()). + And("is_activated=?", true). + Find(&emailAddresses); err != nil { + return nil, err + } + userIDs := make(container.Set[int64]) + for _, email := range emailAddresses { + userIDs.Add(email.UID) + } + users, err := GetUsersByIDs(ctx, userIDs.Values()) + if err != nil { + return nil, err + } + + results := make(map[string]*User, len(emails)) + for _, user := range users { + if user.KeepEmailPrivate { + results[user.LowerName+"@"+setting.Service.NoReplyAddress] = user + } else { + results[user.Email] = user + } + } + users = make([]*User, 0, len(needCheckUserNames)) + if err := db.GetEngine(ctx).In("lower_name", needCheckUserNames.Values()).Find(&users); err != nil { + return nil, err + } + for _, user := range users { + results[user.LowerName+"@"+setting.Service.NoReplyAddress] = user + } + return results, nil } // GetUserByEmail returns the user object by given e-mail if exists. diff --git a/routers/web/repo/blame.go b/routers/web/repo/blame.go index 1022c5e6d6..27de35efb2 100644 --- a/routers/web/repo/blame.go +++ b/routers/web/repo/blame.go @@ -234,7 +234,12 @@ func processBlameParts(ctx *context.Context, blameParts []*git.BlamePart) map[st } // populate commit email addresses to later look up avatars. - for _, c := range user_model.ValidateCommitsWithEmails(ctx, commits) { + validatedCommits, err := user_model.ValidateCommitsWithEmails(ctx, commits) + if err != nil { + ctx.ServerError("ValidateCommitsWithEmails", err) + return nil + } + for _, c := range validatedCommits { commitNames[c.ID.String()] = c } diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index c8291d98c6..24e1636e98 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -80,7 +80,11 @@ func Commits(ctx *context.Context) { ctx.ServerError("CommitsByRange", err) return } - ctx.Data["Commits"] = processGitCommits(ctx, commits) + ctx.Data["Commits"], err = processGitCommits(ctx, commits) + if err != nil { + ctx.ServerError("processGitCommits", err) + return + } commitIDs := make([]string, 0, len(commits)) for _, c := range commits { commitIDs = append(commitIDs, c.ID.String()) @@ -192,7 +196,11 @@ func SearchCommits(ctx *context.Context) { return } ctx.Data["CommitCount"] = len(commits) - ctx.Data["Commits"] = processGitCommits(ctx, commits) + ctx.Data["Commits"], err = processGitCommits(ctx, commits) + if err != nil { + ctx.ServerError("processGitCommits", err) + return + } ctx.Data["Keyword"] = query if all { @@ -235,7 +243,11 @@ func FileHistory(ctx *context.Context) { ctx.ServerError("CommitsByFileAndRange", err) return } - ctx.Data["Commits"] = processGitCommits(ctx, commits) + ctx.Data["Commits"], err = processGitCommits(ctx, commits) + if err != nil { + ctx.ServerError("processGitCommits", err) + return + } ctx.Data["Username"] = ctx.Repo.Owner.Name ctx.Data["Reponame"] = ctx.Repo.Repository.Name @@ -416,13 +428,16 @@ func RawDiff(ctx *context.Context) { } } -func processGitCommits(ctx *context.Context, gitCommits []*git.Commit) []*git_model.SignCommitWithStatuses { - commits := git_model.ConvertFromGitCommit(ctx, gitCommits, ctx.Repo.Repository) +func processGitCommits(ctx *context.Context, gitCommits []*git.Commit) ([]*git_model.SignCommitWithStatuses, error) { + commits, err := git_model.ConvertFromGitCommit(ctx, gitCommits, ctx.Repo.Repository) + if err != nil { + return nil, err + } if !ctx.Repo.CanRead(unit_model.TypeActions) { for _, commit := range commits { commit.Status.HideActionsURL(ctx) git_model.CommitStatusesHideActionsURL(ctx, commit.Statuses) } } - return commits + return commits, nil } diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index b3c1eb7cb0..cb16a3819a 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -647,7 +647,11 @@ func PrepareCompareDiff( return false } - commits := processGitCommits(ctx, ci.CompareInfo.Commits) + commits, err := processGitCommits(ctx, ci.CompareInfo.Commits) + if err != nil { + ctx.ServerError("processGitCommits", err) + return false + } ctx.Data["Commits"] = commits ctx.Data["CommitCount"] = len(commits) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 0c82736eef..43524daeb4 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -631,7 +631,11 @@ func ViewPullCommits(ctx *context.Context) { ctx.Data["Username"] = ctx.Repo.Owner.Name ctx.Data["Reponame"] = ctx.Repo.Repository.Name - commits := processGitCommits(ctx, prInfo.Commits) + commits, err := processGitCommits(ctx, prInfo.Commits) + if err != nil { + ctx.ServerError("processGitCommits", err) + return + } ctx.Data["Commits"] = commits ctx.Data["CommitCount"] = len(commits) diff --git a/routers/web/repo/wiki.go b/routers/web/repo/wiki.go index 19366c0104..f712a71604 100644 --- a/routers/web/repo/wiki.go +++ b/routers/web/repo/wiki.go @@ -437,7 +437,14 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) ctx.ServerError("CommitsByFileAndRange", err) return nil, nil } - ctx.Data["Commits"] = git_model.ConvertFromGitCommit(ctx, commitsHistory, ctx.Repo.Repository) + ctx.Data["Commits"], err = git_model.ConvertFromGitCommit(ctx, commitsHistory, ctx.Repo.Repository) + if err != nil { + if wikiRepo != nil { + wikiRepo.Close() + } + ctx.ServerError("ConvertFromGitCommit", err) + return nil, nil + } pager := context.NewPagination(int(commitsCount), setting.Git.CommitsRangeSize, page, 5) pager.AddParamFromRequest(ctx.Req) diff --git a/tests/integration/repo_commits_test.go b/tests/integration/repo_commits_test.go index fc066e06d3..fbe215eb85 100644 --- a/tests/integration/repo_commits_test.go +++ b/tests/integration/repo_commits_test.go @@ -12,11 +12,14 @@ import ( "testing" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/tests" + "github.com/PuerkitoBio/goquery" "github.com/stretchr/testify/assert" ) @@ -35,6 +38,44 @@ func TestRepoCommits(t *testing.T) { assert.NotEmpty(t, commitURL) } +func Test_ReposGitCommitListNotMaster(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + + // Test getting commits (Page 1) + req := NewRequestf(t, "GET", "/%s/repo16/commits/branch/master", user.Name) + resp := session.MakeRequest(t, req, http.StatusOK) + + doc := NewHTMLParser(t, resp.Body) + commits := []string{} + doc.doc.Find("#commits-table .commit-id-short").Each(func(i int, s *goquery.Selection) { + commitURL, exists := s.Attr("href") + assert.True(t, exists) + assert.NotEmpty(t, commitURL) + commits = append(commits, path.Base(commitURL)) + }) + + assert.Len(t, commits, 3) + assert.EqualValues(t, "69554a64c1e6030f051e5c3f94bfbd773cd6a324", commits[0]) + assert.EqualValues(t, "27566bd5738fc8b4e3fef3c5e72cce608537bd95", commits[1]) + assert.EqualValues(t, "5099b81332712fe655e34e8dd63574f503f61811", commits[2]) + + userNames := []string{} + doc.doc.Find("#commits-table .author-wrapper").Each(func(i int, s *goquery.Selection) { + userPath, exists := s.Attr("href") + assert.True(t, exists) + assert.NotEmpty(t, userPath) + userNames = append(userNames, path.Base(userPath)) + }) + + assert.Len(t, userNames, 3) + assert.EqualValues(t, "User2", userNames[0]) + assert.EqualValues(t, "user21", userNames[1]) + assert.EqualValues(t, "User2", userNames[2]) +} + func doTestRepoCommitWithStatus(t *testing.T, state string, classes ...string) { defer tests.PrepareTestEnv(t)()