From 9c92ae14e08146ab074ba360d80b859eadc2a118 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 16 Nov 2024 02:28:38 +0100 Subject: [PATCH] feat: improve `GetLatestCommitStatusForPairs` - Simplify the function into a single SQL query. This may or may not help with a monster query we are seeing in Codeberg that is using 400MiB and takes 50MiB to simply log the query. The result is now capped to the actual latest index, - Add unit test. --- models/fixtures/commit_status.yml | 32 ++++++++- models/git/commit_status.go | 43 +++--------- models/git/commit_status_test.go | 109 +++++++++++++++++++++++++++++- 3 files changed, 145 insertions(+), 39 deletions(-) diff --git a/models/fixtures/commit_status.yml b/models/fixtures/commit_status.yml index 0ba6caafe9..c568e89cea 100644 --- a/models/fixtures/commit_status.yml +++ b/models/fixtures/commit_status.yml @@ -7,6 +7,7 @@ target_url: https://example.com/builds/ description: My awesome CI-service context: ci/awesomeness + context_hash: c65f4d64a3b14a3eced0c9b36799e66e1bd5ced7 creator_id: 2 - @@ -18,6 +19,7 @@ target_url: https://example.com/coverage/ description: My awesome Coverage service context: cov/awesomeness + context_hash: 3929ac7bccd3fa1bf9b38ddedb77973b1b9a8cfe creator_id: 2 - @@ -29,6 +31,7 @@ target_url: https://example.com/coverage/ description: My awesome Coverage service context: cov/awesomeness + context_hash: 3929ac7bccd3fa1bf9b38ddedb77973b1b9a8cfe creator_id: 2 - @@ -40,6 +43,7 @@ target_url: https://example.com/builds/ description: My awesome CI-service context: ci/awesomeness + context_hash: c65f4d64a3b14a3eced0c9b36799e66e1bd5ced7 creator_id: 2 - @@ -51,15 +55,41 @@ target_url: https://example.com/builds/ description: My awesome deploy service context: deploy/awesomeness + context_hash: ae9547713a6665fc4261d0756904932085a41cf2 creator_id: 2 - id: 6 - index: 6 + index: 1 repo_id: 62 state: "failure" sha: "774f93df12d14931ea93259ae93418da4482fcc1" target_url: "/user2/test_workflows/actions" description: My awesome deploy service context: deploy/awesomeness + context_hash: ae9547713a6665fc4261d0756904932085a41cf2 + creator_id: 2 + +- + id: 7 + index: 6 + repo_id: 1 + state: "pending" + sha: "1234123412341234123412341234123412341234" + target_url: https://example.com/builds/ + description: My awesome deploy service + context: deploy/awesomeness + context_hash: ae9547713a6665fc4261d0756904932085a41cf2 + creator_id: 2 + +- + id: 8 + index: 2 + repo_id: 62 + state: "error" + sha: "774f93df12d14931ea93259ae93418da4482fcc1" + target_url: "/user2/test_workflows/actions" + description: "My awesome deploy service - v2" + context: deploy/awesomeness + context_hash: ae9547713a6665fc4261d0756904932085a41cf2 creator_id: 2 diff --git a/models/git/commit_status.go b/models/git/commit_status.go index 53d1ddc8c3..438eefe81b 100644 --- a/models/git/commit_status.go +++ b/models/git/commit_status.go @@ -288,27 +288,18 @@ 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, repoSHAs []RepoSHA) (map[int64][]*CommitStatus, error) { - type result struct { - Index int64 - RepoID int64 - SHA string - } - - results := make([]result, 0, len(repoSHAs)) - - getBase := func() *xorm.Session { - return db.GetEngine(ctx).Table(&CommitStatus{}) - } + results := []*CommitStatus{} // Create a disjunction of conditions for each repoID and SHA pair 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, sha"). - GroupBy("context_hash, repo_id, sha").OrderBy("max( `index` ) desc") + sess := db.GetEngine(ctx).Table(&CommitStatus{}). + Select("MAX(`index`) AS `index`, *"). + Where(builder.Or(conds...)). + GroupBy("context_hash, repo_id, sha").OrderBy("MAX(`index`) DESC") err := sess.Find(&results) if err != nil { return nil, err @@ -316,27 +307,9 @@ func GetLatestCommitStatusForPairs(ctx context.Context, repoSHAs []RepoSHA) (map repoStatuses := make(map[int64][]*CommitStatus) - if len(results) > 0 { - statuses := make([]*CommitStatus, 0, len(results)) - - conds = make([]builder.Cond, 0, len(results)) - for _, result := range results { - cond := builder.Eq{ - "`index`": result.Index, - "repo_id": result.RepoID, - "sha": result.SHA, - } - conds = append(conds, cond) - } - err = getBase().Where(builder.Or(conds...)).Find(&statuses) - if err != nil { - return nil, err - } - - // Group the statuses by repo ID - for _, status := range statuses { - repoStatuses[status.RepoID] = append(repoStatuses[status.RepoID], status) - } + // Group the statuses by repo ID + for _, status := range results { + repoStatuses[status.RepoID] = append(repoStatuses[status.RepoID], status) } return repoStatuses, nil diff --git a/models/git/commit_status_test.go b/models/git/commit_status_test.go index 1014ee1e13..82440e22a1 100644 --- a/models/git/commit_status_test.go +++ b/models/git/commit_status_test.go @@ -35,8 +35,8 @@ func TestGetCommitStatuses(t *testing.T) { SHA: sha1, }) require.NoError(t, err) - assert.Equal(t, 5, int(maxResults)) - assert.Len(t, statuses, 5) + assert.EqualValues(t, 6, maxResults) + assert.Len(t, statuses, 6) assert.Equal(t, "ci/awesomeness", statuses[0].Context) assert.Equal(t, structs.CommitStatusPending, statuses[0].State) @@ -58,13 +58,17 @@ func TestGetCommitStatuses(t *testing.T) { assert.Equal(t, structs.CommitStatusError, statuses[4].State) assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[4].APIURL(db.DefaultContext)) + assert.Equal(t, "deploy/awesomeness", statuses[5].Context) + assert.Equal(t, structs.CommitStatusPending, statuses[5].State) + assert.Equal(t, "https://try.gitea.io/api/v1/repos/user2/repo1/statuses/1234123412341234123412341234123412341234", statuses[5].APIURL(db.DefaultContext)) + statuses, maxResults, err = db.FindAndCount[git_model.CommitStatus](db.DefaultContext, &git_model.CommitStatusOptions{ ListOptions: db.ListOptions{Page: 2, PageSize: 50}, RepoID: repo1.ID, SHA: sha1, }) require.NoError(t, err) - assert.Equal(t, 5, int(maxResults)) + assert.EqualValues(t, 6, maxResults) assert.Empty(t, statuses) } @@ -265,3 +269,102 @@ func TestCommitStatusesHideActionsURL(t *testing.T) { assert.Empty(t, statuses[0].TargetURL) assert.Equal(t, "https://mycicd.org/1", statuses[1].TargetURL) } + +func TestGetLatestCommitStatusForPairs(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("All", func(t *testing.T) { + pairs, err := git_model.GetLatestCommitStatusForPairs(db.DefaultContext, nil) + require.NoError(t, err) + + assert.EqualValues(t, map[int64][]*git_model.CommitStatus{ + 1: { + { + ID: 7, + Index: 6, + RepoID: 1, + State: structs.CommitStatusPending, + SHA: "1234123412341234123412341234123412341234", + TargetURL: "https://example.com/builds/", + Description: "My awesome deploy service", + ContextHash: "ae9547713a6665fc4261d0756904932085a41cf2", + Context: "deploy/awesomeness", + CreatorID: 2, + }, + { + ID: 4, + Index: 4, + State: structs.CommitStatusFailure, + TargetURL: "https://example.com/builds/", + Description: "My awesome CI-service", + Context: "ci/awesomeness", + CreatorID: 2, + RepoID: 1, + SHA: "1234123412341234123412341234123412341234", + ContextHash: "c65f4d64a3b14a3eced0c9b36799e66e1bd5ced7", + }, + { + ID: 3, + Index: 3, + State: structs.CommitStatusSuccess, + TargetURL: "https://example.com/coverage/", + Description: "My awesome Coverage service", + Context: "cov/awesomeness", + CreatorID: 2, + RepoID: 1, + SHA: "1234123412341234123412341234123412341234", + ContextHash: "3929ac7bccd3fa1bf9b38ddedb77973b1b9a8cfe", + }, + }, + 62: { + { + ID: 8, + Index: 2, + RepoID: 62, + State: structs.CommitStatusError, + TargetURL: "/user2/test_workflows/actions", + Description: "My awesome deploy service - v2", + Context: "deploy/awesomeness", + SHA: "774f93df12d14931ea93259ae93418da4482fcc1", + ContextHash: "ae9547713a6665fc4261d0756904932085a41cf2", + CreatorID: 2, + }, + }, + }, pairs) + }) + t.Run("Repo 62", func(t *testing.T) { + pairs, err := git_model.GetLatestCommitStatusForPairs(db.DefaultContext, []git_model.RepoSHA{{62, "774f93df12d14931ea93259ae93418da4482fcc1"}}) + require.NoError(t, err) + + assert.EqualValues(t, map[int64][]*git_model.CommitStatus{ + 62: { + { + ID: 8, + Index: 2, + RepoID: 62, + State: structs.CommitStatusError, + TargetURL: "/user2/test_workflows/actions", + Description: "My awesome deploy service - v2", + Context: "deploy/awesomeness", + SHA: "774f93df12d14931ea93259ae93418da4482fcc1", + ContextHash: "ae9547713a6665fc4261d0756904932085a41cf2", + CreatorID: 2, + }, + }, + }, pairs) + }) + + t.Run("Repo 62 nonexistant sha", func(t *testing.T) { + pairs, err := git_model.GetLatestCommitStatusForPairs(db.DefaultContext, []git_model.RepoSHA{{62, "774f93df12d14931ea93259ae93418da4482fcc"}}) + require.NoError(t, err) + + assert.EqualValues(t, map[int64][]*git_model.CommitStatus{}, pairs) + }) + + t.Run("SHA with non existant repo id", func(t *testing.T) { + pairs, err := git_model.GetLatestCommitStatusForPairs(db.DefaultContext, []git_model.RepoSHA{{1, "774f93df12d14931ea93259ae93418da4482fcc1"}}) + require.NoError(t, err) + + assert.EqualValues(t, map[int64][]*git_model.CommitStatus{}, pairs) + }) +}