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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change order for how FileDescriptors are returned in reflection service. #153

Merged

Conversation

bjorkstromm
Copy link
Contributor

// cc @mgravell @ChristianWeyer

Example using grpcurl with @ChristianWeyer's blazor app (https://github.com/thinktecture/blazor-webassembly-demo)

C:\temp>grpcurl localhost:5003 describe
ConfTool.Shared.Contracts.TimeService is a service:
service TimeService {
  rpc Subscribe ( .google.protobuf.Empty ) returns ( stream .ConfTool.Shared.Contracts.TimeResult );
}
ConfTool.Shared.Contracts.ConferencesService is a service:
service ConferencesService {
  rpc AddNewConference ( .ConfTool.Shared.Contracts.ConferenceDetails ) returns ( .ConfTool.Shared.Contracts.ConferenceDetails );
  rpc GetConferenceDetails ( .ConfTool.Shared.Contracts.ConferenceDetailsRequest ) returns ( .ConfTool.Shared.Contracts.ConferenceDetails );
  rpc ListConferences ( .google.protobuf.Empty ) returns ( .ConfTool.Shared.Contracts.IEnumerable_ConferenceOverview );
}
grpc.reflection.v1alpha.ServerReflection is a service:
service ServerReflection {
  rpc ServerReflectionInfo ( stream .grpc.reflection.v1alpha.ServerReflectionRequest ) returns ( stream .grpc.reflection.v1alpha.ServerReflectionResponse );
}

- FileDescriptors should be returned with the service first and then all dependencies after. All dependencies should occur after a FileDescriptor.
- See grpc/grpc-go#2949
  The Java implementation of the reflection service includes the entire transitive closure for a request file, in toplogical order such that a file's dependencies always appear after the file (so the requested file is first, with all of its dependencies after it).
bjorkstromm added a commit to bjorkstromm/dotnet-grpc-cli that referenced this pull request Jan 24, 2021
- Dependencies should be the other way around. See protobuf-net/protobuf-net.Grpc#153
bjorkstromm added a commit to bjorkstromm/dotnet-grpc-cli that referenced this pull request Jan 24, 2021
- Dependencies should be the other way around. See protobuf-net/protobuf-net.Grpc#153
@bjorkstromm
Copy link
Contributor Author

@ChristianWeyer, I've also fixed grpc-cli and it should be available on NuGet soonish.
https://www.nuget.org/packages/dotnet-grpc-cli/0.4.0
bjorkstromm/dotnet-grpc-cli@3d40df2

@bjorkstromm
Copy link
Contributor Author

@mgravell I tried to check if protogen also needed a fix after this change... But I can't seem to get it to work.

dotnet run -f net5.0 -- --grpc list https://localhost:5003
Requesting gRPC service directory from 'https://localhost:5003'...
The type initializer for 'DefaultProxyCache`1' threw an exception.
   at ProtoBuf.Grpc.Configuration.ClientFactory.DefaultClientFactory.CreateClient[TService](CallInvoker channel) in /_/src/protobuf-net.Grpc/Configuration/ClientFactory.cs:line 91
   at ProtoBuf.GrpcTools.ExecuteAsync(String modeString, String uri, String serviceName, CodeGenerator codegen, String outPath, Dictionary`2 options) in c:\src\gh\protobuf-net\src\protogen\GrpcTools.cs:line 58
   at protogen.Program.Main(String[] args) in c:\src\gh\protobuf-net\src\protogen\Program.cs:line 141

@mgravell mgravell merged commit 327829f into protobuf-net:main Jan 25, 2021
@mgravell
Copy link
Member

Merged. I'll see what I can find out about protogen, thanks

@ChristianWeyer
Copy link

Thank you!

@bjorkstromm bjorkstromm deleted the feature/change-filedescriptor-order branch January 25, 2021 08:05
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.

Issues with server reflection for code-first
3 participants