From 1dd83dbb917d55bd253001646d6743f247a4d98b Mon Sep 17 00:00:00 2001 From: Matthew Walowski Date: Mon, 8 May 2023 18:06:05 -0700 Subject: [PATCH] Filters for GetAllCommits (#24568) The `GetAllCommits` endpoint can be pretty slow, especially in repos with a lot of commits. The issue is that it spends a lot of time calculating information that may not be useful/needed by the user. The `stat` param was previously added in #21337 to address this, by allowing the user to disable the calculating stats for each commit. But this has two issues: 1. The name `stat` is rather misleading, because disabling `stat` disables the Stat **and** Files. This should be separated out into two different params, because getting a list of affected files is much less expensive than calculating the stats 2. There's still other costly information provided that the user may not need, such as `Verification` This PR, adds two parameters to the endpoint, `files` and `verification` to allow the user to explicitly disable this information when listing commits. The default behavior is true. --- routers/api/v1/repo/commits.go | 20 ++++++++++++++-- routers/api/v1/repo/notes.go | 2 +- routers/api/v1/repo/pull.go | 2 +- services/convert/git_commit.go | 23 +++++++++++++++---- templates/swagger/v1_json.tmpl | 12 ++++++++++ .../integration/api_repo_git_commits_test.go | 21 +++++++++++++++++ 6 files changed, 72 insertions(+), 8 deletions(-) diff --git a/routers/api/v1/repo/commits.go b/routers/api/v1/repo/commits.go index a5e2e7baef..9b7de91a72 100644 --- a/routers/api/v1/repo/commits.go +++ b/routers/api/v1/repo/commits.go @@ -69,7 +69,7 @@ func getCommit(ctx *context.APIContext, identifier string) { return } - json, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, nil, true) + json, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, nil, convert.ToCommitOptions{Stat: true}) if err != nil { ctx.Error(http.StatusInternalServerError, "toCommit", err) return @@ -107,6 +107,14 @@ func GetAllCommits(ctx *context.APIContext) { // in: query // description: include diff stats for every commit (disable for speedup, default 'true') // type: boolean + // - name: verification + // in: query + // description: include verification for every commit (disable for speedup, default 'true') + // type: boolean + // - name: files + // in: query + // description: include a list of affected files for every commit (disable for speedup, default 'true') + // type: boolean // - name: page // in: query // description: page number of results to return (1-based) @@ -238,10 +246,18 @@ func GetAllCommits(ctx *context.APIContext) { apiCommits := make([]*api.Commit, len(commits)) stat := ctx.FormString("stat") == "" || ctx.FormBool("stat") + verification := ctx.FormString("verification") == "" || ctx.FormBool("verification") + files := ctx.FormString("files") == "" || ctx.FormBool("files") for i, commit := range commits { // Create json struct - apiCommits[i], err = convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, userCache, stat) + apiCommits[i], err = convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, commit, userCache, + convert.ToCommitOptions{ + Stat: stat, + Verification: verification, + Files: files, + }) + if err != nil { ctx.Error(http.StatusInternalServerError, "toCommit", err) return diff --git a/routers/api/v1/repo/notes.go b/routers/api/v1/repo/notes.go index 74969f2cad..1bd66101f0 100644 --- a/routers/api/v1/repo/notes.go +++ b/routers/api/v1/repo/notes.go @@ -78,7 +78,7 @@ func getNote(ctx *context.APIContext, identifier string) { return } - cmt, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, note.Commit, nil, true) + cmt, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, note.Commit, nil, convert.ToCommitOptions{Stat: true}) if err != nil { ctx.Error(http.StatusInternalServerError, "ToCommit", err) return diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index f4e2969d7d..a507c1f44d 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1318,7 +1318,7 @@ func GetPullRequestCommits(ctx *context.APIContext) { apiCommits := make([]*api.Commit, 0, end-start) for i := start; i < end; i++ { - apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, baseGitRepo, commits[i], userCache, true) + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, baseGitRepo, commits[i], userCache, convert.ToCommitOptions{Stat: true}) if err != nil { ctx.ServerError("toCommit", err) return diff --git a/services/convert/git_commit.go b/services/convert/git_commit.go index 20fb8c2565..119237e0ca 100644 --- a/services/convert/git_commit.go +++ b/services/convert/git_commit.go @@ -72,8 +72,14 @@ func ToPayloadCommit(ctx context.Context, repo *repo_model.Repository, c *git.Co } } +type ToCommitOptions struct { + Stat bool + Verification bool + Files bool +} + // ToCommit convert a git.Commit to api.Commit -func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, commit *git.Commit, userCache map[string]*user_model.User, stat bool) (*api.Commit, error) { +func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, commit *git.Commit, userCache map[string]*user_model.User, opts ToCommitOptions) (*api.Commit, error) { var apiAuthor, apiCommitter *api.User // Retrieve author and committer information @@ -162,19 +168,24 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep SHA: commit.ID.String(), Created: commit.Committer.When, }, - Verification: ToVerification(ctx, commit), }, Author: apiAuthor, Committer: apiCommitter, Parents: apiParents, } + // Retrieve verification for commit + if opts.Verification { + res.RepoCommit.Verification = ToVerification(ctx, commit) + } + // Retrieve files affected by the commit - if stat { + if opts.Files { fileStatus, err := git.GetCommitFileStatus(gitRepo.Ctx, repo.RepoPath(), commit.ID.String()) if err != nil { return nil, err } + affectedFileList := make([]*api.CommitAffectedFiles, 0, len(fileStatus.Added)+len(fileStatus.Removed)+len(fileStatus.Modified)) for _, files := range [][]string{fileStatus.Added, fileStatus.Removed, fileStatus.Modified} { for _, filename := range files { @@ -184,6 +195,11 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep } } + res.Files = affectedFileList + } + + // Get diff stats for commit + if opts.Stat { diff, err := gitdiff.GetDiff(gitRepo, &gitdiff.DiffOptions{ AfterCommitID: commit.ID.String(), }) @@ -191,7 +207,6 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep return nil, err } - res.Files = affectedFileList res.Stats = &api.CommitStats{ Total: diff.TotalAddition + diff.TotalDeletion, Additions: diff.TotalAddition, diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 7923e93ca4..99c49fec9c 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -3794,6 +3794,18 @@ "name": "stat", "in": "query" }, + { + "type": "boolean", + "description": "include verification for every commit (disable for speedup, default 'true')", + "name": "verification", + "in": "query" + }, + { + "type": "boolean", + "description": "include a list of affected files for every commit (disable for speedup, default 'true')", + "name": "files", + "in": "query" + }, { "type": "integer", "description": "page number of results to return (1-based)", diff --git a/tests/integration/api_repo_git_commits_test.go b/tests/integration/api_repo_git_commits_test.go index d9c3d7b952..90048a5496 100644 --- a/tests/integration/api_repo_git_commits_test.go +++ b/tests/integration/api_repo_git_commits_test.go @@ -135,6 +135,27 @@ func TestAPIReposGitCommitListDifferentBranch(t *testing.T) { compareCommitFiles(t, []string{"readme.md"}, apiData[0].Files) } +func TestAPIReposGitCommitListWithoutSelectFields(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session) + + // Test getting commits without files, verification, and stats + req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo16/commits?token="+token+"&sha=good-sign&stat=false&files=false&verification=false", user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var apiData []api.Commit + DecodeJSON(t, resp, &apiData) + + assert.Len(t, apiData, 1) + assert.Equal(t, "f27c2b2b03dcab38beaf89b0ab4ff61f6de63441", apiData[0].CommitMeta.SHA) + assert.Equal(t, (*api.CommitStats)(nil), apiData[0].Stats) + assert.Equal(t, (*api.PayloadCommitVerification)(nil), apiData[0].RepoCommit.Verification) + assert.Equal(t, ([]*api.CommitAffectedFiles)(nil), apiData[0].Files) +} + func TestDownloadCommitDiffOrPatch(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})