From 072534c388cf20924be3e359819c2674d6d4190a Mon Sep 17 00:00:00 2001 From: leudz Date: Thu, 10 Apr 2025 07:57:43 +0200 Subject: [PATCH 1/7] Fix multi pages search for gh search --- pkg/search/searcher.go | 88 ++++++++--- pkg/search/searcher_test.go | 291 ++++++++++++++++++++++++++++++++++-- 2 files changed, 343 insertions(+), 36 deletions(-) diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index 4168dc7f3..155484ac9 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -14,6 +14,7 @@ import ( ) const ( + // GitHub API has a limit of 100 per page maxPerPage = 100 orderKey = "order" sortKey = "sort" @@ -60,96 +61,138 @@ func NewSearcher(client *http.Client, host string) Searcher { func (s searcher) Code(query Query) (CodeResult, error) { result := CodeResult{} - toRetrieve := query.Limit + var resp *http.Response var err error + + toRetrieve := query.Limit + // We will request either the query limit if it's less than 1 page, or our max page size. + // This number doesn't change to keep a valid offset. + // + // For example, say we want 150 items out of 500. + // We request page #1 for 100 items and get items 0 to 99. + // Then we request page #2 for 100 items, we get items 100 to 199 and only keep 100 to 149. + // If we were to request page #2 for 50 items, we would instead get items 50 to 99. + query.Limit = min(toRetrieve, maxPerPage) + for toRetrieve > 0 { - query.Limit = min(toRetrieve, maxPerPage) query.Page = nextPage(resp) if query.Page == 0 { break } + page := CodeResult{} resp, err = s.search(query, &page) if err != nil { return result, err } + result.IncompleteResults = page.IncompleteResults - result.Total = page.Total - result.Items = append(result.Items, page.Items...) - toRetrieve = toRetrieve - len(page.Items) + + // If we're going to reach the requested limit, only add that many items, + // otherwise add all the results. + itemsToAdd := min(len(page.Items), toRetrieve) + + result.Total += itemsToAdd + result.Items = append(result.Items, page.Items[:itemsToAdd]...) + toRetrieve = toRetrieve - itemsToAdd } + return result, nil } func (s searcher) Commits(query Query) (CommitsResult, error) { result := CommitsResult{} - toRetrieve := query.Limit + var resp *http.Response var err error + + toRetrieve := query.Limit + query.Limit = min(toRetrieve, maxPerPage) + for toRetrieve > 0 { - query.Limit = min(toRetrieve, maxPerPage) query.Page = nextPage(resp) if query.Page == 0 { break } + page := CommitsResult{} resp, err = s.search(query, &page) if err != nil { return result, err } + result.IncompleteResults = page.IncompleteResults - result.Total = page.Total - result.Items = append(result.Items, page.Items...) - toRetrieve = toRetrieve - len(page.Items) + + itemsToAdd := min(len(page.Items), toRetrieve) + + result.Total += itemsToAdd + result.Items = append(result.Items, page.Items[:itemsToAdd]...) + toRetrieve = toRetrieve - itemsToAdd } return result, nil } func (s searcher) Repositories(query Query) (RepositoriesResult, error) { result := RepositoriesResult{} - toRetrieve := query.Limit + var resp *http.Response var err error + + toRetrieve := query.Limit + query.Limit = min(toRetrieve, maxPerPage) + for toRetrieve > 0 { - query.Limit = min(toRetrieve, maxPerPage) query.Page = nextPage(resp) if query.Page == 0 { break } + page := RepositoriesResult{} resp, err = s.search(query, &page) if err != nil { return result, err } + result.IncompleteResults = page.IncompleteResults - result.Total = page.Total - result.Items = append(result.Items, page.Items...) - toRetrieve = toRetrieve - len(page.Items) + + itemsToAdd := min(len(page.Items), toRetrieve) + + result.Total += itemsToAdd + result.Items = append(result.Items, page.Items[:itemsToAdd]...) + toRetrieve = toRetrieve - itemsToAdd } return result, nil } func (s searcher) Issues(query Query) (IssuesResult, error) { result := IssuesResult{} - toRetrieve := query.Limit + var resp *http.Response var err error + + toRetrieve := query.Limit + query.Limit = min(toRetrieve, maxPerPage) + for toRetrieve > 0 { - query.Limit = min(toRetrieve, maxPerPage) query.Page = nextPage(resp) if query.Page == 0 { break } + page := IssuesResult{} resp, err = s.search(query, &page) if err != nil { return result, err } + result.IncompleteResults = page.IncompleteResults - result.Total = page.Total - result.Items = append(result.Items, page.Items...) - toRetrieve = toRetrieve - len(page.Items) + + itemsToAdd := min(len(page.Items), toRetrieve) + + result.Total += itemsToAdd + result.Items = append(result.Items, page.Items[:itemsToAdd]...) + toRetrieve = toRetrieve - itemsToAdd } return result, nil } @@ -236,10 +279,15 @@ func handleHTTPError(resp *http.Response) error { return httpError } +// https://docs.github.com/en/rest/using-the-rest-api/using-pagination-in-the-rest-api func nextPage(resp *http.Response) (page int) { if resp == nil { return 1 } + + // When using pagination, responses get a "Link" field in their header. + // When a next page is available, "Link" contains a link to the next page + // tagged with rel="next". for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { if !(len(m) > 2 && m[2] == "next") { continue diff --git a/pkg/search/searcher_test.go b/pkg/search/searcher_test.go index 8642feed0..1ab8379dd 100644 --- a/pkg/search/searcher_test.go +++ b/pkg/search/searcher_test.go @@ -3,6 +3,7 @@ package search import ( "net/http" "net/url" + "strconv" "testing" "github.com/MakeNowJust/heredoc" @@ -26,6 +27,21 @@ func TestSearcherCode(t *testing.T) { "q": []string{"keyword language:go"}, } + multiplePagesTotalItems := make([]Code, 0, 110) + multiplePagesFirstResItems := make([]Code, 0, 100) + multiplePagesSecondResItems := make([]Code, 0, 10) + for i := range 110 { + commit := Code{Name: "name" + strconv.Itoa(i) + ".go"} + + multiplePagesTotalItems = append(multiplePagesTotalItems, commit) + + if i < 100 { + multiplePagesFirstResItems = append(multiplePagesFirstResItems, commit) + } else { + multiplePagesSecondResItems = append(multiplePagesSecondResItems, commit) + } + } + tests := []struct { name string host string @@ -87,20 +103,64 @@ func TestSearcherCode(t *testing.T) { firstRes := httpmock.JSONResponse(CodeResult{ IncompleteResults: false, Items: []Code{{Name: "file.go"}}, - Total: 2, + Total: 1, }, ) - firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ "page": []string{"2"}, - "per_page": []string{"29"}, + "per_page": []string{"30"}, "q": []string{"keyword language:go"}, }, ) secondRes := httpmock.JSONResponse(CodeResult{ IncompleteResults: false, Items: []Code{{Name: "file2.go"}}, - Total: 2, + Total: 1, + }, + ) + reg.Register(firstReq, firstRes) + reg.Register(secondReq, secondRes) + }, + }, + { + name: "collects results for limit above one page", + query: Query{ + Keywords: []string{"keyword"}, + Kind: "code", + Limit: 110, + Qualifiers: Qualifiers{ + Language: "go", + }, + }, + result: CodeResult{ + IncompleteResults: false, + Items: multiplePagesTotalItems, + Total: 110, + }, + httpStubs: func(reg *httpmock.Registry) { + firstReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ + "page": []string{"1"}, + "per_page": []string{"100"}, + "q": []string{"keyword language:go"}, + }) + firstRes := httpmock.JSONResponse(CodeResult{ + IncompleteResults: false, + Items: multiplePagesFirstResItems, + Total: 100, + }, + ) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + secondReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ + "page": []string{"2"}, + "per_page": []string{"100"}, + "q": []string{"keyword language:go"}, + }, + ) + secondRes := httpmock.JSONResponse(CodeResult{ + IncompleteResults: false, + Items: multiplePagesSecondResItems, + Total: 10, }, ) reg.Register(firstReq, firstRes) @@ -181,6 +241,21 @@ func TestSearcherCommits(t *testing.T) { "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, } + multiplePagesTotalItems := make([]Commit, 0, 110) + multiplePagesFirstResItems := make([]Commit, 0, 100) + multiplePagesSecondResItems := make([]Commit, 0, 10) + for i := range 110 { + commit := Commit{Sha: strconv.Itoa(i)} + + multiplePagesTotalItems = append(multiplePagesTotalItems, commit) + + if i < 100 { + multiplePagesFirstResItems = append(multiplePagesFirstResItems, commit) + } else { + multiplePagesSecondResItems = append(multiplePagesSecondResItems, commit) + } + } + tests := []struct { name string host string @@ -242,13 +317,13 @@ func TestSearcherCommits(t *testing.T) { firstRes := httpmock.JSONResponse(CommitsResult{ IncompleteResults: false, Items: []Commit{{Sha: "abc"}}, - Total: 2, + Total: 1, }, ) - firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ "page": []string{"2"}, - "per_page": []string{"29"}, + "per_page": []string{"30"}, "order": []string{"desc"}, "sort": []string{"committer-date"}, "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, @@ -257,7 +332,58 @@ func TestSearcherCommits(t *testing.T) { secondRes := httpmock.JSONResponse(CommitsResult{ IncompleteResults: false, Items: []Commit{{Sha: "def"}}, - Total: 2, + Total: 1, + }, + ) + reg.Register(firstReq, firstRes) + reg.Register(secondReq, secondRes) + }, + }, + { + name: "collects results for limit above one page", + query: Query{ + Keywords: []string{"keyword"}, + Kind: "commits", + Limit: 110, + Order: "desc", + Sort: "committer-date", + Qualifiers: Qualifiers{ + Author: "foobar", + CommitterDate: ">2021-02-28", + }, + }, + result: CommitsResult{ + IncompleteResults: false, + Items: multiplePagesTotalItems, + Total: 110, + }, + httpStubs: func(reg *httpmock.Registry) { + firstReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ + "page": []string{"1"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"committer-date"}, + "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, + }) + firstRes := httpmock.JSONResponse(CommitsResult{ + IncompleteResults: false, + Items: multiplePagesFirstResItems, + Total: 100, + }, + ) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + secondReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ + "page": []string{"2"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"committer-date"}, + "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, + }, + ) + secondRes := httpmock.JSONResponse(CommitsResult{ + IncompleteResults: false, + Items: multiplePagesSecondResItems, + Total: 10, }, ) reg.Register(firstReq, firstRes) @@ -338,6 +464,25 @@ func TestSearcherRepositories(t *testing.T) { "q": []string{"keyword stars:>=5 topic:topic"}, } + multiplePagesTotalItems := make([]Repository, 0, 110) + multiplePagesFirstResItems := make([]any, 0, 100) + multiplePagesSecondResItems := make([]any, 0, 10) + for i := range 110 { + num := strconv.Itoa(i) + + multiplePagesTotalItems = append(multiplePagesTotalItems, Repository{Name: "name" + num}) + + if i < 100 { + multiplePagesFirstResItems = append(multiplePagesFirstResItems, map[string]any{ + "name": "name" + num, + }) + } else { + multiplePagesSecondResItems = append(multiplePagesSecondResItems, map[string]any{ + "name": "name" + num, + }) + } + } + tests := []struct { name string host string @@ -406,17 +551,17 @@ func TestSearcherRepositories(t *testing.T) { firstReq := httpmock.QueryMatcher("GET", "search/repositories", values) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 2, + "total_count": 1, "items": []interface{}{ map[string]interface{}{ "name": "test", }, }, }) - firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ "page": []string{"2"}, - "per_page": []string{"29"}, + "per_page": []string{"30"}, "order": []string{"desc"}, "sort": []string{"stars"}, "q": []string{"keyword stars:>=5 topic:topic"}, @@ -424,7 +569,7 @@ func TestSearcherRepositories(t *testing.T) { ) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 2, + "total_count": 1, "items": []interface{}{ map[string]interface{}{ "name": "cli", @@ -435,6 +580,54 @@ func TestSearcherRepositories(t *testing.T) { reg.Register(secondReq, secondRes) }, }, + { + name: "collects results for limit above one page", + query: Query{ + Keywords: []string{"keyword"}, + Kind: "repositories", + Limit: 110, + Order: "desc", + Sort: "stars", + Qualifiers: Qualifiers{ + Stars: ">=5", + Topic: []string{"topic"}, + }, + }, + result: RepositoriesResult{ + IncompleteResults: false, + Items: multiplePagesTotalItems, + Total: 110, + }, + httpStubs: func(reg *httpmock.Registry) { + firstReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ + "page": []string{"1"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"stars"}, + "q": []string{"keyword stars:>=5 topic:topic"}, + }) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 100, + "items": multiplePagesFirstResItems, + }) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + secondReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ + "page": []string{"2"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"stars"}, + "q": []string{"keyword stars:>=5 topic:topic"}, + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 10, + "items": multiplePagesSecondResItems, + }) + reg.Register(firstReq, firstRes) + reg.Register(secondReq, secondRes) + }, + }, { name: "handles search errors", query: query, @@ -509,6 +702,21 @@ func TestSearcherIssues(t *testing.T) { "q": []string{"keyword is:locked is:public language:go"}, } + multiplePagesTotalItems := make([]Issue, 0, 110) + multiplePagesFirstResItems := make([]Issue, 0, 100) + multiplePagesSecondResItems := make([]Issue, 0, 10) + for i := range 110 { + issue := Issue{Number: i} + + multiplePagesTotalItems = append(multiplePagesTotalItems, issue) + + if i < 100 { + multiplePagesFirstResItems = append(multiplePagesFirstResItems, issue) + } else { + multiplePagesSecondResItems = append(multiplePagesSecondResItems, issue) + } + } + tests := []struct { name string host string @@ -570,13 +778,13 @@ func TestSearcherIssues(t *testing.T) { firstRes := httpmock.JSONResponse(IssuesResult{ IncompleteResults: false, Items: []Issue{{Number: 1234}}, - Total: 2, + Total: 1, }, ) - firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ "page": []string{"2"}, - "per_page": []string{"29"}, + "per_page": []string{"30"}, "order": []string{"desc"}, "sort": []string{"comments"}, "q": []string{"keyword is:locked is:public language:go"}, @@ -585,7 +793,58 @@ func TestSearcherIssues(t *testing.T) { secondRes := httpmock.JSONResponse(IssuesResult{ IncompleteResults: false, Items: []Issue{{Number: 5678}}, - Total: 2, + Total: 1, + }, + ) + reg.Register(firstReq, firstRes) + reg.Register(secondReq, secondRes) + }, + }, + { + name: "collects results for limit above one page", + query: Query{ + Keywords: []string{"keyword"}, + Kind: "issues", + Limit: 110, + Order: "desc", + Sort: "comments", + Qualifiers: Qualifiers{ + Language: "go", + Is: []string{"public", "locked"}, + }, + }, + result: IssuesResult{ + IncompleteResults: false, + Items: multiplePagesTotalItems, + Total: 110, + }, + httpStubs: func(reg *httpmock.Registry) { + firstReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ + "page": []string{"1"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"comments"}, + "q": []string{"keyword is:locked is:public language:go"}, + }) + firstRes := httpmock.JSONResponse(IssuesResult{ + IncompleteResults: false, + Items: multiplePagesFirstResItems, + Total: 100, + }, + ) + firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) + secondReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ + "page": []string{"2"}, + "per_page": []string{"100"}, + "order": []string{"desc"}, + "sort": []string{"comments"}, + "q": []string{"keyword is:locked is:public language:go"}, + }, + ) + secondRes := httpmock.JSONResponse(IssuesResult{ + IncompleteResults: false, + Items: multiplePagesSecondResItems, + Total: 10, }, ) reg.Register(firstReq, firstRes) From 15ff7ba465d08caabf8ed37530fed4fa1114014e Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 15 Apr 2025 08:00:47 -0400 Subject: [PATCH 2/7] Restore result.Total logic, fix formatting This change restores the original logic of passing the search total count logic as is to the result. Additionally, this undoes some of the contributor's formatting changes that increase the changed lines to review. --- pkg/search/searcher.go | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index 155484ac9..6859c2149 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -87,13 +87,11 @@ func (s searcher) Code(query Query) (CodeResult, error) { return result, err } - result.IncompleteResults = page.IncompleteResults - // If we're going to reach the requested limit, only add that many items, // otherwise add all the results. itemsToAdd := min(len(page.Items), toRetrieve) - - result.Total += itemsToAdd + result.IncompleteResults = page.IncompleteResults + result.Total = page.Total result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd } @@ -122,11 +120,9 @@ func (s searcher) Commits(query Query) (CommitsResult, error) { return result, err } - result.IncompleteResults = page.IncompleteResults - itemsToAdd := min(len(page.Items), toRetrieve) - - result.Total += itemsToAdd + result.IncompleteResults = page.IncompleteResults + result.Total = page.Total result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd } @@ -154,10 +150,8 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { return result, err } - result.IncompleteResults = page.IncompleteResults - itemsToAdd := min(len(page.Items), toRetrieve) - + result.IncompleteResults = page.IncompleteResults result.Total += itemsToAdd result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd @@ -186,10 +180,8 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { return result, err } - result.IncompleteResults = page.IncompleteResults - itemsToAdd := min(len(page.Items), toRetrieve) - + result.IncompleteResults = page.IncompleteResults result.Total += itemsToAdd result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd From f928cb1e19e8c642a24331a1aef70ad21c510e73 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 15 Apr 2025 08:36:45 -0400 Subject: [PATCH 3/7] Fix remaining search logic totals --- pkg/search/searcher.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index 6859c2149..4d2154a4a 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -152,7 +152,7 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { itemsToAdd := min(len(page.Items), toRetrieve) result.IncompleteResults = page.IncompleteResults - result.Total += itemsToAdd + result.Total = page.Total result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd } @@ -182,7 +182,7 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { itemsToAdd := min(len(page.Items), toRetrieve) result.IncompleteResults = page.IncompleteResults - result.Total += itemsToAdd + result.Total = page.Total result.Items = append(result.Items, page.Items[:itemsToAdd]...) toRetrieve = toRetrieve - itemsToAdd } From bdfd51ca7fc796e034a69fd4044c06b41f43ebea Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 15 Apr 2025 14:43:10 -0400 Subject: [PATCH 4/7] Fix search tests around totals, initialization This commit is focused on fixing the `searcher` tests for a few reasons: 1. Correcting the `.Total` logic in the previous commit caused changes to tests to fail 2. Tests involving results that exceed the max per page have been improved with new initialize helper, allowing testing table scenarios to be self contained 3. Tests that stub JSON response payloads have been standardized on maps rather than GitHub type primitives (Repository, Issue, Commit, Code, etc) 4. Tests had some minor formatting changes to make them easier to understand and maintain --- pkg/search/searcher_test.go | 355 ++++++++++++++++++------------------ 1 file changed, 173 insertions(+), 182 deletions(-) diff --git a/pkg/search/searcher_test.go b/pkg/search/searcher_test.go index 1ab8379dd..503751e3e 100644 --- a/pkg/search/searcher_test.go +++ b/pkg/search/searcher_test.go @@ -1,6 +1,7 @@ package search import ( + "fmt" "net/http" "net/url" "strconv" @@ -27,21 +28,6 @@ func TestSearcherCode(t *testing.T) { "q": []string{"keyword language:go"}, } - multiplePagesTotalItems := make([]Code, 0, 110) - multiplePagesFirstResItems := make([]Code, 0, 100) - multiplePagesSecondResItems := make([]Code, 0, 10) - for i := range 110 { - commit := Code{Name: "name" + strconv.Itoa(i) + ".go"} - - multiplePagesTotalItems = append(multiplePagesTotalItems, commit) - - if i < 100 { - multiplePagesFirstResItems = append(multiplePagesFirstResItems, commit) - } else { - multiplePagesSecondResItems = append(multiplePagesSecondResItems, commit) - } - } - tests := []struct { name string host string @@ -100,25 +86,22 @@ func TestSearcherCode(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/code", values) - firstRes := httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: []Code{{Name: "file.go"}}, - Total: 1, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []Code{{Name: "file.go"}}, + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ "page": []string{"2"}, "per_page": []string{"30"}, "q": []string{"keyword language:go"}, - }, - ) - secondRes := httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: []Code{{Name: "file2.go"}}, - Total: 1, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []Code{{Name: "file2.go"}}, + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -135,8 +118,12 @@ func TestSearcherCode(t *testing.T) { }, result: CodeResult{ IncompleteResults: false, - Items: multiplePagesTotalItems, - Total: 110, + Items: initialize(0, 110, func(i int) Code { + return Code{ + Name: fmt.Sprintf("name%d.go", i), + } + }), + Total: 110, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ @@ -144,25 +131,30 @@ func TestSearcherCode(t *testing.T) { "per_page": []string{"100"}, "q": []string{"keyword language:go"}, }) - firstRes := httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: multiplePagesFirstResItems, - Total: 100, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(0, 100, func(i int) interface{} { + return map[string]interface{}{ + "name": fmt.Sprintf("name%d.go", i), + } + }), + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ "page": []string{"2"}, "per_page": []string{"100"}, "q": []string{"keyword language:go"}, - }, - ) - secondRes := httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: multiplePagesSecondResItems, - Total: 10, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(100, 110, func(i int) interface{} { + return map[string]interface{}{ + "name": fmt.Sprintf("name%d.go", i), + } + }), + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -241,21 +233,6 @@ func TestSearcherCommits(t *testing.T) { "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, } - multiplePagesTotalItems := make([]Commit, 0, 110) - multiplePagesFirstResItems := make([]Commit, 0, 100) - multiplePagesSecondResItems := make([]Commit, 0, 10) - for i := range 110 { - commit := Commit{Sha: strconv.Itoa(i)} - - multiplePagesTotalItems = append(multiplePagesTotalItems, commit) - - if i < 100 { - multiplePagesFirstResItems = append(multiplePagesFirstResItems, commit) - } else { - multiplePagesSecondResItems = append(multiplePagesSecondResItems, commit) - } - } - tests := []struct { name string host string @@ -296,10 +273,10 @@ func TestSearcherCommits(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "api/v3/search/commits", values), - httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: []Commit{{Sha: "abc"}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []Commit{{Sha: "abc"}}, }), ) }, @@ -314,12 +291,11 @@ func TestSearcherCommits(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/commits", values) - firstRes := httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: []Commit{{Sha: "abc"}}, - Total: 1, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []Commit{{Sha: "abc"}}, + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ "page": []string{"2"}, @@ -327,14 +303,12 @@ func TestSearcherCommits(t *testing.T) { "order": []string{"desc"}, "sort": []string{"committer-date"}, "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, - }, - ) - secondRes := httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: []Commit{{Sha: "def"}}, - Total: 1, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []Commit{{Sha: "def"}}, + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -354,8 +328,12 @@ func TestSearcherCommits(t *testing.T) { }, result: CommitsResult{ IncompleteResults: false, - Items: multiplePagesTotalItems, - Total: 110, + Items: initialize(0, 110, func(i int) Commit { + return Commit{ + Sha: strconv.Itoa(i), + } + }), + Total: 110, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ @@ -365,12 +343,15 @@ func TestSearcherCommits(t *testing.T) { "sort": []string{"committer-date"}, "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, }) - firstRes := httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: multiplePagesFirstResItems, - Total: 100, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(0, 100, func(i int) Commit { + return Commit{ + Sha: strconv.Itoa(i), + } + }), + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ "page": []string{"2"}, @@ -378,14 +359,16 @@ func TestSearcherCommits(t *testing.T) { "order": []string{"desc"}, "sort": []string{"committer-date"}, "q": []string{"keyword author:foobar committer-date:>2021-02-28"}, - }, - ) - secondRes := httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: multiplePagesSecondResItems, - Total: 10, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(100, 110, func(i int) Commit { + return Commit{ + Sha: strconv.Itoa(i), + } + }), + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -395,8 +378,8 @@ func TestSearcherCommits(t *testing.T) { query: query, wantErr: true, errMsg: heredoc.Doc(` - Invalid search query "keyword author:foobar committer-date:>2021-02-28". - "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), + Invalid search query "keyword author:foobar committer-date:>2021-02-28". + "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/commits", values), @@ -464,25 +447,6 @@ func TestSearcherRepositories(t *testing.T) { "q": []string{"keyword stars:>=5 topic:topic"}, } - multiplePagesTotalItems := make([]Repository, 0, 110) - multiplePagesFirstResItems := make([]any, 0, 100) - multiplePagesSecondResItems := make([]any, 0, 10) - for i := range 110 { - num := strconv.Itoa(i) - - multiplePagesTotalItems = append(multiplePagesTotalItems, Repository{Name: "name" + num}) - - if i < 100 { - multiplePagesFirstResItems = append(multiplePagesFirstResItems, map[string]any{ - "name": "name" + num, - }) - } else { - multiplePagesSecondResItems = append(multiplePagesSecondResItems, map[string]any{ - "name": "name" + num, - }) - } - } - tests := []struct { name string host string @@ -551,7 +515,7 @@ func TestSearcherRepositories(t *testing.T) { firstReq := httpmock.QueryMatcher("GET", "search/repositories", values) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 1, + "total_count": 2, "items": []interface{}{ map[string]interface{}{ "name": "test", @@ -565,11 +529,10 @@ func TestSearcherRepositories(t *testing.T) { "order": []string{"desc"}, "sort": []string{"stars"}, "q": []string{"keyword stars:>=5 topic:topic"}, - }, - ) + }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 1, + "total_count": 2, "items": []interface{}{ map[string]interface{}{ "name": "cli", @@ -595,8 +558,12 @@ func TestSearcherRepositories(t *testing.T) { }, result: RepositoriesResult{ IncompleteResults: false, - Items: multiplePagesTotalItems, - Total: 110, + Items: initialize(0, 110, func(i int) Repository { + return Repository{ + Name: fmt.Sprintf("name%d", i), + } + }), + Total: 110, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ @@ -608,8 +575,12 @@ func TestSearcherRepositories(t *testing.T) { }) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 100, - "items": multiplePagesFirstResItems, + "total_count": 110, + "items": initialize(0, 100, func(i int) interface{} { + return map[string]interface{}{ + "name": fmt.Sprintf("name%d", i), + } + }), }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ @@ -621,8 +592,12 @@ func TestSearcherRepositories(t *testing.T) { }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 10, - "items": multiplePagesSecondResItems, + "total_count": 110, + "items": initialize(100, 110, func(i int) interface{} { + return map[string]interface{}{ + "name": fmt.Sprintf("name%d", i), + } + }), }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) @@ -633,8 +608,8 @@ func TestSearcherRepositories(t *testing.T) { query: query, wantErr: true, errMsg: heredoc.Doc(` - Invalid search query "keyword stars:>=5 topic:topic". - "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), + Invalid search query "keyword stars:>=5 topic:topic". + "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/repositories", values), @@ -702,21 +677,6 @@ func TestSearcherIssues(t *testing.T) { "q": []string{"keyword is:locked is:public language:go"}, } - multiplePagesTotalItems := make([]Issue, 0, 110) - multiplePagesFirstResItems := make([]Issue, 0, 100) - multiplePagesSecondResItems := make([]Issue, 0, 10) - for i := range 110 { - issue := Issue{Number: i} - - multiplePagesTotalItems = append(multiplePagesTotalItems, issue) - - if i < 100 { - multiplePagesFirstResItems = append(multiplePagesFirstResItems, issue) - } else { - multiplePagesSecondResItems = append(multiplePagesSecondResItems, issue) - } - } - tests := []struct { name string host string @@ -737,10 +697,14 @@ func TestSearcherIssues(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/issues", values), - httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: []Issue{{Number: 1234}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []interface{}{ + map[string]interface{}{ + "number": 1234, + }, + }, }), ) }, @@ -757,10 +721,14 @@ func TestSearcherIssues(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "api/v3/search/issues", values), - httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: []Issue{{Number: 1234}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []interface{}{ + map[string]interface{}{ + "number": 1234, + }, + }, }), ) }, @@ -775,12 +743,15 @@ func TestSearcherIssues(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/issues", values) - firstRes := httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: []Issue{{Number: 1234}}, - Total: 1, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []interface{}{ + map[string]interface{}{ + "number": 1234, + }, + }, + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ "page": []string{"2"}, @@ -788,14 +759,16 @@ func TestSearcherIssues(t *testing.T) { "order": []string{"desc"}, "sort": []string{"comments"}, "q": []string{"keyword is:locked is:public language:go"}, - }, - ) - secondRes := httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: []Issue{{Number: 5678}}, - Total: 1, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 2, + "items": []interface{}{ + map[string]interface{}{ + "number": 5678, + }, + }, + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -815,8 +788,12 @@ func TestSearcherIssues(t *testing.T) { }, result: IssuesResult{ IncompleteResults: false, - Items: multiplePagesTotalItems, - Total: 110, + Items: initialize(0, 110, func(i int) Issue { + return Issue{ + Number: i, + } + }), + Total: 110, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ @@ -826,12 +803,15 @@ func TestSearcherIssues(t *testing.T) { "sort": []string{"comments"}, "q": []string{"keyword is:locked is:public language:go"}, }) - firstRes := httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: multiplePagesFirstResItems, - Total: 100, - }, - ) + firstRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(0, 100, func(i int) interface{} { + return map[string]interface{}{ + "number": i, + } + }), + }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ "page": []string{"2"}, @@ -839,14 +819,16 @@ func TestSearcherIssues(t *testing.T) { "order": []string{"desc"}, "sort": []string{"comments"}, "q": []string{"keyword is:locked is:public language:go"}, - }, - ) - secondRes := httpmock.JSONResponse(IssuesResult{ - IncompleteResults: false, - Items: multiplePagesSecondResItems, - Total: 10, - }, - ) + }) + secondRes := httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 110, + "items": initialize(100, 110, func(i int) interface{} { + return map[string]interface{}{ + "number": i, + } + }), + }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, @@ -856,8 +838,8 @@ func TestSearcherIssues(t *testing.T) { query: query, wantErr: true, errMsg: heredoc.Doc(` - Invalid search query "keyword is:locked is:public language:go". - "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), + Invalid search query "keyword is:locked is:public language:go". + "blah" is not a recognized date/time format. Please provide an ISO 8601 date/time value, such as YYYY-MM-DD.`), httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/issues", values), @@ -945,3 +927,12 @@ func TestSearcherURL(t *testing.T) { }) } } + +// initialize generate slices over a range for test scenarios using the provided initializer. +func initialize[T any](start int, stop int, initializer func(i int) T) []T { + results := make([]T, 0, (stop - start)) + for i := start; i < stop; i++ { + results = append(results, initializer(i)) + } + return results +} From becd936e7b904dfc04ba96089e5318a07879870d Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 15 Apr 2025 17:13:43 -0400 Subject: [PATCH 5/7] Update searcher tests This commit moves the remaining searcher tests from using JSON marshaled types to using JSON responses for consistency. There appears to be a weird JSON marshaling error with search.Repository that does not map `Name` field in the process. Additionally, the test scenarios around pulling multiple pages beneath the total results have been updated to demonstrate that the REST API returns full pages in both of these cases, which is below the total number of results. --- pkg/search/searcher_test.go | 116 +++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 42 deletions(-) diff --git a/pkg/search/searcher_test.go b/pkg/search/searcher_test.go index 503751e3e..e893c9a3b 100644 --- a/pkg/search/searcher_test.go +++ b/pkg/search/searcher_test.go @@ -48,10 +48,14 @@ func TestSearcherCode(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/code", values), - httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: []Code{{Name: "file.go"}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []interface{}{ + map[string]interface{}{ + "name": "file.go", + }, + }, }), ) }, @@ -68,10 +72,14 @@ func TestSearcherCode(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "api/v3/search/code", values), - httpmock.JSONResponse(CodeResult{ - IncompleteResults: false, - Items: []Code{{Name: "file.go"}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []interface{}{ + map[string]interface{}{ + "name": "file.go", + }, + }, }), ) }, @@ -89,7 +97,11 @@ func TestSearcherCode(t *testing.T) { firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, "total_count": 2, - "items": []Code{{Name: "file.go"}}, + "items": []interface{}{ + map[string]interface{}{ + "name": "file.go", + }, + }, }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ @@ -100,14 +112,18 @@ func TestSearcherCode(t *testing.T) { secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, "total_count": 2, - "items": []Code{{Name: "file2.go"}}, + "items": []interface{}{ + map[string]interface{}{ + "name": "file2.go", + }, + }, }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, }, { - name: "collects results for limit above one page", + name: "collect full and partial pages under total number of matching search results", query: Query{ Keywords: []string{"keyword"}, Kind: "code", @@ -123,7 +139,7 @@ func TestSearcherCode(t *testing.T) { Name: fmt.Sprintf("name%d.go", i), } }), - Total: 110, + Total: 287, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/code", url.Values{ @@ -133,7 +149,7 @@ func TestSearcherCode(t *testing.T) { }) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, + "total_count": 287, "items": initialize(0, 100, func(i int) interface{} { return map[string]interface{}{ "name": fmt.Sprintf("name%d.go", i), @@ -148,8 +164,8 @@ func TestSearcherCode(t *testing.T) { }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, - "items": initialize(100, 110, func(i int) interface{} { + "total_count": 287, + "items": initialize(100, 200, func(i int) interface{} { return map[string]interface{}{ "name": fmt.Sprintf("name%d.go", i), } @@ -253,10 +269,14 @@ func TestSearcherCommits(t *testing.T) { httpStubs: func(reg *httpmock.Registry) { reg.Register( httpmock.QueryMatcher("GET", "search/commits", values), - httpmock.JSONResponse(CommitsResult{ - IncompleteResults: false, - Items: []Commit{{Sha: "abc"}}, - Total: 1, + httpmock.JSONResponse(map[string]interface{}{ + "incomplete_results": false, + "total_count": 1, + "items": []interface{}{ + map[string]interface{}{ + "sha": "abc", + }, + }, }), ) }, @@ -276,7 +296,11 @@ func TestSearcherCommits(t *testing.T) { httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, "total_count": 1, - "items": []Commit{{Sha: "abc"}}, + "items": []interface{}{ + map[string]interface{}{ + "sha": "abc", + }, + }, }), ) }, @@ -294,7 +318,11 @@ func TestSearcherCommits(t *testing.T) { firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, "total_count": 2, - "items": []Commit{{Sha: "abc"}}, + "items": []interface{}{ + map[string]interface{}{ + "sha": "abc", + }, + }, }) firstRes = httpmock.WithHeader(firstRes, "Link", `; rel="next"`) secondReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ @@ -307,14 +335,18 @@ func TestSearcherCommits(t *testing.T) { secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, "total_count": 2, - "items": []Commit{{Sha: "def"}}, + "items": []interface{}{ + map[string]interface{}{ + "sha": "def", + }, + }, }) reg.Register(firstReq, firstRes) reg.Register(secondReq, secondRes) }, }, { - name: "collects results for limit above one page", + name: "collect full and partial pages under total number of matching search results", query: Query{ Keywords: []string{"keyword"}, Kind: "commits", @@ -333,7 +365,7 @@ func TestSearcherCommits(t *testing.T) { Sha: strconv.Itoa(i), } }), - Total: 110, + Total: 287, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/commits", url.Values{ @@ -345,10 +377,10 @@ func TestSearcherCommits(t *testing.T) { }) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, - "items": initialize(0, 100, func(i int) Commit { - return Commit{ - Sha: strconv.Itoa(i), + "total_count": 287, + "items": initialize(0, 100, func(i int) map[string]interface{} { + return map[string]interface{}{ + "sha": strconv.Itoa(i), } }), }) @@ -362,10 +394,10 @@ func TestSearcherCommits(t *testing.T) { }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, - "items": initialize(100, 110, func(i int) Commit { - return Commit{ - Sha: strconv.Itoa(i), + "total_count": 287, + "items": initialize(100, 200, func(i int) map[string]interface{} { + return map[string]interface{}{ + "sha": strconv.Itoa(i), } }), }) @@ -544,7 +576,7 @@ func TestSearcherRepositories(t *testing.T) { }, }, { - name: "collects results for limit above one page", + name: "collect full and partial pages under total number of matching search results", query: Query{ Keywords: []string{"keyword"}, Kind: "repositories", @@ -563,7 +595,7 @@ func TestSearcherRepositories(t *testing.T) { Name: fmt.Sprintf("name%d", i), } }), - Total: 110, + Total: 287, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/repositories", url.Values{ @@ -575,7 +607,7 @@ func TestSearcherRepositories(t *testing.T) { }) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, + "total_count": 287, "items": initialize(0, 100, func(i int) interface{} { return map[string]interface{}{ "name": fmt.Sprintf("name%d", i), @@ -592,8 +624,8 @@ func TestSearcherRepositories(t *testing.T) { }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, - "items": initialize(100, 110, func(i int) interface{} { + "total_count": 287, + "items": initialize(100, 200, func(i int) interface{} { return map[string]interface{}{ "name": fmt.Sprintf("name%d", i), } @@ -774,7 +806,7 @@ func TestSearcherIssues(t *testing.T) { }, }, { - name: "collects results for limit above one page", + name: "collect full and partial pages under total number of matching search results", query: Query{ Keywords: []string{"keyword"}, Kind: "issues", @@ -793,7 +825,7 @@ func TestSearcherIssues(t *testing.T) { Number: i, } }), - Total: 110, + Total: 287, }, httpStubs: func(reg *httpmock.Registry) { firstReq := httpmock.QueryMatcher("GET", "search/issues", url.Values{ @@ -805,7 +837,7 @@ func TestSearcherIssues(t *testing.T) { }) firstRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, + "total_count": 287, "items": initialize(0, 100, func(i int) interface{} { return map[string]interface{}{ "number": i, @@ -822,8 +854,8 @@ func TestSearcherIssues(t *testing.T) { }) secondRes := httpmock.JSONResponse(map[string]interface{}{ "incomplete_results": false, - "total_count": 110, - "items": initialize(100, 110, func(i int) interface{} { + "total_count": 287, + "items": initialize(100, 200, func(i int) interface{} { return map[string]interface{}{ "number": i, } From ba390db71fe16e86aca1bb0fd5c6e0d8edd55735 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 16 Apr 2025 14:22:29 -0400 Subject: [PATCH 6/7] PR feedback - update local variables to communicate what they are - added docblock explaining search results populated --- pkg/search/searcher.go | 57 ++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index 4d2154a4a..d1f86fdcd 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -65,7 +65,6 @@ func (s searcher) Code(query Query) (CodeResult, error) { var resp *http.Response var err error - toRetrieve := query.Limit // We will request either the query limit if it's less than 1 page, or our max page size. // This number doesn't change to keep a valid offset. // @@ -73,9 +72,10 @@ func (s searcher) Code(query Query) (CodeResult, error) { // We request page #1 for 100 items and get items 0 to 99. // Then we request page #2 for 100 items, we get items 100 to 199 and only keep 100 to 149. // If we were to request page #2 for 50 items, we would instead get items 50 to 99. - query.Limit = min(toRetrieve, maxPerPage) + numItemsToRetrieve := query.Limit + query.Limit = min(numItemsToRetrieve, maxPerPage) - for toRetrieve > 0 { + for numItemsToRetrieve > 0 { query.Page = nextPage(resp) if query.Page == 0 { break @@ -89,11 +89,11 @@ func (s searcher) Code(query Query) (CodeResult, error) { // If we're going to reach the requested limit, only add that many items, // otherwise add all the results. - itemsToAdd := min(len(page.Items), toRetrieve) + numItemsToAdd := min(len(page.Items), numItemsToRetrieve) result.IncompleteResults = page.IncompleteResults result.Total = page.Total - result.Items = append(result.Items, page.Items[:itemsToAdd]...) - toRetrieve = toRetrieve - itemsToAdd + result.Items = append(result.Items, page.Items[:numItemsToAdd]...) + numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd } return result, nil @@ -105,10 +105,10 @@ func (s searcher) Commits(query Query) (CommitsResult, error) { var resp *http.Response var err error - toRetrieve := query.Limit - query.Limit = min(toRetrieve, maxPerPage) + numItemsToRetrieve := query.Limit + query.Limit = min(numItemsToRetrieve, maxPerPage) - for toRetrieve > 0 { + for numItemsToRetrieve > 0 { query.Page = nextPage(resp) if query.Page == 0 { break @@ -120,11 +120,11 @@ func (s searcher) Commits(query Query) (CommitsResult, error) { return result, err } - itemsToAdd := min(len(page.Items), toRetrieve) + numItemsToAdd := min(len(page.Items), numItemsToRetrieve) result.IncompleteResults = page.IncompleteResults result.Total = page.Total - result.Items = append(result.Items, page.Items[:itemsToAdd]...) - toRetrieve = toRetrieve - itemsToAdd + result.Items = append(result.Items, page.Items[:numItemsToAdd]...) + numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd } return result, nil } @@ -135,10 +135,10 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { var resp *http.Response var err error - toRetrieve := query.Limit - query.Limit = min(toRetrieve, maxPerPage) + numItemsToRetrieve := query.Limit + query.Limit = min(numItemsToRetrieve, maxPerPage) - for toRetrieve > 0 { + for numItemsToRetrieve > 0 { query.Page = nextPage(resp) if query.Page == 0 { break @@ -150,11 +150,11 @@ func (s searcher) Repositories(query Query) (RepositoriesResult, error) { return result, err } - itemsToAdd := min(len(page.Items), toRetrieve) + numItemsToAdd := min(len(page.Items), numItemsToRetrieve) result.IncompleteResults = page.IncompleteResults result.Total = page.Total - result.Items = append(result.Items, page.Items[:itemsToAdd]...) - toRetrieve = toRetrieve - itemsToAdd + result.Items = append(result.Items, page.Items[:numItemsToAdd]...) + numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd } return result, nil } @@ -165,10 +165,10 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { var resp *http.Response var err error - toRetrieve := query.Limit - query.Limit = min(toRetrieve, maxPerPage) + numItemsToRetrieve := query.Limit + query.Limit = min(numItemsToRetrieve, maxPerPage) - for toRetrieve > 0 { + for numItemsToRetrieve > 0 { query.Page = nextPage(resp) if query.Page == 0 { break @@ -180,15 +180,24 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { return result, err } - itemsToAdd := min(len(page.Items), toRetrieve) + numItemsToAdd := min(len(page.Items), numItemsToRetrieve) result.IncompleteResults = page.IncompleteResults result.Total = page.Total - result.Items = append(result.Items, page.Items[:itemsToAdd]...) - toRetrieve = toRetrieve - itemsToAdd + result.Items = append(result.Items, page.Items[:numItemsToAdd]...) + numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd } return result, nil } +// search makes a single-page REST search request for code, commits, issues, prs, or repos. +// +// The result argument is populated with the following information: +// +// - Total: the number of search results matching the query, which may exceed the number of items returned +// - IncompleteResults: whether the search request exceeded search time limit, potentially being incomplete +// - Items: the actual matching search results, up to 100 max items per page +// +// For more information, see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28. func (s searcher) search(query Query, result interface{}) (*http.Response, error) { path := fmt.Sprintf("%ssearch/%s", ghinstance.RESTPrefix(s.host), query.Kind) qs := url.Values{} From 4a885899d6a724e2c82b52949b7d6a9151d2559c Mon Sep 17 00:00:00 2001 From: leudz Date: Wed, 16 Apr 2025 20:46:29 +0200 Subject: [PATCH 7/7] Add comments for Total --- pkg/search/result.go | 12 ++++++++---- pkg/search/searcher.go | 2 ++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/pkg/search/result.go b/pkg/search/result.go index 0c7c43cd7..0b9d1ab16 100644 --- a/pkg/search/result.go +++ b/pkg/search/result.go @@ -93,25 +93,29 @@ var PullRequestFields = append(IssueFields, type CodeResult struct { IncompleteResults bool `json:"incomplete_results"` Items []Code `json:"items"` - Total int `json:"total_count"` + // Number of code search results matching the query on the server. Ignoring limit. + Total int `json:"total_count"` } type CommitsResult struct { IncompleteResults bool `json:"incomplete_results"` Items []Commit `json:"items"` - Total int `json:"total_count"` + // Number of commits matching the query on the server. Ignoring limit. + Total int `json:"total_count"` } type RepositoriesResult struct { IncompleteResults bool `json:"incomplete_results"` Items []Repository `json:"items"` - Total int `json:"total_count"` + // Number of repositories matching the query on the server. Ignoring limit. + Total int `json:"total_count"` } type IssuesResult struct { IncompleteResults bool `json:"incomplete_results"` Items []Issue `json:"items"` - Total int `json:"total_count"` + // Number of isssues matching the query on the server. Ignoring limit. + Total int `json:"total_count"` } type Code struct { diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index d1f86fdcd..7cbd35562 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -91,6 +91,8 @@ func (s searcher) Code(query Query) (CodeResult, error) { // otherwise add all the results. numItemsToAdd := min(len(page.Items), numItemsToRetrieve) result.IncompleteResults = page.IncompleteResults + // The API returns how many items match the query in every response. + // With the example above, this would be 500. result.Total = page.Total result.Items = append(result.Items, page.Items[:numItemsToAdd]...) numItemsToRetrieve = numItemsToRetrieve - numItemsToAdd