Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enums from other packages as path parameters is missing package name in front of enumValMap. #1203

Closed
kwonalbert opened this issue Apr 8, 2020 · 10 comments · Fixed by #1453

Comments

@kwonalbert
Copy link

Steps you follow to reproduce the error:

I have two packages package_a, and package_b which imports package_a. package_b defines a RPC that uses a message from package_a, and uses an enum from the said message in the path. E.g.,

// This file is in "/package_a/a.proto"
syntax = "proto3";
package package_a;

enum AEnum {
  ZERO = 0;
  ONE = 1;
  TWO = 2;
}

message AMessage {
  AEnum a = 1;
  string data = 2;
}
// This file is in "/package_b/b.proto"
syntax = "proto3";
package package_b;

import "google/api/annotations.proto";

import "package_a/a.proto";

service BRPC {
  rpc TestEnumFromAnotherPackageInPath(Request) returns (Response) {
    option (google.api.http) = {
      post: "/test/{messagea.a}"
      body: "*"
    };
  }
}

message Request {
  package_a.AMessage messagea = 1;
  string other_data = 2;
}

message Response {
  string some_data = 1;
}

The code that's generated for package_b looks like the following:

// Code generated by protoc-gen-grpc-gateway. DO NOT EDIT.
// source: package_b/b.proto

/*
Package package_b is a reverse proxy.

It translates gRPC into RESTful JSON APIs.
*/
package package_b

import (
	"context"
	"io"
	"net/http"
	"package_a"

	"github.com/golang/protobuf/descriptor"
	"github.com/golang/protobuf/proto"
	"github.com/grpc-ecosystem/grpc-gateway/runtime"
	"github.com/grpc-ecosystem/grpc-gateway/utilities"
	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/grpclog"
	"google.golang.org/grpc/status"
)

// Suppress "imported and not used" errors
var _ codes.Code
var _ io.Reader
var _ status.Status
var _ = runtime.String
var _ = utilities.NewDoubleArray
var _ = descriptor.ForMessage

func request_BRPC_TestEnumFromAnotherPackageInPath_0(ctx context.Context, marshaler runtime.Marshaler, client BRPCClient, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
	var protoReq Request
	var metadata runtime.ServerMetadata

	newReader, berr := utilities.IOReaderFactory(req.Body)
	if berr != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
	}
	if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

	var (
		val string
		e   int32
		ok  bool
		err error
		_   = err
	)

	val, ok = pathParams["messagea.a"]
	if !ok {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "messagea.a")
	}

	err = runtime.PopulateFieldFromPath(&protoReq, "messagea.a", val)

	e, err = runtime.Enum(val, AEnum_value)

	if err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "messagea.a", err)
	}

	protoReq.Messagea.A = AEnum(e)

	msg, err := client.TestEnumFromAnotherPackageInPath(ctx, &protoReq, grpc.Header(&metadata.HeaderMD), grpc.Trailer(&metadata.TrailerMD))
	return msg, metadata, err

}

func local_request_BRPC_TestEnumFromAnotherPackageInPath_0(ctx context.Context, marshaler runtime.Marshaler, server BRPCServer, req *http.Request, pathParams map[string]string) (proto.Message, runtime.ServerMetadata, error) {
	var protoReq Request
	var metadata runtime.ServerMetadata

	newReader, berr := utilities.IOReaderFactory(req.Body)
	if berr != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", berr)
	}
	if err := marshaler.NewDecoder(newReader()).Decode(&protoReq); err != nil && err != io.EOF {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "%v", err)
	}

	var (
		val string
		e   int32
		ok  bool
		err error
		_   = err
	)

	val, ok = pathParams["messagea.a"]
	if !ok {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "missing parameter %s", "messagea.a")
	}

	err = runtime.PopulateFieldFromPath(&protoReq, "messagea.a", val)

	e, err = runtime.Enum(val, AEnum_value)

	if err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "messagea.a", err)
	}

	protoReq.Messagea.A = AEnum(e)

	msg, err := server.TestEnumFromAnotherPackageInPath(ctx, &protoReq)
	return msg, metadata, err

}

// RegisterBRPCHandlerServer registers the http handlers for service BRPC to "mux".
// UnaryRPC     :call BRPCServer directly.
// StreamingRPC :currently unsupported pending https://github.com/grpc/grpc-go/issues/906.
func RegisterBRPCHandlerServer(ctx context.Context, mux *runtime.ServeMux, server BRPCServer) error {

	mux.Handle("POST", pattern_BRPC_TestEnumFromAnotherPackageInPath_0, func(w http.ResponseWriter, req *http.Request, pathParams map[string]string) {
		ctx, cancel := context.WithCancel(req.Context())
		defer cancel()
		inboundMarshaler, outboundMarshaler := runtime.MarshalerForRequest(mux, req)
		rctx, err := runtime.AnnotateIncomingContext(ctx, mux, req)
		if err != nil {
			runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
			return
		}
		resp, md, err := local_request_BRPC_TestEnumFromAnotherPackageInPath_0(rctx, inboundMarshaler, server, req, pathParams)
		ctx = runtime.NewServerMetadataContext(ctx, md)
		if err != nil {
			runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
			return
		}

		forward_BRPC_TestEnumFromAnotherPackageInPath_0(ctx, mux, outboundMarshaler, w, req, resp, mux.GetForwardResponseOptions()...)

	})

	return nil
}

// RegisterBRPCHandlerFromEndpoint is same as RegisterBRPCHandler but
// automatically dials to "endpoint" and closes the connection when "ctx" gets done.
func RegisterBRPCHandlerFromEndpoint(ctx context.Context, mux *runtime.ServeMux, endpoint string, opts []grpc.DialOption) (err error) {
	conn, err := grpc.Dial(endpoint, opts...)
	if err != nil {
		return err
	}
	defer func() {
		if err != nil {
			if cerr := conn.Close(); cerr != nil {
				grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
			}
			return
		}
		go func() {
			<-ctx.Done()
			if cerr := conn.Close(); cerr != nil {
				grpclog.Infof("Failed to close conn to %s: %v", endpoint, cerr)
			}
		}()
	}()

	return RegisterBRPCHandler(ctx, mux, conn)
}

// RegisterBRPCHandler registers the http handlers for service BRPC to "mux".
// The handlers forward requests to the grpc endpoint over "conn".
func RegisterBRPCHandler(ctx context.Context, mux *runtime.ServeMux, conn *grpc.ClientConn) error {
	return RegisterBRPCHandlerClient(ctx, mux, NewBRPCClient(conn))
}

// RegisterBRPCHandlerClient registers the http handlers for service BRPC
// to "mux". The handlers forward requests to the grpc endpoint over the given implementation of "BRPCClient".
// Note: the gRPC framework executes interceptors within the gRPC handler. If the passed in "BRPCClient"
// doesn't go through the normal gRPC flow (creating a gRPC client etc.) then it will be up to the passed in
// "BRPCClient" to call the correct interceptors.
func RegisterBRPCHandlerClient(ctx context.Context, mux *runtime.ServeMux, client BRPCClient) error {

	mux.Handle("POST", pattern_BRPC_TestEnumFromAnotherPackageInPath_0, func(w http.ResponseWriter, req *http.Request, pathParams map[string]string) {
		ctx, cancel := context.WithCancel(req.Context())
		defer cancel()
		inboundMarshaler, outboundMarshaler := runtime.MarshalerForRequest(mux, req)
		rctx, err := runtime.AnnotateContext(ctx, mux, req)
		if err != nil {
			runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
			return
		}
		resp, md, err := request_BRPC_TestEnumFromAnotherPackageInPath_0(rctx, inboundMarshaler, client, req, pathParams)
		ctx = runtime.NewServerMetadataContext(ctx, md)
		if err != nil {
			runtime.HTTPError(ctx, mux, outboundMarshaler, w, req, err)
			return
		}

		forward_BRPC_TestEnumFromAnotherPackageInPath_0(ctx, mux, outboundMarshaler, w, req, resp, mux.GetForwardResponseOptions()...)

	})

	return nil
}

var (
	pattern_BRPC_TestEnumFromAnotherPackageInPath_0 = runtime.MustPattern(runtime.NewPattern(1, []int{2, 0, 1, 0, 4, 1, 5, 1}, []string{"test", "messagea.a"}, "", runtime.AssumeColonVerbOpt(true)))
)

var (
	forward_BRPC_TestEnumFromAnotherPackageInPath_0 = runtime.ForwardResponseMessage
)

This causes compile time error due to AEnum_value and AEnum being undefined (they are defined in package_a).

What did you expect to happen instead:

It should have package_a. in front of AEnum_value and AEnum, which seems to make it compile okay.

What's your theory on why it isn't working:

It seems that when the code for an enum that is part of a message from another package is generated for path parameters, it's omitting the package name in the front, and assumes the enum is part of the current package.

@johanbrandhorst
Copy link
Collaborator

Nice find! Would you be interested in submitting a fix for this?

@kwonalbert
Copy link
Author

Sure, I'd be happy to, though it might take me a bit of time to get started on it (the earliest I can start is probably next weekend).

@kwonalbert
Copy link
Author

I'm waiting on my company to hear back about the CLA. Meanwhile, my fix is at https://github.com/kwonalbert/grpc-gateway. It was just some small updates to the template for codegen.

@johanbrandhorst
Copy link
Collaborator

Great work, looking forward to the PR!

@carvalhorr
Copy link

Really nice find, @kwonalbert . I run into this problem in my first attempt to use grpc-gateway. Looking forward to seeing the fix merged.

@sgtsquiggs
Copy link
Contributor

Bump; I have this same issue and would love this merged into the 1.x branch

@sgtsquiggs
Copy link
Contributor

This change does not work: kwonalbert@14e42a6

You end up with:

	e, err = runtime.Enum(val, path/to/specific/package.Table_value)

	if err != nil {
		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", "table", err)
	}

	protoReq.Table = path / to / specific / package.Table(e)

@sgtsquiggs
Copy link
Contributor

This change worked for me:

diff --git a/protoc-gen-grpc-gateway/internal/gengateway/template.go b/protoc-gen-grpc-gateway/internal/gengateway/template.go
index 392110a..22952f1 100644
--- a/protoc-gen-grpc-gateway/internal/gengateway/template.go
+++ b/protoc-gen-grpc-gateway/internal/gengateway/template.go
@@ -359,10 +359,10 @@ var (
 {{if $param.IsNestedProto3}}
 	err = runtime.PopulateFieldFromPath(&protoReq, {{$param | printf "%q"}}, val)
 	{{if $enum}}
-		e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}_value)
+		e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}_value)
 	{{end}}
 {{else if $enum}}
-	e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}_value)
+	e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}_value)
 {{else}}
 	{{$param.AssignableExpr "protoReq"}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}})
 {{end}}
@@ -370,13 +370,13 @@ var (
 		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", {{$param | printf "%q"}}, err)
 	}
 {{if and $enum $param.IsRepeated}}
-	s := make([]{{$enum.GoType $param.Target.Message.File.GoPkg.Path}}, len(es))
+	s := make([]{{$enum.GoType $param.Method.Service.File.GoPkg.Path}}, len(es))
 	for i, v := range es {
-		s[i] = {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}(v)
+		s[i] = {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}(v)
 	}
 	{{$param.AssignableExpr "protoReq"}} = s
 {{else if $enum}}
-	{{$param.AssignableExpr "protoReq"}} = {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}(e)
+	{{$param.AssignableExpr "protoReq"}} = {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}(e)
 {{end}}
 	{{end}}
 {{end}}
@@ -521,10 +521,10 @@ func local_request_{{.Method.Service.GetName}}_{{.Method.GetName}}_{{.Index}}(ct
 {{if $param.IsNestedProto3}}
 	err = runtime.PopulateFieldFromPath(&protoReq, {{$param | printf "%q"}}, val)
 	{{if $enum}}
-		e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}_value)
+		e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}_value)
 	{{end}}
 {{else if $enum}}
-	e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}_value)
+	e{{if $param.IsRepeated}}s{{end}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}}, {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}_value)
 {{else}}
 	{{$param.AssignableExpr "protoReq"}}, err = {{$param.ConvertFuncExpr}}(val{{if $param.IsRepeated}}, {{$binding.Registry.GetRepeatedPathParamSeparator | printf "%c" | printf "%q"}}{{end}})
 {{end}}
@@ -532,13 +532,13 @@ func local_request_{{.Method.Service.GetName}}_{{.Method.GetName}}_{{.Index}}(ct
 		return nil, metadata, status.Errorf(codes.InvalidArgument, "type mismatch, parameter: %s, error: %v", {{$param | printf "%q"}}, err)
 	}
 {{if and $enum $param.IsRepeated}}
-	s := make([]{{$enum.GoType $param.Target.Message.File.GoPkg.Path}}, len(es))
+	s := make([]{{$enum.GoType $param.Method.Service.File.GoPkg.Path}}, len(es))
 	for i, v := range es {
-		s[i] = {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}(v)
+		s[i] = {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}(v)
 	}
 	{{$param.AssignableExpr "protoReq"}} = s
 {{else if $enum}}
-	{{$param.AssignableExpr "protoReq"}} = {{$enum.GoType $param.Target.Message.File.GoPkg.Path}}(e)
+	{{$param.AssignableExpr "protoReq"}} = {{$enum.GoType $param.Method.Service.File.GoPkg.Path}}(e)
 {{end}}
 	{{end}}
 {{end}}

@kwonalbert
Copy link
Author

That change seems better than what I was doing. I still haven't heard back about the CLA from my company, so feel free to file a PR with your change for this instead..

@johanbrandhorst
Copy link
Collaborator

Thanks Matthew, would you like to contribute a PR with this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants