Don't send placeholder files; improve caching of sent files #66
Conversation
…, which is insufficient since it won't necessarily be unique
emcfarlane
left a comment
emcfarlane
left a comment
There was a problem hiding this comment.
lgtm! The FullName for file descriptor is a great catch.
grpcreflect.go
Outdated
| } | ||
|
|
||
| func (s *fileDescriptorNameSet) Insert(fd protoreflect.FileDescriptor) { | ||
| func (s *fileDescriptorNameSet) Insert(path string) { |
There was a problem hiding this comment.
nit: fileDescriptorNameSet use to take an fd. It looks like it changed to a path for testing. Maybe could keep the fd and the tests reference the fd too to make it clear that FullName wasn't unique. Otherwise this is a generic string set type.
There was a problem hiding this comment.
Perhaps this could be a set of type pather interface { Path() string }? I do like the fact that the existing code keeps the set type a little higher-level - it'd be nice to preserve that. In tests, we could implement the pather interface with type path string. That also leaves a clear place to document that FullName isn't unique.
There was a problem hiding this comment.
Alright. I just changed the signatures back to protoreflect.FileDescriptor and then added a dummyFile implementation that the test can use for this.
akshayjshah
left a comment
akshayjshah
left a comment
There was a problem hiding this comment.
Fine with me as-is, but if it's not too cumbersome I also liked having the set operate on descriptors.
grpcreflect.go
Outdated
| } | ||
|
|
||
| func (s *fileDescriptorNameSet) Insert(fd protoreflect.FileDescriptor) { | ||
| func (s *fileDescriptorNameSet) Insert(path string) { |
There was a problem hiding this comment.
Perhaps this could be a set of type pather interface { Path() string }? I do like the fact that the existing code keeps the set type a little higher-level - it'd be nice to preserve that. In tests, we could implement the pather interface with type path string. That also leaves a clear place to document that FullName isn't unique.
There was a problem hiding this comment.
FYI, these changes were made by Go 1.21. I recently updated Go to this latest version on my laptop, and it made these changes when running make test lint.
The original motivation for this change was to mirror the fix in grpc/grpc-go#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.FullNamewhich 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.