From 9d943bf374e56e4d403303a6a2caafc1c79cdb6f Mon Sep 17 00:00:00 2001 From: qwerty287 <80460567+qwerty287@users.noreply.github.com> Date: Wed, 15 Dec 2021 06:39:34 +0100 Subject: [PATCH] Add missing `X-Total-Count` and fix some related bugs (#17968) * Add missing `X-Total-Count` and fix some related bugs Adds `X-Total-Count` header to APIs that return a list but doesn't have it yet. Fixed bugs: * not returned after reporting error (https://github.com/qwerty287/gitea/blob/39eb82446c6fe5da3d79124e1f701f3795625b69/routers/api/v1/user/star.go#L70) * crash with index out of bounds, API issue/issueSubscriptions I also found various endpoints that return lists but do not apply/support pagination yet: ``` /repos/{owner}/{repo}/issues/{index}/labels /repos/{owner}/{repo}/issues/comments/{id}/reactions /repos/{owner}/{repo}/branch_protections /repos/{owner}/{repo}/contents /repos/{owner}/{repo}/hooks/git /repos/{owner}/{repo}/issue_templates /repos/{owner}/{repo}/releases/{id}/assets /repos/{owner}/{repo}/reviewers /repos/{owner}/{repo}/teams /user/emails /users/{username}/heatmap ``` If this is not expected, an new issue should be opened. Closes #13043 * fmt * Update routers/api/v1/repo/issue_subscription.go Co-authored-by: KN4CK3R * Use FindAndCount Co-authored-by: KN4CK3R Co-authored-by: 6543 <6543@obermui.de> --- models/commit_status.go | 14 +++++++------- models/issue.go | 2 +- models/issue_comment.go | 2 +- models/issue_reaction.go | 18 ++++++++++-------- models/issue_watch.go | 14 ++++++++++++++ modules/gitgraph/graph_models.go | 2 +- modules/indexer/code/indexer_test.go | 3 ++- modules/indexer/issues/indexer_test.go | 3 ++- modules/indexer/stats/indexer_test.go | 3 ++- routers/api/v1/org/team.go | 1 + routers/api/v1/repo/issue_reaction.go | 7 ++++--- routers/api/v1/repo/issue_subscription.go | 11 +++++++++-- routers/api/v1/repo/status.go | 4 ++-- routers/api/v1/repo/tree.go | 1 + routers/api/v1/repo/wiki.go | 2 ++ routers/api/v1/user/star.go | 3 +++ routers/web/repo/commit.go | 2 +- routers/web/repo/pull.go | 6 +++--- routers/web/repo/view.go | 2 +- services/asymkey/ssh_key_test.go | 1 + services/mailer/mailer_test.go | 1 + services/pull/commit_status.go | 2 +- services/pull/pull.go | 2 +- services/webhook/main_test.go | 3 ++- 24 files changed, 73 insertions(+), 36 deletions(-) diff --git a/models/commit_status.go b/models/commit_status.go index 93b6f93f96..cd7497eed8 100644 --- a/models/commit_status.go +++ b/models/commit_status.go @@ -231,11 +231,11 @@ type CommitStatusIndex struct { } // GetLatestCommitStatus returns all statuses with a unique context for a given commit. -func GetLatestCommitStatus(repoID int64, sha string, listOptions db.ListOptions) ([]*CommitStatus, error) { +func GetLatestCommitStatus(repoID int64, sha string, listOptions db.ListOptions) ([]*CommitStatus, int64, error) { return getLatestCommitStatus(db.GetEngine(db.DefaultContext), repoID, sha, listOptions) } -func getLatestCommitStatus(e db.Engine, repoID int64, sha string, listOptions db.ListOptions) ([]*CommitStatus, error) { +func getLatestCommitStatus(e db.Engine, repoID int64, sha string, listOptions db.ListOptions) ([]*CommitStatus, int64, error) { ids := make([]int64, 0, 10) sess := e.Table(&CommitStatus{}). Where("repo_id = ?", repoID).And("sha = ?", sha). @@ -244,15 +244,15 @@ func getLatestCommitStatus(e db.Engine, repoID int64, sha string, listOptions db sess = db.SetSessionPagination(sess, &listOptions) - err := sess.Find(&ids) + count, err := sess.FindAndCount(&ids) if err != nil { - return nil, err + return nil, count, err } statuses := make([]*CommitStatus, 0, len(ids)) if len(ids) == 0 { - return statuses, nil + return statuses, count, nil } - return statuses, e.In("id", ids).Find(&statuses) + return statuses, count, e.In("id", ids).Find(&statuses) } // FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts @@ -340,7 +340,7 @@ func ParseCommitsWithStatus(oldCommits []*asymkey_model.SignCommit, repo *repo_m commit := &SignCommitWithStatuses{ SignCommit: c, } - statuses, err := GetLatestCommitStatus(repo.ID, commit.ID.String(), db.ListOptions{}) + statuses, _, err := GetLatestCommitStatus(repo.ID, commit.ID.String(), db.ListOptions{}) if err != nil { log.Error("GetLatestCommitStatus: %v", err) } else { diff --git a/models/issue.go b/models/issue.go index d2f06846c2..ea8eb4a9aa 100644 --- a/models/issue.go +++ b/models/issue.go @@ -238,7 +238,7 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) { return nil } e := db.GetEngine(ctx) - reactions, err := findReactions(e, FindReactionsOptions{ + reactions, _, err := findReactions(e, FindReactionsOptions{ IssueID: issue.ID, }) if err != nil { diff --git a/models/issue_comment.go b/models/issue_comment.go index 754fa3439e..e54d41bf0d 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -593,7 +593,7 @@ func (c *Comment) loadReactions(e db.Engine, repo *repo_model.Repository) (err e if c.Reactions != nil { return nil } - c.Reactions, err = findReactions(e, FindReactionsOptions{ + c.Reactions, _, err = findReactions(e, FindReactionsOptions{ IssueID: c.IssueID, CommentID: c.ID, }) diff --git a/models/issue_reaction.go b/models/issue_reaction.go index 4072733a1c..2e0ab07db7 100644 --- a/models/issue_reaction.go +++ b/models/issue_reaction.go @@ -71,7 +71,7 @@ func (opts *FindReactionsOptions) toConds() builder.Cond { } // FindCommentReactions returns a ReactionList of all reactions from an comment -func FindCommentReactions(comment *Comment) (ReactionList, error) { +func FindCommentReactions(comment *Comment) (ReactionList, int64, error) { return findReactions(db.GetEngine(db.DefaultContext), FindReactionsOptions{ IssueID: comment.IssueID, CommentID: comment.ID, @@ -79,7 +79,7 @@ func FindCommentReactions(comment *Comment) (ReactionList, error) { } // FindIssueReactions returns a ReactionList of all reactions from an issue -func FindIssueReactions(issue *Issue, listOptions db.ListOptions) (ReactionList, error) { +func FindIssueReactions(issue *Issue, listOptions db.ListOptions) (ReactionList, int64, error) { return findReactions(db.GetEngine(db.DefaultContext), FindReactionsOptions{ ListOptions: listOptions, IssueID: issue.ID, @@ -87,20 +87,22 @@ func FindIssueReactions(issue *Issue, listOptions db.ListOptions) (ReactionList, }) } -func findReactions(e db.Engine, opts FindReactionsOptions) ([]*Reaction, error) { - e = e. +func findReactions(e db.Engine, opts FindReactionsOptions) ([]*Reaction, int64, error) { + sess := e. Where(opts.toConds()). In("reaction.`type`", setting.UI.Reactions). Asc("reaction.issue_id", "reaction.comment_id", "reaction.created_unix", "reaction.id") if opts.Page != 0 { - e = db.SetEnginePagination(e, &opts) + sess = db.SetSessionPagination(sess, &opts) reactions := make([]*Reaction, 0, opts.PageSize) - return reactions, e.Find(&reactions) + count, err := sess.FindAndCount(&reactions) + return reactions, count, err } reactions := make([]*Reaction, 0, 10) - return reactions, e.Find(&reactions) + count, err := sess.FindAndCount(&reactions) + return reactions, count, err } func createReaction(e db.Engine, opts *ReactionOptions) (*Reaction, error) { @@ -120,7 +122,7 @@ func createReaction(e db.Engine, opts *ReactionOptions) (*Reaction, error) { findOpts.CommentID = opts.Comment.ID } - existingR, err := findReactions(e, findOpts) + existingR, _, err := findReactions(e, findOpts) if err != nil { return nil, err } diff --git a/models/issue_watch.go b/models/issue_watch.go index 181cd23433..92dc847415 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -126,6 +126,20 @@ func getIssueWatchers(e db.Engine, issueID int64, listOptions db.ListOptions) (I return watches, sess.Find(&watches) } +// CountIssueWatchers count watchers/unwatchers of a given issue +func CountIssueWatchers(issueID int64) (int64, error) { + return countIssueWatchers(db.GetEngine(db.DefaultContext), issueID) +} + +func countIssueWatchers(e db.Engine, issueID int64) (int64, error) { + return e. + Where("`issue_watch`.issue_id = ?", issueID). + And("`issue_watch`.is_watching = ?", true). + And("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). + Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id").Count(new(IssueWatch)) +} + func removeIssueWatchersByRepoID(e db.Engine, userID, repoID int64) error { _, err := e. Join("INNER", "issue", "`issue`.id = `issue_watch`.issue_id AND `issue`.repo_id = ?", repoID). diff --git a/modules/gitgraph/graph_models.go b/modules/gitgraph/graph_models.go index 4cfa96816a..44773a3b9a 100644 --- a/modules/gitgraph/graph_models.go +++ b/modules/gitgraph/graph_models.go @@ -120,7 +120,7 @@ func (graph *Graph) LoadAndProcessCommits(repository *repo_model.Repository, git return models.IsUserRepoAdmin(repository, user) }, &keyMap) - statuses, err := models.GetLatestCommitStatus(repository.ID, c.Commit.ID.String(), db.ListOptions{}) + statuses, _, err := models.GetLatestCommitStatus(repository.ID, c.Commit.ID.String(), db.ListOptions{}) if err != nil { log.Error("GetLatestCommitStatus: %v", err) } else { diff --git a/modules/indexer/code/indexer_test.go b/modules/indexer/code/indexer_test.go index 98494afceb..42f21ed0df 100644 --- a/modules/indexer/code/indexer_test.go +++ b/modules/indexer/code/indexer_test.go @@ -8,9 +8,10 @@ import ( "path/filepath" "testing" - _ "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/unittest" + _ "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" ) diff --git a/modules/indexer/issues/indexer_test.go b/modules/indexer/issues/indexer_test.go index 0855165556..866e3cc7c9 100644 --- a/modules/indexer/issues/indexer_test.go +++ b/modules/indexer/issues/indexer_test.go @@ -11,11 +11,12 @@ import ( "testing" "time" - _ "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + _ "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" "gopkg.in/ini.v1" ) diff --git a/modules/indexer/stats/indexer_test.go b/modules/indexer/stats/indexer_test.go index b32100b458..50c6cc38e9 100644 --- a/modules/indexer/stats/indexer_test.go +++ b/modules/indexer/stats/indexer_test.go @@ -9,11 +9,12 @@ import ( "testing" "time" - _ "code.gitea.io/gitea/models" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/setting" + _ "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" "gopkg.in/ini.v1" ) diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index 074d6fd009..d39125b050 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -509,6 +509,7 @@ func GetTeamRepos(ctx *context.APIContext) { } repos[i] = convert.ToRepo(repo, access) } + ctx.SetTotalCountHeader(int64(team.NumRepos)) ctx.JSON(http.StatusOK, repos) } diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index d0ba8dac65..6184ea71f0 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/routers/api/v1/repo/issue_reaction.go @@ -67,9 +67,9 @@ func GetIssueCommentReactions(ctx *context.APIContext) { return } - reactions, err := models.FindCommentReactions(comment) + reactions, _, err := models.FindCommentReactions(comment) if err != nil { - ctx.Error(http.StatusInternalServerError, "FindIssueReactions", err) + ctx.Error(http.StatusInternalServerError, "FindCommentReactions", err) return } _, err = reactions.LoadUsers(ctx.Repo.Repository) @@ -285,7 +285,7 @@ func GetIssueReactions(ctx *context.APIContext) { return } - reactions, err := models.FindIssueReactions(issue, utils.GetListOptions(ctx)) + reactions, count, err := models.FindIssueReactions(issue, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "FindIssueReactions", err) return @@ -305,6 +305,7 @@ func GetIssueReactions(ctx *context.APIContext) { }) } + ctx.SetTotalCountHeader(count) ctx.JSON(http.StatusOK, result) } diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index ae7661cfab..ff305c4877 100644 --- a/routers/api/v1/repo/issue_subscription.go +++ b/routers/api/v1/repo/issue_subscription.go @@ -279,9 +279,16 @@ func GetIssueSubscribers(ctx *context.APIContext) { return } apiUsers := make([]*api.User, 0, len(users)) - for i := range users { - apiUsers[i] = convert.ToUser(users[i], ctx.User) + for _, v := range users { + apiUsers = append(apiUsers, convert.ToUser(v, ctx.User)) } + count, err := models.CountIssueWatchers(issue.ID) + if err != nil { + ctx.Error(http.StatusInternalServerError, "CountIssueWatchers", err) + return + } + + ctx.SetTotalCountHeader(count) ctx.JSON(http.StatusOK, apiUsers) } diff --git a/routers/api/v1/repo/status.go b/routers/api/v1/repo/status.go index 583b89dd7a..9d0fbbddad 100644 --- a/routers/api/v1/repo/status.go +++ b/routers/api/v1/repo/status.go @@ -253,7 +253,7 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) { repo := ctx.Repo.Repository - statuses, err := models.GetLatestCommitStatus(repo.ID, sha, utils.GetListOptions(ctx)) + statuses, count, err := models.GetLatestCommitStatus(repo.ID, sha, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "GetLatestCommitStatus", fmt.Errorf("GetLatestCommitStatus[%s, %s]: %v", repo.FullName(), sha, err)) return @@ -266,6 +266,6 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) { combiStatus := convert.ToCombinedStatus(statuses, convert.ToRepo(repo, ctx.Repo.AccessMode)) - // TODO: ctx.SetTotalCountHeader(count) + ctx.SetTotalCountHeader(count) ctx.JSON(http.StatusOK, combiStatus) } diff --git a/routers/api/v1/repo/tree.go b/routers/api/v1/repo/tree.go index 2168a311d0..411d87d6c5 100644 --- a/routers/api/v1/repo/tree.go +++ b/routers/api/v1/repo/tree.go @@ -63,6 +63,7 @@ func GetTree(ctx *context.APIContext) { if tree, err := files_service.GetTreeBySHA(ctx.Repo.Repository, sha, ctx.FormInt("page"), ctx.FormInt("per_page"), ctx.FormBool("recursive")); err != nil { ctx.Error(http.StatusBadRequest, "", err.Error()) } else { + ctx.SetTotalCountHeader(int64(tree.TotalCount)) ctx.JSON(http.StatusOK, tree) } } diff --git a/routers/api/v1/repo/wiki.go b/routers/api/v1/repo/wiki.go index f8969067be..bef32f367b 100644 --- a/routers/api/v1/repo/wiki.go +++ b/routers/api/v1/repo/wiki.go @@ -323,6 +323,7 @@ func ListWikiPages(ctx *context.APIContext) { pages = append(pages, convert.ToWikiPageMetaData(wikiName, c, ctx.Repo.Repository)) } + ctx.SetTotalCountHeader(int64(len(entries))) ctx.JSON(http.StatusOK, pages) } @@ -432,6 +433,7 @@ func ListPageRevisions(ctx *context.APIContext) { return } + ctx.SetTotalCountHeader(commitsCount) ctx.JSON(http.StatusOK, convert.ToWikiCommitList(commitsHistory, commitsCount)) } diff --git a/routers/api/v1/user/star.go b/routers/api/v1/user/star.go index 14bc23b1c3..cc527d9213 100644 --- a/routers/api/v1/user/star.go +++ b/routers/api/v1/user/star.go @@ -67,7 +67,10 @@ func GetStarredRepos(ctx *context.APIContext) { repos, err := getStarredRepos(user, private, utils.GetListOptions(ctx)) if err != nil { ctx.Error(http.StatusInternalServerError, "getStarredRepos", err) + return } + + ctx.SetTotalCountHeader(int64(user.NumStars)) ctx.JSON(http.StatusOK, &repos) } diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 0a51e43cf7..e012bc24d2 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -337,7 +337,7 @@ func Diff(ctx *context.Context) { ctx.Data["Commit"] = commit ctx.Data["Diff"] = diff - statuses, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, commitID, db.ListOptions{}) + statuses, _, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, commitID, db.ListOptions{}) if err != nil { log.Error("GetLatestCommitStatus: %v", err) } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 2fcee33a23..f94424870a 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -339,7 +339,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C if len(compareInfo.Commits) != 0 { sha := compareInfo.Commits[0].ID.String() - commitStatuses, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, sha, db.ListOptions{}) + commitStatuses, _, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, sha, db.ListOptions{}) if err != nil { ctx.ServerError("GetLatestCommitStatus", err) return nil @@ -393,7 +393,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err) return nil } - commitStatuses, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) + commitStatuses, _, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) if err != nil { ctx.ServerError("GetLatestCommitStatus", err) return nil @@ -482,7 +482,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare return nil } - commitStatuses, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) + commitStatuses, _, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) if err != nil { ctx.ServerError("GetLatestCommitStatus", err) return nil diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index 5e9003b16b..e19fb9bf91 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -790,7 +790,7 @@ func renderDirectoryFiles(ctx *context.Context, timeout time.Duration) git.Entri ctx.Data["LatestCommitUser"] = user_model.ValidateCommitWithEmail(latestCommit) } - statuses, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, ctx.Repo.Commit.ID.String(), db.ListOptions{}) + statuses, _, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, ctx.Repo.Commit.ID.String(), db.ListOptions{}) if err != nil { log.Error("GetLatestCommitStatus: %v", err) } diff --git a/services/asymkey/ssh_key_test.go b/services/asymkey/ssh_key_test.go index 0ce235f7f6..9de6a4c11b 100644 --- a/services/asymkey/ssh_key_test.go +++ b/services/asymkey/ssh_key_test.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models/login" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "github.com/stretchr/testify/assert" ) diff --git a/services/mailer/mailer_test.go b/services/mailer/mailer_test.go index 1739a68a64..56f2eb52b0 100644 --- a/services/mailer/mailer_test.go +++ b/services/mailer/mailer_test.go @@ -9,6 +9,7 @@ import ( "time" "code.gitea.io/gitea/modules/setting" + "github.com/stretchr/testify/assert" ) diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 2b834c25f1..5324b49fa9 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -130,7 +130,7 @@ func GetPullRequestCommitStatusState(pr *models.PullRequest) (structs.CommitStat return "", errors.Wrap(err, "LoadBaseRepo") } - commitStatuses, err := models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) + commitStatuses, _, err := models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) if err != nil { return "", errors.Wrap(err, "GetLatestCommitStatus") } diff --git a/services/pull/pull.go b/services/pull/pull.go index 474c211622..3b127b8f1c 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -782,7 +782,7 @@ func getLastCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (statu return nil, err } - statusList, err := models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) + statusList, _, err := models.GetLatestCommitStatus(pr.BaseRepo.ID, sha, db.ListOptions{}) if err != nil { return nil, err } diff --git a/services/webhook/main_test.go b/services/webhook/main_test.go index e64acf3b61..a87b74e89d 100644 --- a/services/webhook/main_test.go +++ b/services/webhook/main_test.go @@ -8,8 +8,9 @@ import ( "path/filepath" "testing" - _ "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/unittest" + + _ "code.gitea.io/gitea/models" ) func TestMain(m *testing.M) {