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.

Author: bfg

Diff for: extras/retrofit2/pom.xml

@@ -13,7 +13,7 @@
 
   <properties>
     <retrofit2.version>2.5.0</retrofit2.version>
-    <lombok.version>1.16.20</lombok.version>
+    <lombok.version>1.18.6</lombok.version>
   </properties>
 
   <dependencies>

Diff for: 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} <a href="http://square.github.io/retrofit/">Retrofit2</a> {@link okhttp3.Call}
@@ -48,44 +49,52 @@ 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.
    *
    * @see #isExecuted()
    * @see #isCanceled()
    */
   private final AtomicReference<CompletableFuture<Response>> futureRef = new AtomicReference<>();
+
   /**
-   * HttpClient instance.
+   * {@link AsyncHttpClient} supplier
    */
   @NonNull
-  AsyncHttpClient httpClient;
+  Supplier<AsyncHttpClient> 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<Consumer<RequestBuilder>> requestCustomizers;
+
   /**
    * List of consumers that get called just before actual HTTP request is being fired.
    */
   @Singular("onRequestStart")
   List<Consumer<Request>> onRequestStart;
+
   /**
    * List of consumers that get called when HTTP request finishes with an exception.
    */
   @Singular("onRequestFailure")
   List<Consumer<Throwable>> 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 for: extras/retrofit2/src/main/java/org/asynchttpclient/extras/retrofit/AsyncHttpClientCallFactory.java

@@ -18,42 +18,36 @@
 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 <a href="http://square.github.io/retrofit/">Retrofit2</a>
+ * {@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<AsyncHttpClient> httpClientSupplier;
 
   /**
    * List of {@link Call} builder customizers that are invoked just before creating it.
    */
   @Singular("callCustomizer")
+  @Getter(AccessLevel.PACKAGE)
   List<Consumer<AsyncHttpClientCall.AsyncHttpClientCallBuilder>> 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<AsyncHttpClient> 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);
+    }
   }
 }

Diff for: 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 for: 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,19 +49,25 @@
 public class AsyncHttpClientCallTest {
     static final Request REQUEST = new Request.Builder().url("http://www.google.com/").build();
 
+    private AsyncHttpClient httpClient;
+    private Supplier<AsyncHttpClient> httpClientSupplier = () -> httpClient;
+
+    @BeforeMethod
+    void setup() {
+      this.httpClient = mock(AsyncHttpClient.class);
+    }
+
     @Test(expectedExceptions = NullPointerException.class, dataProvider = "first")
     void builderShouldThrowInCaseOfMissingProperties(AsyncHttpClientCall.AsyncHttpClientCallBuilder builder) {
         builder.build();
     }
 
     @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<AsyncCompletionHandler<?>> 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<AsyncCompletionHandler<?>> 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<Response> 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() {