Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit bbaa4c3

Browse files
bfgslandelle
authored andcommitted
Retrofit http client supplier npe fix (#1617)
* Lombok update to 1.18.6 * Fixed NPE when http client supplier was specied and http client was not. This patch addresses issue that resulted in NPE if http client supplier was specified in `Call.Factory` builder and concrete http client was not, because `getHttpClient()` method was not invoked while constructing retrofit `Call` instance. New, obviously less error prone approach is that http client supplier gets constructed behind the scenes even if user specifies concrete http client instance at call factory creation time and http client supplier is being used exclusively also by `Call` instance. This way there are no hard references to http client instance dangling around in case some component creates a `Call` instance and never issues `newCall()` on it. Fixes #1616.
1 parent fdf9a7b commit bbaa4c3

File tree

5 files changed

+120
-38
lines changed

5 files changed

+120
-38
lines changed

‎extras/retrofit2/pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
<properties>
1515
<retrofit2.version>2.5.0</retrofit2.version>
16-
<lombok.version>1.16.20</lombok.version>
16+
<lombok.version>1.18.6</lombok.version>
1717
</properties>
1818

1919
<dependencies>

‎extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import java.util.concurrent.TimeUnit;
3131
import java.util.concurrent.atomic.AtomicReference;
3232
import java.util.function.Consumer;
33+
import java.util.function.Supplier;
3334

3435
/**
3536
* {@link AsyncHttpClient} <a href="http://square.github.io/retrofit/">Retrofit2</a> {@link okhttp3.Call}
@@ -48,44 +49,52 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call {
4849
public static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000;
4950

5051
private static final ResponseBody EMPTY_BODY = ResponseBody.create(null, "");
52+
5153
/**
5254
* Tells whether call has been executed.
5355
*
5456
* @see #isExecuted()
5557
* @see #isCanceled()
5658
*/
5759
private final AtomicReference<CompletableFuture<Response>> futureRef = new AtomicReference<>();
60+
5861
/**
59-
* HttpClient instance.
62+
* {@link AsyncHttpClient} supplier
6063
*/
6164
@NonNull
62-
AsyncHttpClient httpClient;
65+
Supplier<AsyncHttpClient> httpClientSupplier;
66+
6367
/**
6468
* {@link #execute()} response timeout in milliseconds.
6569
*/
6670
@Builder.Default
6771
long executeTimeoutMillis = DEFAULT_EXECUTE_TIMEOUT_MILLIS;
72+
6873
/**
6974
* Retrofit request.
7075
*/
7176
@NonNull
7277
@Getter(AccessLevel.NONE)
7378
Request request;
79+
7480
/**
7581
* List of consumers that get called just before actual async-http-client request is being built.
7682
*/
7783
@Singular("requestCustomizer")
7884
List<Consumer<RequestBuilder>> requestCustomizers;
85+
7986
/**
8087
* List of consumers that get called just before actual HTTP request is being fired.
8188
*/
8289
@Singular("onRequestStart")
8390
List<Consumer<Request>> onRequestStart;
91+
8492
/**
8593
* List of consumers that get called when HTTP request finishes with an exception.
8694
*/
8795
@Singular("onRequestFailure")
8896
List<Consumer<Throwable>> onRequestFailure;
97+
8998
/**
9099
* List of consumers that get called when HTTP request finishes successfully.
91100
*/
@@ -236,6 +245,20 @@ public Response onCompleted(org.asynchttpclient.Response response) {
236245
return future;
237246
}
238247

248+
/**
249+
* Returns HTTP client.
250+
*
251+
* @return http client
252+
* @throws IllegalArgumentException if {@link #httpClientSupplier} returned {@code null}.
253+
*/
254+
protected AsyncHttpClient getHttpClient() {
255+
val httpClient = httpClientSupplier.get();
256+
if (httpClient == null) {
257+
throw new IllegalStateException("Async HTTP client instance supplier " + httpClientSupplier + " returned null.");
258+
}
259+
return httpClient;
260+
}
261+
239262
/**
240263
* Converts async-http-client response to okhttp response.
241264
*

‎extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,42 +18,36 @@
1818
import org.asynchttpclient.AsyncHttpClient;
1919

2020
import java.util.List;
21-
import java.util.Optional;
2221
import java.util.function.Consumer;
2322
import java.util.function.Supplier;
2423

2524
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers;
2625

2726
/**
28-
* {@link AsyncHttpClient} implementation of Retrofit2 {@link Call.Factory}
27+
* {@link AsyncHttpClient} implementation of <a href="http://square.github.io/retrofit/">Retrofit2</a>
28+
* {@link Call.Factory}.
2929
*/
3030
@Value
3131
@Builder(toBuilder = true)
3232
public class AsyncHttpClientCallFactory implements Call.Factory {
3333
/**
34-
* {@link AsyncHttpClient} in use.
35-
*
36-
* @see #httpClientSupplier
37-
*/
38-
@Getter(AccessLevel.NONE)
39-
AsyncHttpClient httpClient;
40-
41-
/**
42-
* Supplier of {@link AsyncHttpClient}, takes precedence over {@link #httpClient}.
34+
* Supplier of {@link AsyncHttpClient}.
4335
*/
36+
@NonNull
4437
@Getter(AccessLevel.NONE)
4538
Supplier<AsyncHttpClient> httpClientSupplier;
4639

4740
/**
4841
* List of {@link Call} builder customizers that are invoked just before creating it.
4942
*/
5043
@Singular("callCustomizer")
44+
@Getter(AccessLevel.PACKAGE)
5145
List<Consumer<AsyncHttpClientCall.AsyncHttpClientCallBuilder>> callCustomizers;
5246

5347
@Override
5448
public Call newCall(Request request) {
5549
val callBuilder = AsyncHttpClientCall.builder()
56-
.httpClient(httpClient)
50+
.httpClientSupplier(httpClientSupplier)
5751
.request(request);
5852

5953
// customize builder before creating a call
@@ -64,15 +58,33 @@ public Call newCall(Request request) {
6458
}
6559

6660
/**
67-
* {@link AsyncHttpClient} in use by this factory.
61+
* Returns {@link AsyncHttpClient} from {@link #httpClientSupplier}.
6862
*
69-
* @return
63+
* @return http client.
7064
*/
71-
public AsyncHttpClient getHttpClient() {
72-
return Optional.ofNullable(httpClientSupplier)
73-
.map(Supplier::get)
74-
.map(Optional::of)
75-
.orElseGet(() -> Optional.ofNullable(httpClient))
76-
.orElseThrow(() -> new IllegalStateException("HTTP client is not set."));
65+
AsyncHttpClient getHttpClient() {
66+
return httpClientSupplier.get();
67+
}
68+
69+
/**
70+
* Builder for {@link AsyncHttpClientCallFactory}.
71+
*/
72+
public static class AsyncHttpClientCallFactoryBuilder {
73+
/**
74+
* {@link AsyncHttpClient} supplier that returns http client to be used to execute HTTP requests.
75+
*/
76+
private Supplier<AsyncHttpClient> httpClientSupplier;
77+
78+
/**
79+
* Sets concrete http client to be used by the factory to execute HTTP requests. Invocation of this method
80+
* overrides any previous http client supplier set by {@link #httpClientSupplier(Supplier)}!
81+
*
82+
* @param httpClient http client
83+
* @return reference to itself.
84+
* @see #httpClientSupplier(Supplier)
85+
*/
86+
public AsyncHttpClientCallFactoryBuilder httpClient(@NonNull AsyncHttpClient httpClient) {
87+
return httpClientSupplier(() -> httpClient);
88+
}
7789
}
7890
}

‎extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,23 +14,34 @@
1414

1515
import lombok.extern.slf4j.Slf4j;
1616
import lombok.val;
17+
import okhttp3.MediaType;
1718
import okhttp3.Request;
19+
import okhttp3.RequestBody;
1820
import okhttp3.Response;
1921
import org.asynchttpclient.AsyncHttpClient;
2022
import org.asynchttpclient.RequestBuilder;
2123
import org.testng.annotations.Test;
2224

25+
import java.util.Objects;
2326
import java.util.UUID;
2427
import java.util.concurrent.atomic.AtomicInteger;
2528
import java.util.function.Consumer;
2629

27-
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCallTest.REQUEST;
2830
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCallTest.createConsumer;
2931
import static org.mockito.Mockito.mock;
3032
import static org.testng.Assert.*;
3133

3234
@Slf4j
3335
public class AsyncHttpClientCallFactoryTest {
36+
private static final MediaType MEDIA_TYPE = MediaType.parse("application/json");
37+
private static final String JSON_BODY = "{\"foo\": \"bar\"}";
38+
private static final RequestBody BODY = RequestBody.create(MEDIA_TYPE, JSON_BODY);
39+
private static final String URL = "http://localhost:11000/foo/bar?a=b&c=d";
40+
private static final Request REQUEST = new Request.Builder()
41+
.post(BODY)
42+
.addHeader("X-Foo", "Bar")
43+
.url(URL)
44+
.build();
3445
@Test
3546
void newCallShouldProduceExpectedResult() {
3647
// given
@@ -152,7 +163,8 @@ void shouldApplyAllConsumersToCallBeingConstructed() {
152163
assertTrue(call.getRequestCustomizers().size() == 2);
153164
}
154165

155-
@Test(expectedExceptions = IllegalStateException.class, expectedExceptionsMessageRegExp = "HTTP client is not set.")
166+
@Test(expectedExceptions = NullPointerException.class,
167+
expectedExceptionsMessageRegExp = "httpClientSupplier is marked @NonNull but is null")
156168
void shouldThrowISEIfHttpClientIsNotDefined() {
157169
// given
158170
val factory = AsyncHttpClientCallFactory.builder()
@@ -168,17 +180,23 @@ void shouldThrowISEIfHttpClientIsNotDefined() {
168180
@Test
169181
void shouldUseHttpClientInstanceIfSupplierIsNotAvailable() {
170182
// given
171-
val httpClientA = mock(AsyncHttpClient.class);
183+
val httpClient = mock(AsyncHttpClient.class);
172184

173185
val factory = AsyncHttpClientCallFactory.builder()
174-
.httpClient(httpClientA)
186+
.httpClient(httpClient)
175187
.build();
176188

177189
// when
178190
val usedHttpClient = factory.getHttpClient();
179191

180192
// then
181-
assertTrue(usedHttpClient == httpClientA);
193+
assertTrue(usedHttpClient == httpClient);
194+
195+
// when
196+
val call = (AsyncHttpClientCall) factory.newCall(REQUEST);
197+
198+
// then: call should contain correct http client
199+
assertTrue(call.getHttpClient()== httpClient);
182200
}
183201

184202
@Test
@@ -197,5 +215,12 @@ void shouldPreferHttpClientSupplierOverHttpClient() {
197215

198216
// then
199217
assertTrue(usedHttpClient == httpClientB);
218+
219+
// when: try to create new call
220+
val call = (AsyncHttpClientCall) factory.newCall(REQUEST);
221+
222+
// then: call should contain correct http client
223+
assertNotNull(call);
224+
assertTrue(call.getHttpClient() == httpClientB);
200225
}
201226
}

‎extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.asynchttpclient.Response;
2525
import org.mockito.ArgumentCaptor;
2626
import org.testng.Assert;
27+
import org.testng.annotations.BeforeMethod;
2728
import org.testng.annotations.DataProvider;
2829
import org.testng.annotations.Test;
2930

@@ -35,10 +36,11 @@
3536
import java.util.concurrent.TimeoutException;
3637
import java.util.concurrent.atomic.AtomicInteger;
3738
import java.util.function.Consumer;
39+
import java.util.function.Supplier;
3840

3941
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumer;
4042
import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers;
41-
import static org.mockito.Matchers.any;
43+
import static org.mockito.ArgumentMatchers.any;
4244
import static org.mockito.Mockito.*;
4345
import static org.testng.Assert.assertEquals;
4446
import static org.testng.Assert.assertNotEquals;
@@ -47,19 +49,25 @@
4749
public class AsyncHttpClientCallTest {
4850
static final Request REQUEST = new Request.Builder().url("http://www.google.com/").build();
4951

52+
private AsyncHttpClient httpClient;
53+
private Supplier<AsyncHttpClient> httpClientSupplier = () -> httpClient;
54+
55+
@BeforeMethod
56+
void setup() {
57+
this.httpClient = mock(AsyncHttpClient.class);
58+
}
59+
5060
@Test(expectedExceptions = NullPointerException.class, dataProvider = "first")
5161
void builderShouldThrowInCaseOfMissingProperties(AsyncHttpClientCall.AsyncHttpClientCallBuilder builder) {
5262
builder.build();
5363
}
5464

5565
@DataProvider(name = "first")
5666
Object[][] dataProviderFirst() {
57-
val httpClient = mock(AsyncHttpClient.class);
58-
5967
return new Object[][]{
6068
{AsyncHttpClientCall.builder()},
6169
{AsyncHttpClientCall.builder().request(REQUEST)},
62-
{AsyncHttpClientCall.builder().httpClient(httpClient)}
70+
{AsyncHttpClientCall.builder().httpClientSupplier(httpClientSupplier)}
6371
};
6472
}
6573

@@ -77,7 +85,7 @@ void shouldInvokeConsumersOnEachExecution(Consumer<AsyncCompletionHandler<?>> ha
7785
val numRequestCustomizer = new AtomicInteger();
7886

7987
// prepare http client mock
80-
valhttpClient = mock(AsyncHttpClient.class);
88+
this.httpClient = mock(AsyncHttpClient.class);
8189

8290
val mockRequest = mock(org.asynchttpclient.Request.class);
8391
when(mockRequest.getHeaders()).thenReturn(EmptyHttpHeaders.INSTANCE);
@@ -94,7 +102,7 @@ void shouldInvokeConsumersOnEachExecution(Consumer<AsyncCompletionHandler<?>> ha
94102

95103
// create call instance
96104
val call = AsyncHttpClientCall.builder()
97-
.httpClient(httpClient)
105+
.httpClientSupplier(httpClientSupplier)
98106
.request(REQUEST)
99107
.onRequestStart(e -> numStarted.incrementAndGet())
100108
.onRequestFailure(t -> numFailed.incrementAndGet())
@@ -163,7 +171,7 @@ Object[][] dataProviderSecond() {
163171
void toIOExceptionShouldProduceExpectedResult(Throwable exception) {
164172
// given
165173
val call = AsyncHttpClientCall.builder()
166-
.httpClient(mock(AsyncHttpClient.class))
174+
.httpClientSupplier(httpClientSupplier)
167175
.request(REQUEST)
168176
.build();
169177

@@ -295,6 +303,18 @@ public void bodyIsNotNullInResponse() throws Exception {
295303
assertNotEquals(response.body(), null);
296304
}
297305

306+
@Test(expectedExceptions = IllegalStateException.class, expectedExceptionsMessageRegExp = ".*returned null.")
307+
void getHttpClientShouldThrowISEIfSupplierReturnsNull() {
308+
// given:
309+
val call = AsyncHttpClientCall.builder()
310+
.httpClientSupplier(() -> null)
311+
.request(requestWithBody())
312+
.build();
313+
314+
// when: should throw ISE
315+
call.getHttpClient();
316+
}
317+
298318
private void givenResponseIsProduced(AsyncHttpClient client, Response response) {
299319
when(client.executeRequest(any(org.asynchttpclient.Request.class), any())).thenAnswer(invocation -> {
300320
AsyncCompletionHandler<Response> handler = invocation.getArgument(1);
@@ -304,9 +324,11 @@ private void givenResponseIsProduced(AsyncHttpClient client, Response response)
304324
}
305325

306326
private okhttp3.Response whenRequestIsMade(AsyncHttpClient client, Request request) throws IOException {
307-
AsyncHttpClientCall call = AsyncHttpClientCall.builder().httpClient(client).request(request).build();
308-
309-
return call.execute();
327+
return AsyncHttpClientCall.builder()
328+
.httpClientSupplier(() -> client)
329+
.request(request)
330+
.build()
331+
.execute();
310332
}
311333

312334
private Request requestWithBody() {

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /