Skip to content

Reduce paging inconsistency #34561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions models/issues/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,7 @@ func (issue *Issue) BlockedByDependencies(ctx context.Context, opts db.ListOptio
Where("issue_id = ?", issue.ID).
// sort by repo id then created date, with the issues of the same repo at the beginning of the list
OrderBy("CASE WHEN issue.repo_id = ? THEN 0 ELSE issue.repo_id END, issue.created_unix DESC", issue.RepoID)
// Pagination bypass needed by ViewIssue => prepareIssueViewSidebarDependency
if opts.Page > 0 {
sess = db.SetSessionPagination(sess, &opts)
}
Expand Down
9 changes: 3 additions & 6 deletions models/issues/issue_watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,9 @@ func GetIssueWatchers(ctx context.Context, issueID int64, listOptions db.ListOpt
And("`user`.prohibit_login = ?", false).
Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id")

if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)
watches := make([]*IssueWatch, 0, listOptions.PageSize)
return watches, sess.Find(&watches)
}
watches := make([]*IssueWatch, 0, 8)
listOptions.SetDefaultValues()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new logic will force the pagination but the pagination could be ignored in the old logic.

Copy link
Contributor Author

@ChristopherHX ChristopherHX Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pagination could be ignored in the old logic.

Yes this is exactly one of the inconsistencies, I want to discuss in this PR.

IMO Someone could abuse disabled paging of the rest api controlled by the api client. For reference this function is only used in the Rest Api and TestCode, nowhere else. Where UI code is calling them as well, I only added comments.

Make Gitea do expensive database queries, without constraints.

Other than that SetDefaultValues is not needed here, since SetSessionPagination calls this function.

Suggested change
listOptions.SetDefaultValues()

If this is an expected feature, we should add tests that verifies GET /repos/{owner}/{repo}/issues/{index}/subscriptions can bypass paging by specifing the page as 0 or not including it in the query.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against most of the changes. Maybe we should mark this as break.

sess = db.SetSessionPagination(sess, &listOptions)
watches := make([]*IssueWatch, 0, listOptions.PageSize)
return watches, sess.Find(&watches)
}

Expand Down
2 changes: 2 additions & 0 deletions models/issues/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ func GetLabelsByRepoID(ctx context.Context, repoID int64, sortType string, listO
sess.Asc("name")
}

// Pagination bypass used by some callers
if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)
}
Expand Down Expand Up @@ -483,6 +484,7 @@ func GetLabelsByOrgID(ctx context.Context, orgID int64, sortType string, listOpt
sess.Asc("name")
}

// Why can we bypass limits here?
if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)
}
Expand Down
1 change: 1 addition & 0 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ func GetCurrentReview(ctx context.Context, reviewer *user_model.User, issue *Iss
if reviewer == nil {
return nil, nil
}
// Missing db.ListOptionsAll
reviews, err := FindReviews(ctx, FindReviewOptions{
Types: []ReviewType{ReviewTypePending},
IssueID: issue.ID,
Expand Down
1 change: 1 addition & 0 deletions models/issues/review_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func (opts *FindReviewOptions) toCond() builder.Cond {
func FindReviews(ctx context.Context, opts FindReviewOptions) (ReviewList, error) {
reviews := make([]*Review, 0, 10)
sess := db.GetEngine(ctx).Where(opts.toCond())
// Pagination bypass used by some callers
if opts.Page > 0 && !opts.IsListAll() {
sess = db.SetSessionPagination(sess, &opts)
}
Expand Down
1 change: 1 addition & 0 deletions models/issues/stopwatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ func GetUIDsAndStopwatch(ctx context.Context) ([]*UserStopwatch, error) {
func GetUserStopwatches(ctx context.Context, userID int64, listOptions db.ListOptions) ([]*Stopwatch, error) {
sws := make([]*Stopwatch, 0, 8)
sess := db.GetEngine(ctx).Where("stopwatch.user_id = ?", userID)
// Pagination bypass used by CancelStopwatch
if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)
}
Expand Down
1 change: 1 addition & 0 deletions models/packages/package_version.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ type PackageSearchOptions struct {
HasFileWithName string // only results are found which are associated with a file with the specific name
HasFiles optional.Option[bool] // only results are found which have associated files
Sort VersionSort
// Only one with interface
db.Paginator
}

Expand Down
1 change: 1 addition & 0 deletions models/repo/star.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func IsStaring(ctx context.Context, userID, repoID int64) bool {
func GetStargazers(ctx context.Context, repo *Repository, opts db.ListOptions) ([]*user_model.User, error) {
sess := db.GetEngine(ctx).Where("star.repo_id = ?", repo.ID).
Join("LEFT", "star", "`user`.id = star.uid")
// Paging bypass used by UI
if opts.Page > 0 {
sess = db.SetSessionPagination(sess, &opts)

Expand Down
9 changes: 2 additions & 7 deletions models/repo/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,9 @@ func GetRepoWatchers(ctx context.Context, repoID int64, opts db.ListOptions) ([]
sess := db.GetEngine(ctx).Where("watch.repo_id=?", repoID).
Join("LEFT", "watch", "`user`.id=`watch`.user_id").
And("`watch`.mode<>?", WatchModeDont)
if opts.Page > 0 {
sess = db.SetSessionPagination(sess, &opts)
users := make([]*user_model.User, 0, opts.PageSize)
sess = db.SetSessionPagination(sess, &opts)
users := make([]*user_model.User, 0, opts.PageSize)

return users, sess.Find(&users)
}

users := make([]*user_model.User, 0, 8)
return users, sess.Find(&users)
}

Expand Down
1 change: 1 addition & 0 deletions models/user/search.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ func SearchUsers(ctx context.Context, opts SearchUserOptions) (users []*User, _

sessQuery := opts.toSearchQueryBase(ctx).OrderBy(opts.OrderBy.String())
defer sessQuery.Close()
// Pagination bypass used by UI
if opts.Page > 0 {
sessQuery = db.SetSessionPagination(sessQuery, &opts)
}
Expand Down
11 changes: 3 additions & 8 deletions models/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,15 +321,9 @@ func GetUserFollowers(ctx context.Context, u, viewer *User, listOptions db.ListO
And("`user`.type=?", UserTypeIndividual).
And(isUserVisibleToViewerCond(viewer))

if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)

users := make([]*User, 0, listOptions.PageSize)
count, err := sess.FindAndCount(&users)
return users, count, err
}
sess = db.SetSessionPagination(sess, &listOptions)

users := make([]*User, 0, 8)
users := make([]*User, 0, listOptions.PageSize)
count, err := sess.FindAndCount(&users)
return users, count, err
}
Expand All @@ -343,6 +337,7 @@ func GetUserFollowing(ctx context.Context, u, viewer *User, listOptions db.ListO
And("`user`.type IN (?, ?)", UserTypeIndividual, UserTypeOrganization).
And(isUserVisibleToViewerCond(viewer))

// pagination bypass, otherwise 8
if listOptions.Page > 0 {
sess = db.SetSessionPagination(sess, &listOptions)

Expand Down
1 change: 1 addition & 0 deletions modules/git/repo_tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) {

sortTagsByTime(tags)
tagsTotal := len(tags)
// pagination bypass
if page != 0 {
tags = util.PaginateSlice(tags, page, pageSize).([]*Tag)
}
Expand Down
4 changes: 1 addition & 3 deletions routers/api/v1/admin/adopt.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ func ListUnadoptedRepositories(ctx *context.APIContext) {
// "$ref": "#/responses/forbidden"

listOptions := utils.GetListOptions(ctx)
if listOptions.Page == 0 {
listOptions.Page = 1
}
listOptions.SetDefaultValues()
repoNames, count, err := repo_service.ListUnadoptedRepositories(ctx, ctx.FormString("query"), &listOptions)
if err != nil {
ctx.APIErrorInternal(err)
Expand Down
1 change: 1 addition & 0 deletions routers/api/v1/admin/cron.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func ListCronTasks(ctx *context.APIContext) {
count := len(tasks)

listOpts := utils.GetListOptions(ctx)
listOpts.SetDefaultValues()
tasks = util.PaginateSlice(tasks, listOpts.Page, listOpts.PageSize).(cron.TaskTable)

res := make([]structs.Cron, len(tasks))
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/admin/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ func GetAllOrgs(ctx *context.APIContext) {
// "$ref": "#/responses/forbidden"

listOptions := utils.GetListOptions(ctx)
// SearchUsers allows pagination bypass
listOptions.SetDefaultValues()

users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
Actor: ctx.Doer,
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/admin/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,8 @@ func SearchUsers(ctx *context.APIContext) {
// "$ref": "#/responses/forbidden"

listOptions := utils.GetListOptions(ctx)
// SearchUsers allows pagination bypass
listOptions.SetDefaultValues()

users, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
Actor: ctx.Doer,
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/org/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ func GetAll(ctx *context.APIContext) {
}

listOptions := utils.GetListOptions(ctx)
// SearchUsers allows pagination bypass
listOptions.SetDefaultValues()

publicOrgs, maxResults, err := user_model.SearchUsers(ctx, user_model.SearchUserOptions{
Actor: ctx.Doer,
Expand Down
1 change: 1 addition & 0 deletions routers/api/v1/repo/commits.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ func GetAllCommits(ctx *context.APIContext) {
return
}

// Unusual use of ListOptions, but we need to support pagination
listOptions := utils.GetListOptions(ctx)
if listOptions.Page <= 0 {
listOptions.Page = 1
Expand Down
19 changes: 5 additions & 14 deletions routers/api/v1/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
user_model "code.gitea.io/gitea/models/user"
issue_indexer "code.gitea.io/gitea/modules/indexer/issues"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/web"
Expand Down Expand Up @@ -256,20 +255,12 @@ func SearchIssues(ctx *context.APIContext) {
}
}

// this api is also used in UI,
// so the default limit is set to fit UI needs
limit := ctx.FormInt("limit")
if limit == 0 {
limit = setting.UI.IssuePagingNum
} else if limit > setting.API.MaxResponseItems {
limit = setting.API.MaxResponseItems
}
opts := utils.GetListOptions(ctx)
// field opts.PageSize is used below
opts.SetDefaultValues()

searchOpt := &issue_indexer.SearchOptions{
Paginator: &db.ListOptions{
PageSize: limit,
Page: ctx.FormInt("page"),
},
Paginator: &opts,
Keyword: keyword,
RepoIDs: repoIDs,
AllPublic: allPublic,
Expand Down Expand Up @@ -321,7 +312,7 @@ func SearchIssues(ctx *context.APIContext) {
return
}

ctx.SetLinkHeader(int(total), limit)
ctx.SetLinkHeader(int(total), opts.PageSize)
ctx.SetTotalCountHeader(total)
ctx.JSON(http.StatusOK, convert.ToAPIIssueList(ctx, ctx.Doer, issues))
}
Expand Down
35 changes: 9 additions & 26 deletions routers/api/v1/repo/issue_dependency.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ package repo
import (
"net/http"

"code.gitea.io/gitea/models/db"
issues_model "code.gitea.io/gitea/models/issues"
access_model "code.gitea.io/gitea/models/perm/access"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/routers/api/v1/utils"
"code.gitea.io/gitea/services/context"
"code.gitea.io/gitea/services/convert"
)
Expand Down Expand Up @@ -77,26 +77,16 @@ func GetIssueDependencies(ctx *context.APIContext) {
return
}

page := ctx.FormInt("page")
if page <= 1 {
page = 1
}
limit := ctx.FormInt("limit")
if limit == 0 {
limit = setting.API.DefaultPagingNum
} else if limit > setting.API.MaxResponseItems {
limit = setting.API.MaxResponseItems
}
opts := utils.GetListOptions(ctx)
// field opts.PageSize is used below
opts.SetDefaultValues()

canWrite := ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull)

blockerIssues := make([]*issues_model.Issue, 0, limit)
blockerIssues := make([]*issues_model.Issue, 0, opts.PageSize)

// 2. Get the issues this issue depends on, i.e. the `<#b>`: `<issue> <- <#b>`
blockersInfo, err := issue.BlockedByDependencies(ctx, db.ListOptions{
Page: page,
PageSize: limit,
})
blockersInfo, err := issue.BlockedByDependencies(ctx, opts)
if err != nil {
ctx.APIErrorInternal(err)
return
Expand Down Expand Up @@ -328,17 +318,10 @@ func GetIssueBlocks(ctx *context.APIContext) {
return
}

page := ctx.FormInt("page")
if page <= 1 {
page = 1
}
limit := ctx.FormInt("limit")
if limit <= 1 {
limit = setting.API.DefaultPagingNum
}
opts := utils.GetListOptions(ctx)

skip := (page - 1) * limit
maxNum := page * limit
skip, limit := opts.GetSkipTake()
maxNum := skip + limit

deps, err := issue.BlockingDependencies(ctx)
if err != nil {
Expand Down
6 changes: 5 additions & 1 deletion routers/api/v1/repo/label.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ func ListLabels(ctx *context.APIContext) {
// "404":
// "$ref": "#/responses/notFound"

labels, err := issues_model.GetLabelsByRepoID(ctx, ctx.Repo.Repository.ID, ctx.FormString("sort"), utils.GetListOptions(ctx))
opts := utils.GetListOptions(ctx)
// GetLabelsByRepoID allows pagination bypass
opts.SetDefaultValues()

labels, err := issues_model.GetLabelsByRepoID(ctx, ctx.Repo.Repository.ID, ctx.FormString("sort"), opts)
if err != nil {
ctx.APIErrorInternal(err)
return
Expand Down
7 changes: 5 additions & 2 deletions routers/api/v1/repo/mirror.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,11 @@ func ListPushMirrors(ctx *context.APIContext) {
}

repo := ctx.Repo.Repository
opts := utils.GetListOptions(ctx)
// field opts.PageSize is used below
opts.SetDefaultValues()
// Get all push mirrors for the specified repository.
pushMirrors, count, err := repo_model.GetPushMirrorsByRepoID(ctx, repo.ID, utils.GetListOptions(ctx))
pushMirrors, count, err := repo_model.GetPushMirrorsByRepoID(ctx, repo.ID, opts)
if err != nil {
ctx.APIError(http.StatusNotFound, err)
return
Expand All @@ -179,7 +182,7 @@ func ListPushMirrors(ctx *context.APIContext) {
responsePushMirrors = append(responsePushMirrors, m)
}
}
ctx.SetLinkHeader(len(responsePushMirrors), utils.GetListOptions(ctx).PageSize)
ctx.SetLinkHeader(len(responsePushMirrors), opts.PageSize)
ctx.SetTotalCountHeader(count)
ctx.JSON(http.StatusOK, responsePushMirrors)
}
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ func ListPullReviews(ctx *context.APIContext) {
ListOptions: utils.GetListOptions(ctx),
IssueID: pr.IssueID,
}
// FindReviews allows pagination bypass
opts.SetDefaultValues()

allReviews, err := issues_model.FindReviews(ctx, opts)
if err != nil {
Expand Down
6 changes: 5 additions & 1 deletion routers/api/v1/repo/star.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ func ListStargazers(ctx *context.APIContext) {
// "403":
// "$ref": "#/responses/forbidden"

stargazers, err := repo_model.GetStargazers(ctx, ctx.Repo.Repository, utils.GetListOptions(ctx))
opts := utils.GetListOptions(ctx)
// GetStargazers allows pagination bypass
opts.SetDefaultValues()

stargazers, err := repo_model.GetStargazers(ctx, ctx.Repo.Repository, opts)
if err != nil {
ctx.APIErrorInternal(err)
return
Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/repo/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ func ListTags(ctx *context.APIContext) {
// "$ref": "#/responses/notFound"

listOpts := utils.GetListOptions(ctx)
// GetTagInfos allows pagination bypass
listOpts.SetDefaultValues()

tags, total, err := ctx.Repo.GitRepo.GetTagInfos(listOpts.Page, listOpts.PageSize)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion routers/api/v1/repo/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package repo
import (
"net/http"

"code.gitea.io/gitea/routers/api/v1/utils"
"code.gitea.io/gitea/services/context"
files_service "code.gitea.io/gitea/services/repository/files"
)
Expand Down Expand Up @@ -61,7 +62,9 @@ func GetTree(ctx *context.APIContext) {
ctx.APIError(http.StatusBadRequest, "sha not provided")
return
}
if tree, err := files_service.GetTreeBySHA(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, sha, ctx.FormInt("page"), ctx.FormInt("per_page"), ctx.FormBool("recursive")); err != nil {
opts := utils.GetListOptions(ctx)
opts.SetDefaultValues()
if tree, err := files_service.GetTreeBySHA(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, sha, opts.Page, opts.PageSize, ctx.FormBool("recursive")); err != nil {
ctx.APIError(http.StatusBadRequest, err.Error())
} else {
ctx.SetTotalCountHeader(int64(tree.TotalCount))
Expand Down
Loading