connectrpc / grpcreflect-go

gRPC-compatible server reflection for any net/http server.
https://connectrpc.com
Apache License 2.0
73 stars 7 forks source link

Don't send placeholder files; improve caching of sent files #66

Closed jhump closed 10 months ago

jhump commented 10 months ago

The original motivation for this change was to mirror the fix in https://github.com/grpc/grpc-go/pull/6771. (See that issue for more context; originally reported as a bug against recent version of grpcurl.)

The issue there is that "placeholder" files (see Descriptor.IsPlaceholder) would be serialized as invalid descriptor protos (related). But they really shouldn't be serialized at all: they represent missing dependencies, so it's better to elide them.

While writing new tests for this, I also discovered that we were caching sent files incorrectly. The code was using FileDescriptor.FullName which is sadly not really specified in the docs. In practice, this returns the file's package, which is not enough to uniquely identify a file. That means we would only send back one dependency per package (instead of the entire graph), and then require the client ask for the others one-at-a-time. So this branch fixes that, too.