reflection: don't serialize placeholders

[jhump]

Resolves #6770.

On a related note, I'm also trying to land a patch to the protobuf runtime:
https://go-review.googlesource.com/c/protobuf/+/540455

I think both that and this fix are useful. This one is likely more useful since a "not found" error for a particular file will be a better error message to users, given that the issue is in fact that an imported file is missing from the global registry.

RELEASE NOTES:

• reflection: do not send invalid descriptors to clients for files that cannot be fully resolved

[codecov]

Codecov Report

Merging #6771 (6237a8b) into master (cf9ae52) will increase coverage by 0.00%.
Report is 8 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

[dfawley]

Thank you for the fix!

[dfawley]

@jhump

If you have some idea how to explain "placeholder" better in a user-friendly changelog message (I said "not completely defined"???) then please update the RELEASE NOTES section I added to #6771 (comment) - thanks!

[jhump]

If you have some idea how to explain "placeholder" better in a user-friendly changelog message

@dfawley, another way is to frame it more like the title of the corresponding issue (#6770):

• reflection: do not send invalid descriptors to the client for files that cannot be fully resolved

WDYT? I went ahead and updated the description in case that looks good to you.

[dfawley]

WDYT? I went ahead and updated the description in case that looks good to you.

LGTM!

[arvindbr8]

LGTM, thanks for the fix @jhump

[nikitaksv]

@arvindbr8 Hello. This update broke all popular clients with Reflection API support. Postman, Insomnia, grpcurl can't read API via reflection due to missing IsPlaceholder() on line https://github.com/grpc/grpc-go/blob/master/reflection/serverreflection.go#L189.

Add various imports (validation, openapiv2) to proto and see for yourself that reflection in clients will not be read.
I commented out the code at
https://github.com/grpc/grpc-go/blob/master/reflection/serverreflection.go#L189-L191
and
https://github.com/grpc/grpc-go/blob/master/reflection/serverreflection.go#L179-L183

And reflection API in Postman, Insomnia, grpcurl worked.

[jhump]

Add various imports (validation, openapiv2) to proto and see for yourself that reflection in clients will not be read.

@nikitaksv, this is not true. I have plenty of examples of working protos with those imports, and reflection works fine with them. This only happens if you use the wrong import path when importing those files. (See https://buf.build/docs/reference/protobuf-files-and-packages#imports for more info.) For example for openapiv2, you must use the following path:

import "protoc-gen-openapiv2/options/annotations.proto";

If your import references that file using any other path, then the descriptors cannot be correctly linked at runtime.

The placeholder descriptors are junk -- they come across as empty files at best (as of this fix) or as invalid, broken files (before that fix).

What library were you using in your reflection clients that they could successfully process these descriptors? I know that grpcurl used older versions of github.com/jhump/protoreflect/grpcreflect. Perhaps the others did, too, because that is likely the only library that happened to tolerate these files. That package (prior to v1.15) did not properly validate the downloaded file descriptors, so it was a fluke that they ever really worked. Using the official Protobuf runtimes/packages (such as google.golang.org/protobuf/reflect/protoreflect, but also languages other than Go), the placeholder descriptors downloaded from the server never worked. These files were only tolerated because they only provide custom options, which weren't resolved by that grpcreflect package, which is why that package didn't "notice" that the files were junk. If the same symptom (incorrect import path) occurred with a file that contained a message or enum type that the importing file needed, it would have broken.