diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index 999d55aa3..155c3e6f7 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -41,11 +41,9 @@ import static java.util.logging.Level.*; /** * A GitHub API Client *

- * A GitHubClient can be used to send requests and retrieve their responses. Once built, a GitHubClient is thread-safe - * and can be used to send multiple requests. GitHubClient does, however cache some GitHub API information such as - * {@link #rateLimit()}. + * A GitHubClient can be used to send requests and retrieve their responses. GitHubClient is thread-safe and can be used + * to send multiple requests. GitHubClient also track some GitHub API information such as {@link #rateLimit()}. *

- * Class {@link GitHubClient} retireves */ abstract class GitHubClient { diff --git a/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java b/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java index 5a9a8a759..80145aa92 100644 --- a/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java +++ b/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java @@ -22,13 +22,14 @@ import static java.util.logging.Level.*; import static org.apache.commons.lang3.StringUtils.defaultString; /** - * A GitHub API Client + * A GitHub API Client for HttpUrlConnection *

- * A GitHubClient can be used to send requests and retrieve their responses. Once built, a GitHubClient is thread-safe - * and can be used to send multiple requests. GitHubClient does, however cache some GitHub API information such as - * {@link #rateLimit()}. + * A GitHubClient can be used to send requests and retrieve their responses. GitHubClient is thread-safe and can be used + * to send multiple requests. GitHubClient also track some GitHub API information such as {@link #rateLimit()}. + *

+ *

+ * GitHubHttpUrlConnectionClient gets a new {@link HttpURLConnection} for each call to send. *

- * Class {@link GitHubHttpUrlConnectionClient} retireves */ class GitHubHttpUrlConnectionClient extends GitHubClient { diff --git a/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java b/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java index 902382238..e76facb37 100644 --- a/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java +++ b/src/main/java/org/kohsuke/github/GitHubPageContentsIterable.java @@ -37,7 +37,7 @@ class GitHubPageContentsIterable extends PagedIterable { public PagedIterator _iterator(int pageSize) { final GitHubPageIterator iterator = GitHubPageIterator .create(client, clazz, request.toBuilder().withPageSize(pageSize)); - return new GitHubPagedIterator(iterator); + return new GitHubPageContentsIterator(iterator); } /** @@ -50,18 +50,16 @@ class GitHubPageContentsIterable extends PagedIterable { */ @Nonnull GitHubResponse toResponse() throws IOException { - GitHubPagedIterator iterator = (GitHubPagedIterator) iterator(); + GitHubPageContentsIterator iterator = (GitHubPageContentsIterator) iterator(); T[] items = toArray(iterator); GitHubResponse lastResponse = iterator.lastResponse(); return new GitHubResponse<>(lastResponse, items); } - private class GitHubPagedIterator extends PagedIterator { - private final GitHubPageIterator baseIterator; + private class GitHubPageContentsIterator extends PagedIterator { - public GitHubPagedIterator(GitHubPageIterator iterator) { + public GitHubPageContentsIterator(GitHubPageIterator iterator) { super(iterator); - baseIterator = iterator; } @Override @@ -79,7 +77,7 @@ class GitHubPageContentsIterable extends PagedIterable { * @return the {@link GitHubResponse} for the last page received. */ private GitHubResponse lastResponse() { - return baseIterator.lastResponse(); + return ((GitHubPageIterator) base).finalResponse(); } } } diff --git a/src/main/java/org/kohsuke/github/GitHubPageIterator.java b/src/main/java/org/kohsuke/github/GitHubPageIterator.java index 0ac79ece2..9e2be1f2b 100644 --- a/src/main/java/org/kohsuke/github/GitHubPageIterator.java +++ b/src/main/java/org/kohsuke/github/GitHubPageIterator.java @@ -1,15 +1,17 @@ package org.kohsuke.github; +import java.io.IOException; import java.net.MalformedURLException; +import java.net.URL; import java.util.Iterator; -import java.util.Objects; +import java.util.NoSuchElementException; import javax.annotation.Nonnull; /** * May be used for any item that has pagination information. Iterates over paginated {@link T} objects (not the items - * inside the page). Also exposes {@link #nextResponse()} to allow getting the full {@link GitHubResponse} instead of - * T. + * inside the page). Also exposes {@link #finalResponse()} to allow getting a full {@link GitHubResponse} after + * iterating completes. * * Works for array responses, also works for search results which are single instances with an array of items inside. * @@ -18,19 +20,41 @@ import javax.annotation.Nonnull; */ class GitHubPageIterator implements Iterator { - private final Iterator> delegate; - private GitHubResponse lastResponse = null; + private final GitHubClient client; + private final Class type; - public GitHubPageIterator(GitHubClient client, Class type, GitHubRequest request) { - this(new GitHubPageResponseIterator<>(client, type, request)); + /** + * The page that will be returned when {@link #next()} is called. + * + *

+ * Will be {@code null} after {@link #next()} is called. + *

+ *

+ * Will not be {@code null} after {@link #fetch()} is called if a new page was fetched. + *

+ */ + private T next; + + /** + * The request that will be sent when to get a new response page if {@link #next} is {@code null}. Will be + * {@code null} when there are no more pages to fetch. + */ + private GitHubRequest nextRequest; + + /** + * When done iterating over pages, it is on rare occasions useful to be able to get information from the final + * response that was retrieved. + */ + private GitHubResponse finalResponse = null; + + GitHubPageIterator(GitHubClient client, Class type, GitHubRequest request) { if (!"GET".equals(request.method())) { - throw new IllegalStateException("Request method \"GET\" is required for iterator."); + throw new IllegalStateException("Request method \"GET\" is required for page iterator."); } - } - - GitHubPageIterator(Iterator> delegate) { - this.delegate = delegate; + this.client = client; + this.type = type; + this.nextRequest = request; } /** @@ -52,37 +76,104 @@ class GitHubPageIterator implements Iterator { } } + /** + * {@inheritDoc} + */ public boolean hasNext() { - return delegate.hasNext(); + synchronized (this) { + fetch(); + return next != null; + } } /** * Gets the next page. - * + * * @return the next page. */ @Nonnull public T next() { - return Objects.requireNonNull(nextResponse().body()); + synchronized (this) { + fetch(); + T result = next; + if (result == null) + throw new NoSuchElementException(); + // If this is the last page, keep the response + next = null; + return result; + } } /** - * Gets the next response page. - * - * @return the next response page. + * On rare occasions the final response from iterating is needed. + * + * @return the final response of the iterator. */ - @Nonnull - public GitHubResponse nextResponse() { - GitHubResponse result = Objects.requireNonNull(delegate.next()); - lastResponse = result; - return result; + public GitHubResponse finalResponse() { + if (hasNext()) { + throw new GHException("Final response is not available until after iterator is done."); + } + return finalResponse; } public void remove() { throw new UnsupportedOperationException(); } - public GitHubResponse lastResponse() { - return lastResponse; + /** + * Fetch is called at the start of {@link #hasNext()} or {@link #next()} to fetch another page of data if it is + * needed. + *

+ * If {@link #next} is not {@code null}, no further action is need. If {@link #next} is {@code null} and + * {@link #nextRequest} is {@code null}, there are no more pages to fetch. + *

+ *

+ * Otherwise, a new response page is fetched using {@link #nextRequest}. The response is then check to see if there + * is a page after it and {@link #nextRequest} is updated to point to it. If there are no pages available after the + * current response, {@link #nextRequest} is set to {@code null}. + *

+ */ + private void fetch() { + if (next != null) + return; // already fetched + if (nextRequest == null) + return; // no more data to fetch + + URL url = nextRequest.url(); + try { + GitHubResponse nextResponse = client.sendRequest(nextRequest, + (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)); + assert nextResponse.body() != null; + next = nextResponse.body(); + nextRequest = findNextURL(nextResponse); + if (nextRequest == null) { + finalResponse = nextResponse; + } + } 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); + } } + + /** + * Locate the next page from the pagination "Link" tag. + */ + private GitHubRequest findNextURL(GitHubResponse nextResponse) throws MalformedURLException { + GitHubRequest result = null; + String link = nextResponse.headerField("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" + int idx = token.indexOf('>'); + result = nextResponse.request().toBuilder().setRawUrlPath(token.substring(1, idx)).build(); + break; + } + } + } + return result; + } + } diff --git a/src/main/java/org/kohsuke/github/GitHubPageResponseIterator.java b/src/main/java/org/kohsuke/github/GitHubPageResponseIterator.java deleted file mode 100644 index 46a36b0ab..000000000 --- a/src/main/java/org/kohsuke/github/GitHubPageResponseIterator.java +++ /dev/null @@ -1,84 +0,0 @@ -package org.kohsuke.github; - -import java.io.IOException; -import java.net.MalformedURLException; -import java.net.URL; -import java.util.Iterator; -import java.util.NoSuchElementException; - -/** - * May be used for any item that has pagination information. Iterates over paginated {@link GitHubResponse} objects - * containing each page (not items on the page). - * - * Works for array responses, also works for search results which are single instances with an array of items inside. - * - * @param - * type of each page (not the items in the page). - */ -class GitHubPageResponseIterator implements Iterator> { - - private final GitHubClient client; - private final Class type; - private GitHubRequest nextRequest; - private GitHubResponse next; - - GitHubPageResponseIterator(GitHubClient client, Class type, GitHubRequest request) { - this.client = client; - this.type = type; - this.nextRequest = request; - } - - public boolean hasNext() { - fetch(); - return next != null; - } - - public GitHubResponse next() { - fetch(); - GitHubResponse r = next; - if (r == null) - throw new NoSuchElementException(); - next = null; - return r; - } - - public void remove() { - throw new UnsupportedOperationException(); - } - - private void fetch() { - if (next != null) - return; // already fetched - if (nextRequest == null) - return; // no more data to fetch - - URL url = nextRequest.url(); - try { - next = client.sendRequest(nextRequest, (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)); - assert next.body() != null; - nextRequest = findNextURL(); - } catch (IOException e) { - throw new GHException("Failed to retrieve " + url, e); - } - } - - /** - * Locate the next page from the pagination "Link" tag. - */ - private GitHubRequest findNextURL() throws MalformedURLException { - GitHubRequest result = null; - String link = next.headerField("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" - int idx = token.indexOf('>'); - result = next.request().toBuilder().setRawUrlPath(token.substring(1, idx)).build(); - break; - } - } - } - return result; - } -} diff --git a/src/main/java/org/kohsuke/github/GitHubResponse.java b/src/main/java/org/kohsuke/github/GitHubResponse.java index 9ae8cdca1..cdb38bf3e 100644 --- a/src/main/java/org/kohsuke/github/GitHubResponse.java +++ b/src/main/java/org/kohsuke/github/GitHubResponse.java @@ -290,7 +290,7 @@ class GitHubResponse { /** * Gets the body of the response as a {@link String}. * - * @return the body of the response as a {@link String}.F + * @return the body of the response as a {@link String}. * @throws IOException * if an I/O Exception occurs. */ @@ -302,7 +302,6 @@ class GitHubResponse { } finally { IOUtils.closeQuietly(r); } - } } diff --git a/src/main/java/org/kohsuke/github/PagedIterator.java b/src/main/java/org/kohsuke/github/PagedIterator.java index 90796f2ae..e6e645985 100644 --- a/src/main/java/org/kohsuke/github/PagedIterator.java +++ b/src/main/java/org/kohsuke/github/PagedIterator.java @@ -55,17 +55,21 @@ public abstract class PagedIterator implements Iterator { * {@inheritDoc} */ public boolean hasNext() { - fetch(); - return currentPage.length > nextItemIndex; + synchronized (this) { + fetch(); + return currentPage.length > nextItemIndex; + } } /** * {@inheritDoc} */ public T next() { - if (!hasNext()) - throw new NoSuchElementException(); - return currentPage[nextItemIndex++]; + synchronized (this) { + if (!hasNext()) + throw new NoSuchElementException(); + return currentPage[nextItemIndex++]; + } } /** @@ -116,21 +120,23 @@ public abstract class PagedIterator implements Iterator { */ @Nonnull T[] nextPageArray() { - // if we have not fetched any pages yet, always fetch. - // If we have fetched at least one page, check hasNext() - if (currentPage == null) { - fetch(); - } else if (!hasNext()) { - throw new NoSuchElementException(); - } + synchronized (this) { + // if we have not fetched any pages yet, always fetch. + // If we have fetched at least one page, check hasNext() + if (currentPage == null) { + fetch(); + } else if (!hasNext()) { + throw new NoSuchElementException(); + } - // Current should never be null after fetch - Objects.requireNonNull(currentPage); - T[] r = currentPage; - if (nextItemIndex != 0) { - r = Arrays.copyOfRange(r, nextItemIndex, r.length); + // Current should never be null after fetch + Objects.requireNonNull(currentPage); + T[] r = currentPage; + if (nextItemIndex != 0) { + r = Arrays.copyOfRange(r, nextItemIndex, r.length); + } + nextItemIndex = currentPage.length; + return r; } - nextItemIndex = currentPage.length; - return r; } }