Repository: square/retrofit
Issue: Expose public API for creating a Call.Factory from OkHttpClient instance?
Labels: Feature

This would simplify delegation by allowing implementors of `Call.Factory` instances to create a delegate from an `OkHttpClient` (and thus get `OkHttpCall` for free). Without this there's no way to create your own `C.F` that re-uses internal machinery around OkHttp.


Issue comments:
No longer needed as of #1406 


Pull request review comments:

Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit2/Call.java b/retrofit/src/main/java/retrofit2/Call.java
index 2280a6e159..873bb0c6fe 100644
--- a/retrofit/src/main/java/retrofit2/Call.java
+++ b/retrofit/src/main/java/retrofit2/Call.java
@@ -16,7 +16,6 @@
 package retrofit2;
 
 import java.io.IOException;
-import okhttp3.ResponseBody;
 
 /**
  * An invocation of a Retrofit method that sends a request to a webserver and returns a response.
@@ -67,13 +66,4 @@ public interface Call<T> extends Cloneable {
    * has already been.
    */
   Call<T> clone();
-
-  /** Creates {@link Call} instances. */
-  interface Factory {
-    /**
-     * Returns a {@link Call} which will send {@code request} when executed or enqueue and use
-     * {@code converter} to parse the response. May not return null.
-     */
-    <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> converter);
-  }
 }
diff --git a/retrofit/src/main/java/retrofit2/DeferredRequest.java b/retrofit/src/main/java/retrofit2/DeferredRequest.java
deleted file mode 100644
index 2c9a8cfa18..0000000000
--- a/retrofit/src/main/java/retrofit2/DeferredRequest.java
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * Copyright (C) 2015 Square, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package retrofit2;
-
-import java.io.IOException;
-import okhttp3.Request;
-
-/**
- * An un-built HTTP request. This class defers any work necessary to create an HTTP request until
- * {@link #get()} is called.
- */
-public interface DeferredRequest {
-  /** Perform the work necessary to create and then return the {@link Request}. */
-  Request get() throws IOException;
-}
diff --git a/retrofit/src/main/java/retrofit2/MethodHandler.java b/retrofit/src/main/java/retrofit2/MethodHandler.java
index 35fcd0c259..a64afdc2c5 100644
--- a/retrofit/src/main/java/retrofit2/MethodHandler.java
+++ b/retrofit/src/main/java/retrofit2/MethodHandler.java
@@ -58,12 +58,12 @@ private static CallAdapter<?> createCallAdapter(Method method, Retrofit retrofit
     }
   }
 
-  private final Call.Factory callFactory;
+  private final okhttp3.Call.Factory callFactory;
   private final RequestFactory requestFactory;
   private final CallAdapter<?> callAdapter;
   private final Converter<ResponseBody, ?> responseConverter;
 
-  private MethodHandler(Call.Factory callFactory, RequestFactory requestFactory,
+  private MethodHandler(okhttp3.Call.Factory callFactory, RequestFactory requestFactory,
       CallAdapter<?> callAdapter, Converter<ResponseBody, ?> responseConverter) {
     this.callFactory = callFactory;
     this.requestFactory = requestFactory;
@@ -72,11 +72,7 @@ private MethodHandler(Call.Factory callFactory, RequestFactory requestFactory,
   }
 
   Object invoke(Object... args) {
-    DeferredRequest request = requestFactory.defer(args);
-    Call<?> call = callFactory.create(request, responseConverter);
-    if (call == null) {
-      throw new NullPointerException("Call.Factory returned null.");
-    }
-    return callAdapter.adapt(call);
+    return callAdapter.adapt(
+        new OkHttpCall<>(callFactory, requestFactory, args, responseConverter));
   }
 }
diff --git a/retrofit/src/main/java/retrofit2/OkHttpCall.java b/retrofit/src/main/java/retrofit2/OkHttpCall.java
index ea064fcd6d..6bfa8da9d8 100644
--- a/retrofit/src/main/java/retrofit2/OkHttpCall.java
+++ b/retrofit/src/main/java/retrofit2/OkHttpCall.java
@@ -17,7 +17,6 @@
 
 import java.io.IOException;
 import okhttp3.MediaType;
-import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.ResponseBody;
 import okio.Buffer;
@@ -28,24 +27,26 @@
 import static retrofit2.Utils.closeQuietly;
 
 final class OkHttpCall<T> implements Call<T> {
-  private final OkHttpClient client;
-  private final DeferredRequest request;
+  private final okhttp3.Call.Factory callFactory;
+  private final RequestFactory requestFactory;
+  private final Object[] args;
   private final Converter<ResponseBody, T> responseConverter;
 
   private volatile okhttp3.Call rawCall;
   private boolean executed; // Guarded by this.
   private volatile boolean canceled;
 
-  OkHttpCall(OkHttpClient client, DeferredRequest request,
+  OkHttpCall(okhttp3.Call.Factory callFactory, RequestFactory requestFactory, Object[] args,
       Converter<ResponseBody, T> responseConverter) {
-    this.client = client;
-    this.request = request;
+    this.callFactory = callFactory;
+    this.requestFactory = requestFactory;
+    this.args = args;
     this.responseConverter = responseConverter;
   }
 
   @SuppressWarnings("CloneDoesntCallSuperClone") // We are a final type & this saves clearing state.
   @Override public OkHttpCall<T> clone() {
-    return new OkHttpCall<>(client, request, responseConverter);
+    return new OkHttpCall<>(callFactory, requestFactory, args, responseConverter);
   }
 
   @Override public void enqueue(final Callback<T> callback) {
@@ -120,7 +121,11 @@ private void callSuccess(Response<T> response) {
   }
 
   private okhttp3.Call createRawCall() throws IOException {
-    return client.newCall(request.get());
+    okhttp3.Call call = callFactory.newCall(requestFactory.create(args));
+    if (call == null) {
+      throw new NullPointerException("Call.Factory returned null.");
+    }
+    return call;
   }
 
   Response<T> parseResponse(okhttp3.Response rawResponse) throws IOException {
diff --git a/retrofit/src/main/java/retrofit2/OkHttpCallFactory.java b/retrofit/src/main/java/retrofit2/OkHttpCallFactory.java
deleted file mode 100644
index c8810d6f62..0000000000
--- a/retrofit/src/main/java/retrofit2/OkHttpCallFactory.java
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * Copyright (C) 2015 Square, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package retrofit2;
-
-import okhttp3.OkHttpClient;
-import okhttp3.ResponseBody;
-
-final class OkHttpCallFactory implements Call.Factory {
-  final OkHttpClient client;
-
-  OkHttpCallFactory(OkHttpClient client) {
-    this.client = client;
-  }
-
-  @Override
-  public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> converter) {
-    return new OkHttpCall<>(client, request, converter);
-  }
-}
diff --git a/retrofit/src/main/java/retrofit2/RequestFactory.java b/retrofit/src/main/java/retrofit2/RequestFactory.java
index b8bf330290..7df67b6397 100644
--- a/retrofit/src/main/java/retrofit2/RequestFactory.java
+++ b/retrofit/src/main/java/retrofit2/RequestFactory.java
@@ -45,14 +45,6 @@ final class RequestFactory {
     this.requestActions = requestActions;
   }
 
-  DeferredRequest defer(final Object... args) {
-    return new DeferredRequest() {
-      @Override public Request get() throws IOException {
-        return create(args);
-      }
-    };
-  }
-
   Request create(Object... args) throws IOException {
     RequestBuilder requestBuilder =
         new RequestBuilder(method, baseUrl.url(), relativeUrl, headers, contentType, hasBody,
diff --git a/retrofit/src/main/java/retrofit2/Retrofit.java b/retrofit/src/main/java/retrofit2/Retrofit.java
index 3b798985b4..2ce5ef8e53 100644
--- a/retrofit/src/main/java/retrofit2/Retrofit.java
+++ b/retrofit/src/main/java/retrofit2/Retrofit.java
@@ -59,16 +59,16 @@
 public final class Retrofit {
   private final Map<Method, MethodHandler> methodHandlerCache = new LinkedHashMap<>();
 
-  private final Call.Factory callFactory;
+  private final okhttp3.Call.Factory callFactory;
   private final BaseUrl baseUrl;
   private final List<Converter.Factory> converterFactories;
   private final List<CallAdapter.Factory> adapterFactories;
   private final Executor callbackExecutor;
   private final boolean validateEagerly;
 
-  Retrofit(Call.Factory callFactory, BaseUrl baseUrl, List<Converter.Factory> converterFactories,
-      List<CallAdapter.Factory> adapterFactories, Executor callbackExecutor,
-      boolean validateEagerly) {
+  Retrofit(okhttp3.Call.Factory callFactory, BaseUrl baseUrl,
+      List<Converter.Factory> converterFactories, List<CallAdapter.Factory> adapterFactories,
+      Executor callbackExecutor, boolean validateEagerly) {
     this.callFactory = callFactory;
     this.baseUrl = baseUrl;
     this.converterFactories = converterFactories;
@@ -168,8 +168,11 @@ MethodHandler loadMethodHandler(Method method) {
     return handler;
   }
 
-  /** The factory used to create {@link Call} instances. */
-  public Call.Factory callFactory() {
+  /**
+   * The factory used to create {@linkplain okhttp3.Call OkHttp calls} for sending a HTTP requests.
+   * Typically an instance of {@link OkHttpClient}.
+   */
+  public okhttp3.Call.Factory callFactory() {
     return callFactory;
   }
 
@@ -358,7 +361,7 @@ public Executor callbackExecutor() {
    * are optional.
    */
   public static final class Builder {
-    private Call.Factory callFactory;
+    private okhttp3.Call.Factory callFactory;
     private BaseUrl baseUrl;
     private List<Converter.Factory> converterFactories = new ArrayList<>();
     private List<CallAdapter.Factory> adapterFactories = new ArrayList<>();
@@ -374,15 +377,14 @@ public Builder() {
     /**
      * The HTTP client used for requests.
      * <p>
-     * This is a convenience method for calling {@link #callFactory} with an instance that uses
-     * {@code client} for execution.
+     * This is a convenience method for calling {@link #callFactory}.
      * <p>
      * Note: This method <b>does not</b> make a defensive copy of {@code client}. Changes to its
      * settings will affect subsequent requests. Pass in a {@linkplain OkHttpClient#clone() cloned}
      * instance to prevent this if desired.
      */
     public Builder client(OkHttpClient client) {
-      return callFactory(new OkHttpCallFactory(checkNotNull(client, "client == null")));
+      return callFactory(checkNotNull(client, "client == null"));
     }
 
     /**
@@ -390,7 +392,7 @@ public Builder client(OkHttpClient client) {
      * <p>
      * Note: Calling {@link #client} automatically sets this value.
      */
-    public Builder callFactory(Call.Factory factory) {
+    public Builder callFactory(okhttp3.Call.Factory factory) {
       this.callFactory = checkNotNull(factory, "factory == null");
       return this;
     }
@@ -529,9 +531,9 @@ public Retrofit build() {
         throw new IllegalStateException("Base URL required.");
       }
 
-      Call.Factory callFactory = this.callFactory;
+      okhttp3.Call.Factory callFactory = this.callFactory;
       if (callFactory == null) {
-        callFactory = new OkHttpCallFactory(new OkHttpClient());
+        callFactory = new OkHttpClient();
       }
 
       // Make a defensive copy of the adapters and add the default Call adapter.


Test patch:
diff --git a/retrofit/src/test/java/retrofit2/RetrofitTest.java b/retrofit/src/test/java/retrofit2/RetrofitTest.java
index 7110550013..d2abb06854 100644
--- a/retrofit/src/test/java/retrofit2/RetrofitTest.java
+++ b/retrofit/src/test/java/retrofit2/RetrofitTest.java
@@ -18,6 +18,7 @@
 import okhttp3.HttpUrl;
 import okhttp3.MediaType;
 import okhttp3.OkHttpClient;
+import okhttp3.Request;
 import okhttp3.RequestBody;
 import okhttp3.ResponseBody;
 import okhttp3.mockwebserver.MockResponse;
@@ -656,7 +657,7 @@ class MyConverterFactory extends Converter.Factory {
   }
 
   @Test public void callFactoryPropagated() {
-    Call.Factory callFactory = mock(Call.Factory.class);
+    okhttp3.Call.Factory callFactory = mock(okhttp3.Call.Factory.class);
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl("http://example.com/")
         .callFactory(callFactory)
@@ -670,35 +671,31 @@ class MyConverterFactory extends Converter.Factory {
         .baseUrl("http://example.com/")
         .client(client)
         .build();
-    OkHttpCallFactory factory = (OkHttpCallFactory) retrofit.callFactory();
-    assertThat(factory.client).isSameAs(client);
+    assertThat(retrofit.callFactory()).isSameAs(client);
   }
 
-  @Test public void callFactoryUsed() {
-    final Retrofit blackbox = new Retrofit.Builder()
-        .baseUrl("http://example.com/")
-        .build();
-    Call.Factory callFactory = spy(new Call.Factory() {
-      @Override
-      public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> converter) {
-        // Wrap the default Call.Factory without directly relying on its implementation.
-        return blackbox.callFactory().create(request, converter);
+  @Test public void callFactoryUsed() throws IOException {
+    okhttp3.Call.Factory callFactory = spy(new okhttp3.Call.Factory() {
+      @Override public okhttp3.Call newCall(Request request) {
+        return new OkHttpClient().newCall(request);
       }
     });
     Retrofit retrofit = new Retrofit.Builder()
-        .baseUrl("http://example.com/")
+        .baseUrl(server.url("/"))
         .callFactory(callFactory)
         .build();
+
+    server.enqueue(new MockResponse());
+
     CallMethod service = retrofit.create(CallMethod.class);
-    service.getResponseBody();
-    verify(callFactory).create(any(DeferredRequest.class), any(Converter.class));
+    service.getResponseBody().execute();
+    verify(callFactory).newCall(any(Request.class));
     verifyNoMoreInteractions(callFactory);
   }
 
-  @Test public void callFactoryReturningNullThrows() {
-    Call.Factory callFactory = new Call.Factory() {
-      @Override
-      public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> converter) {
+  @Test public void callFactoryReturningNullThrows() throws IOException {
+    okhttp3.Call.Factory callFactory = new okhttp3.Call.Factory() {
+      @Override public okhttp3.Call newCall(Request request) {
         return null;
       }
     };
@@ -706,9 +703,13 @@ public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> co
         .baseUrl("http://example.com/")
         .callFactory(callFactory)
         .build();
+
+    server.enqueue(new MockResponse());
+
     CallMethod service = retrofit.create(CallMethod.class);
+    Call<ResponseBody> call = service.getResponseBody();
     try {
-      service.getResponseBody();
+      call.execute();
       fail();
     } catch (NullPointerException e) {
       assertThat(e).hasMessage("Call.Factory returned null.");
@@ -717,9 +718,8 @@ public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> co
 
   @Test public void callFactoryThrowingPropagates() {
     final RuntimeException cause = new RuntimeException("Broken!");
-    Call.Factory callFactory = new Call.Factory() {
-      @Override
-      public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> converter) {
+    okhttp3.Call.Factory callFactory = new okhttp3.Call.Factory() {
+      @Override public okhttp3.Call newCall(Request request) {
         throw cause;
       }
     };
@@ -727,9 +727,13 @@ public <T> Call<T> create(DeferredRequest request, Converter<ResponseBody, T> co
         .baseUrl("http://example.com/")
         .callFactory(callFactory)
         .build();
+
+    server.enqueue(new MockResponse());
+
     CallMethod service = retrofit.create(CallMethod.class);
+    Call<ResponseBody> call = service.getResponseBody();
     try {
-      service.getResponseBody();
+      call.execute();
       fail();
     } catch (Exception e) {
       assertThat(e).isSameAs(cause);