From e2e5d4514fb7271cc57440926f9ed577f23b0346 Mon Sep 17 00:00:00 2001 From: Vadym Date: Thu, 23 Jul 2026 15:12:25 +0200 Subject: [PATCH] fix(repo): stop List pagination early once Limit is reached Backend Repos().List implementations (GitHub, GitLab, Gitea, Bitbucket) previously ignored ListRepoOpts.Limit entirely and always fetched every page of results before returning, even when the caller only asked for a handful of repositories via `forge repo list --limit N`. PR #135 worked around this by capping the CLI's printed output after the fact, but the backends still made all the underlying API requests, wasting rate-limit budget and latency for large orgs/users, exactly as described in #134. This closes the gap called out in #134's "Proposed changes": - Add forge.RepoMatchesFilters, a shared archived/fork predicate reused by both the pagination loops and the existing FilterRepos post-filter. - Add forge.CapRepos to truncate matched results to opts.Limit. - In each backend's Repos().List, count filter-matching repos while paginating and stop requesting further pages once opts.Limit matching repos have been collected. Also shrink the requested page size to the limit when it's smaller, so a single small page is fetched instead of a full default page. - Bitbucket's cursor-based pagination (follows a `next` URL) is handled the same way: stop following `next` once enough repos are collected. No behavior change when Limit is 0 (unlimited); all backends still paginate fully in that case, same as before. Tested: - go build ./... - go vet ./... - go test ./... (all existing tests pass unchanged) - Added TestGitHubListReposStopsPaginatingAtLimit, TestGitLabListReposStopsPaginatingAtLimit, TestGiteaListReposStopsPaginatingAtLimit, TestBitbucketListReposStopsPaginatingAtLimit; each asserts the backend issues exactly one HTTP request when Limit is satisfied by the first page, even though the mock server always advertises a next page. Fixes #134 --- bitbucket/bitbucket.go | 15 ++++++++-- bitbucket/bitbucket_test.go | 45 +++++++++++++++++++++++++++++ forge.go | 57 ++++++++++++++++++++++++------------- gitea/gitea.go | 33 ++++++++++++++++----- gitea/gitea_test.go | 35 +++++++++++++++++++++++ github/github.go | 33 ++++++++++++++++----- github/github_test.go | 37 ++++++++++++++++++++++++ gitlab/gitlab.go | 33 ++++++++++++++++----- gitlab/gitlab_test.go | 40 ++++++++++++++++++++++++++ 9 files changed, 286 insertions(+), 42 deletions(-) diff --git a/bitbucket/bitbucket.go b/bitbucket/bitbucket.go index 549b9c1..68a96d6 100644 --- a/bitbucket/bitbucket.go +++ b/bitbucket/bitbucket.go @@ -216,8 +216,12 @@ func (s *bitbucketRepoService) List(ctx context.Context, owner string, opts forg if perPage <= 0 { perPage = 100 } + if opts.Limit > 0 && perPage > opts.Limit { + perPage = opts.Limit + } var all []forge.Repository + matched := 0 url := fmt.Sprintf("%s/repositories/%s?pagelen=%d", bitbucketAPI, owner, perPage) for url != "" { @@ -229,12 +233,19 @@ func (s *bitbucketRepoService) List(ctx context.Context, owner string, opts forg return nil, err } for _, bb := range page.Values { - all = append(all, convertBitbucketRepo(bb)) + repo := convertBitbucketRepo(bb) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } url = page.Next } - return forge.FilterRepos(all, opts), nil + return forge.CapRepos(forge.FilterRepos(all, opts), opts), nil } func (s *bitbucketRepoService) Create(ctx context.Context, opts forge.CreateRepoOpts) (*forge.Repository, error) { diff --git a/bitbucket/bitbucket_test.go b/bitbucket/bitbucket_test.go index b99f683..c1b3433 100644 --- a/bitbucket/bitbucket_test.go +++ b/bitbucket/bitbucket_test.go @@ -3,6 +3,7 @@ package bitbucket import ( "context" "encoding/json" + "fmt" forge "github.com/git-pkgs/forge" "net/http" "net/http/httptest" @@ -211,3 +212,47 @@ func TestBitbucketListTags(t *testing.T) { assertEqual(t, "Tag[1].Name", "v0.1.0", tags[1].Name) assertEqual(t, "Tag[1].Commit", "fff666", tags[1].Commit) } + +func TestBitbucketListReposStopsPaginatingAtLimit(t *testing.T) { + var requests int + mux := http.NewServeMux() + mux.HandleFunc("GET /2.0/repositories/atlassian", func(w http.ResponseWriter, r *http.Request) { + requests++ + n := requests + _ = json.NewEncoder(w).Encode(map[string]any{ + "values": []bbRepository{ + { + Slug: "repo", + FullName: "atlassian/repo", + Owner: &struct { + Username string `json:"username"` + DisplayName string `json:"display_name"` + }{Username: "atlassian"}, + }, + }, + // Always advertise a next page so the test fails loudly if the + // implementation keeps following it past the requested limit. + "next": fmt.Sprintf("%s?page=%d", r.URL.Path, n+1), + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + origAPI := bitbucketAPI + defer func() { setBitbucketAPI(origAPI) }() + setBitbucketAPI(srv.URL + "/2.0") + + f := New("test-token", nil) + + repos, err := f.Repos().List(context.Background(), "atlassian", forge.ListRepoOpts{Limit: 1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(repos)) + } + if requests != 1 { + t.Fatalf("expected List to stop after 1 request once Limit was reached, got %d requests", requests) + } +} diff --git a/forge.go b/forge.go index 6d46242..a687302 100644 --- a/forge.go +++ b/forge.go @@ -219,29 +219,39 @@ func (c *Client) ListRepositories(ctx context.Context, domain, owner string, opt return f.Repos().List(ctx, owner, opts) } +// RepoMatchesFilters reports whether a repository satisfies the archived and +// fork filters requested in opts. Backends can use this to decide, while +// paginating, whether a repository counts toward opts.Limit. +func RepoMatchesFilters(r Repository, opts ListRepoOpts) bool { + switch opts.Archived { + case ArchivedExclude: + if r.Archived { + return false + } + case ArchivedOnly: + if !r.Archived { + return false + } + } + switch opts.Forks { + case ForkExclude: + if r.Fork { + return false + } + case ForkOnly: + if !r.Fork { + return false + } + } + return true +} + // FilterRepos applies archived and fork filters to a slice of repositories. func FilterRepos(repos []Repository, opts ListRepoOpts) []Repository { n := 0 for _, r := range repos { - switch opts.Archived { - case ArchivedExclude: - if r.Archived { - continue - } - case ArchivedOnly: - if !r.Archived { - continue - } - } - switch opts.Forks { - case ForkExclude: - if r.Fork { - continue - } - case ForkOnly: - if !r.Fork { - continue - } + if !RepoMatchesFilters(r, opts) { + continue } repos[n] = r n++ @@ -249,6 +259,15 @@ func FilterRepos(repos []Repository, opts ListRepoOpts) []Repository { return repos[:n] } +// CapRepos truncates repos to at most opts.Limit entries. A Limit of 0 or +// less means unlimited and the slice is returned unchanged. +func CapRepos(repos []Repository, opts ListRepoOpts) []Repository { + if opts.Limit > 0 && len(repos) > opts.Limit { + return repos[:opts.Limit] + } + return repos +} + // FetchTagsFromPURL fetches git tags using a PURL's repository_url qualifier. func (c *Client) FetchTagsFromPURL(ctx context.Context, p *purl.PURL) ([]Tag, error) { repoURL := p.RepositoryURL() diff --git a/gitea/gitea.go b/gitea/gitea.go index 5f450a0..c970f77 100644 --- a/gitea/gitea.go +++ b/gitea/gitea.go @@ -106,21 +106,25 @@ func (s *giteaRepoService) Get(ctx context.Context, owner, repo string) (*forge. func (s *giteaRepoService) List(ctx context.Context, owner string, opts forge.ListRepoOpts) ([]forge.Repository, error) { perPage := pageSize(opts.PerPage) + if opts.Limit > 0 && opts.Limit < perPage { + perPage = pageSize(opts.Limit) + } // Try org endpoint first, fall back to user on 404. - repos, err := s.listOrgRepos(ctx, owner, perPage) + repos, err := s.listOrgRepos(ctx, owner, perPage, opts) if err != nil { - repos, err = s.listUserRepos(ctx, owner, perPage) + repos, err = s.listUserRepos(ctx, owner, perPage, opts) if err != nil { return nil, err } } - return forge.FilterRepos(repos, opts), nil + return forge.CapRepos(forge.FilterRepos(repos, opts), opts), nil } -func (s *giteaRepoService) listOrgRepos(_ context.Context, owner string, perPage int) ([]forge.Repository, error) { +func (s *giteaRepoService) listOrgRepos(_ context.Context, owner string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 page := 1 for { gRepos, resp, err := s.client.ListOrgRepos(owner, gitea.ListOrgReposOptions{ @@ -133,7 +137,14 @@ func (s *giteaRepoService) listOrgRepos(_ context.Context, owner string, perPage return nil, err } for _, r := range gRepos { - all = append(all, convertGiteaRepo(r)) + repo := convertGiteaRepo(r) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if lastPage(resp, len(gRepos), perPage) { break @@ -143,8 +154,9 @@ func (s *giteaRepoService) listOrgRepos(_ context.Context, owner string, perPage return all, nil } -func (s *giteaRepoService) listUserRepos(_ context.Context, owner string, perPage int) ([]forge.Repository, error) { +func (s *giteaRepoService) listUserRepos(_ context.Context, owner string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 page := 1 for { gRepos, resp, err := s.client.ListUserRepos(owner, gitea.ListReposOptions{ @@ -157,7 +169,14 @@ func (s *giteaRepoService) listUserRepos(_ context.Context, owner string, perPag return nil, err } for _, r := range gRepos { - all = append(all, convertGiteaRepo(r)) + repo := convertGiteaRepo(r) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if lastPage(resp, len(gRepos), perPage) { break diff --git a/gitea/gitea_test.go b/gitea/gitea_test.go index 9320bdd..528241c 100644 --- a/gitea/gitea_test.go +++ b/gitea/gitea_test.go @@ -319,3 +319,38 @@ func TestGiteaListTags(t *testing.T) { assertEqual(t, "Tag[1].Name", "v2.0.0", tags[1].Name) assertEqual(t, "Tag[1].Commit", "ddd444", tags[1].Commit) } + +func TestGiteaListReposStopsPaginatingAtLimit(t *testing.T) { + var requests int + mux := http.NewServeMux() + mux.HandleFunc("GET /api/v1/version", giteaVersionHandler) + mux.HandleFunc("GET /api/v1/orgs/testorg/repos", func(w http.ResponseWriter, r *http.Request) { + requests++ + page := 1 + _, _ = fmt.Sscan(r.URL.Query().Get("page"), &page) + + w.Header().Set("Link", fmt.Sprintf(`; rel="first", ; rel="next"`, page+1)) + _ = json.NewEncoder(w).Encode([]map[string]any{ + { + "full_name": fmt.Sprintf("testorg/repo-%d", page), + "name": fmt.Sprintf("repo-%d", page), + "owner": map[string]any{"login": "testorg"}, + }, + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + f := New(srv.URL, "", nil) + repos, err := f.Repos().List(context.Background(), "testorg", forge.ListRepoOpts{Limit: 1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(repos)) + } + if requests != 1 { + t.Fatalf("expected List to stop after 1 request once Limit was reached, got %d requests", requests) + } +} diff --git a/github/github.go b/github/github.go index 1cdabd0..b272b67 100644 --- a/github/github.go +++ b/github/github.go @@ -113,21 +113,25 @@ func (s *gitHubRepoService) List(ctx context.Context, owner string, opts forge.L if perPage <= 0 { perPage = defaultPageSize } + if opts.Limit > 0 && perPage > opts.Limit { + perPage = opts.Limit + } // Try org endpoint first, fall back to user on 404. - repos, err := s.listOrgRepos(ctx, owner, perPage) + repos, err := s.listOrgRepos(ctx, owner, perPage, opts) if err != nil { - repos, err = s.listUserRepos(ctx, owner, perPage) + repos, err = s.listUserRepos(ctx, owner, perPage, opts) if err != nil { return nil, err } } - return forge.FilterRepos(repos, opts), nil + return forge.CapRepos(forge.FilterRepos(repos, opts), opts), nil } -func (s *gitHubRepoService) listOrgRepos(ctx context.Context, owner string, perPage int) ([]forge.Repository, error) { +func (s *gitHubRepoService) listOrgRepos(ctx context.Context, owner string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 ghOpts := &github.RepositoryListByOrgOptions{ ListOptions: github.ListOptions{PerPage: perPage}, } @@ -140,7 +144,14 @@ func (s *gitHubRepoService) listOrgRepos(ctx context.Context, owner string, perP return nil, err } for _, r := range ghRepos { - all = append(all, convertGitHubRepo(r)) + repo := convertGitHubRepo(r) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if resp.NextPage == 0 { break @@ -150,8 +161,9 @@ func (s *gitHubRepoService) listOrgRepos(ctx context.Context, owner string, perP return all, nil } -func (s *gitHubRepoService) listUserRepos(ctx context.Context, owner string, perPage int) ([]forge.Repository, error) { +func (s *gitHubRepoService) listUserRepos(ctx context.Context, owner string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 ghOpts := &github.RepositoryListByUserOptions{ ListOptions: github.ListOptions{PerPage: perPage}, } @@ -164,7 +176,14 @@ func (s *gitHubRepoService) listUserRepos(ctx context.Context, owner string, per return nil, err } for _, r := range ghRepos { - all = append(all, convertGitHubRepo(r)) + repo := convertGitHubRepo(r) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if resp.NextPage == 0 { break diff --git a/github/github_test.go b/github/github_test.go index d536c82..fcbc326 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -344,3 +344,40 @@ func TestGitHubDeleteRepoNotFound(t *testing.T) { t.Fatalf("expected forge.ErrNotFound, got %v", err) } } + +func TestGitHubListReposStopsPaginatingAtLimit(t *testing.T) { + var requests int + mux := http.NewServeMux() + mux.HandleFunc("GET /api/v3/orgs/myorg/repos", func(w http.ResponseWriter, r *http.Request) { + requests++ + page := r.URL.Query().Get("page") + if page == "" { + page = "1" + } + w.Header().Set("Link", `; rel="next"`) + _ = json.NewEncoder(w).Encode([]*github.Repository{ + { + FullName: ptr("myorg/repo-" + page), + Name: ptr("repo-" + page), + Owner: &github.User{Login: ptr("myorg")}, + Archived: ptrBool(false), + Fork: ptrBool(false), + }, + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + s := newTestGitHubRepoService(srv) + repos, err := s.List(context.Background(), "myorg", forge.ListRepoOpts{Limit: 1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(repos)) + } + if requests != 1 { + t.Fatalf("expected List to stop after 1 request once Limit was reached, got %d requests", requests) + } +} diff --git a/gitlab/gitlab.go b/gitlab/gitlab.go index 37a6d9f..288476e 100644 --- a/gitlab/gitlab.go +++ b/gitlab/gitlab.go @@ -104,21 +104,25 @@ func (s *gitLabRepoService) List(ctx context.Context, owner string, opts forge.L if perPage <= 0 { perPage = 100 } + if opts.Limit > 0 && perPage > opts.Limit { + perPage = opts.Limit + } // Try group endpoint first, fall back to user projects on 404. - repos, err := s.listGroupProjects(ctx, owner, perPage) + repos, err := s.listGroupProjects(ctx, owner, perPage, opts) if err != nil { - repos, err = s.listUserProjects(ctx, owner, perPage) + repos, err = s.listUserProjects(ctx, owner, perPage, opts) if err != nil { return nil, err } } - return forge.FilterRepos(repos, opts), nil + return forge.CapRepos(forge.FilterRepos(repos, opts), opts), nil } -func (s *gitLabRepoService) listGroupProjects(_ context.Context, group string, perPage int) ([]forge.Repository, error) { +func (s *gitLabRepoService) listGroupProjects(_ context.Context, group string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 glOpts := &gitlab.ListGroupProjectsOptions{ ListOptions: gitlab.ListOptions{PerPage: int64(perPage)}, } @@ -131,7 +135,14 @@ func (s *gitLabRepoService) listGroupProjects(_ context.Context, group string, p return nil, err } for _, p := range projects { - all = append(all, convertGitLabProject(p)) + repo := convertGitLabProject(p) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if resp.NextPage == 0 { break @@ -141,8 +152,9 @@ func (s *gitLabRepoService) listGroupProjects(_ context.Context, group string, p return all, nil } -func (s *gitLabRepoService) listUserProjects(_ context.Context, user string, perPage int) ([]forge.Repository, error) { +func (s *gitLabRepoService) listUserProjects(_ context.Context, user string, perPage int, opts forge.ListRepoOpts) ([]forge.Repository, error) { var all []forge.Repository + matched := 0 glOpts := &gitlab.ListProjectsOptions{ ListOptions: gitlab.ListOptions{PerPage: int64(perPage)}, } @@ -155,7 +167,14 @@ func (s *gitLabRepoService) listUserProjects(_ context.Context, user string, per return nil, err } for _, p := range projects { - all = append(all, convertGitLabProject(p)) + repo := convertGitLabProject(p) + all = append(all, repo) + if forge.RepoMatchesFilters(repo, opts) { + matched++ + } + } + if opts.Limit > 0 && matched >= opts.Limit { + break } if resp.NextPage == 0 { break diff --git a/gitlab/gitlab_test.go b/gitlab/gitlab_test.go index 0513a9a..a033843 100644 --- a/gitlab/gitlab_test.go +++ b/gitlab/gitlab_test.go @@ -203,3 +203,43 @@ func TestGitLabListTags(t *testing.T) { assertEqual(t, "Tag[1].Name", "v1.0.0", tags[1].Name) assertEqual(t, "Tag[1].Commit", "bbb222", tags[1].Commit) } + +func TestGitLabListReposStopsPaginatingAtLimit(t *testing.T) { + var requests int + mux := http.NewServeMux() + mux.HandleFunc("GET /api/v4/groups/mygroup/projects", func(w http.ResponseWriter, r *http.Request) { + requests++ + page := r.URL.Query().Get("page") + if page == "" { + page = "1" + } + w.Header().Set("X-Next-Page", "2") + w.Header().Set("X-Page", page) + _ = json.NewEncoder(w).Encode([]map[string]any{ + { + "path_with_namespace": "mygroup/project-" + page, + "name": "project-" + page, + "default_branch": "main", + "archived": false, + "visibility": "public", + "namespace": map[string]any{"path": "mygroup"}, + }, + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + f := New(srv.URL, "test-token", nil) + + repos, err := f.Repos().List(context.Background(), "mygroup", forge.ListRepoOpts{Limit: 1}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(repos)) + } + if requests != 1 { + t.Fatalf("expected List to stop after 1 request once Limit was reached, got %d requests", requests) + } +}