diff --git a/api/src/main/java/io/grpc/Uri.java b/api/src/main/java/io/grpc/Uri.java index 3034211752b..612cbbaab99 100644 --- a/api/src/main/java/io/grpc/Uri.java +++ b/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}. + * + *

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. file:/path as expected. But these methods surprisingly also return null + * when the authority is the empty string, e.g.file:///path. {@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; diff --git a/api/src/test/java/io/grpc/UriTest.java b/api/src/test/java/io/grpc/UriTest.java index e34319e8910..a0f2e96b1cd 100644 --- a/api/src/test/java/io/grpc/UriTest.java +++ b/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 = diff --git a/netty/src/main/java/io/grpc/netty/UdsNameResolver.java b/netty/src/main/java/io/grpc/netty/UdsNameResolver.java index de14dc8b460..3477a458933 100644 --- a/netty/src/main/java/io/grpc/netty/UdsNameResolver.java +++ b/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; } diff --git a/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java b/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java index fe6300057fd..baf18e3d7de 100644 --- a/netty/src/main/java/io/grpc/netty/UdsNameResolverProvider.java +++ b/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; diff --git a/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java b/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java index 9dacf00cfad..1766a8e4134 100644 --- a/netty/src/test/java/io/grpc/netty/UdsNameResolverProviderTest.java +++ b/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,16 +27,20 @@ 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; @@ -43,9 +48,17 @@ 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 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 list = result.getAddressesOrError().getValue(); - assertThat(list).isNotNull(); - assertThat(list).hasSize(1); - EquivalentAddressGroup eag = list.get(0); - assertThat(eag).isNotNull(); - List 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 list = result.getAddressesOrError().getValue(); - assertThat(list).isNotNull(); - assertThat(list).hasSize(1); - EquivalentAddressGroup eag = list.get(0); - assertThat(eag).isNotNull(); - List 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 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); } } diff --git a/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java b/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java index 22790a41c77..7bf808c18ce 100644 --- a/netty/src/test/java/io/grpc/netty/UdsNameResolverTest.java +++ b/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"); } } }