Remove temporary AbstractManagedChannelImplBuilderTest

sergiitk · sergiitk · commit b30c7ca8fd1b · 2023-09-11T09:22:10.000-07:00
diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder.java
@@ -35,6 +35,7 @@
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/3363")
 public abstract class ForwardingChannelBuilder<T extends ForwardingChannelBuilder<T>>
     extends ManagedChannelBuilder<T> {
+  // TODO(sergiitk): explain why to ForwardingChannelBuilder2 over this. Also - deprecate?
 
   /**
    * The default constructor.
diff --git a/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java b/api/src/main/java/io/grpc/ForwardingChannelBuilder2.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,58 +14,43 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
 import com.google.errorprone.annotations.DoNotCall;
-import io.grpc.BinaryLog;
-import io.grpc.ClientInterceptor;
-import io.grpc.CompressorRegistry;
-import io.grpc.DecompressorRegistry;
-import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
-import io.grpc.NameResolver;
-import io.grpc.ProxyDetector;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 
 /**
- * Temporarily duplicates {@link io.grpc.ForwardingChannelBuilder} to fix ABI backward
- * compatibility.
+ * A {@link ManagedChannelBuilder} that delegates all its builder methods to another builder by
+ * default.
  *
- * @param <T> The concrete type of this builder.
- * @see <a href="https://github.com/grpc/grpc-java/issues/7211">grpc/grpc-java#7211</a>
+ * @param <T> The type of the subclass extending this abstract class.
+ * @since TODO
  */
-public abstract class AbstractManagedChannelImplBuilder
-    <T extends AbstractManagedChannelImplBuilder<T>> extends ManagedChannelBuilder<T> {
-
-  /**
-   * Added for ABI compatibility.
-   *
-   * <p>See details in {@link #maxInboundMessageSize(int)}.
-   * TODO(sergiitk): move back to concrete classes as a private field, when this class is removed.
-   */
-  protected int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
+@ExperimentalApi("https://github.com/grpc/grpc-java/issues/TODO")
+public abstract class ForwardingChannelBuilder2<T extends ManagedChannelBuilder<T>>
+    extends ManagedChannelBuilder<T> {
+  // TODO(sergiitk): explain why to choose over ForwardingChannelBuilder
 
   /**
    * The default constructor.
    */
-  protected AbstractManagedChannelImplBuilder() {}
+  protected ForwardingChannelBuilder2() {}
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forAddress(String name, int port) {
     throw new UnsupportedOperationException("Subclass failed to hide static factory");
   }
 
   /**
-   * This method serves to force sub classes to "hide" this static factory.
+   * This method serves to force subclasses to "hide" this static factory.
    */
   @DoNotCall("Unsupported")
   public static ManagedChannelBuilder<?> forTarget(String target) {
@@ -170,31 +155,7 @@ public T idleTimeout(long value, TimeUnit unit) {
 
   @Override
   public T maxInboundMessageSize(int max) {
-    /*
-     Why this method is not delegating, as the rest of the methods?
-
-     In refactoring described in #7211, the implementation of #maxInboundMessageSize(int)
-     (and its corresponding field) was pulled down from internal AbstractManagedChannelImplBuilder
-     to concrete classes that actually enforce this setting. For the same reason, it wasn't ported
-     to ManagedChannelImplBuilder (the #delegate()).
-
-     Then AbstractManagedChannelImplBuilder was brought back to fix ABI backward compatibility,
-     and temporarily turned into a ForwardingChannelBuilder, ref PR #7564. Eventually it will
-     be deleted, after a period with "bridge" ABI solution introduced in #7834.
-
-     However, restoring AbstractManagedChannelImplBuilder unintentionally made ABI of
-     #maxInboundMessageSize(int) implemented by the concrete classes backward incompatible:
-     pre-refactoring builds expect it to be a method of AbstractManagedChannelImplBuilder,
-     and not concrete classes, ref #8313.
-
-     The end goal is to keep #maxInboundMessageSize(int) only in concrete classes that enforce it.
-     To fix method's ABI, we temporary reintroduce it to the original layer it was removed from:
-     AbstractManagedChannelImplBuilder. This class' only intention is to provide short-term
-     ABI compatibility. Once we move forward with dropping the ABI, both fixes are no longer
-     necessary, and both will perish with removing AbstractManagedChannelImplBuilder.
-    */
-    Preconditions.checkArgument(max >= 0, "negative max");
-    maxInboundMessageSize = max;
+    delegate().maxInboundMessageSize(max);
     return thisT();
   }
 
diff --git a/api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java b/api/src/test/java/io/grpc/ForwardingChannelBuilder2Test.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2020 The gRPC Authors
+ * Copyright 2023 The gRPC Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,33 +14,30 @@
  * limitations under the License.
  */
 
-package io.grpc.internal;
+package io.grpc;
 
 import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 
 import com.google.common.base.Defaults;
-import com.google.common.collect.ImmutableSet;
-import io.grpc.ForwardingTestUtil;
-import io.grpc.ManagedChannel;
-import io.grpc.ManagedChannelBuilder;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.Collections;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 /**
- * Unit tests for {@link AbstractManagedChannelImplBuilder}.
+ * Unit tests for {@link ForwardingChannelBuilder2}.
  */
 @RunWith(JUnit4.class)
-public class AbstractManagedChannelImplBuilderTest {
+public class ForwardingChannelBuilder2Test {
   private final ManagedChannelBuilder<?> mockDelegate = mock(ManagedChannelBuilder.class);
 
-  private final AbstractManagedChannelImplBuilder<?> testChannelBuilder = new TestBuilder();
+  private final ForwardingChannelBuilder2<?> testChannelBuilder = new TestBuilder();
 
-  private final class TestBuilder extends AbstractManagedChannelImplBuilder<TestBuilder> {
+  private final class TestBuilder extends ForwardingChannelBuilder2<TestBuilder> {
     @Override
     protected ManagedChannelBuilder<?> delegate() {
       return mockDelegate;
@@ -53,8 +50,7 @@ public void allMethodsForwarded() throws Exception {
         ManagedChannelBuilder.class,
         mockDelegate,
         testChannelBuilder,
-        // maxInboundMessageSize is the only method that shouldn't forward.
-        ImmutableSet.of(ManagedChannelBuilder.class.getMethod("maxInboundMessageSize", int.class)),
+        Collections.emptyList(),
         new ForwardingTestUtil.ArgumentProvider() {
           @Override
           public Object get(Method method, int argPos, Class<?> clazz) {
@@ -67,15 +63,6 @@ public Object get(Method method, int argPos, Class<?> clazz) {
         });
   }
 
-  @Test
-  public void testMaxInboundMessageSize() {
-    assertThat(testChannelBuilder.maxInboundMessageSize)
-        .isEqualTo(GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE);
-
-    testChannelBuilder.maxInboundMessageSize(42);
-    assertThat(testChannelBuilder.maxInboundMessageSize).isEqualTo(42);
-  }
-
   @Test
   public void allBuilderMethodsReturnThis() throws Exception {
     for (Method method : ManagedChannelBuilder.class.getDeclaredMethods()) {
diff --git a/core/build.gradle b/core/build.gradle
@@ -14,10 +14,6 @@ plugins {
     id "ru.vyarus.animalsniffer"
 }
 
-import static java.nio.charset.StandardCharsets.US_ASCII;
-
-import com.google.common.primitives.Bytes;
-
 description = 'gRPC: Core'
 
 dependencies {
@@ -73,50 +69,6 @@ animalsniffer {
 components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) { skip() }
 components.java.withVariantsFromConfiguration(configurations.testFixturesRuntimeElements) { skip() }
 
-import net.ltgt.gradle.errorprone.CheckSeverity
-
-def replaceBytes(byte[] haystack, byte[] needle, byte[] replacement) {
-    int i = Bytes.indexOf(haystack, needle);
-    assert i != -1;
-    byte[] result = new byte[haystack.length - needle.length + replacement.length];
-    System.arraycopy(haystack, 0, result, 0, i);
-    System.arraycopy(replacement, 0, result, i, replacement.length);
-    System.arraycopy(haystack, i + needle.length, result, i + replacement.length, haystack.length - i - needle.length);
-    return result;
-}
-
-def bigEndianShortBytes(int value) {
-    return [value >> 8, value & 0xFF] as byte[];
-}
-
-def replaceConstant(File file, String needle, String replacement) {
-    // CONSTANT_Utf8_info. https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4.7
-    byte[] needleBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(needle.length()), needle.getBytes(US_ASCII));
-    byte[] replacementBytes = Bytes.concat(
-        [1] as byte[], bigEndianShortBytes(replacement.length()), replacement.getBytes(US_ASCII));
-    file.setBytes(replaceBytes(file.getBytes(), needleBytes, replacementBytes));
-}
-
-plugins.withId("java") {
-    tasks.named("compileJava").configure {
-        doLast {
-            // Replace value of Signature Attribute.
-            // https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.9
-	    //
-	    // Have new compilations compile against a public class, without breaking the internal
-	    // ABI for the moment. After giving users some time to recompile, this should be removed
-	    // and #7211 can continue. When this is removed, at the very least the generics need to
-	    // be changed to avoid <? extends io.grpc.internal.*>.
-            project.replaceConstant(
-                destinationDirectory.file(
-                    "io/grpc/internal/AbstractManagedChannelImplBuilder.class").get().getAsFile(),
-                "<T:Lio/grpc/internal/AbstractManagedChannelImplBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;",
-                "<T:Lio/grpc/ManagedChannelBuilder<TT;>;>Lio/grpc/ManagedChannelBuilder<TT;>;");
-        }
-    }
-}
-
 tasks.register("versionFile") {
     doLast {
         new File(buildDir, "version").write("${project.version}\n")
diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java
@@ -28,9 +28,9 @@
 import io.grpc.ChannelCredentials;
 import io.grpc.ChannelLogger;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
@@ -52,8 +52,7 @@
 
 /** Convenience class for building channels with the cronet transport. */
 @ExperimentalApi("There is no plan to make this API stable, given transport API instability")
-public final class CronetChannelBuilder
-    extends AbstractManagedChannelImplBuilder<CronetChannelBuilder> {
+public final class CronetChannelBuilder extends ForwardingChannelBuilder2<CronetChannelBuilder> {
 
   private static final String LOG_TAG = "CronetChannelBuilder";
 
diff --git a/inprocess/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/inprocess/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java
@@ -23,9 +23,9 @@
 import io.grpc.ChannelCredentials;
 import io.grpc.ChannelLogger;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
 import io.grpc.internal.GrpcUtil;
@@ -47,7 +47,8 @@
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1783")
 public final class InProcessChannelBuilder extends
-    AbstractManagedChannelImplBuilder<InProcessChannelBuilder> {
+    ForwardingChannelBuilder2<InProcessChannelBuilder> {
+
   /**
    * Create a channel builder that will connect to the server with the given name.
    *
diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java
@@ -33,10 +33,10 @@
 import io.grpc.ChannelLogger;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.HttpConnectProxiedSocketAddress;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.AtomicBackoff;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
@@ -72,8 +72,7 @@
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784")
 @CheckReturnValue
-public final class NettyChannelBuilder extends
-    AbstractManagedChannelImplBuilder<NettyChannelBuilder> {
+public final class NettyChannelBuilder extends ForwardingChannelBuilder2<NettyChannelBuilder> {
 
   // 1MiB.
   public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024;
@@ -102,6 +101,7 @@ public final class NettyChannelBuilder extends
   private boolean autoFlowControl = DEFAULT_AUTO_FLOW_CONTROL;
   private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW;
   private int maxHeaderListSize = GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE;
+  private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
   private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED;
   private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS;
   private boolean keepAliveWithoutCalls;
diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java
@@ -29,11 +29,11 @@
 import io.grpc.CompositeCallCredentials;
 import io.grpc.CompositeChannelCredentials;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingChannelBuilder2;
 import io.grpc.InsecureChannelCredentials;
 import io.grpc.Internal;
 import io.grpc.ManagedChannelBuilder;
 import io.grpc.TlsChannelCredentials;
-import io.grpc.internal.AbstractManagedChannelImplBuilder;
 import io.grpc.internal.AtomicBackoff;
 import io.grpc.internal.ClientTransportFactory;
 import io.grpc.internal.ConnectionClientTransport;
@@ -83,8 +83,7 @@
 
 /** Convenience class for building channels with the OkHttp transport. */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785")
-public final class OkHttpChannelBuilder extends
-    AbstractManagedChannelImplBuilder<OkHttpChannelBuilder> {
+public final class OkHttpChannelBuilder extends ForwardingChannelBuilder2<OkHttpChannelBuilder> {
   private static final Logger log = Logger.getLogger(OkHttpChannelBuilder.class.getName());
   public static final int DEFAULT_FLOW_CONTROL_WINDOW = 65535;
 
@@ -188,6 +187,7 @@ public static OkHttpChannelBuilder forTarget(String target, ChannelCredentials c
   private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS;
   private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW;
   private boolean keepAliveWithoutCalls;
+  private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
   private int maxInboundMetadataSize = Integer.MAX_VALUE;
 
   /**
