response_body of http rule is ignored in case of server streaming

[adasari]

HI Team,

response_body field of http rule is ignored in case of server streaming. i checked the template.go and handler.go, seems they are not checking ResponseBody type.

template.go
handler.go

Also noticed stream response is wrapped in a map here. is there any specific reason ?

Thanks

[johanbrandhorst]

Hi Anil, thanks for the issue report! I think when we first added support for the response_body (#712) we didn't consider it in the light of streaming RPCs. There may be a sensible way to implement this for streaming APIs, so lets discuss!

The reason we're using a wrapper by default for streaming responses is so that users can use a single type to marshal the chunks, while still allowing us to return an error at any point in the stream. This means the error has to always be there.

If a user were to use the ResponseBody though, we would allow the user to stream whatever they want.

What ideas do you have for how to implement this? I can't promise it will be merged, but feel free to open a Pull Request and we can take a look together.

[adasari]

Thanks @johanbrandhorst for quick response. can you take a look at following ? let me know it if creates side effects. thanks.
At high level, below changes should work. (not tested yet)

1. Following code in handler.go

var buf []byte
switch {
case resp == nil:
	buf, err = marshaler.Marshal(errorChunk(streamError(ctx, mux.streamErrorHandler, errEmptyResponse)))
default:
	if rb, ok := resp.(responseBody); ok {
		buf, err = marshaler.Marshal(rb.XXX_ResponseBody())
	} else {
		buf, err = marshaler.Marshal(map[string]proto.Message{"result": resp})
	}
}

2. In if block in template.go

{{ if $b.ResponseBody }}
forward_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}}(ctx, mux, outboundMarshaler, w, req, func() (proto.Message, error) { 
	res, err := resp.Recv() 
	return response_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}}{res}, err
}, mux.GetForwardResponseOptions()...)
{{ else }}
forward_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}}(ctx, mux, outboundMarshaler, w, req, func() (proto.Message, error) { return resp.Recv() }, mux.GetForwardResponseOptions()...)
{{end}}

i am yet to find a way to test the changes.

[johanbrandhorst]

It's hard to me to say from here, but we definitely would need a test for this. How about adding a test to the existing test server?

We could add a new RPC that returns a stream of ResponseBody here, and then add a test that uses this here.

[adasari]

cool. will work on it. thanks.

[adasari]

@johanbrandhorst raised CL (#1205). please take a look.

generate step failed to due to proto error. not sure if this is something related to changes. pleae take a look and suggest. thanks.

[johanbrandhorst]

If the generate step failed it will be related to the changes.

[johanbrandhorst]

Please try regenerating locally by following the instructions in CONTRIBUTING.md

[adasari]

after latest pull, seems it is failing in local too. But i didnt understand the error. not sure why it is referring to data in ResponseBodyOut .would you mind helping in pointing the error ?

W0409 14:53:12.460320     419 services.go:38] No HttpRule found for method: ABitOfEverythingService.NoBindings
W0409 14:53:12.467545     419 services.go:38] No HttpRule found for method: AnotherServiceWithNoBindings.NoBindings
--grpc-gateway_out: no field "data" found in ResponseBodyOut

[adasari]

i found the issue. not sure why it is there. let me fix. thanks.

[johanbrandhorst]

Fixed by #1208