core: Optionally parse targets as RFC 3986 URIs

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

@@ -158,13 +158,48 @@ public abstract static class Factory {
      * cannot be resolved by this factory. The decision should be solely based on the scheme of the
      * URI.
      *
+     * <p>This method will eventually be deprecated and removed as part of a migration from {@code
+     * java.net.URI} to {@code io.grpc.Uri}. Implementations will override {@link
+     * #newNameResolver(Uri, Args)} instead.
+     *
      * @param targetUri the target URI to be resolved, whose scheme must not be {@code null}
      * @param args other information that may be useful
      *
      * @since 1.21.0
      */
     public abstract NameResolver newNameResolver(URI targetUri, final Args args);
 
+    /**
+     * Creates a {@link NameResolver} for the given target URI.
+     *
+     * <p>Implementations return {@code null} if 'targetUri' cannot be resolved by this factory. The
+     * decision should be solely based on the target's scheme.
+     *
+     * <p>All {@link NameResolver.Factory} implementations should override this method, as it will
+     * eventually replace {@link #newNameResolver(URI, Args)}. For backwards compatibility, this
+     * default implementation delegates to {@link #newNameResolver(URI, Args)} if 'targetUri' can be
+     * converted to a java.net.URI.
+     *
+     * <p>NB: Conversion is not always possible, for example {@code scheme:#frag} is a valid {@link
+     * Uri} but not a valid {@link URI} because its path is empty. The default implementation throws
+     * IllegalArgumentException in these cases.
+     *
+     * @param targetUri the target URI to be resolved
+     * @param args other information that may be useful
+     * @throws IllegalArgumentException if targetUri does not have the expected form
+     * @since 1.79
+     */
+    public NameResolver newNameResolver(Uri targetUri, final Args args) {
+      // Not every io.grpc.Uri can be converted but in the ordinary ManagedChannel creation flow,
+      // any IllegalArgumentException thrown here would happened anyway, just earlier. That's
+      // because parse/toString is transparent so java.net.URI#create here sees the original target
+      // string just like it did before the io.grpc.Uri migration.
+      //
+      // Throwing IAE shouldn't surprise non-framework callers either. After all, many existing
+      // Factory impls are picky about targetUri and throw IAE when it doesn't look how they expect.
+      return newNameResolver(URI.create(targetUri.toString()), args);
+    }
+
     /**
      * Returns the default scheme, which will be used to construct a URI when {@link
      * ManagedChannelBuilder#forTarget(String)} is given an authority string instead of a compliant

binder/src/androidTest/AndroidManifest.xml

@@ -13,12 +13,18 @@
         <action android:name="action1"/>
         <data android:scheme="scheme" android:host="authority" android:path="/path"/>
       </intent-filter>
+      <intent-filter>
+        <action android:name="bare.action1"/>
+      </intent-filter>
     </service>
     <service android:name="io.grpc.binder.HostServices$HostService2" android:exported="false">
       <intent-filter>
         <action android:name="action2"/>
         <data android:scheme="scheme" android:host="authority" android:path="/path"/>
       </intent-filter>
+      <intent-filter>
+        <action android:name="bare.action2"/>
+      </intent-filter>
     </service>
   </application>
 </manifest>

binder/src/androidTest/java/io/grpc/binder/BinderChannelSmokeTest.java

@@ -17,6 +17,7 @@
 package io.grpc.binder;
 
 import static com.google.common.truth.Truth.assertThat;
+import static io.grpc.internal.TestUtils.setFlagForScope;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.concurrent.TimeUnit.MINUTES;
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -252,6 +253,17 @@ public void testConnectViaTargetUri() throws Exception {
     assertThat(doCall("Hello").get()).isEqualTo("Hello");
   }
 
+  @Test
+  public void testConnectViaRfc3986TargetUri() throws Exception {
+    try (AutoCloseable unused = setFlagForScope("GRPC_ENABLE_RFC3986_URIS", true)) {
+      // Compare with the <intent-filter> mapping in AndroidManifest.xml.
+      channel =
+          BinderChannelBuilder.forTarget("intent:#Intent;action=bare.action1;end;", appContext)
+              .build();
+      assertThat(doCall("Hello").get()).isEqualTo("Hello");
+    }
+  }
+
   @Test
   public void testConnectViaIntentFilter() throws Exception {
     // Compare with the <intent-filter> mapping in AndroidManifest.xml.

binder/src/main/java/io/grpc/binder/internal/IntentNameResolverProvider.java

@@ -20,6 +20,7 @@
 import android.content.Intent;
 import com.google.common.collect.ImmutableSet;
 import io.grpc.NameResolver;
+import io.grpc.Uri;
 import io.grpc.NameResolver.Args;
 import io.grpc.NameResolverProvider;
 import io.grpc.binder.AndroidComponentAddress;
@@ -46,7 +47,17 @@ public String getDefaultScheme() {
   @Override
   public NameResolver newNameResolver(URI targetUri, final Args args) {
     if (Objects.equals(targetUri.getScheme(), ANDROID_INTENT_SCHEME)) {
-      return new IntentNameResolver(parseUriArg(targetUri), args);
+      return new IntentNameResolver(parseUriArg(targetUri.toString()), args);
+    } else {
+      return null;
+    }
+  }
+
+  @Nullable
+  @Override
+  public NameResolver newNameResolver(Uri targetUri, final Args args) {
+    if (Objects.equals(targetUri.getScheme(), ANDROID_INTENT_SCHEME)) {
+      return new IntentNameResolver(parseUriArg(targetUri.toString()), args);
     } else {
       return null;
     }
@@ -67,9 +78,9 @@ public ImmutableSet<Class<? extends SocketAddress>> getProducedSocketAddressType
     return ImmutableSet.of(AndroidComponentAddress.class);
   }
 
-  private static Intent parseUriArg(URI targetUri) {
+  private static Intent parseUriArg(String targetUri) {
     try {
-      return Intent.parseUri(targetUri.toString(), URI_INTENT_SCHEME);
+      return Intent.parseUri(targetUri, URI_INTENT_SCHEME);
     } catch (URISyntaxException e) {
       throw new IllegalArgumentException(e);
     }

binder/src/test/java/io/grpc/binder/internal/IntentNameResolverProviderTest.java

@@ -30,6 +30,7 @@
 import io.grpc.NameResolver.ServiceConfigParser;
 import io.grpc.NameResolverProvider;
 import io.grpc.SynchronizationContext;
+import io.grpc.Uri;
 import io.grpc.binder.ApiConstants;
 import java.net.URI;
 import org.junit.Before;
@@ -70,18 +71,32 @@ public void testProviderScheme_returnsIntentScheme() throws Exception {
 
   @Test
   public void testNoResolverForUnknownScheme_returnsNull() throws Exception {
-    assertThat(provider.newNameResolver(new URI("random://uri"), args)).isNull();
+    assertThat(provider.newNameResolver(Uri.create("random://uri"), args)).isNull();
   }
 
   @Test
   public void testResolutionWithBadUri_throwsIllegalArg() throws Exception {
     assertThrows(
         IllegalArgumentException.class,
-        () -> provider.newNameResolver(new URI("intent:xxx#Intent;e.x=1;end;"), args));
+        () -> provider.newNameResolver(Uri.create("intent:xxx#Intent;e.x=1;end;"), args));
   }
 
   @Test
   public void testResolverForIntentScheme_returnsResolver() throws Exception {
+    Uri uri = Uri.create("intent:#Intent;action=action;end");
+    NameResolver resolver = provider.newNameResolver(uri, args);
+    assertThat(resolver).isNotNull();
+    assertThat(resolver.getServiceAuthority()).isEqualTo("localhost");
+    syncContext.execute(() -> resolver.start(mockListener));
+    shadowOf(getMainLooper()).idle();
+    verify(mockListener).onResult2(resultCaptor.capture());
+    assertThat(resultCaptor.getValue().getAddressesOrError()).isNotNull();
+    syncContext.execute(resolver::shutdown);
+    shadowOf(getMainLooper()).idle();
+  }
+
+  @Test
+  public void testResolverForIntentScheme_returnsResolver_javaNetUri() throws Exception {
     URI uri = new URI("intent://authority/path#Intent;action=action;scheme=scheme;end");
     NameResolver resolver = provider.newNameResolver(uri, args);
     assertThat(resolver).isNotNull();

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

@@ -94,7 +94,6 @@
 import io.grpc.internal.ManagedChannelServiceConfig.ServiceConfigConvertedSelector;
 import io.grpc.internal.RetriableStream.ChannelBufferMeter;
 import io.grpc.internal.RetriableStream.Throttle;
-import java.net.URI;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -159,7 +158,7 @@ public Result selectConfig(PickSubchannelArgs args) {
   @Nullable
   private final String authorityOverride;
   private final NameResolverRegistry nameResolverRegistry;
-  private final URI targetUri;
+  private final UriWrapper targetUri;
   private final NameResolverProvider nameResolverProvider;
   private final NameResolver.Args nameResolverArgs;
   private final AutoConfiguredLoadBalancerFactory loadBalancerFactory;
@@ -546,7 +545,7 @@ ClientStream newSubstream(
   ManagedChannelImpl(
       ManagedChannelImplBuilder builder,
       ClientTransportFactory clientTransportFactory,
-      URI targetUri,
+      UriWrapper targetUri,
       NameResolverProvider nameResolverProvider,
       BackoffPolicy.Provider backoffPolicyProvider,
       ObjectPool<? extends Executor> balancerRpcExecutorPool,
@@ -677,9 +676,9 @@ public CallTracer create() {
 
   @VisibleForTesting
   static NameResolver getNameResolver(
-      URI targetUri, @Nullable final String overrideAuthority,
+      UriWrapper targetUri, @Nullable final String overrideAuthority,
       NameResolverProvider provider, NameResolver.Args nameResolverArgs) {
-    NameResolver resolver = provider.newNameResolver(targetUri, nameResolverArgs);
+    NameResolver resolver = targetUri.newNameResolver(provider, nameResolverArgs);
     if (resolver == null) {
       throw new IllegalArgumentException("cannot create a NameResolver for " + targetUri);
     }

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

@@ -18,6 +18,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static io.grpc.internal.UriWrapper.wrap;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
@@ -46,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;
@@ -718,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,
@@ -814,19 +819,32 @@ int getDefaultPort() {
 
   @VisibleForTesting
   static class ResolvedNameResolver {
-    public final URI targetUri;
+    public final UriWrapper targetUri;
     public final NameResolverProvider provider;
 
-    public ResolvedNameResolver(URI targetUri, NameResolverProvider provider) {
+    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;
@@ -862,17 +880,48 @@ 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(targetUri, provider);
+    return new ResolvedNameResolver(wrap(targetUri), provider);
   }
 
   private static class DirectAddressNameResolverProvider extends NameResolverProvider {