From 4e9a794021f9c2e7fa5bf91a328d13827bedc455 Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Fri, 10 Nov 2023 15:44:26 -0500 Subject: [PATCH 1/4] don't include placeholders --- grpcreflect.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/grpcreflect.go b/grpcreflect.go index c02dfea..e22c322 100644 --- a/grpcreflect.go +++ b/grpcreflect.go @@ -329,12 +329,20 @@ func (s *fileDescriptorNameSet) Contains(fd protoreflect.FileDescriptor) bool { return ok } -func fileDescriptorWithDependencies(fd protoreflect.FileDescriptor, sent *fileDescriptorNameSet) ([][]byte, error) { +func fileDescriptorWithDependencies(rootFile protoreflect.FileDescriptor, sent *fileDescriptorNameSet) ([][]byte, error) { + if rootFile.IsPlaceholder() { + // A placeholder is used when a dependency is missing. If a placeholder is all we have + // then we don't actually have anything. + return nil, protoregistry.NotFound + } results := make([][]byte, 0, 1) - queue := []protoreflect.FileDescriptor{fd} + queue := []protoreflect.FileDescriptor{rootFile} for len(queue) > 0 { curr := queue[0] queue = queue[1:] + if curr.IsPlaceholder() { + continue // don't bother serializing placeholders + } if len(results) == 0 || !sent.Contains(curr) { // always send root fd // Mark as sent immediately. If we hit an error marshaling below, there's // no point trying again later. From 54d7c960dd91930f5a46c9a9632ef76f3fe2de44 Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Fri, 10 Nov 2023 16:39:35 -0500 Subject: [PATCH 2/4] need to cache by path; full name for a file descriptor is its package, which is insufficient since it won't necessarily be unique --- grpcreflect.go | 16 ++++++++-------- grpcreflect_test.go | 12 +++++++----- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/grpcreflect.go b/grpcreflect.go index e22c322..036a961 100644 --- a/grpcreflect.go +++ b/grpcreflect.go @@ -314,18 +314,18 @@ type ExtensionResolver interface { } type fileDescriptorNameSet struct { - names map[protoreflect.FullName]struct{} + names map[string]struct{} } -func (s *fileDescriptorNameSet) Insert(fd protoreflect.FileDescriptor) { +func (s *fileDescriptorNameSet) Insert(path string) { if s.names == nil { - s.names = make(map[protoreflect.FullName]struct{}, 1) + s.names = make(map[string]struct{}, 1) } - s.names[fd.FullName()] = struct{}{} + s.names[path] = struct{}{} } -func (s *fileDescriptorNameSet) Contains(fd protoreflect.FileDescriptor) bool { - _, ok := s.names[fd.FullName()] +func (s *fileDescriptorNameSet) Contains(path string) bool { + _, ok := s.names[path] return ok } @@ -343,10 +343,10 @@ func fileDescriptorWithDependencies(rootFile protoreflect.FileDescriptor, sent * if curr.IsPlaceholder() { continue // don't bother serializing placeholders } - if len(results) == 0 || !sent.Contains(curr) { // always send root fd + if len(results) == 0 || !sent.Contains(curr.Path()) { // always send root fd // Mark as sent immediately. If we hit an error marshaling below, there's // no point trying again later. - sent.Insert(curr) + sent.Insert(curr.Path()) encoded, err := proto.Marshal(protodesc.ToFileDescriptorProto(curr)) if err != nil { return nil, err diff --git a/grpcreflect_test.go b/grpcreflect_test.go index 893ded8..3c60ffc 100644 --- a/grpcreflect_test.go +++ b/grpcreflect_test.go @@ -87,6 +87,7 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { assertFileDescriptorResponseContains := func( tb testing.TB, req *reflectionv1.ServerReflectionRequest, + numFiles int, substring string, ) { tb.Helper() @@ -102,8 +103,8 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { tb.Fatal("got nil FileDescriptorResponse") return // convinces staticcheck that remaining code is unreachable } - if len(fds.FileDescriptorProto) != 1 { - tb.Fatalf("got %d FileDescriptorProtos, expected 1", len(fds.FileDescriptorProto)) + if len(fds.FileDescriptorProto) != numFiles { + tb.Fatalf("got %d FileDescriptorProtos, expected %d", len(fds.FileDescriptorProto), numFiles) } if !bytes.Contains(fds.FileDescriptorProto[0], []byte(substring)) { tb.Fatalf( @@ -171,7 +172,7 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { FileByFilename: "connectext/grpc/reflection/v1/reflection.proto", }, } - assertFileDescriptorResponseContains(t, req, reflectionRequestFQN) + assertFileDescriptorResponseContains(t, req, 1, reflectionRequestFQN) }) t.Run("file_by_filename_missing", func(t *testing.T) { t.Parallel() @@ -191,7 +192,7 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { FileContainingSymbol: reflectionRequestFQN, }, } - assertFileDescriptorResponseContains(t, req, "reflection.proto") + assertFileDescriptorResponseContains(t, req, 1, "reflection.proto") }) t.Run("file_containing_symbol_missing", func(t *testing.T) { t.Parallel() @@ -214,7 +215,8 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { }, }, } - assertFileDescriptorResponseContains(t, req, "reflecttest_ext.proto") + // We expect two files here: both reflecttest_ext.proto and its dependency, reflecttest.proto + assertFileDescriptorResponseContains(t, req, 2, "reflecttest_ext.proto") }) t.Run("file_containing_extension_missing", func(t *testing.T) { t.Parallel() From 47a1d201de2740e89d58decdd93e038884ab97f6 Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Fri, 10 Nov 2023 16:44:20 -0500 Subject: [PATCH 3/4] new tests --- grpcreflect_test.go | 191 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/grpcreflect_test.go b/grpcreflect_test.go index 3c60ffc..3f4e971 100644 --- a/grpcreflect_test.go +++ b/grpcreflect_test.go @@ -19,14 +19,20 @@ import ( "context" "net/http" "net/http/httptest" + "reflect" + "sort" "testing" "connectrpc.com/connect" _ "connectrpc.com/grpcreflect/internal/gen/go/connect/reflecttest/v1" reflectionv1 "connectrpc.com/grpcreflect/internal/gen/go/connectext/grpc/reflection/v1" "github.com/google/go-cmp/cmp" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protodesc" + "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/testing/protocmp" + "google.golang.org/protobuf/types/descriptorpb" ) const actualServiceName = "connectext.grpc.reflection.v1.ServerReflection" @@ -295,3 +301,188 @@ func testReflector(t *testing.T, reflector *Reflector, servicePath string) { assertFileDescriptorResponseNotFound(t, req) }) } + +func TestFileDescriptorWithDependencies(t *testing.T) { + t.Parallel() + + depFile, err := protodesc.NewFile( + &descriptorpb.FileDescriptorProto{ + Name: proto.String("dep.proto"), + }, nil, + ) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + deps := &protoregistry.Files{} + if err := deps.RegisterFile(depFile); err != nil { + t.Fatalf("unexpected error: %s", err) + } + + rootFileProto := &descriptorpb.FileDescriptorProto{ + Name: proto.String("root.proto"), + Dependency: []string{ + "google/protobuf/descriptor.proto", + "connect/reflecttest/v1/reflecttest_ext.proto", + "dep.proto", + }, + } + + // dep.proto is in deps; the other imports come from protoregistry.GlobalFiles + resolver := &combinedResolver{first: protoregistry.GlobalFiles, second: deps} + rootFile, err := protodesc.NewFile(rootFileProto, resolver) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + // Create a file hierarchy that contains a placeholder for dep.proto + placeholderDep := placeholderFile{depFile} + placeholderDeps := &protoregistry.Files{} + if err := placeholderDeps.RegisterFile(placeholderDep); err != nil { + t.Fatalf("unexpected error: %s", err) + } + resolver = &combinedResolver{first: protoregistry.GlobalFiles, second: placeholderDeps} + + rootFileHasPlaceholderDep, err := protodesc.NewFile(rootFileProto, resolver) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + rootFileIsPlaceholder := placeholderFile{rootFile} + + // Full transitive dependency graph of root.proto includes five files: + // - root.proto + // - google/protobuf/descriptor.proto + // - connect/reflecttest/v1/reflecttest_ext.proto + // - connect/reflecttest/v1/reflecttest.proto + // - dep.proto + + testCases := []struct { + name string + sent []string + root protoreflect.FileDescriptor + expect []string + }{ + { + name: "send_all", + root: rootFile, + // expect full transitive closure + expect: []string{ + "root.proto", + "google/protobuf/descriptor.proto", + "connect/reflecttest/v1/reflecttest_ext.proto", + "connect/reflecttest/v1/reflecttest.proto", + "dep.proto", + }, + }, + { + name: "already_sent", + sent: []string{ + "root.proto", + "google/protobuf/descriptor.proto", + "connect/reflecttest/v1/reflecttest_ext.proto", + "connect/reflecttest/v1/reflecttest.proto", + "dep.proto", + }, + root: rootFile, + // expect only the root to be re-sent + expect: []string{"root.proto"}, + }, + { + name: "some_already_sent", + sent: []string{ + "connect/reflecttest/v1/reflecttest_ext.proto", + "connect/reflecttest/v1/reflecttest.proto", + }, + root: rootFile, + expect: []string{ + "root.proto", + "google/protobuf/descriptor.proto", + "dep.proto", + }, + }, + { + name: "root_is_placeholder", + root: rootFileIsPlaceholder, + // expect error, no files + }, + { + name: "placeholder_skipped", + root: rootFileHasPlaceholderDep, + // dep.proto is a placeholder so is skipped + expect: []string{ + "root.proto", + "google/protobuf/descriptor.proto", + "connect/reflecttest/v1/reflecttest_ext.proto", + "connect/reflecttest/v1/reflecttest.proto", + }, + }, + { + name: "placeholder_skipped_and_some_sent", + sent: []string{ + "connect/reflecttest/v1/reflecttest_ext.proto", + "connect/reflecttest/v1/reflecttest.proto", + }, + root: rootFileHasPlaceholderDep, + expect: []string{ + "root.proto", + "google/protobuf/descriptor.proto", + }, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + sent := &fileDescriptorNameSet{} + for _, path := range testCase.sent { + sent.Insert(path) + } + + descriptors, err := fileDescriptorWithDependencies(testCase.root, sent) + if len(testCase.expect) == 0 { + // if we're not expecting any files then we're expecting an error + if err == nil { + t.Fatalf("expecting an error; instead got %d files", len(descriptors)) + } + return + } + + checkDescriptorResults(t, descriptors, testCase.expect) + }) + } +} + +func checkDescriptorResults(t *testing.T, descriptors [][]byte, expect []string) { + t.Helper() + if len(descriptors) != len(expect) { + t.Errorf("expected result to contain %d descriptor(s); instead got %d", len(expect), len(descriptors)) + } + names := map[string]struct{}{} + for i, desc := range descriptors { + var descProto descriptorpb.FileDescriptorProto + if err := proto.Unmarshal(desc, &descProto); err != nil { + t.Fatalf("could not unmarshal descriptor result #%d", i+1) + } + names[descProto.GetName()] = struct{}{} + } + actual := make([]string, 0, len(names)) + for name := range names { + actual = append(actual, name) + } + sort.Strings(actual) + sort.Strings(expect) + if !reflect.DeepEqual(actual, expect) { + t.Fatalf("expected file descriptors for %v; instead got %v", expect, actual) + } +} + +type placeholderFile struct { + protoreflect.FileDescriptor +} + +func (placeholderFile) IsPlaceholder() bool { + return true +} From f7dd665a101d60eb789be453ca980d54e5fda7ae Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Tue, 14 Nov 2023 11:01:46 -0500 Subject: [PATCH 4/4] change name set back to accepting descriptors, add dummyFile to test code --- go.work.sum | 3 ++- grpcreflect.go | 12 ++++++------ grpcreflect_test.go | 11 ++++++++++- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/go.work.sum b/go.work.sum index 46199c1..57a60bf 100644 --- a/go.work.sum +++ b/go.work.sum @@ -1,2 +1,3 @@ -connectrpc.com/grpchealth v1.2.0/go.mod h1:fZos12C4p/ZaZC6OwBGZUM+i/fhnRhv75ax/6V/zIeM= connectrpc.com/grpcreflect v1.1.0/go.mod h1:QBX/Lf8oW35iCFCBYlPVTM5xrlKG1dMC+55klHv7YvE= +github.com/golang/protobuf v1.5.0 h1:LUVKkCeviFUMKqHa4tXIIij/lbhnMbP7Fn5wKdKkRh4= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4= diff --git a/grpcreflect.go b/grpcreflect.go index 036a961..430605b 100644 --- a/grpcreflect.go +++ b/grpcreflect.go @@ -317,15 +317,15 @@ type fileDescriptorNameSet struct { names map[string]struct{} } -func (s *fileDescriptorNameSet) Insert(path string) { +func (s *fileDescriptorNameSet) Insert(fd protoreflect.FileDescriptor) { if s.names == nil { s.names = make(map[string]struct{}, 1) } - s.names[path] = struct{}{} + s.names[fd.Path()] = struct{}{} } -func (s *fileDescriptorNameSet) Contains(path string) bool { - _, ok := s.names[path] +func (s *fileDescriptorNameSet) Contains(fd protoreflect.FileDescriptor) bool { + _, ok := s.names[fd.Path()] return ok } @@ -343,10 +343,10 @@ func fileDescriptorWithDependencies(rootFile protoreflect.FileDescriptor, sent * if curr.IsPlaceholder() { continue // don't bother serializing placeholders } - if len(results) == 0 || !sent.Contains(curr.Path()) { // always send root fd + if len(results) == 0 || !sent.Contains(curr) { // always send root fd // Mark as sent immediately. If we hit an error marshaling below, there's // no point trying again later. - sent.Insert(curr.Path()) + sent.Insert(curr) encoded, err := proto.Marshal(protodesc.ToFileDescriptorProto(curr)) if err != nil { return nil, err diff --git a/grpcreflect_test.go b/grpcreflect_test.go index 3f4e971..a60b96b 100644 --- a/grpcreflect_test.go +++ b/grpcreflect_test.go @@ -438,7 +438,7 @@ func TestFileDescriptorWithDependencies(t *testing.T) { sent := &fileDescriptorNameSet{} for _, path := range testCase.sent { - sent.Insert(path) + sent.Insert(dummyFile{path: path}) } descriptors, err := fileDescriptorWithDependencies(testCase.root, sent) @@ -486,3 +486,12 @@ type placeholderFile struct { func (placeholderFile) IsPlaceholder() bool { return true } + +type dummyFile struct { + protoreflect.FileDescriptor + path string +} + +func (f dummyFile) Path() string { + return f.path +}