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 f669280b4d..176e6cb980 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -629,6 +629,7 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) { LOGGER.log(FINEST, () -> { String body; try { + 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 b71fc8abc5..3098105eb0 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -7,15 +7,25 @@ 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; 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. *

+ * 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. * @@ -35,6 +45,11 @@ public abstract class GitHubConnectorResponse implements Closeable { private final GitHubConnectorRequest request; @Nonnull private final Map> headers; + private boolean bodyStreamCalled = false; + private InputStream bodyStream = null; + private byte[] bodyBytes = null; + private boolean isClosed = false; + private boolean isBodyStreamRereadable; /** * GitHubConnectorResponse constructor @@ -58,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; } /** @@ -79,17 +95,72 @@ 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 abstract InputStream bodyStream() throws IOException; + public InputStream bodyStream() throws IOException { + synchronized (this) { + if (isClosed) { + throw new IOException("Response is closed"); + } + + if (bodyStreamCalled) { + if (!isBodyStreamRereadable()) { + throw new IOException("Response body not rereadable"); + } + } else { + bodyStream = wrapStream(rawBodyStream()); + bodyStreamCalled = true; + } + + if (bodyStream == null) { + throw new IOException("Response body missing, stream null"); + } else if (!isBodyStreamRereadable()) { + return bodyStream; + } + + // Load rereadable byte array + if (bodyBytes == null) { + bodyBytes = IOUtils.toByteArray(bodyStream); + // Close the raw body stream after successfully reading + IOUtils.closeQuietly(bodyStream); + } + + return new ByteArrayInputStream(bodyBytes); + } + } /** - * Gets the {@link GitHubConnectorRequest} for this response. + * Get the raw implementation specific body stream for this response. + * + * This method will only be called once to completion. If an exception is thrown by this method, it may be called + * multiple times. * - * @return the {@link GitHubConnectorRequest} for this response. + * 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 + * if an I/O Exception occurs. + */ + @CheckForNull + protected abstract InputStream rawBodyStream() throws IOException; + + /** + * Gets the {@link GitHubConnector} for this response. + * + * @return the {@link GitHubConnector} for this response. */ @Nonnull public GitHubConnectorRequest request() { @@ -116,6 +187,56 @@ public Map> allHeaders() { return headers; } + /** + * 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. + * + * 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. + */ + public boolean isBodyStreamRereadable() { + synchronized (this) { + return isBodyStreamRereadable || statusCode != HTTP_OK; + } + } + + /** + * 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. + * + * 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 setBodyStreamRereadable() { + synchronized (this) { + if (bodyStreamCalled && !isBodyStreamRereadable()) { + throw new RuntimeException("bodyStream() already called in read-once mode"); + } + isBodyStreamRereadable = true; + } + } + + /** + * {@inheritDoc} + */ + @Override + public void close() throws IOException { + synchronized (this) { + IOUtils.closeQuietly(bodyStream); + isClosed = true; + this.bodyBytes = null; + } + } + /** * Handles wrapping the body stream if indicated by the "Content-Encoding" header. * @@ -155,13 +276,12 @@ 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 { - private boolean inputStreamRead = false; - private byte[] inputBytes = null; - private boolean isClosed = false; - /** * Constructor for ByteArray Response * @@ -177,52 +297,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) { - if (!inputStreamRead) { - InputStream rawStream = rawBodyStream(); - try (InputStream stream = wrapStream(rawStream)) { - if (stream != null) { - inputBytes = IOUtils.toByteArray(stream); - } - } - inputStreamRead = true; - } - } - - 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/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 f1c7679230..48ec9e137a 100644 --- a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java +++ b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java @@ -103,7 +103,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/GHWorkflowRunTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java index e3727d7c56..2d9d63dd32 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; @@ -18,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; @@ -393,8 +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) -> { + // 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 81a8b34ec9..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; } @@ -651,6 +607,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'"); + } } /** 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; + } + }; + +}