Repeated fields in parameter are set as a single string v.s. a slice of string (incompatible with grpc-gateway)

[hyang74]

I am using this client generator to generate clients that can talk to my server via REST/GRPC in an easy approach. And I am using grpc-gateway for routing the HTTP/REST requests to GRPC. I thought both this and grpc-gateway are popular tools and they all generate code from protobuf so that they should be compatible with each other.
However I noticed that the repeated field parameter didn't work well.

Could you please help to check if this is a bug/issue, or if this is a feature that is nice to have?
I think people would be more than happy if the generated REST Clients can be directly used against grpc-gateway!

Thanks!

Environment details

• Programming language: Golang
• OS: MacOS
• Language runtime version: v1.17.13
• Package version:

Steps to reproduce

My proto is similar to this:

rpc List(ListRequest) returns (ListResponse) {
    option (google.api.http) = {
      get: "/v1/{parent=*}/lists"
    };
}

message ListRequest {
    enum Types {
        A=0;
        B=1;
    }
    repeated Types = 1; 
}

So I can send a request with Types = [0, 1].

And in the generated REST client's List method, the Types is set in this way

		if req.GetTypes() != nil {
			params.Add("types", fmt.Sprintf("%v", req.GetTypes()))
		}

This sets a single string [0,1] to the parameter types.

Whereas in the server side, grpc-gateway receives the request and parses the parameter using this method: Parse, where the input values is the request.Form which contains the parameter k-v pair.

And it will parse the above types: "[0,1]" to key: types, value: "[0,1]" (Note, it is a string "[0,1]"). And it will fail saying that [0,1] is not a correct enum type.

My solution is to adjust the above code in the generated REST clients:

		if req.GetTypes() != nil {
			for _,n := range req.GetTypes(){
				params.Add("types", fmt.Sprintf("%v", n))
			}
		}

to add multiple strings for the types and this will work perfectly with grpc-gateway.
This will set the parameter fields with a slice of strings 0, 1 and grpc-gateway will parse and return correct values.

[noahdietz]

Thanks for the bug report. I think you are right and your suggested fix to the generated code seems like it would work. I will see about fixing this soon.

[noahdietz]

This is just a note:
We probably want to change the code for handling repeated fields in the QP here:

gapic-generator-go/internal/gengapic/genrest.go

Lines 452 to 454 in 1f99ec7

	if field.GetLabel() == fieldLabelRepeated {
	// It's a slice, so check for nil
	p("if req%s != nil {", accessor)

.

The paramAdd content is something like params.Add(... so we'd want to either modify that to include the for loop @HuairuoYang suggested, or generate that for loop in place there. Probably the former considering this is what we do for proto-JSON well known types.

[noahdietz]

Relevant gRPC-HTTP Transcoding semantics that the generator is currently violating are documented in the following comments on the google.api.http message:

• https://github.com/googleapis/googleapis/blob/cb8c91471c9c4540aba3b684b1126e3d06b1ef0b/google/api/http.proto#L119-L120
• https://github.com/googleapis/googleapis/blob/cb8c91471c9c4540aba3b684b1126e3d06b1ef0b/google/api/http.proto#L215-L218

This is the standard for the behavior that @HuairuoYang sees in grpc-gateway and that we need to comply with in this code generator.

[hyang74]

Thank you @noahdietz ! Could I ask when would this be fixed and which will it be included in the next release (when)?

[noahdietz]

Thank you @noahdietz ! Could I ask when would this be fixed and which will it be included in the next release (when)?

Just opened a PR :) If merged today, I can release today. Otherwise, it will go out on Monday (we try not to release on Friday).

[hyang74]

Impressive! Thanks!

[noahdietz]

Release to come on Monday @HuairuoYang

[noahdietz]

Should be available in v0.33.2: #1162

[hyang74]

Verified that it works! Thanks again for fast response and execution of the team! @noahdietz