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

add support for the google.api.HttpBody proto as a response #458

Closed
6 changes: 3 additions & 3 deletions runtime/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,6 @@ func (*errorBody) ProtoMessage() {}
func DefaultHTTPError(ctx context.Context, mux *ServeMux, marshaler Marshaler, w http.ResponseWriter, _ *http.Request, err error) {
const fallback = `{"error": "failed to marshal error message"}`

w.Header().Del("Trailer")
w.Header().Set("Content-Type", marshaler.ContentType())

s, ok := status.FromError(err)
if !ok {
s = status.New(codes.Unknown, err.Error())
Expand All @@ -108,6 +105,9 @@ func DefaultHTTPError(ctx context.Context, mux *ServeMux, marshaler Marshaler, w
}
}

w.Header().Del("Trailer")
w.Header().Set("Content-Type", marshaler.ContentType(body))

buf, merr := marshaler.Marshal(body)
if merr != nil {
grpclog.Printf("Failed to marshal error message %q: %v", body, merr)
Expand Down
12 changes: 9 additions & 3 deletions runtime/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func ForwardResponseStream(ctx context.Context, mux *ServeMux, marshaler Marshal
handleForwardResponseServerMetadata(w, mux, md)

w.Header().Set("Transfer-Encoding", "chunked")
w.Header().Set("Content-Type", marshaler.ContentType())
w.Header().Set("Content-Type", marshaler.ContentType(nil))
if err := handleForwardResponseOptions(ctx, w, nil, opts); err != nil {
HTTPError(ctx, mux, marshaler, w, req, err)
return
Expand All @@ -47,6 +47,7 @@ func ForwardResponseStream(ctx context.Context, mux *ServeMux, marshaler Marshal
}

var wroteHeader bool
contentTypeSet := false
for {
resp, err := recv()
if err == io.EOF {
Expand All @@ -61,7 +62,12 @@ func ForwardResponseStream(ctx context.Context, mux *ServeMux, marshaler Marshal
return
}

buf, err := marshaler.Marshal(streamChunk(resp, nil))
chunk := streamChunk(resp, nil)
if !contentTypeSet {
w.Header().Set("Content-Type", marshaler.ContentType(chunk))
contentTypeSet = true
}
buf, err := marshaler.Marshal(chunk)
if err != nil {
grpclog.Printf("Failed to marshal response chunk: %v", err)
handleForwardResponseStreamError(wroteHeader, marshaler, w, err)
Expand Down Expand Up @@ -115,7 +121,7 @@ func ForwardResponseMessage(ctx context.Context, mux *ServeMux, marshaler Marsha

handleForwardResponseServerMetadata(w, mux, md)
handleForwardResponseTrailerHeader(w, md)
w.Header().Set("Content-Type", marshaler.ContentType())
w.Header().Set("Content-Type", marshaler.ContentType(resp))
if err := handleForwardResponseOptions(ctx, w, resp, opts); err != nil {
HTTPError(ctx, mux, marshaler, w, req, err)
return
Expand Down
2 changes: 1 addition & 1 deletion runtime/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func (c *CustomMarshaler) Marshal(v interface{}) ([]byte, error) { return c
func (c *CustomMarshaler) Unmarshal(data []byte, v interface{}) error { return c.m.Unmarshal(data, v) }
func (c *CustomMarshaler) NewDecoder(r io.Reader) runtime.Decoder { return c.m.NewDecoder(r) }
func (c *CustomMarshaler) NewEncoder(w io.Writer) runtime.Encoder { return c.m.NewEncoder(w) }
func (c *CustomMarshaler) ContentType() string { return c.m.ContentType() }
func (c *CustomMarshaler) ContentType(i interface{}) string { return c.m.ContentType(i) }

func TestForwardResponseStreamCustomMarshaler(t *testing.T) {
type msg struct {
Expand Down
74 changes: 74 additions & 0 deletions runtime/marshal_httpbody.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package runtime
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you could add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea sure, but let's finish the other discussion first so I don't have to change things too much.


import (
"io"
"reflect"

"github.com/golang/protobuf/proto"
hb "google.golang.org/genproto/googleapis/api/httpbody"
)

var (
backupMarshaler = &JSONPb{OrigName: true}
)

// HTTPBodyMarshaler is a Marshaler which supports marshaling of a
// google.api.HttpBody message as the full response body if it is
// the actual message used as the response. If not, then this will
// simply fallback to the JSONPb marshaler.
type HTTPBodyMarshaler struct{}

// ContentType returns the type specified in the google.api.HttpBody
// proto if "v" is a google.api.HttpBody proto, otherwise returns
// "application/json".
func (*HTTPBodyMarshaler) ContentType(v interface{}) string {
if h := tryHTTPBody(v); h != nil {
return h.GetContentType()
}
return "application/json"
}

// Marshal marshals "v" by returning the body bytes if v is a
// google.api.HttpBody message, or it marshals to JSON.
func (*HTTPBodyMarshaler) Marshal(v interface{}) ([]byte, error) {
if h := tryHTTPBody(v); h != nil {
return h.GetData(), nil
}
return backupMarshaler.Marshal(v)
}

// Unmarshal unmarshals JSON data into "v".
// google.api.HttpBody messages are not supported on the request.
func (*HTTPBodyMarshaler) Unmarshal(data []byte, v interface{}) error {
return backupMarshaler.Unmarshal(data, v)
}

// NewDecoder returns a Decoder which reads JSON stream from "r".
func (*HTTPBodyMarshaler) NewDecoder(r io.Reader) Decoder {
return backupMarshaler.NewDecoder(r)
}

// NewEncoder returns an Encoder which writes JSON stream into "w".
func (*HTTPBodyMarshaler) NewEncoder(w io.Writer) Encoder {
return backupMarshaler.NewEncoder(w)
}

func tryHTTPBody(v interface{}) *hb.HttpBody {
rv := reflect.ValueOf(v)
if rv.Kind() != reflect.Ptr {
return nil
}
for rv.Kind() == reflect.Ptr {
if rv.IsNil() {
rv.Set(reflect.New(rv.Type().Elem()))
}
if rv.Type().ConvertibleTo(typeProtoMessage) {
pb := rv.Interface().(proto.Message)
if proto.MessageName(pb) == "google.api.HttpBody" {
return v.(*hb.HttpBody)
}
}
rv = rv.Elem()
}
return nil
}
2 changes: 1 addition & 1 deletion runtime/marshal_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
type JSONBuiltin struct{}

// ContentType always Returns "application/json".
func (*JSONBuiltin) ContentType() string {
func (*JSONBuiltin) ContentType(v interface{}) string {
return "application/json"
}

Expand Down
2 changes: 1 addition & 1 deletion runtime/marshal_jsonpb.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
type JSONPb jsonpb.Marshaler

// ContentType always returns "application/json".
func (*JSONPb) ContentType() string {
func (*JSONPb) ContentType(v interface{}) string {
return "application/json"
}

Expand Down
4 changes: 2 additions & 2 deletions runtime/marshaler.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ type Marshaler interface {
NewDecoder(r io.Reader) Decoder
// NewEncoder returns an Encoder which writes bytes sequence into "w".
NewEncoder(w io.Writer) Encoder
// ContentType returns the Content-Type which this marshaler is responsible for.
ContentType() string
// ContentType returns the response Content-Type for "v".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on what "v" would be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v is the same thing that Marshal(v interface{}) is passed, I'm honestly just following convention here. should I update comments on Marshal as well? (happy to do that just lmk)

side note, I am not actually sure why all of these interfaces are not instead of type proto.Message which is what is actually being passed...

ContentType(v interface{}) string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a painful breaking change to the API. Is there ANY way to do this without passing in v?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm well the problem is that the content type right now is not dynamic. I suppose we could add another function like ContentTypeForMessage(m proto.Message) string and make that work as a preferred content-type setter. i.e. if ContentTypeForMessage(m proto.Message) string returns a non-empty string, we use it. otherwise we use the existing ContentType() string. now this has a similar problem as it changes a public interface, which would result in compilation errors for implementors anyway, so I'm afraid I don't have a clever solution. this feels like the lesser of two evils to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bumping this - any thoughts here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately adding another method to the interface suffers from the same API breaking change problem so I would rather go with the simpler breaking change if it is possible. Just spitballing cause I don't think it's possible, there isn't a way to make a type union or something, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually wow yea I think it is possible. so via a type union I think we can do something like this - https://play.golang.org/p/G4x7sEtEHbh - which in this case would mean we have something like this probably in this file:

// ContentType returns the Content-Type header value given a Marshaler or TypeAwareMarshaler and the interface to marshal v.
func contentType(m, v interface{}) string {
  switch t := m.(type) {
  case TypeAwareMarshaler:
    return t.ContentTypeForInterface(v)
  case Marshaler:
    return t.ContentType()
  }
  return ""
}

then we'd update all the marshaler.ContentType() calls in this package to contentType(marshaler, v). what do you think of that? I think that is not technically API breaking, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been staring at this and I think you're right. That would not be API breaking. I think we should go that route.

}

// Decoder decodes a byte sequence
Expand Down
2 changes: 1 addition & 1 deletion runtime/marshaler_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var (
acceptHeader = http.CanonicalHeaderKey("Accept")
contentTypeHeader = http.CanonicalHeaderKey("Content-Type")

defaultMarshaler = &JSONPb{OrigName: true}
defaultMarshaler = &HTTPBodyMarshaler{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems incorrect. Why does the default marshaller need to change from this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why shouldn't the default behavior be to treat HTTP body protos specially?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change. I try not to make those except on major version bumps. If you want to be added to the list of 2.0 breaking changes and do it then, I'd be happy to do that.

)

// MarshalerForRequest returns the inbound/outbound marshalers for this request.
Expand Down
10 changes: 5 additions & 5 deletions runtime/marshaler_registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ func TestMarshalerForRequest(t *testing.T) {
mux := runtime.NewServeMux()

in, out := runtime.MarshalerForRequest(mux, r)
if _, ok := in.(*runtime.JSONPb); !ok {
t.Errorf("in = %#v; want a runtime.JSONPb", in)
if _, ok := in.(*runtime.HTTPBodyMarshaler); !ok {
t.Errorf("in = %#v; want a runtime.HTTPBodyMarshaler", in)
}
if _, ok := out.(*runtime.JSONPb); !ok {
t.Errorf("out = %#v; want a runtime.JSONPb", in)
if _, ok := out.(*runtime.HTTPBodyMarshaler); !ok {
t.Errorf("out = %#v; want a runtime.HTTPBodyMarshaler", in)
}

var marshalers [3]dummyMarshaler
Expand Down Expand Up @@ -78,7 +78,7 @@ func TestMarshalerForRequest(t *testing.T) {

type dummyMarshaler struct{}

func (dummyMarshaler) ContentType() string { return "" }
func (dummyMarshaler) ContentType(v interface{}) string { return "" }
func (dummyMarshaler) Marshal(interface{}) ([]byte, error) {
return nil, errors.New("not implemented")
}
Expand Down
9 changes: 5 additions & 4 deletions runtime/proto_errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,16 @@ func DefaultHTTPProtoErrorHandler(ctx context.Context, mux *ServeMux, marshaler
// return Internal when Marshal failed
const fallback = `{"code": 13, "message": "failed to marshal error message"}`

w.Header().Del("Trailer")
w.Header().Set("Content-Type", marshaler.ContentType())

s, ok := status.FromError(err)
if !ok {
s = status.New(codes.Unknown, err.Error())
}

buf, merr := marshaler.Marshal(s.Proto())
pb := s.Proto()
w.Header().Del("Trailer")
w.Header().Set("Content-Type", marshaler.ContentType(pb))

buf, merr := marshaler.Marshal(pb)
if merr != nil {
grpclog.Printf("Failed to marshal error message %q: %v", s.Proto(), merr)
w.WriteHeader(http.StatusInternalServerError)
Expand Down