From 53f56a32f146a7730f8ca9894a8feaf0815451f3 Mon Sep 17 00:00:00 2001 From: Carl Mastrangelo Date: Wed, 6 Sep 2017 15:21:17 -0700 Subject: [PATCH] inprocess,core: add ManagedChannelBuilder and ServerBuilder factory hiders * inprocess,core: add ManagedChannelBuilder and ServerBuilder factory hiders Because the factory for Channels and Servers resides on the builder itself, it is easy for subclasses to accidentally inherit the factory. This causes confusion, because calling a static method on a specific class may result in a different class. This change adds hiding static factories to each builder, and a test to enforce that each subclass hides the factory. The test lives in the interop tests, because it has a classpath dependency on all the existing transports. Minor note: the test scans the classpath using a Beta Guava API. The test can be disabled if the API goes away. --- .../io/grpc/ForwardingChannelBuilder.java | 14 +++ .../inprocess/InProcessChannelBuilder.java | 14 +++ .../inprocess/InProcessServerBuilder.java | 7 ++ .../AbstractManagedChannelImplBuilder.java | 8 ++ .../internal/AbstractServerImplBuilder.java | 4 + .../io/grpc/ChannelAndServerBuilderTest.java | 95 +++++++++++++++++++ 6 files changed, 142 insertions(+) create mode 100644 interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java diff --git a/core/src/main/java/io/grpc/ForwardingChannelBuilder.java b/core/src/main/java/io/grpc/ForwardingChannelBuilder.java index 766d9d8d83..7b8151c868 100644 --- a/core/src/main/java/io/grpc/ForwardingChannelBuilder.java +++ b/core/src/main/java/io/grpc/ForwardingChannelBuilder.java @@ -36,6 +36,20 @@ public abstract class ForwardingChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException("Subclass failed to hide static factory"); + } + + /** + * This method serves to force sub classes to "hide" this static factory. + */ + public static ManagedChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException("Subclass failed to hide static factory"); + } + /** * Returns the delegated {@code ManagedChannelBuilder}. */ diff --git a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java index 937edf7983..e21c5305e7 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessChannelBuilder.java @@ -48,6 +48,20 @@ public final class InProcessChannelBuilder extends return new InProcessChannelBuilder(name); } + /** + * Always fails. Call {@link #forName} instead. + */ + public static InProcessChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException("call forName() instead"); + } + + /** + * Always fails. Call {@link #forName} instead. + */ + public static InProcessChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException("call forName() instead"); + } + private final String name; private InProcessChannelBuilder(String name) { diff --git a/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java b/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java index 8f5f8d5f15..1e25e0fa71 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java @@ -68,6 +68,13 @@ public final class InProcessServerBuilder return new InProcessServerBuilder(name); } + /** + * Always fails. Call {@link #forName} instead. + */ + public static InProcessServerBuilder forPort(int port) { + throw new UnsupportedOperationException("call forName() instead"); + } + private final String name; private InProcessServerBuilder(String name) { diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index ad41668a07..d21393633e 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -56,6 +56,14 @@ public abstract class AbstractManagedChannelImplBuilder > extends ManagedChannelBuilder { private static final String DIRECT_ADDRESS_SCHEME = "directaddress"; + public static ManagedChannelBuilder forAddress(String name, int port) { + throw new UnsupportedOperationException("Subclass failed to hide static factory"); + } + + public static ManagedChannelBuilder forTarget(String target) { + throw new UnsupportedOperationException("Subclass failed to hide static factory"); + } + /** * An idle timeout larger than this would disable idle mode. */ diff --git a/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java index c17e842fc2..79a4a40d45 100644 --- a/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractServerImplBuilder.java @@ -51,6 +51,10 @@ import javax.annotation.Nullable; public abstract class AbstractServerImplBuilder> extends ServerBuilder { + public static ServerBuilder forPort(int port) { + throw new UnsupportedOperationException("Subclass failed to hide static factory"); + } + private static final ObjectPool DEFAULT_EXECUTOR_POOL = SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR); private static final HandlerRegistry DEFAULT_FALLBACK_REGISTRY = new HandlerRegistry() { diff --git a/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java b/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java new file mode 100644 index 0000000000..ac830b9848 --- /dev/null +++ b/interop-testing/src/test/java/io/grpc/ChannelAndServerBuilderTest.java @@ -0,0 +1,95 @@ +/* + * Copyright 2017, gRPC Authors All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import com.google.common.reflect.ClassPath; +import com.google.common.reflect.ClassPath.ClassInfo; +import com.google.common.truth.Truth; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import org.junit.Assume; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests that Channel and Server builders properly hide the static constructors. + */ +@RunWith(Parameterized.class) +public class ChannelAndServerBuilderTest { + + @Parameter + public Class builderClass; + + /** + * Javadoc. + */ + @Parameters(name = "class={0}") + public static Collection params() throws Exception { + ClassLoader loader = ChannelAndServerBuilderTest.class.getClassLoader(); + List classes = new ArrayList(); + for (ClassInfo classInfo : ClassPath.from(loader).getTopLevelClassesRecursive("io.grpc")) { + Class clazz = Class.forName(classInfo.getName(), false /*initialize*/, loader); + if (ServerBuilder.class.isAssignableFrom(clazz) && clazz != ServerBuilder.class) { + classes.add(new Object[]{clazz}); + } else if (ManagedChannelBuilder.class.isAssignableFrom(clazz) + && clazz != ManagedChannelBuilder.class ) { + classes.add(new Object[]{clazz}); + } + } + Truth.assertWithMessage("Unable to find any builder classes").that(classes).isNotEmpty(); + return classes; + } + + @Test + public void serverBuilderHidesMethod_forPort() throws Exception { + Assume.assumeTrue(ServerBuilder.class.isAssignableFrom(builderClass)); + Method method = builderClass.getMethod("forPort", int.class); + + assertTrue(Modifier.isStatic(method.getModifiers())); + assertTrue(ServerBuilder.class.isAssignableFrom(method.getReturnType())); + assertSame(builderClass, method.getDeclaringClass()); + } + + @Test + public void channelBuilderHidesMethod_forAddress() throws Exception { + Assume.assumeTrue(ManagedChannelBuilder.class.isAssignableFrom(builderClass)); + Method method = builderClass.getMethod("forAddress", String.class, int.class); + + assertTrue(Modifier.isStatic(method.getModifiers())); + assertTrue(ManagedChannelBuilder.class.isAssignableFrom(method.getReturnType())); + assertSame(builderClass, method.getDeclaringClass()); + } + + @Test + public void channelBuilderHidesMethod_forTarget() throws Exception { + Assume.assumeTrue(ManagedChannelBuilder.class.isAssignableFrom(builderClass)); + Method method = builderClass.getMethod("forTarget", String.class); + + assertTrue(Modifier.isStatic(method.getModifiers())); + assertTrue(ManagedChannelBuilder.class.isAssignableFrom(method.getReturnType())); + assertSame(builderClass, method.getDeclaringClass()); + } +}