From 55cf6c6a64cb5f32d03ae849685a83076f5db4a3 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 12 Jun 2026 15:29:29 +0200 Subject: [PATCH] Fix pagination prefix duplication on GHE with redirect Determine the GHE path prefix by matching the Link header URL host against c.bases, instead of inferring it from path suffix matching which breaks when a redirect changes the response URL path. --- pkg/github/client.go | 23 ++++++++------ pkg/github/client_test.go | 63 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 9 deletions(-) diff --git a/pkg/github/client.go b/pkg/github/client.go index e328f9e66..765dc43da 100644 --- a/pkg/github/client.go +++ b/pkg/github/client.go @@ -2028,19 +2028,24 @@ func (c *client) readPaginatedResultsWithValuesWithContext(ctx context.Context, // * next: /api/v3/repositories/22/pulls?per_page=100&page=2 // * prefix will be "/api/v3" and we strip it so we don't duplicate it // when prepending c.bases[hostIndex] - // Example for a redirect (e.g. repo rename): - // * initial call: api.github.com/repos/old-org/old-repo/pulls?per_page=100 - // * resp.Request.URL (after redirect): api.github.com/repos/new-org/new-repo/pulls?per_page=100 - // * next: api.github.com/repos/new-org/new-repo/pulls?per_page=100&page=2 - // * prefix will be empty; we compare only Path (not full RequestURI) so - // the differing response URL doesn't break the suffix match - pathOnly := strings.SplitN(pagedPath, "?", 2)[0] - prefix := strings.TrimSuffix(resp.Request.URL.Path, pathOnly) - + // Example for a redirect (e.g. repo rename), with or without GHE: + // * initial call: /repos/old-org/old-repo/pulls?per_page=100 + // * resp.Request.URL (after redirect): /repos/new-org/new-repo/pulls?per_page=100 + // * next: /repos/new-org/new-repo/pulls?per_page=100&page=2 + // * prefix is determined from c.bases by matching the Link host, so + // redirects that change the path don't affect prefix detection u, err := url.Parse(link) if err != nil { return fmt.Errorf("failed to parse 'next' link: %w", err) } + var prefix string + for _, base := range c.bases { + baseURL, err := url.Parse(base) + if err == nil && baseURL.Host == u.Host { + prefix = baseURL.Path + break + } + } pagedPath = strings.TrimPrefix(u.RequestURI(), prefix) if len(pagedPath) == 0 || pagedPath[0] != '/' { pagedPath = u.RequestURI() diff --git a/pkg/github/client_test.go b/pkg/github/client_test.go index 63dff37a7..68d557395 100644 --- a/pkg/github/client_test.go +++ b/pkg/github/client_test.go @@ -1492,6 +1492,69 @@ func TestReadPaginatedResultsWithRedirect(t *testing.T) { } } +func TestReadPaginatedResultsWithGHEAndRedirect(t *testing.T) { + requestCount := 0 + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + switch r.URL.Path { + case "/api/v3/repos/old-org/old-repo/branches": + newURL := fmt.Sprintf("https://%s/api/v3/repos/new-org/new-repo/branches", r.Host) + if r.URL.RawQuery != "" { + newURL += "?" + r.URL.RawQuery + } + http.Redirect(w, r, newURL, http.StatusMovedPermanently) + case "/api/v3/repos/new-org/new-repo/branches": + var labels []Label + if r.URL.Query().Get("page") == "" { + labels = []Label{{Name: "page1"}} + w.Header().Set("Link", fmt.Sprintf( + `; rel="next"`, + r.Host, r.URL.RawQuery)) + } else { + labels = []Label{{Name: "page2"}} + } + b, err := json.Marshal(labels) + if err != nil { + t.Errorf("Failed to marshal: %v", err) + } + fmt.Fprint(w, string(b)) + default: + t.Errorf("Unexpected request path: %s (full: %s)", r.URL.Path, r.URL.String()) + http.Error(w, "not found", http.StatusNotFound) + } + })) + defer ts.Close() + + c := getClient(ts.URL) + c.bases[0] = c.bases[0] + "/api/v3" + var labels []Label + err := c.readPaginatedResultsWithValues( + "/repos/old-org/old-repo/branches", + url.Values{ + "per_page": []string{"100"}, + "protected": []string{"false"}, + }, + "", + "", + func() interface{} { + return &[]Label{} + }, + func(obj interface{}) { + labels = append(labels, *(obj.(*[]Label))...) + }, + ) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + expected := []Label{{Name: "page1"}, {Name: "page2"}} + if !reflect.DeepEqual(labels, expected) { + t.Errorf("Expected %v, got %v", expected, labels) + } + if requestCount != 3 { + t.Errorf("Expected 3 requests (redirect + page1 + page2), got %d", requestCount) + } +} + func TestListPullRequestComments(t *testing.T) { ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet {