Skip to content

Commit

Permalink
protoparse should not return dynamic extensions, take 2: must also ex…
Browse files Browse the repository at this point in the history
…amine dependencies
  • Loading branch information
jhump committed Aug 23, 2023
1 parent 0880b17 commit f770609
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 8 deletions.
3 changes: 1 addition & 2 deletions desc/protoparse/options_test.go
Expand Up @@ -3,7 +3,6 @@ package protoparse
import (
"fmt"
"io"
"io/ioutil"
"os"
"strings"
"testing"
Expand Down Expand Up @@ -147,7 +146,7 @@ func TestOptionsInUnlinkedFiles(t *testing.T) {
func accessorFor(name, contents string) FileAccessor {
return func(n string) (io.ReadCloser, error) {
if n == name {
return ioutil.NopCloser(strings.NewReader(contents)), nil
return io.NopCloser(strings.NewReader(contents)), nil
}
return nil, os.ErrNotExist
}
Expand Down
27 changes: 21 additions & 6 deletions desc/protoparse/parser.go
Expand Up @@ -179,11 +179,10 @@ func (p Parser) ParseFiles(filenames ...string) ([]*desc.FileDescriptor, error)
}

fds := make([]protoreflect.FileDescriptor, len(results))
alreadySeen := make(map[string]struct{}, len(results))
for i, res := range results {
if linkRes, ok := res.(linker.Result); ok {
removeDynamicExtensions(linkRes.FileDescriptorProto())
}
fds[i] = results[i]
removeDynamicExtensions(res, alreadySeen)
fds[i] = res
}
return desc.WrapFiles(fds)
}
Expand Down Expand Up @@ -261,7 +260,7 @@ func (p Parser) ParseFilesButDoNotLink(filenames ...string) ([]*descriptorpb.Fil
if err != nil {
return nil, err
}
removeDynamicExtensions(protos[i])
removeDynamicExtensionsFromProto(protos[i])
}
if p.IncludeSourceCodeInfo {
protos[i].SourceCodeInfo = sourceinfo.GenerateSourceInfo(res.AST(), optsIndex)
Expand Down Expand Up @@ -597,7 +596,23 @@ func fixupFilenames(protos map[string]parser.Result) map[string]parser.Result {
return revisedProtos
}

func removeDynamicExtensions(fd *descriptorpb.FileDescriptorProto) {
func removeDynamicExtensions(fd protoreflect.FileDescriptor, alreadySeen map[string]struct{}) {
if _, ok := alreadySeen[fd.Path()]; ok {
// already processed
return
}
alreadySeen[fd.Path()] = struct{}{}
res, ok := fd.(linker.Result)
if ok {
removeDynamicExtensionsFromProto(res.FileDescriptorProto())
}
// also remove extensions from dependencies
for i, length := 0, fd.Imports().Len(); i < length; i++ {
removeDynamicExtensions(fd.Imports().Get(i).FileDescriptor, alreadySeen)
}
}

func removeDynamicExtensionsFromProto(fd *descriptorpb.FileDescriptorProto) {
// protocompile returns descriptors with dynamic extension fields for custom options.
// But protoparse only used known custom options and everything else defined in the
// sources would be stored as unrecognized fields. So to bridge the difference in
Expand Down
31 changes: 31 additions & 0 deletions desc/protoparse/parser_test.go
Expand Up @@ -7,14 +7,18 @@ import (
"io"
"io/ioutil"
"os"
"path/filepath"
"sort"
"strings"
"testing"

"github.com/bufbuild/protocompile/parser"
"github.com/bufbuild/protocompile/reporter"
"github.com/golang/protobuf/proto"
"google.golang.org/protobuf/types/descriptorpb"

"github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/internal/testprotos"
"github.com/jhump/protoreflect/internal/testutil"
)

Expand Down Expand Up @@ -370,6 +374,33 @@ func TestParseFilesWithDependencies(t *testing.T) {
})
}

func TestParseFilesReturnsKnownExtensions(t *testing.T) {
accessor := func(filename string) (io.ReadCloser, error) {
if filename == "desc_test3.proto" {
return io.NopCloser(strings.NewReader(`
syntax = "proto3";
import "desc_test_complex.proto";
message Foo {
foo.bar.Simple field = 1;
}
`)), nil
}
return os.Open(filepath.Join("../../internal/testprotos", filename))
}
p := Parser{
Accessor: accessor,
}
fds, err := p.ParseFiles("desc_test3.proto")
testutil.Ok(t, err)
fd := fds[0].GetDependencies()[0]
md := fd.FindMessage("foo.bar.Test.Nested._NestedNested")
testutil.Require(t, md != nil)
val, err := proto.GetExtension(md.GetOptions(), testprotos.E_Rept)
testutil.Ok(t, err)
_, ok := val.([]*testprotos.Test)
testutil.Require(t, ok)
}

func TestParseCommentsBeforeDot(t *testing.T) {
accessor := FileContentsFromMap(map[string]string{
"test.proto": `
Expand Down

0 comments on commit f770609

Please sign in to comment.