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.go b/github/reviews.go similarity index 76% rename from github/review.go rename to github/reviews.go index 467f890..b365171 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 { @@ -112,12 +126,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) } @@ -136,33 +145,51 @@ 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) + var allReviews []vcs.Review - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) - } + for page := 1; page <= maxReviewPages; 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, reviewsPerPage, page) - var responses []reviewResponse - if err := json.Unmarshal(body, &responses); err != nil { - return nil, fmt.Errorf("parse reviews response: %w", err) - } + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews page %d: %w", page, err) + } - 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, + 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) < reviewsPerPage { + break + } + + if page == maxReviewPages { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxReviewPages, "reviewsFetched", len(allReviews)) } } - return reviews, nil + + return allReviews, nil } // DeleteReview deletes a pull request review. @@ -173,7 +200,6 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) - // nil body: the GitHub DELETE endpoint for reviews requires no request body. _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { var apiErr *APIError @@ -199,12 +225,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) } @@ -223,3 +244,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 +}