Use db.ListOptions directly instead of Paginator interface to make it easier to use and fix performance of /pulls and /issues (#29990)

This PR uses `db.ListOptions` instead of `Paginor` to make the code
simpler.
And it also fixed the performance problem when viewing /pulls or
/issues. Before the counting in fact will also do the search.

---------

Co-authored-by: Jason Song <[email protected]>
Co-authored-by: silverwind <[email protected]>

Author: 3 people

models/issues/issue_search.go

@@ -21,7 +21,7 @@ import (
 
 // IssuesOptions represents options of an issue.
 type IssuesOptions struct { //nolint
-	db.Paginator
+	Paginator          *db.ListOptions
 	RepoIDs            []int64 // overwrites RepoCond if the length is not 0
 	AllPublic          bool    // include also all public repositories
 	RepoCond           builder.Cond
@@ -104,23 +104,11 @@ func applyLimit(sess *xorm.Session, opts *IssuesOptions) *xorm.Session {
 		return sess
 	}
 
-	// Warning: Do not use GetSkipTake() for *db.ListOptions
-	// Its implementation could reset the page size with setting.API.MaxResponseItems
-	if listOptions, ok := opts.Paginator.(*db.ListOptions); ok {
-		if listOptions.Page >= 0 && listOptions.PageSize > 0 {
-			var start int
-			if listOptions.Page == 0 {
-				start = 0
-			} else {
-				start = (listOptions.Page - 1) * listOptions.PageSize
-			}
-			sess.Limit(listOptions.PageSize, start)
-		}
-		return sess
+	start := 0
+	if opts.Paginator.Page > 1 {
+		start = (opts.Paginator.Page - 1) * opts.Paginator.PageSize
 	}
-
-	start, limit := opts.Paginator.GetSkipTake()
-	sess.Limit(limit, start)
+	sess.Limit(opts.Paginator.PageSize, start)
 
 	return sess
 }

models/issues/issue_stats.go

@@ -68,13 +68,17 @@ func CountIssuesByRepo(ctx context.Context, opts *IssuesOptions) (map[int64]int6
 }
 
 // CountIssues number return of issues by given conditions.
-func CountIssues(ctx context.Context, opts *IssuesOptions) (int64, error) {
+func CountIssues(ctx context.Context, opts *IssuesOptions, otherConds ...builder.Cond) (int64, error) {
 	sess := db.GetEngine(ctx).
 		Select("COUNT(issue.id) AS count").
 		Table("issue").
 		Join("INNER", "repository", "`issue`.repo_id = `repository`.id")
 	applyConditions(sess, opts)
 
+	for _, cond := range otherConds {
+		sess.And(cond)
+	}
+
 	return sess.Count()
 }
 

modules/indexer/internal/paginator.go

@@ -10,7 +10,7 @@ import (
 )
 
 // ParsePaginator parses a db.Paginator into a skip and limit
-func ParsePaginator(paginator db.Paginator, max ...int) (int, int) {
+func ParsePaginator(paginator *db.ListOptions, max ...int) (int, int) {
 	// Use a very large number to indicate no limit
 	unlimited := math.MaxInt32
 	if len(max) > 0 {
@@ -19,22 +19,15 @@ func ParsePaginator(paginator db.Paginator, max ...int) (int, int) {
 	}
 
 	if paginator == nil || paginator.IsListAll() {
+		// It shouldn't happen. In actual usage scenarios, there should not be requests to search all.
+		// But if it does happen, respect it and return "unlimited".
+		// And it's also useful for testing.
 		return 0, unlimited
 	}
 
-	// Warning: Do not use GetSkipTake() for *db.ListOptions
-	// Its implementation could reset the page size with setting.API.MaxResponseItems
-	if listOptions, ok := paginator.(*db.ListOptions); ok {
-		if listOptions.Page >= 0 && listOptions.PageSize > 0 {
-			var start int
-			if listOptions.Page == 0 {
-				start = 0
-			} else {
-				start = (listOptions.Page - 1) * listOptions.PageSize
-			}
-			return start, listOptions.PageSize
-		}
-		return 0, unlimited
+	if paginator.PageSize == 0 {
+		// Do not return any results when searching, it's used to get the total count only.
+		return 0, 0
 	}
 
 	return paginator.GetSkipTake()

modules/indexer/issues/db/db.go

@@ -78,6 +78,17 @@ func (i *Indexer) Search(ctx context.Context, options *internal.SearchOptions) (
 		return nil, err
 	}
 
+	// If pagesize == 0, return total count only. It's a special case for search count.
+	if options.Paginator != nil && options.Paginator.PageSize == 0 {
+		total, err := issue_model.CountIssues(ctx, opt, cond)
+		if err != nil {
+			return nil, err
+		}
+		return &internal.SearchResult{
+			Total: total,
+		}, nil
+	}
+
 	ids, total, err := issue_model.IssueIDs(ctx, opt, cond)
 	if err != nil {
 		return nil, err

modules/indexer/issues/indexer.go

@@ -308,7 +308,7 @@ func SearchIssues(ctx context.Context, opts *SearchOptions) ([]int64, int64, err
 
 // CountIssues counts issues by options. It is a shortcut of SearchIssues(ctx, opts) but only returns the total count.
 func CountIssues(ctx context.Context, opts *SearchOptions) (int64, error) {
-	opts = opts.Copy(func(options *SearchOptions) { opts.Paginator = &db_model.ListOptions{PageSize: 0} })
+	opts = opts.Copy(func(options *SearchOptions) { options.Paginator = &db_model.ListOptions{PageSize: 0} })
 
 	_, total, err := SearchIssues(ctx, opts)
 	return total, err

modules/indexer/issues/internal/model.go

@@ -106,7 +106,7 @@ type SearchOptions struct {
 	UpdatedAfterUnix  optional.Option[int64]
 	UpdatedBeforeUnix optional.Option[int64]
 
-	db.Paginator
+	Paginator *db.ListOptions
 
 	SortBy SortBy // sort by field
 }

modules/indexer/issues/internal/tests/tests.go

@@ -77,6 +77,13 @@ func TestIndexer(t *testing.T, indexer internal.Indexer) {
 				assert.Equal(t, c.ExpectedIDs, ids)
 				assert.Equal(t, c.ExpectedTotal, result.Total)
 			}
+
+			// test counting
+			c.SearchOptions.Paginator = &db.ListOptions{PageSize: 0}
+			countResult, err := indexer.Search(context.Background(), c.SearchOptions)
+			require.NoError(t, err)
+			assert.Empty(t, countResult.Hits)
+			assert.Equal(t, result.Total, countResult.Total)
 		})
 	}
 }

modules/indexer/issues/meilisearch/meilisearch.go

@@ -218,6 +218,14 @@ func (b *Indexer) Search(ctx context.Context, options *internal.SearchOptions) (
 
 	skip, limit := indexer_internal.ParsePaginator(options.Paginator, maxTotalHits)
 
+	counting := limit == 0
+	if counting {
+		// If set limit to 0, it will be 20 by default, and -1 is not allowed.
+		// See https://www.meilisearch.com/docs/reference/api/search#limit
+		// So set limit to 1 to make the cost as low as possible, then clear the result before returning.
+		limit = 1
+	}
+
 	keyword := options.Keyword
 	if !options.IsFuzzyKeyword {
 		// to make it non fuzzy ("typo tolerance" in meilisearch terms), we have to quote the keyword(s)
@@ -236,6 +244,10 @@ func (b *Indexer) Search(ctx context.Context, options *internal.SearchOptions) (
 		return nil, err
 	}
 
+	if counting {
+		searchRes.Hits = nil
+	}
+
 	hits, err := convertHits(searchRes)
 	if err != nil {
 		return nil, err