okhttp: deprecate OkHttpChannelBuilder#negotiationType (#4533)

4 files changed, 53 insertions, 26 deletions

diff --git a/okhttp/src/main/java/io/grpc/okhttp/NegotiationType.java b/okhttp/src/main/java/io/grpc/okhttp/NegotiationType.java
index 57f1b59a3..ebd8fd93d 100644
--- a/okhttp/src/main/java/io/grpc/okhttp/NegotiationType.java
+++ b/okhttp/src/main/java/io/grpc/okhttp/NegotiationType.java
@@ -18,7 +18,12 @@ package io.grpc.okhttp;
 
 /**
  * Identifies the negotiation used for starting up HTTP/2.
+ *
+ * @deprecated use {@link OkHttpChannelBuilder#usePlaintext()} or {@link
+ *     OkHttpChannelBuilder#useTransportSecurity()} directly rather than {@link
+ *     OkHttpChannelBuilder#negotiationType(NegotiationType)}.
  */
+@Deprecated
 public enum NegotiationType {
   /**
    * Uses TLS ALPN/NPN negotiation, assumes an SSL connection.
diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
index 005504b1c..542e5bd65 100644
--- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
+++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
@@ -62,6 +62,18 @@ import javax.net.ssl.TrustManagerFactory;
 public class OkHttpChannelBuilder extends
         AbstractManagedChannelImplBuilder<OkHttpChannelBuilder> {
 
+  /** Identifies the negotiation used for starting up HTTP/2. */
+  private enum NegotiationType {
+    /** Uses TLS ALPN/NPN negotiation, assumes an SSL connection. */
+    TLS,
+
+    /**
+     * Just assume the connection is plaintext (non-SSL) and the remote endpoint supports HTTP/2
+     * directly without an upgrade.
+     */
+    PLAINTEXT
+  }
+
   /**
    * ConnectionSpec closely matching the default configuration that could be used as a basis for
    * modification.
@@ -183,9 +195,22 @@ public class OkHttpChannelBuilder extends
    * {@link #sslSocketFactory} to override the socket factory used.
    *
    * <p>Default: <code>TLS</code>
+   *
+   * @deprecated use {@link #usePlaintext()} or {@link #useTransportSecurity()} instead.
    */
-  public final OkHttpChannelBuilder negotiationType(NegotiationType type) {
-    negotiationType = Preconditions.checkNotNull(type, "type");
+  @Deprecated
+  public final OkHttpChannelBuilder negotiationType(io.grpc.okhttp.NegotiationType type) {
+    Preconditions.checkNotNull(type, "type");
+    switch (type) {
+      case TLS:
+        negotiationType = NegotiationType.TLS;
+        break;
+      case PLAINTEXT:
+        negotiationType = NegotiationType.PLAINTEXT;
+        break;
+      default:
+        throw new AssertionError("Unknown negotiation type: " + type);
+    }
     return this;
   }
 
@@ -262,16 +287,11 @@ public class OkHttpChannelBuilder extends
   }
 
   /**
-   * Override the default {@link SSLSocketFactory} and enable {@link NegotiationType#TLS}
-   * negotiation.
-   *
-   * <p>By default, when TLS is enabled, <code>SSLSocketFactory.getDefault()</code> will be used.
-   *
-   * <p>{@link NegotiationType#TLS} will be applied by calling this method.
+   * Override the default {@link SSLSocketFactory} and enable TLS negotiation.
    */
   public final OkHttpChannelBuilder sslSocketFactory(SSLSocketFactory factory) {
     this.sslSocketFactory = factory;
-    negotiationType(NegotiationType.TLS);
+    negotiationType = NegotiationType.TLS;
     return this;
   }
 
@@ -320,7 +340,7 @@ public class OkHttpChannelBuilder extends
   }
 
   /**
-   * Equivalent to using {@link #negotiationType(NegotiationType)} with {@code PLAINTEXT}.
+   * Equivalent to using {@link #negotiationType} with {@code PLAINTEXT}.
    *
    * @deprecated use {@link #usePlaintext()} instead.
    */
@@ -328,28 +348,31 @@ public class OkHttpChannelBuilder extends
   @Deprecated
   public final OkHttpChannelBuilder usePlaintext(boolean skipNegotiation) {
     if (skipNegotiation) {
-      negotiationType(NegotiationType.PLAINTEXT);
+      negotiationType(io.grpc.okhttp.NegotiationType.PLAINTEXT);
     } else {
       throw new IllegalArgumentException("Plaintext negotiation not currently supported");
     }
     return this;
   }
 
-  /**
-   * Equivalent to using {@link #negotiationType(NegotiationType)} with {@code PLAINTEXT}.
-   */
+  /** Sets the negotiation type for the HTTP/2 connection to plaintext. */
   @Override
   public final OkHttpChannelBuilder usePlaintext() {
-    negotiationType(NegotiationType.PLAINTEXT);
+    negotiationType = NegotiationType.PLAINTEXT;
     return this;
   }
 
   /**
-   * Equivalent to using {@link #negotiationType(NegotiationType)} with {@code TLS}.
+   * Sets the negotiation type for the HTTP/2 connection to TLS (this is the default).
+   *
+   * <p>With TLS enabled, a default {@link SSLSocketFactory} is created using the best {@link
+   * java.security.Provider} available and is NOT based on {@link SSLSocketFactory#getDefault}. To
+   * more precisely control the TLS configuration call {@link #sslSocketFactory} to override the
+   * socket factory used.
    */
   @Override
   public final OkHttpChannelBuilder useTransportSecurity() {
-    negotiationType(NegotiationType.TLS);
+    negotiationType = NegotiationType.TLS;
     return this;
   }
 
diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java
index 7b02b9f0c..98d171162 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java
@@ -77,9 +77,7 @@ public class OkHttpChannelBuilderTest {
       }
     };
 
-    builder.overrideAuthority("[invalidauthority")
-        .negotiationType(NegotiationType.PLAINTEXT)
-        .buildTransportFactory();
+    builder.overrideAuthority("[invalidauthority").usePlaintext().buildTransportFactory();
   }
 
   @Test
diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java
index 5b5fe91e9..2c3360718 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java
@@ -37,12 +37,13 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class OkHttpTransportTest extends AbstractTransportTest {
   private final FakeClock fakeClock = new FakeClock();
-  private ClientTransportFactory clientFactory = OkHttpChannelBuilder
-      // Although specified here, address is ignored because we never call build.
-      .forAddress("localhost", 0)
-      .negotiationType(NegotiationType.PLAINTEXT)
-      .setTransportTracerFactory(fakeClockTransportTracer)
-      .buildTransportFactory();
+  private ClientTransportFactory clientFactory =
+      OkHttpChannelBuilder
+          // Although specified here, address is ignored because we never call build.
+          .forAddress("localhost", 0)
+          .usePlaintext()
+          .setTransportTracerFactory(fakeClockTransportTracer)
+          .buildTransportFactory();
 
   @After
   public void releaseClientFactory() {