Add ability to filter out services from being bound to server factories #213
Conversation
| public DefaultGrpcServerFactory(String address, List<ServerBuilderCustomizer<T>> serverBuilderCustomizers, | ||
| KeyManagerFactory keyManager, TrustManagerFactory trustManager, ClientAuth clientAuth) { | ||
| KeyManagerFactory keyManager, TrustManagerFactory trustManager, ClientAuth clientAuth, | ||
| ServerServiceDefinitionFilter serviceFilter) { | ||
| this(address, serverBuilderCustomizers); | ||
| this.keyManager = keyManager; | ||
| this.trustManager = trustManager; | ||
| this.clientAuth = clientAuth; | ||
| this.serviceFilter = serviceFilter; | ||
| } |
There was a problem hiding this comment.
Should we maintain backward compatibility by adding another constructor? This question arose because of this comment: #203 (comment)
There was a problem hiding this comment.
I don't like the way that some of the new constructors use Optional and this one doesn't (not optional but @Nullable seems better in a mostly internal API). I'm neutral on whether to add another constructor (but mostly against).
There was a problem hiding this comment.
Yeh, we typically rarely use Optional for things besides returns types. For consistency I would say lets go w/ null and @Nullable as well.
| @@ -61,7 +63,7 @@ static class ShadedNettyServerFactoryConfiguration { | |||
| @Bean | |||
| ShadedNettyGrpcServerFactory shadedNettyGrpcServerFactory(GrpcServerProperties properties, | |||
| GrpcServiceDiscoverer grpcServicesDiscoverer, ServerBuilderCustomizers serverBuilderCustomizers, | |||
| SslBundles bundles) { | |||
| SslBundles bundles, Optional<ServerServiceDefinitionFilter> serviceFilter) { | |||
There was a problem hiding this comment.
I think that's okay? I haven't seen any other simpler options.
| * @param serverFactory the server factory in use. | ||
| * @return {@code true} if the service should be included; {@code false} otherwise. | ||
| */ | ||
| boolean filter(ServerServiceDefinition serviceDefinition, GrpcServerFactory serverFactory); |
There was a problem hiding this comment.
This seems wrong - there are no methods on GrpcServerFactory that we would expect to be useful to implementations of this interface, so it's confusing to pass it in. I don't know if removing it will cause more trouble than it is worth but I'd like to at least see that option.
There was a problem hiding this comment.
Actually, since there is a single filter bean allowed to be configured, the server factory can be used to check if the service should be added to the particular factory (e.g. in process does not want health checks).
therepanic
force-pushed
the
gh-207
branch
3 times, most recently
from
66ec313 to
7c9e2f3
Compare
|
Thanks for the review. I removed the second argument from |
therepanic
force-pushed
the
gh-207
branch
2 times, most recently
from
edbdf5d to
4136c04
Compare
…es (spring-projects#207) Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
There was a problem hiding this comment.
Thanks @therepanic - looking good. A couple of comments to address and then we will be good to go.
...main/java/org/springframework/grpc/autoconfigure/server/GrpcServerFactoryConfigurations.java
Show resolved
Hide resolved
...est/java/org/springframework/grpc/autoconfigure/server/GrpcServerAutoConfigurationTests.java
Show resolved
Hide resolved
...est/java/org/springframework/grpc/autoconfigure/server/GrpcServerAutoConfigurationTests.java
Show resolved
Hide resolved
…pcServerFactory` Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
There was a problem hiding this comment.
Thanks for the updates @therepanic ! Looks great. The only outstanding item is the passing of the server factory into the filter. I think it needs to be there but lets wait for @dsyer reply. I will take it from here though and update accordingly based on what we agree upon. Great work! It should be merged w/in the next 24hrs.
With this change, we will give users the ability to configure their filters for the server-side factory.
I created an interface similar to the ones we already have, and in
DefaultGrpcServerFactorywe use it to add/not add a service. Overall, nothing special, but I will leave a couple of comments on the points that bother me in some way.Closes: #207