From 063d108931bc55a3846366ccffc6563d3fe2ad87 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Wed, 26 Apr 2017 08:09:40 -0700 Subject: [PATCH 01/12] ManagedChannelImpl: use authorityOverride for target if present --- .../AbstractManagedChannelImplBuilder.java | 3 ++- gradle/wrapper/gradle-wrapper.jar | Bin 54212 -> 54212 bytes .../grpc/netty/NettyChannelBuilderTest.java | 10 ++++++++++ .../grpc/okhttp/OkHttpChannelBuilderTest.java | 10 ++++++++++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 5f4b4431851..b3fba355045 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -35,6 +35,7 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import com.google.instrumentation.stats.Stats; @@ -330,7 +331,7 @@ public ManagedChannel build() { } return new ManagedChannelImpl( - target, + MoreObjects.firstNonNull(authorityOverride, target), // TODO(carl-mastrangelo): Allow clients to pass this in new ExponentialBackoffPolicy.Provider(), nameResolverFactory, diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index 459344875602759f71ff759b0850903f98a9a27b..c8fea932375fd4e4beab5dc6351f7b1712556766 100644 GIT binary patch delta 26 gcmX@IocYLd<_)ojnD;K5xjEsGh#-hDx%{#(0Io_5&Hw-a delta 26 gcmX@IocYLd<_)ojm@9(XHYXes5d<+NmtXb;0H4YWb^rhX diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 8109c56921f..ff7739803ae 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -35,6 +35,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; +import io.grpc.ManagedChannel; import io.grpc.netty.InternalNettyChannelBuilder.OverrideAuthorityChecker; import io.grpc.netty.ProtocolNegotiators.TlsNegotiator; import io.netty.handler.ssl.SslContext; @@ -54,6 +55,15 @@ public class NettyChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); private final SslContext noSslContext = null; + @Test + public void overrideAuthorityIsReadable() { + NettyChannelBuilder builder = new NettyChannelBuilder("original", 1234); + String override = "override:5678"; + builder.overrideAuthority(override); + ManagedChannel channel = builder.build(); + assertEquals(override, channel.authority()); + } + @Test public void overrideAllowsInvalidAuthority() { NettyChannelBuilder builder = new NettyChannelBuilder(new SocketAddress(){}); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 011f5071e99..e02bbcb0d29 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -36,6 +36,7 @@ import static org.junit.Assert.assertNull; import com.squareup.okhttp.ConnectionSpec; +import io.grpc.ManagedChannel; import io.grpc.NameResolver; import io.grpc.internal.GrpcUtil; import java.net.InetSocketAddress; @@ -53,6 +54,15 @@ public class OkHttpChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); + @Test + public void overrideAuthorityIsReadable() { + OkHttpChannelBuilder builder = new OkHttpChannelBuilder("original", 1234); + String override = "override:5678"; + builder.overrideAuthority(override); + ManagedChannel channel = builder.build(); + assertEquals(override, channel.authority()); + } + @Test public void overrideAllowsInvalidAuthority() { OkHttpChannelBuilder builder = new OkHttpChannelBuilder("good", 1234) { From f26f39bf6cd2a9fdcc92eb445ac9a92035baa1af Mon Sep 17 00:00:00 2001 From: spencerfang Date: Wed, 26 Apr 2017 17:47:39 -0700 Subject: [PATCH 02/12] test all constructors --- .../AbstractManagedChannelImplBuilder.java | 46 +++++++++++++++++++ .../grpc/netty/NettyChannelBuilderTest.java | 20 +++++++- .../grpc/okhttp/OkHttpChannelBuilderTest.java | 14 +++++- 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index b3fba355045..394202ed6a7 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -309,6 +309,10 @@ public ManagedChannel build() { // getResource(), then this shouldn't be a problem unless called on the UI thread. nameResolverFactory = NameResolverProvider.asFactory(); } + if (authorityOverride != null) { + nameResolverFactory = + new OverridableAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); + } List effectiveInterceptors = new ArrayList(this.interceptors); @@ -441,6 +445,48 @@ public String getDefaultScheme() { } } + /** + * A wrapper class that allows override the authority of a NameResolver, while preserving all + * other functionality. + */ + private static class OverridableAuthorityNameResolverFactory extends NameResolver.Factory { + final NameResolver.Factory delegate; + final String authorityOverride; + + OverridableAuthorityNameResolverFactory(NameResolver.Factory delegate, + String authorityOverride) { + this.delegate = delegate; + this.authorityOverride = authorityOverride; + } + + @Nullable + @Override + public NameResolver newNameResolver(URI targetUri, Attributes params) { + final NameResolver resolver = delegate.newNameResolver(targetUri, params); + return new NameResolver() { + @Override + public String getServiceAuthority() { + return authorityOverride; + } + + @Override + public void start(Listener listener) { + resolver.start(listener); + } + + @Override + public void shutdown() { + resolver.shutdown(); + } + }; + } + + @Override + public String getDefaultScheme() { + return delegate.getDefaultScheme(); + } + } + /** * Returns the correctly typed version of the builder. */ diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index ff7739803ae..9a800b7810f 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -56,8 +56,24 @@ public class NettyChannelBuilderTest { private final SslContext noSslContext = null; @Test - public void overrideAuthorityIsReadable() { - NettyChannelBuilder builder = new NettyChannelBuilder("original", 1234); + public void overrideAuthorityIsReadableForAddress() { + NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); + overrideAuthorityIsReadableHelper(builder); + } + + @Test + public void overrideAuthorityIsReadableForTarget() { + NettyChannelBuilder builder = NettyChannelBuilder.forTarget("original:1234"); + overrideAuthorityIsReadableHelper(builder); + } + + @Test + public void overrideAuthorityIsReadableForSocketAddress() { + NettyChannelBuilder builder = NettyChannelBuilder.forAddress(new SocketAddress(){}); + overrideAuthorityIsReadableHelper(builder); + } + + public void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder) { String override = "override:5678"; builder.overrideAuthority(override); ManagedChannel channel = builder.build(); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index e02bbcb0d29..3a25cad3582 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -55,8 +55,18 @@ public class OkHttpChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); @Test - public void overrideAuthorityIsReadable() { - OkHttpChannelBuilder builder = new OkHttpChannelBuilder("original", 1234); + public void overrideAuthorityIsReadableForAddress() { + OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("original", 1234); + overrideAuthorityIsReadableHelper(builder); + } + + @Test + public void overrideAuthorityIsReadableForTarget() { + OkHttpChannelBuilder builder = OkHttpChannelBuilder.forTarget("original:1234"); + overrideAuthorityIsReadableHelper(builder); + } + + private void overrideAuthorityIsReadableHelper(OkHttpChannelBuilder builder) { String override = "override:5678"; builder.overrideAuthority(override); ManagedChannel channel = builder.build(); From 0034ca2d561bbf505c5fc3395dc6e71f0a748581 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 08:08:46 -0700 Subject: [PATCH 03/12] whitespace --- .../test/java/io/grpc/netty/NettyChannelBuilderTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 9a800b7810f..020d409be37 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -57,19 +57,19 @@ public class NettyChannelBuilderTest { @Test public void overrideAuthorityIsReadableForAddress() { - NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); + NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); overrideAuthorityIsReadableHelper(builder); } @Test public void overrideAuthorityIsReadableForTarget() { - NettyChannelBuilder builder = NettyChannelBuilder.forTarget("original:1234"); + NettyChannelBuilder builder = NettyChannelBuilder.forTarget("original:1234"); overrideAuthorityIsReadableHelper(builder); } @Test public void overrideAuthorityIsReadableForSocketAddress() { - NettyChannelBuilder builder = NettyChannelBuilder.forAddress(new SocketAddress(){}); + NettyChannelBuilder builder = NettyChannelBuilder.forAddress(new SocketAddress(){}); overrideAuthorityIsReadableHelper(builder); } From 701003f074afff5c91c138761b0aad45a8cb617e Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 08:16:26 -0700 Subject: [PATCH 04/12] refactor tests --- .../AbstractManagedChannelImplBuilder.java | 2 +- .../io/grpc/netty/NettyChannelBuilderTest.java | 14 +++++++------- .../io/grpc/okhttp/OkHttpChannelBuilderTest.java | 12 ++++++------ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 394202ed6a7..71ca00f951d 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -335,7 +335,7 @@ public ManagedChannel build() { } return new ManagedChannelImpl( - MoreObjects.firstNonNull(authorityOverride, target), + target, // TODO(carl-mastrangelo): Allow clients to pass this in new ExponentialBackoffPolicy.Provider(), nameResolverFactory, diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 020d409be37..9a4d26b7361 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -58,26 +58,26 @@ public class NettyChannelBuilderTest { @Test public void overrideAuthorityIsReadableForAddress() { NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); - overrideAuthorityIsReadableHelper(builder); + overrideAuthorityIsReadableHelper(builder, "override:5678"); } @Test public void overrideAuthorityIsReadableForTarget() { NettyChannelBuilder builder = NettyChannelBuilder.forTarget("original:1234"); - overrideAuthorityIsReadableHelper(builder); + overrideAuthorityIsReadableHelper(builder, "override:5678"); } @Test public void overrideAuthorityIsReadableForSocketAddress() { NettyChannelBuilder builder = NettyChannelBuilder.forAddress(new SocketAddress(){}); - overrideAuthorityIsReadableHelper(builder); + overrideAuthorityIsReadableHelper(builder, "override:5678"); } - public void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder) { - String override = "override:5678"; - builder.overrideAuthority(override); + public void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, + String overrideAuthority) { + builder.overrideAuthority(overrideAuthority); ManagedChannel channel = builder.build(); - assertEquals(override, channel.authority()); + assertEquals(overrideAuthority, channel.authority()); } @Test diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 3a25cad3582..7f95ec7787e 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -57,20 +57,20 @@ public class OkHttpChannelBuilderTest { @Test public void overrideAuthorityIsReadableForAddress() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("original", 1234); - overrideAuthorityIsReadableHelper(builder); + overrideAuthorityIsReadableHelper(builder, "override:5678"); } @Test public void overrideAuthorityIsReadableForTarget() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forTarget("original:1234"); - overrideAuthorityIsReadableHelper(builder); + overrideAuthorityIsReadableHelper(builder, "override:5678"); } - private void overrideAuthorityIsReadableHelper(OkHttpChannelBuilder builder) { - String override = "override:5678"; - builder.overrideAuthority(override); + private void overrideAuthorityIsReadableHelper(OkHttpChannelBuilder builder, + String overrideAuthority) { + builder.overrideAuthority(overrideAuthority); ManagedChannel channel = builder.build(); - assertEquals(override, channel.authority()); + assertEquals(overrideAuthority, channel.authority()); } @Test From 597aa279d5003ef2e2a1cb32ee32756fd041b039 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 08:18:17 -0700 Subject: [PATCH 05/12] remove unused import --- .../java/io/grpc/internal/AbstractManagedChannelImplBuilder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 71ca00f951d..cd1e57ebbec 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -35,7 +35,6 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import com.google.instrumentation.stats.Stats; From c88ce8d41fc625400ca4b57546e4783cf5ba7b18 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 08:23:33 -0700 Subject: [PATCH 06/12] better commets --- .../AbstractManagedChannelImplBuilder.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index cd1e57ebbec..bad4c410d1b 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -310,7 +310,7 @@ public ManagedChannel build() { } if (authorityOverride != null) { nameResolverFactory = - new OverridableAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); + new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride); } List effectiveInterceptors = @@ -445,14 +445,20 @@ public String getDefaultScheme() { } /** - * A wrapper class that allows override the authority of a NameResolver, while preserving all - * other functionality. + * A wrapper class that overrides the authority of a NameResolver, while preserving all other + * functionality. */ - private static class OverridableAuthorityNameResolverFactory extends NameResolver.Factory { + private static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { final NameResolver.Factory delegate; final String authorityOverride; - OverridableAuthorityNameResolverFactory(NameResolver.Factory delegate, + /** + * Constructor for the {@link NameResolver.Factory} + * @param delegate The actual underlying factory that will produce the a {@link NameResolver} + * @param authorityOverride The authority that will be returned by {@link + * NameResolver#getServiceAuthority()} + */ + OverrideAuthorityNameResolverFactory(NameResolver.Factory delegate, String authorityOverride) { this.delegate = delegate; this.authorityOverride = authorityOverride; From d7542fdf63a10f667bba269f1433a620a6715f43 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 09:33:36 -0700 Subject: [PATCH 07/12] add test case for non override --- .../test/java/io/grpc/netty/NettyChannelBuilderTest.java | 6 ++++++ .../java/io/grpc/okhttp/OkHttpChannelBuilderTest.java | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 9a4d26b7361..917e98b9e4f 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -55,6 +55,12 @@ public class NettyChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); private final SslContext noSslContext = null; + @Test + public void authorityIsReadable() { + NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); + assertEquals("original:1234", builder.build().authority()); + } + @Test public void overrideAuthorityIsReadableForAddress() { NettyChannelBuilder builder = NettyChannelBuilder.forAddress("original", 1234); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 7f95ec7787e..5bbabe4248e 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -54,6 +54,12 @@ public class OkHttpChannelBuilderTest { @Rule public final ExpectedException thrown = ExpectedException.none(); + @Test + public void authorityIsReadable() { + OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("original", 1234); + assertEquals("original:1234", builder.build().authority()); + } + @Test public void overrideAuthorityIsReadableForAddress() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("original", 1234); @@ -69,8 +75,7 @@ public void overrideAuthorityIsReadableForTarget() { private void overrideAuthorityIsReadableHelper(OkHttpChannelBuilder builder, String overrideAuthority) { builder.overrideAuthority(overrideAuthority); - ManagedChannel channel = builder.build(); - assertEquals(overrideAuthority, channel.authority()); + assertEquals(overrideAuthority, builder.build().authority()); } @Test From afc43fd291cf9b2cbb6906f80a127352adf90742 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 10:22:16 -0700 Subject: [PATCH 08/12] fix bug where we wrap nulls, thereby hiding the special error return value --- .../AbstractManagedChannelImplBuilder.java | 7 +++- ...AbstractManagedChannelImplBuilderTest.java | 34 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index bad4c410d1b..37ffea38878 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -448,7 +448,8 @@ public String getDefaultScheme() { * A wrapper class that overrides the authority of a NameResolver, while preserving all other * functionality. */ - private static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { + @VisibleForTesting + protected static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { final NameResolver.Factory delegate; final String authorityOverride; @@ -468,6 +469,10 @@ private static class OverrideAuthorityNameResolverFactory extends NameResolver.F @Override public NameResolver newNameResolver(URI targetUri, Attributes params) { final NameResolver resolver = delegate.newNameResolver(targetUri, params); + // Do not wrap null values. We do not want to impede error signaling. + if (resolver == null) { + return null; + } return new NameResolver() { @Override public String getServiceAuthority() { diff --git a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java index 3413e778155..40e00f49f02 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java @@ -32,11 +32,18 @@ package io.grpc.internal; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.net.InetSocketAddress; import java.net.URI; import java.util.concurrent.TimeUnit; + +import io.grpc.Attributes; +import io.grpc.NameResolver; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -98,4 +105,31 @@ public Builder usePlaintext(boolean value) { builder.idleTimeout(30, TimeUnit.SECONDS); assertEquals(TimeUnit.SECONDS.toMillis(30), builder.getIdleTimeoutMillis()); } + + @Test + public void overrideAuthorityNameResolverWrapsDelegateTest() { + NameResolver nameResolverMock = mock(NameResolver.class); + NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); + when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))) + .thenReturn(nameResolverMock); + String override = "override:5678"; + NameResolver.Factory factory = + new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, + override); + NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), + Attributes.EMPTY); + assertNotNull(nameResolver); + assertEquals(override, nameResolver.getServiceAuthority()); + } + + @Test + public void overrideAuthorityNameResolverWontWrapNullTest() { + NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); + when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))).thenReturn(null); + NameResolver.Factory factory = + new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, + "override:5678"); + assertEquals(null, + factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY)); + } } From 56fd8161fdd77254f49bac1b291427eb88ba6508 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 12:50:32 -0700 Subject: [PATCH 09/12] fix cs --- .../AbstractManagedChannelImplBuilderTest.java | 17 +++++++++-------- .../io/grpc/netty/NettyChannelBuilderTest.java | 5 +++++ .../grpc/okhttp/OkHttpChannelBuilderTest.java | 1 - 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java index 40e00f49f02..ad6d7d3bc41 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java @@ -38,12 +38,13 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import io.grpc.Attributes; +import io.grpc.NameResolver; + import java.net.InetSocketAddress; import java.net.URI; import java.util.concurrent.TimeUnit; -import io.grpc.Attributes; -import io.grpc.NameResolver; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -114,10 +115,10 @@ public void overrideAuthorityNameResolverWrapsDelegateTest() { .thenReturn(nameResolverMock); String override = "override:5678"; NameResolver.Factory factory = - new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, - override); + new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, + override); NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"), - Attributes.EMPTY); + Attributes.EMPTY); assertNotNull(nameResolver); assertEquals(override, nameResolver.getServiceAuthority()); } @@ -127,9 +128,9 @@ public void overrideAuthorityNameResolverWontWrapNullTest() { NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class); when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))).thenReturn(null); NameResolver.Factory factory = - new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, - "override:5678"); + new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory, + "override:5678"); assertEquals(null, - factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY)); + factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY)); } } diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 917e98b9e4f..3d428442566 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -79,6 +79,11 @@ public void overrideAuthorityIsReadableForSocketAddress() { overrideAuthorityIsReadableHelper(builder, "override:5678"); } + /** + * A helper method for setting an authority override on a factory and testing the result. + * @param builder The builder object whose authority we will override + * @param overrideAuthority The override value + */ public void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, String overrideAuthority) { builder.overrideAuthority(overrideAuthority); diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java index 5bbabe4248e..f1d2ff73439 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpChannelBuilderTest.java @@ -36,7 +36,6 @@ import static org.junit.Assert.assertNull; import com.squareup.okhttp.ConnectionSpec; -import io.grpc.ManagedChannel; import io.grpc.NameResolver; import io.grpc.internal.GrpcUtil; import java.net.InetSocketAddress; From 9556761c1e4cde5a750a7d91bb73a90074643110 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 12:58:24 -0700 Subject: [PATCH 10/12] small fix: NettyChannelBuilderTest.overrideAuthorityIsReadableHelper should be private --- .../test/java/io/grpc/netty/NettyChannelBuilderTest.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 3d428442566..9f03113dbe5 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -79,12 +79,7 @@ public void overrideAuthorityIsReadableForSocketAddress() { overrideAuthorityIsReadableHelper(builder, "override:5678"); } - /** - * A helper method for setting an authority override on a factory and testing the result. - * @param builder The builder object whose authority we will override - * @param overrideAuthority The override value - */ - public void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, + private void overrideAuthorityIsReadableHelper(NettyChannelBuilder builder, String overrideAuthority) { builder.overrideAuthority(overrideAuthority); ManagedChannel channel = builder.build(); From 0e3fc79063c8ee399e0bc5044314808e12181a2d Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 13:35:31 -0700 Subject: [PATCH 11/12] changes for review comments --- .../AbstractManagedChannelImplBuilder.java | 34 +++---------------- ...AbstractManagedChannelImplBuilderTest.java | 2 -- 2 files changed, 4 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index 37ffea38878..35e19503618 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -297,10 +297,6 @@ public void setEnableTracing(boolean enabled) { @Override public ManagedChannel build() { ClientTransportFactory transportFactory = buildTransportFactory(); - if (authorityOverride != null) { - transportFactory = new AuthorityOverridingTransportFactory( - transportFactory, authorityOverride); - } NameResolver.Factory nameResolverFactory = this.nameResolverFactory; if (nameResolverFactory == null) { // Avoid loading the provider unless necessary, as a way to workaround a possibly-costly @@ -386,29 +382,6 @@ public Executor returnObject(Object returned) { } } - private static class AuthorityOverridingTransportFactory implements ClientTransportFactory { - final ClientTransportFactory factory; - final String authorityOverride; - - AuthorityOverridingTransportFactory( - ClientTransportFactory factory, String authorityOverride) { - this.factory = Preconditions.checkNotNull(factory, "factory should not be null"); - this.authorityOverride = Preconditions.checkNotNull( - authorityOverride, "authorityOverride should not be null"); - } - - @Override - public ConnectionClientTransport newClientTransport(SocketAddress serverAddress, - String authority, @Nullable String userAgent) { - return factory.newClientTransport(serverAddress, authorityOverride, userAgent); - } - - @Override - public void close() { - factory.close(); - } - } - private static class DirectAddressNameResolverFactory extends NameResolver.Factory { final SocketAddress address; final String authority; @@ -449,12 +422,13 @@ public String getDefaultScheme() { * functionality. */ @VisibleForTesting - protected static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { - final NameResolver.Factory delegate; - final String authorityOverride; + static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory { + private final NameResolver.Factory delegate; + private final String authorityOverride; /** * Constructor for the {@link NameResolver.Factory} + * * @param delegate The actual underlying factory that will produce the a {@link NameResolver} * @param authorityOverride The authority that will be returned by {@link * NameResolver#getServiceAuthority()} diff --git a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java index ad6d7d3bc41..c4df0bf5206 100644 --- a/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java @@ -40,11 +40,9 @@ import io.grpc.Attributes; import io.grpc.NameResolver; - import java.net.InetSocketAddress; import java.net.URI; import java.util.concurrent.TimeUnit; - import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; From d6f0926e203a353203011a3e6030b09d2e989ac0 Mon Sep 17 00:00:00 2001 From: spencerfang Date: Thu, 27 Apr 2017 13:42:47 -0700 Subject: [PATCH 12/12] reset gradle-wrapper.jar --- gradle/wrapper/gradle-wrapper.jar | Bin 54212 -> 54212 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/gradle/wrapper/gradle-wrapper.jar b/gradle/wrapper/gradle-wrapper.jar index c8fea932375fd4e4beab5dc6351f7b1712556766..459344875602759f71ff759b0850903f98a9a27b 100644 GIT binary patch delta 26 gcmX@IocYLd<_)ojm@9(XHYXes5d<+NmtXb;0H4YWb^rhX delta 26 gcmX@IocYLd<_)ojnD;K5xjEsGh#-hDx%{#(0Io_5&Hw-a