core: introduce NameResolver.Helper and deprecate the params on newNameResolver()

core/src/main/java/io/grpc/NameResolver.java

@@ -94,16 +94,24 @@ public abstract static class Factory {
      * The port number used in case the target or the underlying naming system doesn't provide a
      * port number.
      *
+     * @deprecated this will be deleted along with {@link #newNameResolver(URI, Attributes)} in
+     *             a future release.
+     *
      * @since 1.0.0
      */
+    @Deprecated
     public static final Attributes.Key<Integer> PARAMS_DEFAULT_PORT =
         Attributes.Key.create("params-default-port");
 
     /**
      * If the NameResolver wants to support proxy, it should inquire this {@link ProxyDetector}.
      * See documentation on {@link ProxyDetector} about how proxies work in gRPC.
+     *
+     * @deprecated this will be deleted along with {@link #newNameResolver(URI, Attributes)} in
+     *             a future release
      */
     @ExperimentalApi("https://github.com/grpc/grpc-java/issues/5113")
+    @Deprecated
     public static final Attributes.Key<ProxyDetector> PARAMS_PROXY_DETECTOR =
         Attributes.Key.create("params-proxy-detector");
 
@@ -114,11 +122,39 @@ public abstract static class Factory {
      *
      * @param targetUri the target URI to be resolved, whose scheme must not be {@code null}
      * @param params optional parameters. Canonical keys are defined as {@code PARAMS_*} fields in
-     * {@link Factory}.
+     *               {@link Factory}.
+     *
+     * @deprecated Implement {@link #newNameResolver(URI, NameResolver.Helper)} instead.  This is
+     *             going to be deleted in a future release.
+     *
      * @since 1.0.0
      */
     @Nullable
-    public abstract NameResolver newNameResolver(URI targetUri, Attributes params);
+    @Deprecated
+    public NameResolver newNameResolver(URI targetUri, Attributes params) {
+      throw new UnsupportedOperationException("This method is going to be deleted");
+    }
+
+    /**
+     * Creates a {@link NameResolver} for the given target URI, or {@code null} if the given URI
+     * cannot be resolved by this factory. The decision should be solely based on the scheme of the
+     * URI.
+     *
+     * @param targetUri the target URI to be resolved, whose scheme must not be {@code null}
+     * @param helper utility that may be used by the NameResolver implementation
+     *
+     * @since 1.19.0
+     */
+    // TODO(zhangkun83): make this abstract when the other override is deleted
+    @Nullable
+    public NameResolver newNameResolver(URI targetUri, Helper helper) {
+      return newNameResolver(
+          targetUri,
+          Attributes.newBuilder()
+              .set(PARAMS_DEFAULT_PORT, helper.getDefaultPort())
+              .set(PARAMS_PROXY_DETECTOR, helper.getProxyDetector())
+              .build());
+    }
 
     /**
      * Returns the default scheme, which will be used to construct a URI when {@link
@@ -170,4 +206,21 @@ void onAddresses(
   @Retention(RetentionPolicy.SOURCE)
   @Documented
   public @interface ResolutionResultAttr {}
+
+  /**
+   * A utility object passed to {@link Factory#newNameResolver(URI, NameResolver.Helper)}.
+   */
+  public abstract static class Helper {
+    /**
+     * The port number used in case the target or the underlying naming system doesn't provide a
+     * port number.
+     */
+    public abstract int getDefaultPort();
+
+    /**
+     * If the NameResolver wants to support proxy, it should inquire this {@link ProxyDetector}.
+     * See documentation on {@link ProxyDetector} about how proxies work in gRPC.
+     */
+    public abstract ProxyDetector getProxyDetector();
+  }
 }

core/src/main/java/io/grpc/NameResolverProvider.java

@@ -44,6 +44,7 @@ public abstract class NameResolverProvider extends NameResolver.Factory {
    * @since 1.0.0
    */
   @SuppressWarnings("unused") // Avoids outside callers accidentally depending on the super class.
+  @Deprecated
   public static final Attributes.Key<Integer> PARAMS_DEFAULT_PORT =
       NameResolver.Factory.PARAMS_DEFAULT_PORT;
 
@@ -106,10 +107,10 @@ private static final class NameResolverFactory extends NameResolver.Factory {
 
     @Override
     @Nullable
-    public NameResolver newNameResolver(URI targetUri, Attributes params) {
+    public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) {
       checkForProviders();
       for (NameResolverProvider provider : providers) {
-        NameResolver resolver = provider.newNameResolver(targetUri, params);
+        NameResolver resolver = provider.newNameResolver(targetUri, helper);
         if (resolver != null) {
           return resolver;
         }

core/src/main/java/io/grpc/ProxyDetector.java

@@ -31,11 +31,10 @@
  * <p>In order for gRPC to use a proxy, {@link NameResolver}, {@link ProxyDetector} and the
  * underlying transport need to work together.
  *
- * <p>The {@link NameResolver} should invoke the {@link ProxyDetector} retrieved from the {@code
- * params} of {@link NameResolver.Factory#newNameResolver} using {@link
- * io.grpc.NameResolver.Factory#PARAMS_PROXY_DETECTOR PARAMS_PROXY_DETECTOR}, and pass the returned
- * {@link ProxiedSocketAddress} to {@link NameResolver.Listener#onAddresses}.  The DNS name resolver
- * shipped with gRPC is already doing so.
+ * <p>The {@link NameResolver} should invoke the {@link ProxyDetector} retrieved from the {@link
+ * NameResolver.Helper#getProxyDetector}, and pass the returned {@link ProxiedSocketAddress} to
+ * {@link NameResolver.Listener#onAddresses}.  The DNS name resolver shipped with gRPC is already
+ * doing so.
  *
  * <p>The default {@code ProxyDetector} uses Java's standard {@link java.net.ProxySelector} and
  * {@link java.net.Authenticator} to detect proxies and authentication credentials and produce

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

@@ -488,12 +488,12 @@ final List<ClientInterceptor> getEffectiveInterceptors() {
   protected abstract ClientTransportFactory buildTransportFactory();
 
   /**
-   * Subclasses can override this method to provide additional parameters to {@link
-   * NameResolver.Factory#newNameResolver}. The default implementation returns {@link
-   * Attributes#EMPTY}.
+   * Subclasses can override this method to provide a default port to {@link NameResolver} for use
+   * in cases where the target string doesn't include a port.  The default implementation returns
+   * {@link GrpcUtil.DEFAULT_PORT_SSL}.
    */
-  protected Attributes getNameResolverParams() {
-    return Attributes.EMPTY;
+  protected int getDefaultPort() {
+    return GrpcUtil.DEFAULT_PORT_SSL;
   }
 
   /**
@@ -517,7 +517,7 @@ private static class DirectAddressNameResolverFactory extends NameResolver.Facto
     }
 
     @Override
-    public NameResolver newNameResolver(URI notUsedUri, Attributes params) {
+    public NameResolver newNameResolver(URI notUsedUri, NameResolver.Helper helper) {
       return new NameResolver() {
         @Override
         public String getServiceAuthority() {

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -149,9 +149,8 @@ final class DnsNameResolver extends NameResolver {
 
   private final Runnable resolveRunnable;
 
-  DnsNameResolver(@Nullable String nsAuthority, String name, Attributes params,
-      Resource<Executor> executorResource, ProxyDetector proxyDetector,
-      Stopwatch stopwatch, boolean isAndroid) {
+  DnsNameResolver(@Nullable String nsAuthority, String name, Helper helper,
+      Resource<Executor> executorResource, Stopwatch stopwatch, boolean isAndroid) {
     // TODO: if a DNS server is provided as nsAuthority, use it.
     // https://www.captechconsulting.com/blogs/accessing-the-dusty-corners-of-dns-with-java
     this.executorResource = executorResource;
@@ -163,17 +162,11 @@ final class DnsNameResolver extends NameResolver {
         "nameUri (%s) doesn't have an authority", nameUri);
     host = nameUri.getHost();
     if (nameUri.getPort() == -1) {
-      Integer defaultPort = params.get(NameResolver.Factory.PARAMS_DEFAULT_PORT);
-      if (defaultPort != null) {
-        port = defaultPort;
-      } else {
-        throw new IllegalArgumentException(
-            "name '" + name + "' doesn't contain a port, and default port is not set in params");
-      }
+      port = helper.getDefaultPort();
     } else {
       port = nameUri.getPort();
     }
-    this.proxyDetector = proxyDetector;
+    this.proxyDetector = Preconditions.checkNotNull(helper.getProxyDetector(), "proxyDetector");
     this.resolveRunnable = new Resolve(this, stopwatch, getNetworkAddressCacheTtlNanos(isAndroid));
   }
 

core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java

@@ -18,11 +18,9 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;
-import io.grpc.Attributes;
 import io.grpc.InternalServiceProviders;
-import io.grpc.NameResolver.Factory;
+import io.grpc.NameResolver;
 import io.grpc.NameResolverProvider;
-import io.grpc.ProxyDetector;
 import java.net.URI;
 
 /**
@@ -45,20 +43,17 @@ public final class DnsNameResolverProvider extends NameResolverProvider {
   private static final String SCHEME = "dns";
 
   @Override
-  public DnsNameResolver newNameResolver(URI targetUri, Attributes params) {
+  public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) {
     if (SCHEME.equals(targetUri.getScheme())) {
       String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath");
       Preconditions.checkArgument(targetPath.startsWith("/"),
           "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
       String name = targetPath.substring(1);
-      ProxyDetector proxyDetector = Preconditions
-          .checkNotNull(params.get(Factory.PARAMS_PROXY_DETECTOR), "proxyDetector");
       return new DnsNameResolver(
           targetUri.getAuthority(),
           name,
-          params,
+          helper,
           GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-          proxyDetector,
           Stopwatch.createUnstarted(),
           InternalServiceProviders.isAndroid(getClass().getClassLoader()));
     } else {

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -126,7 +126,7 @@ final class ManagedChannelImpl extends ManagedChannel implements
   private final InternalLogId logId;
   private final String target;
   private final NameResolver.Factory nameResolverFactory;
-  private final Attributes nameResolverParams;
+  private final NameResolver.Helper nameResolverHelper;
   private final LoadBalancer.Factory loadBalancerFactory;
   private final ClientTransportFactory transportFactory;
   private final ScheduledExecutorForBalancer scheduledExecutorForBalancer;
@@ -136,7 +136,6 @@ final class ManagedChannelImpl extends ManagedChannel implements
   private final ExecutorHolder balancerRpcExecutorHolder;
   private final TimeProvider timeProvider;
   private final int maxTraceEvents;
-  private final ProxyDetector proxyDetector;
 
   @VisibleForTesting
   final SynchronizationContext syncContext = new SynchronizationContext(
@@ -320,7 +319,7 @@ private void shutdownNameResolverAndLoadBalancer(boolean channelIsActive) {
       nameResolver.shutdown();
       nameResolverStarted = false;
       if (channelIsActive) {
-        nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams);
+        nameResolver = getNameResolver(target, nameResolverFactory, nameResolverHelper);
       } else {
         nameResolver = null;
       }
@@ -545,11 +544,21 @@ ClientStream newSubstream(ClientStreamTracer.Factory tracerFactory, Metadata new
     this.target = checkNotNull(builder.target, "target");
     this.logId = InternalLogId.allocate("Channel", target);
     this.nameResolverFactory = builder.getNameResolverFactory();
-    this.proxyDetector =
+    final ProxyDetector proxyDetector =
         builder.proxyDetector != null ? builder.proxyDetector : GrpcUtil.getDefaultProxyDetector();
-    this.nameResolverParams = addProxyToAttributes(this.proxyDetector,
-        checkNotNull(builder.getNameResolverParams(), "nameResolverParams"));
-    this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams);
+    final int defaultPort = builder.getDefaultPort();
+    this.nameResolverHelper = new NameResolver.Helper() {
+        @Override
+        public int getDefaultPort() {
+          return defaultPort;
+        }
+
+        @Override
+        public ProxyDetector getProxyDetector() {
+          return proxyDetector;
+        }
+      };
+    this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverHelper);
     this.timeProvider = checkNotNull(timeProvider, "timeProvider");
     maxTraceEvents = builder.maxTraceEvents;
     channelTracer = new ChannelTracer(
@@ -617,19 +626,9 @@ public CallTracer create() {
     channelz.addRootChannel(this);
   }
 
-  private static Attributes addProxyToAttributes(ProxyDetector proxyDetector,
-      Attributes attributes) {
-    if (attributes.get(NameResolver.Factory.PARAMS_PROXY_DETECTOR) == null) {
-      return attributes.toBuilder()
-          .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, proxyDetector).build();
-    } else {
-      return attributes;
-    }
-  }
-
   @VisibleForTesting
   static NameResolver getNameResolver(String target, NameResolver.Factory nameResolverFactory,
-      Attributes nameResolverParams) {
+      NameResolver.Helper nameResolverHelper) {
     // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending
     // "dns:///".
     URI targetUri = null;
@@ -644,7 +643,7 @@ static NameResolver getNameResolver(String target, NameResolver.Factory nameReso
       uriSyntaxErrors.append(e.getMessage());
     }
     if (targetUri != null) {
-      NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams);
+      NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverHelper);
       if (resolver != null) {
         return resolver;
       }
@@ -662,7 +661,7 @@ static NameResolver getNameResolver(String target, NameResolver.Factory nameReso
         // Should not be possible.
         throw new IllegalArgumentException(e);
       }
-      NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverParams);
+      NameResolver resolver = nameResolverFactory.newNameResolver(targetUri, nameResolverHelper);
       if (resolver != null) {
         return resolver;
       }

core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java

@@ -16,7 +16,6 @@
 
 package io.grpc.internal;
 
-import io.grpc.Attributes;
 import io.grpc.NameResolver;
 import java.net.URI;
 import javax.annotation.Nullable;
@@ -43,8 +42,8 @@ final class OverrideAuthorityNameResolverFactory extends NameResolver.Factory {
 
   @Nullable
   @Override
-  public NameResolver newNameResolver(URI targetUri, Attributes params) {
-    final NameResolver resolver = delegate.newNameResolver(targetUri, params);
+  public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) {
+    final NameResolver resolver = delegate.newNameResolver(targetUri, helper);
     // Do not wrap null values. We do not want to impede error signaling.
     if (resolver == null) {
       return null;

core/src/test/java/io/grpc/NameResolverProviderTest.java

@@ -22,13 +22,16 @@
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verifyZeroInteractions;
 
 import io.grpc.internal.DnsNameResolverProvider;
 import java.net.URI;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.Callable;
+import org.junit.After;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -37,7 +40,13 @@
 @RunWith(JUnit4.class)
 public class NameResolverProviderTest {
   private final URI uri = URI.create("dns:///localhost");
-  private final Attributes attributes = Attributes.EMPTY;
+  private final NameResolver.Helper helper = mock(NameResolver.Helper.class);
+
+  @After
+  public void wrapUp() {
+    // The helper is not implemented.  Make sure it's not used in the test.
+    verifyZeroInteractions(helper);
+  }
 
   @Test
   public void getDefaultScheme_noProvider() {
@@ -56,21 +65,21 @@ public void newNameResolver_providerReturnsNull() {
     List<NameResolverProvider> providers = Collections.<NameResolverProvider>singletonList(
         new BaseProvider(true, 5) {
           @Override
-          public NameResolver newNameResolver(URI passedUri, Attributes passedAttributes) {
+          public NameResolver newNameResolver(URI passedUri, NameResolver.Helper passedHelper) {
             assertSame(uri, passedUri);
-            assertSame(attributes, passedAttributes);
+            assertSame(helper, passedHelper);
             return null;
           }
         });
-    assertNull(NameResolverProvider.asFactory(providers).newNameResolver(uri, attributes));
+    assertNull(NameResolverProvider.asFactory(providers).newNameResolver(uri, helper));
   }
 
   @Test
   public void newNameResolver_noProvider() {
     List<NameResolverProvider> providers = Collections.emptyList();
     NameResolver.Factory factory = NameResolverProvider.asFactory(providers);
     try {
-      factory.newNameResolver(uri, attributes);
+      factory.newNameResolver(uri, helper);
       fail("Expected exception");
     } catch (RuntimeException ex) {
       assertTrue(ex.toString(), ex.getMessage().contains("No NameResolverProviders found"));
@@ -142,7 +151,7 @@ protected int priority() {
     }
 
     @Override
-    public NameResolver newNameResolver(URI targetUri, Attributes params) {
+    public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) {
       throw new UnsupportedOperationException();
     }