Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Use BufferedInputStream to inspect HttpResponse error #1411

Merged
merged 2 commits into from Jul 12, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -18,6 +18,7 @@
import com.google.api.client.util.LoggingInputStream;
import com.google.api.client.util.Preconditions;
import com.google.api.client.util.StringUtils;
import java.io.BufferedInputStream;
import java.io.ByteArrayOutputStream;
import java.io.EOFException;
import java.io.IOException;
Expand Down Expand Up @@ -371,6 +372,11 @@ public InputStream getContent() throws IOException {
new LoggingInputStream(
lowLevelResponseContent, logger, Level.CONFIG, contentLoggingLimit);
}
if (!returnRawInputStream) {
Copy link
Contributor

@elharo elharo Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marginally cleaner to do an if-else

if (returnRawInputStream) {
  content = lowLevelResponseContent;
} else {
  content = new BufferedInputStream(lowLevelResponseContent);
}

This avoids reassigning a local variable which can be confusing and error prone in the long term.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose this way just because all previous code in this method used this way. lowLevelResponseContent reassigned two times in preceding code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth cleaning this up in a separate PR some time, but for now please don't introduce more reassignments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// wrap the content with BufferedInputStream to support
// mark()/reset() while error checking in error handlers
lowLevelResponseContent = new BufferedInputStream(lowLevelResponseContent);
}
content = lowLevelResponseContent;
contentProcessed = true;
} catch (EOFException e) {
Expand Down
Expand Up @@ -22,9 +22,13 @@
import com.google.api.client.testing.util.LogRecordingHandler;
import com.google.api.client.testing.util.TestableByteArrayInputStream;
import com.google.api.client.util.Key;
import com.google.common.io.ByteStreams;
import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.Type;
import java.nio.charset.StandardCharsets;
import java.text.NumberFormat;
Expand Down Expand Up @@ -59,6 +63,8 @@ public void testParseAsString_none() throws Exception {
private static final String SAMPLE = "123\u05D9\u05e0\u05D9\u05D1";
private static final String SAMPLE2 = "123abc";
private static final String JSON_SAMPLE = "{\"foo\": \"ßar\"}";
private static final String ERROR_SAMPLE =
"{domain:'global',reason:'domainPolicy',message:'msg'}";
private static final String VALID_CONTENT_TYPE = "text/plain";
private static final String VALID_CONTENT_TYPE_WITH_PARAMS =
"application/vnd.com.google.datastore.entity+json; charset=utf-8; version=v1; q=0.9";
Expand Down Expand Up @@ -543,7 +549,8 @@ public LowLevelHttpResponse execute() throws IOException {
};
HttpRequest request =
transport.createRequestFactory().buildHeadRequest(HttpTesting.SIMPLE_GENERIC_URL);
request.execute().getContent();
InputStream noContent = request.execute().getContent();
assertNull(noContent);
}

public void testGetContent_gzipEncoding_ReturnRawStream() throws IOException {
Expand All @@ -570,6 +577,9 @@ public LowLevelHttpResponse execute() throws IOException {
assertFalse(
"it should not decompress stream",
request.execute().getContent() instanceof GZIPInputStream);
assertFalse(
"it should not buffer stream",
request.execute().getContent() instanceof BufferedInputStream);
}

public void testGetContent_gzipEncoding_finishReading() throws IOException {
Expand Down Expand Up @@ -665,4 +675,71 @@ public LowLevelHttpResponse execute() throws IOException {
HttpResponse response = request.execute();
assertEquals("abcd", response.parseAsString());
}

public void testGetContent_bufferedContent() throws IOException {
HttpTransport transport =
new MockHttpTransport() {
@Override
public LowLevelHttpRequest buildRequest(String method, String url) throws IOException {
return new MockLowLevelHttpRequest() {
@Override
public LowLevelHttpResponse execute() throws IOException {
// have to use gzip here because MockLowLevelHttpResponse.setContent()
// returns BufferedStream by itself, so test always success
byte[] dataToCompress = ERROR_SAMPLE.getBytes(StandardCharsets.UTF_8);
ByteArrayOutputStream content = new ByteArrayOutputStream(dataToCompress.length);
try (GZIPOutputStream zipStream = new GZIPOutputStream((content))) {
zipStream.write(dataToCompress);
}

MockLowLevelHttpResponse result = new MockLowLevelHttpResponse();
result.setStatusCode(403);
result.setContentType(JSON_CONTENT_TYPE);
result.setContentEncoding("gzip");
result.setContent(content.toByteArray());

return result;
}
};
}
};
HttpRequest request =
transport
.createRequestFactory()
.buildGetRequest(HttpTesting.SIMPLE_GENERIC_URL)
.setThrowExceptionOnExecuteError(false);

HttpResponse response = request.execute();
InputStream content = response.getContent();
assertTrue(content.markSupported());

// inspect content like in HttpUnsuccessfulResponseHandler
try (RollbackInputStream is = new RollbackInputStream(content)) {
byte[] bytes = ByteStreams.toByteArray(is);
String text = new String(bytes, response.getContentCharset());
assertEquals(ERROR_SAMPLE, text);
}

// original response still parsable by HttpResponseException
HttpResponseException exception = new HttpResponseException(response);
assertEquals(exception.getStatusCode(), 403);
assertEquals(exception.getContent(), ERROR_SAMPLE);
}

static class RollbackInputStream extends FilterInputStream {
private boolean closed;

RollbackInputStream(InputStream in) {
super(in);
in.mark(8192); // big enough to keep most error messages
}

@Override
public void close() throws IOException {
if (!closed) {
closed = true;
in.reset();
}
}
}
}