Skip to content

Commit 5bf7037

Browse files
sarlamsarlam
committed
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
1 parent c2d4ba1 commit 5bf7037

File tree

8 files changed

+105
-59
lines changed

8 files changed

+105
-59
lines changed

‎pom.xml‎

Lines changed: 5 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>

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

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
package com.spotify.github.v3.clients;
2222

2323
import static java.util.Arrays.stream;
24-
import static java.util.Objects.isNull;
2524
import static java.util.Objects.nonNull;
2625
import static java.util.function.Function.identity;
2726
import static java.util.stream.Collectors.toMap;
@@ -36,13 +35,11 @@
3635
import java.util.List;
3736
import java.util.Map;
3837
import java.util.NoSuchElementException;
39-
import java.util.Objects;
4038
import java.util.Optional;
4139
import java.util.concurrent.CompletableFuture;
4240
import java.util.regex.Matcher;
4341
import java.util.regex.Pattern;
44-
import javax.ws.rs.core.UriBuilder;
45-
import okhttp3.HttpUrl;
42+
import org.apache.http.client.utils.URIBuilder;
4643

4744
/**
4845
* Async page implementation for github resources
@@ -57,22 +54,17 @@ public class GithubPage<T> implements AsyncPage<T> {
5754
private final TypeReference<List<T>> typeReference;
5855
private final int itemsPerPage;
5956

60-
private String formatPath(String path) {
61-
String fullURL = github.urlFor(path);
62-
HttpUrl inputUrl = HttpUrl.parse(fullURL);
63-
64-
assert inputUrl != null;
65-
if (isNull(inputUrl.queryParameter("per_page"))) {
66-
return inputUrl
67-
.newBuilder()
68-
.addQueryParameter("per_page", Integer.toString(itemsPerPage))
69-
.build()
70-
.encodedPath();
71-
72-
57+
private static String formatPath(final String path, final int itemsPerPage) {
58+
try {
59+
URIBuilder uriBuilder = new URIBuilder(path);
60+
if (uriBuilder.getQueryParams().stream().anyMatch(p -> p.getName().equals("per_page"))) {
61+
return path;
62+
}
63+
uriBuilder.addParameter("per_page", Integer.toString(itemsPerPage));
64+
return uriBuilder.toString();
65+
} catch (Exception e) {
66+
return path;
7367
}
74-
75-
return path;
7668
}
7769

7870
/**
@@ -84,10 +76,10 @@ private String formatPath(String path) {
8476
*/
8577
GithubPage(
8678
final GitHubClient github, final String path, final TypeReference<List<T>> typeReference) {
79+
this.itemsPerPage = ITEM_PER_PAGE_DEFAULT;
8780
this.github = github;
88-
this.path = formatPath(path);
81+
this.path = formatPath(path, ITEM_PER_PAGE_DEFAULT);
8982
this.typeReference = typeReference;
90-
this.itemsPerPage = ITEM_PER_PAGE_DEFAULT;
9183
}
9284

9385
/**
@@ -102,10 +94,10 @@ private String formatPath(String path) {
10294
final String path,
10395
final TypeReference<List<T>> typeReference,
10496
final int itemsPerPage) {
97+
this.itemsPerPage = itemsPerPage;
10598
this.github = github;
106-
this.path = formatPath(path);
99+
this.path = formatPath(path, itemsPerPage);
107100
this.typeReference = typeReference;
108-
this.itemsPerPage = itemsPerPage;
109101
}
110102

111103
/** {@inheritDoc} */
@@ -123,7 +115,7 @@ public CompletableFuture<Pagination> pagination() {
123115
.map(
124116
prevLink ->
125117
pageNumberFromUri(prevLink.url().toString())
126-
.<RuntimeException>orElseThrow(
118+
.orElseThrow(
127119
() ->
128120
new RuntimeException(
129121
"Could not parse page number from Link header with rel=\"next\"")));
@@ -137,7 +129,7 @@ public CompletableFuture<Pagination> pagination() {
137129
.map(
138130
lastLink ->
139131
pageNumberFromUri(lastLink.url().toString())
140-
.<RuntimeException>orElseThrow(
132+
.orElseThrow(
141133
() ->
142134
new RuntimeException(
143135
"Could not parse page number from Link "
@@ -167,7 +159,7 @@ public CompletableFuture<AsyncPage<T>> nextPage() {
167159
Optional.ofNullable(linkMap.get("next"))
168160
.map(nextLink -> nextLink.url().toString().replaceAll(github.urlFor(""), ""))
169161
.orElseThrow(() -> new NoSuchElementException("Page iteration exhausted"));
170-
return new GithubPage<>(github, nextPath, typeReference);
162+
return new GithubPage<>(github, nextPath, typeReference, itemsPerPage);
171163
});
172164
}
173165

@@ -180,7 +172,7 @@ public CompletableFuture<Boolean> hasNextPage() {
180172
/** {@inheritDoc} */
181173
@Override
182174
public AsyncPage<T> clone() {
183-
return new GithubPage<>(github, path, typeReference);
175+
return new GithubPage<>(github, path, typeReference, itemsPerPage);
184176
}
185177

186178
/** {@inheritDoc} */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ public CompletableFuture<List<CommitItem>> listCommits(final long prNumber) {
195195
}
196196

197197
public Iterator<AsyncPage<CommitItem>> listCommits(final long prNumber, final int itemsPerPage) {
198-
final String path = "";
198+
final String path = String.format(PR_COMMITS_TEMPLATE, owner, repo, prNumber);
199199

200200
return new GithubPageIterator<>(new GithubPage<>(github, path, LIST_COMMIT_TYPE_REFERENCE, itemsPerPage));
201201
}

‎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/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 + "?per_page=30", "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&per_page=30", "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(

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

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@
4444
import com.spotify.github.async.Async;
4545
import com.spotify.github.async.AsyncPage;
4646
import com.spotify.github.http.HttpResponse;
47+
import com.spotify.github.jackson.Json;
48+
import com.spotify.github.v3.User;
4749
import com.spotify.github.v3.exceptions.RequestNotOkException;
4850
import com.spotify.github.v3.git.FileItem;
4951
import com.spotify.github.v3.git.ImmutableFileItem;
@@ -433,7 +435,7 @@ void testChangedFiles() throws IOException {
433435

434436
final HttpResponse firstPageResponse = createMockResponse(pageLink, expectedBody);
435437

436-
when(mockGithub.request(format(PR_CHANGED_FILES_TEMPLATE, "owner", "repo", "1")))
438+
when(mockGithub.request(format(PR_CHANGED_FILES_TEMPLATE + "?per_page=30", "owner", "repo", "1")))
437439
.thenReturn(completedFuture(firstPageResponse));
438440

439441
when(mockGithub.json()).thenReturn(github.json());
@@ -454,40 +456,76 @@ public void listCommitsWithoutSpecifyingPages() throws Exception {
454456
final int COMMIT_PER_PAGE = 30;
455457

456458
final String firstPageLink =
457-
"<https://api.github.com/repositories/10270250/pulls/20463/commits?page=2>; rel=\"next\", <https://api.github.com/repositories/10270250/pulls/20463/commits?page=4>; rel=\"last\"";
459+
"<https://api.github.com/repos/owner/repo/pulls/1/commits?page=2>; rel=\"next\", <https://api.github.com/repos/owner/repo/pulls/1/commits?page=4>; rel=\"last\"";
458460
final String firstPageBody =
459461
Resources.toString(getResource(this.getClass(), "pull_request_commits_page1.json"), defaultCharset());
460462
final HttpResponse firstPageResponse = createMockResponse(firstPageLink, firstPageBody);
461463

462-
final String secondPageLink =
463-
"<https://api.github.com/repositories/10270250/pulls/20463/commits?page=1>; rel=\"prev\", <https://api.github.com/repositories/10270250/pulls/20463/commits?page=3>; rel=\"next\", <https://api.github.com/repositories/10270250/pulls/20463/commits?page=3>; rel=\"last\", <https://api.github.com/repositories/10270250/pulls/20463/commits?page=1>; rel=\"first\"";
464+
when(mockGithub.request("/repos/owner/repo/pulls/1/commits"))
465+
.thenReturn(completedFuture(firstPageResponse));
466+
467+
final PullRequestClient pullRequestClient = PullRequestClient.create(mockGithub, "owner", "repo");
468+
469+
// When
470+
final List<CommitItem> commits = pullRequestClient.listCommits(1L).get();
471+
472+
// Then
473+
assertThat(commits.size(), is(COMMIT_PER_PAGE));
474+
assertThat(
475+
commits.get(COMMIT_PER_PAGE - 1).commit().tree().sha(), is("219cb4c1ffada21259876d390df1a85767481617"));
476+
}
477+
478+
@Test
479+
public void listCommitsSpecifyingPages() throws Exception {
480+
// Given
481+
final int COMMIT_PER_PAGE = 30;
482+
483+
final String firstPageLink = String.format(
484+
"<%s/repos/owner/repo/pulls/1/commits?page=2&per_page=30>; rel=\"next\", <%s/repos/owner/repo/pulls/1/commits?page=3&per_page=30>; rel=\"last\"",
485+
MOCK_GITHUB_URI,
486+
MOCK_GITHUB_URI);
487+
final String firstPageBody =
488+
Resources.toString(getResource(this.getClass(), "pull_request_commits_page1.json"), defaultCharset());
489+
final HttpResponse firstPageResponse = createMockResponse(firstPageLink, firstPageBody);
490+
491+
final String secondPageLink = String.format(
492+
"<%s/repos/owner/repo/pulls/1/commits?page=1&per_page=30>; rel=\"prev\", <%s/repos/owner/repo/pulls/1/commits?page=3&per_page=30>; rel=\"next\", <%s/repos/owner/repo/pulls/1/commits?page=3&per_page=30>; rel=\"last\", <%s/repos/owner/repo/pulls/1/commits?page=1&per_page=30>; rel=\"first\"",
493+
MOCK_GITHUB_URI,
494+
MOCK_GITHUB_URI,
495+
MOCK_GITHUB_URI,
496+
MOCK_GITHUB_URI);
464497
final String secondPageBody =
465498
Resources.toString(getResource(this.getClass(), "pull_request_commits_page2.json"), defaultCharset());
466499
final HttpResponse secondPageResponse = createMockResponse(secondPageLink, secondPageBody);
467500

468501
final String thirdPageLink =
469-
"<https://api.github.com/repositories/10270250/pulls/20463/commits?page=2>; rel=\"prev\", <https://api.github.com/repositories/10270250/pulls/20463/commits?page=1>; rel=\"first\"";
502+
String.format("<%s/repos/owner/repo/pulls/1/commits?page=2&per_page=30>; rel=\"prev\", <%s/repos/owner/repo/pulls/1/commits?page=1&per_page=30>; rel=\"first\"",
503+
MOCK_GITHUB_URI,
504+
MOCK_GITHUB_URI);
470505
final String thirdPageBody =
471506
Resources.toString(getResource(this.getClass(), "pull_request_commits_page3.json"), defaultCharset());
472507
final HttpResponse thirdPageResponse = createMockResponse(thirdPageLink, thirdPageBody);
473508

474-
when(mockGithub.request("/repos/owner/repo/pulls/1/commits"))
509+
when(mockGithub.urlFor("")).thenReturn(MOCK_GITHUB_URI.toString());
510+
when(mockGithub.json()).thenReturn(Json.create());
511+
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?per_page=30"))
475512
.thenReturn(completedFuture(firstPageResponse));
476-
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=1"))
513+
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=1&per_page=30"))
477514
.thenReturn(completedFuture(firstPageResponse));
478-
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=2"))
515+
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=2&per_page=30"))
479516
.thenReturn(completedFuture(secondPageResponse));
480-
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=3"))
517+
when(mockGithub.request("/repos/owner/repo/pulls/1/commits?page=3&per_page=30"))
481518
.thenReturn(completedFuture(thirdPageResponse));
482519

483520
final PullRequestClient pullRequestClient = PullRequestClient.create(mockGithub, "owner", "repo");
484521

485-
final List<CommitItem> commits = pullRequestClient.listCommits(1L).get();
486-
assertThat(commits.size(), is(COMMIT_PER_PAGE));
522+
// When
523+
final Iterable<AsyncPage<CommitItem>> pageIterator = () -> pullRequestClient.listCommits(1L, 30);
524+
final List<CommitItem> commits = Async.streamFromPaginatingIterable(pageIterator).collect(toList());
525+
526+
// Then
527+
assertThat(commits.size(), is(COMMIT_PER_PAGE * 3));
487528
assertThat(
488529
commits.get(COMMIT_PER_PAGE - 1).commit().tree().sha(), is("219cb4c1ffada21259876d390df1a85767481617"));
489-
// make this test works for the current situation
490-
// make another test for a new method/signature that would return the full list
491-
// code the logic to follow the links
492530
}
493531
}

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public void listAuthenticatedUserRepositories() throws Exception {
153153
final String pageBody = getFixture("list_of_repos_for_authenticated_user.json");
154154
final HttpResponse pageResponse = createMockResponse(pageLink, pageBody);
155155

156-
when(github.request("/user/repos")).thenReturn(completedFuture(pageResponse));
156+
when(github.request("/user/repos?per_page=30")).thenReturn(completedFuture(pageResponse));
157157

158158
final Iterable<AsyncPage<Repository>> pageIterator =
159159
() ->
@@ -515,10 +515,10 @@ public void listBranches() throws Exception {
515515
@Test
516516
void listAllBranches() throws Exception {
517517
final String link =
518-
"<https://github.com/api/v3/repos/someowner/somerepo/branches>; rel=\"last\"";
518+
"<https://github.com/api/v3/repos/someowner/somerepo/branches?page=1>; rel=\"last\"";
519519
final HttpResponse response = createMockResponse(link, getFixture("list_branches.json"));
520520

521-
when(github.request("/repos/someowner/somerepo/branches"))
521+
when(github.request("/repos/someowner/somerepo/branches?per_page=30"))
522522
.thenReturn(completedFuture(response));
523523
final List<Branch> branches =
524524
Async.streamFromPaginatingIterable(repoClient::listAllBranches)
@@ -573,14 +573,14 @@ public void testStatusesPaginationForeach() throws Exception {
573573

574574
when(github.request(
575575
format(
576-
STATUS_URI_TEMPLATE,
576+
STATUS_URI_TEMPLATE + "?per_page=30",
577577
"someowner",
578578
"somerepo",
579579
"553c2077f0edc3d5dc5d17262f6aa498e69d6f8e")))
580580
.thenReturn(completedFuture(firstPageResponse));
581581
when(github.request(
582582
format(
583-
STATUS_URI_TEMPLATE + "?page=2",
583+
STATUS_URI_TEMPLATE + "?page=2&per_page=30",
584584
"someowner",
585585
"somerepo",
586586
"553c2077f0edc3d5dc5d17262f6aa498e69d6f8e")))

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,9 @@ public void listTeamMembersPaged() throws Exception {
184184

185185
final HttpResponse lastPageResponse = createMockResponse(lastPageLink, lastPageBody);
186186

187-
when(github.request(endsWith("/orgs/github/teams/1/members?per_page=30")))
187+
when(github.request(endsWith("/orgs/github/teams/1/members?per_page=1")))
188188
.thenReturn(completedFuture(firstPageResponse));
189-
when(github.request(endsWith("/orgs/github/teams/1/members?page=2&per_page=30")))
189+
when(github.request(endsWith("/orgs/github/teams/1/members?page=2&per_page=1")))
190190
.thenReturn(completedFuture(lastPageResponse));
191191

192192
final Iterable<AsyncPage<User>> pageIterator = () -> teamClient.listTeamMembers("1", 1);

0 commit comments

Comments
 (0)