From 3be1f62f523afd67582874f9acd14728f027b20b Mon Sep 17 00:00:00 2001 From: Ellis Givens Date: Mon, 17 Aug 2026 20:17:10 -0500 Subject: [PATCH] Complete box pagination for --all --- internal/cmd/box.go | 20 +++++----- internal/cmd/box_test.go | 81 +++++++++++++++++++++++++++++++++++----- 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/internal/cmd/box.go b/internal/cmd/box.go index 9721ae0e..e12db150 100644 --- a/internal/cmd/box.go +++ b/internal/cmd/box.go @@ -14,8 +14,6 @@ import ( "github.com/basecamp/hey-cli/internal/output" ) -const maxAdditionalPages = 100 - type boxCommand struct { cmd *cobra.Command limit int @@ -81,7 +79,7 @@ func (c *boxCommand) run(cmd *cobra.Command, args []string) error { } else { resp.NextHistoryUrl = finalNextURL } - notice := boxTruncationNotice(len(postings), total, hasMore, c.all) + notice := boxTruncationNotice(len(postings), total, hasMore) if writer.IsStyled() { fmt.Fprintf(cmd.OutOrStdout(), "Box: %s (%s)\n\n", resp.Name, resp.Kind) @@ -255,15 +253,18 @@ func paginateBoxPostings(ctx context.Context, firstPage *generated.BoxShowRespon return nil, "", fmt.Errorf("paginateBoxPostings: fetch function is nil while pagination is required") } - for page := 1; page <= maxAdditionalPages && nextURL != ""; page++ { + seenPageURLs := make(map[string]struct{}) + for nextURL != "" { + if _, seen := seenPageURLs[nextURL]; seen { + return nil, "", fmt.Errorf("box pagination loop detected") + } + seenPageURLs[nextURL] = struct{}{} + resp, err := fetch(ctx, nextURL) if err != nil { return nil, "", err } nextURL = resp.NextHistoryUrl - if len(resp.Postings) == 0 { - break - } postings = append(postings, resp.Postings...) if !all && limit > 0 && len(postings) >= limit { @@ -275,13 +276,10 @@ func paginateBoxPostings(ctx context.Context, firstPage *generated.BoxShowRespon } // boxTruncationNotice returns a user-facing notice about truncated or paginated results. -func boxTruncationNotice(shown, fetched int, hasMore, all bool) string { +func boxTruncationNotice(shown, fetched int, hasMore bool) string { if shown < fetched { return fmt.Sprintf("Showing %d of %d results. Use --all to see everything.", shown, fetched) } - if hasMore && all { - return fmt.Sprintf("Showing %d results. Pagination limit reached; not all results could be fetched.", shown) - } if hasMore { return fmt.Sprintf("Showing %d results. More available; use --all to fetch all.", shown) } diff --git a/internal/cmd/box_test.go b/internal/cmd/box_test.go index b72efc19..f90b5a53 100644 --- a/internal/cmd/box_test.go +++ b/internal/cmd/box_test.go @@ -140,6 +140,49 @@ func TestPaginateBoxPostings_AllFlag(t *testing.T) { } } +func TestPaginateBoxPostings_AllFlagContinuesPastPreviousPageLimit(t *testing.T) { + first := &generated.BoxShowResponse{ + Postings: makePostings(30, 0), + NextHistoryUrl: "https://app.hey.com/page2", + } + pages := make([]generated.BoxShowResponse, 101) + for i := range pages { + pages[i].Postings = makePostings(10, 30+i*10) + if i < len(pages)-1 { + pages[i].NextHistoryUrl = fmt.Sprintf("https://app.hey.com/page%d", i+3) + } + } + + postings, nextURL, err := paginateBoxPostings(context.Background(), first, 0, true, mockFetcher(pages)) + if err != nil { + t.Fatal(err) + } + if len(postings) != 1040 { + t.Errorf("expected 1040 postings, got %d", len(postings)) + } + if nextURL != "" { + t.Errorf("expected empty nextURL when last page has no next URL, got %q", nextURL) + } +} + +func TestPaginateBoxPostings_RejectsPaginationLoop(t *testing.T) { + first := &generated.BoxShowResponse{ + Postings: makePostings(30, 0), + NextHistoryUrl: "https://app.hey.com/page2", + } + pages := []generated.BoxShowResponse{ + {Postings: makePostings(10, 30), NextHistoryUrl: "https://app.hey.com/page2"}, + } + + _, _, err := paginateBoxPostings(context.Background(), first, 0, true, mockFetcher(pages)) + if err == nil { + t.Fatal("expected pagination loop error") + } + if !strings.Contains(err.Error(), "pagination loop") { + t.Errorf("error = %q, want pagination loop error", err) + } +} + func TestPaginateBoxPostings_LimitExceedsFirstPage(t *testing.T) { first := &generated.BoxShowResponse{ Postings: makePostings(30, 0), @@ -196,7 +239,7 @@ func TestPaginateBoxPostings_NoNextURL(t *testing.T) { } } -func TestPaginateBoxPostings_EmptyPageStopsPagination(t *testing.T) { +func TestPaginateBoxPostings_EmptyFinalPageStopsPagination(t *testing.T) { first := &generated.BoxShowResponse{ Postings: makePostings(30, 0), NextHistoryUrl: "https://app.hey.com/page2", @@ -217,6 +260,28 @@ func TestPaginateBoxPostings_EmptyPageStopsPagination(t *testing.T) { } } +func TestPaginateBoxPostings_EmptyPageWithNextURLContinuesPagination(t *testing.T) { + first := &generated.BoxShowResponse{ + Postings: makePostings(30, 0), + NextHistoryUrl: "https://app.hey.com/page2", + } + pages := []generated.BoxShowResponse{ + {NextHistoryUrl: "https://app.hey.com/page3"}, + {Postings: makePostings(10, 30)}, + } + + postings, nextURL, err := paginateBoxPostings(context.Background(), first, 0, true, mockFetcher(pages)) + if err != nil { + t.Fatal(err) + } + if len(postings) != 40 { + t.Errorf("expected 40 postings, got %d", len(postings)) + } + if nextURL != "" { + t.Errorf("expected empty nextURL when last page has no next URL, got %q", nextURL) + } +} + func TestPaginateBoxPostings_NilFetchReturnsError(t *testing.T) { first := &generated.BoxShowResponse{ Postings: makePostings(30, 0), @@ -257,20 +322,18 @@ func TestBoxTruncationNotice(t *testing.T) { shown int fetched int hasMore bool - all bool want string }{ - {"client truncated", 10, 30, false, false, "Showing 10 of 30 results. Use --all to see everything."}, - {"more pages available", 30, 30, true, false, "Showing 30 results. More available; use --all to fetch all."}, - {"all shown no more", 30, 30, false, false, ""}, - {"truncated with more", 10, 30, true, false, "Showing 10 of 30 results. Use --all to see everything."}, - {"all flag pagination capped", 30, 30, true, true, "Showing 30 results. Pagination limit reached; not all results could be fetched."}, + {"client truncated", 10, 30, false, "Showing 10 of 30 results. Use --all to see everything."}, + {"more pages available", 30, 30, true, "Showing 30 results. More available; use --all to fetch all."}, + {"all shown no more", 30, 30, false, ""}, + {"truncated with more", 10, 30, true, "Showing 10 of 30 results. Use --all to see everything."}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := boxTruncationNotice(tt.shown, tt.fetched, tt.hasMore, tt.all) + got := boxTruncationNotice(tt.shown, tt.fetched, tt.hasMore) if got != tt.want { - t.Errorf("boxTruncationNotice(%d, %d, %v, %v) = %q, want %q", tt.shown, tt.fetched, tt.hasMore, tt.all, got, tt.want) + t.Errorf("boxTruncationNotice(%d, %d, %v) = %q, want %q", tt.shown, tt.fetched, tt.hasMore, got, tt.want) } }) }