From a49819fef91cc7dd320bcfe8bdffb6116bd92051 Mon Sep 17 00:00:00 2001 From: Anuj Hydrabadi Date: Fri, 13 Oct 2023 20:09:51 +0530 Subject: [PATCH 1/5] Added support for Pagination --- .../github/GHAppInstallationsIterable.java | 53 +++++ .../kohsuke/github/GHArtifactsIterable.java | 53 +++++ .../kohsuke/github/GHCheckRunsIterable.java | 53 +++++ .../kohsuke/github/GHCommitFileIterable.java | 6 + .../java/org/kohsuke/github/GHCompare.java | 80 ++++++- .../github/GHWorkflowJobsIterable.java | 53 +++++ .../github/GHWorkflowRunsIterable.java | 60 +++++ .../kohsuke/github/GHWorkflowsIterable.java | 57 +++++ .../github/GitHubPageContentsIterable.java | 7 + .../org/kohsuke/github/GitHubPaginator.java | 191 +++++++++++++++ .../org/kohsuke/github/NavigableIterator.java | 34 +++ .../org/kohsuke/github/PagedIterable.java | 27 +++ .../kohsuke/github/PagedSearchIterable.java | 52 +++++ .../java/org/kohsuke/github/Paginator.java | 218 ++++++++++++++++++ .../GitHubConnectorHttpConnectorAdapter.java | 2 +- 15 files changed, 940 insertions(+), 6 deletions(-) create mode 100644 src/main/java/org/kohsuke/github/GitHubPaginator.java create mode 100644 src/main/java/org/kohsuke/github/NavigableIterator.java create mode 100644 src/main/java/org/kohsuke/github/Paginator.java diff --git a/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java b/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java index 8a150de1fb..d101aa54e0 100644 --- a/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java +++ b/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java @@ -25,6 +25,17 @@ public GHAppInstallationsIterable(GitHub root) { this.root = root; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + final GitHubRequest request = root.createRequest().withUrlPath(APP_INSTALLATIONS_URL).build(); + + return new Paginator<>( + adapt(GitHubPaginator + .create(root.getClient(), GHAppInstallationsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -63,4 +74,46 @@ public GHAppInstallation[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHAppInstallation[] previous() { + return base.previous().getInstallations(); + } + + @Override + public GHAppInstallation[] first() { + return base.first().getInstallations(); + } + + @Override + public GHAppInstallation[] last() { + return base.last().getInstallations(); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + public boolean hasNext() { + return base.hasNext(); + } + + public GHAppInstallation[] next() { + return base.next().getInstallations(); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GHArtifactsIterable.java b/src/main/java/org/kohsuke/github/GHArtifactsIterable.java index 2a574150cc..ecf582b86a 100644 --- a/src/main/java/org/kohsuke/github/GHArtifactsIterable.java +++ b/src/main/java/org/kohsuke/github/GHArtifactsIterable.java @@ -27,6 +27,15 @@ public GHArtifactsIterable(GHRepository owner, GitHubRequest.Builder requestB this.request = requestBuilder.build(); } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>( + adapt(GitHubPaginator + .create(owner.root().getClient(), GHArtifactsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -64,4 +73,48 @@ public GHArtifact[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHArtifact[] previous() { + return base.previous().getArtifacts(owner); + } + + @Override + public GHArtifact[] first() { + return base.first().getArtifacts(owner); + } + + @Override + public GHArtifact[] last() { + return base.last().getArtifacts(owner); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public GHArtifact[] next() { + return base.next().getArtifacts(owner); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java b/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java index 0866bd1f58..6117a676c8 100644 --- a/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java +++ b/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java @@ -27,6 +27,15 @@ public GHCheckRunsIterable(GHRepository owner, GitHubRequest request) { this.request = request; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>( + adapt(GitHubPaginator + .create(owner.root().getClient(), GHCheckRunsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -64,4 +73,48 @@ public GHCheckRun[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHCheckRun[] previous() { + return base.previous().getCheckRuns(owner); + } + + @Override + public GHCheckRun[] first() { + return base.first().getCheckRuns(owner); + } + + @Override + public GHCheckRun[] last() { + return base.last().getCheckRuns(owner); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public GHCheckRun[] next() { + return base.next().getCheckRuns(owner); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GHCommitFileIterable.java b/src/main/java/org/kohsuke/github/GHCommitFileIterable.java index 8a3f02a6fe..9b6684ca15 100644 --- a/src/main/java/org/kohsuke/github/GHCommitFileIterable.java +++ b/src/main/java/org/kohsuke/github/GHCommitFileIterable.java @@ -41,6 +41,12 @@ public GHCommitFileIterable(GHRepository owner, String sha, List files) { this.files = files != null ? files.toArray(new File[0]) : null; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + throw new UnsupportedOperationException(); + } + /** * Iterator. * diff --git a/src/main/java/org/kohsuke/github/GHCompare.java b/src/main/java/org/kohsuke/github/GHCompare.java index 08c6f051b1..8f93d697e9 100644 --- a/src/main/java/org/kohsuke/github/GHCompare.java +++ b/src/main/java/org/kohsuke/github/GHCompare.java @@ -3,6 +3,7 @@ import com.fasterxml.jackson.annotation.JacksonInject; import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import org.jetbrains.annotations.NotNull; import java.io.IOException; import java.net.URL; @@ -174,6 +175,12 @@ public PagedIterable listCommits() { } else { // if not using paginated commits, adapt the returned commits array return new PagedIterable() { + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + throw new UnsupportedOperationException("Paginator nor supported if not using paginated commits."); + } + @Nonnull @Override public PagedIterator _iterator(int pageSize) { @@ -373,6 +380,21 @@ class GHCompareCommitsIterable extends PagedIterable { public GHCompareCommitsIterable() { } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + GitHubRequest request = getRequest(); + + // page_size must be set for GHCompare commit pagination + if (pageSize == 0) { + pageSize = 10; + } + return new Paginator<>( + adapt(GitHubPaginator + .create(owner.root().getClient(), GHCompare.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -383,11 +405,7 @@ public GHCompareCommitsIterable() { @Nonnull @Override public PagedIterator _iterator(int pageSize) { - GitHubRequest request = owner.root() - .createRequest() - .injectMappingValue("GHCompare_usePaginatedCommits", usePaginatedCommits) - .withUrlPath(owner.getApiTailUrl(url.substring(url.lastIndexOf("/compare/")))) - .build(); + GitHubRequest request = getRequest(); // page_size must be set for GHCompare commit pagination if (pageSize == 0) { @@ -398,6 +416,14 @@ public PagedIterator _iterator(int pageSize) { item -> item.wrapUp(owner)); } + private GitHubRequest getRequest() { + return owner.root() + .createRequest() + .injectMappingValue("GHCompare_usePaginatedCommits", usePaginatedCommits) + .withUrlPath(owner.getApiTailUrl(url.substring(url.lastIndexOf("/compare/")))) + .build(); + } + /** * Adapt. * @@ -420,5 +446,49 @@ public Commit[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public Commit[] previous() { + return base.previous().commits; + } + + @Override + public Commit[] first() { + return base.first().commits; + } + + @Override + public Commit[] last() { + return base.last().commits; + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public Commit[] next() { + return base.next().commits; + } + }; + } } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java index 6ab751850d..5708d2abd9 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java @@ -27,6 +27,15 @@ public GHWorkflowJobsIterable(GHRepository repo, GitHubRequest request) { this.request = request; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>( + adapt(GitHubPaginator + .create(repo.root().getClient(), GHWorkflowJobsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -64,4 +73,48 @@ public GHWorkflowJob[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHWorkflowJob[] previous() { + return base.previous().getWorkflowJobs(repo); + } + + @Override + public GHWorkflowJob[] first() { + return base.first().getWorkflowJobs(repo); + } + + @Override + public GHWorkflowJob[] last() { + return base.last().getWorkflowJobs(repo); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public GHWorkflowJob[] next() { + return base.next().getWorkflowJobs(repo); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java index 4a525a83dc..285c3f05fa 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java @@ -27,6 +27,15 @@ public GHWorkflowRunsIterable(GHRepository owner, GitHubRequest.Builder reque this.request = requestBuilder.build(); } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>( + adapt(GitHubPaginator + .create(owner.root().getClient(), GHWorkflowRunsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -64,4 +73,55 @@ public GHWorkflowRun[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHWorkflowRun[] previous() { + GHWorkflowRunsPage v = base.previous(); + + return v.getWorkflowRuns(owner); + } + + @Override + public GHWorkflowRun[] first() { + GHWorkflowRunsPage v = base.first(); + + return v.getWorkflowRuns(owner); + } + + @Override + public GHWorkflowRun[] last() { + GHWorkflowRunsPage v = base.last(); + + return v.getWorkflowRuns(owner); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public GHWorkflowRun[] next() { + GHWorkflowRunsPage v = base.next(); + return v.getWorkflowRuns(owner); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java index 66d3d9480f..675dd7cabf 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java @@ -23,6 +23,19 @@ public GHWorkflowsIterable(GHRepository owner) { this.owner = owner; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + GitHubRequest request = owner.root() + .createRequest() + .withUrlPath(owner.getApiTailUrl("actions/workflows")) + .build(); + return new Paginator<>( + adapt(GitHubPaginator + .create(owner.root().getClient(), GHWorkflowsPage.class, request, pageSize, startPage)), + null); + } + /** * Iterator. * @@ -65,4 +78,48 @@ public GHWorkflow[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasPrevious(); + } + + @Override + public GHWorkflow[] previous() { + return base.previous().getWorkflows(owner); + } + + @Override + public GHWorkflow[] first() { + return base.first().getWorkflows(owner); + } + + @Override + public GHWorkflow[] last() { + return base.last().getWorkflows(owner); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public GHWorkflow[] next() { + return base.next().getWorkflows(owner); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java b/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java index 0af3fe96c7..3a3eec8723 100644 --- a/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java +++ b/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java @@ -46,6 +46,13 @@ class GitHubPageContentsIterable extends PagedIterable { this.itemInitializer = itemInitializer; } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>(GitHubPaginator.create(client, receiverType, request, pageSize, startPage), + itemInitializer); + } + /** * {@inheritDoc} */ diff --git a/src/main/java/org/kohsuke/github/GitHubPaginator.java b/src/main/java/org/kohsuke/github/GitHubPaginator.java new file mode 100644 index 0000000000..ca27e11692 --- /dev/null +++ b/src/main/java/org/kohsuke/github/GitHubPaginator.java @@ -0,0 +1,191 @@ +package org.kohsuke.github; + +import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; +import java.util.NoSuchElementException; + +public class GitHubPaginator implements NavigableIterator { + private final GitHubClient client; + private final Class type; + + private int currentPage; + private int finalPage; + private GitHubRequest currentRequest; + private boolean hasPrevious; + private boolean hasNext; + private boolean firstCallMade; + private GitHubPaginator(GitHubClient client, Class type, GitHubRequest request, int startPage) { + if (!"GET".equals(request.method())) { + throw new IllegalStateException("Request method \"GET\" is required for page iterator."); + } + + this.client = client; + this.type = type; + this.currentRequest = request; + + this.currentPage = startPage; + this.firstCallMade = false; + } + + static GitHubPaginator create(GitHubClient client, + Class type, + GitHubRequest request, + int pageSize, + int startPage) { + + if (pageSize > 0) { + GitHubRequest.Builder builder = request.toBuilder().with("per_page", pageSize); + request = builder.build(); + } + + if (startPage > 0) { + GitHubRequest.Builder builder = request.toBuilder().with("page", startPage); + request = builder.build(); + } + + return new GitHubPaginator<>(client, type, request, startPage); + } + + @Override + public boolean hasNext() { + if (!firstCallMade) { + makeRequest(); + } + return hasNext; + } + + public int totalPages() { + return finalPage; + } + + @Override + public T next() { + if (!firstCallMade) { + makeRequest(); + } + if (!hasNext) { + throw new NoSuchElementException(); + } + + currentRequest = currentRequest.toBuilder().with("page", String.valueOf(++currentPage)).build(); + return makeRequest(); + } + + private T makeRequest() { + URL url = currentRequest.url(); + try { + GitHubResponse response = client.sendRequest(currentRequest, + (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)); + assert response.body() != null; + + updateRequests(currentRequest, response); + firstCallMade = true; + return response.body(); + } catch (IOException e) { + // Iterators do not throw IOExceptions, so we wrap any IOException + // in a runtime GHException to bubble out if needed. + throw new GHException("Failed to retrieve " + url, e); + } + } + + @Override + public boolean hasPrevious() { + if (!firstCallMade) { + makeRequest(); + } + return hasPrevious; + } + + @Override + public T previous() { + if (!firstCallMade) { + makeRequest(); + } + if (!hasPrevious) { + throw new NoSuchElementException(); + } + currentRequest = currentRequest.toBuilder().with("page", String.valueOf(--currentPage)).build(); + return makeRequest(); + } + + @Override + public T first() { + if (!firstCallMade) { + makeRequest(); + } + currentPage = 1; + currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + return makeRequest(); + } + + @Override + public T last() { + if (!firstCallMade) { + makeRequest(); + } + currentPage = finalPage; + currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + return makeRequest(); + } + + @Override + public int totalCount() { + if (!firstCallMade) { + makeRequest(); + } + return finalPage; + } + + @Override + public int currentPage() { + return currentPage; + } + + private void updateRequests(GitHubRequest currentRequest, GitHubResponse currentResponse) { + hasPrevious = false; + hasNext = false; + finalPage = getPageFromUrl(currentRequest.url()); + + String link = currentResponse.header("Link"); + if (link != null) { + for (String token : link.split(", ")) { + if (token.endsWith("rel=\"next\"")) { + // found the next page. This should look something like + // ; rel="next" + hasNext = true; + } else if (token.endsWith("rel=\"prev\"")) { + // found the next page. This should look something like + // ; rel="prev" + hasPrevious = true; + } else if (token.endsWith("rel=\"last\"")) { + // found the next page. This should look something like + // ; rel="last" + int idx = token.indexOf('>'); + String url = token.substring(1, idx); + try { + finalPage = getPageFromUrl(new URI(url).toURL()); + } catch (URISyntaxException | MalformedURLException exception) { + throw new GHException(String.format("Unable to extract last page from url %s", url), exception); + } + } + } + } + + } + + private static int getPageFromUrl(URL url) { + String query = url.getQuery(); + String[] queryParams = query.split("&"); + for (String param : queryParams) { + String[] keyValue = param.split("="); + if (keyValue.length == 2 && keyValue[0].equals("page")) { + // Return the "page" parameter as an integer + return Integer.parseInt(keyValue[1]); + } + } + return 1; + } +} diff --git a/src/main/java/org/kohsuke/github/NavigableIterator.java b/src/main/java/org/kohsuke/github/NavigableIterator.java new file mode 100644 index 0000000000..b278b157ba --- /dev/null +++ b/src/main/java/org/kohsuke/github/NavigableIterator.java @@ -0,0 +1,34 @@ +package org.kohsuke.github; + +import java.util.Iterator; +import java.util.NoSuchElementException; + +public interface NavigableIterator extends Iterator { + /** + * Returns {@code true} if this list iterator has more elements when traversing the list in the reverse direction. + * (In other words, returns {@code true} if {@link #previous} would return an element rather than throwing an + * exception.) + * + * @return {@code true} if the list iterator has more elements when traversing the list in the reverse direction + */ + boolean hasPrevious(); + + /** + * Returns the previous element in the list and moves the cursor position backwards. This method may be called + * repeatedly to iterate through the list backwards, or intermixed with calls to {@link #next} to go back and forth. + * (Note that alternating calls to {@code next} and {@code previous} will return the same element repeatedly.) + * + * @return the previous element in the list + * @throws NoSuchElementException + * if the iteration has no previous element + */ + E previous(); + + E first(); + + E last(); + + int totalCount(); + + int currentPage(); +} diff --git a/src/main/java/org/kohsuke/github/PagedIterable.java b/src/main/java/org/kohsuke/github/PagedIterable.java index fc3528492e..eb9dac9efe 100644 --- a/src/main/java/org/kohsuke/github/PagedIterable.java +++ b/src/main/java/org/kohsuke/github/PagedIterable.java @@ -26,6 +26,8 @@ public abstract class PagedIterable implements Iterable { */ private int pageSize = 0; + private int startPage = 0; + /** * Sets the pagination size. * @@ -41,6 +43,11 @@ public PagedIterable withPageSize(int size) { return this; } + public PagedIterable withStartPage(int startPage) { + this.startPage = startPage; + return this; + } + /** * Returns an iterator over elements of type {@code T}. * @@ -51,6 +58,26 @@ public final PagedIterator iterator() { return _iterator(pageSize); } + /** + * Returns an iterator over elements of type {@code T}. + * + * @return an Iterator. + */ + @Nonnull + public final Paginator paginator() { + return _paginator(pageSize, startPage); + } + + /** + * Iterator over page items. + * + * @param pageSize + * the page size + * @return the paged iterator + */ + @Nonnull + public abstract Paginator _paginator(int pageSize, int startPage); + /** * Iterator over page items. * diff --git a/src/main/java/org/kohsuke/github/PagedSearchIterable.java b/src/main/java/org/kohsuke/github/PagedSearchIterable.java index 65f7442a2a..fb06c59ab2 100644 --- a/src/main/java/org/kohsuke/github/PagedSearchIterable.java +++ b/src/main/java/org/kohsuke/github/PagedSearchIterable.java @@ -58,6 +58,14 @@ public PagedSearchIterable withPageSize(int size) { return (PagedSearchIterable) super.withPageSize(size); } + @Nonnull + @Override + public Paginator _paginator(int pageSize, int startPage) { + return new Paginator<>( + adapt(GitHubPaginator.create(root.getClient(), receiverType, request, pageSize, startPage)), + null); + } + /** * Returns the total number of hit, including the results that's not yet fetched. * @@ -119,4 +127,48 @@ public T[] next() { } }; } + + protected NavigableIterator adapt(final NavigableIterator> base) { + return new NavigableIterator() { + @Override + public boolean hasPrevious() { + return base.hasNext(); + } + + @Override + public T[] previous() { + return base.previous().getItems(root); + } + + @Override + public T[] first() { + return base.first().getItems(root); + } + + @Override + public T[] last() { + return base.last().getItems(root); + } + + @Override + public int totalCount() { + return base.totalCount(); + } + + @Override + public int currentPage() { + return base.currentPage(); + } + + @Override + public boolean hasNext() { + return base.hasNext(); + } + + @Override + public T[] next() { + return base.next().getItems(root); + } + }; + } } diff --git a/src/main/java/org/kohsuke/github/Paginator.java b/src/main/java/org/kohsuke/github/Paginator.java new file mode 100644 index 0000000000..e09a3fa929 --- /dev/null +++ b/src/main/java/org/kohsuke/github/Paginator.java @@ -0,0 +1,218 @@ +package org.kohsuke.github; + +import java.util.Arrays; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Objects; +import java.util.function.Consumer; + +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; + +public class Paginator { + /** + * The base. + */ + @Nonnull + protected final NavigableIterator base; + + @CheckForNull + private final Consumer itemInitializer; + + private T[] currentPage; + private T[] nextPage; + private T[] previousPage; + + private int nextItemIndex; + + Paginator(@Nonnull NavigableIterator base, @CheckForNull Consumer itemInitializer) { + this.base = base; + this.itemInitializer = itemInitializer; + } + + protected void wrapUp(T[] page) { + if (itemInitializer != null) { + for (T item : page) { + itemInitializer.accept(item); + } + } + } + + public boolean hasPrevious() { + return (currentPage != null && nextItemIndex > 0) || previousPage != null || base.hasPrevious(); + } + + public T previous() { + if (!hasPrevious()) + throw new NoSuchElementException(); + if (currentPage != null && nextItemIndex > 0) + return currentPage[--nextItemIndex]; + if (previousPage == null) { + fetchPrevious(); + } + if (previousPage == null) { + throw new NoSuchElementException(); + } + nextItemIndex = previousPage.length - 1; + nextPage = currentPage; + currentPage = previousPage; + previousPage = null; + return currentPage[nextItemIndex]; + } + + public T first() { + nextItemIndex = 0; + nextPage = null; + currentPage = base.first(); + previousPage = null; + return currentPage[nextItemIndex++]; + } + + T[] firstPageArray() { + nextPage = null; + previousPage = null; + currentPage = base.first(); + nextItemIndex = currentPage.length; + return currentPage; + } + + public List firstPageList() { + return Arrays.asList(firstPageArray()); + } + + public T last() { + nextPage = null; + currentPage = base.last(); + nextItemIndex = currentPage.length - 1; + previousPage = null; + return currentPage[nextItemIndex++]; + } + + T[] lastPageArray() { + nextPage = null; + previousPage = null; + currentPage = base.last(); + nextItemIndex = currentPage.length; + return currentPage; + } + + public List lastPageList() { + return Arrays.asList(lastPageArray()); + } + + public boolean hasNext() { + return (currentPage != null && nextItemIndex < currentPage.length) || nextPage != null || base.hasNext(); + } + + public T next() { + if (!hasNext()) + throw new NoSuchElementException(); + if (currentPage != null && nextItemIndex < currentPage.length) + return currentPage[nextItemIndex++]; + if (nextPage == null) { + fetchNext(); + } + if (nextPage == null) { + throw new NoSuchElementException(); + } + nextItemIndex = 0; + previousPage = currentPage; + currentPage = nextPage; + nextPage = null; + return currentPage[nextItemIndex++]; + } + + public int totalPages() { + return base.totalCount(); + } + public int currentPage() { + return base.currentPage(); + } + + private void fetchNext() { + if (nextPage != null) { + return; + } + if (base.hasNext()) { + T[] result = Objects.requireNonNull(base.next()); + wrapUp(result); + nextPage = result; + } + } + + private void fetchPrevious() { + if (previousPage != null) { + return; + } + if (base.hasPrevious()) { + T[] result = Objects.requireNonNull(base.previous()); + wrapUp(result); + previousPage = result; + } + } + + /** + * Gets the next page worth of data. + * + * @return the list + */ + public List nextPage() { + return Arrays.asList(nextPageArray()); + } + + /** + * Gets the next page worth of data. + * + * @return the list + */ + @Nonnull + T[] nextPageArray() { + // if we have not fetched any pages yet, always fetch. + // If we have fetched at least one page, check hasNext() + if (!hasNext()) { + throw new NoSuchElementException(); + } + next(); + + // Current should never be null after fetch + Objects.requireNonNull(currentPage); + T[] r = currentPage; + if (nextItemIndex != 0) { + r = Arrays.copyOfRange(r, nextItemIndex - 1, r.length); + } + nextItemIndex = currentPage.length; + return r; + } + + /** + * Gets the next page worth of data. + * + * @return the list + */ + public List previousPage() { + return Arrays.asList(previousPageArray()); + } + + /** + * Gets the previous page worth of data. + * + * @return the list + */ + @Nonnull + T[] previousPageArray() { + // if we have not fetched any pages yet, always fetch. + // If we have fetched at least one page, check hasNext() + if (!hasPrevious()) { + throw new NoSuchElementException(); + } + previous(); + // Current should never be null after fetch + Objects.requireNonNull(currentPage); + T[] r = currentPage; + if (nextItemIndex != currentPage.length - 1) { + r = Arrays.copyOfRange(r, 0, nextItemIndex + 1); + } + nextItemIndex = 0; + return r; + } +} diff --git a/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java index 3dc0e9d7b0..719b8024ea 100644 --- a/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java +++ b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java @@ -126,7 +126,7 @@ private static void setRequestMethod(String method, HttpURLConnection connection $method.setAccessible(true); $method.set(connection, method); } catch (Exception x) { - throw (IOException) new IOException("Failed to set the custom verb").initCause(x); + throw (IOException) new IOException("Failed to set the custom verb", x); } // sun.net.www.protocol.https.DelegatingHttpsURLConnection delegates to another HttpURLConnection try { From 06807fac9678d3e50df9d340c4cae859056dfd14 Mon Sep 17 00:00:00 2001 From: Anuj Hydrabadi Date: Sat, 14 Oct 2023 01:37:04 +0530 Subject: [PATCH 2/5] bug fix in paginator, documentation, refactoring --- .../github/GHAppInstallationsIterable.java | 15 +- .../kohsuke/github/GHArtifactsIterable.java | 14 +- .../kohsuke/github/GHCheckRunsIterable.java | 14 +- .../java/org/kohsuke/github/GHCompare.java | 15 +- .../github/GHWorkflowJobsIterable.java | 14 +- .../github/GHWorkflowRunsIterable.java | 14 +- .../kohsuke/github/GHWorkflowsIterable.java | 14 +- .../org/kohsuke/github/GitHubPaginator.java | 130 +++++++-- .../org/kohsuke/github/NavigableIterator.java | 29 +- .../kohsuke/github/NavigablePageIterator.java | 40 +++ .../kohsuke/github/PagedSearchIterable.java | 14 +- .../java/org/kohsuke/github/Paginator.java | 248 ++++++++++++------ 12 files changed, 435 insertions(+), 126 deletions(-) create mode 100644 src/main/java/org/kohsuke/github/NavigablePageIterator.java diff --git a/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java b/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java index d101aa54e0..28420a4a73 100644 --- a/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java +++ b/src/main/java/org/kohsuke/github/GHAppInstallationsIterable.java @@ -75,8 +75,9 @@ public GHAppInstallation[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt( + final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -107,6 +108,16 @@ public int currentPage() { return base.currentPage(); } + @Override + public GHAppInstallation[] jumpToPage(int page) { + return base.jumpToPage(page).getInstallations(); + } + + @Override + public void refresh() { + base.refresh(); + } + public boolean hasNext() { return base.hasNext(); } diff --git a/src/main/java/org/kohsuke/github/GHArtifactsIterable.java b/src/main/java/org/kohsuke/github/GHArtifactsIterable.java index ecf582b86a..a8e3be0895 100644 --- a/src/main/java/org/kohsuke/github/GHArtifactsIterable.java +++ b/src/main/java/org/kohsuke/github/GHArtifactsIterable.java @@ -74,8 +74,8 @@ public GHArtifact[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -115,6 +115,16 @@ public boolean hasNext() { public GHArtifact[] next() { return base.next().getArtifacts(owner); } + + @Override + public GHArtifact[] jumpToPage(int page) { + return base.jumpToPage(page).getArtifacts(owner); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java b/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java index 6117a676c8..03478652db 100644 --- a/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java +++ b/src/main/java/org/kohsuke/github/GHCheckRunsIterable.java @@ -74,8 +74,8 @@ public GHCheckRun[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -115,6 +115,16 @@ public boolean hasNext() { public GHCheckRun[] next() { return base.next().getCheckRuns(owner); } + + @Override + public GHCheckRun[] jumpToPage(int page) { + return base.jumpToPage(page).getCheckRuns(owner); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GHCompare.java b/src/main/java/org/kohsuke/github/GHCompare.java index 8f93d697e9..cc1b296387 100644 --- a/src/main/java/org/kohsuke/github/GHCompare.java +++ b/src/main/java/org/kohsuke/github/GHCompare.java @@ -3,7 +3,6 @@ import com.fasterxml.jackson.annotation.JacksonInject; import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -import org.jetbrains.annotations.NotNull; import java.io.IOException; import java.net.URL; @@ -447,8 +446,8 @@ public Commit[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -488,6 +487,16 @@ public boolean hasNext() { public Commit[] next() { return base.next().commits; } + + @Override + public Commit[] jumpToPage(int page) { + return base.jumpToPage(page).commits; + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java index 5708d2abd9..a7fd3fa987 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowJobsIterable.java @@ -74,8 +74,8 @@ public GHWorkflowJob[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -115,6 +115,16 @@ public boolean hasNext() { public GHWorkflowJob[] next() { return base.next().getWorkflowJobs(repo); } + + @Override + public GHWorkflowJob[] jumpToPage(int page) { + return base.jumpToPage(page).getWorkflowJobs(repo); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java index 285c3f05fa..e0bf3f510b 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowRunsIterable.java @@ -74,8 +74,8 @@ public GHWorkflowRun[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -122,6 +122,16 @@ public GHWorkflowRun[] next() { GHWorkflowRunsPage v = base.next(); return v.getWorkflowRuns(owner); } + + @Override + public GHWorkflowRun[] jumpToPage(int page) { + return base.jumpToPage(page).getWorkflowRuns(owner); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java b/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java index 675dd7cabf..1053701794 100644 --- a/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java +++ b/src/main/java/org/kohsuke/github/GHWorkflowsIterable.java @@ -79,8 +79,8 @@ public GHWorkflow[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasPrevious(); @@ -120,6 +120,16 @@ public boolean hasNext() { public GHWorkflow[] next() { return base.next().getWorkflows(owner); } + + @Override + public GHWorkflow[] jumpToPage(int page) { + return base.jumpToPage(page).getWorkflows(owner); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/GitHubPaginator.java b/src/main/java/org/kohsuke/github/GitHubPaginator.java index ca27e11692..6133fb43bc 100644 --- a/src/main/java/org/kohsuke/github/GitHubPaginator.java +++ b/src/main/java/org/kohsuke/github/GitHubPaginator.java @@ -7,15 +7,34 @@ import java.net.URL; import java.util.NoSuchElementException; -public class GitHubPaginator implements NavigableIterator { +/** + * May be used for any item that has pagination information. Iterates over paginated {@link T} objects (not the items + * inside the page). Equivalent to {@link GitHubPageIterator} but with increased functionality, supporting bidirectional + * movement and jumping to first, last or any specific page. + *

+ * Works for array responses, also works for search results which are single instances with an array of items inside. + *

+ * This class is not thread-safe. Any one instance should only be called from a single thread. + * + * @author Anuj Hydrabadi + * @param + * type of each page (not the items in the page). + */ +public class GitHubPaginator implements NavigablePageIterator { private final GitHubClient client; private final Class type; + /** Current page number. */ private int currentPage; + /** Total pages. */ private int finalPage; + /** The latest request that was sent. */ private GitHubRequest currentRequest; + /** Whether there is a previous page. Refreshed every time a request is made. */ private boolean hasPrevious; + /** Whether there is a next page. Refreshed every time a request is made. */ private boolean hasNext; + /** Whether at least one API call is made. Starts as false when object is created, once set to true, stays true. */ private boolean firstCallMade; private GitHubPaginator(GitHubClient client, Class type, GitHubRequest request, int startPage) { if (!"GET".equals(request.method())) { @@ -30,6 +49,23 @@ private GitHubPaginator(GitHubClient client, Class type, GitHubRequest reques this.firstCallMade = false; } + /** + * Loads paginated resources. + * + * @param client + * the {@link GitHubClient} from which to request responses + * @param type + * type of each page (not the items in the page). + * @param request + * the request + * @param pageSize + * the page size + * @param startPage + * the page to start from + * @return the paginator + * @param + * type of each page (not the items in the page). + */ static GitHubPaginator create(GitHubClient client, Class type, GitHubRequest request, @@ -49,6 +85,7 @@ static GitHubPaginator create(GitHubClient client, return new GitHubPaginator<>(client, type, request, startPage); } + /** {@inheritDoc} */ @Override public boolean hasNext() { if (!firstCallMade) { @@ -57,10 +94,7 @@ public boolean hasNext() { return hasNext; } - public int totalPages() { - return finalPage; - } - + /** {@inheritDoc} */ @Override public T next() { if (!firstCallMade) { @@ -74,23 +108,7 @@ public T next() { return makeRequest(); } - private T makeRequest() { - URL url = currentRequest.url(); - try { - GitHubResponse response = client.sendRequest(currentRequest, - (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)); - assert response.body() != null; - - updateRequests(currentRequest, response); - firstCallMade = true; - return response.body(); - } catch (IOException e) { - // Iterators do not throw IOExceptions, so we wrap any IOException - // in a runtime GHException to bubble out if needed. - throw new GHException("Failed to retrieve " + url, e); - } - } - + /** {@inheritDoc} */ @Override public boolean hasPrevious() { if (!firstCallMade) { @@ -99,6 +117,7 @@ public boolean hasPrevious() { return hasPrevious; } + /** {@inheritDoc} */ @Override public T previous() { if (!firstCallMade) { @@ -111,6 +130,7 @@ public T previous() { return makeRequest(); } + /** {@inheritDoc} */ @Override public T first() { if (!firstCallMade) { @@ -121,6 +141,7 @@ public T first() { return makeRequest(); } + /** {@inheritDoc} */ @Override public T last() { if (!firstCallMade) { @@ -131,6 +152,7 @@ public T last() { return makeRequest(); } + /** {@inheritDoc} */ @Override public int totalCount() { if (!firstCallMade) { @@ -139,12 +161,61 @@ public int totalCount() { return finalPage; } + /** {@inheritDoc} */ @Override public int currentPage() { return currentPage; } - private void updateRequests(GitHubRequest currentRequest, GitHubResponse currentResponse) { + /** {@inheritDoc} */ + @Override + public T jumpToPage(int page) { + if (page < 1 || page > finalPage) { + throw new NoSuchElementException(); + } + currentPage = page; + currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + return makeRequest(); + } + + /** {@inheritDoc} */ + @Override + public void refresh() { + makeRequest(); + } + + /** + * Make the request specified in {@link #currentRequest}, and update all other state variables of the class. + * + * @return the response + */ + private T makeRequest() { + URL url = currentRequest.url(); + try { + GitHubResponse response = client.sendRequest(currentRequest, + (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)); + assert response.body() != null; + + updateState(currentRequest, response); + firstCallMade = true; + return response.body(); + } catch (IOException e) { + // Iterators do not throw IOExceptions, so we wrap any IOException + // in a runtime GHException to bubble out if needed. + throw new GHException("Failed to retrieve " + url, e); + } + } + + /** + * Called after every request is made. Updates the state of the class comprising three fields: ({@link #hasNext}, + * {@link #hasPrevious}, {@link #finalPage}), based on the "Link" header. + * + * @param currentRequest + * the request just made. + * @param currentResponse + * the response just received. + */ + private void updateState(GitHubRequest currentRequest, GitHubResponse currentResponse) { hasPrevious = false; hasNext = false; finalPage = getPageFromUrl(currentRequest.url()); @@ -157,12 +228,12 @@ private void updateRequests(GitHubRequest currentRequest, GitHubResponse curr // ; rel="next" hasNext = true; } else if (token.endsWith("rel=\"prev\"")) { - // found the next page. This should look something like - // ; rel="prev" + // found the previous page. This should look something like + // ; rel="prev" hasPrevious = true; } else if (token.endsWith("rel=\"last\"")) { - // found the next page. This should look something like - // ; rel="last" + // found the last page. This should look something like + // ; rel="last" int idx = token.indexOf('>'); String url = token.substring(1, idx); try { @@ -176,6 +247,11 @@ private void updateRequests(GitHubRequest currentRequest, GitHubResponse curr } + /** + * @param url + * of type "https://api.github.com/repos?page=42&per_page=100" + * @return the value of the "page" param from the url as int if present, else 1. + */ private static int getPageFromUrl(URL url) { String query = url.getQuery(); String[] queryParams = query.split("&"); diff --git a/src/main/java/org/kohsuke/github/NavigableIterator.java b/src/main/java/org/kohsuke/github/NavigableIterator.java index b278b157ba..1f31bc4534 100644 --- a/src/main/java/org/kohsuke/github/NavigableIterator.java +++ b/src/main/java/org/kohsuke/github/NavigableIterator.java @@ -3,10 +3,17 @@ import java.util.Iterator; import java.util.NoSuchElementException; +/** + * Extension to {@link Iterator} supporting bidirectional iteration and also jumping to first or last entry. + * + * @author Anuj Hydrabadi + * @param + * the type of the page of the data. + */ public interface NavigableIterator extends Iterator { /** - * Returns {@code true} if this list iterator has more elements when traversing the list in the reverse direction. - * (In other words, returns {@code true} if {@link #previous} would return an element rather than throwing an + * Returns {@code true} if this iterator has more elements when traversing the list in the reverse direction. (In + * other words, returns {@code true} if {@link #previous} would return an element rather than throwing an * exception.) * * @return {@code true} if the list iterator has more elements when traversing the list in the reverse direction @@ -24,11 +31,19 @@ public interface NavigableIterator extends Iterator { */ E previous(); - E first(); - + /** + * Returns the last element in the list and sets the cursor after the last element, such that a call to + * {@link #hasNext()} returns false, and a call to {@link #previous()} returns the last element again. + * + * @return the last element in the list. + */ E last(); - int totalCount(); - - int currentPage(); + /** + * Returns the first element in the list and sets the cursor onto the second element, such that a call to + * {@link #next()} returns the second element, and a call to {@link #previous()} returns the first element again. + * + * @return the first element in the list. + */ + E first(); } diff --git a/src/main/java/org/kohsuke/github/NavigablePageIterator.java b/src/main/java/org/kohsuke/github/NavigablePageIterator.java new file mode 100644 index 0000000000..97d3e1c8c4 --- /dev/null +++ b/src/main/java/org/kohsuke/github/NavigablePageIterator.java @@ -0,0 +1,40 @@ +package org.kohsuke.github; + +/** + * Extension to {@link NavigableIterator} for iterating over a list of pages of data. + * + * @author Anuj Hydrabadi + * @param + * the type of the page of the data. + */ +public interface NavigablePageIterator extends NavigableIterator { + + /** + * Get the total number of pages. + * + * @return the total number of pages + */ + int totalCount(); + + /** + * Get the current page number. + * + * @return the current page number. + */ + int currentPage(); + + /** + * Jump the cursor to a specific page. + * + * @param page + * the page number + * @return the page. + */ + E jumpToPage(int page); + + /** + * Refresh stale data. Needed when the underlying data has changed but is not reflected due to caching in the + * implementation. + */ + void refresh(); +} diff --git a/src/main/java/org/kohsuke/github/PagedSearchIterable.java b/src/main/java/org/kohsuke/github/PagedSearchIterable.java index fb06c59ab2..f536a6696f 100644 --- a/src/main/java/org/kohsuke/github/PagedSearchIterable.java +++ b/src/main/java/org/kohsuke/github/PagedSearchIterable.java @@ -128,8 +128,8 @@ public T[] next() { }; } - protected NavigableIterator adapt(final NavigableIterator> base) { - return new NavigableIterator() { + protected NavigablePageIterator adapt(final NavigablePageIterator> base) { + return new NavigablePageIterator() { @Override public boolean hasPrevious() { return base.hasNext(); @@ -169,6 +169,16 @@ public boolean hasNext() { public T[] next() { return base.next().getItems(root); } + + @Override + public T[] jumpToPage(int page) { + return base.jumpToPage(page).getItems(root); + } + + @Override + public void refresh() { + base.refresh(); + } }; } } diff --git a/src/main/java/org/kohsuke/github/Paginator.java b/src/main/java/org/kohsuke/github/Paginator.java index e09a3fa929..810e10e6a4 100644 --- a/src/main/java/org/kohsuke/github/Paginator.java +++ b/src/main/java/org/kohsuke/github/Paginator.java @@ -8,28 +8,59 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; - -public class Paginator { +/** + * Iterator over a paginated data source. Iterates over the content items of each page, automatically requesting new + * pages as needed. + *

+ * Equivalent to {@link PagedIterator} but with support for bidirectional movement and jumping to first, last or any + * specific page. This class is not thread-safe. Any one instance should only be called from a single thread. + * + * @author Anuj Hydrabadi + * @param + * the type parameter + */ +public class Paginator implements NavigableIterator { /** * The base. */ @Nonnull - protected final NavigableIterator base; + protected final NavigablePageIterator base; @CheckForNull private final Consumer itemInitializer; + /** + * Current batch of items. This field, long with {@link #nextItemIndex} maintains the state of the class. If + * {@link #nextItemIndex} moves out of bounds of this array, the next/previous batch of data is fetched and replaced + * here. + */ private T[] currentPage; - private T[] nextPage; - private T[] previousPage; + /** + * The index of the next item to be fetched from the {@link #currentPage}. + */ private int nextItemIndex; - Paginator(@Nonnull NavigableIterator base, @CheckForNull Consumer itemInitializer) { + /** + * Instantiates a new paginator. + * + * @param base + * the base + * @param itemInitializer + * the item initializer + */ + Paginator(@Nonnull NavigablePageIterator base, @CheckForNull Consumer itemInitializer) { this.base = base; this.itemInitializer = itemInitializer; } + /** + * This method initializes items with local data after they are fetched. It is up to the implementer to decide what + * local data to apply. + * + * @param page + * the page of items to be initialized + */ protected void wrapUp(T[] page) { if (itemInitializer != null) { for (T item : page) { @@ -38,121 +69,144 @@ protected void wrapUp(T[] page) { } } + /** + * @return true if there is any data on the next index. + */ + @Override + public boolean hasNext() { + return (currentPage != null && nextItemIndex < currentPage.length) || base.hasNext(); + } + + /** + * Increments the pointer and returns the next item. + * + * @return the item on the next index if present + * @throws NoSuchElementException + * if not present + */ + @Override + public T next() { + if (!hasNext()) + throw new NoSuchElementException(); + if (currentPage != null && nextItemIndex < currentPage.length) + return currentPage[nextItemIndex++]; + fetchNext(); + if (currentPage == null) { + throw new NoSuchElementException(); + } + nextItemIndex = 0; + return currentPage[nextItemIndex++]; + } + + /** + * @return true if there is any data on the previous index. + */ + @Override public boolean hasPrevious() { - return (currentPage != null && nextItemIndex > 0) || previousPage != null || base.hasPrevious(); + return (currentPage != null && nextItemIndex > 0) || base.hasPrevious(); } + /** + * Decrements the pointer and returns the previous item. + * + * @return the item on the previous index if present + * @throws NoSuchElementException + * if not present + */ + @Override public T previous() { if (!hasPrevious()) throw new NoSuchElementException(); if (currentPage != null && nextItemIndex > 0) return currentPage[--nextItemIndex]; - if (previousPage == null) { - fetchPrevious(); - } - if (previousPage == null) { + fetchPrevious(); + if (currentPage == null) { throw new NoSuchElementException(); } - nextItemIndex = previousPage.length - 1; - nextPage = currentPage; - currentPage = previousPage; - previousPage = null; + nextItemIndex = currentPage.length - 1; return currentPage[nextItemIndex]; } + /** + * Returns the first item and sets the pointer after that item. + * + * @return the first item. + */ + @Override public T first() { nextItemIndex = 0; - nextPage = null; currentPage = base.first(); - previousPage = null; return currentPage[nextItemIndex++]; } + /** + * Gets the entire first page of data. Sets the pointer to the end of the first page. + * + * @return the list + */ T[] firstPageArray() { - nextPage = null; - previousPage = null; currentPage = base.first(); nextItemIndex = currentPage.length; return currentPage; } + /** + * Gets the entire first page of data. Sets the pointer to the end of the first page. + * + * @return the list + */ public List firstPageList() { return Arrays.asList(firstPageArray()); } + /** + * Returns the last item and sets the pointer after that item. + * + * @return the last item. + */ + @Override public T last() { - nextPage = null; currentPage = base.last(); nextItemIndex = currentPage.length - 1; - previousPage = null; return currentPage[nextItemIndex++]; } + /** + * Gets the entire last page of data. Sets the pointer to the end of the last page. + * + * @return the list + */ T[] lastPageArray() { - nextPage = null; - previousPage = null; currentPage = base.last(); nextItemIndex = currentPage.length; return currentPage; } + /** + * Gets the entire last page of data. Sets the pointer to the end of the last page. + * + * @return the list + */ public List lastPageList() { return Arrays.asList(lastPageArray()); } - public boolean hasNext() { - return (currentPage != null && nextItemIndex < currentPage.length) || nextPage != null || base.hasNext(); - } - - public T next() { - if (!hasNext()) - throw new NoSuchElementException(); - if (currentPage != null && nextItemIndex < currentPage.length) - return currentPage[nextItemIndex++]; - if (nextPage == null) { - fetchNext(); - } - if (nextPage == null) { - throw new NoSuchElementException(); - } - nextItemIndex = 0; - previousPage = currentPage; - currentPage = nextPage; - nextPage = null; - return currentPage[nextItemIndex++]; - } - + /** + * @return total number of pages + */ public int totalPages() { return base.totalCount(); } + + /** + * @return the current page number + */ public int currentPage() { return base.currentPage(); } - private void fetchNext() { - if (nextPage != null) { - return; - } - if (base.hasNext()) { - T[] result = Objects.requireNonNull(base.next()); - wrapUp(result); - nextPage = result; - } - } - - private void fetchPrevious() { - if (previousPage != null) { - return; - } - if (base.hasPrevious()) { - T[] result = Objects.requireNonNull(base.previous()); - wrapUp(result); - previousPage = result; - } - } - /** - * Gets the next page worth of data. + * Gets the next page worth of data. Sets the pointer to the end of that page. * * @return the list */ @@ -161,14 +215,12 @@ public List nextPage() { } /** - * Gets the next page worth of data. + * Gets the next page worth of data. Sets the pointer to the end of that page. * * @return the list */ @Nonnull T[] nextPageArray() { - // if we have not fetched any pages yet, always fetch. - // If we have fetched at least one page, check hasNext() if (!hasNext()) { throw new NoSuchElementException(); } @@ -185,7 +237,7 @@ T[] nextPageArray() { } /** - * Gets the next page worth of data. + * Gets the previous page worth of data. Sets the pointer to the start of that page. * * @return the list */ @@ -194,14 +246,12 @@ public List previousPage() { } /** - * Gets the previous page worth of data. + * Gets the previous page worth of data. Sets the pointer to the start of that page. * * @return the list */ @Nonnull T[] previousPageArray() { - // if we have not fetched any pages yet, always fetch. - // If we have fetched at least one page, check hasNext() if (!hasPrevious()) { throw new NoSuchElementException(); } @@ -215,4 +265,52 @@ T[] previousPageArray() { nextItemIndex = 0; return r; } + + /** + * Clears out the cached data and updates it with the underlying data source. The position of the pointer stays the + * same, though the data it is pointing to may have changed. + */ + public void refresh() { + base.refresh(); + currentPage = null; + } + + /** + * Jump to a particular page. + * + * @param page + * the page to jump to. + * @return the paginator object to support fluent method chaining. + * @throws NoSuchElementException + * if the page does not exist. + */ + public Paginator jumpToPage(int page) { + currentPage = base.jumpToPage(page); + nextItemIndex = 0; + return this; + } + + /** + * Called at the start of the {@link #next()} method if we have reached the end of {@link #currentPage}. Updates the + * current page with the next page data. Does not update the pointer. + */ + private void fetchNext() { + if (base.hasNext()) { + T[] result = Objects.requireNonNull(base.next()); + wrapUp(result); + currentPage = result; + } + } + + /** + * Called at the start of the {@link #previous()} method if we are at the start of {@link #currentPage}. Updates the + * current page with the previous page data. Does not update the pointer. + */ + private void fetchPrevious() { + if (base.hasPrevious()) { + T[] result = Objects.requireNonNull(base.previous()); + wrapUp(result); + currentPage = result; + } + } } From b4deb7178eaae3407a49e3a74afc896b65df9009 Mon Sep 17 00:00:00 2001 From: Anuj Hydrabadi Date: Sat, 14 Oct 2023 03:53:29 +0530 Subject: [PATCH 3/5] bug fixes --- .../org/kohsuke/github/GitHubPaginator.java | 12 +- .../org/kohsuke/github/PagedIterable.java | 10 +- .../github/GHWorkflowRunPaginationTest.java | 160 ++++++++++++++++++ 3 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java diff --git a/src/main/java/org/kohsuke/github/GitHubPaginator.java b/src/main/java/org/kohsuke/github/GitHubPaginator.java index 6133fb43bc..0ac07a252d 100644 --- a/src/main/java/org/kohsuke/github/GitHubPaginator.java +++ b/src/main/java/org/kohsuke/github/GitHubPaginator.java @@ -78,7 +78,7 @@ static GitHubPaginator create(GitHubClient client, } if (startPage > 0) { - GitHubRequest.Builder builder = request.toBuilder().with("page", startPage); + GitHubRequest.Builder builder = request.toBuilder().set("page", startPage); request = builder.build(); } @@ -104,7 +104,7 @@ public T next() { throw new NoSuchElementException(); } - currentRequest = currentRequest.toBuilder().with("page", String.valueOf(++currentPage)).build(); + currentRequest = currentRequest.toBuilder().set("page", String.valueOf(++currentPage)).build(); return makeRequest(); } @@ -126,7 +126,7 @@ public T previous() { if (!hasPrevious) { throw new NoSuchElementException(); } - currentRequest = currentRequest.toBuilder().with("page", String.valueOf(--currentPage)).build(); + currentRequest = currentRequest.toBuilder().set("page", String.valueOf(--currentPage)).build(); return makeRequest(); } @@ -137,7 +137,7 @@ public T first() { makeRequest(); } currentPage = 1; - currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); return makeRequest(); } @@ -148,7 +148,7 @@ public T last() { makeRequest(); } currentPage = finalPage; - currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); return makeRequest(); } @@ -174,7 +174,7 @@ public T jumpToPage(int page) { throw new NoSuchElementException(); } currentPage = page; - currentRequest = currentRequest.toBuilder().with("page", String.valueOf(currentPage)).build(); + currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); return makeRequest(); } diff --git a/src/main/java/org/kohsuke/github/PagedIterable.java b/src/main/java/org/kohsuke/github/PagedIterable.java index eb9dac9efe..7a52ca3d0c 100644 --- a/src/main/java/org/kohsuke/github/PagedIterable.java +++ b/src/main/java/org/kohsuke/github/PagedIterable.java @@ -26,7 +26,8 @@ public abstract class PagedIterable implements Iterable { */ private int pageSize = 0; - private int startPage = 0; + /** Start page. 1 by default. */ + private int startPage = 1; /** * Sets the pagination size. @@ -43,6 +44,13 @@ public PagedIterable withPageSize(int size) { return this; } + /** + * Sets the page size. 1 by default. + * + * @param startPage + * start page to set + * @return the paged iterable + */ public PagedIterable withStartPage(int startPage) { this.startPage = startPage; return this; diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java new file mode 100644 index 0000000000..177d80b823 --- /dev/null +++ b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java @@ -0,0 +1,160 @@ +package org.kohsuke.github; + +import org.junit.Before; +import org.junit.Test; + +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.notNullValue; + +public class GHWorkflowRunPaginationTest extends AbstractGitHubWireMockTest { + private static final String REPO_NAME = "hub4j/github-api"; + private static final String SINGLE_RUN_REPO_NAME = "hub4j-test-org/GHWorkflowRunTest"; + private GHRepository repo; + + @Before + public void setUp() throws Exception { + repo = gitHub.getRepository(REPO_NAME); + } + + @Test + public void testBasicPagination() throws Exception { + Paginator paginator = repo.getWorkflow("maven-build.yml") + .listRuns() + .withStartPage(6) + .withPageSize(6) + .paginator(); + + // total pages + assertThat(paginator.totalPages(), greaterThan(0)); + + // current page + assertThat(paginator.currentPage(), equalTo(6)); + + // last + assertThat(paginator.last().getRunNumber(), equalTo(3084L)); + assertThat(paginator.hasNext(), equalTo(false)); + assertThat(paginator.hasPrevious(), equalTo(true)); + + // first, and moving around + GHWorkflowRun first = paginator.first(); + assertThat(first, notNullValue()); + assertThat(paginator.hasNext(), equalTo(true)); + assertThat(paginator.hasPrevious(), equalTo(true)); + assertThat(paginator.previous().getRunNumber(), equalTo(first.getRunNumber())); + assertThat(paginator.hasPrevious(), equalTo(false)); + assertThat(paginator.hasNext(), equalTo(true)); + assertThat(paginator.nextPage().size(), equalTo(6)); + assertThat(paginator.previousPage().size(), equalTo(6)); + assertThat(paginator.hasPrevious(), equalTo(false)); + assertThat(paginator.hasNext(), equalTo(true)); + + // starting at 0 index, check previousPage size from middle of the page + paginator.next(); + paginator.next(); + assertThat(paginator.previousPage().size(), equalTo(2)); + + // starting at 0 index, check nextPage size from middle of the page + paginator.next(); + paginator.next(); + assertThat(paginator.nextPage().size(), equalTo(4)); + + // next page + int pageNumber = paginator.currentPage(); + for (int i = 0; i < 4; i++) { + List page = paginator.nextPage(); + assertThat(page.size(), equalTo(6)); + assertThat(paginator.currentPage(), equalTo(++pageNumber)); + } + + // previous page + pageNumber = paginator.currentPage(); + for (int i = 0; i < 4; i++) { + List page = paginator.previousPage(); + assertThat(page.size(), equalTo(6)); + assertThat(paginator.currentPage(), equalTo(pageNumber--)); + } + + // next and previous over multiple pages + long[] ascending = new long[14]; + long[] descending = new long[14]; + for (int i = 0; i < 14; i++) { + ascending[i] = paginator.next().getRunNumber(); + } + for (int i = 13; i >= 0; i--) { + descending[i] = paginator.previous().getRunNumber(); + } + assertThat(ascending, equalTo(descending)); + + // jump to page + assertThat(paginator.jumpToPage(4).currentPage(), equalTo(4)); + assertThat(paginator.jumpToPage(8).currentPage(), equalTo(8)); + assertThat(paginator.jumpToPage(6).currentPage(), equalTo(6)); + + // first page list vs jump to page 1 + assertThat(getRunNumbers(paginator.firstPageList()), + equalTo(getRunNumbers(paginator.jumpToPage(1).nextPage()))); + + // last page list vs jump to page number totalPages + assertThat(getRunNumbers(paginator.lastPageList()), + equalTo(getRunNumbers(paginator.jumpToPage(paginator.totalPages()).nextPage()))); + } + + @Test + public void testRepoWithSingleRun() throws Exception { + Paginator paginator = gitHub.getRepository(SINGLE_RUN_REPO_NAME) + .queryWorkflowRuns() + .list() + .withPageSize(6) + .paginator(); + // total pages + assertThat(paginator.totalPages(), equalTo(1)); + + // current page + assertThat(paginator.currentPage(), equalTo(1)); + + // last + assertThat(paginator.last().getRunNumber(), equalTo(78L)); + assertThat(paginator.hasNext(), equalTo(false)); + assertThat(paginator.hasPrevious(), equalTo(true)); + + // first + assertThat(paginator.first().getRunNumber(), equalTo(78L)); + assertThat(paginator.hasNext(), equalTo(false)); + assertThat(paginator.hasPrevious(), equalTo(true)); + + // pages + List list = Collections.singletonList(78L); + assertThat(getRunNumbers(paginator.lastPageList()), equalTo(list)); + assertThat(getRunNumbers(paginator.firstPageList()), equalTo(list)); + assertThat(getRunNumbers(paginator.jumpToPage(1).nextPage()), equalTo(list)); + + // previous + assertThat(paginator.hasPrevious(), equalTo(true)); + assertThat(paginator.hasNext(), equalTo(false)); + assertThat(paginator.previous().getRunNumber(), equalTo(78L)); + + // next + assertThat(paginator.hasPrevious(), equalTo(false)); + assertThat(paginator.hasNext(), equalTo(true)); + assertThat(paginator.next().getRunNumber(), equalTo(78L)); + + // previous page + assertThat(getRunNumbers(paginator.previousPage()), equalTo(list)); + assertThat(paginator.hasPrevious(), equalTo(false)); + assertThat(paginator.hasNext(), equalTo(true)); + + // next page + assertThat(getRunNumbers(paginator.nextPage()), equalTo(list)); + assertThat(paginator.hasPrevious(), equalTo(true)); + assertThat(paginator.hasNext(), equalTo(false)); + } + + private static List getRunNumbers(List runs) { + return runs.stream().map(GHWorkflowRun::getRunNumber).collect(Collectors.toList()); + } +} From 5b6e1d910d0ec7a8d7ebce6cfebbb34c7296c44b Mon Sep 17 00:00:00 2001 From: Anuj Hydrabadi Date: Thu, 19 Oct 2023 20:37:13 +0530 Subject: [PATCH 4/5] bug fixes, more tests --- .../org/kohsuke/github/GitHubPaginator.java | 63 +++++---- .../java/org/kohsuke/github/Paginator.java | 20 ++- .../java/org/kohsuke/github/CommitTest.java | 14 ++ .../github/GHWorkflowRunPaginationTest.java | 124 +++++++++++++++++- 4 files changed, 185 insertions(+), 36 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHubPaginator.java b/src/main/java/org/kohsuke/github/GitHubPaginator.java index 0ac07a252d..16418fbcd8 100644 --- a/src/main/java/org/kohsuke/github/GitHubPaginator.java +++ b/src/main/java/org/kohsuke/github/GitHubPaginator.java @@ -36,6 +36,7 @@ public class GitHubPaginator implements NavigablePageIterator { private boolean hasNext; /** Whether at least one API call is made. Starts as false when object is created, once set to true, stays true. */ private boolean firstCallMade; + private T next; private GitHubPaginator(GitHubClient client, Class type, GitHubRequest request, int startPage) { if (!"GET".equals(request.method())) { throw new IllegalStateException("Request method \"GET\" is required for page iterator."); @@ -88,77 +89,71 @@ static GitHubPaginator create(GitHubClient client, /** {@inheritDoc} */ @Override public boolean hasNext() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); return hasNext; } /** {@inheritDoc} */ @Override public T next() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); if (!hasNext) { throw new NoSuchElementException(); } + if (next != null) + return next; currentRequest = currentRequest.toBuilder().set("page", String.valueOf(++currentPage)).build(); - return makeRequest(); + return makeRequest(false); } /** {@inheritDoc} */ @Override public boolean hasPrevious() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); return hasPrevious; } /** {@inheritDoc} */ @Override public T previous() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); if (!hasPrevious) { throw new NoSuchElementException(); } currentRequest = currentRequest.toBuilder().set("page", String.valueOf(--currentPage)).build(); - return makeRequest(); + return makeRequest(false); } /** {@inheritDoc} */ @Override public T first() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); currentPage = 1; currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); - return makeRequest(); + return makeRequest(false); } /** {@inheritDoc} */ @Override public T last() { - if (!firstCallMade) { - makeRequest(); - } + initialise(); currentPage = finalPage; currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); - return makeRequest(); + return makeRequest(false); } /** {@inheritDoc} */ @Override public int totalCount() { + initialise(); + return finalPage; + } + + private void initialise() { if (!firstCallMade) { - makeRequest(); + makeRequest(true); } - return finalPage; } /** {@inheritDoc} */ @@ -170,18 +165,22 @@ public int currentPage() { /** {@inheritDoc} */ @Override public T jumpToPage(int page) { + initialise(); if (page < 1 || page > finalPage) { throw new NoSuchElementException(); } currentPage = page; currentRequest = currentRequest.toBuilder().set("page", String.valueOf(currentPage)).build(); - return makeRequest(); + return makeRequest(false); } /** {@inheritDoc} */ @Override public void refresh() { - makeRequest(); + if (!firstCallMade) { + throw new GHException("Cannot refresh before the first call has been made!"); + } + makeRequest(false); } /** @@ -189,14 +188,14 @@ public void refresh() { * * @return the response */ - private T makeRequest() { + private T makeRequest(boolean firstCall) { URL url = currentRequest.url(); try { GitHubResponse response = client.sendRequest(currentRequest, (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)); assert response.body() != null; - updateState(currentRequest, response); + updateState(currentRequest, response, firstCall); firstCallMade = true; return response.body(); } catch (IOException e) { @@ -215,7 +214,7 @@ private T makeRequest() { * @param currentResponse * the response just received. */ - private void updateState(GitHubRequest currentRequest, GitHubResponse currentResponse) { + private void updateState(GitHubRequest currentRequest, GitHubResponse currentResponse, boolean firstCall) { hasPrevious = false; hasNext = false; finalPage = getPageFromUrl(currentRequest.url()); @@ -244,6 +243,12 @@ private void updateState(GitHubRequest currentRequest, GitHubResponse current } } } + if (firstCall) { + hasNext = true; + next = currentResponse.body(); + } else { + next = null; + } } diff --git a/src/main/java/org/kohsuke/github/Paginator.java b/src/main/java/org/kohsuke/github/Paginator.java index 810e10e6a4..58bbe90f0e 100644 --- a/src/main/java/org/kohsuke/github/Paginator.java +++ b/src/main/java/org/kohsuke/github/Paginator.java @@ -91,7 +91,7 @@ public T next() { if (currentPage != null && nextItemIndex < currentPage.length) return currentPage[nextItemIndex++]; fetchNext(); - if (currentPage == null) { + if (currentPage == null || currentPage.length == 0) { throw new NoSuchElementException(); } nextItemIndex = 0; @@ -120,7 +120,7 @@ public T previous() { if (currentPage != null && nextItemIndex > 0) return currentPage[--nextItemIndex]; fetchPrevious(); - if (currentPage == null) { + if (currentPage == null || currentPage.length == 0) { throw new NoSuchElementException(); } nextItemIndex = currentPage.length - 1; @@ -135,7 +135,9 @@ public T previous() { @Override public T first() { nextItemIndex = 0; - currentPage = base.first(); + T[] result = base.first(); + wrapUp(result); + currentPage = result; return currentPage[nextItemIndex++]; } @@ -145,7 +147,9 @@ public T first() { * @return the list */ T[] firstPageArray() { - currentPage = base.first(); + T[] result = base.first(); + wrapUp(result); + currentPage = result; nextItemIndex = currentPage.length; return currentPage; } @@ -166,7 +170,9 @@ public List firstPageList() { */ @Override public T last() { - currentPage = base.last(); + T[] result = base.last(); + wrapUp(result); + currentPage = result; nextItemIndex = currentPage.length - 1; return currentPage[nextItemIndex++]; } @@ -177,7 +183,9 @@ public T last() { * @return the list */ T[] lastPageArray() { - currentPage = base.last(); + T[] result = base.last(); + wrapUp(result); + currentPage = result; nextItemIndex = currentPage.length; return currentPage; } diff --git a/src/test/java/org/kohsuke/github/CommitTest.java b/src/test/java/org/kohsuke/github/CommitTest.java index 257e681cca..c04b871819 100644 --- a/src/test/java/org/kohsuke/github/CommitTest.java +++ b/src/test/java/org/kohsuke/github/CommitTest.java @@ -214,6 +214,20 @@ public void listPullRequests() throws Exception { listedPrs.stream().findFirst().filter(it -> it.getNumber() == prNumber).isPresent()); } + @Test + public void listPullRequestsPaginationWrapUpTest() throws Exception { + GHRepository repo = gitHub.getOrganization("hub4j-test-org").getRepository("listPrsListHeads"); + + GHCommit commit = repo.getCommit("6b9956fe8c3d030dbc49c9d4c4166b0ceb4198fc"); + + Paginator paginator = commit.listPullRequests().withPageSize(3).paginator(); + + assertThat(paginator.nextPage().get(0).owner, notNullValue()); + assertThat(paginator.previousPage().get(0).owner, notNullValue()); + assertThat(paginator.firstPageList().get(0).owner, notNullValue()); + assertThat(paginator.lastPageList().get(0).owner, notNullValue()); + } + /** * List pull requests of commit with 2 pull requests. * diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java index 177d80b823..9006a9048e 100644 --- a/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java +++ b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java @@ -3,6 +3,8 @@ import org.junit.Before; import org.junit.Test; +import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; @@ -14,6 +16,7 @@ public class GHWorkflowRunPaginationTest extends AbstractGitHubWireMockTest { private static final String REPO_NAME = "hub4j/github-api"; private static final String SINGLE_RUN_REPO_NAME = "hub4j-test-org/GHWorkflowRunTest"; + private static final String NO_RUNS_REPO_NAME = "hub4j-test-org/GHWorkflowTest"; private GHRepository repo; @Before @@ -36,7 +39,7 @@ public void testBasicPagination() throws Exception { assertThat(paginator.currentPage(), equalTo(6)); // last - assertThat(paginator.last().getRunNumber(), equalTo(3084L)); + assertThat(paginator.last().getRunNumber(), equalTo(3089L)); assertThat(paginator.hasNext(), equalTo(false)); assertThat(paginator.hasPrevious(), equalTo(true)); @@ -104,6 +107,112 @@ public void testBasicPagination() throws Exception { equalTo(getRunNumbers(paginator.jumpToPage(paginator.totalPages()).nextPage()))); } + @Test + public void testPageWithSizeEqualTo1() throws Exception { + Paginator paginator1 = repo.getWorkflow("maven-build.yml") + .listRuns() + .withPageSize(1) + .paginator(); + Paginator paginator2 = repo.getWorkflow("maven-build.yml") + .listRuns() + .withPageSize(1) + .paginator(); + + List page = paginator1.nextPage(); + assertThat(page.size(), equalTo(1)); + assertThat(page.get(0).getRunNumber(), equalTo(paginator2.next().getRunNumber())); + assertThat(page.get(0).getRunNumber(), equalTo(paginator1.previous().getRunNumber())); + assertThat(page.get(0).getRunNumber(), equalTo(paginator2.previousPage().get(0).getRunNumber())); + assertThat(page.get(0).getRunNumber(), equalTo(paginator1.firstPageList().get(0).getRunNumber())); + assertThat(page.get(0).getRunNumber(), equalTo(paginator1.first().getRunNumber())); + + page = paginator1.lastPageList(); + assertThat(page.size(), equalTo(1)); + assertThat(page.get(0).getRunNumber(), equalTo(paginator2.last().getRunNumber())); + + // jump to page + assertThat(paginator1.jumpToPage(4).currentPage(), equalTo(4)); + assertThat(paginator1.jumpToPage(8).currentPage(), equalTo(8)); + assertThat(paginator1.jumpToPage(6).currentPage(), equalTo(6)); + + // next and previous over multiple pages + long[] ascending = new long[14]; + long[] descending = new long[14]; + for (int i = 0; i < 14; i++) { + ascending[i] = paginator1.next().getRunNumber(); + } + for (int i = 13; i >= 0; i--) { + descending[i] = paginator1.previous().getRunNumber(); + } + assertThat(ascending, equalTo(descending)); + + // first page list vs jump to page 1 + assertThat(getRunNumbers(paginator1.firstPageList()), + equalTo(getRunNumbers(paginator1.jumpToPage(1).nextPage()))); + + // last page list vs jump to page number totalPages + assertThat(getRunNumbers(paginator1.lastPageList()), + equalTo(getRunNumbers(paginator1.jumpToPage(paginator1.totalPages()).nextPage()))); + } + + @Test + public void testNext() throws Exception { + assertThat(3584L, equalTo(getNewPaginator().next().getRunNumber())); + } + + @Test + public void testPrevious() throws Exception { + assertThat(3585L, equalTo(getNewPaginator().previous().getRunNumber())); + } + + @Test + public void testFirst() throws Exception { + assertThat(3589L, equalTo(getNewPaginator().first().getRunNumber())); + } + + @Test + public void testLast() throws Exception { + assertThat(3089L, equalTo(getNewPaginator().last().getRunNumber())); + } + + @Test + public void testHasNext() throws Exception { + assertThat(true, equalTo(getNewPaginator().hasNext())); + } + + @Test + public void testHasPrevious() throws Exception { + assertThat(true, equalTo(getNewPaginator().hasPrevious())); + } + + @Test + public void testNextPage() throws Exception { + assertThat(Arrays.asList(3584L, 3583L, 3582L, 3581L, 3580L), + equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + } + + @Test + public void testPreviousPage() throws Exception { + assertThat(Arrays.asList(3589L, 3588L, 3587L, 3586L, 3585L), + equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + } + + @Test + public void testFirstPage() throws Exception { + assertThat(Arrays.asList(3589L, 3588L, 3587L, 3586L, 3585L), + equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + } + + @Test + public void testLastPage() throws Exception { + assertThat(Arrays.asList(3091L, 3090L, 3089L), equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + } + + @Test + public void testJumpToPage() throws Exception { + assertThat(3579L, equalTo(getNewPaginator().jumpToPage(3).next().getRunNumber())); + } + @Test public void testRepoWithSingleRun() throws Exception { Paginator paginator = gitHub.getRepository(SINGLE_RUN_REPO_NAME) @@ -154,7 +263,20 @@ public void testRepoWithSingleRun() throws Exception { assertThat(paginator.hasNext(), equalTo(false)); } + @Test + public void testRepoWithNoRuns() throws Exception { + Paginator paginator = gitHub.getRepository(NO_RUNS_REPO_NAME) + .queryWorkflowRuns() + .list() + .paginator(); + paginator.next(); + } + private static List getRunNumbers(List runs) { return runs.stream().map(GHWorkflowRun::getRunNumber).collect(Collectors.toList()); } + + private Paginator getNewPaginator() throws IOException { + return repo.getWorkflow("maven-build.yml").listRuns().withPageSize(5).withStartPage(2).paginator(); + } } From 8e2ca8042db476d0029f860671f530f78642de77 Mon Sep 17 00:00:00 2001 From: Anuj Hydrabadi Date: Fri, 20 Oct 2023 00:10:47 +0530 Subject: [PATCH 5/5] fixes for when there is no data, more tests --- .../org/kohsuke/github/GitHubPaginator.java | 3 + .../java/org/kohsuke/github/Paginator.java | 16 +++- .../github/GHWorkflowRunPaginationTest.java | 91 +++++++++++++++---- 3 files changed, 93 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHubPaginator.java b/src/main/java/org/kohsuke/github/GitHubPaginator.java index 16418fbcd8..4afbb614bd 100644 --- a/src/main/java/org/kohsuke/github/GitHubPaginator.java +++ b/src/main/java/org/kohsuke/github/GitHubPaginator.java @@ -180,6 +180,9 @@ public void refresh() { if (!firstCallMade) { throw new GHException("Cannot refresh before the first call has been made!"); } + if (client.isOffline()) { + return; // cannot populate, will have to live with what we have + } makeRequest(false); } diff --git a/src/main/java/org/kohsuke/github/Paginator.java b/src/main/java/org/kohsuke/github/Paginator.java index 58bbe90f0e..250c109d76 100644 --- a/src/main/java/org/kohsuke/github/Paginator.java +++ b/src/main/java/org/kohsuke/github/Paginator.java @@ -74,7 +74,12 @@ protected void wrapUp(T[] page) { */ @Override public boolean hasNext() { - return (currentPage != null && nextItemIndex < currentPage.length) || base.hasNext(); + if (currentPage != null) { + return nextItemIndex < currentPage.length || base.hasNext(); + } else { + fetchNext(); + return currentPage.length != 0; + } } /** @@ -138,6 +143,9 @@ public T first() { T[] result = base.first(); wrapUp(result); currentPage = result; + if (currentPage.length == 0) { + throw new NoSuchElementException(); + } return currentPage[nextItemIndex++]; } @@ -174,6 +182,9 @@ public T last() { wrapUp(result); currentPage = result; nextItemIndex = currentPage.length - 1; + if (currentPage.length == 0) { + throw new NoSuchElementException(); + } return currentPage[nextItemIndex++]; } @@ -203,6 +214,9 @@ public List lastPageList() { * @return total number of pages */ public int totalPages() { + if (!hasPrevious() && !hasNext()) { + return 0; + } return base.totalCount(); } diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java index 9006a9048e..c16de9523c 100644 --- a/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java +++ b/src/test/java/org/kohsuke/github/GHWorkflowRunPaginationTest.java @@ -1,5 +1,6 @@ package org.kohsuke.github; +import org.jetbrains.annotations.NotNull; import org.junit.Before; import org.junit.Test; @@ -7,8 +8,10 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.NoSuchElementException; import java.util.stream.Collectors; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.notNullValue; @@ -157,17 +160,24 @@ public void testPageWithSizeEqualTo1() throws Exception { @Test public void testNext() throws Exception { - assertThat(3584L, equalTo(getNewPaginator().next().getRunNumber())); + assertThat(3585L, equalTo(getNewPaginator().next().getRunNumber())); + } + + @Test + public void testHasNextThenNext() throws Exception { + Paginator paginator = getNewPaginator(); + assertThat(paginator.hasNext(), equalTo(true)); + assertThat(3585L, equalTo(paginator.next().getRunNumber())); } @Test public void testPrevious() throws Exception { - assertThat(3585L, equalTo(getNewPaginator().previous().getRunNumber())); + assertThat(3586L, equalTo(getNewPaginator().previous().getRunNumber())); } @Test public void testFirst() throws Exception { - assertThat(3589L, equalTo(getNewPaginator().first().getRunNumber())); + assertThat(3590L, equalTo(getNewPaginator().first().getRunNumber())); } @Test @@ -187,30 +197,30 @@ public void testHasPrevious() throws Exception { @Test public void testNextPage() throws Exception { - assertThat(Arrays.asList(3584L, 3583L, 3582L, 3581L, 3580L), - equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + assertThat(Arrays.asList(3585L, 3584L, 3583L, 3582L, 3581L), + equalTo(getRunNumbers(getNewPaginator().nextPage()))); } @Test public void testPreviousPage() throws Exception { - assertThat(Arrays.asList(3589L, 3588L, 3587L, 3586L, 3585L), - equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + assertThat(Arrays.asList(3590L, 3589L, 3588L, 3587L, 3586L), + equalTo(getRunNumbers(getNewPaginator().previousPage()))); } @Test public void testFirstPage() throws Exception { - assertThat(Arrays.asList(3589L, 3588L, 3587L, 3586L, 3585L), - equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + assertThat(Arrays.asList(3590L, 3589L, 3588L, 3587L, 3586L), + equalTo(getRunNumbers(getNewPaginator().firstPageList()))); } @Test public void testLastPage() throws Exception { - assertThat(Arrays.asList(3091L, 3090L, 3089L), equalTo(getRunNumbers(getNewPaginator().lastPageList()))); + assertThat(List.of(3089L), equalTo(getRunNumbers(getNewPaginator().lastPageList()))); } @Test public void testJumpToPage() throws Exception { - assertThat(3579L, equalTo(getNewPaginator().jumpToPage(3).next().getRunNumber())); + assertThat(3580L, equalTo(getNewPaginator().jumpToPage(3).next().getRunNumber())); } @Test @@ -265,11 +275,60 @@ public void testRepoWithSingleRun() throws Exception { @Test public void testRepoWithNoRuns() throws Exception { - Paginator paginator = gitHub.getRepository(NO_RUNS_REPO_NAME) - .queryWorkflowRuns() - .list() - .paginator(); - paginator.next(); + Paginator paginator = getPaginatorWithNoRuns(); + assertThat(paginator.hasNext(), equalTo(false)); + assertThat(paginator.hasPrevious(), equalTo(false)); + assertThat(getPaginatorWithNoRuns().hasPrevious(), equalTo(false)); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_last_throwsException() throws Exception { + getPaginatorWithNoRuns().last(); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_first_throwsException() throws Exception { + getPaginatorWithNoRuns().first(); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_next_throwsException() throws Exception { + getPaginatorWithNoRuns().next(); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_previous_throwsException() throws Exception { + getPaginatorWithNoRuns().previous(); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_nextPage_emptyList() throws Exception { + getPaginatorWithNoRuns().nextPage(); + } + + @Test(expected = NoSuchElementException.class) + public void testRepoWithNoRuns_previousPage_emptyList() throws Exception { + getPaginatorWithNoRuns().previousPage(); + } + + @Test + public void testRepoWithNoRuns_firstPage_emptyList() throws Exception { + assertThat(getPaginatorWithNoRuns().firstPageList(), empty()); + } + + @Test + public void testRepoWithNoRuns_lastPage_emptyList() throws Exception { + assertThat(getPaginatorWithNoRuns().lastPageList(), empty()); + } + + @Test + public void testRepoWithNoRuns_totalPages_zero() throws Exception { + assertThat(getPaginatorWithNoRuns().totalPages(), equalTo(0)); + } + + @NotNull + private Paginator getPaginatorWithNoRuns() throws IOException { + return gitHub.getRepository(NO_RUNS_REPO_NAME).queryWorkflowRuns().list().paginator(); } private static List getRunNumbers(List runs) {