Add RFC 3986 support to the 'unix:' name resolver.

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

@@ -148,6 +148,15 @@
  * itself. RFC 9844 claims to obsolete RFC 6874 because web browsers would not support it. This
  * class implements RFC 6874 anyway, mostly to avoid creating a barrier to migration away from
  * {@link java.net.URI}.
+ *
+ * <p>Some URI components, e.g. scheme, are required while others may or may not be present, e.g.
+ * authority. {@link Uri} is careful to preserve the distinction between an absent string component
+ * (getter returns null) and one with an empty value (getter returns ""). {@link java.net.URI} makes
+ * this distinction too, *except* when it comes to the authority and host components: {@link
+ * java.net.URI#getAuthority()} and {@link java.net.URI#getHost()} return null when an authority is
+ * absent, e.g. <code>file:/path</code> as expected. But these methods surprisingly also return null
+ * when the authority is the empty string, e.g.<code>file:///path</code>. {@link Uri}'s getters
+ * correctly return null and "" in these cases, respectively, as one would expect.
  */
 @Internal
 public final class Uri {
@@ -437,9 +446,9 @@ public String getRawHost() {
     return host;
   }
 
-  /** Returns the "port" component of this URI, or -1 if not present. */
+  /** Returns the "port" component of this URI, or -1 if empty or not present. */
   public int getPort() {
-    return port != null ? Integer.parseInt(port) : -1;
+    return port != null && !port.isEmpty() ? Integer.parseInt(port) : -1;
   }
 
   /** Returns the raw port component of this URI in its originally parsed form. */
@@ -934,10 +943,12 @@ public Builder setPort(int port) {
 
     @CanIgnoreReturnValue
     Builder setRawPort(String port) {
-      try {
-        Integer.parseInt(port); // Result unused.
-      } catch (NumberFormatException e) {
-        throw new IllegalArgumentException("Invalid port", e);
+      if (port != null && !port.isEmpty()) {
+        try {
+          Integer.parseInt(port); // Result unused.
+        } catch (NumberFormatException e) {
+          throw new IllegalArgumentException("Invalid port", e);
+        }
       }
       this.port = port;
       return this;

api/src/test/java/io/grpc/UriTest.java

@@ -161,6 +161,42 @@ public void parse_emptyPath() throws URISyntaxException {
     assertThat(uri.isPathRootless()).isFalse();
   }
 
+  @Test
+  public void parse_emptyQuery() {
+    Uri uri = Uri.create("scheme:?");
+    assertThat(uri.getScheme()).isEqualTo("scheme");
+    assertThat(uri.getQuery()).isEmpty();
+  }
+
+  @Test
+  public void parse_emptyFragment() {
+    Uri uri = Uri.create("scheme:#");
+    assertThat(uri.getScheme()).isEqualTo("scheme");
+    assertThat(uri.getFragment()).isEmpty();
+  }
+
+  @Test
+  public void parse_emptyUserInfo() {
+    Uri uri = Uri.create("scheme://@host");
+    assertThat(uri.getScheme()).isEqualTo("scheme");
+    assertThat(uri.getAuthority()).isEqualTo("@host");
+    assertThat(uri.getHost()).isEqualTo("host");
+    assertThat(uri.getUserInfo()).isEmpty();
+    assertThat(uri.toString()).isEqualTo("scheme://@host");
+  }
+
+  @Test
+  public void parse_emptyPort() {
+    Uri uri = Uri.create("scheme://host:");
+    assertThat(uri.getScheme()).isEqualTo("scheme");
+    assertThat(uri.getAuthority()).isEqualTo("host:");
+    assertThat(uri.getRawAuthority()).isEqualTo("host:");
+    assertThat(uri.getHost()).isEqualTo("host");
+    assertThat(uri.getPort()).isEqualTo(-1);
+    assertThat(uri.getRawPort()).isEqualTo("");
+    assertThat(uri.toString()).isEqualTo("scheme://host:");
+  }
+
   @Test
   public void parse_invalidScheme_throws() {
     URISyntaxException e =
@@ -235,13 +271,6 @@ public void parse_invalidBackslashScope_throws() {
     assertThat(e).hasMessageThat().contains("Invalid character in scope");
   }
 
-  @Test
-  public void parse_emptyPort_throws() {
-    URISyntaxException e =
-        assertThrows(URISyntaxException.class, () -> Uri.parse("scheme://user@host:/path"));
-    assertThat(e).hasMessageThat().contains("Invalid port");
-  }
-
   @Test
   public void parse_invalidCharactersInPort_throws() {
     URISyntaxException e =

netty/src/main/java/io/grpc/netty/UdsNameResolver.java

@@ -18,6 +18,7 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.isNullOrEmpty;
 
 import com.google.common.base.Preconditions;
 import io.grpc.EquivalentAddressGroup;
@@ -31,8 +32,18 @@ final class UdsNameResolver extends NameResolver {
   private NameResolver.Listener2 listener;
   private final String authority;
 
+  /**
+   * Constructs a new instance of UdsNameResolver.
+   *
+   * @param authority authority of the 'unix:' URI to resolve, or null if target has no authority
+   * @param targetPath path of the 'unix:' URI to resolve
+   */
   UdsNameResolver(String authority, String targetPath, Args args) {
-    checkArgument(authority == null, "non-null authority not supported");
+    // UDS is inherently local. According to https://github.com/grpc/grpc/blob/master/doc/naming.md,
+    // this is expressed in the target URI either by using a blank authority, like "unix:///sock",
+    // or by omitting authority completely, e.g. "unix:/sock".
+    // TODO(jdcormie): Allow the explicit authority string "localhost"?
+    checkArgument(isNullOrEmpty(authority), "authority not supported: %s", authority);
     this.authority = targetPath;
   }
 

netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java

@@ -20,6 +20,7 @@
 import io.grpc.Internal;
 import io.grpc.NameResolver;
 import io.grpc.NameResolverProvider;
+import io.grpc.Uri;
 import io.netty.channel.unix.DomainSocketAddress;
 import java.net.SocketAddress;
 import java.net.URI;
@@ -31,9 +32,21 @@ public final class UdsNameResolverProvider extends NameResolverProvider {
 
   private static final String SCHEME = "unix";
 
+  @Override
+  public NameResolver newNameResolver(Uri targetUri, NameResolver.Args args) {
+    if (SCHEME.equals(targetUri.getScheme())) {
+      return new UdsNameResolver(targetUri.getAuthority(), targetUri.getPath(), args);
+    } else {
+      return null;
+    }
+  }
+
   @Override
   public UdsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
     if (SCHEME.equals(targetUri.getScheme())) {
+      // TODO(jdcormie): java.net.URI has a bug where getAuthority() returns null for both the
+      // undefined and zero-length authority. Doesn't matter for now because UdsNameResolver doesn't
+      // distinguish these cases.
       return new UdsNameResolver(targetUri.getAuthority(), getTargetPathFromUri(targetUri), args);
     } else {
       return null;
@@ -44,6 +57,10 @@ static String getTargetPathFromUri(URI targetUri) {
     Preconditions.checkArgument(SCHEME.equals(targetUri.getScheme()), "scheme must be " + SCHEME);
     String targetPath = targetUri.getPath();
     if (targetPath == null) {
+      // TODO(jdcormie): This incorrectly includes '?' and any characters that follow. In the
+      // hierarchical case ('unix:///path'), java.net.URI parses these into a query component that's
+      // distinct from the path. But in the present "opaque" case ('unix:/path'), what may look like
+      // a query is considered part of the SSP.
       targetPath = Preconditions.checkNotNull(targetUri.getSchemeSpecificPart(), "targetPath");
     }
     return targetPath;

netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java

@@ -17,6 +17,7 @@
 package io.grpc.netty;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -26,26 +27,38 @@
 import io.grpc.NameResolver;
 import io.grpc.NameResolver.ServiceConfigParser;
 import io.grpc.SynchronizationContext;
+import io.grpc.Uri;
 import io.grpc.internal.FakeClock;
 import io.grpc.internal.GrpcUtil;
 import io.netty.channel.unix.DomainSocketAddress;
 import java.net.SocketAddress;
 import java.net.URI;
+import java.util.Arrays;
 import java.util.List;
 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.ArgumentCaptor;
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnit;
 import org.mockito.junit.MockitoRule;
 
 /** Unit tests for {@link UdsNameResolverProvider}. */
-@RunWith(JUnit4.class)
+@RunWith(Parameterized.class)
 public class UdsNameResolverProviderTest {
   private static final int DEFAULT_PORT = 887;
+
+  @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();
 
@@ -73,48 +86,60 @@ public class UdsNameResolverProviderTest {
 
   @Test
   public void testUnixRelativePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:sock.sock"), args);
-    assertThat(udsNameResolver).isNotNull();
-    udsNameResolver.start(mockListener);
-    verify(mockListener).onResult2(resultCaptor.capture());
-    NameResolver.ResolutionResult result = resultCaptor.getValue();
-    List<EquivalentAddressGroup> list = result.getAddressesOrError().getValue();
-    assertThat(list).isNotNull();
-    assertThat(list).hasSize(1);
-    EquivalentAddressGroup eag = list.get(0);
-    assertThat(eag).isNotNull();
-    List<SocketAddress> addresses = eag.getAddresses();
-    assertThat(addresses).hasSize(1);
-    assertThat(addresses.get(0)).isInstanceOf(DomainSocketAddress.class);
-    DomainSocketAddress domainSocketAddress = (DomainSocketAddress) addresses.get(0);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:sock.sock", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
     assertThat(domainSocketAddress.path()).isEqualTo("sock.sock");
   }
 
   @Test
   public void testUnixAbsolutePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:/sock.sock"), args);
-    assertThat(udsNameResolver).isNotNull();
-    udsNameResolver.start(mockListener);
-    verify(mockListener).onResult2(resultCaptor.capture());
-    NameResolver.ResolutionResult result = resultCaptor.getValue();
-    List<EquivalentAddressGroup> list = result.getAddressesOrError().getValue();
-    assertThat(list).isNotNull();
-    assertThat(list).hasSize(1);
-    EquivalentAddressGroup eag = list.get(0);
-    assertThat(eag).isNotNull();
-    List<SocketAddress> addresses = eag.getAddresses();
-    assertThat(addresses).hasSize(1);
-    assertThat(addresses.get(0)).isInstanceOf(DomainSocketAddress.class);
-    DomainSocketAddress domainSocketAddress = (DomainSocketAddress) addresses.get(0);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:/sock.sock", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
     assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
   }
 
   @Test
   public void testUnixAbsoluteAlternatePath() {
-    UdsNameResolver udsNameResolver =
-        udsNameResolverProvider.newNameResolver(URI.create("unix:///sock.sock"), args);
+    UdsNameResolver udsNameResolver = newNameResolver("unix:///sock.sock", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
+    assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
+  }
+
+  @Test
+  public void testUnixPathWithAuthority() {
+    try {
+      newNameResolver("unix://localhost/sock.sock", args);
+      fail("exception expected");
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessageThat().isEqualTo("authority not supported: localhost");
+    }
+  }
+
+  @Test
+  public void testUnixAbsolutePathDoesNotIncludeQueryOrFragment() {
+    UdsNameResolver udsNameResolver = newNameResolver("unix:///sock.sock?query#fragment", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
+    assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
+  }
+
+  @Test
+  public void testUnixRelativePathDoesNotIncludeQueryOrFragment() {
+    // This test fails without RFC 3986 support because of a bug in the legacy java.net.URI-based
+    // NRP implementation.
+    assume().that(enableRfc3986UrisParam).isTrue();
+
+    UdsNameResolver udsNameResolver = newNameResolver("unix:sock.sock?query#fragment", args);
+    DomainSocketAddress domainSocketAddress = startAndGetUniqueResolvedAddress(udsNameResolver);
+    assertThat(domainSocketAddress.path()).isEqualTo("sock.sock");
+  }
+
+  private UdsNameResolver newNameResolver(String uriString, NameResolver.Args args) {
+    return enableRfc3986UrisParam
+        ? (UdsNameResolver) udsNameResolverProvider.newNameResolver(Uri.create(uriString), args)
+        : udsNameResolverProvider.newNameResolver(URI.create(uriString), args);
+  }
+
+  private DomainSocketAddress startAndGetUniqueResolvedAddress(UdsNameResolver udsNameResolver) {
     assertThat(udsNameResolver).isNotNull();
     udsNameResolver.start(mockListener);
     verify(mockListener).onResult2(resultCaptor.capture());
@@ -127,17 +152,6 @@ public void testUnixAbsoluteAlternatePath() {
     List<SocketAddress> addresses = eag.getAddresses();
     assertThat(addresses).hasSize(1);
     assertThat(addresses.get(0)).isInstanceOf(DomainSocketAddress.class);
-    DomainSocketAddress domainSocketAddress = (DomainSocketAddress) addresses.get(0);
-    assertThat(domainSocketAddress.path()).isEqualTo("/sock.sock");
-  }
-
-  @Test
-  public void testUnixPathWithAuthority() {
-    try {
-      udsNameResolverProvider.newNameResolver(URI.create("unix://localhost/sock.sock"), args);
-      fail("exception expected");
-    } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("non-null authority not supported");
-    }
+    return (DomainSocketAddress) addresses.get(0);
   }
 }

netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java

@@ -91,10 +91,10 @@ public void testValidTargetPath() {
   @Test
   public void testNonNullAuthority() {
     try {
-      udsNameResolver = new UdsNameResolver("authority", "sock.sock", args);
+      udsNameResolver = new UdsNameResolver("somehost", "sock.sock", args);
       fail("exception expected");
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessageThat().isEqualTo("non-null authority not supported");
+      assertThat(e).hasMessageThat().isEqualTo("authority not supported: somehost");
     }
   }
 }