Incorporate feedback from server code review

Addresses the following:

- nits
- visibility (move to package protected etc..)
- javadocs
- make server properties anemic

Does not cover the following points:

- rename / restructure the ConditionalOn... for server
- observation API verify/check on direction
- security concerns (need more review but should be good the way it is)

Signed-off-by: onobc <[email protected]>

Author: onobc

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/DefaultServerFactoryPropertyMapper.java

@@ -36,7 +36,7 @@
  */
 class DefaultServerFactoryPropertyMapper<T extends ServerBuilder<T>> {
 
-	final GrpcServerProperties properties;
+	private final GrpcServerProperties properties;
 
 	DefaultServerFactoryPropertyMapper(GrpcServerProperties properties) {
 		this.properties = properties;
@@ -47,37 +47,37 @@ class DefaultServerFactoryPropertyMapper<T extends ServerBuilder<T>> {
 	 * @param serverBuilder the builder
 	 */
 	void customizeServerBuilder(T serverBuilder) {
-		PropertyMapper mapper = PropertyMapper.get();
-		customizeKeepAlive(serverBuilder, mapper);
-		customizeInboundLimits(serverBuilder, mapper);
+		PropertyMapper map = PropertyMapper.get();
+		customizeKeepAlive(serverBuilder, map);
+		customizeInboundLimits(serverBuilder, map);
 	}
 
 	/**
 	 * Map the keep-alive properties to the server factory's server builder.
 	 * @param serverBuilder the builder
-	 * @param mapper the property mapper
+	 * @param map the property mapper
 	 */
-	void customizeKeepAlive(T serverBuilder, PropertyMapper mapper) {
+	void customizeKeepAlive(T serverBuilder, PropertyMapper map) {
 		GrpcServerProperties.KeepAlive keepAliveProps = this.properties.getKeepAlive();
-		mapper.from(keepAliveProps.getTime()).to(durationProperty(serverBuilder::keepAliveTime));
-		mapper.from(keepAliveProps.getTimeout()).to(durationProperty(serverBuilder::keepAliveTimeout));
-		mapper.from(keepAliveProps.getMaxIdle()).to(durationProperty(serverBuilder::maxConnectionIdle));
-		mapper.from(keepAliveProps.getMaxAge()).to(durationProperty(serverBuilder::maxConnectionAge));
-		mapper.from(keepAliveProps.getMaxAgeGrace()).to(durationProperty(serverBuilder::maxConnectionAgeGrace));
-		mapper.from(keepAliveProps.getPermitTime()).to(durationProperty(serverBuilder::permitKeepAliveTime));
-		mapper.from(keepAliveProps.isPermitWithoutCalls()).to(serverBuilder::permitKeepAliveWithoutCalls);
+		map.from(keepAliveProps.getTime()).to(durationProperty(serverBuilder::keepAliveTime));
+		map.from(keepAliveProps.getTimeout()).to(durationProperty(serverBuilder::keepAliveTimeout));
+		map.from(keepAliveProps.getMaxIdle()).to(durationProperty(serverBuilder::maxConnectionIdle));
+		map.from(keepAliveProps.getMaxAge()).to(durationProperty(serverBuilder::maxConnectionAge));
+		map.from(keepAliveProps.getMaxAgeGrace()).to(durationProperty(serverBuilder::maxConnectionAgeGrace));
+		map.from(keepAliveProps.getPermitTime()).to(durationProperty(serverBuilder::permitKeepAliveTime));
+		map.from(keepAliveProps.isPermitWithoutCalls()).to(serverBuilder::permitKeepAliveWithoutCalls);
 	}
 
 	/**
 	 * Map the inbound limits properties to the server factory's server builder.
 	 * @param serverBuilder the builder
-	 * @param mapper the property mapper
+	 * @param map the property mapper
 	 */
-	void customizeInboundLimits(T serverBuilder, PropertyMapper mapper) {
-		mapper.from(this.properties.getMaxInboundMessageSize())
+	void customizeInboundLimits(T serverBuilder, PropertyMapper map) {
+		map.from(this.properties.getMaxInboundMessageSize())
 			.asInt(DataSize::toBytes)
 			.to(serverBuilder::maxInboundMessageSize);
-		mapper.from(this.properties.getMaxInboundMetadataSize())
+		map.from(this.properties.getMaxInboundMetadataSize())
 			.asInt(DataSize::toBytes)
 			.to(serverBuilder::maxInboundMetadataSize);
 	}

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/codec/GrpcCodecConfiguration.java renamed to module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/GrpcCodecConfiguration.java

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package org.springframework.boot.grpc.server.autoconfigure.codec;
+package org.springframework.boot.grpc.server.autoconfigure;
 
 import io.grpc.Codec;
 import io.grpc.Compressor;
@@ -29,14 +29,13 @@
 import org.springframework.context.annotation.Configuration;
 
 /**
- * The configuration that contains all codec related beans for clients/servers.
+ * The configuration that contains all codec related beans for gRPC servers.
  *
  * @author Andrei Lisa
- * @since 4.0.0
  */
 @Configuration(proxyBeanMethods = false)
 @ConditionalOnClass(Codec.class)
-public class GrpcCodecConfiguration {
+class GrpcCodecConfiguration {
 
 	@Bean
 	@ConditionalOnMissingBean

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/GrpcServerAutoConfiguration.java

@@ -28,7 +28,6 @@
 import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
-import org.springframework.boot.grpc.server.autoconfigure.codec.GrpcCodecConfiguration;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/GrpcServerExecutorProvider.java

@@ -18,10 +18,17 @@
 
 import java.util.concurrent.Executor;
 
+/**
+ * Strategy interface to determine the {@link Executor} to use for the gRPC server.
+ *
+ * @author Chris Bono
+ * @since 4.0.0
+ */
+@FunctionalInterface
 public interface GrpcServerExecutorProvider {
 
 	/**
-	 * Returns a {@link Executor} for the gRPC server, if it needs tio be customized.
+	 * Returns a {@link Executor} for the gRPC server, if it needs to be customized.
 	 * @return the executor to use for the gRPC server
 	 */
 	Executor getExecutor();

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/GrpcServerFactoryConfigurations.java

@@ -78,15 +78,15 @@ ShadedNettyGrpcServerFactory shadedNettyGrpcServerFactory(GrpcServerProperties p
 				.of(mapper::customizeServerBuilder, serverBuilderCustomizers::customize);
 			KeyManagerFactory keyManager = null;
 			TrustManagerFactory trustManager = null;
-			if (properties.getSsl().isEnabled()) {
+			if (properties.getSsl().determineEnabled()) {
 				String bundleName = properties.getSsl().getBundle();
 				Assert.notNull(bundleName, () -> "SSL bundleName must not be null");
 				SslBundle bundle = bundles.getBundle(bundleName);
 				keyManager = bundle.getManagers().getKeyManagerFactory();
 				trustManager = properties.getSsl().isSecure() ? bundle.getManagers().getTrustManagerFactory()
 						: io.grpc.netty.shaded.io.netty.handler.ssl.util.InsecureTrustManagerFactory.INSTANCE;
 			}
-			ShadedNettyGrpcServerFactory factory = new ShadedNettyGrpcServerFactory(properties.getAddress(),
+			ShadedNettyGrpcServerFactory factory = new ShadedNettyGrpcServerFactory(properties.determineAddress(),
 					builderCustomizers, keyManager, trustManager, properties.getSsl().getClientAuth());
 			applyServerFactoryCustomizers(customizers, factory);
 			serviceDiscoverer.findServices()
@@ -124,16 +124,16 @@ NettyGrpcServerFactory nettyGrpcServerFactory(GrpcServerProperties properties,
 				.of(mapper::customizeServerBuilder, serverBuilderCustomizers::customize);
 			KeyManagerFactory keyManager = null;
 			TrustManagerFactory trustManager = null;
-			if (properties.getSsl().isEnabled()) {
+			if (properties.getSsl().determineEnabled()) {
 				String bundleName = properties.getSsl().getBundle();
 				Assert.notNull(bundleName, () -> "SSL bundleName must not be null");
 				SslBundle bundle = bundles.getBundle(bundleName);
 				keyManager = bundle.getManagers().getKeyManagerFactory();
 				trustManager = properties.getSsl().isSecure() ? bundle.getManagers().getTrustManagerFactory()
 						: InsecureTrustManagerFactory.INSTANCE;
 			}
-			NettyGrpcServerFactory factory = new NettyGrpcServerFactory(properties.getAddress(), builderCustomizers,
-					keyManager, trustManager, properties.getSsl().getClientAuth());
+			NettyGrpcServerFactory factory = new NettyGrpcServerFactory(properties.determineAddress(),
+					builderCustomizers, keyManager, trustManager, properties.getSsl().getClientAuth());
 			applyServerFactoryCustomizers(customizers, factory);
 			serviceDiscoverer.findServices()
 				.stream()

module/spring-boot-grpc-server/src/main/java/org/springframework/boot/grpc/server/autoconfigure/GrpcServerFactoryCustomizer.java

@@ -18,6 +18,14 @@
 
 import org.springframework.grpc.server.GrpcServerFactory;
 
+/**
+ * Callback interface that can be implemented by beans wishing to customize the
+ * {@link GrpcServerFactory server factory} before it is fully initialized.
+ *
+ * @author Chris Bono
+ * @since 4.0.0
+ */
+@FunctionalInterface
 public interface GrpcServerFactoryCustomizer {
 
 	/**