proxy: inconsistent error messages between unary and stream response

[thurt]

i am using google.golang.org/grpc/status to create errors sent in server responses. However, i noticed that my client is receiving different messages depending on whether the rpc has a unary or stream response.

example body response from a unary rpc:

{
    "error":"Unauthenticated",
    "code":16
}

example body response from a stream rpc:

{
    "error":{
        "grpc_code":16,
        "http_code":401,
        "message":"rpc error: code = Unauthenticated desc = Unauthenticated",
        "http_status":"Unauthorized"
    }
}

I expected the response body for an error to be the same message shape for both unary and stream rpcs.

[thurt]

it looks like the unary rpc error message is defined here (inside DefaultHTTPError)

grpc-gateway/runtime/errors.go

Line 93 in 7542c76

body := &errorBody{

and the stream rpc error message is defined here (inside streamChunk)

grpc-gateway/runtime/handler.go

Line 176 in 8db8c1a

return map[string]proto.Message{

[johanbrandhorst]

I agree that this is a problem. The reason for the streaming error being the way it is seems to be to make it fit into the for loop that writes to the ResponseWriter: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/handler.go#L63.

I think actually the solution to this problem is to move away from our custom error types and just use the status.Proto(), which is perfectly marshallable and consistent. It does not contain any HTTP translations of course, but tbh we're already translating the gRPC Code to a HTTP code in the actual response code, so I don't think it's necessary. It would mean we can remove the stream_chunk.proto type altogether.

The {"error": <actual error>} object key probably has to stay, to differentiate from normal messages (which use the {"result": <actual message>} key).

@tmc @achew22 thoughts on this? Would obviously break backwards compatibility in terms of the message returned from errors, but some change here is obviously needed.

[johanbrandhorst]

Making this change makes DefaultHTTPProtoErrorHandler and DefaultHTTPError almost identical. In general, I think this area could do with an overhaul, and will necessitate a 2.0 release.

[achew22]

I'm generally supportive of any experimentation people want to make in the handlers as long as they provide them as an alternative to the default. The breaking change would be modifying the default return type in the error case, but adding a new handler is totally kosher in my book.

@johanbrandhorst, would you like to play around with upstreaming that style of error handling into grpc-gateway?

[johanbrandhorst]

@achew22 please review #560 and #561 for initial, backward compatible changes.

[johanbrandhorst]

I drafted #564 for returning status.Proto() from streams, but I feel more confident baout #560 and #561.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.