From 0d029ebe6d7ed609d7521f7e6b1c09486a4fef55 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 20:53:47 +0200 Subject: [PATCH 1/2] Fix git_model.FindBranchesByRepoAndBranchName When a logged in user with no repositories visits their dashboard, it will display a search box that lists their own repositories. This is served by the `repo.SearchRepos` handler, which in turn calls `commitstatus_service.FindReposLastestCommitStatuses()` with an empty repo list. That, in turn, will call `git_model.FindBranchesByRepoAndBranchName()`, with an empty map. With no map, `FindBranchesByRepoAndBranchName()` ends up querying the entire `branch` table, because no conditions were set up. Armed with a gazillion repo & commit shas, we return to `FindReposLastestCommitStatuses`, and promptly call `git_model.GetLatestCommitStatusForPairs`, which constructs a monstrous query with so many placeholders that the database tells us to go somewhere else, and flips us off. At least on instances the size of Codeberg. On smaller instances, it will eventually return, and throw away all the data, and return an empty set, having performed all this for naught. We fix this by short-circuiting `FindBranchesByRepoAndBranchName`, and returning fast if our inputs are empty. A test case is included. Fixes #3521. Signed-off-by: Gergely Nagy --- models/git/branch_list.go | 3 +++ models/git/branch_test.go | 9 +++++++++ release-notes/8.0.0/fix/3570.md | 1 + 3 files changed, 13 insertions(+) create mode 100644 release-notes/8.0.0/fix/3570.md diff --git a/models/git/branch_list.go b/models/git/branch_list.go index 980bd7b4c9..493611f217 100644 --- a/models/git/branch_list.go +++ b/models/git/branch_list.go @@ -116,6 +116,9 @@ func FindBranchNames(ctx context.Context, opts FindBranchOptions) ([]string, err } func FindBranchesByRepoAndBranchName(ctx context.Context, repoBranches map[int64]string) (map[int64]string, error) { + if len(repoBranches) == 0 { + return nil, nil + } cond := builder.NewCond() for repoID, branchName := range repoBranches { cond = cond.Or(builder.And(builder.Eq{"repo_id": repoID}, builder.Eq{"name": branchName})) diff --git a/models/git/branch_test.go b/models/git/branch_test.go index b8ea663e81..3aa578f44b 100644 --- a/models/git/branch_test.go +++ b/models/git/branch_test.go @@ -183,3 +183,12 @@ func TestOnlyGetDeletedBranchOnCorrectRepo(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, deletedBranch) } + +func TestFindBranchesByRepoAndBranchName(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // With no repos or branches given, we find no branches. + branches, err := git_model.FindBranchesByRepoAndBranchName(db.DefaultContext, map[int64]string{}) + assert.NoError(t, err) + assert.Len(t, branches, 0) +} diff --git a/release-notes/8.0.0/fix/3570.md b/release-notes/8.0.0/fix/3570.md new file mode 100644 index 0000000000..1e860eb958 --- /dev/null +++ b/release-notes/8.0.0/fix/3570.md @@ -0,0 +1 @@ +Fixed an issue that resulted in excessive and unnecessary database queries when a user with no repositories was viewing their dashboard. From 33cd8446d3675796939c7204e31814c0c0bbcbab Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 21:32:23 +0200 Subject: [PATCH 2/2] Performance improvement for FindReposLastestCommitStatuses If `commitstatus_service.FindReposLastestCommitStatuses` receives no repos in its params, short-circuit, and return early, without performing any potentially expensive work. Signed-off-by: Gergely Nagy --- services/repository/commitstatus/commitstatus.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/repository/commitstatus/commitstatus.go b/services/repository/commitstatus/commitstatus.go index bd40a75dc8..1944f11f03 100644 --- a/services/repository/commitstatus/commitstatus.go +++ b/services/repository/commitstatus/commitstatus.go @@ -127,6 +127,9 @@ 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) { + if len(repos) == 0 { + return nil, nil + } results := make([]*git_model.CommitStatus, len(repos)) for i, repo := range repos { if cv := getCommitStatusCache(repo.ID, repo.DefaultBranch); cv != nil {