Skip to content

Commit 9a18e00

Browse files
bitwisemanbitwiseman
committed
Cleanup and test coverage
1 parent 7b7f1bd commit 9a18e00

File tree

4 files changed

+242
-58
lines changed

4 files changed

+242
-58
lines changed

‎src/main/java/org/kohsuke/github/GitHubClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -629,7 +629,7 @@ private void logResponseBody(@Nonnull final GitHubConnectorResponse response) {
629629
LOGGER.log(FINEST, () -> {
630630
String body;
631631
try {
632-
response.forceBufferedBodyStream();
632+
response.setBodyStreamRereadable();
633633
body = GitHubResponse.getBodyAsString(response);
634634
} catch (Throwable e) {
635635
body = "Error reading response body";

‎src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public abstract class GitHubConnectorResponse implements Closeable {
4747
private InputStream bodyStream = null;
4848
private byte[] bodyBytes = null;
4949
private boolean isClosed = false;
50-
private boolean forceBufferedBodyStream;
50+
private boolean isBodyStreamRereadable;
5151

5252
/**
5353
* GitHubConnectorResponse constructor
@@ -71,6 +71,7 @@ protected GitHubConnectorResponse(@Nonnull GitHubConnectorRequest request,
7171
caseInsensitiveMap.put(entry.getKey(), Collections.unmodifiableList(new ArrayList<>(entry.getValue())));
7272
}
7373
this.headers = Collections.unmodifiableMap(caseInsensitiveMap);
74+
this.isBodyStreamRereadable = false;
7475
}
7576

7677
/**
@@ -106,13 +107,13 @@ public InputStream bodyStream() throws IOException {
106107

107108
if (bodyStreamCalled) {
108109
if (!bodyBytesLoaded) {
109-
throw new IOException("Response is already consumed");
110+
throw new IOException("Response body not rereadable");
110111
}
111112
} else {
112113
body = wrapStream(rawBodyStream());
113114
bodyStreamCalled = true;
114115
bodyStream = body;
115-
if (useBufferedBodyStream()) {
116+
if (isBodyStreamRereadable()) {
116117
bodyBytesLoaded = true;
117118
try (InputStream stream = body) {
118119
if (stream != null) {
@@ -178,24 +179,30 @@ public Map<String, List<String>> allHeaders() {
178179
}
179180

180181
/**
181-
* Use unbufferred body stream.
182+
* The body stream rereadable state.
182183
*
183-
* @return true when unbuffered body stream can should be used.
184+
* Body stream defaults to read once for HTTP_OK responses (to reduce memory usage). For non-HTTP_OK responses, body
185+
* stream is switched to rereadable (in-memory byte array) for error processing.
186+
*
187+
* @return true when body stream is rereadable.
184188
*/
185-
boolean useBufferedBodyStream() {
189+
public boolean isBodyStreamRereadable() {
186190
synchronized (this) {
187-
return forceBufferedBodyStream || statusCode() != HTTP_OK;
191+
return isBodyStreamRereadable || statusCode != HTTP_OK;
188192
}
189193
}
190194

191195
/**
192-
* Use unbufferred body stream.
196+
* Force body stream to rereadable regardless of status code.
193197
*
194-
* @return true when unbuffered body stream can should be used.
198+
* Will throw runtime exception if a non-rereadable body stream has already be loaded.
195199
*/
196-
public void forceBufferedBodyStream() {
200+
public void setBodyStreamRereadable() {
197201
synchronized (this) {
198-
this.forceBufferedBodyStream = true;
202+
if (bodyStreamCalled && !isBodyStreamRereadable()) {
203+
throw new RuntimeException("Response.bodyStream() already called");
204+
}
205+
isBodyStreamRereadable = true;
199206
}
200207
}
201208

‎src/test/java/org/kohsuke/github/RequesterRetryTest.java‎

Lines changed: 2 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,17 @@
33
import okhttp3.ConnectionPool;
44
import okhttp3.OkHttpClient;
55
import org.jetbrains.annotations.NotNull;
6-
import org.jetbrains.annotations.Nullable;
76
import org.junit.Before;
87
import org.junit.Ignore;
98
import org.junit.Test;
109
import org.kohsuke.github.connector.GitHubConnector;
1110
import org.kohsuke.github.connector.GitHubConnectorRequest;
1211
import org.kohsuke.github.connector.GitHubConnectorResponse;
12+
import org.kohsuke.github.connector.GitHubConnectorResponseTest;
1313
import org.kohsuke.github.extras.HttpClientGitHubConnector;
1414
import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector;
1515

1616
import java.io.*;
17-
import java.net.URL;
1817
import java.util.HashMap;
1918
import java.util.List;
2019
import java.util.Map;
@@ -566,55 +565,12 @@ public InputStream bodyStream() throws IOException {
566565
}
567566
}
568567

569-
private static final GitHubConnectorRequest IGNORED_EMPTY_REQUEST = new GitHubConnectorRequest() {
570-
@NotNull
571-
@Override
572-
public String method() {
573-
return null;
574-
}
575-
576-
@NotNull
577-
@Override
578-
public Map<String, List<String>> allHeaders() {
579-
return null;
580-
}
581-
582-
@Nullable
583-
@Override
584-
public String header(String name) {
585-
return null;
586-
}
587-
588-
@Nullable
589-
@Override
590-
public String contentType() {
591-
return null;
592-
}
593-
594-
@Nullable
595-
@Override
596-
public InputStream body() {
597-
return null;
598-
}
599-
600-
@NotNull
601-
@Override
602-
public URL url() {
603-
return null;
604-
}
605-
606-
@Override
607-
public boolean hasBody() {
608-
return false;
609-
}
610-
};
611-
612568
private static class GitHubConnectorResponseWrapper extends GitHubConnectorResponse {
613569

614570
private final GitHubConnectorResponse wrapped;
615571

616572
GitHubConnectorResponseWrapper(GitHubConnectorResponse response) {
617-
super(IGNORED_EMPTY_REQUEST, -1, new HashMap<>());
573+
super(GitHubConnectorResponseTest.EMPTY_REQUEST, -1, new HashMap<>());
618574
wrapped = response;
619575
}
620576

‎src/test/java/org/kohsuke/github/connector/GitHubConnectorResponseTest.java‎

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
package org.kohsuke.github.connector;
2+
3+
import com.fasterxml.jackson.databind.util.ByteBufferBackedInputStream;
4+
import org.apache.commons.io.IOUtils;
5+
import org.jetbrains.annotations.NotNull;
6+
import org.jetbrains.annotations.Nullable;
7+
import org.junit.Assert;
8+
import org.junit.Test;
9+
import org.kohsuke.github.AbstractGitHubWireMockTest;
10+
11+
import java.io.ByteArrayInputStream;
12+
import java.io.IOException;
13+
import java.io.InputStream;
14+
import java.net.URL;
15+
import java.nio.ByteBuffer;
16+
import java.nio.charset.StandardCharsets;
17+
import java.util.HashMap;
18+
import java.util.List;
19+
import java.util.Map;
20+
21+
import static org.hamcrest.Matchers.equalTo;
22+
import static org.hamcrest.Matchers.isA;
23+
24+
/**
25+
* Test GitHubConnectorResponse
26+
*/
27+
public class GitHubConnectorResponseTest extends AbstractGitHubWireMockTest {
28+
29+
/**
30+
* Test basic body stream.
31+
*
32+
* @throws Exception
33+
* for failures
34+
*/
35+
@Test
36+
public void testBodyStream() throws Exception {
37+
Exception e;
38+
GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200,
39+
new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8))));
40+
InputStream stream = response.bodyStream();
41+
assertThat(stream, isA(ByteBufferBackedInputStream.class));
42+
String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8);
43+
assertThat(bodyString, equalTo("Hello!"));
44+
45+
// Cannot change to rereadable
46+
e = Assert.assertThrows(RuntimeException.class, () -> response.setBodyStreamRereadable());
47+
assertThat(e.getMessage(), equalTo("Response.bodyStream() already called"));
48+
49+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
50+
assertThat(e.getMessage(), equalTo("Response body not rereadable"));
51+
response.close();
52+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
53+
assertThat(e.getMessage(), equalTo("Response is closed"));
54+
}
55+
56+
/**
57+
* Test rereadable body stream.
58+
*
59+
* @throws Exception
60+
* for failures
61+
*/
62+
@Test
63+
public void tesBodyStream_rereadable() throws Exception {
64+
Exception e;
65+
GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(404,
66+
new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8))));
67+
InputStream stream = response.bodyStream();
68+
assertThat(stream, isA(ByteArrayInputStream.class));
69+
String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8);
70+
assertThat(bodyString, equalTo("Hello!"));
71+
72+
// Buffered response can be read multiple times
73+
bodyString = IOUtils.toString(response.bodyStream(), StandardCharsets.UTF_8);
74+
assertThat(bodyString, equalTo("Hello!"));
75+
76+
// should have no effect if already rereadable
77+
response.setBodyStreamRereadable();
78+
79+
response.close();
80+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
81+
assertThat(e.getMessage(), equalTo("Response is closed"));
82+
}
83+
84+
/**
85+
* Test forced rereadable body stream.
86+
*
87+
* @throws Exception
88+
* for failures
89+
*/
90+
@Test
91+
public void tesBodyStream_forced() throws Exception {
92+
Exception e;
93+
GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200,
94+
new ByteBufferBackedInputStream(ByteBuffer.wrap("Hello!".getBytes(StandardCharsets.UTF_8))));
95+
// 200 status would be streamed body, force to buffered
96+
response.setBodyStreamRereadable();
97+
98+
InputStream stream = response.bodyStream();
99+
assertThat(stream, isA(ByteArrayInputStream.class));
100+
String bodyString = IOUtils.toString(stream, StandardCharsets.UTF_8);
101+
assertThat(bodyString, equalTo("Hello!"));
102+
103+
// Buffered response can be read multiple times
104+
bodyString = IOUtils.toString(response.bodyStream(), StandardCharsets.UTF_8);
105+
assertThat(bodyString, equalTo("Hello!"));
106+
107+
response.close();
108+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
109+
assertThat(e.getMessage(), equalTo("Response is closed"));
110+
}
111+
112+
/**
113+
* Test null body stream.
114+
*
115+
* @throws Exception
116+
* for failures
117+
*/
118+
@Test
119+
public void testBodyStream_null() throws Exception {
120+
Exception e;
121+
GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(200, null);
122+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
123+
assertThat(e.getMessage(), equalTo("Response body missing, stream null"));
124+
125+
// Cannot change to rereadable
126+
e = Assert.assertThrows(RuntimeException.class, () -> response.setBodyStreamRereadable());
127+
assertThat(e.getMessage(), equalTo("Response.bodyStream() already called"));
128+
129+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
130+
assertThat(e.getMessage(), equalTo("Response body not rereadable"));
131+
132+
response.close();
133+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
134+
assertThat(e.getMessage(), equalTo("Response is closed"));
135+
}
136+
137+
/**
138+
* Test null rereadable body stream.
139+
*
140+
* @throws Exception
141+
* for failures
142+
*/
143+
@Test
144+
public void testBodyStream_null_buffered() throws Exception {
145+
Exception e;
146+
GitHubConnectorResponse response = new CustomBodyGitHubConnectorResponse(404, null);
147+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
148+
assertThat(e.getMessage(), equalTo("Response body missing, stream null"));
149+
// Buffered response can be read multiple times
150+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
151+
assertThat(e.getMessage(), equalTo("Response body missing, stream null"));
152+
153+
// force should have no effect after first read attempt
154+
response.setBodyStreamRereadable();
155+
156+
response.close();
157+
e = Assert.assertThrows(IOException.class, () -> response.bodyStream());
158+
assertThat(e.getMessage(), equalTo("Response is closed"));
159+
}
160+
161+
private static class CustomBodyGitHubConnectorResponse extends GitHubConnectorResponse {
162+
private final InputStream stream;
163+
164+
CustomBodyGitHubConnectorResponse(int statusCode, InputStream stream) {
165+
super(EMPTY_REQUEST, statusCode, new HashMap<>());
166+
this.stream = stream;
167+
}
168+
169+
@Override
170+
protected InputStream rawBodyStream() throws IOException {
171+
return stream;
172+
}
173+
}
174+
175+
/**
176+
* Empty request for response testing.
177+
*/
178+
public static final GitHubConnectorRequest EMPTY_REQUEST = new GitHubConnectorRequest() {
179+
@NotNull
180+
@Override
181+
public String method() {
182+
return null;
183+
}
184+
185+
@NotNull
186+
@Override
187+
public Map<String, List<String>> allHeaders() {
188+
return null;
189+
}
190+
191+
@Nullable
192+
@Override
193+
public String header(String name) {
194+
return null;
195+
}
196+
197+
@Nullable
198+
@Override
199+
public String contentType() {
200+
return null;
201+
}
202+
203+
@Nullable
204+
@Override
205+
public InputStream body() {
206+
return null;
207+
}
208+
209+
@NotNull
210+
@Override
211+
public URL url() {
212+
return null;
213+
}
214+
215+
@Override
216+
public boolean hasBody() {
217+
return false;
218+
}
219+
};
220+
221+
}

0 commit comments

Comments
 (0)