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 #2949

Closed
jhump opened this issue Aug 2, 2019 · 4 comments · Fixed by #3851
Closed

service reflection: include transitive closure for a file #2949

jhump opened this issue Aug 2, 2019 · 4 comments · Fixed by #3851

Comments

@jhump
Copy link
Member

jhump commented Aug 2, 2019

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).

However, the Go implementation only ever includes a single file and never includes its dependencies. It would be nice to also include the transitive closure (at least if a given file has never been sent) so that clients have a better chance of trying to link files even if the import name differs from the descriptor name. This is basically the same problem as golang/protobuf#567, except for downloading descriptors (vs. just querying the set of linked-in/registered descriptors).

Admittedly, having the set of transitive dependencies will still be error-prone if the names mismatch. The client could attempt to reconcile unlinkable descriptors by trying to match dependencies based only on the base name of the file and not its relative path. Or the set of descriptors could be provided in a well-specified order, that allows clients to compute the index for a given import and link it without relying on the name (which may not match).

For more context, see this: grpc/grpc#19832

@ejona86
Copy link
Member

ejona86 commented Aug 8, 2019

https://github.com/grpc/grpc/blob/master/doc/server-reflection.md specifies:

Any given request must either result in an error code or an answer, usually in the form of a series of FileDescriptorProtos with the requested file itself and all previously unsent transitive imports of that file.

So it appears this may be a bug instead of a feature request.

@dfawley dfawley added fixit P2 Status: Help Wanted Type: Bug and removed Type: Feature New features or improvements in behavior labels Aug 15, 2019
@stale
Copy link

stale bot commented Sep 6, 2019

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@stale stale bot added the stale label Sep 6, 2019
@jhump
Copy link
Member Author

jhump commented Sep 6, 2019

@dfawley, do you know what label that the stale bot is talking about? AFAICT, this is not waiting on any updates from me.

@stale stale bot removed the stale label Sep 6, 2019
@dfawley
Copy link
Member

dfawley commented Sep 6, 2019

stale bot is broken (I think just for labels with spaces): probot/stale#207

bjorkstromm added a commit to bjorkstromm/protobuf-net.Grpc that referenced this issue Jan 24, 2021
- 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).
mgravell pushed a commit to protobuf-net/protobuf-net.Grpc that referenced this issue Jan 25, 2021
…ce. (#153)

- 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).
@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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants