spotify · sarlam · Jun 26, 2025 · Jun 24, 2025 · Jun 24, 2025 · Jun 24, 2025
diff --git a/pom.xml b/pom.xml
@@ -138,6 +138,11 @@
             <artifactId>jsr311-api</artifactId>
             <version>${jsr311-api.version}</version>
         </dependency>
+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpclient</artifactId>
+            <version>4.5.14</version>
+        </dependency>
         <dependency>
             <groupId>com.fasterxml.jackson.core</groupId>
             <artifactId>jackson-annotations</artifactId>
@@ -222,6 +227,12 @@
             <version>2.2</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>com.github.npathai</groupId>
+            <artifactId>hamcrest-optional</artifactId>
+            <version>2.0.0</version>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.junit.jupiter</groupId>
             <artifactId>junit-jupiter-engine</artifactId>

diff --git a/src/main/java/com/spotify/github/v3/clients/GithubPage.java b/src/main/java/com/spotify/github/v3/clients/GithubPage.java
@@ -36,6 +36,9 @@
 import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.http.client.utils.URIBuilder;
 
 /**
  * Async page implementation for github resources
@@ -44,21 +47,55 @@
  */
 public class GithubPage<T> implements AsyncPage<T> {
 
+  static final int ITEM_PER_PAGE_DEFAULT = 30;
   private final GitHubClient github;
   private final String path;
   private final TypeReference<List<T>> typeReference;
+  private final int itemsPerPage;
+
+  protected static String formatPath(final String path, final int itemsPerPage) {
+    try {
+      URIBuilder uriBuilder = new URIBuilder(path);
+      if (uriBuilder.getQueryParams().stream().anyMatch(p -> p.getName().equals("per_page"))) {
+        return path;
+      }
+      uriBuilder.addParameter("per_page", Integer.toString(itemsPerPage));
+      return uriBuilder.toString();
+    } catch (Exception e) {
+      return path;
+    }
+  }
 
   /**
-   * C'tor.
+   * Constructor.
    *
    * @param github github client
    * @param path resource page path
    * @param typeReference type reference for deserialization
    */
   GithubPage(
       final GitHubClient github, final String path, final TypeReference<List<T>> typeReference) {
+    this.itemsPerPage = ITEM_PER_PAGE_DEFAULT;
+    this.github = github;
+    this.path = formatPath(path, ITEM_PER_PAGE_DEFAULT);
+    this.typeReference = typeReference;
+  }
+
+  /**
+   * Constructor.
+   *
+   * @param github github client
+   * @param path resource page path
+   * @param typeReference type reference for deserialization
+   */
+  GithubPage(
+      final GitHubClient github,
+      final String path,
+      final TypeReference<List<T>> typeReference,
+      final int itemsPerPage) {
+    this.itemsPerPage = itemsPerPage;
     this.github = github;
-    this.path = path;
+    this.path = formatPath(path, itemsPerPage);
     this.typeReference = typeReference;
   }
 
@@ -77,7 +114,7 @@ public CompletableFuture<Pagination> pagination() {
                       .map(
                           prevLink ->
                               pageNumberFromUri(prevLink.url().toString())
-                                  .<RuntimeException>orElseThrow(
+                                  .orElseThrow(
                                       () ->
                                           new RuntimeException(
                                               "Could not parse page number from Link header with rel=\"next\"")));
@@ -91,7 +128,7 @@ public CompletableFuture<Pagination> pagination() {
                                   .map(
                                       lastLink ->
                                           pageNumberFromUri(lastLink.url().toString())
-                                              .<RuntimeException>orElseThrow(
+                                              .orElseThrow(
                                                   () ->
                                                       new RuntimeException(
                                                           "Could not parse page number from Link "
@@ -121,7 +158,7 @@ public CompletableFuture<AsyncPage<T>> nextPage() {
                   Optional.ofNullable(linkMap.get("next"))
                       .map(nextLink -> nextLink.url().toString().replaceAll(github.urlFor(""), ""))
                       .orElseThrow(() -> new NoSuchElementException("Page iteration exhausted"));
-              return new GithubPage<>(github, nextPath, typeReference);
+              return new GithubPage<>(github, nextPath, typeReference, itemsPerPage);
             });
   }
 
@@ -134,7 +171,7 @@ public CompletableFuture<Boolean> hasNextPage() {
   /** {@inheritDoc} */
   @Override
   public AsyncPage<T> clone() {
-    return new GithubPage<>(github, path, typeReference);
+    return new GithubPage<>(github, path, typeReference, itemsPerPage);
   }
 
   /** {@inheritDoc} */
@@ -153,19 +190,22 @@ private CompletableFuture<Map<String, Link>> linkMapAsync() {
     return github
         .request(path)
         .thenApply(
-            response -> {
-              return Optional.ofNullable(response.header("Link"))
-                  .stream()
-                  .flatMap(linkHeader -> stream(linkHeader.split(",")))
-                  .map(linkString -> Link.from(linkString.split(";")))
-                  .filter(link -> link.rel().isPresent())
-                  .collect(toMap(link -> link.rel().get(), identity()));
-            });
+            response ->
+                Optional.ofNullable(response.header("Link")).stream()
+                    .flatMap(linkHeader -> stream(linkHeader.split(",")))
+                    .map(linkString -> Link.from(linkString.split(";")))
+                    .filter(link -> link.rel().isPresent())
+                    .collect(toMap(link -> link.rel().get(), identity())));
   }
 
-  private Optional<Integer> pageNumberFromUri(final String uri) {
-    return Optional.ofNullable(uri.replaceAll(".*\\?page=", "").replaceAll("&.*", ""))
-        .filter(string -> string.matches("\\d+"))
-        .map(Integer::parseInt);
+  protected static Optional<Integer> pageNumberFromUri(final String uri) {
+    Pattern pageInQueryPattern = Pattern.compile("(^|\\?|&)page=(?<page>\\d+)", Pattern.CASE_INSENSITIVE);
+    try {
+      String query = new URIBuilder(uri).build().getQuery();
+      Matcher matcher = pageInQueryPattern.matcher(query);
+      return matcher.find() ? Optional.of(Integer.parseInt(matcher.group("page"))) : Optional.empty();
+    } catch (Exception e) {
+      return Optional.empty();
+    }
   }
 }
diff --git a/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java b/src/main/java/com/spotify/github/v3/clients/PullRequestClient.java
@@ -28,10 +28,12 @@
 import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_REQUEST_TYPE_REFERENCE;
 import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_TYPE_REFERENCE;
 import static java.util.Objects.isNull;
+import static java.lang.Math.toIntExact;
 
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.spotify.github.async.AsyncPage;
+import com.spotify.github.jackson.Json;
 import com.spotify.github.v3.git.FileItem;
 import com.spotify.github.v3.prs.Comment;
 import com.spotify.github.v3.prs.MergeParameters;
@@ -190,7 +192,14 @@ public CompletableFuture<List<CommitItem>> listCommits(final int prNumber) {
   public CompletableFuture<List<CommitItem>> listCommits(final long prNumber) {
     final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, prNumber);
     log.debug("Fetching pull request commits from " + path);
-    return github.request(path, LIST_COMMIT_TYPE_REFERENCE);
+    return github.request(path).thenApply(
+        response -> Json.create().fromJsonUncheckedNotNull(response.bodyString(), LIST_COMMIT_TYPE_REFERENCE));
+  }
+
+  public Iterator<AsyncPage<CommitItem>> listCommits(final long prNumber, final int itemsPerPage) {
+    final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, prNumber);
+
+    return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_COMMIT_TYPE_REFERENCE, itemsPerPage));
   }
 
   /**
@@ -238,10 +247,9 @@ public Iterator<AsyncPage<Review>> listReviews(final int prNumber, final int ite
    * @return iterator of reviews
    */
   public Iterator<AsyncPage<Review>> listReviews(final long prNumber, final long itemsPerPage) {
-    // FIXME Use itemsPerPage property
     final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, prNumber);
     log.debug("Fetching pull request reviews from " + path);
-    return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_REVIEW_TYPE_REFERENCE));
+    return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_REVIEW_TYPE_REFERENCE, toIntExact(itemsPerPage)));
   }
 
   /**

diff --git a/src/main/java/com/spotify/github/v3/clients/TeamClient.java b/src/main/java/com/spotify/github/v3/clients/TeamClient.java
@@ -49,7 +49,7 @@ public class TeamClient {
 
   private static final String MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members";
 
-  private static final String PAGED_MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members?per_page=%d";
+  private static final String PAGED_MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members";
 
   private static final String MEMBERSHIP_TEMPLATE = "/orgs/%s/teams/%s/memberships/%s";
 
@@ -176,9 +176,9 @@ public CompletableFuture<List<User>> listTeamMembers(final String slug) {
    * @return list of all users in a team
    */
   public Iterator<AsyncPage<User>> listTeamMembers(final String slug, final int pageSize) {
-    final String path = String.format(PAGED_MEMBERS_TEMPLATE, org, slug, pageSize);
+    final String path = String.format(PAGED_MEMBERS_TEMPLATE, org, slug);
     log.debug("Fetching members for: {}", path);
-    return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_TEAM_MEMBERS));
+    return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_TEAM_MEMBERS, pageSize));
   }
 
   /**

diff --git a/src/test/java/com/spotify/github/v3/clients/GitHubPageTest.java b/src/test/java/com/spotify/github/v3/clients/GitHubPageTest.java
@@ -0,0 +1,53 @@
+/*-
+ * -\-\-
+ * github-api
+ * --
+ * Copyright (C) 2016 - 2025 Spotify AB
+ * --
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * -/-/-
+ */
+
+package com.spotify.github.v3.clients;
+
+import static org.hamcrest.core.Is.is;
+
+import java.net.URI;
+import org.junit.jupiter.api.Test;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static com.github.npathai.hamcrestopt.OptionalMatchers.isPresentAndIs;
+import static com.github.npathai.hamcrestopt.OptionalMatchers.isEmpty;
+
+public class GitHubPageTest {
+  private static final String MOCK_GITHUB_HOST = "bogus.host";
+  private static final URI MOCK_GITHUB_URI = URI.create(String.format("http://%s/api/v3/", MOCK_GITHUB_HOST));
+
+  @Test
+  public void testFormatPathWithPerPage() {
+    assertThat(GithubPage.formatPath("/commits?page=2", 3), is("/commits?page=2&per_page=3"));
+    assertThat(GithubPage.formatPath("NOT_A_CORRECT PATH ", 3), is("NOT_A_CORRECT PATH "));
+    assertThat(GithubPage.formatPath("/commits", 3), is("/commits?per_page=3"));
+    assertThat(GithubPage.formatPath("/commits?page=2&per_page=7", 3), is("/commits?page=2&per_page=7"));
+  }
+
+  @Test
+  public void testPageNumberFromURI() {
+    assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("/commits?page=5").toString()), isPresentAndIs(5));
+    assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("/commits").toString()), isEmpty());
+    assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("commits").toString()), isEmpty());
+    assertThat(GithubPage.pageNumberFromUri("/commits?page=2"), isPresentAndIs(2));
+    assertThat(GithubPage.pageNumberFromUri("/commits?per_page=4&page=2"), isPresentAndIs(2));
+    assertThat(GithubPage.pageNumberFromUri("NOT_A_CORRECT PATH "), isEmpty());
+    assertThat(GithubPage.pageNumberFromUri("/commits"), isEmpty());
+  }
+}
diff --git a/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java b/src/test/java/com/spotify/github/v3/clients/IssueClientTest.java
@@ -81,21 +81,26 @@ public void setUp() {
   @Test
   public void testCommentPaginationSpliterator() throws IOException {
     final String firstPageLink =
-        "<https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=2>; rel=\"next\", <https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=2>; rel=\"last\"";
+        "<https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=2&per_page=30>; rel=\"next\", <https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=2&per_page=30>; rel=\"last\"";
     final String firstPageBody =
         Resources.toString(getResource(this.getClass(), "comments_page1.json"), defaultCharset());
     final HttpResponse firstPageResponse = createMockResponse(firstPageLink, firstPageBody);
 
     final String lastPageLink =
-        "<https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments>; rel=\"first\", <https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments>; rel=\"prev\"";
+        "<https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=1&per_page=30>; rel=\"first\", <https://github.com/api/v3/repos/someowner/somerepo/issues/123/comments?page=1&per_page=30>; rel=\"prev\"";
     final String lastPageBody =
         Resources.toString(getResource(this.getClass(), "comments_page2.json"), defaultCharset());
     final HttpResponse lastPageResponse = createMockResponse(lastPageLink, lastPageBody);
 
-    when(github.request(format(COMMENTS_URI_NUMBER_TEMPLATE, "someowner", "somerepo", "123")))
+    when(github.request(
+            format(COMMENTS_URI_NUMBER_TEMPLATE + "?per_page=30", "someowner", "somerepo", "123")))
         .thenReturn(completedFuture(firstPageResponse));
     when(github.request(
-            format(COMMENTS_URI_NUMBER_TEMPLATE + "?page=2", "someowner", "somerepo", "123")))
+            format(
+                COMMENTS_URI_NUMBER_TEMPLATE + "?page=2&per_page=30",
+                "someowner",
+                "somerepo",
+                "123")))
         .thenReturn(completedFuture(lastPageResponse));
 
     final Iterable<AsyncPage<Comment>> pageIterator = () -> issueClient.listComments(123);
@@ -121,10 +126,15 @@ public void testCommentPaginationForeach() throws IOException {
         Resources.toString(getResource(this.getClass(), "comments_page2.json"), defaultCharset());
     final HttpResponse lastPageResponse = createMockResponse(lastPageLink, lastPageBody);
 
-    when(github.request(format(COMMENTS_URI_NUMBER_TEMPLATE, "someowner", "somerepo", "123")))
+    when(github.request(
+            format(COMMENTS_URI_NUMBER_TEMPLATE + "?per_page=30", "someowner", "somerepo", "123")))
         .thenReturn(completedFuture(firstPageResponse));
     when(github.request(
-            format(COMMENTS_URI_NUMBER_TEMPLATE + "?page=2", "someowner", "somerepo", "123")))
+            format(
+                COMMENTS_URI_NUMBER_TEMPLATE + "?page=2&per_page=30",
+                "someowner",
+                "somerepo",
+                "123")))
         .thenReturn(completedFuture(lastPageResponse));
 
     final List<Comment> listComments = Lists.newArrayList();
@@ -233,7 +243,8 @@ public void testListIssueCommentReaction() throws IOException {
                     .content(CommentReactionContent.HEART)
                     .user(ImmutableUser.builder().login("octocat").build())
                     .build())));
-    final String path = format(COMMENTS_REACTION_TEMPLATE, "someowner", "somerepo", commentId);
+    final String path =
+        format(COMMENTS_REACTION_TEMPLATE + "?per_page=30", "someowner", "somerepo", commentId);
 
     final String firstPageLink =
         format(