search

spring-projects-experimental / spring-grpc

88 stars 19 forks source link

Add support for service specific server interceptors #50

Closed onobc closed 1 week ago

onobc commented 1 week ago

[!NOTE] @dsyer this is still a WIP (needs docs / tests / etc..) but wanted to get some eyes on it before continuing.

This commit introduces the @GrpcService annotation which can be used to associate one or more ServerInterceptor beans with a BindableService.

See #5

dsyer commented 1 week ago

The only thing I'm not 100% comfortable with is the new interface GrpcServiceConfigurer because it has an annotation type in its method signature. I prefer that I can call the interface with an object that I can easily create myself, and the annotation is only used as an implementation detail. I can see why you did it that way but I think it might need a tweak, possibly at the expense of yet another abstraction.

onobc commented 1 week ago

The only thing I'm not 100% comfortable with is the new interface GrpcServiceConfigurer because it has an annotation type in its method signature. I prefer that I can call the interface with an object that I can easily create myself, and the annotation is only used as an implementation detail. I can see why you did it that way but I think it might need a tweak, possibly at the expense of yet another abstraction.

I agree w/ this and I noticed the struggle when writing tests for this. Its a bit challenging to create an annotation instance on the fly and pass it in 😄 . I will find a way to pull the annotation out of the signature. Good feedback.

onobc commented 1 week ago

@dsyer I still need to handle a few tests and docs but the pieces are almost in place. Just ran out of time. You can see the wrapper I created to specify additional service info.

dsyer commented 1 week ago

Sounds good. The only question I had was if maybe some of the new abstractions (like the service info record) could live in core?

onobc commented 1 week ago

Sounds good. The only question I had was if maybe some of the new abstractions (like the service info record) could live in core?

That is going to happen in the next commit for sure. I too noticed this. Did not want to get too many things moving at once though.

onobc commented 1 week ago

~Next commit will shuffle some components into core and then we will be ready to remove from draft.~

On 2nd thought, I would like to do the move to core in a subsequent PR if that is ok w/ you @dsyer . I already have it queued up and close to completion but its more changes than I originally thought.