service reflection: include transitive closure for a file

[GarrettGutierrez1]

Fixes #2949.

[dfawley]

Did you confirm grpcurl and/or grpc_cli is happy with this? And were you able to reproduce the problem first?

Could you make sure there is a test for this functionality? If a unit test is too tedious, I'd be okay with a shell script that runs grpcurl and requires this functionality, that we run as part of the "extras" travis target.

[GarrettGutierrez1]

I got a simple server running with the C++ implementation and grpcurl/grpc_cli both have the same output with the go implementation. In none of the cases are the imported proto files reported in the output, so the output after this change is the same as the output prior to this change. That means a test on console output of grpcurl/grpc_cli can't verify this change. I could create a unit test that checks the messages sent back from the reflection service to ensure the imported proto file descriptors are sent.

[dfawley]

I got a simple server running with the C++ implementation and grpcurl/grpc_cli both have the same output with the go implementation.

How simple is this service?

Does the Request/Response for your method include a proto message defined in another package? Can you share your example and what you ran and what the output was?

[GarrettGutierrez1]

Here is the service I created to test. proto/simple/simple.proto imports proto/simple/messages/messages.proto (the latter implements the message types used by the former). Below are the outputs for grpc_cli and grpcurl.

$ grpc_cli ls localhost:50051 simple.Simple -l
filename: simple/simple.proto
package: simple;
service Simple {
  rpc SimpleUnary(simple.messages.SimpleRequest) returns (simple.messages.SimpleReply) {}
  rpc SimpleServerStream(simple.messages.SimpleRequest) returns (stream simple.messages.SimpleReply) {}
  rpc SimpleClientStream(stream simple.messages.SimpleRequest) returns (simple.messages.SimpleReply) {}
  rpc SimpleBidirectionalStream(stream simple.messages.SimpleRequest) returns (stream simple.messages.SimpleReply) {}
}

$ grpcurl -v -plaintext localhost:50051 list simple.Simple
simple.Simple.SimpleBidirectionalStream
simple.Simple.SimpleClientStream
simple.Simple.SimpleServerStream
simple.Simple.SimpleUnary

[dfawley]

You're just doing a list for both. You'll need to actually try to send a request via grpcurl/grpc_cli to see what happens if it can't marshal the messages with the data the server is including in the reflection response. My guess is grpcurl (maybe grpc_cli) will compensate for this by requesting the dependencies explicitly, but I don't know for sure.

[GarrettGutierrez1]

The output with grpcurl, on the go server, with the simple server provided is now as follows

$ grpcurl -plaintext -d '{"host": "myhost", "file_by_filename": "simple/simple.proto"}' localhost:50051 grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo
{
  "validHost": "myhost",
  "originalRequest": {
    "host": "myhost",
    "fileByFilename": "simple/simple.proto"
  },
  "fileDescriptorResponse": {
    "fileDescriptorProto": [
      "ChNzaW1wbGUvc2ltcGxlLnByb3RvEgZzaW1wbGUaHnNpbXBsZS9tZXNzYWdlcy9tZXNzYWdlcy5wcm90bzLoAgoGU2ltcGxlEk0KC1NpbXBsZVVuYXJ5Eh4uc2ltcGxlLm1lc3NhZ2VzLlNpbXBsZVJlcXVlc3QaHC5zaW1wbGUubWVzc2FnZXMuU2ltcGxlUmVwbHkiABJWChJTaW1wbGVTZXJ2ZXJTdHJlYW0SHi5zaW1wbGUubWVzc2FnZXMuU2ltcGxlUmVxdWVzdBocLnNpbXBsZS5tZXNzYWdlcy5TaW1wbGVSZXBseSIAMAESVgoSU2ltcGxlQ2xpZW50U3RyZWFtEh4uc2ltcGxlLm1lc3NhZ2VzLlNpbXBsZVJlcXVlc3QaHC5zaW1wbGUubWVzc2FnZXMuU2ltcGxlUmVwbHkiACgBEl8KGVNpbXBsZUJpZGlyZWN0aW9uYWxTdHJlYW0SHi5zaW1wbGUubWVzc2FnZXMuU2ltcGxlUmVxdWVzdBocLnNpbXBsZS5tZXNzYWdlcy5TaW1wbGVSZXBseSIAKAEwAUIsWipnaXRodWIuY29tL0dhcnJldHRHdXRpZXJyZXoxL3NpbXBsZS9zaW1wbGViBnByb3RvMw==",
      "Ch5zaW1wbGUvbWVzc2FnZXMvbWVzc2FnZXMucHJvdG8SD3NpbXBsZS5tZXNzYWdlcyIpCg1TaW1wbGVSZXF1ZXN0EhgKB21lc3NhZ2UYASABKAlSB21lc3NhZ2UiJwoLU2ltcGxlUmVwbHkSGAoHbWVzc2FnZRgBIAEoCVIHbWVzc2FnZUI1WjNnaXRodWIuY29tL0dhcnJldHRHdXRpZXJyZXoxL3NpbXBsZS9zaW1wbGUvbWVzc2FnZXNiBnByb3RvMw=="
    ]
  }
}

I see there are two file descriptors sent, however the 2nd one is different with the go server than with the C++ server? The first is identical in both.

[dfawley]

(Reassigning to reflect that we're waiting on more information about how this behaves when a normal RPC is called via grpc_cli / grpcurl), and what the differences observed mean (i.e. un-base64/gzip the FDProtos).

[GarrettGutierrez1]

File descriptor for messages.proto (which is imported) from the C++ server is as follows, followed by the go server. Looks like go just has a json_name parameter for the message types.

name:"simple/messages/messages.proto"
package:"simple.messages"
message_type:{name:"SimpleRequest"  field:{name:"message"  number:1  label:LABEL_OPTIONAL  type:TYPE_STRING}}
message_type:{name:"SimpleReply"  field:{name:"message"  number:1  label:LABEL_OPTIONAL  type:TYPE_STRING}}
options:{go_package:"github.com/GarrettGutierrez1/simple/simple/messages"}  syntax:"proto3"

name:"simple/messages/messages.proto"
package:"simple.messages"
message_type:{name:"SimpleRequest"  field:{name:"message"  number:1  label:LABEL_OPTIONAL  type:TYPE_STRING  json_name:"message"}}
message_type:{name:"SimpleReply"  field:{name:"message"  number:1  label:LABEL_OPTIONAL  type:TYPE_STRING  json_name:"message"}}
options:{go_package:"github.com/GarrettGutierrez1/simple/simple/messages"}  syntax:"proto3"

[dfawley]

!= 2 would be even more precise.