Support adding gRPC interceptors using annotation '@GrpcInterceptor'

[Dogacel]

Motivation:

#5359

Adding annotations to add interceptors to gRPC services and methods.

Modifications:

• Create the GrpcInterceptor annotation.
• Update GrpcServerBuilder to find interceptors defined by annotations on gRPC servers and methods.

Result:

• Closes Add @GrpcInterceptor #5359

The way services are added to the HandlerRegistry has changed slightly, now we are not adding the whole Service objects as an entry but rather we find all MethodDescriptors under those services and add those methods one by one as Entry objects. The reason is that we can't create a single interceptor under a single Service object that would intercept individual methods, those methods need to be added as separate entries.

It can be checked if there is any method interception annotations and if there isn't any, it can fallback to the legacy logic (add the whole Service object as an Entry).

Question Do you think adding each method as a separate Entry would create performance issues?

Also, let me know if the interceptor order looks right based on the test.

[github-actions]

🔍 Build Scan® (commit: fdccead)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/4xyrseb4wymye
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/sjttku4b75fwy
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/ibruz3okmyimq
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/sdoymsocgvpmc
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/maoz6xuu4bxee
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/pr7lq5tfweezm
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/w22tspw5kbe6e
build-macos-12-jdk-21	❌ (failure)	https://ge.armeria.dev/s/yvy75by4g4qic

[minwoox]

Sorry about the late review. Left some suggestions. 😄

[minwoox]

Suggested change

	* {@link GrpcExceptionHandlerFunction} implementation type.
	* {@link ServerInterceptor} implementation type.

[codecov]

Codecov Report

Attention: Patch coverage is 88.31169% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.01%. Comparing base (14c5566) to head (78e9d53).
Report is 72 commits behind head on main.

❗ Current head 78e9d53 differs from pull request most recent head 89f1b76

Please upload reports for the commit 89f1b76 to get more accurate results.

Files	Patch %	Lines
.../linecorp/armeria/server/grpc/HandlerRegistry.java	89.18%	3 Missing and 5 partials ⚠️
...necorp/armeria/server/grpc/GrpcServiceBuilder.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5397      +/-   ##
============================================
- Coverage     74.05%   74.01%   -0.04%     
+ Complexity    21253    20745     -508     
============================================
  Files          1850     1800      -50     
  Lines         78600    76488    -2112     
  Branches      10032     9737     -295     
============================================
- Hits          58209    56615    -1594     
+ Misses        15686    15263     -423     
+ Partials       4705     4610      -95     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[trustin]

I think this PR looks good. Is there anything left for discussion or feedback, @Dogacel ?

[Dogacel]

I think this PR looks good. Is there anything left for discussion or feedback, @Dogacel ?

I don't think anything left for discussion, @minwoox 's comment should be resolved now 🙂 I merged latest master to double check the state of the tests.

[ikhoon]

It is not your work but there was a bug before. We should not create a DependencyInjector internally but use ServerConfig.dependencyInjector() instead.
Let me add a commit for that.

[ikhoon]

It was difficult to inject ServerConfig.dependencyInjector() without a huge refactoring. It would be better to use ReflectiveDependencyInjector as is in this PR. I will send a f/u PR to fix it when this PR is merged.
Could you add a TODO comment so as not to forget it?

// TODO(ikhoon): Use ServerConfig.dependencyInjector() instead of creating ReflectiveDependencyInjector directly.

[ikhoon]

For now, I will review the implementation correctness.

[Dogacel]

I pushed a commit to fix checkstyle error. Let me know if there are any other concerns 🙂

[trustin]

Looks good to me! I think we should do a follow-up so this uses the configured dependency injector.

[minwoox]

I think the order should be:

• global decorator -> class level decorator -> method level decorator
  like how @Decorator works.

However, the current order is reversed. Could you fix it?

[Dogacel]

Good point, let me fix 👍

[Dogacel]

Hmm taking a look at the test,

assertThat(headersCapture.get().get(TEST_HEADER)).isEqualTo("foobarqux");

Here first Foo (global) is appended, later bar which is the class level and finally qux which is the method level. Am I reading this incorrectly? I feel like this is the right order?

[Dogacel]

Note that we append headers in requests, not responses so the order of foo bar qux indicate order of decorators being executed.

[minwoox]

It seems like addToHeader is called in the reverse order.
You can check the order by simply adding a log:

private static class InterceptorAddFoo implements ServerInterceptor {
    @Override
    public <REQ, RESP> Listener<REQ> interceptCall(ServerCall<REQ, RESP> call, Metadata headers,
                                                   ServerCallHandler<REQ, RESP> next) {
        System.err.println("Should call first");
        return next.startCall(addToHeader("foo", call), headers);
    }
}

private static class InterceptorAddBar implements ServerInterceptor {
    @Override
    public <REQ, RESP> Listener<REQ> interceptCall(ServerCall<REQ, RESP> call, Metadata headers,
                                                   ServerCallHandler<REQ, RESP> next) {
        System.err.println("Should call second");
        return next.startCall(addToHeader("bar", call), headers);
    }
}

private static class InterceptorAddQux implements ServerInterceptor {
    @Override
    public <REQ, RESP> Listener<REQ> interceptCall(ServerCall<REQ, RESP> call, Metadata headers,
                                                   ServerCallHandler<REQ, RESP> next) {
        System.err.println("Should call third");
        return next.startCall(addToHeader("qux", call), headers);
    }
}

[Dogacel]

Fixed 🙌

[Dogacel]

Bump ^ 🙂 Seems like this was forgotten