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

Issue 4567 - Grpc component Does not handle package directive in proto files. #5283

Merged

Conversation

klustria
Copy link
Member

Problems:

  1. ServiceDescriptor$Builder.getTypeFromMethodDescriptor throws ClassNotFoundException. This is the original issue reported by the filer. example java.lang.RuntimeException: java.lang.ClassNotFoundException: io.helidon.grpc.server.test.Echo$helidon$tests$EchoRequest
  2. Request throws this exception: "io.grpc.StatusRuntimeException: UNIMPLEMENTED: Method not found". This issue was only exposed when "mvn verify" was used which triggered 3 additional integration tests, ContextIT, TracingIT and SslIT that all encountered the same problem. example: [ERROR] io.helidon.grpc.server.ContextIT.shouldObtainValueFromContextForRequest Time elapsed: 0.192 s <<< ERROR!
    io.grpc.StatusRuntimeException: UNIMPLEMENTED: Method not found: helidon.test.EchoService/Echo
    at io.helidon.grpc.server.ContextIT.shouldObtainValueFromContextForRequest(ContextIT.java:90)

Solutions:

  1. For the 1st Problem, change className generation in ServiceDescriptor$Builder.getTypeFromMethodDescriptor of using "type.getFullName().replace('.', '$')" to instead use "type.getName()".
  2. For 2nd Problem, There are 2 fixes that needs to be done: a. io.grpc.MethodDescriptor() needs to have the service part of the fullMethodName to be prefixed with the package directive name, example it should be "helidon.test.EchoService/Echo" instead of "EchoService/Echo" if the package directive name is "helidon.test". Not doing this will cause "registry.lookupMethod(methodName)" call in ServerImpl.runInternal() on gprc-java-core library to throw "UNIMPLEMENTED: Method not found" as it won't be able to find it without the service name prefixed with the package name. This is fixed in ServiceDescriptor$Builder.build() and ServiceDescriptor$Builder.createMethodDescriptor() by passing in the fullMethodName (that prefixes it with the package name) though the fullName() method in io.helidon.grpc.server.MethodDescriptor$Builder which in turn sets the io.grpc.MethodDescriptor() that it encapsulates. b. Even if above is fixed, io.grpc.ServiceDescriptor() also needs to have its name be prefixed with the package directive name. This is because when io.grpc.ServiceDescriptor() is instantiated, it validates its name against the service Name of all io.grpc.MethodDescriptor() objects that it holds and will throw an error like this: "java.lang.IllegalArgumentException: service names helidon.test.EchoService != EchoService". To fix this issue, BindableServiceImpl.bindService() needs to use the fullServiceName (i.e. the name of the service prefixed with the package name) when creating a new Builder for io.grpc.ServiceDescriptor.
  3. getFullName logic is moved inside ServiceDescriptor builder and will set ServiceDescriptor fullName field when build is triggered to create a new instance.
  4. More refactoring and additional unit test to cover no java package in proto file scenario
  5. Rename methods for retrieving fullName and packageNane in ServiceDescriptor and add shouldBuildFromBindableService test in GrpcProtoPackageTest and GrpcProtoPackageNoJavaPackageTest to test scenario where name is already prefixed with package.

…o files.

Problems:
1. ServiceDescriptor$Builder.getTypeFromMethodDescriptor throws ClassNotFoundException. This is the original issue reported by the filer.
example java.lang.RuntimeException: java.lang.ClassNotFoundException: io.helidon.grpc.server.test.Echo$helidon$tests$EchoRequest
2. Request throws this exception: "io.grpc.StatusRuntimeException: UNIMPLEMENTED: Method not found". This issue was only exposed when "mvn verify" was used which triggered 3 additional integration tests, ContextIT, TracingIT and SslIT that all encountered the same problem.
example: [ERROR] io.helidon.grpc.server.ContextIT.shouldObtainValueFromContextForRequest  Time elapsed: 0.192 s  <<< ERROR!
         io.grpc.StatusRuntimeException: UNIMPLEMENTED: Method not found: helidon.test.EchoService/Echo
                 at io.helidon.grpc.server.ContextIT.shouldObtainValueFromContextForRequest(ContextIT.java:90)

Solutions:
1. For the 1st Problem, change className generation in ServiceDescriptor$Builder.getTypeFromMethodDescriptor of using "type.getFullName().replace('.', '$')" to instead use "type.getName()".
2. For 2nd Problem, There are 2 fixes that needs to be done:
   a. io.grpc.MethodDescriptor() needs to have the service part of the fullMethodName to be prefixed with the package directive name, example it should be "helidon.test.EchoService/Echo" instead of "EchoService/Echo" if the package directive name is "helidon.test". Not doing this will cause "registry.lookupMethod(methodName)" call in ServerImpl.runInternal() on gprc-java-core library to throw "UNIMPLEMENTED: Method not found" as it won't be able to find it without the service name prefixed with the package name. This is fixed in ServiceDescriptor$Builder.build() and ServiceDescriptor$Builder.createMethodDescriptor() by passing in the fullMethodName (that prefixes it with the package name) though the fullName() method in io.helidon.grpc.server.MethodDescriptor$Builder which in turn sets the io.grpc.MethodDescriptor() that it encapsulates.
   b. Even if above is fixed, io.grpc.ServiceDescriptor() also needs to have its name be prefixed with the package directive name. This is because when io.grpc.ServiceDescriptor() is instantiated, it validates its name against the service Name of all io.grpc.MethodDescriptor() objects that it holds and will throw an error like this: "java.lang.IllegalArgumentException: service names helidon.test.EchoService != EchoService". To fix this issue, BindableServiceImpl.bindService() needs to use the fullServiceName (i.e. the name of the service prefixed with the package name) when creating a new Builder for io.grpc.ServiceDescriptor.
3. getFullName logic is moved inside ServiceDescriptor builder and will set ServiceDescriptor fullName field when build is triggered to create a new instance.
4. More refactoring and additional unit test to cover no java package in proto file scenario
5. Rename methods for retrieving fullName and packageNane in ServiceDescriptor and add shouldBuildFromBindableService test in GrpcProtoPackageTest and GrpcProtoPackageNoJavaPackageTest to test scenario where name is already prefixed with package.
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 29, 2022
Copy link
Collaborator

@aseovic aseovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@klustria klustria merged commit 2c63140 into helidon-io:helidon-3.x Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants