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

reflection: if some files cannot be resolved, reflection will send invalid descriptors to client #6770

Closed
jhump opened this issue Nov 7, 2023 · 0 comments · Fixed by #6771

Comments

@jhump
Copy link
Member

jhump commented Nov 7, 2023

When generated Protobuf code registers file descriptors with the global registry, whenever there are missing dependencies, they are silently replaced with "placeholder" descriptors.

This happens if a file uses the wrong import path. For example, a file is compiled with the path "validate/validate.proto", so it is registered in protoregistry.GlobalFiles with that string as its path. Another file that imports it uses an import statement of "proto/validate/validate.proto" instead. When this file is registered during program initialization, the dependency cannot be found (because its name doesn't match the way the file was actually registered). So the reflection library inserts a placeholder file descriptor.

When gRPC server reflection is used to download descriptors for the importing file, it will provide a junk file descriptor for the missing dependency, the one named "proto/validate/validate.proto". (Original grpcurl bug report: fullstorydev/grpcurl#423).

This can result in confusing error messages because the reason the descriptor is invalid (aside from the fact that it is empty, so any symbols that the importing file references will be unresolvable) is that the syntax field of the file descriptor is the string <unknown:0>. When trying to process this descriptor, this can result in a very confusing error message to users.

A separate fix is needed in the Protobuf runtime to just not set the syntax field of the descriptor if its value is unknown, instead of setting this string, That would help... a little. But without a change to the gRPC reflection implementation, the client will still get a descriptor that is not actually usable. The error message would then be a linking issue since the imported symbol still won't be available. IMO, the proper solution is for the reflection implementation to elide placeholder descriptors. That will result in a better error message along the lines of could not resolve file "proto/validate/validate.proto": not found.

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.

1 participant