Skip to content

Commit 900ea76

Browse files
sarlamsarlam
authored
feat(pull-request): support pagination in list commits (#241)
* feat(pull-request): support pagination in list commits (bisect: bad) Summary ======= This commits introduces a test as a ground base to sustain full pagination when listing commits from a pull request WORK IN PROGRESS * feat(GitHubPages): introduce per pages (bisect: bad) Summary ======= implements the per pages limit * feat(pr-commits): Offer a new paginated method Summary ======= This introduce a new methods that returns a paginated version of the endpoint listCommits in a pull request context * fix: page extraction regex issue * chore: update test
1 parent d2b4785 commit 900ea76

File tree

12 files changed

+6359
-41
lines changed

12 files changed

+6359
-41
lines changed

‎pom.xml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,11 @@
138138
<artifactId>jsr311-api</artifactId>
139139
<version>${jsr311-api.version}</version>
140140
</dependency>
141+
<dependency>
142+
<groupId>org.apache.httpcomponents</groupId>
143+
<artifactId>httpclient</artifactId>
144+
<version>4.5.14</version>
145+
</dependency>
141146
<dependency>
142147
<groupId>com.fasterxml.jackson.core</groupId>
143148
<artifactId>jackson-annotations</artifactId>
@@ -222,6 +227,12 @@
222227
<version>2.2</version>
223228
<scope>test</scope>
224229
</dependency>
230+
<dependency>
231+
<groupId>com.github.npathai</groupId>
232+
<artifactId>hamcrest-optional</artifactId>
233+
<version>2.0.0</version>
234+
<scope>test</scope>
235+
</dependency>
225236
<dependency>
226237
<groupId>org.junit.jupiter</groupId>
227238
<artifactId>junit-jupiter-engine</artifactId>

‎src/main/java/com/spotify/github/v3/clients/GithubPage.java‎

Lines changed: 58 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@
3636
import java.util.NoSuchElementException;
3737
import java.util.Optional;
3838
import java.util.concurrent.CompletableFuture;
39+
import java.util.regex.Matcher;
40+
import java.util.regex.Pattern;
41+
import org.apache.http.client.utils.URIBuilder;
3942

4043
/**
4144
* Async page implementation for github resources
@@ -44,21 +47,55 @@
4447
*/
4548
public class GithubPage<T> implements AsyncPage<T> {
4649

50+
static final int ITEM_PER_PAGE_DEFAULT = 30;
4751
private final GitHubClient github;
4852
private final String path;
4953
private final TypeReference<List<T>> typeReference;
54+
private final int itemsPerPage;
55+
56+
protected static String formatPath(final String path, final int itemsPerPage) {
57+
try {
58+
URIBuilder uriBuilder = new URIBuilder(path);
59+
if (uriBuilder.getQueryParams().stream().anyMatch(p -> p.getName().equals("per_page"))) {
60+
return path;
61+
}
62+
uriBuilder.addParameter("per_page", Integer.toString(itemsPerPage));
63+
return uriBuilder.toString();
64+
} catch (Exception e) {
65+
return path;
66+
}
67+
}
5068

5169
/**
52-
* C'tor.
70+
* Constructor.
5371
*
5472
* @param github github client
5573
* @param path resource page path
5674
* @param typeReference type reference for deserialization
5775
*/
5876
GithubPage(
5977
final GitHubClient github, final String path, final TypeReference<List<T>> typeReference) {
78+
this.itemsPerPage = ITEM_PER_PAGE_DEFAULT;
79+
this.github = github;
80+
this.path = formatPath(path, ITEM_PER_PAGE_DEFAULT);
81+
this.typeReference = typeReference;
82+
}
83+
84+
/**
85+
* Constructor.
86+
*
87+
* @param github github client
88+
* @param path resource page path
89+
* @param typeReference type reference for deserialization
90+
*/
91+
GithubPage(
92+
final GitHubClient github,
93+
final String path,
94+
final TypeReference<List<T>> typeReference,
95+
final int itemsPerPage) {
96+
this.itemsPerPage = itemsPerPage;
6097
this.github = github;
61-
this.path = path;
98+
this.path = formatPath(path, itemsPerPage);
6299
this.typeReference = typeReference;
63100
}
64101

@@ -77,7 +114,7 @@ public CompletableFuture<Pagination> pagination() {
77114
.map(
78115
prevLink ->
79116
pageNumberFromUri(prevLink.url().toString())
80-
.<RuntimeException>orElseThrow(
117+
.orElseThrow(
81118
() ->
82119
new RuntimeException(
83120
"Could not parse page number from Link header with rel=\"next\"")));
@@ -91,7 +128,7 @@ public CompletableFuture<Pagination> pagination() {
91128
.map(
92129
lastLink ->
93130
pageNumberFromUri(lastLink.url().toString())
94-
.<RuntimeException>orElseThrow(
131+
.orElseThrow(
95132
() ->
96133
new RuntimeException(
97134
"Could not parse page number from Link "
@@ -121,7 +158,7 @@ public CompletableFuture<AsyncPage<T>> nextPage() {
121158
Optional.ofNullable(linkMap.get("next"))
122159
.map(nextLink -> nextLink.url().toString().replaceAll(github.urlFor(""), ""))
123160
.orElseThrow(() -> new NoSuchElementException("Page iteration exhausted"));
124-
return new GithubPage<>(github, nextPath, typeReference);
161+
return new GithubPage<>(github, nextPath, typeReference, itemsPerPage);
125162
});
126163
}
127164

@@ -134,7 +171,7 @@ public CompletableFuture<Boolean> hasNextPage() {
134171
/** {@inheritDoc} */
135172
@Override
136173
public AsyncPage<T> clone() {
137-
return new GithubPage<>(github, path, typeReference);
174+
return new GithubPage<>(github, path, typeReference, itemsPerPage);
138175
}
139176

140177
/** {@inheritDoc} */
@@ -153,19 +190,22 @@ private CompletableFuture<Map<String, Link>> linkMapAsync() {
153190
return github
154191
.request(path)
155192
.thenApply(
156-
response -> {
157-
return Optional.ofNullable(response.header("Link"))
158-
.stream()
159-
.flatMap(linkHeader -> stream(linkHeader.split(",")))
160-
.map(linkString -> Link.from(linkString.split(";")))
161-
.filter(link -> link.rel().isPresent())
162-
.collect(toMap(link -> link.rel().get(), identity()));
163-
});
193+
response ->
194+
Optional.ofNullable(response.header("Link")).stream()
195+
.flatMap(linkHeader -> stream(linkHeader.split(",")))
196+
.map(linkString -> Link.from(linkString.split(";")))
197+
.filter(link -> link.rel().isPresent())
198+
.collect(toMap(link -> link.rel().get(), identity())));
164199
}
165200

166-
private Optional<Integer> pageNumberFromUri(final String uri) {
167-
return Optional.ofNullable(uri.replaceAll(".*\\?page=", "").replaceAll("&.*", ""))
168-
.filter(string -> string.matches("\\d+"))
169-
.map(Integer::parseInt);
201+
protected static Optional<Integer> pageNumberFromUri(final String uri) {
202+
Pattern pageInQueryPattern = Pattern.compile("(^|\\?|&)page=(?<page>\\d+)", Pattern.CASE_INSENSITIVE);
203+
try {
204+
String query = new URIBuilder(uri).build().getQuery();
205+
Matcher matcher = pageInQueryPattern.matcher(query);
206+
return matcher.find() ? Optional.of(Integer.parseInt(matcher.group("page"))) : Optional.empty();
207+
} catch (Exception e) {
208+
return Optional.empty();
209+
}
170210
}
171211
}

‎src/main/java/com/spotify/github/v3/clients/PullRequestClient.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,12 @@
2828
import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_REQUEST_TYPE_REFERENCE;
2929
import static com.spotify.github.v3.clients.GitHubClient.LIST_REVIEW_TYPE_REFERENCE;
3030
import static java.util.Objects.isNull;
31+
import static java.lang.Math.toIntExact;
3132

3233
import com.google.common.base.Strings;
3334
import com.google.common.collect.ImmutableMap;
3435
import com.spotify.github.async.AsyncPage;
36+
import com.spotify.github.jackson.Json;
3537
import com.spotify.github.v3.git.FileItem;
3638
import com.spotify.github.v3.prs.Comment;
3739
import com.spotify.github.v3.prs.MergeParameters;
@@ -190,7 +192,14 @@ public CompletableFuture<List<CommitItem>> listCommits(final int prNumber) {
190192
public CompletableFuture<List<CommitItem>> listCommits(final long prNumber) {
191193
final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, prNumber);
192194
log.debug("Fetching pull request commits from " + path);
193-
return github.request(path, LIST_COMMIT_TYPE_REFERENCE);
195+
return github.request(path).thenApply(
196+
response -> Json.create().fromJsonUncheckedNotNull(response.bodyString(), LIST_COMMIT_TYPE_REFERENCE));
197+
}
198+
199+
public Iterator<AsyncPage<CommitItem>> listCommits(final long prNumber, final int itemsPerPage) {
200+
final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, prNumber);
201+
202+
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_COMMIT_TYPE_REFERENCE, itemsPerPage));
194203
}
195204

196205
/**
@@ -238,10 +247,9 @@ public Iterator<AsyncPage<Review>> listReviews(final int prNumber, final int ite
238247
* @return iterator of reviews
239248
*/
240249
public Iterator<AsyncPage<Review>> listReviews(final long prNumber, final long itemsPerPage) {
241-
// FIXME Use itemsPerPage property
242250
final String path = String.format(PR_REVIEWS_TEMPLATE, owner, repo, prNumber);
243251
log.debug("Fetching pull request reviews from " + path);
244-
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_REVIEW_TYPE_REFERENCE));
252+
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_REVIEW_TYPE_REFERENCE, toIntExact(itemsPerPage)));
245253
}
246254

247255
/**

‎src/main/java/com/spotify/github/v3/clients/TeamClient.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public class TeamClient {
4949

5050
private static final String MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members";
5151

52-
private static final String PAGED_MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members?per_page=%d";
52+
private static final String PAGED_MEMBERS_TEMPLATE = "/orgs/%s/teams/%s/members";
5353

5454
private static final String MEMBERSHIP_TEMPLATE = "/orgs/%s/teams/%s/memberships/%s";
5555

@@ -176,9 +176,9 @@ public CompletableFuture<List<User>> listTeamMembers(final String slug) {
176176
* @return list of all users in a team
177177
*/
178178
public Iterator<AsyncPage<User>> listTeamMembers(final String slug, final int pageSize) {
179-
final String path = String.format(PAGED_MEMBERS_TEMPLATE, org, slug, pageSize);
179+
final String path = String.format(PAGED_MEMBERS_TEMPLATE, org, slug);
180180
log.debug("Fetching members for: {}", path);
181-
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_TEAM_MEMBERS));
181+
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_TEAM_MEMBERS, pageSize));
182182
}
183183

184184
/**

‎src/test/java/com/spotify/github/v3/clients/GitHubPageTest.java‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/*-
2+
* -\-\-
3+
* github-api
4+
* --
5+
* Copyright (C) 2016 - 2025 Spotify AB
6+
* --
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
* -/-/-
19+
*/
20+
21+
package com.spotify.github.v3.clients;
22+
23+
import static org.hamcrest.core.Is.is;
24+
25+
import java.net.URI;
26+
import org.junit.jupiter.api.Test;
27+
import static org.hamcrest.MatcherAssert.assertThat;
28+
import static com.github.npathai.hamcrestopt.OptionalMatchers.isPresentAndIs;
29+
import static com.github.npathai.hamcrestopt.OptionalMatchers.isEmpty;
30+
31+
public class GitHubPageTest {
32+
private static final String MOCK_GITHUB_HOST = "bogus.host";
33+
private static final URI MOCK_GITHUB_URI = URI.create(String.format("http://%s/api/v3/", MOCK_GITHUB_HOST));
34+
35+
@Test
36+
public void testFormatPathWithPerPage() {
37+
assertThat(GithubPage.formatPath("/commits?page=2", 3), is("/commits?page=2&per_page=3"));
38+
assertThat(GithubPage.formatPath("NOT_A_CORRECT PATH ", 3), is("NOT_A_CORRECT PATH "));
39+
assertThat(GithubPage.formatPath("/commits", 3), is("/commits?per_page=3"));
40+
assertThat(GithubPage.formatPath("/commits?page=2&per_page=7", 3), is("/commits?page=2&per_page=7"));
41+
}
42+
43+
@Test
44+
public void testPageNumberFromURI() {
45+
assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("/commits?page=5").toString()), isPresentAndIs(5));
46+
assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("/commits").toString()), isEmpty());
47+
assertThat(GithubPage.pageNumberFromUri(MOCK_GITHUB_URI.resolve("commits").toString()), isEmpty());
48+
assertThat(GithubPage.pageNumberFromUri("/commits?page=2"), isPresentAndIs(2));
49+
assertThat(GithubPage.pageNumberFromUri("/commits?per_page=4&page=2"), isPresentAndIs(2));
50+
assertThat(GithubPage.pageNumberFromUri("NOT_A_CORRECT PATH "), isEmpty());
51+
assertThat(GithubPage.pageNumberFromUri("/commits"), isEmpty());
52+
}
53+
}

‎src/test/java/com/spotify/github/v3/clients/IssueClientTest.java‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,21 +81,26 @@ public void setUp() {
8181
@Test
8282
public void testCommentPaginationSpliterator() throws IOException {
8383
final String firstPageLink =
84-
"<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\"";
84+
"<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\"";
8585
final String firstPageBody =
8686
Resources.toString(getResource(this.getClass(), "comments_page1.json"), defaultCharset());
8787
final HttpResponse firstPageResponse = createMockResponse(firstPageLink, firstPageBody);
8888

8989
final String lastPageLink =
90-
"<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\"";
90+
"<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\"";
9191
final String lastPageBody =
9292
Resources.toString(getResource(this.getClass(), "comments_page2.json"), defaultCharset());
9393
final HttpResponse lastPageResponse = createMockResponse(lastPageLink, lastPageBody);
9494

95-
when(github.request(format(COMMENTS_URI_NUMBER_TEMPLATE, "someowner", "somerepo", "123")))
95+
when(github.request(
96+
format(COMMENTS_URI_NUMBER_TEMPLATE + "?per_page=30", "someowner", "somerepo", "123")))
9697
.thenReturn(completedFuture(firstPageResponse));
9798
when(github.request(
98-
format(COMMENTS_URI_NUMBER_TEMPLATE + "?page=2", "someowner", "somerepo", "123")))
99+
format(
100+
COMMENTS_URI_NUMBER_TEMPLATE + "?page=2&per_page=30",
101+
"someowner",
102+
"somerepo",
103+
"123")))
99104
.thenReturn(completedFuture(lastPageResponse));
100105

101106
final Iterable<AsyncPage<Comment>> pageIterator = () -> issueClient.listComments(123);
@@ -121,10 +126,15 @@ public void testCommentPaginationForeach() throws IOException {
121126
Resources.toString(getResource(this.getClass(), "comments_page2.json"), defaultCharset());
122127
final HttpResponse lastPageResponse = createMockResponse(lastPageLink, lastPageBody);
123128

124-
when(github.request(format(COMMENTS_URI_NUMBER_TEMPLATE, "someowner", "somerepo", "123")))
129+
when(github.request(
130+
format(COMMENTS_URI_NUMBER_TEMPLATE + "?per_page=30", "someowner", "somerepo", "123")))
125131
.thenReturn(completedFuture(firstPageResponse));
126132
when(github.request(
127-
format(COMMENTS_URI_NUMBER_TEMPLATE + "?page=2", "someowner", "somerepo", "123")))
133+
format(
134+
COMMENTS_URI_NUMBER_TEMPLATE + "?page=2&per_page=30",
135+
"someowner",
136+
"somerepo",
137+
"123")))
128138
.thenReturn(completedFuture(lastPageResponse));
129139

130140
final List<Comment> listComments = Lists.newArrayList();
@@ -233,7 +243,8 @@ public void testListIssueCommentReaction() throws IOException {
233243
.content(CommentReactionContent.HEART)
234244
.user(ImmutableUser.builder().login("octocat").build())
235245
.build())));
236-
final String path = format(COMMENTS_REACTION_TEMPLATE, "someowner", "somerepo", commentId);
246+
final String path =
247+
format(COMMENTS_REACTION_TEMPLATE + "?per_page=30", "someowner", "somerepo", commentId);
237248

238249
final String firstPageLink =
239250
format(

0 commit comments

Comments
 (0)