From 7c6b1b9ebbc3f5ca126bfb646923b5bc45f42d09 Mon Sep 17 00:00:00 2001 From: ruohai0925 Date: Sun, 2 Aug 2026 02:21:44 -0400 Subject: [PATCH] Fix issue counts always returning zero GitHub migrated the repo issues list endpoint to cursor-based pagination. Its Link header now carries only a rel="next" entry with an opaque cursor and no rel="last", so the per_page=1 trick used by FetchIssueCount reads LastPage as 0 and returns 0 for every repository. This zeroes updated_issues_count and closed_issues_count, and because issue_comment_frequency divides the comment count by the updated count, that signal is zeroed too. Fall back to paging through the results and counting them when the total cannot be read off the Link header. The rel="last" fast path is kept for when the API still provides it, and the MaxIssuesLimit cap is preserved so large repositories do not page indefinitely. The comments and contributors endpoints still return rel="last", so FetchIssueCommentCount and FetchTotalContributors are unchanged. Adds httptest-backed tests for FetchIssueCount, which had no coverage. Signed-off-by: ruohai0925 --- internal/collector/github/legacy/constants.go | 1 + internal/collector/github/legacy/issues.go | 41 +++- .../collector/github/legacy/issues_test.go | 212 ++++++++++++++++++ 3 files changed, 252 insertions(+), 2 deletions(-) create mode 100644 internal/collector/github/legacy/issues_test.go diff --git a/internal/collector/github/legacy/constants.go b/internal/collector/github/legacy/constants.go index c29eb5b6..b5fd2ff3 100644 --- a/internal/collector/github/legacy/constants.go +++ b/internal/collector/github/legacy/constants.go @@ -32,6 +32,7 @@ const ( TooManyCommentsFrequency = 2.0 releasesPerPage = 100 + issuesPerPage = 100 ) var ErrorTooManyResults = errors.New("too many results") diff --git a/internal/collector/github/legacy/issues.go b/internal/collector/github/legacy/issues.go index e7f7e4d9..3d40a6ee 100644 --- a/internal/collector/github/legacy/issues.go +++ b/internal/collector/github/legacy/issues.go @@ -16,6 +16,7 @@ package legacy import ( "context" + "fmt" "time" "github.com/google/go-github/v47/github" @@ -36,8 +37,9 @@ const ( // // This count includes both issues and pull requests. func FetchIssueCount(ctx context.Context, c *githubapi.Client, owner, name string, state IssueState, lookback time.Duration) (int, error) { + since := time.Now().UTC().Add(-lookback) opts := &github.IssueListByRepoOptions{ - Since: time.Now().UTC().Add(-lookback), + Since: since, State: string(state), ListOptions: github.ListOptions{PerPage: 1}, // 1 result per page means LastPage is total number of records. } @@ -52,7 +54,42 @@ func FetchIssueCount(ctx context.Context, c *githubapi.Client, owner, name strin if resp.NextPage == 0 { return len(is), nil } - return resp.LastPage, nil + if resp.LastPage > 0 { + return resp.LastPage, nil + } + // GitHub has migrated this endpoint to cursor-based pagination, so the + // Link header no longer carries a rel="last" and LastPage is 0. Fall back + // to walking the pages and counting the results. + return countIssuePages(ctx, c, owner, name, state, since) +} + +// countIssuePages counts issues by paging through the results, for when the +// total cannot be read directly from the Link header. +func countIssuePages(ctx context.Context, c *githubapi.Client, owner, name string, state IssueState, since time.Time) (int, error) { + opts := &github.IssueListByRepoOptions{ + Since: since, + State: string(state), + ListOptions: github.ListOptions{PerPage: issuesPerPage, Page: 1}, + } + total := 0 + for { + is, resp, err := c.Rest().Issues.ListByRepo(ctx, owner, name, opts) + // The API returns 5xx responses if there are too many issues. + if c := githubapi.ErrorResponseStatusCode(err); 500 <= c && c < 600 { + return MaxIssuesLimit, nil + } + if err != nil { + return 0, fmt.Errorf("listing issues: %w", err) + } + total += len(is) + if total >= MaxIssuesLimit { + return MaxIssuesLimit, nil + } + if len(is) < issuesPerPage || resp.NextPage == 0 { + return total, nil + } + opts.Page = resp.NextPage + } } // FetchIssueCommentCount returns the total number of comments for a given repo diff --git a/internal/collector/github/legacy/issues_test.go b/internal/collector/github/legacy/issues_test.go new file mode 100644 index 00000000..77a4a745 --- /dev/null +++ b/internal/collector/github/legacy/issues_test.go @@ -0,0 +1,212 @@ +// Copyright 2026 Criticality Score Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package legacy_test + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + "time" + + "github.com/ossf/criticality_score/v2/internal/collector/github/legacy" + "github.com/ossf/criticality_score/v2/internal/githubapi" +) + +// newTestClient returns a githubapi.Client pointed at the given test server. +func newTestClient(t *testing.T, srv *httptest.Server) *githubapi.Client { + t.Helper() + c := githubapi.NewClient(srv.Client()) + u, err := url.Parse(srv.URL + "/") + if err != nil { + t.Fatalf("failed to parse test server url: %v", err) + } + c.Rest().BaseURL = u + return c +} + +// writeIssues writes n placeholder issues as the response body. +func writeIssues(w http.ResponseWriter, n int) { + var sb strings.Builder + sb.WriteString("[") + for i := 0; i < n; i++ { + if i > 0 { + sb.WriteString(",") + } + fmt.Fprintf(&sb, `{"number":%d}`, i+1) + } + sb.WriteString("]") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(sb.String())) +} + +func TestFetchIssueCount(t *testing.T) { + //nolint:govet // field alignment is irrelevant for a test table. + tests := []struct { + name string + // handler serves the issues endpoint. + handler func(w http.ResponseWriter, r *http.Request) + want int + }{ + { + // GitHub migrated this endpoint to cursor-based pagination: the + // Link header carries only a rel="next" with an opaque cursor, so + // LastPage is 0 and the count has to come from paging. + name: "cursor pagination without rel=last", + handler: func(w http.ResponseWriter, r *http.Request) { + perPage := r.URL.Query().Get("per_page") + page := r.URL.Query().Get("page") + if perPage == "1" { + // The initial probe request. + w.Header().Set("Link", + `; rel="next"`) + writeIssues(w, 1) + return + } + switch page { + case "", "1": + w.Header().Set("Link", + `; rel="next"`) + writeIssues(w, 100) + default: + // Final short page, no Link header. + writeIssues(w, 50) + } + }, + want: 150, + }, + { + // Older behaviour: rel="last" is present, so the total can be read + // straight off the Link header without paging. + name: "rel=last is used when present", + handler: func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("per_page") != "1" { + t.Errorf("unexpected paging request: %s", r.URL.String()) + } + w.Header().Set("Link", + `; rel="next", `+ + `; rel="last"`) + writeIssues(w, 1) + }, + want: 42, + }, + { + name: "single result", + handler: func(w http.ResponseWriter, r *http.Request) { + writeIssues(w, 1) + }, + want: 1, + }, + { + name: "no results", + handler: func(w http.ResponseWriter, r *http.Request) { + writeIssues(w, 0) + }, + want: 0, + }, + { + // The API returns 5xx when there are too many issues to count. + name: "too many issues", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + want: legacy.MaxIssuesLimit, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(test.handler)) + defer srv.Close() + + got, err := legacy.FetchIssueCount(context.Background(), newTestClient(t, srv), + "owner", "name", legacy.IssueStateAll, legacy.IssueLookback) + if err != nil { + t.Fatalf("FetchIssueCount() returned err: %v", err) + } + if got != test.want { + t.Errorf("FetchIssueCount() = %d, want %d", got, test.want) + } + }) + } +} + +// TestFetchIssueCountPagingIsCapped ensures paging stops once the limit is hit, +// rather than walking an unbounded number of pages. +func TestFetchIssueCountPagingIsCapped(t *testing.T) { + requests := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if r.URL.Query().Get("per_page") == "1" { + w.Header().Set("Link", + `; rel="next"`) + writeIssues(w, 1) + return + } + page := r.URL.Query().Get("page") + if page == "" { + page = "1" + } + n, _ := strconv.Atoi(page) + w.Header().Set("Link", fmt.Sprintf( + `; rel="next"`, n+1)) + writeIssues(w, 100) + })) + defer srv.Close() + + got, err := legacy.FetchIssueCount(context.Background(), newTestClient(t, srv), + "owner", "name", legacy.IssueStateAll, legacy.IssueLookback) + if err != nil { + t.Fatalf("FetchIssueCount() returned err: %v", err) + } + if got != legacy.MaxIssuesLimit { + t.Errorf("FetchIssueCount() = %d, want %d", got, legacy.MaxIssuesLimit) + } + // 1 probe request, plus MaxIssuesLimit/100 paging requests. + if want := 1 + legacy.MaxIssuesLimit/100; requests != want { + t.Errorf("made %d requests, want %d", requests, want) + } +} + +// TestFetchIssueCountLookback checks the since parameter is derived from the +// lookback duration. +func TestFetchIssueCountLookback(t *testing.T) { + var since string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + since = r.URL.Query().Get("since") + writeIssues(w, 0) + })) + defer srv.Close() + + lookback := 90 * 24 * time.Hour + before := time.Now().UTC().Add(-lookback) + if _, err := legacy.FetchIssueCount(context.Background(), newTestClient(t, srv), + "owner", "name", legacy.IssueStateAll, lookback); err != nil { + t.Fatalf("FetchIssueCount() returned err: %v", err) + } + after := time.Now().UTC().Add(-lookback) + + got, err := time.Parse(time.RFC3339, since) + if err != nil { + t.Fatalf("failed to parse since %q: %v", since, err) + } + if got.Before(before.Truncate(time.Second)) || got.After(after.Add(time.Second)) { + t.Errorf("since = %v, want between %v and %v", got, before, after) + } +}