Repository: jhump/protoreflect
Pull request #488 (2022-02-05 14:46:56)

protoparse: extensions are not allowed to use json_name option
Another rule that protoc validates but that protoparse was not.


Patch:
diff --git a/desc/protoparse/options.go b/desc/protoparse/options.go
index c13cf15..f9f306b 100644
--- a/desc/protoparse/options.go
+++ b/desc/protoparse/options.go
@@ -696,7 +696,7 @@ func interpretFileOptions(l *linker, r *parseResult, fd fileDescriptorish) error
 		}
 	}
 	for _, fld := range fd.GetExtensions() {
-		if err := interpretFieldOptions(l, r, fld); err != nil {
+		if err := interpretFieldOptions(l, r, fld, true); err != nil {
 			return err
 		}
 	}
@@ -740,7 +740,7 @@ func interpretMessageOptions(l *linker, r *parseResult, md msgDescriptorish) err
 		}
 	}
 	for _, fld := range md.GetFields() {
-		if err := interpretFieldOptions(l, r, fld); err != nil {
+		if err := interpretFieldOptions(l, r, fld, false); err != nil {
 			return err
 		}
 	}
@@ -755,7 +755,7 @@ func interpretMessageOptions(l *linker, r *parseResult, md msgDescriptorish) err
 		}
 	}
 	for _, fld := range md.GetNestedExtensions() {
-		if err := interpretFieldOptions(l, r, fld); err != nil {
+		if err := interpretFieldOptions(l, r, fld, true); err != nil {
 			return err
 		}
 	}
@@ -782,7 +782,7 @@ func interpretMessageOptions(l *linker, r *parseResult, md msgDescriptorish) err
 	return nil
 }
 
-func interpretFieldOptions(l *linker, r *parseResult, fld fldDescriptorish) error {
+func interpretFieldOptions(l *linker, r *parseResult, fld fldDescriptorish, isExtension bool) error {
 	opts := fld.GetFieldOptions()
 	if len(opts.GetUninterpretedOption()) > 0 {
 		uo := opts.UninterpretedOption
@@ -804,6 +804,10 @@ func interpretFieldOptions(l *linker, r *parseResult, fld fldDescriptorish) erro
 				if err := r.errs.handleErrorWithPos(optNode.GetValue().Start(), "%s: expecting string value for json_name option", scope); err != nil {
 					return err
 				}
+			} else if isExtension {
+				if err := r.errs.handleErrorWithPos(optNode.GetName().Start(), "%s: option json_name is not allowed on extensions", scope); err != nil {
+					return err
+				}
 			} else {
 				fld.AsFieldDescriptorProto().JsonName = proto.String(string(opt.StringValue))
 			}
diff --git a/grpcreflect/client.go b/grpcreflect/client.go
index 3fca3eb..84b0055 100644
--- a/grpcreflect/client.go
+++ b/grpcreflect/client.go
@@ -159,7 +159,11 @@ func (cr *Client) FileByFilename(filename string) (*desc.FileDescriptor, error)
 			FileByFilename: filename,
 		},
 	}
-	fd, err := cr.getAndCacheFileDescriptors(req, filename, "")
+	accept := func(fd *desc.FileDescriptor) bool {
+		return fd.GetName() == filename
+	}
+
+	fd, err := cr.getAndCacheFileDescriptors(req, filename, "", accept)
 	if isNotFound(err) {
 		// file not found? see if we can look up via alternate name
 		if alternate, ok := internal.StdFileAliases[filename]; ok {
@@ -168,7 +172,7 @@ func (cr *Client) FileByFilename(filename string) (*desc.FileDescriptor, error)
 					FileByFilename: alternate,
 				},
 			}
-			fd, err = cr.getAndCacheFileDescriptors(req, alternate, filename)
+			fd, err = cr.getAndCacheFileDescriptors(req, alternate, filename, accept)
 			if isNotFound(err) {
 				err = fileNotFound(filename, nil)
 			}
@@ -197,7 +201,10 @@ func (cr *Client) FileContainingSymbol(symbol string) (*desc.FileDescriptor, err
 			FileContainingSymbol: symbol,
 		},
 	}
-	fd, err := cr.getAndCacheFileDescriptors(req, "", "")
+	accept := func(fd *desc.FileDescriptor) bool {
+		return fd.FindSymbol(symbol) != nil
+	}
+	fd, err := cr.getAndCacheFileDescriptors(req, "", "", accept)
 	if isNotFound(err) {
 		err = symbolNotFound(symbol, symbolTypeUnknown, nil)
 	} else if e, ok := err.(*elementNotFoundError); ok {
@@ -226,7 +233,10 @@ func (cr *Client) FileContainingExtension(extendedMessageName string, extensionN
 			},
 		},
 	}
-	fd, err := cr.getAndCacheFileDescriptors(req, "", "")
+	accept := func(fd *desc.FileDescriptor) bool {
+		return fd.FindExtension(extendedMessageName, extensionNumber) != nil
+	}
+	fd, err := cr.getAndCacheFileDescriptors(req, "", "", accept)
 	if isNotFound(err) {
 		err = extensionNotFound(extendedMessageName, extensionNumber, nil)
 	} else if e, ok := err.(*elementNotFoundError); ok {
@@ -235,7 +245,7 @@ func (cr *Client) FileContainingExtension(extendedMessageName string, extensionN
 	return fd, err
 }
 
-func (cr *Client) getAndCacheFileDescriptors(req *rpb.ServerReflectionRequest, expectedName, alias string) (*desc.FileDescriptor, error) {
+func (cr *Client) getAndCacheFileDescriptors(req *rpb.ServerReflectionRequest, expectedName, alias string, accept func(*desc.FileDescriptor) bool) (*desc.FileDescriptor, error) {
 	resp, err := cr.send(req)
 	if err != nil {
 		return nil, err
@@ -253,7 +263,7 @@ func (cr *Client) getAndCacheFileDescriptors(req *rpb.ServerReflectionRequest, e
 	// should be the answer). If we're looking for a file by name, we can be
 	// smarter and make sure to grab one by name instead of just grabbing the
 	// first one.
-	var firstFd *dpb.FileDescriptorProto
+	var fds []*dpb.FileDescriptorProto
 	for _, fdBytes := range fdResp.FileDescriptorProto {
 		fd := &dpb.FileDescriptorProto{}
 		if err = proto.Unmarshal(fdBytes, fd); err != nil {
@@ -266,13 +276,6 @@ func (cr *Client) getAndCacheFileDescriptors(req *rpb.ServerReflectionRequest, e
 		}
 
 		cr.cacheMu.Lock()
-		// see if this file was created and cached concurrently
-		if firstFd == nil {
-			if d, ok := cr.filesByName[fd.GetName()]; ok {
-				cr.cacheMu.Unlock()
-				return d, nil
-			}
-		}
 		// store in cache of raw descriptor protos, but don't overwrite existing protos
 		if existingFd, ok := cr.protosByName[fd.GetName()]; ok {
 			fd = existingFd
@@ -280,15 +283,22 @@ func (cr *Client) getAndCacheFileDescriptors(req *rpb.ServerReflectionRequest, e
 			cr.protosByName[fd.GetName()] = fd
 		}
 		cr.cacheMu.Unlock()
-		if firstFd == nil {
-			firstFd = fd
-		}
+
+		fds = append(fds, fd)
 	}
-	if firstFd == nil {
-		return nil, &ProtocolError{reflect.TypeOf(firstFd).Elem()}
+
+	// find the right result from the files returned
+	for _, fd := range fds {
+		result, err := cr.descriptorFromProto(fd)
+		if err != nil {
+			return nil, err
+		}
+		if accept(result) {
+			return result, nil
+		}
 	}
 
-	return cr.descriptorFromProto(firstFd)
+	return nil, status.Errorf(codes.NotFound, "response does not include expected file")
 }
 
 func (cr *Client) descriptorFromProto(fd *dpb.FileDescriptorProto) (*desc.FileDescriptor, error) {


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 1f05f7e..3365e54 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -868,6 +868,16 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"foo.proto:9:6: message foo.bar.Baz: option (foo.bar.any): could not resolve type reference type.googleapis.com/Foo",
 		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto3\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"extend google.protobuf.MessageOptions {\n" +
+					"  string foobar = 10001 [json_name=\"FooBar\"];\n" +
+					"}\n",
+			},
+			"foo.proto:4:26: field foobar: option json_name is not allowed on extensions",
+		},
 	}
 	for i, tc := range testCases {
 		acc := func(filename string) (io.ReadCloser, error) {
diff --git a/grpcreflect/client_test.go b/grpcreflect/client_test.go
index 544cdae..5f361e5 100644
--- a/grpcreflect/client_test.go
+++ b/grpcreflect/client_test.go
@@ -1,12 +1,15 @@
 package grpcreflect
 
 import (
+	"encoding/base64"
 	"fmt"
+	"io"
 	"net"
 	"os"
 	"sort"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	_ "github.com/golang/protobuf/protoc-gen-go/plugin"
 	_ "github.com/golang/protobuf/ptypes/empty"
@@ -16,9 +19,11 @@ import (
 	_ "google.golang.org/genproto/protobuf/ptype"
 	_ "google.golang.org/genproto/protobuf/source_context"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/reflection"
 	rpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha"
 
+	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/internal"
 	"github.com/jhump/protoreflect/internal/testprotos"
 	"github.com/jhump/protoreflect/internal/testutil"
@@ -239,3 +244,126 @@ func TestRecover(t *testing.T) {
 	testutil.Ok(t, err)
 	testutil.Eq(t, true, client.stream != nil && client.stream != stream)
 }
+
+func TestMultipleFiles(t *testing.T) {
+	svr := grpc.NewServer()
+	rpb.RegisterServerReflectionServer(svr, testReflectionServer{})
+
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	testutil.Ok(t, err, "failed to listen")
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	go func() {
+		defer cancel()
+		if err := svr.Serve(l); err != nil {
+			t.Logf("serve returned error: %v", err)
+		}
+	}()
+	time.Sleep(100 * time.Millisecond) // give server a chance to start
+	testutil.Ok(t, ctx.Err(), "failed to start server")
+	defer func() {
+		svr.Stop()
+	}()
+
+	dialCtx, dialCancel := context.WithTimeout(ctx, 3*time.Second)
+	defer dialCancel()
+	cc, err := grpc.DialContext(dialCtx, l.Addr().String(), grpc.WithInsecure(), grpc.WithBlock())
+	testutil.Ok(t, err, "failed ot dial %v", l.Addr().String())
+	cl := rpb.NewServerReflectionClient(cc)
+
+	client := NewClient(ctx, cl)
+	defer client.Reset()
+	svcs, err := client.ListServices()
+	testutil.Ok(t, err, "failed to list services")
+	for _, svc := range svcs {
+		fd, err := client.FileContainingSymbol(svc)
+		testutil.Ok(t, err, "failed to file for service %v", svc)
+		sd := fd.FindSymbol(svc)
+		_, ok := sd.(*desc.ServiceDescriptor)
+		testutil.Require(t, ok, "symbol for %s is not a service descriptor, instead is %T", svc, sd)
+	}
+}
+
+type testReflectionServer struct{}
+
+func (t testReflectionServer) ServerReflectionInfo(server rpb.ServerReflection_ServerReflectionInfoServer) error {
+	const svcA_file = "ChdzYW5kYm94L3NlcnZpY2VfQS5wcm90bxIHc2FuZGJveCIWCghSZXF1ZXN0QRIKCgJpZBgBIAEoBSIYCglSZXNwb25zZUESCwoDc3RyGAEgASgJMj0KCVNlcnZpY2VfQRIwCgdFeGVjdXRlEhEuc2FuZGJveC5SZXF1ZXN0QRoSLnNhbmRib3guUmVzcG9uc2VBYgZwcm90bzM="
+	const svcB_file = "ChdzYW5kYm94L1NlcnZpY2VfQi5wcm90bxIHc2FuZGJveCIWCghSZXF1ZXN0QhIKCgJpZBgBIAEoBSIYCglSZXNwb25zZUISCwoDc3RyGAEgASgJMj0KCVNlcnZpY2VfQhIwCgdFeGVjdXRlEhEuc2FuZGJveC5SZXF1ZXN0QhoSLnNhbmRib3guUmVzcG9uc2VCYgZwcm90bzM="
+
+	for {
+		req, err := server.Recv()
+		if err == io.EOF {
+			return nil
+		} else if err != nil {
+			return err
+		}
+		var resp rpb.ServerReflectionResponse
+		resp.OriginalRequest = req
+		switch req := req.MessageRequest.(type) {
+		case *rpb.ServerReflectionRequest_FileByFilename:
+			switch req.FileByFilename {
+			case "sandbox/service_A.proto":
+				resp.MessageResponse = msgResponseForFiles(svcA_file)
+			case "sandbox/service_B.proto":
+				resp.MessageResponse = msgResponseForFiles(svcB_file)
+			default:
+				resp.MessageResponse = &rpb.ServerReflectionResponse_ErrorResponse{
+					ErrorResponse: &rpb.ErrorResponse{
+						ErrorCode:    int32(codes.NotFound),
+						ErrorMessage: "not found",
+					},
+				}
+			}
+		case *rpb.ServerReflectionRequest_FileContainingSymbol:
+			switch req.FileContainingSymbol {
+			case "sandbox.Service_A":
+				resp.MessageResponse = msgResponseForFiles(svcA_file)
+			case "sandbox.Service_B":
+				// HERE is where we return two files instead of one
+				resp.MessageResponse = msgResponseForFiles(svcA_file, svcB_file)
+			default:
+				resp.MessageResponse = &rpb.ServerReflectionResponse_ErrorResponse{
+					ErrorResponse: &rpb.ErrorResponse{
+						ErrorCode:    int32(codes.NotFound),
+						ErrorMessage: "not found",
+					},
+				}
+			}
+		case *rpb.ServerReflectionRequest_ListServices:
+			resp.MessageResponse = &rpb.ServerReflectionResponse_ListServicesResponse{
+				ListServicesResponse: &rpb.ListServiceResponse{
+					Service: []*rpb.ServiceResponse{
+						{Name: "sandbox.Service_A"},
+						{Name: "sandbox.Service_B"},
+					},
+				},
+			}
+		default:
+			resp.MessageResponse = &rpb.ServerReflectionResponse_ErrorResponse{
+				ErrorResponse: &rpb.ErrorResponse{
+					ErrorCode:    int32(codes.NotFound),
+					ErrorMessage: "not found",
+				},
+			}
+		}
+		if err := server.Send(&resp); err != nil {
+			return err
+		}
+	}
+}
+
+func msgResponseForFiles(files ...string) *rpb.ServerReflectionResponse_FileDescriptorResponse {
+	descs := make([][]byte, len(files))
+	for i, f := range files {
+		b, err := base64.StdEncoding.DecodeString(f)
+		if err != nil {
+			panic(err)
+		}
+		descs[i] = b
+	}
+	return &rpb.ServerReflectionResponse_FileDescriptorResponse{
+		FileDescriptorResponse: &rpb.FileDescriptorResponse{
+			FileDescriptorProto: descs,
+		},
+	}
+}