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

Add the ProtoReflectionService for better client side support #14 #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liweinan
Copy link
Contributor

@liweinan liweinan requested a review from ronsigal July 31, 2023 16:05
@liweinan liweinan self-assigned this Jul 31, 2023
@liweinan liweinan requested a review from jamezp as a code owner July 31, 2023 16:05
@liweinan
Copy link
Contributor Author

This currently doesn't work with WildFly gRPC subsystem:

Caused by: java.lang.IllegalAccessException: class org.wildfly.extension.grpc.GrpcServerService cannot access a member of class io.grpc.protobuf.services.ProtoReflectionService with modifiers "private"
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:420)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:709)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:493)
	at java.base/java.lang.reflect.ReflectAccess.newInstance(ReflectAccess.java:128)
	at java.base/jdk.internal.reflect.ReflectionFactory.newInstance(ReflectionFactory.java:306)
	at java.base/java.lang.Class.newInstance(Class.java:684)
	at [email protected]//org.wildfly.extension.grpc.GrpcServerService.newService(GrpcServerService.java:454)
	at [email protected]//org.wildfly.extension.grpc.GrpcServerService.addServiceClasses(GrpcServerService.java:369)
	at [email protected]//org.wildfly.extension.grpc.GrpcServerService.install(GrpcServerService.java:331)
	at [email protected]//org.wildfly.extension.grpc.deployment.GrpcDeploymentProcessor.deploy(GrpcDeploymentProcessor.java:54)
	... 9 more

@liweinan
Copy link
Contributor Author

@jamezp If I want to enable this ProtoReflectionService, is there anyway to fix the above problem?

@liweinan liweinan linked an issue Jul 31, 2023 that may be closed by this pull request
@liweinan
Copy link
Contributor Author

liweinan commented Aug 1, 2023

With the ProtoReflectionService added and the server started in standalone way, the grpc service can be accessed in a reflected way:

image
➤ grpcurl -plaintext localhost:8082 list                                                                              00:14:53
grpc.reflection.v1alpha.ServerReflection
org.greet.GreetService

bash-3.2$ grpcurl -plaintext  localhost:8082 describe org.greet.GreetService
org.greet.GreetService is a service:
service GreetService {
  rpc generalGreet ( .org.greet.GeneralEntityMessage ) returns ( .org.greet.GeneralReturnMessage );
  rpc greet ( .org.greet.GeneralEntityMessage ) returns ( .org.greet.GeneralReturnMessage );
}

However it's not worked with WildFly gRPC subsystem yet.

@liweinan
Copy link
Contributor Author

liweinan commented Aug 1, 2023

For example, the benefit to use reflection we can use tools like Postman to get the APIs and send request:

image

@jamezp
Copy link
Contributor

jamezp commented Aug 2, 2023

I'm not sure I follow this error. The org.wildfly.extension.grpc.GrpcServerService.ewService(Class<?> serviceClass, Set<Class<?>> serviceClasses, Set<BindableService> services) is private, but it should be. I'd have to try to build this and see what happens to understand it I think.

We don't want to use reflection if we can avoid it. I'm sure there has to be a way to avoid using it.

@liweinan
Copy link
Contributor Author

liweinan commented Aug 2, 2023

@jamezp Thanks for checking this!

The feature to enable the API reflection is important to the client side IMHO. Similar to the OpenAPI feature, for the clients that doesn't have the server side .proto file definition, they have to use the gRPC reflection feature to get the API description. I guess we can provide configuration to enable/disable this reflection feature in future, but the first step is to support it at least.

@jamezp
Copy link
Contributor

jamezp commented Aug 2, 2023

The feature to enable the API reflection is important to the client side IMHO. Similar to the OpenAPI feature, for the clients that doesn't have the server side .proto file definition, they have to use the gRPC reflection feature to get the API description. I guess we can provide configuration to enable/disable this reflection feature in future, but the first step is to support it at least.

Yes, I think it should be opt-in. We shouldn't default to allowing data descriptions to be presented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ProtoReflectionService for better client side support
2 participants