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

service reflection: include transitive closure for a file #3851

Merged
merged 6 commits into from Sep 9, 2020
Merged

service reflection: include transitive closure for a file #3851

merged 6 commits into from Sep 9, 2020

Conversation

GarrettGutierrez1
Copy link
Contributor

Fixes #2949.

Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

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.

reflection/serverreflection.go Outdated Show resolved Hide resolved
reflection/serverreflection.go Outdated Show resolved Hide resolved
reflection/serverreflection.go Outdated Show resolved Hide resolved
@GarrettGutierrez1
Copy link
Contributor Author

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
Copy link
Contributor

dfawley commented Sep 2, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

dfawley commented Sep 2, 2020

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
Copy link
Contributor Author

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 dfawley assigned GarrettGutierrez1 and unassigned dfawley Sep 8, 2020
@dfawley
Copy link
Contributor

dfawley commented Sep 8, 2020

(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
Copy link
Contributor Author

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"

t.Errorf("FileByFilename(%v)\nreceived: %q,\nwant: %q", filename, r.GetFileDescriptorResponse().FileDescriptorProto[0], fdProto2Ext2Byte)
}
if expectClosure {
if len(r.GetFileDescriptorResponse().FileDescriptorProto) < 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

!= 2 would be even more precise.

@GarrettGutierrez1 GarrettGutierrez1 merged commit 52029da into grpc:master Sep 9, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

service reflection: include transitive closure for a file
2 participants