core: Use io.grpc.Uri to parse the target, if flagged

Author: jdcormie

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

@@ -47,6 +47,7 @@
 import io.grpc.NameResolverRegistry;
 import io.grpc.ProxyDetector;
 import io.grpc.StatusOr;
+import io.grpc.Uri;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.net.SocketAddress;
@@ -719,8 +720,11 @@ protected ManagedChannelImplBuilder addMetricSink(MetricSink metricSink) {
   public ManagedChannel build() {
     ClientTransportFactory clientTransportFactory =
         clientTransportFactoryBuilder.buildClientTransportFactory();
-    ResolvedNameResolver resolvedResolver = getNameResolverProvider(
-        target, nameResolverRegistry, clientTransportFactory.getSupportedSocketAddressTypes());
+    ResolvedNameResolver resolvedResolver =
+        GrpcUtil.getFlag("GRPC_ENABLE_RFC3986_URIS", false)
+            ? getNameResolverProviderNew(target, nameResolverRegistry)
+            : getNameResolverProvider(target, nameResolverRegistry);
+    resolvedResolver.checkAddressTypes(clientTransportFactory.getSupportedSocketAddressTypes());
     return new ManagedChannelOrphanWrapper(new ManagedChannelImpl(
         this,
         clientTransportFactory,
@@ -822,12 +826,25 @@ public ResolvedNameResolver(UriWrapper targetUri, NameResolverProvider provider)
       this.targetUri = checkNotNull(targetUri, "targetUri");
       this.provider = checkNotNull(provider, "provider");
     }
+
+    void checkAddressTypes(
+        Collection<Class<? extends SocketAddress>> channelTransportSocketAddressTypes) {
+      if (channelTransportSocketAddressTypes != null) {
+        Collection<Class<? extends SocketAddress>> nameResolverSocketAddressTypes =
+            provider.getProducedSocketAddressTypes();
+        if (!channelTransportSocketAddressTypes.containsAll(nameResolverSocketAddressTypes)) {
+          throw new IllegalArgumentException(
+              String.format(
+                  "Address types of NameResolver '%s' for '%s' not supported by transport",
+                  provider.getDefaultScheme(), targetUri));
+        }
+      }
+    }
   }
 
   @VisibleForTesting
   static ResolvedNameResolver getNameResolverProvider(
-      String target, NameResolverRegistry nameResolverRegistry,
-      Collection<Class<? extends SocketAddress>> channelTransportSocketAddressTypes) {
+      String target, NameResolverRegistry nameResolverRegistry) {
     // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending
     // "dns:///".
     NameResolverProvider provider = null;
@@ -863,14 +880,45 @@ static ResolvedNameResolver getNameResolverProvider(
           target, uriSyntaxErrors.length() > 0 ? " (" + uriSyntaxErrors + ")" : ""));
     }
 
-    if (channelTransportSocketAddressTypes != null) {
-      Collection<Class<? extends SocketAddress>> nameResolverSocketAddressTypes
-          = provider.getProducedSocketAddressTypes();
-      if (!channelTransportSocketAddressTypes.containsAll(nameResolverSocketAddressTypes)) {
-        throw new IllegalArgumentException(String.format(
-            "Address types of NameResolver '%s' for '%s' not supported by transport",
-            targetUri.getScheme(), target));
-      }
+    return new ResolvedNameResolver(wrap(targetUri), provider);
+  }
+
+  static ResolvedNameResolver getNameResolverProviderNew(
+      String target, NameResolverRegistry nameResolverRegistry) {
+    // Finding a NameResolver. Try using the target string as the URI. If that fails, try prepending
+    // "dns:///".
+    NameResolverProvider provider = null;
+    Uri targetUri = null;
+    StringBuilder uriSyntaxErrors = new StringBuilder();
+    try {
+      targetUri = Uri.parse(target);
+    } catch (URISyntaxException e) {
+      // Can happen with ip addresses like "[::1]:1234" or 127.0.0.1:1234.
+      uriSyntaxErrors.append(e.getMessage());
+    }
+    if (targetUri != null) {
+      // For "localhost:8080" this would likely cause provider to be null, because "localhost" is
+      // parsed as the scheme. Will hit the next case and try "dns:///localhost:8080".
+      provider = nameResolverRegistry.getProviderForScheme(targetUri.getScheme());
+    }
+
+    if (provider == null && !URI_PATTERN.matcher(target).matches()) {
+      // It doesn't look like a URI target. Maybe it's an authority string. Try with the default
+      // scheme from the registry.
+      targetUri =
+          Uri.newBuilder()
+              .setScheme(nameResolverRegistry.getDefaultScheme())
+              .setHost("")
+              .setPath("/" + target)
+              .build();
+      provider = nameResolverRegistry.getProviderForScheme(targetUri.getScheme());
+    }
+
+    if (provider == null) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Could not find a NameResolverProvider for %s%s",
+              target, uriSyntaxErrors.length() > 0 ? " (" + uriSyntaxErrors + ")" : ""));
     }
 
     return new ResolvedNameResolver(wrap(targetUri), provider);

core/src/test/java/io/grpc/internal/ManagedChannelImplBuilderTest.java

@@ -17,6 +17,7 @@
 package io.grpc.internal;
 
 import static com.google.common.truth.Truth.assertThat;
+import static io.grpc.internal.TestUtils.setFlagForScope;
 import static junit.framework.TestCase.assertFalse;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
@@ -56,26 +57,31 @@
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.net.URI;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Deque;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Pattern;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
 /** Unit tests for {@link ManagedChannelImplBuilder}. */
-@RunWith(JUnit4.class)
+@RunWith(Parameterized.class)
 public class ManagedChannelImplBuilderTest {
   private static final int DUMMY_PORT = 42;
   private static final String DUMMY_TARGET = "fake-target";
@@ -98,6 +104,13 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
         }
       };
 
+  @Parameters(name = "enableRfc3986UrisParam={0}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][] {{true}, {false}});
+  }
+
+  @Parameter public boolean enableRfc3986UrisParam;
+
   @Rule public final MockitoRule mocks = MockitoJUnit.rule();
   @Rule public final GrpcCleanupRule grpcCleanupRule = new GrpcCleanupRule();
 
@@ -114,9 +127,12 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
               "io\\.grpc\\.InternalConfigurator|io\\.grpc\\.Configurator|"
                   + "io\\.grpc\\.InternalConfiguratorRegistry|io\\.grpc\\.ConfiguratorRegistry|"
                   + "io\\.grpc\\.internal\\.[^.]+"));
+  private final Deque<AutoCloseable> toBeClosedUponTeardown = new ArrayDeque<>();
 
   @Before
   public void setUp() throws Exception {
+    toBeClosedUponTeardown.push(
+        setFlagForScope("GRPC_ENABLE_RFC3986_URIS", enableRfc3986UrisParam));
     builder = new ManagedChannelImplBuilder(
         DUMMY_TARGET,
         new UnsupportedClientTransportFactoryBuilder(),
@@ -128,6 +144,13 @@ public void setUp() throws Exception {
         new FixedPortProvider(DUMMY_PORT));
   }
 
+  @After
+  public void cleanUpCloseables() throws Exception {
+    while (!toBeClosedUponTeardown.isEmpty()) {
+      toBeClosedUponTeardown.pop().close();
+    }
+  }
+
   /** Ensure getDefaultPort() returns default port when no custom implementation provided. */
   @Test
   public void getDefaultPort_default() {
@@ -373,8 +396,11 @@ public void transportDoesNotSupportAddressTypes() {
       ManagedChannel unused = grpcCleanupRule.register(builder.build());
       fail("Should fail");
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo(
-          "Address types of NameResolver 'dns' for 'valid:1234' not supported by transport");
+      assertThat(e)
+          .hasMessageThat()
+          .isEqualTo(
+              "Address types of NameResolver 'dns' for 'dns:///valid:1234' not supported by"
+                  + " transport");
     }
   }