Initial server review

Author: philwebb

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

@@ -42,4 +42,6 @@
 @ConditionalOnProperty(prefix = "spring.grpc.server", name = "enabled", matchIfMissing = true)
 public @interface ConditionalOnGrpcServerEnabled {
 
+	// FIXME PW: rename? Not just enabled.
+
 }

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

@@ -47,4 +47,7 @@
 		matchIfMissing = true)
 public @interface ConditionalOnGrpcServletServer {
 
+	// FIXME PW: Can we merge ConditionalOnGrpcNativeServer,
+	// ConditionalOnGrpcServerEnabled and this with an enum attribute?
+
 }

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

@@ -36,7 +36,9 @@
  */
 class DefaultServerFactoryPropertyMapper<T extends ServerBuilder<T>> {
 
-	final GrpcServerProperties properties;
+	// FIXME PW: Might be able to simplify to reduce number of subclasses
+
+	final GrpcServerProperties properties; // FIXME PW private?
 
 	DefaultServerFactoryPropertyMapper(GrpcServerProperties properties) {
 		this.properties = properties;
@@ -58,7 +60,9 @@ void customizeServerBuilder(T serverBuilder) {
 	 * @param mapper the property mapper
 	 */
 	void customizeKeepAlive(T serverBuilder, PropertyMapper mapper) {
+		// FIXME PW: private?
 		GrpcServerProperties.KeepAlive keepAliveProps = this.properties.getKeepAlive();
+		// FIXME PW: nit - mapper -> map
 		mapper.from(keepAliveProps.getTime()).to(durationProperty(serverBuilder::keepAliveTime));
 		mapper.from(keepAliveProps.getTimeout()).to(durationProperty(serverBuilder::keepAliveTimeout));
 		mapper.from(keepAliveProps.getMaxIdle()).to(durationProperty(serverBuilder::maxConnectionIdle));
@@ -74,6 +78,7 @@ void customizeKeepAlive(T serverBuilder, PropertyMapper mapper) {
 	 * @param mapper the property mapper
 	 */
 	void customizeInboundLimits(T serverBuilder, PropertyMapper mapper) {
+		// FIXME PW: private?
 		mapper.from(this.properties.getMaxInboundMessageSize())
 			.asInt(DataSize::toBytes)
 			.to(serverBuilder::maxInboundMessageSize);

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

@@ -20,6 +20,9 @@
 
 public interface GrpcServerExecutorProvider {
 
+	// FIXME PW: Doc comment
+	// FIXME PW: Functional interface?
+
 	/**
 	 * Returns a {@link Executor} for the gRPC server, if it needs tio be customized.
 	 * @return the executor to use for the gRPC server

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

@@ -125,6 +125,7 @@ NettyGrpcServerFactory nettyGrpcServerFactory(GrpcServerProperties properties,
 			KeyManagerFactory keyManager = null;
 			TrustManagerFactory trustManager = null;
 			if (properties.getSsl().isEnabled()) {
+				// FIXME PW: looks similar to code above. Extract?
 				String bundleName = properties.getSsl().getBundle();
 				Assert.notNull(bundleName, () -> "SSL bundleName must not be null");
 				SslBundle bundle = bundles.getBundle(bundleName);

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

@@ -20,6 +20,9 @@
 
 public interface GrpcServerFactoryCustomizer {
 
+	// FIXME PW: Doc comment
+	// FIXME PW: Functional interface?
+
 	/**
 	 * Customize the given {@link GrpcServerFactory}.
 	 * @param serverFactory the server factory to customize

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

@@ -47,6 +47,8 @@
 @ConditionalOnProperty(name = "spring.grpc.server.observation.enabled", havingValue = "true", matchIfMissing = true)
 public final class GrpcServerObservationAutoConfiguration {
 
+	// FIXME PW: Observation direction (same as client)
+
 	@Bean
 	@Order(0)
 	@GlobalServerInterceptor

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

@@ -50,6 +50,8 @@ public class GrpcServerProperties {
 	 */
 	private int port = GrpcUtils.DEFAULT_PORT;
 
+	// FIXME PW: check that metadata works for defaults
+
 	/**
 	 * Maximum time to wait for the server to gracefully shutdown. When the value is
 	 * negative, the server waits forever. When the value is 0, the server will force
@@ -70,6 +72,8 @@ public class GrpcServerProperties {
 	@DataSizeUnit(DataUnit.BYTES)
 	private DataSize maxInboundMetadataSize = DataSize.ofBytes(8192);
 
+	// FIXME PW: nit drop default in doc comment.
+
 	private final Health health = new Health();
 
 	private final KeepAlive keepAlive = new KeepAlive();
@@ -82,14 +86,19 @@ public class GrpcServerProperties {
 	 */
 	@Nullable private String address;
 
+	// FIXME PW: private @Nullable (sweep code)
+
 	public String getAddress() {
+		// FIXME PW: make anemic
 		return (this.address != null) ? this.address : this.host + ":" + this.port;
 	}
 
 	public void setAddress(String address) {
 		this.address = address;
 	}
 
+	// FIXME PW: all privates to top
+
 	private final Ssl ssl = new Ssl();
 
 	public Ssl getSsl() {
@@ -101,20 +110,25 @@ public String getHost() {
 	}
 
 	public void setHost(String host) {
+		// FIXME PW: should probably check when accessing not settings
+		// FIXME PW: nit use Assert.state
 		if (this.address != null) {
 			throw new IllegalStateException("Cannot set host when address is already set");
 		}
 		this.host = host;
 	}
 
 	public int getPort() {
+		// FIXME PW: make anemic
 		if (this.address != null) {
 			return GrpcUtils.getPort(this.address);
 		}
 		return this.port;
 	}
 
 	public void setPort(int port) {
+		// FIXME PW: should probably check when accessing not settings
+		// FIXME PW: nit use Assert.state
 		if (this.address != null) {
 			throw new IllegalStateException("Cannot set port when address is already set");
 		}
@@ -180,19 +194,26 @@ public ActuatorAdapt getActuator() {
 
 	}
 
+	// FIXME PW: nit, rename to Actuator move under Health
+	// FIXME PW: need to review actuator integration
+
 	public static class ActuatorAdapt {
 
 		/**
 		 * Whether to adapt Actuator health indicators into gRPC health checks.
 		 */
 		private Boolean enabled = true;
 
+		// FIXME PW: Boolean or boolean?
+
 		/**
 		 * Whether to update the overall gRPC server health (the '' service) with the
 		 * aggregate status of the configured health indicators.
 		 */
 		private Boolean updateOverallHealth = true;
 
+		// FIXME PW: Boolean or boolean?
+
 		/**
 		 * How often to update the health status.
 		 */
@@ -273,6 +294,8 @@ public static class KeepAlive {
 		@DurationUnit(ChronoUnit.SECONDS)
 		private @Nullable Duration maxIdle = null;
 
+		// FIXME PW: nit, not '= null' (sweep code)
+
 		/**
 		 * Maximum time a connection may exist before being gracefully terminated (default
 		 * infinite).
@@ -298,6 +321,8 @@ public static class KeepAlive {
 		 */
 		private boolean permitWithoutCalls = false;
 
+		// FIXME PW: nit not = false
+
 		public @Nullable Duration getTime() {
 			return this.time;
 		}
@@ -364,6 +389,8 @@ public static class Ssl {
 		 */
 		private @Nullable Boolean enabled;
 
+		// FIXME PW: Check how enabled is used? Is enabled without bundle possible?
+
 		/**
 		 * Client authentication mode.
 		 */
@@ -381,10 +408,12 @@ public static class Ssl {
 		private boolean secure = true;
 
 		public boolean isEnabled() {
+			// FIXME PW: Make anemic
 			return (this.enabled != null) ? this.enabled : this.bundle != null;
 		}
 
 		public void copyDefaultsFrom(Ssl config) {
+			// FIXME PW: Check where used. Move out somewhere else?
 			if (this.enabled == null) {
 				this.enabled = config.enabled;
 			}
@@ -437,6 +466,8 @@ public static class Inprocess {
 		 */
 		private @Nullable Boolean exclusive;
 
+		// FIXME PW: Boolean or boolean?
+
 		public @Nullable String getName() {
 			return this.name;
 		}

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

@@ -34,6 +34,8 @@
  */
 public class ServerBuilderCustomizers {
 
+	// FIXME PW: Package private?
+
 	private final List<ServerBuilderCustomizer<?>> customizers;
 
 	ServerBuilderCustomizers(List<? extends ServerBuilderCustomizer<?>> customizers) {

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

@@ -26,6 +26,9 @@
 
 public class ServletEnvironmentPostProcessor implements EnvironmentPostProcessor {
 
+	// FIXME PW: Package private
+	// FIXME PW: Unusual EPP. Not sure if there's a better way?
+
 	private static final boolean SERVLET_AVAILABLE = ClassUtils.isPresent("io.grpc.servlet.jakarta.GrpcServlet", null);
 
 	@Override