New proposal: support for the google.api.HttpBody #904

wimspaargaren · 2019-03-11T13:09:49Z

No description provided.

googlebot · 2019-03-11T13:09:54Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

go.sum

johanbrandhorst

This is great, thank you so much for this! A couple of comments and I'm also wondering if you could put together something for the documentation as well? Along the lines of the patch examples: https://grpc-ecosystem.github.io/grpc-gateway/docs/patch.html.

johanbrandhorst · 2019-03-11T13:36:55Z

runtime/marshaler.go

@@ -16,7 +16,7 @@ type Marshaler interface {
 	// 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(v interface{}) string


I don't think we can change this without breaking backwards compatibility. Could we add a new interface with this new method and then type assert at marshalling time? That way we could support both new and old marshallers.

Changed it back and assert it at marshalling time. What do you think of this implementation?

runtime/marshal_httpbodyproto.go

wimspaargaren · 2019-03-11T14:09:22Z

I signed it!

googlebot · 2019-03-11T14:09:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

wimspaargaren · 2019-03-11T14:31:53Z

I signed it!

googlebot · 2019-03-11T14:31:59Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

go.mod

johanbrandhorst · 2019-03-11T14:32:08Z

runtime/marshal_httpbodyproto.go

+// the actual message used as the response. If not, then this will
+// simply fallback to the JSONPb marshaler.
+type HTTPBodyMarshaler struct {
+	DefaultMarshaler Marshaler


If you embed this directly you can avoid defining the methods which just call the underlying implementations:

type HTTPBodyMarshaler struct { Marshaler } func SetHTTPBodyMarshaler(serveMux *ServeMux) { serveMux.marshalers.mimeMap[MIMEWildcard] = &HTTPBodyMarshaler{ Marshaler: &JSONPb{OrigName: true}, } }

johanbrandhorst · 2019-03-11T14:32:32Z

runtime/marshal_httpbodyproto_test.go

+	"bytes"
+	"github.com/grpc-ecosystem/grpc-gateway/runtime"
+	"google.golang.org/genproto/googleapis/api/httpbody"
+	"testing"


Sorry, same thing about the imports please.

johanbrandhorst · 2019-03-11T15:00:02Z

A couple of comments and I'm also wondering if you could put together something for the documentation as well? Along the lines of the patch examples: https://grpc-ecosystem.github.io/grpc-gateway/docs/patch.html.

What do you think about this?

codecov-io · 2019-03-11T15:20:09Z

Codecov Report

Merging #904 into master will decrease coverage by 0.07%.
The diff coverage is 41.37%.

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   53.61%   53.54%   -0.08%     
==========================================
  Files          39       40       +1     
  Lines        3937     3961      +24     
==========================================
+ Hits         2111     2121      +10     
- Misses       1629     1642      +13     
- Partials      197      198       +1

Impacted Files	Coverage Δ
runtime/handler.go	`40.62% <0%> (-0.98%)`	⬇️
runtime/proto_errors.go	`0% <0%> (ø)`	⬆️
runtime/errors.go	`44.73% <50%> (-1.1%)`	⬇️
runtime/marshal_httpbodyproto.go	`69.23% <69.23%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6523154...44b123a. Read the comment docs.

runtime/marshal_httpbodyproto.go

johanbrandhorst · 2019-03-11T15:48:01Z

A couple of comments and I'm also wondering if you could put together something for the documentation as well? Along the lines of the patch examples: https://grpc-ecosystem.github.io/grpc-gateway/docs/patch.html.

What do you think about this?

Bump :D

wimspaargaren · 2019-03-11T15:52:52Z

Not sure if it is added at the right place, but I added a small README for the httpbody :)

johanbrandhorst

Great work, just a few comments on the new docs page.

docs/_docs/httpbody.md

johanbrandhorst

Thanks for picking this up!

wimspaargaren · 2019-03-11T16:19:12Z

No problem!

johanbrandhorst · 2019-03-29T17:06:43Z

I think that'd have to be part of your proto file annotation:

get: "/picture.jpg"

* Implementation of HTTPBody marshaler * Rework HTTPBodyMarshaler fallback marshaler * Added tests. * Fix gomod * Fixed tests and added two new ones. * Added marshal backwards compatibility * Last line modsum was removed. * Embedded Marshaler & fixed import order * Removed inherited methods. * Comment improvements * Added small readme. * Doc enhancements * Identation fix * What's wrong with identation

Wim Spaargaren added 4 commits March 11, 2019 12:10

Implementation of HTTPBody marshaler

8d0726f

Rework HTTPBodyMarshaler fallback marshaler

c8ee96e

Added tests.

09f2163

Fix gomod

e22e985

googlebot added the cla: no label Mar 11, 2019

johanbrandhorst reviewed Mar 11, 2019

View reviewed changes

go.sum Outdated Show resolved Hide resolved

johanbrandhorst requested changes Mar 11, 2019

View reviewed changes

Fixed tests and added two new ones.

59db7b4

Added marshal backwards compatibility

8c4c5c6

googlebot added cla: yes and removed cla: no labels Mar 11, 2019

Last line modsum was removed.

c16bd8c

johanbrandhorst reviewed Mar 11, 2019

View reviewed changes

Embedded Marshaler & fixed import order

bf44607

johanbrandhorst requested changes Mar 11, 2019

View reviewed changes

runtime/marshal_httpbodyproto.go Outdated Show resolved Hide resolved

runtime/marshal_httpbodyproto.go Outdated Show resolved Hide resolved

Wim Spaargaren added 2 commits March 11, 2019 16:34

Removed inherited methods.

27a32fe

Comment improvements

9742e53

Added small readme.

ce3b075

johanbrandhorst requested changes Mar 11, 2019

View reviewed changes

docs/_docs/httpbody.md Show resolved Hide resolved

docs/_docs/httpbody.md Outdated Show resolved Hide resolved

Doc enhancements

5eb3f71

johanbrandhorst reviewed Mar 11, 2019

View reviewed changes

docs/_docs/httpbody.md Show resolved Hide resolved

Wim Spaargaren added 2 commits March 11, 2019 17:14

Identation fix

16d2346

What's wrong with identation

44b123a

johanbrandhorst approved these changes Mar 11, 2019

View reviewed changes

johanbrandhorst merged commit 7aca14d into grpc-ecosystem:master Mar 11, 2019

johanbrandhorst mentioned this pull request Mar 11, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New proposal: support for the google.api.HttpBody #904

New proposal: support for the google.api.HttpBody #904

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

johanbrandhorst left a comment

johanbrandhorst Mar 11, 2019

wimspaargaren Mar 11, 2019

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

johanbrandhorst Mar 11, 2019

wimspaargaren Mar 11, 2019

johanbrandhorst Mar 11, 2019

wimspaargaren Mar 11, 2019

johanbrandhorst commented Mar 11, 2019

codecov-io commented Mar 11, 2019 •

edited

Loading

johanbrandhorst commented Mar 11, 2019

wimspaargaren commented Mar 11, 2019

johanbrandhorst left a comment

johanbrandhorst left a comment

wimspaargaren commented Mar 11, 2019

johanbrandhorst commented Mar 29, 2019 •

edited

Loading

New proposal: support for the google.api.HttpBody #904

New proposal: support for the google.api.HttpBody #904

Conversation

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst Mar 11, 2019

Choose a reason for hiding this comment

wimspaargaren Mar 11, 2019

Choose a reason for hiding this comment

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

wimspaargaren commented Mar 11, 2019

googlebot commented Mar 11, 2019

johanbrandhorst Mar 11, 2019

Choose a reason for hiding this comment

wimspaargaren Mar 11, 2019

Choose a reason for hiding this comment

johanbrandhorst Mar 11, 2019

Choose a reason for hiding this comment

wimspaargaren Mar 11, 2019

Choose a reason for hiding this comment

johanbrandhorst commented Mar 11, 2019

codecov-io commented Mar 11, 2019 • edited Loading

Codecov Report

johanbrandhorst commented Mar 11, 2019

wimspaargaren commented Mar 11, 2019

johanbrandhorst left a comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

wimspaargaren commented Mar 11, 2019

johanbrandhorst commented Mar 29, 2019 • edited Loading

codecov-io commented Mar 11, 2019 •

edited

Loading

johanbrandhorst commented Mar 29, 2019 •

edited

Loading