From dd8d3f5ebe619ae16dc7f7142676aa2cd23d79e7 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 20:53:47 +0200 Subject: [PATCH] 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 (cherry picked from commit 0d029ebe6d7ed609d7521f7e6b1c09486a4fef55) --- 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 8319e5ecd0..c6fc8ad4b1 100644 --- a/models/git/branch_list.go +++ b/models/git/branch_list.go @@ -120,6 +120,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.