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 (39eb82446c/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 <admin@oldschoolhack.me>

* Use FindAndCount

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: 6543 <6543@obermui.de>
This commit is contained in:
qwerty287 2021-12-15 06:39:34 +01:00 committed by GitHub
parent 790e6cfeec
commit 9d943bf374
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
24 changed files with 73 additions and 36 deletions

View File

@ -231,11 +231,11 @@ type CommitStatusIndex struct {
} }
// GetLatestCommitStatus returns all statuses with a unique context for a given commit. // 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) 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) ids := make([]int64, 0, 10)
sess := e.Table(&CommitStatus{}). sess := e.Table(&CommitStatus{}).
Where("repo_id = ?", repoID).And("sha = ?", sha). 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) sess = db.SetSessionPagination(sess, &listOptions)
err := sess.Find(&ids) count, err := sess.FindAndCount(&ids)
if err != nil { if err != nil {
return nil, err return nil, count, err
} }
statuses := make([]*CommitStatus, 0, len(ids)) statuses := make([]*CommitStatus, 0, len(ids))
if len(ids) == 0 { 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 // FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts
@ -340,7 +340,7 @@ func ParseCommitsWithStatus(oldCommits []*asymkey_model.SignCommit, repo *repo_m
commit := &SignCommitWithStatuses{ commit := &SignCommitWithStatuses{
SignCommit: c, 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 { if err != nil {
log.Error("GetLatestCommitStatus: %v", err) log.Error("GetLatestCommitStatus: %v", err)
} else { } else {

View File

@ -238,7 +238,7 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) {
return nil return nil
} }
e := db.GetEngine(ctx) e := db.GetEngine(ctx)
reactions, err := findReactions(e, FindReactionsOptions{ reactions, _, err := findReactions(e, FindReactionsOptions{
IssueID: issue.ID, IssueID: issue.ID,
}) })
if err != nil { if err != nil {

View File

@ -593,7 +593,7 @@ func (c *Comment) loadReactions(e db.Engine, repo *repo_model.Repository) (err e
if c.Reactions != nil { if c.Reactions != nil {
return nil return nil
} }
c.Reactions, err = findReactions(e, FindReactionsOptions{ c.Reactions, _, err = findReactions(e, FindReactionsOptions{
IssueID: c.IssueID, IssueID: c.IssueID,
CommentID: c.ID, CommentID: c.ID,
}) })

View File

@ -71,7 +71,7 @@ func (opts *FindReactionsOptions) toConds() builder.Cond {
} }
// FindCommentReactions returns a ReactionList of all reactions from an comment // 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{ return findReactions(db.GetEngine(db.DefaultContext), FindReactionsOptions{
IssueID: comment.IssueID, IssueID: comment.IssueID,
CommentID: comment.ID, CommentID: comment.ID,
@ -79,7 +79,7 @@ func FindCommentReactions(comment *Comment) (ReactionList, error) {
} }
// FindIssueReactions returns a ReactionList of all reactions from an issue // 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{ return findReactions(db.GetEngine(db.DefaultContext), FindReactionsOptions{
ListOptions: listOptions, ListOptions: listOptions,
IssueID: issue.ID, IssueID: issue.ID,
@ -87,20 +87,22 @@ func FindIssueReactions(issue *Issue, listOptions db.ListOptions) (ReactionList,
}) })
} }
func findReactions(e db.Engine, opts FindReactionsOptions) ([]*Reaction, error) { func findReactions(e db.Engine, opts FindReactionsOptions) ([]*Reaction, int64, error) {
e = e. sess := e.
Where(opts.toConds()). Where(opts.toConds()).
In("reaction.`type`", setting.UI.Reactions). In("reaction.`type`", setting.UI.Reactions).
Asc("reaction.issue_id", "reaction.comment_id", "reaction.created_unix", "reaction.id") Asc("reaction.issue_id", "reaction.comment_id", "reaction.created_unix", "reaction.id")
if opts.Page != 0 { if opts.Page != 0 {
e = db.SetEnginePagination(e, &opts) sess = db.SetSessionPagination(sess, &opts)
reactions := make([]*Reaction, 0, opts.PageSize) 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) 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) { 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 findOpts.CommentID = opts.Comment.ID
} }
existingR, err := findReactions(e, findOpts) existingR, _, err := findReactions(e, findOpts)
if err != nil { if err != nil {
return nil, err return nil, err
} }

View File

@ -126,6 +126,20 @@ func getIssueWatchers(e db.Engine, issueID int64, listOptions db.ListOptions) (I
return watches, sess.Find(&watches) 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 { func removeIssueWatchersByRepoID(e db.Engine, userID, repoID int64) error {
_, err := e. _, err := e.
Join("INNER", "issue", "`issue`.id = `issue_watch`.issue_id AND `issue`.repo_id = ?", repoID). Join("INNER", "issue", "`issue`.id = `issue_watch`.issue_id AND `issue`.repo_id = ?", repoID).

View File

@ -120,7 +120,7 @@ func (graph *Graph) LoadAndProcessCommits(repository *repo_model.Repository, git
return models.IsUserRepoAdmin(repository, user) return models.IsUserRepoAdmin(repository, user)
}, &keyMap) }, &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 { if err != nil {
log.Error("GetLatestCommitStatus: %v", err) log.Error("GetLatestCommitStatus: %v", err)
} else { } else {

View File

@ -8,9 +8,10 @@ import (
"path/filepath" "path/filepath"
"testing" "testing"
_ "code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
_ "code.gitea.io/gitea/models"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )

View File

@ -11,11 +11,12 @@ import (
"testing" "testing"
"time" "time"
_ "code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/util"
_ "code.gitea.io/gitea/models"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"gopkg.in/ini.v1" "gopkg.in/ini.v1"
) )

View File

@ -9,11 +9,12 @@ import (
"testing" "testing"
"time" "time"
_ "code.gitea.io/gitea/models"
repo_model "code.gitea.io/gitea/models/repo" repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
_ "code.gitea.io/gitea/models"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"gopkg.in/ini.v1" "gopkg.in/ini.v1"
) )

View File

@ -509,6 +509,7 @@ func GetTeamRepos(ctx *context.APIContext) {
} }
repos[i] = convert.ToRepo(repo, access) repos[i] = convert.ToRepo(repo, access)
} }
ctx.SetTotalCountHeader(int64(team.NumRepos))
ctx.JSON(http.StatusOK, repos) ctx.JSON(http.StatusOK, repos)
} }

View File

@ -67,9 +67,9 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
return return
} }
reactions, err := models.FindCommentReactions(comment) reactions, _, err := models.FindCommentReactions(comment)
if err != nil { if err != nil {
ctx.Error(http.StatusInternalServerError, "FindIssueReactions", err) ctx.Error(http.StatusInternalServerError, "FindCommentReactions", err)
return return
} }
_, err = reactions.LoadUsers(ctx.Repo.Repository) _, err = reactions.LoadUsers(ctx.Repo.Repository)
@ -285,7 +285,7 @@ func GetIssueReactions(ctx *context.APIContext) {
return return
} }
reactions, err := models.FindIssueReactions(issue, utils.GetListOptions(ctx)) reactions, count, err := models.FindIssueReactions(issue, utils.GetListOptions(ctx))
if err != nil { if err != nil {
ctx.Error(http.StatusInternalServerError, "FindIssueReactions", err) ctx.Error(http.StatusInternalServerError, "FindIssueReactions", err)
return return
@ -305,6 +305,7 @@ func GetIssueReactions(ctx *context.APIContext) {
}) })
} }
ctx.SetTotalCountHeader(count)
ctx.JSON(http.StatusOK, result) ctx.JSON(http.StatusOK, result)
} }

View File

@ -279,9 +279,16 @@ func GetIssueSubscribers(ctx *context.APIContext) {
return return
} }
apiUsers := make([]*api.User, 0, len(users)) apiUsers := make([]*api.User, 0, len(users))
for i := range users { for _, v := range users {
apiUsers[i] = convert.ToUser(users[i], ctx.User) 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) ctx.JSON(http.StatusOK, apiUsers)
} }

View File

@ -253,7 +253,7 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
repo := ctx.Repo.Repository 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 { if err != nil {
ctx.Error(http.StatusInternalServerError, "GetLatestCommitStatus", fmt.Errorf("GetLatestCommitStatus[%s, %s]: %v", repo.FullName(), sha, err)) ctx.Error(http.StatusInternalServerError, "GetLatestCommitStatus", fmt.Errorf("GetLatestCommitStatus[%s, %s]: %v", repo.FullName(), sha, err))
return return
@ -266,6 +266,6 @@ func GetCombinedCommitStatusByRef(ctx *context.APIContext) {
combiStatus := convert.ToCombinedStatus(statuses, convert.ToRepo(repo, ctx.Repo.AccessMode)) combiStatus := convert.ToCombinedStatus(statuses, convert.ToRepo(repo, ctx.Repo.AccessMode))
// TODO: ctx.SetTotalCountHeader(count) ctx.SetTotalCountHeader(count)
ctx.JSON(http.StatusOK, combiStatus) ctx.JSON(http.StatusOK, combiStatus)
} }

View File

@ -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 { 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()) ctx.Error(http.StatusBadRequest, "", err.Error())
} else { } else {
ctx.SetTotalCountHeader(int64(tree.TotalCount))
ctx.JSON(http.StatusOK, tree) ctx.JSON(http.StatusOK, tree)
} }
} }

View File

@ -323,6 +323,7 @@ func ListWikiPages(ctx *context.APIContext) {
pages = append(pages, convert.ToWikiPageMetaData(wikiName, c, ctx.Repo.Repository)) pages = append(pages, convert.ToWikiPageMetaData(wikiName, c, ctx.Repo.Repository))
} }
ctx.SetTotalCountHeader(int64(len(entries)))
ctx.JSON(http.StatusOK, pages) ctx.JSON(http.StatusOK, pages)
} }
@ -432,6 +433,7 @@ func ListPageRevisions(ctx *context.APIContext) {
return return
} }
ctx.SetTotalCountHeader(commitsCount)
ctx.JSON(http.StatusOK, convert.ToWikiCommitList(commitsHistory, commitsCount)) ctx.JSON(http.StatusOK, convert.ToWikiCommitList(commitsHistory, commitsCount))
} }

View File

@ -67,7 +67,10 @@ func GetStarredRepos(ctx *context.APIContext) {
repos, err := getStarredRepos(user, private, utils.GetListOptions(ctx)) repos, err := getStarredRepos(user, private, utils.GetListOptions(ctx))
if err != nil { if err != nil {
ctx.Error(http.StatusInternalServerError, "getStarredRepos", err) ctx.Error(http.StatusInternalServerError, "getStarredRepos", err)
return
} }
ctx.SetTotalCountHeader(int64(user.NumStars))
ctx.JSON(http.StatusOK, &repos) ctx.JSON(http.StatusOK, &repos)
} }

View File

@ -337,7 +337,7 @@ func Diff(ctx *context.Context) {
ctx.Data["Commit"] = commit ctx.Data["Commit"] = commit
ctx.Data["Diff"] = diff 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 { if err != nil {
log.Error("GetLatestCommitStatus: %v", err) log.Error("GetLatestCommitStatus: %v", err)
} }

View File

@ -339,7 +339,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C
if len(compareInfo.Commits) != 0 { if len(compareInfo.Commits) != 0 {
sha := compareInfo.Commits[0].ID.String() 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 { if err != nil {
ctx.ServerError("GetLatestCommitStatus", err) ctx.ServerError("GetLatestCommitStatus", err)
return nil 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) ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
return nil return nil
} }
commitStatuses, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) commitStatuses, _, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{})
if err != nil { if err != nil {
ctx.ServerError("GetLatestCommitStatus", err) ctx.ServerError("GetLatestCommitStatus", err)
return nil return nil
@ -482,7 +482,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
return nil return nil
} }
commitStatuses, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{}) commitStatuses, _, err := models.GetLatestCommitStatus(repo.ID, sha, db.ListOptions{})
if err != nil { if err != nil {
ctx.ServerError("GetLatestCommitStatus", err) ctx.ServerError("GetLatestCommitStatus", err)
return nil return nil

View File

@ -790,7 +790,7 @@ func renderDirectoryFiles(ctx *context.Context, timeout time.Duration) git.Entri
ctx.Data["LatestCommitUser"] = user_model.ValidateCommitWithEmail(latestCommit) 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 { if err != nil {
log.Error("GetLatestCommitStatus: %v", err) log.Error("GetLatestCommitStatus: %v", err)
} }

View File

@ -11,6 +11,7 @@ import (
"code.gitea.io/gitea/models/login" "code.gitea.io/gitea/models/login"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )

View File

@ -9,6 +9,7 @@ import (
"time" "time"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )

View File

@ -130,7 +130,7 @@ func GetPullRequestCommitStatusState(pr *models.PullRequest) (structs.CommitStat
return "", errors.Wrap(err, "LoadBaseRepo") 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 { if err != nil {
return "", errors.Wrap(err, "GetLatestCommitStatus") return "", errors.Wrap(err, "GetLatestCommitStatus")
} }

View File

@ -782,7 +782,7 @@ func getLastCommitStatus(gitRepo *git.Repository, pr *models.PullRequest) (statu
return nil, err 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 { if err != nil {
return nil, err return nil, err
} }

View File

@ -8,8 +8,9 @@ import (
"path/filepath" "path/filepath"
"testing" "testing"
_ "code.gitea.io/gitea/models"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
_ "code.gitea.io/gitea/models"
) )
func TestMain(m *testing.M) { func TestMain(m *testing.M) {