diff --git a/github/client.go b/github/client.go index cbaf828..695e289 100644 --- a/github/client.go +++ b/github/client.go @@ -110,6 +110,11 @@ type Client struct { // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff. retryBackoff []time.Duration + + // reviewPageSize overrides reviewsPerPage for testing. Zero means use default. + reviewPageSize int + // reviewMaxPages overrides maxReviewPages for testing. Zero means use default. + reviewMaxPages int } // defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil). @@ -194,6 +199,13 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error { return nil } +// SetReviewPagination overrides the page size and max pages for ListReviews. +// Intended for testing only; must be called before any goroutines issue requests. +func (c *Client) SetReviewPagination(pageSize, maxPages int) { + c.reviewPageSize = pageSize + c.reviewMaxPages = maxPages +} + // requestOptions holds per-request configuration for doRequestCore. type requestOptions struct { // bodyFn returns a fresh io.Reader for the request body on each attempt. diff --git a/github/identity.go b/github/identity.go deleted file mode 100644 index 46c26a8..0000000 --- a/github/identity.go +++ /dev/null @@ -1,29 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "fmt" -) - -// userResponse is the GitHub API response for the authenticated user. -type userResponse struct { - Login string `json:"login"` -} - -// GetAuthenticatedUser returns the login of the currently authenticated user. -func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { - reqURL := fmt.Sprintf("%s/user", c.baseURL) - - body, err := c.doGet(ctx, reqURL) - if err != nil { - return "", fmt.Errorf("get authenticated user: %w", err) - } - - var resp userResponse - if err := json.Unmarshal(body, &resp); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - - return resp.Login, nil -} diff --git a/github/review_test.go b/github/review_test.go index ad14b9f..0c1e4d3 100644 --- a/github/review_test.go +++ b/github/review_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "strings" @@ -482,3 +483,195 @@ func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) { t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha") } } + +// --- ListReviews pagination tests --- + +func TestListReviews_MultiPage(t *testing.T) { + // Test multi-page pagination: 2 full pages + 1 partial page. + // pageSize=3, so pages return [3, 3, 2] reviews = 8 total. + const pageSize = 3 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("expected GET, got %s", r.Method) + } + callCount++ + + page := r.URL.Query().Get("page") + var reviews []map[string]interface{} + + switch page { + case "1": + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user1"}, + }) + } + case "2": + for i := pageSize + 1; i <= pageSize*2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "COMMENTED", "commit_id": "sha1", + "user": map[string]string{"login": "user2"}, + }) + } + case "3": + // Partial page: only 2 reviews (less than pageSize) + for i := pageSize*2 + 1; i <= pageSize*2+2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "CHANGES_REQUESTED", "commit_id": "sha1", + "user": map[string]string{"login": "user3"}, + }) + } + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 8 { + t.Fatalf("expected 8 reviews, got %d", len(reviews)) + } + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } + + // Verify reviews are correctly concatenated in order + for i, r := range reviews { + expectedID := int64(i + 1) + if r.ID != expectedID { + t.Errorf("review[%d]: expected ID %d, got %d", i, expectedID, r.ID) + } + } +} + +func TestListReviews_ExactMultipleOfPageSize(t *testing.T) { + // When total reviews is an exact multiple of pageSize, an extra request + // returning 0 results terminates the loop. No truncation warning. + const pageSize = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + var reviews []map[string]interface{} + switch page { + case "1": + reviews = []map[string]interface{}{ + {"id": 1, "body": "r1", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u1"}}, + {"id": 2, "body": "r2", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u2"}}, + } + case "2": + reviews = []map[string]interface{}{ + {"id": 3, "body": "r3", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u3"}}, + {"id": 4, "body": "r4", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u4"}}, + } + case "3": + // Empty page — signals end of data + reviews = []map[string]interface{}{} + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 4 { + t.Fatalf("expected 4 reviews, got %d", len(reviews)) + } + // 3 calls: page 1 (full), page 2 (full), page 3 (empty) + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } +} + +func TestListReviews_MaxPagesCutoff(t *testing.T) { + // When maxPages is hit and the last page is full, results are truncated + // and a warning would fire (we verify the reviews are still returned). + const pageSize = 2 + const maxPages = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + // Always return a full page (simulating more data exists) + var reviews []map[string]interface{} + var baseID int + switch page { + case "1": + baseID = 0 + case "2": + baseID = pageSize + default: + t.Fatalf("unexpected page %s (should not exceed maxPages)", page) + } + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": baseID + i, "body": fmt.Sprintf("r%d", baseID+i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user"}, + }) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, maxPages) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Should return all reviews fetched within the cap + expectedCount := pageSize * maxPages + if len(reviews) != expectedCount { + t.Fatalf("expected %d reviews, got %d", expectedCount, len(reviews)) + } + if callCount != maxPages { + t.Errorf("expected %d API calls, got %d", maxPages, callCount) + } + // Verify concatenation order + for i, r := range reviews { + if r.ID != int64(i+1) { + t.Errorf("review[%d]: expected ID %d, got %d", i, i+1, r.ID) + } + } +} + +func TestListReviews_EmptyFirstPage(t *testing.T) { + // PR with no reviews: first page returns empty array. + callCount := 0 + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + json.NewEncoder(w).Encode([]map[string]interface{}{}) + }) + c.SetReviewPagination(10, 5) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 0 { + t.Fatalf("expected 0 reviews, got %d", len(reviews)) + } + if callCount != 1 { + t.Errorf("expected 1 API call, got %d", callCount) + } +} diff --git a/github/review.go b/github/reviews.go similarity index 74% rename from github/review.go rename to github/reviews.go index 1d043e5..a49a915 100644 --- a/github/review.go +++ b/github/reviews.go @@ -5,12 +5,21 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "net/url" "gitea.weiker.me/rodin/review-bot/vcs" ) +const ( + // reviewsPerPage is the number of reviews to fetch per API page. + reviewsPerPage = 100 + // maxReviewPages is the maximum number of pages to paginate through + // when listing reviews. Acts as a safeguard against infinite pagination. + maxReviewPages = 100 +) + // ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on // a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT). // GitHub only allows deletion of PENDING reviews. Callers that need to replace @@ -54,6 +63,11 @@ type dismissReviewRequest struct { Event string `json:"event"` } +// userResponse is the GitHub API response for the authenticated user. +type userResponse struct { + Login string `json:"login"` +} + // translateGitHubReviewState translates a GitHub API review state to the // canonical vcs.Review.State value. func translateGitHubReviewState(state string) string { @@ -117,12 +131,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }) } - data, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("marshal review request: %w", err) - } - - body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) + body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload) if err != nil { return nil, fmt.Errorf("post review: %w", err) } @@ -141,33 +150,69 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }, nil } -// ListReviews retrieves all reviews for a pull request. +// ListReviews retrieves all reviews for a pull request with pagination. // GitHub review states are translated to canonical vcs values. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) - - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) + perPage := reviewsPerPage + if c.reviewPageSize > 0 { + perPage = c.reviewPageSize + } + maxPages := maxReviewPages + if c.reviewMaxPages > 0 { + maxPages = c.reviewMaxPages } - var responses []reviewResponse - if err := json.Unmarshal(body, &responses); err != nil { - return nil, fmt.Errorf("parse reviews response: %w", err) - } + var allReviews []vcs.Review + truncated := false - reviews := make([]vcs.Review, len(responses)) - for i, r := range responses { - reviews[i] = vcs.Review{ - ID: r.ID, - Body: r.Body, - User: vcs.UserInfo{Login: r.User.Login}, - State: translateGitHubReviewState(r.State), - CommitID: r.CommitID, + for page := 1; page <= maxPages; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page) + + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews page %d: %w", page, err) + } + + var responses []reviewResponse + if err := json.Unmarshal(body, &responses); err != nil { + return nil, fmt.Errorf("parse reviews response: %w", err) + } + + if len(responses) == 0 { + break + } + + for _, r := range responses { + allReviews = append(allReviews, vcs.Review{ + ID: r.ID, + Body: r.Body, + User: vcs.UserInfo{Login: r.User.Login}, + State: translateGitHubReviewState(r.State), + CommitID: r.CommitID, + }) + } + + if len(responses) < perPage { + break + } + + // Truncation detection: this runs on the final allowed iteration + // (page == maxPages) only when the page was full (the len < perPage + // early-break above didn't fire). A full final page means additional + // reviews likely exist beyond our pagination limit. + if page == maxPages { + truncated = true } } - return reviews, nil + + if truncated { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxPages, "reviewsFetched", len(allReviews)) + } + + return allReviews, nil } // DeleteReview deletes a pull request review. @@ -204,12 +249,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i Event: "DISMISS", } - data, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("marshal dismiss request: %w", err) - } - - _, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data) + _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) if err != nil { return fmt.Errorf("dismiss review: %w", err) } @@ -228,3 +268,17 @@ func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNum } return errors.Join(errs...) } + +// GetAuthenticatedUser returns the login name of the authenticated user. +func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { + reqURL := fmt.Sprintf("%s/user", c.baseURL) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("get authenticated user: %w", err) + } + var resp userResponse + if err := json.Unmarshal(body, &resp); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + return resp.Login, nil +}