Skip to content

Conversation

@onobc
Copy link
Contributor

@onobc onobc commented Nov 6, 2024

This commit builds upon the previously added service discoverer by adding support for finding all server interceptor beans that are marked w/ the newly added @GlobalServerInterceptor annotation.

Resolves #4

@onobc onobc requested a review from dsyer November 6, 2024 06:46
@onobc onobc added the enhancement New feature or request label Nov 6, 2024
@onobc onobc added this to the 0.2.0 milestone Nov 6, 2024
@onobc onobc force-pushed the GH-4-global-server-interceptors-part2 branch from f5cfcc8 to 47ba60a Compare November 6, 2024 06:49
@dsyer
Copy link
Member

dsyer commented Nov 6, 2024

I'm confused. Maybe this is just a stepping stone? But the docs don't seem to make sense for the non-global case - ServerBuilder.intercept() is a method to add a global interceptor and you use it in the example of a non-global, unless I'm reading it wrong somehow. I think you need to create and wrap a service definition with ServerInterceptors.

Also, I know Spring Integration has global interceptors, so we probably copied the pattern from there? But there is also the pattern of a "registration bean" with an enabled flag in Spring Boot for servlets and filters. That would work here too, and then we wouldn't need an annotation, or the annotation could be a convenient way of marking the interceptor registration as "enabled".

@onobc
Copy link
Contributor Author

onobc commented Nov 6, 2024

I'm confused. Maybe this is just a stepping stone?

This is definitely a stepping stone for server interceptors as a whole - it solves the global server interceptors path.
The service specific path I am handling next in #5. Sorry that I did not mention that in my description.

But the docs don't seem to make sense for the non-global case - ServerBuilder.intercept() is a method to add a global interceptor and you use it in the example of a non-global, unless I'm reading it wrong somehow. I think you need to create and wrap a service definition with ServerInterceptors.

Great catch. The 2nd half of the doc is incorrect and I will remove until tomorrow and put the proper doc in when I do #5 next.

Also, I know Spring Integration has global interceptors, so we probably copied the pattern from there?

The annotation was how they did it in the gRPC ecosystem and also we use similar annotations in other projects so it seems like a standard way to go.

But there is also the pattern of a "registration bean" with an enabled flag in Spring Boot for servlets and filters. That would work here too, and then we wouldn't need an annotation, or the annotation could be a convenient way of marking the interceptor registration as "enabled".

Ok, let me take a look into that a bit more tomorrow before we push this through.

@dsyer
Copy link
Member

dsyer commented Nov 6, 2024

I added support for registering beans of type ServerServiceDefnition so that should be the way to fix your service-specific interceptor docs.

This commit builds upon the previously added service discoverer
by adding support for finding all server interceptor beans that
are marked w/ the newly added `@GlobalServerInterceptor`
annotation.

Resolves spring-projects#4

Signed-off-by: Chris Bono <[email protected]>
@onobc onobc force-pushed the GH-4-global-server-interceptors-part2 branch from 47ba60a to e9ba943 Compare November 7, 2024 01:25
@dsyer dsyer merged commit 4daf413 into spring-projects:main Nov 7, 2024
1 check passed
@onobc onobc deleted the GH-4-global-server-interceptors-part2 branch November 7, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatically pick up global ServiceInterceptors

2 participants