Skip to content

Commit ecd30c8

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

File tree

4 files changed

+239
-58
lines changed

4 files changed

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

0 commit comments

Comments
 (0)