From 3f26fe2fa2c7141c9e622297e50a70f3e0003e4d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 25 Mar 2024 02:51:08 +0800 Subject: [PATCH] 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 Co-authored-by: silverwind --- models/issues/issue_search.go | 22 +++++-------------- models/issues/issue_stats.go | 6 ++++- modules/indexer/internal/paginator.go | 21 ++++++------------ modules/indexer/issues/db/db.go | 11 ++++++++++ modules/indexer/issues/indexer.go | 2 +- modules/indexer/issues/internal/model.go | 2 +- .../indexer/issues/internal/tests/tests.go | 7 ++++++ .../indexer/issues/meilisearch/meilisearch.go | 12 ++++++++++ 8 files changed, 49 insertions(+), 34 deletions(-) diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index 4e1bd9e87..921dd9973 100644 --- a/models/issues/issue_search.go +++ b/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 } diff --git a/models/issues/issue_stats.go b/models/issues/issue_stats.go index 32c5674fc..39326616f 100644 --- a/models/issues/issue_stats.go +++ b/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() } diff --git a/modules/indexer/internal/paginator.go b/modules/indexer/internal/paginator.go index de0a33c06..ee204bf04 100644 --- a/modules/indexer/internal/paginator.go +++ b/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() diff --git a/modules/indexer/issues/db/db.go b/modules/indexer/issues/db/db.go index 1016523b7..05ec54843 100644 --- a/modules/indexer/issues/db/db.go +++ b/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 diff --git a/modules/indexer/issues/indexer.go b/modules/indexer/issues/indexer.go index e3bc21b49..1cb86feb8 100644 --- a/modules/indexer/issues/indexer.go +++ b/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 diff --git a/modules/indexer/issues/internal/model.go b/modules/indexer/issues/internal/model.go index b7102c35a..e9c4eca55 100644 --- a/modules/indexer/issues/internal/model.go +++ b/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 } diff --git a/modules/indexer/issues/internal/tests/tests.go b/modules/indexer/issues/internal/tests/tests.go index 2209377c2..7f32876d8 100644 --- a/modules/indexer/issues/internal/tests/tests.go +++ b/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) }) } } diff --git a/modules/indexer/issues/meilisearch/meilisearch.go b/modules/indexer/issues/meilisearch/meilisearch.go index b735c2696..8a7cec6cb 100644 --- a/modules/indexer/issues/meilisearch/meilisearch.go +++ b/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