diff --git a/core/src/main/java/io/grpc/NameResolver.java b/core/src/main/java/io/grpc/NameResolver.java index b748d25dd9..c188eca0d9 100644 --- a/core/src/main/java/io/grpc/NameResolver.java +++ b/core/src/main/java/io/grpc/NameResolver.java @@ -94,16 +94,24 @@ public abstract class NameResolver { * 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 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 PARAMS_PROXY_DETECTOR = Attributes.Key.create("params-proxy-detector"); @@ -114,11 +122,39 @@ public abstract class NameResolver { * * @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 @@ public abstract class NameResolver { @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(); + } } diff --git a/core/src/main/java/io/grpc/NameResolverProvider.java b/core/src/main/java/io/grpc/NameResolverProvider.java index f0b76ea70c..0347a174aa 100644 --- a/core/src/main/java/io/grpc/NameResolverProvider.java +++ b/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 PARAMS_DEFAULT_PORT = NameResolver.Factory.PARAMS_DEFAULT_PORT; @@ -106,10 +107,10 @@ public abstract class NameResolverProvider 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; } diff --git a/core/src/main/java/io/grpc/ProxyDetector.java b/core/src/main/java/io/grpc/ProxyDetector.java index 56866eacf2..5202516bca 100644 --- a/core/src/main/java/io/grpc/ProxyDetector.java +++ b/core/src/main/java/io/grpc/ProxyDetector.java @@ -31,11 +31,10 @@ import javax.annotation.Nullable; *

In order for gRPC to use a proxy, {@link NameResolver}, {@link ProxyDetector} and the * underlying transport need to work together. * - *

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. + *

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. * *

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 diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 76ce3e57b8..b72a840b84 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -488,12 +488,12 @@ public abstract class AbstractManagedChannelImplBuilder 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 @@ public abstract class AbstractManagedChannelImplBuilder } @Override - public NameResolver newNameResolver(URI notUsedUri, Attributes params) { + public NameResolver newNameResolver(URI notUsedUri, NameResolver.Helper helper) { return new NameResolver() { @Override public String getServiceAuthority() { diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolver.java b/core/src/main/java/io/grpc/internal/DnsNameResolver.java index 3adfe1ba9f..fcbb3bb1b1 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolver.java +++ b/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 executorResource, ProxyDetector proxyDetector, - Stopwatch stopwatch, boolean isAndroid) { + DnsNameResolver(@Nullable String nsAuthority, String name, Helper helper, + Resource 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)); } diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index bf80c7eaba..83eaef2e83 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -18,11 +18,9 @@ package io.grpc.internal; 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 { diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 020311e3c1..a6bb50463c 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/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 @@ final class ManagedChannelImpl extends ManagedChannel implements nameResolver.shutdown(); nameResolverStarted = false; if (channelIsActive) { - nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); + nameResolver = getNameResolver(target, nameResolverFactory, nameResolverHelper); } else { nameResolver = null; } @@ -545,11 +544,21 @@ final class ManagedChannelImpl extends ManagedChannel implements 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 @@ final class ManagedChannelImpl extends ManagedChannel implements 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 @@ final class ManagedChannelImpl extends ManagedChannel implements 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 @@ final class ManagedChannelImpl extends ManagedChannel implements // 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; } diff --git a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java b/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java index 1e2154a4e0..ca75af764a 100644 --- a/core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java +++ b/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; diff --git a/core/src/test/java/io/grpc/NameResolverProviderTest.java b/core/src/test/java/io/grpc/NameResolverProviderTest.java index 10a529b4c3..5b5f6a8a95 100644 --- a/core/src/test/java/io/grpc/NameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/NameResolverProviderTest.java @@ -22,6 +22,8 @@ import static org.junit.Assert.assertNull; 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; @@ -29,6 +31,7 @@ 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 @@ import org.junit.runners.JUnit4; @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,13 +65,13 @@ public class NameResolverProviderTest { List providers = Collections.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 @@ -70,7 +79,7 @@ public class NameResolverProviderTest { List 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 @@ public class NameResolverProviderTest { } @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { throw new UnsupportedOperationException(); } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 273e6d2871..4922d6350d 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -20,8 +20,8 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; -import io.grpc.Attributes; import io.grpc.NameResolver; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,10 +31,17 @@ import org.junit.runners.JUnit4; @RunWith(JUnit4.class) public class DnsNameResolverProviderTest { - private static final Attributes ATTRIBUTES = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + throw new UnsupportedOperationException("Should not be called"); + } + + @Override + public ProxyDetector getProxyDetector() { + return GrpcUtil.getDefaultProxyDetector(); + } + }; private DnsNameResolverProvider provider = new DnsNameResolverProvider(); @@ -46,8 +53,8 @@ public class DnsNameResolverProviderTest { @Test public void newNameResolver() { assertSame(DnsNameResolver.class, - provider.newNameResolver(URI.create("dns:///localhost:443"), ATTRIBUTES).getClass()); + provider.newNameResolver(URI.create("dns:///localhost:443"), HELPER).getClass()); assertNull( - provider.newNameResolver(URI.create("notdns:///localhost:443"), ATTRIBUTES)); + provider.newNameResolver(URI.create("notdns:///localhost:443"), HELPER)); } } diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java index e609a91b73..367c855834 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -95,11 +95,17 @@ public class DnsNameResolverTest { private final Map serviceConfig = new LinkedHashMap<>(); private static final int DEFAULT_PORT = 887; - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, DEFAULT_PORT) - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return DEFAULT_PORT; + } + + @Override + public ProxyDetector getProxyDetector() { + return GrpcUtil.getDefaultProxyDetector(); + } + }; private final DnsNameResolverProvider provider = new DnsNameResolverProvider(); private final FakeClock fakeClock = new FakeClock(); @@ -146,16 +152,25 @@ public class DnsNameResolverTest { private DnsNameResolver newResolver( String name, - int port, - ProxyDetector proxyDetector, + final int port, + final ProxyDetector proxyDetector, Stopwatch stopwatch, boolean isAndroid) { DnsNameResolver dnsResolver = new DnsNameResolver( null, name, - Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, port).build(), + new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return port; + } + + @Override + public ProxyDetector getProxyDetector() { + return proxyDetector; + } + }, fakeExecutorResource, - proxyDetector, stopwatch, isAndroid); // By default, using the mocked ResourceResolver to avoid I/O @@ -279,9 +294,7 @@ public class DnsNameResolverTest { public void resolveAll_failsOnEmptyResult() throws Exception { String hostname = "dns:///addr.fake:1234"; DnsNameResolver nrf = - new DnsNameResolverProvider().newNameResolver(new URI(hostname), Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build()); + new DnsNameResolverProvider().newNameResolver(new URI(hostname), HELPER); nrf.setAddressResolver(new AddressResolver() { @Override public List resolveAddress(String host) throws Exception { @@ -993,7 +1006,7 @@ public class DnsNameResolverTest { private void testInvalidUri(URI uri) { try { - provider.newNameResolver(uri, NAME_RESOLVER_PARAMS); + provider.newNameResolver(uri, HELPER); fail("Should have failed"); } catch (IllegalArgumentException e) { // expected @@ -1001,7 +1014,7 @@ public class DnsNameResolverTest { } private void testValidUri(URI uri, String exportedAuthority, int expectedPort) { - DnsNameResolver resolver = provider.newNameResolver(uri, NAME_RESOLVER_PARAMS); + DnsNameResolver resolver = provider.newNameResolver(uri, HELPER); assertNotNull(resolver); assertEquals(expectedPort, resolver.getPort()); assertEquals(exportedAuthority, resolver.getServiceAuthority()); diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java index 2fd3b9b694..f08806102a 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java @@ -20,9 +20,9 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; -import io.grpc.Attributes; import io.grpc.NameResolver; import io.grpc.NameResolver.Factory; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,8 +31,17 @@ import org.junit.runners.JUnit4; /** Unit tests for {@link ManagedChannelImpl#getNameResolver}. */ @RunWith(JUnit4.class) public class ManagedChannelImplGetNameResolverTest { - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder().set(NameResolver.Factory.PARAMS_DEFAULT_PORT, 447).build(); + private static final NameResolver.Helper NAMERESOLVER_HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + return 447; + } + + @Override + public ProxyDetector getProxyDetector() { + throw new UnsupportedOperationException("Should not be called"); + } + }; @Test public void invalidUriTarget() { @@ -96,7 +105,7 @@ public class ManagedChannelImplGetNameResolverTest { public void validTargetNoResovler() { Factory nameResolverFactory = new NameResolver.Factory() { @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { return null; } @@ -107,7 +116,7 @@ public class ManagedChannelImplGetNameResolverTest { }; try { ManagedChannelImpl.getNameResolver( - "foo.googleapis.com:8080", nameResolverFactory, NAME_RESOLVER_PARAMS); + "foo.googleapis.com:8080", nameResolverFactory, NAMERESOLVER_HELPER); fail("Should fail"); } catch (IllegalArgumentException e) { // expected @@ -117,7 +126,7 @@ public class ManagedChannelImplGetNameResolverTest { private void testValidTarget(String target, String expectedUriString, URI expectedUri) { Factory nameResolverFactory = new FakeNameResolverFactory(expectedUri.getScheme()); FakeNameResolver nameResolver = (FakeNameResolver) ManagedChannelImpl.getNameResolver( - target, nameResolverFactory, NAME_RESOLVER_PARAMS); + target, nameResolverFactory, NAMERESOLVER_HELPER); assertNotNull(nameResolver); assertEquals(expectedUri, nameResolver.uri); assertEquals(expectedUriString, nameResolver.uri.toString()); @@ -128,7 +137,7 @@ public class ManagedChannelImplGetNameResolverTest { try { FakeNameResolver nameResolver = (FakeNameResolver) ManagedChannelImpl.getNameResolver( - target, nameResolverFactory, NAME_RESOLVER_PARAMS); + target, nameResolverFactory, NAMERESOLVER_HELPER); fail("Should have failed, but got resolver with " + nameResolver.uri); } catch (IllegalArgumentException e) { // expected @@ -143,7 +152,7 @@ public class ManagedChannelImplGetNameResolverTest { } @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { if (expectedScheme.equals(targetUri.getScheme())) { return new FakeNameResolver(targetUri); } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 75cf2ee056..e48a1c2527 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -142,7 +142,7 @@ public class ManagedChannelImplIdlenessTest { LoadBalancerRegistry.getDefaultRegistry().register(mockLoadBalancerProvider); when(mockNameResolver.getServiceAuthority()).thenReturn(AUTHORITY); when(mockNameResolverFactory - .newNameResolver(any(URI.class), any(Attributes.class))) + .newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(mockNameResolver); when(mockTransportFactory.getScheduledExecutorService()) .thenReturn(timer.getScheduledExecutorService()); @@ -181,7 +181,7 @@ public class ManagedChannelImplIdlenessTest { } servers.add(new EquivalentAddressGroup(addrs)); } - verify(mockNameResolverFactory).newNameResolver(any(URI.class), any(Attributes.class)); + verify(mockNameResolverFactory).newNameResolver(any(URI.class), any(NameResolver.Helper.class)); // Verify the initial idleness verify(mockLoadBalancerProvider, never()).newLoadBalancer(any(Helper.class)); verify(mockTransportFactory, never()).newClientTransport( diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 248a0692ce..7a4b6d8762 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -89,6 +89,8 @@ import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.MethodType; import io.grpc.NameResolver; +import io.grpc.ProxiedSocketAddress; +import io.grpc.ProxyDetector; import io.grpc.SecurityLevel; import io.grpc.ServerMethodDefinition; import io.grpc.Status; @@ -142,11 +144,7 @@ import org.mockito.stubbing.Answer; /** Unit tests for {@link ManagedChannelImpl}. */ @RunWith(JUnit4.class) public class ManagedChannelImplTest { - private static final Attributes NAME_RESOLVER_PARAMS = - Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, 447) - .set(NameResolver.Factory.PARAMS_PROXY_DETECTOR, GrpcUtil.getDefaultProxyDetector()) - .build(); + private static final int DEFAULT_PORT = 447; private static final MethodDescriptor method = MethodDescriptor.newBuilder() @@ -3175,7 +3173,7 @@ public class ManagedChannelImplTest { @Nullable @Override - public NameResolver newNameResolver(URI targetUri, Attributes params) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { return (resolver = new FakeNameResolver()); } @@ -3240,6 +3238,93 @@ public class ManagedChannelImplTest { mychannel.shutdownNow(); } + @Test + public void nameResolverHelperPropagation() { + final AtomicReference capturedHelper = new AtomicReference<>(); + final NameResolver noopResolver = new NameResolver() { + @Override + public String getServiceAuthority() { + return "fake-authority"; + } + + @Override + public void start(Listener listener) { + } + + @Override + public void shutdown() {} + }; + ProxyDetector neverProxy = new ProxyDetector() { + @Override + public ProxiedSocketAddress proxyFor(SocketAddress targetAddress) { + return null; + } + }; + NameResolver.Factory oldApiFactory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, NameResolver.Helper helper) { + capturedHelper.set(helper); + return noopResolver; + } + + @Override + public String getDefaultScheme() { + return "fakescheme"; + } + }; + channelBuilder.nameResolverFactory(oldApiFactory).proxyDetector(neverProxy); + createChannel(); + + NameResolver.Helper helper = capturedHelper.get(); + assertThat(helper).isNotNull(); + assertThat(helper.getDefaultPort()).isEqualTo(DEFAULT_PORT); + assertThat(helper.getProxyDetector()).isSameAs(neverProxy); + } + + @Test + @Deprecated + public void nameResolverParams_oldApi() { + final AtomicReference capturedParams = new AtomicReference<>(); + final NameResolver noopResolver = new NameResolver() { + @Override + public String getServiceAuthority() { + return "fake-authority"; + } + + @Override + public void start(Listener listener) { + } + + @Override + public void shutdown() {} + }; + ProxyDetector neverProxy = new ProxyDetector() { + @Override + public ProxiedSocketAddress proxyFor(SocketAddress targetAddress) { + return null; + } + }; + NameResolver.Factory oldApiFactory = new NameResolver.Factory() { + @Override + public NameResolver newNameResolver(URI targetUri, Attributes params) { + capturedParams.set(params); + return noopResolver; + } + + @Override + public String getDefaultScheme() { + return "fakescheme"; + } + }; + channelBuilder.nameResolverFactory(oldApiFactory).proxyDetector(neverProxy); + createChannel(); + + Attributes attrs = capturedParams.get(); + assertThat(attrs).isNotNull(); + assertThat(attrs.get(NameResolver.Factory.PARAMS_DEFAULT_PORT)).isEqualTo(DEFAULT_PORT); + assertThat(attrs.get(NameResolver.Factory.PARAMS_PROXY_DETECTOR)).isSameAs(neverProxy); + } + @Test public void getAuthorityAfterShutdown() throws Exception { createChannel(); @@ -3259,8 +3344,8 @@ public class ManagedChannelImplTest { throw new UnsupportedOperationException(); } - @Override protected Attributes getNameResolverParams() { - return NAME_RESOLVER_PARAMS; + @Override protected int getDefaultPort() { + return DEFAULT_PORT; } } @@ -3300,11 +3385,11 @@ public class ManagedChannelImplTest { } @Override - public NameResolver newNameResolver(final URI targetUri, Attributes params) { + public NameResolver newNameResolver(final URI targetUri, NameResolver.Helper helper) { if (!expectedUri.equals(targetUri)) { return null; } - assertSame(NAME_RESOLVER_PARAMS, params); + assertEquals(DEFAULT_PORT, helper.getDefaultPort()); FakeNameResolver resolver = new FakeNameResolver(error); resolvers.add(resolver); return resolver; diff --git a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java index 619b5ffc56..5c3ae49b15 100644 --- a/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/OverrideAuthorityNameResolverTest.java @@ -23,8 +23,8 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import io.grpc.Attributes; import io.grpc.NameResolver; +import io.grpc.ProxyDetector; import java.net.URI; import org.junit.Test; import org.junit.runner.RunWith; @@ -33,17 +33,28 @@ import org.junit.runners.JUnit4; /** Unit tests for {@link OverrideAuthorityNameResolverFactory}. */ @RunWith(JUnit4.class) public class OverrideAuthorityNameResolverTest { + private static final NameResolver.Helper HELPER = new NameResolver.Helper() { + @Override + public int getDefaultPort() { + throw new UnsupportedOperationException("Should not be called"); + } + + @Override + public ProxyDetector getProxyDetector() { + throw new UnsupportedOperationException("Should not be called"); + } + }; + @Test public void overridesAuthority() { NameResolver nameResolverMock = mock(NameResolver.class); NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))) + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(nameResolverMock); String override = "override:5678"; NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, override); - NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), - Attributes.EMPTY); + NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER); assertNotNull(nameResolver); assertEquals(override, nameResolver.getServiceAuthority()); } @@ -51,23 +62,24 @@ public class OverrideAuthorityNameResolverTest { @Test public void wontWrapNull() { NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))).thenReturn(null); + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) + .thenReturn(null); NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, "override:5678"); assertEquals(null, - factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY)); + factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER)); } @Test public void forwardsNonOverridenCalls() { NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); NameResolver mockResolver = mock(NameResolver.class); - when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))) + when(wrappedFactory.newNameResolver(any(URI.class), any(NameResolver.Helper.class))) .thenReturn(mockResolver); NameResolver.Factory factory = new OverrideAuthorityNameResolverFactory(wrappedFactory, "override:5678"); NameResolver overrideResolver = - factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY); + factory.newNameResolver(URI.create("dns:///localhost:443"), HELPER); assertNotNull(overrideResolver); NameResolver.Listener listener = mock(NameResolver.Listener.class); diff --git a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java index b91a950036..c0d200ce01 100644 --- a/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java +++ b/cronet/src/main/java/io/grpc/cronet/CronetChannelBuilder.java @@ -23,9 +23,7 @@ import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; -import io.grpc.Attributes; import io.grpc.ExperimentalApi; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.ConnectionClientTransport; @@ -193,12 +191,6 @@ public final class CronetChannelBuilder extends transportTracerFactory.create()); } - @Override - protected Attributes getNameResolverParams() { - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, GrpcUtil.DEFAULT_PORT_SSL).build(); - } - @VisibleForTesting static class CronetTransportFactory implements ClientTransportFactory { private final ScheduledExecutorService timeoutService; diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 1294183a73..bcb91a3046 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -30,7 +30,6 @@ import io.grpc.EquivalentAddressGroup; import io.grpc.ExperimentalApi; import io.grpc.HttpConnectProxiedSocketAddress; import io.grpc.Internal; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; @@ -415,21 +414,16 @@ public final class NettyChannelBuilder @Override @CheckReturnValue - protected Attributes getNameResolverParams() { - int defaultPort; + protected int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: case PLAINTEXT_UPGRADE: - defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT; - break; + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; case TLS: - defaultPort = GrpcUtil.DEFAULT_PORT_SSL; - break; + return GrpcUtil.DEFAULT_PORT_SSL; default: throw new AssertionError(negotiationType + " not handled"); } - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build(); } void overrideAuthorityChecker(@Nullable OverrideAuthorityChecker authorityChecker) { diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 230f761595..0311976e94 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -23,10 +23,8 @@ import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import io.grpc.Attributes; import io.grpc.ExperimentalApi; import io.grpc.Internal; -import io.grpc.NameResolver; import io.grpc.internal.AbstractManagedChannelImplBuilder; import io.grpc.internal.AtomicBackoff; import io.grpc.internal.ClientTransportFactory; @@ -406,20 +404,15 @@ public class OkHttpChannelBuilder extends } @Override - protected Attributes getNameResolverParams() { - int defaultPort; + protected int getDefaultPort() { switch (negotiationType) { case PLAINTEXT: - defaultPort = GrpcUtil.DEFAULT_PORT_PLAINTEXT; - break; + return GrpcUtil.DEFAULT_PORT_PLAINTEXT; case TLS: - defaultPort = GrpcUtil.DEFAULT_PORT_SSL; - break; + return GrpcUtil.DEFAULT_PORT_SSL; default: throw new AssertionError(negotiationType + " not handled"); } - return Attributes.newBuilder() - .set(NameResolver.Factory.PARAMS_DEFAULT_PORT, defaultPort).build(); } @VisibleForTesting diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 993d5fe0b4..3f9104cf72 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -23,7 +23,6 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import com.squareup.okhttp.ConnectionSpec; -import io.grpc.NameResolver; import io.grpc.internal.ClientTransportFactory; import io.grpc.internal.FakeClock; import io.grpc.internal.GrpcUtil; @@ -120,8 +119,7 @@ public class OkHttpChannelBuilderTest { @Test public void usePlaintextDefaultPort() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("host", 1234).usePlaintext(); - assertEquals(GrpcUtil.DEFAULT_PORT_PLAINTEXT, - builder.getNameResolverParams().get(NameResolver.Factory.PARAMS_DEFAULT_PORT).intValue()); + assertEquals(GrpcUtil.DEFAULT_PORT_PLAINTEXT, builder.getDefaultPort()); } @Test