From 3400bbe7d88bc53ccb65f956a670cbea36f05ff0 Mon Sep 17 00:00:00 2001 From: Atsushi Eno Date: Thu, 13 Mar 2025 15:38:07 +0800 Subject: [PATCH 1/7] Add an option to avoid buffered response stream. context: #1405 GHArtifact.download() now explicitly sets this option to make response stream non-buffered. --- .../java/org/kohsuke/github/GHArtifact.java | 6 +++- .../org/kohsuke/github/GitHubRequest.java | 28 +++++++++++++++++-- .../connector/GitHubConnectorRequest.java | 9 ++++++ .../connector/GitHubConnectorResponse.java | 13 +++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GHArtifact.java b/src/main/java/org/kohsuke/github/GHArtifact.java index cc37a5bf4d..f9376cd878 100644 --- a/src/main/java/org/kohsuke/github/GHArtifact.java +++ b/src/main/java/org/kohsuke/github/GHArtifact.java @@ -115,7 +115,11 @@ public void delete() throws IOException { public T download(InputStreamFunction streamFunction) throws IOException { requireNonNull(streamFunction, "Stream function must not be null"); - return root().createRequest().method("GET").withUrlPath(getApiRoute(), "zip").fetchStream(streamFunction); + return root().createRequest() + .method("GET") + .withUrlPath(getApiRoute(), "zip") + .avoidBufferedResponseStream() + .fetchStream(streamFunction); } private String getApiRoute() { diff --git a/src/main/java/org/kohsuke/github/GitHubRequest.java b/src/main/java/org/kohsuke/github/GitHubRequest.java index 2bb4a459a8..c018814ce9 100644 --- a/src/main/java/org/kohsuke/github/GitHubRequest.java +++ b/src/main/java/org/kohsuke/github/GitHubRequest.java @@ -51,6 +51,7 @@ public class GitHubRequest implements GitHubConnectorRequest { private final RateLimitTarget rateLimitTarget; private final byte[] body; private final boolean forceBody; + private final boolean avoidBufferedResponseStream; private final URL url; @@ -63,7 +64,8 @@ private GitHubRequest(@Nonnull List args, @Nonnull String method, @Nonnull RateLimitTarget rateLimitTarget, @CheckForNull byte[] body, - boolean forceBody) { + boolean forceBody, + boolean avoidBufferedResponseStream) { this.args = Collections.unmodifiableList(new ArrayList<>(args)); TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); for (Map.Entry> entry : headers.entrySet()) { @@ -77,6 +79,7 @@ private GitHubRequest(@Nonnull List args, this.rateLimitTarget = rateLimitTarget; this.body = body; this.forceBody = forceBody; + this.avoidBufferedResponseStream = avoidBufferedResponseStream; String tailApiUrl = buildTailApiUrl(); url = getApiURL(apiUrl, tailApiUrl); } @@ -269,6 +272,14 @@ public boolean hasBody() { return forceBody || !METHODS_WITHOUT_BODY.contains(method); } + /** + * Whether the response to this request should avoid buffered stream or not. + */ + @Override + public boolean avoidBufferedResponseStream() { + return avoidBufferedResponseStream; + } + /** * Create a {@link Builder} from this request. Initial values of the builder will be the same as this * {@link GitHubRequest}. @@ -354,6 +365,7 @@ static class Builder> { private byte[] body; private boolean forceBody; + private boolean avoidBufferedResponseStream; /** * Create a new {@link GitHubRequest.Builder} @@ -410,7 +422,8 @@ public GitHubRequest build() { method, rateLimitTarget, body, - forceBody); + forceBody, + avoidBufferedResponseStream); } /** @@ -800,6 +813,17 @@ public B inBody() { forceBody = true; return (B) this; } + + /** + * We cache response stream into buffer by default, but some responses can be huge so we should avoid that. + * Setting this flag indicates that we should avoid buffered stream response. + * + * @return the request builder + */ + public B avoidBufferedResponseStream() { + avoidBufferedResponseStream = true; + return (B) this; + } } /** diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java index e00d59dcec..4533917e3d 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java @@ -81,4 +81,13 @@ public interface GitHubConnectorRequest { * @return true, if the body is not null. Otherwise, false. */ boolean hasBody(); + + /** + * Whether the response stream to this request is not buffered. It is used to avoid huge response caching. + * + * @return true, if the response stream is not buffered. + */ + default boolean avoidBufferedResponseStream() { + return false; + } } diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index b71fc8abc5..fe6e60f19a 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -161,6 +161,7 @@ public abstract static class ByteArrayResponse extends GitHubConnectorResponse { private boolean inputStreamRead = false; private byte[] inputBytes = null; private boolean isClosed = false; + private boolean avoidBufferedResponseStream; /** * Constructor for ByteArray Response @@ -176,6 +177,7 @@ protected ByteArrayResponse(@Nonnull GitHubConnectorRequest request, int statusCode, @Nonnull Map> headers) { super(request, statusCode, headers); + avoidBufferedResponseStream = request.avoidBufferedResponseStream(); } /** @@ -187,6 +189,17 @@ public InputStream bodyStream() throws IOException { if (isClosed) { throw new IOException("Response is closed"); } + + if (avoidBufferedResponseStream) { + synchronized (this) { + if (inputStreamRead) { + throw new IOException("Response is already consumed"); + } + inputStreamRead = true; + return wrapStream(rawBodyStream()); + } + } + synchronized (this) { if (!inputStreamRead) { InputStream rawStream = rawBodyStream(); From 489e80b8b3cbc36cf352499fb5d922d370122275 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Fri, 14 Mar 2025 09:42:49 -0700 Subject: [PATCH 2/7] Ignore default interface method coverage --- pom.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pom.xml b/pom.xml index 9475718f04..8456d5e747 100644 --- a/pom.xml +++ b/pom.xml @@ -169,6 +169,9 @@ org.kohsuke.github.GHCommit.GHAuthor + + org.kohsuke.github.connector.GitHubConnectorRequest + org.kohsuke.github.GHIssue.PullRequest org.kohsuke.github.GHCommitSearchBuilder From d47b1cf16883aadcb00b7f085e4ec46322477a78 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Fri, 14 Mar 2025 23:05:49 -0700 Subject: [PATCH 3/7] Finish wiring in streaming body response --- src/main/java/org/kohsuke/github/GitHubClient.java | 6 +++++- src/main/java/org/kohsuke/github/GitHubRequest.java | 8 ++++++-- src/test/java/org/kohsuke/github/GHWorkflowRunTest.java | 3 +++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index f669280b4d..a4e245c91c 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -629,7 +629,11 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) { LOGGER.log(FINEST, () -> { String body; try { - body = GitHubResponse.getBodyAsString(response); + if (response.request().avoidBufferedResponseStream()) { + body = "Stream-once body not logged."; + } else { + body = GitHubResponse.getBodyAsString(response); + } } catch (Throwable e) { body = "Error reading response body"; } diff --git a/src/main/java/org/kohsuke/github/GitHubRequest.java b/src/main/java/org/kohsuke/github/GitHubRequest.java index c018814ce9..6027ec040b 100644 --- a/src/main/java/org/kohsuke/github/GitHubRequest.java +++ b/src/main/java/org/kohsuke/github/GitHubRequest.java @@ -295,7 +295,8 @@ Builder toBuilder() { method, rateLimitTarget, body, - forceBody); + forceBody, + avoidBufferedResponseStream); } private String buildTailApiUrl() { @@ -379,6 +380,7 @@ protected Builder() { "GET", RateLimitTarget.CORE, null, + false, false); } @@ -390,7 +392,8 @@ private Builder(@Nonnull List args, @Nonnull String method, @Nonnull RateLimitTarget rateLimitTarget, @CheckForNull byte[] body, - boolean forceBody) { + boolean forceBody, + boolean avoidBufferedResponseStream) { this.args = new ArrayList<>(args); TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); for (Map.Entry> entry : headers.entrySet()) { @@ -404,6 +407,7 @@ private Builder(@Nonnull List args, this.rateLimitTarget = rateLimitTarget; this.body = body; this.forceBody = forceBody; + this.avoidBufferedResponseStream = avoidBufferedResponseStream; } /** diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java index e3727d7c56..2e2a4fde95 100644 --- a/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java +++ b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java @@ -9,6 +9,7 @@ import org.kohsuke.github.GHWorkflowRun.Status; import org.kohsuke.github.function.InputStreamFunction; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.time.Duration; import java.time.Instant; @@ -395,6 +396,8 @@ public void testArtifacts() throws IOException { // Test download from upload-artifact@v3 infrastructure String artifactContent = artifacts.get(0).download((is) -> { + assertThat(is, not(isA(ByteArrayInputStream.class))); + try (ZipInputStream zis = new ZipInputStream(is)) { StringBuilder sb = new StringBuilder(); From b2641c0dbab7be81c1d74390a13a53652c2324b2 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 15 Mar 2025 00:12:23 -0700 Subject: [PATCH 4/7] Update GitHubConnectorResponse.bodyStream() implementation --- pom.xml | 3 -- .../connector/GitHubConnectorResponse.java | 34 ++++++++----------- .../kohsuke/github/RequesterRetryTest.java | 3 ++ 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/pom.xml b/pom.xml index 8456d5e747..9475718f04 100644 --- a/pom.xml +++ b/pom.xml @@ -169,9 +169,6 @@ org.kohsuke.github.GHCommit.GHAuthor - - org.kohsuke.github.connector.GitHubConnectorRequest - org.kohsuke.github.GHIssue.PullRequest org.kohsuke.github.GHCommitSearchBuilder diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index fe6e60f19a..edd7732fb7 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -87,9 +87,9 @@ public String header(String name) { public abstract InputStream bodyStream() throws IOException; /** - * Gets the {@link GitHubConnectorRequest} for this response. + * Gets the {@link GitHubConnector} for this response. * - * @return the {@link GitHubConnectorRequest} for this response. + * @return the {@link GitHubConnector} for this response. */ @Nonnull public GitHubConnectorRequest request() { @@ -161,7 +161,6 @@ public abstract static class ByteArrayResponse extends GitHubConnectorResponse { private boolean inputStreamRead = false; private byte[] inputBytes = null; private boolean isClosed = false; - private boolean avoidBufferedResponseStream; /** * Constructor for ByteArray Response @@ -177,7 +176,6 @@ protected ByteArrayResponse(@Nonnull GitHubConnectorRequest request, int statusCode, @Nonnull Map> headers) { super(request, statusCode, headers); - avoidBufferedResponseStream = request.avoidBufferedResponseStream(); } /** @@ -190,29 +188,27 @@ public InputStream bodyStream() throws IOException { throw new IOException("Response is closed"); } - if (avoidBufferedResponseStream) { - synchronized (this) { - if (inputStreamRead) { - throw new IOException("Response is already consumed"); - } - inputStreamRead = true; - return wrapStream(rawBodyStream()); - } - } - synchronized (this) { + InputStream body; if (!inputStreamRead) { - InputStream rawStream = rawBodyStream(); - try (InputStream stream = wrapStream(rawStream)) { - if (stream != null) { - inputBytes = IOUtils.toByteArray(stream); + body = wrapStream(rawBodyStream()); + if (!request().avoidBufferedResponseStream()) { + try (InputStream stream = body) { + if (stream != null) { + inputBytes = IOUtils.toByteArray(stream); + } } } inputStreamRead = true; + if (request().avoidBufferedResponseStream()) { + return body; + } } } - if (inputBytes == null) { + if (request().avoidBufferedResponseStream()) { + throw new IOException("Response is already consumed"); + } else if (inputBytes == null) { throw new IOException("Response body missing, stream null"); } diff --git a/src/test/java/org/kohsuke/github/RequesterRetryTest.java b/src/test/java/org/kohsuke/github/RequesterRetryTest.java index 81a8b34ec9..fba5323c3c 100644 --- a/src/test/java/org/kohsuke/github/RequesterRetryTest.java +++ b/src/test/java/org/kohsuke/github/RequesterRetryTest.java @@ -275,6 +275,9 @@ public void testGitHubIsApiUrlValid() throws Exception { */ @Test public void testResponseCodeFailureExceptions() throws Exception { + // Cover default method in GitHubConnectorRequest + assertThat(IGNORED_EMPTY_REQUEST.avoidBufferedResponseStream(), equalTo(false)); + // No retry for these Exceptions GitHubConnector connector = new SendThrowingGitHubConnector<>(() -> { throw new IOException("Custom"); From 9c06af553c98440a7c7db698205cdc7d1831ac07 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 15 Mar 2025 02:05:44 -0700 Subject: [PATCH 5/7] Expand use for raw body stream --- .../java/org/kohsuke/github/GHArtifact.java | 6 +----- src/main/java/org/kohsuke/github/Requester.java | 5 ++++- .../connector/GitHubConnectorResponse.java | 17 ++++++++++++++--- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GHArtifact.java b/src/main/java/org/kohsuke/github/GHArtifact.java index f9376cd878..cc37a5bf4d 100644 --- a/src/main/java/org/kohsuke/github/GHArtifact.java +++ b/src/main/java/org/kohsuke/github/GHArtifact.java @@ -115,11 +115,7 @@ public void delete() throws IOException { public T download(InputStreamFunction streamFunction) throws IOException { requireNonNull(streamFunction, "Stream function must not be null"); - return root().createRequest() - .method("GET") - .withUrlPath(getApiRoute(), "zip") - .avoidBufferedResponseStream() - .fetchStream(streamFunction); + return root().createRequest().method("GET").withUrlPath(getApiRoute(), "zip").fetchStream(streamFunction); } private String getApiRoute() { diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java index ad65f1a2d1..f475eda856 100644 --- a/src/main/java/org/kohsuke/github/Requester.java +++ b/src/main/java/org/kohsuke/github/Requester.java @@ -128,7 +128,10 @@ public int fetchHttpStatusCode() throws IOException { * the io exception */ public T fetchStream(@Nonnull InputStreamFunction handler) throws IOException { - return client.sendRequest(this, (connectorResponse) -> handler.apply(connectorResponse.bodyStream())).body(); + return client + .sendRequest(this.avoidBufferedResponseStream(), + (connectorResponse) -> handler.apply(connectorResponse.bodyStream())) + .body(); } /** diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index edd7732fb7..5b2e068256 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -13,6 +13,8 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import static java.net.HttpURLConnection.HTTP_OK; + /** * Response information supplied when a response is received and before the body is processed. *

@@ -116,6 +118,15 @@ public Map> allHeaders() { return headers; } + /** + * Use unbufferred body stream. + * + * @return true when unbuffered body stream can should be used. + */ + boolean useUnbufferedBodyStream() { + return statusCode() == HTTP_OK && request().avoidBufferedResponseStream(); + } + /** * Handles wrapping the body stream if indicated by the "Content-Encoding" header. * @@ -192,7 +203,7 @@ public InputStream bodyStream() throws IOException { InputStream body; if (!inputStreamRead) { body = wrapStream(rawBodyStream()); - if (!request().avoidBufferedResponseStream()) { + if (!useUnbufferedBodyStream()) { try (InputStream stream = body) { if (stream != null) { inputBytes = IOUtils.toByteArray(stream); @@ -200,13 +211,13 @@ public InputStream bodyStream() throws IOException { } } inputStreamRead = true; - if (request().avoidBufferedResponseStream()) { + if (useUnbufferedBodyStream()) { return body; } } } - if (request().avoidBufferedResponseStream()) { + if (useUnbufferedBodyStream()) { throw new IOException("Response is already consumed"); } else if (inputBytes == null) { throw new IOException("Response body missing, stream null"); From 7b7f1bdf4c1b690735c25a776688dce102833532 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sun, 16 Mar 2025 21:01:19 -0700 Subject: [PATCH 6/7] Change all response body calls to default to streamed output --- pom.xml | 2 +- .../java/org/kohsuke/github/GitHubClient.java | 7 +- .../org/kohsuke/github/GitHubRequest.java | 36 +---- .../java/org/kohsuke/github/Requester.java | 5 +- .../connector/GitHubConnectorRequest.java | 9 -- .../connector/GitHubConnectorResponse.java | 152 ++++++++++-------- .../org/kohsuke/github/GHWorkflowRunTest.java | 10 +- .../kohsuke/github/RequesterRetryTest.java | 9 +- 8 files changed, 110 insertions(+), 120 deletions(-) diff --git a/pom.xml b/pom.xml index 9475718f04..723e8728b6 100644 --- a/pom.xml +++ b/pom.xml @@ -611,7 +611,7 @@ - httpclient-test + httpclient-test-tracing integration-test test diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index a4e245c91c..2cad9f6923 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -629,11 +629,8 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) { LOGGER.log(FINEST, () -> { String body; try { - if (response.request().avoidBufferedResponseStream()) { - body = "Stream-once body not logged."; - } else { - body = GitHubResponse.getBodyAsString(response); - } + response.forceBufferedBodyStream(); + body = GitHubResponse.getBodyAsString(response); } catch (Throwable e) { body = "Error reading response body"; } diff --git a/src/main/java/org/kohsuke/github/GitHubRequest.java b/src/main/java/org/kohsuke/github/GitHubRequest.java index 6027ec040b..2bb4a459a8 100644 --- a/src/main/java/org/kohsuke/github/GitHubRequest.java +++ b/src/main/java/org/kohsuke/github/GitHubRequest.java @@ -51,7 +51,6 @@ public class GitHubRequest implements GitHubConnectorRequest { private final RateLimitTarget rateLimitTarget; private final byte[] body; private final boolean forceBody; - private final boolean avoidBufferedResponseStream; private final URL url; @@ -64,8 +63,7 @@ private GitHubRequest(@Nonnull List args, @Nonnull String method, @Nonnull RateLimitTarget rateLimitTarget, @CheckForNull byte[] body, - boolean forceBody, - boolean avoidBufferedResponseStream) { + boolean forceBody) { this.args = Collections.unmodifiableList(new ArrayList<>(args)); TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); for (Map.Entry> entry : headers.entrySet()) { @@ -79,7 +77,6 @@ private GitHubRequest(@Nonnull List args, this.rateLimitTarget = rateLimitTarget; this.body = body; this.forceBody = forceBody; - this.avoidBufferedResponseStream = avoidBufferedResponseStream; String tailApiUrl = buildTailApiUrl(); url = getApiURL(apiUrl, tailApiUrl); } @@ -272,14 +269,6 @@ public boolean hasBody() { return forceBody || !METHODS_WITHOUT_BODY.contains(method); } - /** - * Whether the response to this request should avoid buffered stream or not. - */ - @Override - public boolean avoidBufferedResponseStream() { - return avoidBufferedResponseStream; - } - /** * Create a {@link Builder} from this request. Initial values of the builder will be the same as this * {@link GitHubRequest}. @@ -295,8 +284,7 @@ Builder toBuilder() { method, rateLimitTarget, body, - forceBody, - avoidBufferedResponseStream); + forceBody); } private String buildTailApiUrl() { @@ -366,7 +354,6 @@ static class Builder> { private byte[] body; private boolean forceBody; - private boolean avoidBufferedResponseStream; /** * Create a new {@link GitHubRequest.Builder} @@ -380,7 +367,6 @@ protected Builder() { "GET", RateLimitTarget.CORE, null, - false, false); } @@ -392,8 +378,7 @@ private Builder(@Nonnull List args, @Nonnull String method, @Nonnull RateLimitTarget rateLimitTarget, @CheckForNull byte[] body, - boolean forceBody, - boolean avoidBufferedResponseStream) { + boolean forceBody) { this.args = new ArrayList<>(args); TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); for (Map.Entry> entry : headers.entrySet()) { @@ -407,7 +392,6 @@ private Builder(@Nonnull List args, this.rateLimitTarget = rateLimitTarget; this.body = body; this.forceBody = forceBody; - this.avoidBufferedResponseStream = avoidBufferedResponseStream; } /** @@ -426,8 +410,7 @@ public GitHubRequest build() { method, rateLimitTarget, body, - forceBody, - avoidBufferedResponseStream); + forceBody); } /** @@ -817,17 +800,6 @@ public B inBody() { forceBody = true; return (B) this; } - - /** - * We cache response stream into buffer by default, but some responses can be huge so we should avoid that. - * Setting this flag indicates that we should avoid buffered stream response. - * - * @return the request builder - */ - public B avoidBufferedResponseStream() { - avoidBufferedResponseStream = true; - return (B) this; - } } /** diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java index f475eda856..ad65f1a2d1 100644 --- a/src/main/java/org/kohsuke/github/Requester.java +++ b/src/main/java/org/kohsuke/github/Requester.java @@ -128,10 +128,7 @@ public int fetchHttpStatusCode() throws IOException { * the io exception */ public T fetchStream(@Nonnull InputStreamFunction handler) throws IOException { - return client - .sendRequest(this.avoidBufferedResponseStream(), - (connectorResponse) -> handler.apply(connectorResponse.bodyStream())) - .body(); + return client.sendRequest(this, (connectorResponse) -> handler.apply(connectorResponse.bodyStream())).body(); } /** diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java index 4533917e3d..e00d59dcec 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java @@ -81,13 +81,4 @@ public interface GitHubConnectorRequest { * @return true, if the body is not null. Otherwise, false. */ boolean hasBody(); - - /** - * Whether the response stream to this request is not buffered. It is used to avoid huge response caching. - * - * @return true, if the response stream is not buffered. - */ - default boolean avoidBufferedResponseStream() { - return false; - } } diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index 5b2e068256..b1ef4cbbdb 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -7,7 +7,12 @@ import java.io.Closeable; import java.io.IOException; import java.io.InputStream; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; import java.util.zip.GZIPInputStream; import javax.annotation.CheckForNull; @@ -37,6 +42,12 @@ public abstract class GitHubConnectorResponse implements Closeable { private final GitHubConnectorRequest request; @Nonnull private final Map> headers; + private boolean bodyStreamCalled = false; + private boolean bodyBytesLoaded = false; + private InputStream bodyStream = null; + private byte[] bodyBytes = null; + private boolean isClosed = false; + private boolean forceBufferedBodyStream; /** * GitHubConnectorResponse constructor @@ -86,7 +97,55 @@ public String header(String name) { * if response stream is null or an I/O Exception occurs. */ @Nonnull - public abstract InputStream bodyStream() throws IOException; + public InputStream bodyStream() throws IOException { + InputStream body = null; + synchronized (this) { + if (isClosed) { + throw new IOException("Response is closed"); + } + + if (bodyStreamCalled) { + if (!bodyBytesLoaded) { + throw new IOException("Response is already consumed"); + } + } else { + body = wrapStream(rawBodyStream()); + bodyStreamCalled = true; + bodyStream = body; + if (useBufferedBodyStream()) { + bodyBytesLoaded = true; + try (InputStream stream = body) { + if (stream != null) { + bodyBytes = IOUtils.toByteArray(stream); + } + } + bodyStream = null; + } + } + + if (bodyBytesLoaded) { + body = bodyBytes == null ? null : new ByteArrayInputStream(bodyBytes); + } + } + + if (body == null) { + throw new IOException("Response body missing, stream null"); + } + + return body; + } + + /** + * Get the raw implementation specific body stream for this response. + * + * This method will only be called once to completion. If an exception is thrown, it may be called multiple times. + * + * @return the stream for the raw response + * @throws IOException + * if an I/O Exception occurs. + */ + @CheckForNull + protected abstract InputStream rawBodyStream() throws IOException; /** * Gets the {@link GitHubConnector} for this response. @@ -123,8 +182,33 @@ public Map> allHeaders() { * * @return true when unbuffered body stream can should be used. */ - boolean useUnbufferedBodyStream() { - return statusCode() == HTTP_OK && request().avoidBufferedResponseStream(); + boolean useBufferedBodyStream() { + synchronized (this) { + return forceBufferedBodyStream || statusCode() != HTTP_OK; + } + } + + /** + * Use unbufferred body stream. + * + * @return true when unbuffered body stream can should be used. + */ + public void forceBufferedBodyStream() { + synchronized (this) { + this.forceBufferedBodyStream = true; + } + } + + /** + * {@inheritDoc} + */ + @Override + public void close() throws IOException { + synchronized (this) { + IOUtils.closeQuietly(bodyStream); + isClosed = true; + this.bodyBytes = null; + } } /** @@ -169,10 +253,6 @@ public final int parseInt(String name) throws NumberFormatException { */ public abstract static class ByteArrayResponse extends GitHubConnectorResponse { - private boolean inputStreamRead = false; - private byte[] inputBytes = null; - private boolean isClosed = false; - /** * Constructor for ByteArray Response * @@ -188,61 +268,5 @@ protected ByteArrayResponse(@Nonnull GitHubConnectorRequest request, @Nonnull Map> headers) { super(request, statusCode, headers); } - - /** - * {@inheritDoc} - */ - @Override - @Nonnull - public InputStream bodyStream() throws IOException { - if (isClosed) { - throw new IOException("Response is closed"); - } - - synchronized (this) { - InputStream body; - if (!inputStreamRead) { - body = wrapStream(rawBodyStream()); - if (!useUnbufferedBodyStream()) { - try (InputStream stream = body) { - if (stream != null) { - inputBytes = IOUtils.toByteArray(stream); - } - } - } - inputStreamRead = true; - if (useUnbufferedBodyStream()) { - return body; - } - } - } - - if (useUnbufferedBodyStream()) { - throw new IOException("Response is already consumed"); - } else if (inputBytes == null) { - throw new IOException("Response body missing, stream null"); - } - - return new ByteArrayInputStream(inputBytes); - } - - /** - * Get the raw implementation specific body stream for this response. - * - * This method will only be called once to completion. If an exception is thrown, it may be called multiple - * times. - * - * @return the stream for the raw response - * @throws IOException - * if an I/O Exception occurs. - */ - @CheckForNull - protected abstract InputStream rawBodyStream() throws IOException; - - @Override - public void close() throws IOException { - isClosed = true; - this.inputBytes = null; - } } } diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java index 2e2a4fde95..2d9d63dd32 100644 --- a/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java +++ b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java @@ -19,6 +19,8 @@ import java.util.Optional; import java.util.Scanner; import java.util.function.Function; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.stream.Collectors; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; @@ -394,10 +396,14 @@ public void testArtifacts() throws IOException { checkArtifactProperties(artifacts.get(0), "artifact1"); checkArtifactProperties(artifacts.get(1), "artifact2"); + Logger clientLogger = Logger.getLogger(GitHubClient.class.getName()); + // Test download from upload-artifact@v3 infrastructure String artifactContent = artifacts.get(0).download((is) -> { - assertThat(is, not(isA(ByteArrayInputStream.class))); - + // At finest log level, all body responses are byte arrays. + if (clientLogger.getLevel() != Level.FINEST) { + assertThat(is, not(isA(ByteArrayInputStream.class))); + } try (ZipInputStream zis = new ZipInputStream(is)) { StringBuilder sb = new StringBuilder(); diff --git a/src/test/java/org/kohsuke/github/RequesterRetryTest.java b/src/test/java/org/kohsuke/github/RequesterRetryTest.java index fba5323c3c..0f07444b25 100644 --- a/src/test/java/org/kohsuke/github/RequesterRetryTest.java +++ b/src/test/java/org/kohsuke/github/RequesterRetryTest.java @@ -275,9 +275,6 @@ public void testGitHubIsApiUrlValid() throws Exception { */ @Test public void testResponseCodeFailureExceptions() throws Exception { - // Cover default method in GitHubConnectorRequest - assertThat(IGNORED_EMPTY_REQUEST.avoidBufferedResponseStream(), equalTo(false)); - // No retry for these Exceptions GitHubConnector connector = new SendThrowingGitHubConnector<>(() -> { throw new IOException("Custom"); @@ -654,6 +651,12 @@ public Map> allHeaders() { public void close() throws IOException { wrapped.close(); } + + @Override + protected InputStream rawBodyStream() throws IOException { + // TODO Auto-generated method stub + throw new UnsupportedOperationException("Unimplemented method 'rawBodyStream'"); + } } /** From 89c2efc91baf1f12c50f00e042fc42e5067b8f27 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Mon, 17 Mar 2025 02:05:13 -0700 Subject: [PATCH 7/7] Cleanup and test coverage --- .../java/org/kohsuke/github/GitHubClient.java | 2 +- .../connector/GitHubConnectorResponse.java | 93 +++++--- .../extras/HttpClientGitHubConnector.java | 3 +- .../extras/okhttp3/OkHttpGitHubConnector.java | 2 +- .../kohsuke/github/RequesterRetryTest.java | 48 +--- .../GitHubConnectorResponseTest.java | 223 ++++++++++++++++++ 6 files changed, 289 insertions(+), 82 deletions(-) create mode 100644 src/test/java/org/kohsuke/github/connector/GitHubConnectorResponseTest.java diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index 2cad9f6923..176e6cb980 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -629,7 +629,7 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) { LOGGER.log(FINEST, () -> { String body; try { - response.forceBufferedBodyStream(); + response.setBodyStreamRereadable(); body = GitHubResponse.getBodyAsString(response); } catch (Throwable e) { body = "Error reading response body"; diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index b1ef4cbbdb..3098105eb0 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -23,6 +23,9 @@ /** * Response information supplied when a response is received and before the body is processed. *

+ * During a request to GitHub, {@link GitHubConnector#send(GitHubConnectorRequest)} returns a + * {@link GitHubConnectorResponse}. This is processed to create a GitHubResponse. + *

* Instances of this class are closed once the response is done being processed. This means that {@link #bodyStream()} * will not be readable after a call is completed. * @@ -43,11 +46,10 @@ public abstract class GitHubConnectorResponse implements Closeable { @Nonnull private final Map> headers; private boolean bodyStreamCalled = false; - private boolean bodyBytesLoaded = false; private InputStream bodyStream = null; private byte[] bodyBytes = null; private boolean isClosed = false; - private boolean forceBufferedBodyStream; + private boolean isBodyStreamRereadable; /** * GitHubConnectorResponse constructor @@ -71,6 +73,7 @@ protected GitHubConnectorResponse(@Nonnull GitHubConnectorRequest request, caseInsensitiveMap.put(entry.getKey(), Collections.unmodifiableList(new ArrayList<>(entry.getValue()))); } this.headers = Collections.unmodifiableMap(caseInsensitiveMap); + this.isBodyStreamRereadable = false; } /** @@ -92,53 +95,60 @@ public String header(String name) { /** * The response body as an {@link InputStream}. * + * When {@link #isBodyStreamRereadable} is false, {@link #bodyStream()} can only be called once and the returned + * stream should be assumed to be read-once and not resetable. This is the default behavior for HTTP_OK responses + * and significantly reduces memory usage. + * + * When {@link #isBodyStreamRereadable} is true, {@link #bodyStream()} can be called be called multiple times. The + * full stream data is read into a byte array during the first call. Each call returns a new stream backed by the + * same byte array. This uses more memory, but is required to enable rereading the body stream during trace logging, + * debugging, and error responses. + * * @return the response body * @throws IOException * if response stream is null or an I/O Exception occurs. */ @Nonnull public InputStream bodyStream() throws IOException { - InputStream body = null; synchronized (this) { if (isClosed) { throw new IOException("Response is closed"); } if (bodyStreamCalled) { - if (!bodyBytesLoaded) { - throw new IOException("Response is already consumed"); + if (!isBodyStreamRereadable()) { + throw new IOException("Response body not rereadable"); } } else { - body = wrapStream(rawBodyStream()); + bodyStream = wrapStream(rawBodyStream()); bodyStreamCalled = true; - bodyStream = body; - if (useBufferedBodyStream()) { - bodyBytesLoaded = true; - try (InputStream stream = body) { - if (stream != null) { - bodyBytes = IOUtils.toByteArray(stream); - } - } - bodyStream = null; - } } - if (bodyBytesLoaded) { - body = bodyBytes == null ? null : new ByteArrayInputStream(bodyBytes); + if (bodyStream == null) { + throw new IOException("Response body missing, stream null"); + } else if (!isBodyStreamRereadable()) { + return bodyStream; } - } - if (body == null) { - throw new IOException("Response body missing, stream null"); - } + // Load rereadable byte array + if (bodyBytes == null) { + bodyBytes = IOUtils.toByteArray(bodyStream); + // Close the raw body stream after successfully reading + IOUtils.closeQuietly(bodyStream); + } - return body; + return new ByteArrayInputStream(bodyBytes); + } } /** * Get the raw implementation specific body stream for this response. * - * This method will only be called once to completion. If an exception is thrown, it may be called multiple times. + * This method will only be called once to completion. If an exception is thrown by this method, it may be called + * multiple times. + * + * The stream returned from this method will be closed when the response is closed or sooner. Inheriting classes do + * not need to close it. * * @return the stream for the raw response * @throws IOException @@ -178,24 +188,40 @@ public Map> allHeaders() { } /** - * Use unbufferred body stream. + * The body stream rereadable state. + * + * Body stream defaults to read once for HTTP_OK responses (to reduce memory usage). For non-HTTP_OK responses, body + * stream is switched to rereadable (in-memory byte array) for error processing. * - * @return true when unbuffered body stream can should be used. + * Calling {@link #setBodyStreamRereadable()} will force {@link #isBodyStreamRereadable} to be true for this + * response regardless of {@link #statusCode} value. + * + * @return true when body stream is rereadable. */ - boolean useBufferedBodyStream() { + public boolean isBodyStreamRereadable() { synchronized (this) { - return forceBufferedBodyStream || statusCode() != HTTP_OK; + return isBodyStreamRereadable || statusCode != HTTP_OK; } } /** - * Use unbufferred body stream. + * Force body stream to rereadable regardless of status code. + * + * Calling {@link #setBodyStreamRereadable()} will force {@link #isBodyStreamRereadable} to be true for this + * response regardless of {@link #statusCode} value. * - * @return true when unbuffered body stream can should be used. + * This is required to support body value logging during low-level tracing but should be avoided in general since it + * consumes significantly more memory. + * + * Will throw runtime exception if a non-rereadable body stream has already been returned from + * {@link #bodyStream()}. */ - public void forceBufferedBodyStream() { + public void setBodyStreamRereadable() { synchronized (this) { - this.forceBufferedBodyStream = true; + if (bodyStreamCalled && !isBodyStreamRereadable()) { + throw new RuntimeException("bodyStream() already called in read-once mode"); + } + isBodyStreamRereadable = true; } } @@ -250,7 +276,10 @@ public final int parseInt(String name) throws NumberFormatException { /** * A ByteArrayResponse class + * + * @deprecated Inherit directly from {@link GitHubConnectorResponse}. */ + @Deprecated public abstract static class ByteArrayResponse extends GitHubConnectorResponse { /** diff --git a/src/main/java/org/kohsuke/github/extras/HttpClientGitHubConnector.java b/src/main/java/org/kohsuke/github/extras/HttpClientGitHubConnector.java index 52f7e610d7..aaf9d9acb7 100644 --- a/src/main/java/org/kohsuke/github/extras/HttpClientGitHubConnector.java +++ b/src/main/java/org/kohsuke/github/extras/HttpClientGitHubConnector.java @@ -93,7 +93,7 @@ public GitHubConnectorResponse send(GitHubConnectorRequest connectorRequest) thr * * Implementation specific to {@link HttpResponse}. */ - private static class HttpClientGitHubConnectorResponse extends GitHubConnectorResponse.ByteArrayResponse { + private static class HttpClientGitHubConnectorResponse extends GitHubConnectorResponse { @Nonnull private final HttpResponse response; @@ -113,7 +113,6 @@ protected InputStream rawBodyStream() throws IOException { @Override public void close() throws IOException { super.close(); - IOUtils.closeQuietly(response.body()); } } } diff --git a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java index d61fde220b..d179b1357d 100644 --- a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java +++ b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java @@ -104,7 +104,7 @@ private List TlsConnectionSpecs() { * * Implementation specific to {@link okhttp3.Response}. */ - private static class OkHttpGitHubConnectorResponse extends GitHubConnectorResponse.ByteArrayResponse { + private static class OkHttpGitHubConnectorResponse extends GitHubConnectorResponse { @Nonnull private final Response response; diff --git a/src/test/java/org/kohsuke/github/RequesterRetryTest.java b/src/test/java/org/kohsuke/github/RequesterRetryTest.java index 0f07444b25..fb031ce6d0 100644 --- a/src/test/java/org/kohsuke/github/RequesterRetryTest.java +++ b/src/test/java/org/kohsuke/github/RequesterRetryTest.java @@ -3,18 +3,17 @@ import okhttp3.ConnectionPool; import okhttp3.OkHttpClient; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; import org.junit.Before; import org.junit.Ignore; import org.junit.Test; import org.kohsuke.github.connector.GitHubConnector; import org.kohsuke.github.connector.GitHubConnectorRequest; import org.kohsuke.github.connector.GitHubConnectorResponse; +import org.kohsuke.github.connector.GitHubConnectorResponseTest; import org.kohsuke.github.extras.HttpClientGitHubConnector; import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector; import java.io.*; -import java.net.URL; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -566,55 +565,12 @@ public InputStream bodyStream() throws IOException { } } - private static final GitHubConnectorRequest IGNORED_EMPTY_REQUEST = new GitHubConnectorRequest() { - @NotNull - @Override - public String method() { - return null; - } - - @NotNull - @Override - public Map> allHeaders() { - return null; - } - - @Nullable - @Override - public String header(String name) { - return null; - } - - @Nullable - @Override - public String contentType() { - return null; - } - - @Nullable - @Override - public InputStream body() { - return null; - } - - @NotNull - @Override - public URL url() { - return null; - } - - @Override - public boolean hasBody() { - return false; - } - }; - private static class GitHubConnectorResponseWrapper extends GitHubConnectorResponse { private final GitHubConnectorResponse wrapped; GitHubConnectorResponseWrapper(GitHubConnectorResponse response) { - super(IGNORED_EMPTY_REQUEST, -1, new HashMap<>()); + super(GitHubConnectorResponseTest.EMPTY_REQUEST, -1, new HashMap<>()); wrapped = response; } diff --git a/src/test/java/org/kohsuke/github/connector/GitHubConnectorResponseTest.java b/src/test/java/org/kohsuke/github/connector/GitHubConnectorResponseTest.java new file mode 100644 index 0000000000..fbc2098902 --- /dev/null +++ b/src/test/java/org/kohsuke/github/connector/GitHubConnectorResponseTest.java @@ -0,0 +1,223 @@ +package org.kohsuke.github.connector; + +import com.fasterxml.jackson.databind.util.ByteBufferBackedInputStream; +import org.apache.commons.io.IOUtils; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; +import org.kohsuke.github.AbstractGitHubWireMockTest; +import org.kohsuke.github.connector.GitHubConnectorResponse.ByteArrayResponse; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.isA; + +/** + * Test GitHubConnectorResponse + */ +public class GitHubConnectorResponseTest extends AbstractGitHubWireMockTest { + + /** + * Test basic body stream. + * + * @throws Exception + * for failures + */ + @Test + public void testBodyStream() throws Exception { + Exception e; + GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200, + new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8)))); + InputStream stream = response.bodyStream(); + assertThat(stream, isA(ByteBufferBackedInputStream.class)); + String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8); + assertThat(bodyString, equalTo("Hello!")); + + // Cannot change to rereadable + e = Assert.assertThrows(RuntimeException.class, () -> response.setBodyStreamRereadable()); + assertThat(e.getMessage(), equalTo("bodyStream() already called in read-once mode")); + + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response body not rereadable")); + response.close(); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response is closed")); + } + + /** + * Test rereadable body stream. + * + * @throws Exception + * for failures + */ + @Test + public void tesBodyStream_rereadable() throws Exception { + Exception e; + GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(404, + new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8)))); + InputStream stream = response.bodyStream(); + assertThat(stream, isA(ByteArrayInputStream.class)); + String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8); + assertThat(bodyString, equalTo("Hello!")); + + // Buffered response can be read multiple times + bodyString = IOUtils.toString(response.bodyStream(), StandardCharsets.UTF_8); + assertThat(bodyString, equalTo("Hello!")); + + // should have no effect if already rereadable + response.setBodyStreamRereadable(); + + response.close(); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response is closed")); + } + + /** + * Test forced rereadable body stream. + * + * @throws Exception + * for failures + */ + @Test + public void tesBodyStream_forced() throws Exception { + Exception e; + GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200, + new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8)))); + // 200 status would be streamed body, force to buffered + response.setBodyStreamRereadable(); + + InputStream stream = response.bodyStream(); + assertThat(stream, isA(ByteArrayInputStream.class)); + String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8); + assertThat(bodyString, equalTo("Hello!")); + + // Buffered response can be read multiple times + bodyString = IOUtils.toString(response.bodyStream(), StandardCharsets.UTF_8); + assertThat(bodyString, equalTo("Hello!")); + + response.close(); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response is closed")); + } + + /** + * Test null body stream. + * + * @throws Exception + * for failures + */ + @Test + public void testBodyStream_null() throws Exception { + Exception e; + GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200, null); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response body missing, stream null")); + + // Cannot change to rereadable + e = Assert.assertThrows(RuntimeException.class, () -> response.setBodyStreamRereadable()); + assertThat(e.getMessage(), equalTo("bodyStream() already called in read-once mode")); + + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response body not rereadable")); + + response.close(); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response is closed")); + } + + /** + * Test null rereadable body stream. + * + * @throws Exception + * for failures + */ + @Test + public void testBodyStream_null_buffered() throws Exception { + Exception e; + GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(404, null); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response body missing, stream null")); + // Buffered response can be read multiple times + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response body missing, stream null")); + + // force should have no effect after first read attempt + response.setBodyStreamRereadable(); + + response.close(); + e = Assert.assertThrows(IOException.class, () -> response.bodyStream()); + assertThat(e.getMessage(), equalTo("Response is closed")); + } + + // Extend ByteArrayResponse to preserve test coverage + private static class CustomBodyGitHubConnectorResponse extends ByteArrayResponse { + private final InputStream stream; + + CustomBodyGitHubConnectorResponse(int statusCode, InputStream stream) { + super(EMPTY_REQUEST, statusCode, new HashMap<>()); + this.stream = stream; + } + + @Override + protected InputStream rawBodyStream() throws IOException { + return stream; + } + } + + /** + * Empty request for response testing. + */ + public static final GitHubConnectorRequest EMPTY_REQUEST = new GitHubConnectorRequest() { + @NotNull + @Override + public String method() { + return null; + } + + @NotNull + @Override + public Map> allHeaders() { + return null; + } + + @Nullable + @Override + public String header(String name) { + return null; + } + + @Nullable + @Override + public String contentType() { + return null; + } + + @Nullable + @Override + public InputStream body() { + return null; + } + + @NotNull + @Override + public URL url() { + return null; + } + + @Override + public boolean hasBody() { + return false; + } + }; + +}