From 816d9307c6c71ed59ce7b037d04e789d32efe22e Mon Sep 17 00:00:00 2001 From: Muyu Date: Tue, 7 Jul 2026 21:57:34 +0800 Subject: [PATCH] fix github issue pagination loop --- pkg/github/client.go | 15 +++++- pkg/github/client_test.go | 105 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/pkg/github/client.go b/pkg/github/client.go index 3647b98..8ef9083 100644 --- a/pkg/github/client.go +++ b/pkg/github/client.go @@ -28,15 +28,28 @@ func (f *IssueFetcher) FetchIssues(ctx context.Context, owner, repo string, sinc } var allIssues []*github.Issue + seenIssueIDs := make(map[int64]struct{}) for { issues, resp, err := f.client.Issues.ListByRepository(ctx, owner, repo, opt) if err != nil { return nil, fmt.Errorf("failed to list issues: %w", err) } - allIssues = append(allIssues, issues...) + for _, issue := range issues { + id := issue.GetID() + if id != 0 { + if _, ok := seenIssueIDs[id]; ok { + continue + } + seenIssueIDs[id] = struct{}{} + } + allIssues = append(allIssues, issue) + } if resp.NextPage == 0 { break } + if resp.NextPage <= opt.Page { + return nil, fmt.Errorf("pagination did not advance: current page %d, next page %d", opt.Page, resp.NextPage) + } opt.Page = resp.NextPage } return allIssues, nil diff --git a/pkg/github/client_test.go b/pkg/github/client_test.go index 884b946..ea07119 100644 --- a/pkg/github/client_test.go +++ b/pkg/github/client_test.go @@ -176,6 +176,111 @@ func TestFetchIssues_EmptyPageWithNext(t *testing.T) { } } +func TestFetchIssues_DetectsNonAdvancingPaginationWithSince(t *testing.T) { + sinceTime := time.Now().Add(-24 * time.Hour).Truncate(time.Second) + sinceStr := sinceTime.Format(time.RFC3339) + requests := 0 + + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > 2 { + t.Fatal("pagination loop was not terminated") + } + + q := r.URL.Query() + if q.Get("since") != sinceStr { + t.Errorf("expected since parameter %q, got %q", sinceStr, q.Get("since")) + } + + issues := []*github.Issue{ + {ID: github.Int64(1), Title: github.String("Issue 1")}, + } + + linkHeader := fmt.Sprintf("<%s?page=1&since=%s>; rel=\"next\"", server.URL+r.URL.Path, url.QueryEscape(sinceStr)) + w.Header().Set("Link", linkHeader) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(issues) + })) + defer server.Close() + + httpClient := server.Client() + client := github.NewClient(httpClient) + baseURL, _ := url.Parse(server.URL + "/") + client.BaseURL = baseURL + + fetcher := NewIssueFetcher(client) + _, err := fetcher.FetchIssues(context.Background(), "owner", "repo", sinceTime, 2) + if err == nil { + t.Fatal("expected pagination loop error, got nil") + } + + if !strings.Contains(err.Error(), "pagination did not advance") { + t.Errorf("expected pagination loop error, got: %v", err) + } + if requests != 2 { + t.Errorf("expected 2 requests before loop detection, got %d", requests) + } +} + +func TestFetchIssues_DeduplicatesIssuesAcrossPages(t *testing.T) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + pageStr := q.Get("page") + page := 1 + if pageStr != "" { + var err error + page, err = strconv.Atoi(pageStr) + if err != nil { + t.Fatalf("invalid page parameter: %v", err) + } + } + + var issues []*github.Issue + switch page { + case 1: + issues = []*github.Issue{ + {ID: github.Int64(1), Title: github.String("Issue 1")}, + {ID: github.Int64(2), Title: github.String("Issue 2")}, + } + w.Header().Set("Link", fmt.Sprintf("<%s?page=2>; rel=\"next\"", server.URL+r.URL.Path)) + case 2: + issues = []*github.Issue{ + {ID: github.Int64(2), Title: github.String("Issue 2 duplicate")}, + {ID: github.Int64(3), Title: github.String("Issue 3")}, + } + default: + t.Fatalf("unexpected page request: %d", page) + } + + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(issues) + })) + defer server.Close() + + httpClient := server.Client() + client := github.NewClient(httpClient) + baseURL, _ := url.Parse(server.URL + "/") + client.BaseURL = baseURL + + fetcher := NewIssueFetcher(client) + issues, err := fetcher.FetchIssues(context.Background(), "owner", "repo", time.Now(), 2) + if err != nil { + t.Fatalf("FetchIssues failed: %v", err) + } + + expectedIDs := []int64{1, 2, 3} + if len(issues) != len(expectedIDs) { + t.Fatalf("expected %d issues, got %d", len(expectedIDs), len(issues)) + } + for i, issue := range issues { + if issue.GetID() != expectedIDs[i] { + t.Errorf("at index %d: expected ID %d, got %d", i, expectedIDs[i], issue.GetID()) + } + } +} + func TestFetchIssues_Integration(t *testing.T) { token := os.Getenv("GITHUB_TOKEN") repoFullName := os.Getenv("GITHUB_REPOSITORY")