Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grpc client interceptors are not registered when injected via constructor #44932

Closed
RoMiRoSSaN opened this issue Dec 5, 2024 · 8 comments · Fixed by #44961
Closed

Grpc client interceptors are not registered when injected via constructor #44932

RoMiRoSSaN opened this issue Dec 5, 2024 · 8 comments · Fixed by #44961
Assignees
Labels
area/grpc gRPC kind/bug Something isn't working
Milestone

Comments

@RoMiRoSSaN
Copy link
Contributor

Describe the bug

Since version 3.9 client interceptors are not registered when injected via constructor, if there are more than 1 of interceptor.
If there is one, then it registers successfully.
When inject by field - all works.

How I understand, there were changes in InjectionPointInfo or somewhere else that affected this. If more than 1 interceptor is listed, they are not included in the qualifiers. As a result, not a single interceptor is registered.
https://github.com/quarkusio/quarkus/blob/main/extensions/grpc/deployment/src/main/java/io/quarkus/grpc/deployment/GrpcClientProcessor.java#L433

Before version 3.9 it works normal. I checked from 3.9 to 3.17, and this bug containing.

Expected behavior

  1. If 1 interceptor - interceptor registered
  2. If 2 or more interceptors - interceptor registered

Actual behavior

  1. If 1 interceptor - interceptor registered
  2. If 2 or more interceptors - none are registered

How to Reproduce?

Reproducer: grpc-demo.zip

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@RoMiRoSSaN RoMiRoSSaN added the kind/bug Something isn't working label Dec 5, 2024
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Dec 5, 2024
Copy link

quarkus-bot bot commented Dec 5, 2024

/cc @alesj (grpc), @cescoffier (grpc)

@geoand
Copy link
Contributor

geoand commented Dec 5, 2024

cc @mkouba

@mkouba mkouba self-assigned this Dec 5, 2024
@mkouba
Copy link
Contributor

mkouba commented Dec 5, 2024

It's very likely a regression caused by d34eb5a#diff-c176df2cb8a1cd56ae30ff243a7c6f817604c94f4b755ff424a79bdf0fd6b798. The code filters the qualifiers by annotation target but for nested annotations (which is the case for repeatable annotations used in a container) it's null.

CC @manovotn

@mkouba mkouba removed their assignment Dec 5, 2024
@manovotn manovotn self-assigned this Dec 5, 2024
@manovotn
Copy link
Contributor

manovotn commented Dec 5, 2024

@RoMiRoSSaN in your reproducer, I don't see the FirstClientInterceptor and SecondClientInterceptor being invoked regardless of whether you use field injection or constructor. Am I missing something?
I do see the GlobalClientInterceptor being invoked but none other.

It's very likely a regression caused by d34eb5a#diff-c176df2cb8a1cd56ae30ff243a7c6f817604c94f4b755ff424a79bdf0fd6b798. The code filters the qualifiers by annotation target but for nested annotations (which is the case for repeatable annotations used in a container) it's null.

Yes, after some digging it seems that the code here is a leftover of the commit you posted that should have been removed with #40841. We should now be able to just return transformationContext.getQualifiers()
That doesn't seem to break any ArC tests but doesn't fix the reproducer for me either 🤔
I'll get back to it tomorrow...

@RoMiRoSSaN
Copy link
Contributor Author

@manovotn Hi. I have redesigned the reproducer for better understanding.
Start it and call GET/client method. In out will be

Start call from injected by constructor one - first interceptor will be
FIRST CLIENT
End call from injected by constructor one. If you see FIRST CLIENT - all works

Start call from injected by constructor two - must be two interceptors messages
End call from injected by constructor two. Nothing. Interceptors not working     <------ problem

Start call from injected by field one - first interceptor will be
FIRST CLIENT
End call from injected by field one. If you see FIRST CLIENT - all works

Start call from injected by field two - will be first and second interceptors
FIRST CLIENT
SECOND CLIENT
End call from injected by field two. If you see FIRST and SECOND CLIENT - all works

Reproducer: grpc-demo.zip

@mkouba
Copy link
Contributor

mkouba commented Dec 5, 2024

That doesn't seem to break any ArC tests but doesn't fix the reproducer for me either

In any case, we should add an alternative to ClientInterceptorRegistrationTest with constructor injection.

@alesj
Copy link
Contributor

alesj commented Dec 5, 2024

with constructor injection

Yeah, just copy/paste the test and should be fine.
Or perhaps some base class for both cases - field and ctor injection test.

    public CtorClientInterceptorRegistrationTest(
        @RegisterClientInterceptor(MyLastClientInterceptor.class)
        @RegisterClientInterceptor(MyThirdClientInterceptor.class)
        @GrpcClient("hello-service") GreeterGrpc.GreeterBlockingStub client)
    {
        this.client = client;
    }

@manovotn
Copy link
Contributor

manovotn commented Dec 6, 2024

Ah, I see, I misunderstood it originally, thanks for clarification @RoMiRoSSaN :)

The fix I mentioned above works for your reproducer, so I'll push that along with a test momentarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc gRPC kind/bug Something isn't working
Projects
None yet
6 participants