Adress some late codereview feedback on #238

doc.go

@@ -63,6 +63,15 @@ Or it can be loaded from a file using -f or --file:
 
 	$ yab -p localhost:9787 -t kv.thrift kv KeyValue::Get --file req.yaml
 
+To make a proto requrest, specify a compiled FileDescriptorSet and pass the
+fully qualified service an method name as procedure.
+
+	$ yab -p localhost:5435 --file-descriptor-set-bin proto.bin \
+	    service package.Service/Method -r '{"key": "value"}'
+
+A FileDescriptorSet can be generated using the --descriptor_set_out= flag
+in protoc.
+
 Request options can also be specified in a YAML file, e.g., get.yab:
 
 	service: kv

encoding/protobuf.go

@@ -20,26 +20,26 @@ type protoSerializer struct {
 
 // NewProtobuf returns a protobuf serializer.
 func NewProtobuf(fullMethodName string, source protobuf.DescriptorProvider) (Serializer, error) {
-	svc, mth, err := splitMethod(fullMethodName)
+	serviceName, methodName, err := splitMethod(fullMethodName)
 	if err != nil {
 		return nil, err
 	}
-	dsc, err := source.FindSymbol(svc)
+	service, err := source.FindSymbol(serviceName)
 	if err != nil {
-		return nil, fmt.Errorf("failed to query for service for symbol %q: %v", svc, err)
+		return nil, fmt.Errorf("failed to query for service for symbol %q: %v", serviceName, err)
 	}
-	sd, ok := dsc.(*desc.ServiceDescriptor)
+	serviceDescriptor, ok := service.(*desc.ServiceDescriptor)
 	if !ok {
-		return nil, fmt.Errorf("target server does not expose service %q", svc)
+		return nil, fmt.Errorf("target server does not expose service %q", serviceName)
 	}
-	mtd := sd.FindMethodByName(mth)
-	if mtd == nil {
-		return nil, fmt.Errorf("service %q does not include a method named %q", svc, mth)
+	methodDescriptor := serviceDescriptor.FindMethodByName(methodName)
+	if methodDescriptor == nil {
+		return nil, fmt.Errorf("service %q does not include a method named %q", serviceName, methodName)
 	}
 	return &protoSerializer{
-		serviceName: svc,
-		methodName:  mth,
-		method:      mtd,
+		serviceName: serviceName,
+		methodName:  methodName,
+		method:      methodDescriptor,
 	}, nil
 }
 
@@ -57,7 +57,7 @@ func (p protoSerializer) Request(body []byte) (*transport.Request, error) {
 		return nil, fmt.Errorf("could marshal message of type %q: %v", p.method.GetInputType().GetFullyQualifiedName(), err)
 	}
 	return &transport.Request{
-		Method: fmt.Sprintf("%s::%s", p.serviceName, p.methodName),
+		Method: p.serviceName + "::" + p.methodName,
 		Body:   bytes,
 	}, nil
 }
@@ -71,11 +71,11 @@ func (p protoSerializer) Response(body *transport.Response) (interface{}, error)
 	if err != nil {
 		return nil, err
 	}
-	var objmap map[string]*json.RawMessage
-	if err = json.Unmarshal(str, &objmap); err != nil {
+	var unmarshaledJson json.RawMessage
+	if err = json.Unmarshal(str, &unmarshaledJson); err != nil {
 		return nil, err
 	}
-	return objmap, nil
+	return unmarshaledJson, nil
 }
 
 func (p protoSerializer) CheckSuccess(body *transport.Response) error {
@@ -86,8 +86,6 @@ func (p protoSerializer) CheckSuccess(body *transport.Response) error {
 func splitMethod(fullMethod string) (svc, method string, err error) {
 	parts := strings.Split(fullMethod, "/")
 	switch len(parts) {
-	case 1:
-		return parts[0], "", nil
 	case 2:
 		return parts[0], parts[1], nil
 	default:

encoding/protobuf_test.go

@@ -22,8 +22,8 @@ func TestNewProtobuf(t *testing.T) {
 		},
 		{
 			desc:   "no method",
-			method: "Bar",
-			errMsg: `service "Bar" does not include a method named ""`,
+			method: "Bar/baq",
+			errMsg: `service "Bar" does not include a method named "baq"`,
 		},
 		{
 			desc:   "invalid method",
@@ -152,7 +152,8 @@ func TestProtobufResponse(t *testing.T) {
 			if tt.errMsg == "" {
 				assert.NoError(t, err, "%v", tt.desc)
 				assert.NotNil(t, got, "%v: Invalid request")
-				r, _ := json.Marshal(got)
+				r, err := json.Marshal(got)
+				assert.NoError(t, err)
 				assert.JSONEq(t, tt.outAsJSON, string(r))
 
 				err = serializer.CheckSuccess(response)

options.go

@@ -40,10 +40,10 @@ type Options struct {
 
 // RequestOptions are request related options
 type RequestOptions struct {
-	Encoding          encoding.Encoding `short:"e" long:"encoding" description:"The encoding of the data, options are: Thrift, proto, JSON, raw. Defaults to Thrift if the method contains '::' or a Thrift file is specified"`
+	Encoding          encoding.Encoding `short:"e" long:"encoding" description:"The encoding of the data, options are: Thrift, proto, JSON, raw. Defaults to Thrift if the method contains '::' or a Thrift file is specified. Defaults to proto if the method contains '/' or a proto filedescriptorset is specified"`
 	ThriftFile        string            `short:"t" long:"thrift" description:"Path of the .thrift file"`
 	FileDescriptorSet []string          `short:"F" long:"file-descriptor-set-bin" description:"A binary file containing a compiled protobuf FileDescriptorSet."`
-	Procedure         string            `long:"procedure" description:"The full Thrift method name (Svc::Method) to invoke"`
+	Procedure         string            `long:"procedure" description:"The full method name to invoke (Thrift: Svc::Method, Proto: package.Service/Method)."`
 	MethodName        stringAlias       `short:"m" long:"method" description:"Alias for procedure"`
 	RequestJSON       string            `short:"r" long:"request" unquote:"false" description:"The request body, in JSON or YAML format"`
 	RequestFile       string            `short:"f" long:"file" description:"Path of a file containing the request body in JSON or YAML"`

transport.go

@@ -37,6 +37,8 @@ import (
 	"github.com/uber/tchannel-go"
 )
 
+const transportUnspecified = "unspecified"
+
 var (
 	errServiceRequired = errors.New("specify a target service using --service")
 	errCallerRequired  = errors.New("caller name is required")
@@ -59,9 +61,9 @@ func remapLocalHost(hostPorts []string) {
 }
 
 func parsePeer(peer string) (protocol, host string) {
-	// If we get a pure host:port, we return unspecified to determine based on encoding
+	// If we get a pure host:port, we return empty protocol to determine based on encoding
 	if _, _, err := net.SplitHostPort(peer); err == nil && !strings.Contains(peer, "://") {
-		return "unspecified", peer
+		return "", peer
 	}
 
 	u, err := url.ParseRequestURI(peer)
@@ -148,7 +150,7 @@ func getTransport(opts TransportOptions, enc encoding.Encoding, tracer opentraci
 		return nil, err
 	}
 
-	if protocol == "unspecified" {
+	if protocol == "" {
 		// Before yab had grpc/protobuf support, peers that did not specify a
 		// protocol assumed tchannel. Keep that default but allow this to be
 		// overridden based on the encoding for saner defaults.

transport_test.go

@@ -44,8 +44,8 @@ func TestParsePeer(t *testing.T) {
 		protocol string
 		host     string
 	}{
-		{"1.1.1.1:1", "unspecified", "1.1.1.1:1"},
-		{"some.host:1234", "unspecified", "some.host:1234"},
+		{"1.1.1.1:1", "", "1.1.1.1:1"},
+		{"some.host:1234", "", "some.host:1234"},
 		{"1.1.1.1", "unknown", ""},
 		{"ftp://1.1.1.1", "ftp", "1.1.1.1"},
 		{"http://1.1.1.1", "http", "1.1.1.1"},
@@ -64,13 +64,14 @@ func TestParsePeer(t *testing.T) {
 
 func TestEnsureSameProtocol(t *testing.T) {
 	tests := []struct {
-		peers []string
-		want  string // if want is empty, expect an error.
+		peers   []string
+		want    string
+		wantErr bool
 	}{
 		{
 			// tchannel host:ports
 			peers: []string{"1.1.1.1:1234", "2.2.2.2:1234"},
-			want:  "unspecified",
+			want:  "",
 		},
 		{
 			// only hosts without port
@@ -87,17 +88,19 @@ func TestEnsureSameProtocol(t *testing.T) {
 		},
 		{
 			// mix of http and https
-			peers: []string{"https://1.1.1.1", "http://2.2.2.2:8080"},
+			peers:   []string{"https://1.1.1.1", "http://2.2.2.2:8080"},
+			wantErr: true,
 		},
 		{
 			// mix of tchannel and unknown
-			peers: []string{"1.1.1.1:1234", "1.1.1.1"},
+			peers:   []string{"1.1.1.1:1234", "1.1.1.1"},
+			wantErr: true,
 		},
 	}
 
 	for _, tt := range tests {
 		got, err := ensureSameProtocol(tt.peers)
-		if tt.want == "" {
+		if tt.wantErr {
 			assert.Error(t, err, "Expect error for %v", tt.peers)
 			continue
 		}