-
Notifications
You must be signed in to change notification settings - Fork 821
MapGrpcService with service definition refactor #2634
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
MapGrpcService with service definition refactor #2634
Conversation
…ttps://github.com/aka-nse/grpc-dotnet into feature/MapGrpcService-with-ServerServiceDefinition
…ttps://github.com/aka-nse/grpc-dotnet into feature/MapGrpcService-with-ServerServiceDefinition
|
cc @aka-nse |
| /// Forwards all the previously stored <c>AddMethod</c> calls to the service binder. | ||
| /// </summary> | ||
| internal void BindService(ServiceBinderBase serviceBinder) | ||
| public void BindService(ServiceBinderBase serviceBinder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aka-nse How was ServerServiceDefinition usable before this change? It doesn't seem like there is any way to access the methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I could find, this method was used within ServereServicesDefinitionExtensions.GetCallHandlers in Grpc.Core. In addition, it appears that the method's visibility is overridden using InternalsVisibleToAttribute from Grpc.Core.Api to Grpc.Core.
I do not know why this kind of visibility is used as I do not know gRPC at the time this implementation was written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think it is fine to make this public. It seems useful, it doesn't leak other API, and eventually removes need for internalsvisibleto which I've found always causes issues eventually.
src/Grpc.AspNetCore.Server/GrpcEndpointRouteBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Grpc.AspNetCore.Server/Model/Internal/ServiceDefinitionMethodProvider.cs
Show resolved
Hide resolved
This reverts commit 770a146.
Overall changes in PR:
Definer. Tempoarily references projects instead of nuget packages because nuget packages don't have APIs yetMapGrpcServiceoverloads to acceptServerServiceDefinitiontype. Binds methods added to the definition.ServerServiceDefinition.BindServicemethod to public.This PR is a refactor of #2586. Adds same features but remove a lot of duplication.
TServicetype parameter during binding. Possible by have a special internal service type that isn't created.