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

theRealWardo · 2017-09-10T08:58:17Z

#457 fix proof of concept - probably needs some discussion on the bug as this is not very straight forward. I just hacked this together, no tests yet, so not ready for serious review until I hear from someone on the bug (if the general idea here is cool then I'll polish this PR up for submission).

tmc · 2017-09-14T04:52:18Z

Thanks for your contribution! master has a build system fix, can you please rebase this?

theRealWardo · 2017-09-18T18:43:21Z

@tmc sorry for the delay! rebased and pushed finally.

tmc · 2017-12-06T05:49:58Z

@theRealWardo sorry, can you do so again?

achew22 · 2017-12-06T06:01:43Z

runtime/handler.go

@@ -7,7 +7,7 @@ import (
 	"net/textproto"

 	"github.com/golang/protobuf/proto"
-	"github.com/grpc-ecosystem/grpc-gateway/runtime/internal"
+	"github.com/therealwardo/grpc-gateway/runtime/internal"


I don't think we should merge with this sourcing from another repo.

achew22 · 2017-12-06T06:03:50Z

@tmc, I found out something nifty about github the other day. You can git clone [email protected]:theRealWardo/grpc-gateway, check out the branch that has a PR coming your way and then push back onto it. The default PR behavior is to grant the PR recipient write into that branch (which is you). We don't need to impotently wait for a PR author to rebase so we can confirm everything works before merging... yay! 🎆

tmc · 2018-01-02T17:34:05Z

I'd expect this to come with greater test code change/addition.

jinleileiking · 2018-01-18T06:56:01Z

waiting for merge.

jinleileiking · 2018-01-18T06:58:24Z

Needs example to show me how to use it...

achew22 · 2018-01-21T04:02:09Z

runtime/handler.go

 	}

 	var wroteHeader bool
+	ctSet := false


I'm all for short variable names, but this is too far away from the code using it to use a 2 letter abbreviation IMO. "contentTypeSet" would be preferred

achew22 · 2018-01-21T04:04:15Z

runtime/marshal_httpbody.go

@@ -0,0 +1,74 @@
+package runtime


Do you think you could add a test for this?

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

achew22 · 2018-01-21T04:05:32Z

runtime/marshaler.go

@@ -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".


Can you elaborate on what "v" would be?

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...

achew22 · 2018-01-21T04:05:42Z

runtime/marshaler.go

-	// ContentType returns the Content-Type which this marshaler is responsible for.
-	ContentType() string
+	// ContentType returns the response Content-Type for "v".
+	ContentType(v interface{}) string


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

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?

bumping this - any thoughts here?

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?

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?

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.

achew22 · 2018-01-21T04:06:23Z

runtime/marshaler_registry.go

@@ -13,7 +13,7 @@ var (
 	acceptHeader      = http.CanonicalHeaderKey("Accept")
 	contentTypeHeader = http.CanonicalHeaderKey("Content-Type")

-	defaultMarshaler = &JSONPb{OrigName: true}
+	defaultMarshaler = &HTTPBodyMarshaler{}


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

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

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.

jinleileiking · 2018-02-27T07:25:42Z

@theRealWardo Can I modify your codes? If you donot have time, I will make code changes. I like this to be merged.

johanbrandhorst · 2018-09-09T13:37:34Z

This looks like it still needs some work, and may even be considered for v2 if the submitter wants to change the default marshaller.

johanbrandhorst · 2018-10-17T11:13:26Z

Please rebase on master for new CI functionality.

wimspaargaren · 2019-03-08T14:20:48Z

Hi all, Can someone tell me if there's still any work being done to support HttpBody proto as response?

johanbrandhorst · 2019-03-08T14:27:17Z

This PR looks abandoned. Would you like to pick up the mantle @wimspaargaren?

wimspaargaren · 2019-03-08T16:07:27Z

Sure, I know it needs a rebase, but could you enhance a bit if there need to be more adjustments before this can be merged?

johanbrandhorst · 2019-03-08T16:09:57Z

Judging by the comments from @achew22 there still needs to be a decision made whether this should be a backwards compatible change (perhaps under a flag) and so mergable today or whether we want to make it a breaking change in which case it would have to wait until v2. As v2 is currently somewhat unclear, I would suggest the former. In any case, you personally couldn't take over this PR, you'd have to submit a new one because of how the CLA works.

wimspaargaren · 2019-03-08T16:27:10Z

Allright! Flag or env var would work fine for me. I'll propose my own PR on Monday then.

johanbrandhorst · 2019-03-11T16:27:04Z

Superceded by #904

theRealWardo · 2019-03-11T16:49:04Z

awesome! thanks for picking this up @wimspaargaren

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

136b143

theRealWardo force-pushed the google-api-httpbody branch from 10c40d2 to 136b143 Compare September 18, 2017 18:42

theRealWardo added 2 commits November 1, 2017 15:00

Merge branch 'master' of github.com:grpc-ecosystem/grpc-gateway

a605dfe

patch some runtime stuff

78a226c

achew22 requested changes Dec 6, 2017

View reviewed changes

achew22 mentioned this pull request Dec 14, 2017

support google.api.HttpBody #457

Closed

jinleileiking mentioned this pull request Jan 18, 2018

add support for the google.api.HttpBody proto as a request #528

Closed

theRealWardo added 6 commits January 17, 2018 23:57

merge

f0b42c6

fix merge mistake

787e198

fix a couple imports

d611609

fix lint

e318b89

fix httpbodymarshaler

74f758e

Get tests passing

a7ebb37

achew22 reviewed Jan 21, 2018

View reviewed changes

achew22 mentioned this pull request Feb 28, 2018

chunked transfer encoding for streaming responses #562

Closed

theRealWardo added 2 commits March 15, 2018 22:23

merge master

7ae723a

contentTypeSet

c5909a7

googlebot added the cla: yes label Mar 16, 2018

johanbrandhorst added enhancement help wanted waiting on reporter labels Sep 9, 2018

johanbrandhorst closed this Mar 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theRealWardo commented Sep 10, 2017

tmc commented Sep 14, 2017

theRealWardo commented Sep 18, 2017

tmc commented Dec 6, 2017

achew22 Dec 6, 2017

achew22 commented Dec 6, 2017

tmc commented Jan 2, 2018

jinleileiking commented Jan 18, 2018

jinleileiking commented Jan 18, 2018

achew22 Jan 21, 2018

theRealWardo Mar 16, 2018

achew22 Jan 21, 2018

theRealWardo Mar 16, 2018

achew22 Jan 21, 2018

theRealWardo Mar 16, 2018

achew22 Jan 21, 2018

theRealWardo Mar 16, 2018

theRealWardo Mar 30, 2018

achew22 Mar 31, 2018

theRealWardo Mar 31, 2018

achew22 Apr 8, 2018

achew22 Jan 21, 2018

theRealWardo Mar 16, 2018

achew22 Mar 31, 2018

jinleileiking commented Feb 27, 2018

johanbrandhorst commented Sep 9, 2018

johanbrandhorst commented Oct 17, 2018

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 8, 2019

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 8, 2019

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 11, 2019

theRealWardo commented Mar 11, 2019

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

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

Conversation

theRealWardo commented Sep 10, 2017

tmc commented Sep 14, 2017

theRealWardo commented Sep 18, 2017

tmc commented Dec 6, 2017

Choose a reason for hiding this comment

achew22 commented Dec 6, 2017

tmc commented Jan 2, 2018

jinleileiking commented Jan 18, 2018

jinleileiking commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinleileiking commented Feb 27, 2018

johanbrandhorst commented Sep 9, 2018

johanbrandhorst commented Oct 17, 2018

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 8, 2019

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 8, 2019

wimspaargaren commented Mar 8, 2019

johanbrandhorst commented Mar 11, 2019

theRealWardo commented Mar 11, 2019