Retrofit http client supplier npe fix

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>

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.
    *

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);
+    }
   }
 }

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);
   }
 }

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() {