From b0cf720efb82aa0fbba97448aefe6de61db45669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Brane=20F=2E=20Gra=C4=8Dnar?= Date: Wed, 27 Feb 2019 15:48:55 +0100 Subject: [PATCH 1/2] Lombok update to 1.18.6 --- extras/retrofit2/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extras/retrofit2/pom.xml b/extras/retrofit2/pom.xml index 2271afe0ea..317e96b28a 100644 --- a/extras/retrofit2/pom.xml +++ b/extras/retrofit2/pom.xml @@ -13,7 +13,7 @@ 2.5.0 - 1.16.20 + 1.18.6 From 51a5cdf659d24015919aeb7e2609b01940c94875 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Brane=20F=2E=20Gra=C4=8Dnar?= Date: Thu, 28 Feb 2019 00:13:48 +0100 Subject: [PATCH 2/2] 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. --- .../extras/retrofit/AsyncHttpClientCall.java | 27 +++++++++- .../retrofit/AsyncHttpClientCallFactory.java | 52 ++++++++++++------- .../AsyncHttpClientCallFactoryTest.java | 35 +++++++++++-- .../retrofit/AsyncHttpClientCallTest.java | 42 +++++++++++---- 4 files changed, 119 insertions(+), 37 deletions(-) diff --git a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java index 9197351233..39107ce02a 100644 --- a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java +++ b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCall.java @@ -30,6 +30,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; +import java.util.function.Supplier; /** * {@link AsyncHttpClient} Retrofit2 {@link okhttp3.Call} @@ -48,6 +49,7 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call { public static final long DEFAULT_EXECUTE_TIMEOUT_MILLIS = 30_000; private static final ResponseBody EMPTY_BODY = ResponseBody.create(null, ""); + /** * Tells whether call has been executed. * @@ -55,37 +57,44 @@ class AsyncHttpClientCall implements Cloneable, okhttp3.Call { * @see #isCanceled() */ private final AtomicReference> futureRef = new AtomicReference<>(); + /** - * HttpClient instance. + * {@link AsyncHttpClient} supplier */ @NonNull - AsyncHttpClient httpClient; + Supplier httpClientSupplier; + /** * {@link #execute()} response timeout in milliseconds. */ @Builder.Default long executeTimeoutMillis = DEFAULT_EXECUTE_TIMEOUT_MILLIS; + /** * Retrofit request. */ @NonNull @Getter(AccessLevel.NONE) Request request; + /** * List of consumers that get called just before actual async-http-client request is being built. */ @Singular("requestCustomizer") List> requestCustomizers; + /** * List of consumers that get called just before actual HTTP request is being fired. */ @Singular("onRequestStart") List> onRequestStart; + /** * List of consumers that get called when HTTP request finishes with an exception. */ @Singular("onRequestFailure") List> onRequestFailure; + /** * List of consumers that get called when HTTP request finishes successfully. */ @@ -236,6 +245,20 @@ public Response onCompleted(org.asynchttpclient.Response response) { return future; } + /** + * Returns HTTP client. + * + * @return http client + * @throws IllegalArgumentException if {@link #httpClientSupplier} returned {@code null}. + */ + protected AsyncHttpClient getHttpClient() { + val httpClient = httpClientSupplier.get(); + if (httpClient == null) { + throw new IllegalStateException("Async HTTP client instance supplier " + httpClientSupplier + " returned null."); + } + return httpClient; + } + /** * Converts async-http-client response to okhttp response. * diff --git a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java index 85139718d5..0077cd32e3 100644 --- a/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java +++ b/extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java @@ -18,29 +18,22 @@ import org.asynchttpclient.AsyncHttpClient; import java.util.List; -import java.util.Optional; import java.util.function.Consumer; import java.util.function.Supplier; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers; /** - * {@link AsyncHttpClient} implementation of Retrofit2 {@link Call.Factory} + * {@link AsyncHttpClient} implementation of Retrofit2 + * {@link Call.Factory}. */ @Value @Builder(toBuilder = true) public class AsyncHttpClientCallFactory implements Call.Factory { /** - * {@link AsyncHttpClient} in use. - * - * @see #httpClientSupplier - */ - @Getter(AccessLevel.NONE) - AsyncHttpClient httpClient; - - /** - * Supplier of {@link AsyncHttpClient}, takes precedence over {@link #httpClient}. + * Supplier of {@link AsyncHttpClient}. */ + @NonNull @Getter(AccessLevel.NONE) Supplier httpClientSupplier; @@ -48,12 +41,13 @@ public class AsyncHttpClientCallFactory implements Call.Factory { * List of {@link Call} builder customizers that are invoked just before creating it. */ @Singular("callCustomizer") + @Getter(AccessLevel.PACKAGE) List> callCustomizers; @Override public Call newCall(Request request) { val callBuilder = AsyncHttpClientCall.builder() - .httpClient(httpClient) + .httpClientSupplier(httpClientSupplier) .request(request); // customize builder before creating a call @@ -64,15 +58,33 @@ public Call newCall(Request request) { } /** - * {@link AsyncHttpClient} in use by this factory. + * Returns {@link AsyncHttpClient} from {@link #httpClientSupplier}. * - * @return + * @return http client. */ - public AsyncHttpClient getHttpClient() { - return Optional.ofNullable(httpClientSupplier) - .map(Supplier::get) - .map(Optional::of) - .orElseGet(() -> Optional.ofNullable(httpClient)) - .orElseThrow(() -> new IllegalStateException("HTTP client is not set.")); + AsyncHttpClient getHttpClient() { + return httpClientSupplier.get(); + } + + /** + * Builder for {@link AsyncHttpClientCallFactory}. + */ + public static class AsyncHttpClientCallFactoryBuilder { + /** + * {@link AsyncHttpClient} supplier that returns http client to be used to execute HTTP requests. + */ + private Supplier httpClientSupplier; + + /** + * Sets concrete http client to be used by the factory to execute HTTP requests. Invocation of this method + * overrides any previous http client supplier set by {@link #httpClientSupplier(Supplier)}! + * + * @param httpClient http client + * @return reference to itself. + * @see #httpClientSupplier(Supplier) + */ + public AsyncHttpClientCallFactoryBuilder httpClient(@NonNull AsyncHttpClient httpClient) { + return httpClientSupplier(() -> httpClient); + } } } \ No newline at end of file diff --git a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java index cec3ece12b..864931a583 100644 --- a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java +++ b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactoryTest.java @@ -14,23 +14,34 @@ import lombok.extern.slf4j.Slf4j; import lombok.val; +import okhttp3.MediaType; import okhttp3.Request; +import okhttp3.RequestBody; import okhttp3.Response; import org.asynchttpclient.AsyncHttpClient; import org.asynchttpclient.RequestBuilder; import org.testng.annotations.Test; +import java.util.Objects; import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; -import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCallTest.REQUEST; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCallTest.createConsumer; import static org.mockito.Mockito.mock; import static org.testng.Assert.*; @Slf4j public class AsyncHttpClientCallFactoryTest { + private static final MediaType MEDIA_TYPE = MediaType.parse("application/json"); + private static final String JSON_BODY = "{\"foo\": \"bar\"}"; + private static final RequestBody BODY = RequestBody.create(MEDIA_TYPE, JSON_BODY); + private static final String URL = "http://localhost:11000/foo/bar?a=b&c=d"; + private static final Request REQUEST = new Request.Builder() + .post(BODY) + .addHeader("X-Foo", "Bar") + .url(URL) + .build(); @Test void newCallShouldProduceExpectedResult() { // given @@ -152,7 +163,8 @@ void shouldApplyAllConsumersToCallBeingConstructed() { assertTrue(call.getRequestCustomizers().size() == 2); } - @Test(expectedExceptions = IllegalStateException.class, expectedExceptionsMessageRegExp = "HTTP client is not set.") + @Test(expectedExceptions = NullPointerException.class, + expectedExceptionsMessageRegExp = "httpClientSupplier is marked @NonNull but is null") void shouldThrowISEIfHttpClientIsNotDefined() { // given val factory = AsyncHttpClientCallFactory.builder() @@ -168,17 +180,23 @@ void shouldThrowISEIfHttpClientIsNotDefined() { @Test void shouldUseHttpClientInstanceIfSupplierIsNotAvailable() { // given - val httpClientA = mock(AsyncHttpClient.class); + val httpClient = mock(AsyncHttpClient.class); val factory = AsyncHttpClientCallFactory.builder() - .httpClient(httpClientA) + .httpClient(httpClient) .build(); // when val usedHttpClient = factory.getHttpClient(); // then - assertTrue(usedHttpClient == httpClientA); + assertTrue(usedHttpClient == httpClient); + + // when + val call = (AsyncHttpClientCall) factory.newCall(REQUEST); + + // then: call should contain correct http client + assertTrue(call.getHttpClient()== httpClient); } @Test @@ -197,5 +215,12 @@ void shouldPreferHttpClientSupplierOverHttpClient() { // then assertTrue(usedHttpClient == httpClientB); + + // when: try to create new call + val call = (AsyncHttpClientCall) factory.newCall(REQUEST); + + // then: call should contain correct http client + assertNotNull(call); + assertTrue(call.getHttpClient() == httpClientB); } } diff --git a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java index 84ed4ddf47..ab908730af 100644 --- a/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java +++ b/extras/retrofit2/src/test/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallTest.java @@ -24,6 +24,7 @@ import org.asynchttpclient.Response; import org.mockito.ArgumentCaptor; import org.testng.Assert; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -35,10 +36,11 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Consumer; +import java.util.function.Supplier; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumer; import static org.asynchttpclient.extras.retrofit.AsyncHttpClientCall.runConsumers; -import static org.mockito.Matchers.any; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotEquals; @@ -47,6 +49,14 @@ public class AsyncHttpClientCallTest { static final Request REQUEST = new Request.Builder().url("http://www.google.com/").build(); + private AsyncHttpClient httpClient; + private Supplier httpClientSupplier = () -> httpClient; + + @BeforeMethod + void setup() { + this.httpClient = mock(AsyncHttpClient.class); + } + @Test(expectedExceptions = NullPointerException.class, dataProvider = "first") void builderShouldThrowInCaseOfMissingProperties(AsyncHttpClientCall.AsyncHttpClientCallBuilder builder) { builder.build(); @@ -54,12 +64,10 @@ void builderShouldThrowInCaseOfMissingProperties(AsyncHttpClientCall.AsyncHttpCl @DataProvider(name = "first") Object[][] dataProviderFirst() { - val httpClient = mock(AsyncHttpClient.class); - return new Object[][]{ {AsyncHttpClientCall.builder()}, {AsyncHttpClientCall.builder().request(REQUEST)}, - {AsyncHttpClientCall.builder().httpClient(httpClient)} + {AsyncHttpClientCall.builder().httpClientSupplier(httpClientSupplier)} }; } @@ -77,7 +85,7 @@ void shouldInvokeConsumersOnEachExecution(Consumer> ha val numRequestCustomizer = new AtomicInteger(); // prepare http client mock - val httpClient = mock(AsyncHttpClient.class); + this.httpClient = mock(AsyncHttpClient.class); val mockRequest = mock(org.asynchttpclient.Request.class); when(mockRequest.getHeaders()).thenReturn(EmptyHttpHeaders.INSTANCE); @@ -94,7 +102,7 @@ void shouldInvokeConsumersOnEachExecution(Consumer> ha // create call instance val call = AsyncHttpClientCall.builder() - .httpClient(httpClient) + .httpClientSupplier(httpClientSupplier) .request(REQUEST) .onRequestStart(e -> numStarted.incrementAndGet()) .onRequestFailure(t -> numFailed.incrementAndGet()) @@ -163,7 +171,7 @@ Object[][] dataProviderSecond() { void toIOExceptionShouldProduceExpectedResult(Throwable exception) { // given val call = AsyncHttpClientCall.builder() - .httpClient(mock(AsyncHttpClient.class)) + .httpClientSupplier(httpClientSupplier) .request(REQUEST) .build(); @@ -295,6 +303,18 @@ public void bodyIsNotNullInResponse() throws Exception { assertNotEquals(response.body(), null); } + @Test(expectedExceptions = IllegalStateException.class, expectedExceptionsMessageRegExp = ".*returned null.") + void getHttpClientShouldThrowISEIfSupplierReturnsNull() { + // given: + val call = AsyncHttpClientCall.builder() + .httpClientSupplier(() -> null) + .request(requestWithBody()) + .build(); + + // when: should throw ISE + call.getHttpClient(); + } + private void givenResponseIsProduced(AsyncHttpClient client, Response response) { when(client.executeRequest(any(org.asynchttpclient.Request.class), any())).thenAnswer(invocation -> { AsyncCompletionHandler handler = invocation.getArgument(1); @@ -304,9 +324,11 @@ private void givenResponseIsProduced(AsyncHttpClient client, Response response) } private okhttp3.Response whenRequestIsMade(AsyncHttpClient client, Request request) throws IOException { - AsyncHttpClientCall call = AsyncHttpClientCall.builder().httpClient(client).request(request).build(); - - return call.execute(); + return AsyncHttpClientCall.builder() + .httpClientSupplier(() -> client) + .request(request) + .build() + .execute(); } private Request requestWithBody() {