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

grpc-gateway calls golang/protobuf/jsonpb #178

Closed
mwitkow opened this issue May 13, 2016 · 62 comments
Closed

grpc-gateway calls golang/protobuf/jsonpb #178

mwitkow opened this issue May 13, 2016 · 62 comments
Labels

Comments

@mwitkow
Copy link

@mwitkow mwitkow commented May 13, 2016

This particular bug bit us:

To properly serialize/deserailize enums in jsonpb you need to call RegisterEnum. gogo/protobuf-generated messages register with gogo/protobuf/properties.go. However, if you have a library that is not under your control (e.g. gRPC Gateway), it will be using standard golang/jsonpb and thus the golang/protobuf/properties registration. Thus, any library using golang/jsonpb will not know about enums field maps.

Can we have a fix for this? Proposals include:

  • having gogo register by only with golang/protobuf
  • having gogo register with both, or passing it up.
@mwitkow
Copy link
Author

@mwitkow mwitkow commented May 13, 2016

This is a bit of sed that helps:

  # import upstream golang/protobuf as well
  sed -i -E 's~import proto "github.com/gogo/protobuf/proto"~\0\nimport golang_proto "github.com/golang/protobuf/proto~' ${GO_FILE_FULL}
  # Double up all registrations to golang_protobuf
  sed -i -E 's~^([[:space:]]*)proto.Register(.*)$~\0\n\1golang_proto.Register\2~' ${GO_FILE_FULL}
@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented May 13, 2016

Damn that sucks :(
I'll have to think about this more.

I do have an extension that imports golang/protobuf instead of gogo/protobuf, maybe that can help?
This is what protoc-gen-gofast uses.

@tamird
Copy link
Contributor

@tamird tamird commented May 16, 2016

I'm not sure this is a "real" issue - truth is that using golang/protobuf/jsonpb to serialize/deserialize protos generated by gogo/protobuf doesn't work reliably (e.g. it doesn't support non-nullable non-scalar fields), so you really want to use gogo/protobuf/jsonpb.

The workaround isn't so bad, see my solution in cockroachdb/cockroach@2101119

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented May 16, 2016

Ok maybe it just bothers me :-)
On 16 May 2016 9:48 PM, "Tamir Duberstein" notifications@github.com wrote:

I'm not sure this is a "real" issue - truth is that using
golang/protobuf/jsonpb to serialize/deserialize protos generated by
gogo/protobuf doesn't work reliably (e.g. it doesn't support non-nullable
non-scalar fields), so you really want to use gogo/protobuf/jsonpb.

The workaround isn't so bad, see my solution in cockroachdb/cockroach@
2101119
cockroachdb/cockroach@2101119


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#178 (comment)

@mwitkow
Copy link
Author

@mwitkow mwitkow commented May 16, 2016

Tamir, agreed. However if your vendored dependency used
golang/protobuf/jsonpb you're in very much of a pickle.

I think that having golang/protobuf generate MarshalJSON functions that use
jsonpb would be the best solution to the problem.

On Mon, 16 May 2016, 20:53 Walter Schulze, notifications@github.com wrote:

Ok maybe it just bothers me :-)
On 16 May 2016 9:48 PM, "Tamir Duberstein" notifications@github.com
wrote:

I'm not sure this is a "real" issue - truth is that using
golang/protobuf/jsonpb to serialize/deserialize protos generated by
gogo/protobuf doesn't work reliably (e.g. it doesn't support non-nullable
non-scalar fields), so you really want to use gogo/protobuf/jsonpb.

The workaround isn't so bad, see my solution in cockroachdb/cockroach@
2101119
<
cockroachdb/cockroach@2101119


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#178 (comment)


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#178 (comment)

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented May 16, 2016

Sorry I replied to wrong issue
On 16 May 2016 9:53 PM, "Walter Schulze" awalterschulze@gmail.com wrote:

Ok maybe it just bothers me :-)
On 16 May 2016 9:48 PM, "Tamir Duberstein" notifications@github.com
wrote:

I'm not sure this is a "real" issue - truth is that using
golang/protobuf/jsonpb to serialize/deserialize protos generated by
gogo/protobuf doesn't work reliably (e.g. it doesn't support non-nullable
non-scalar fields), so you really want to use gogo/protobuf/jsonpb.

The workaround isn't so bad, see my solution in cockroachdb/cockroach@
2101119
cockroachdb/cockroach@2101119


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#178 (comment)

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented May 17, 2016

Line 58 of jsonpb_marshaler should probably be jsonpb.Unmarshal not
proto.Unmarshal
On 16 May 2016 21:48, "Tamir Duberstein" notifications@github.com wrote:

I'm not sure this is a "real" issue - truth is that using
golang/protobuf/jsonpb to serialize/deserialize protos generated by
gogo/protobuf doesn't work reliably (e.g. it doesn't support non-nullable
non-scalar fields), so you really want to use gogo/protobuf/jsonpb.

The workaround isn't so bad, see my solution in cockroachdb/cockroach@
2101119
cockroachdb/cockroach@2101119


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#178 (comment)

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented May 17, 2016

I also think the generated MarshalJSON functions will solve all a lot of problems.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Sep 10, 2016

Maybe we should make an issue with grpc-gateway to allow us to inject our own jsonpb marshaler codec, just like grpc-go allows you to inject a codec interface?

@awalterschulze awalterschulze changed the title gogo/protobuf should Register with upstream golang/protobuf grpc-gateway calls golang/protobuf/jsonpb Sep 10, 2016
@mwitkow
Copy link
Author

@mwitkow mwitkow commented Sep 12, 2016

gRPC gateway already allows one to do that. It's just confusing as hell.

I actually think that double registration would be the better solution here.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Sep 12, 2016

How is it confusing, maybe you can explain more? Worst case for someone else finding this issue. I haven't used grpc-gateway. I have only read about it.

@mwitkow
Copy link
Author

@mwitkow mwitkow commented Sep 12, 2016

For example: you use the default grpc gateway codes and gogo as your proto
compiler. Unknowingly you pass your messages to grpc gateway. Your enums
are not registered and serialised as ints and you can parse valid proto3 in
json format.

On Mon, 12 Sep 2016, 17:58 Walter Schulze, notifications@github.com wrote:

How is it confusing, maybe you can explain more? Worst case for someone
else finding this issue.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#178 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJNWo6RtqaHe1CLWL2srs9l4-cnwY8LRks5qpYTEgaJpZM4Id0EI
.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Sep 12, 2016

Ok so everything works in that case?

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Sep 13, 2016

Wow that is a lot of quite complex code, but as you said there are other issues with using golang/protobuf/jsonpb with gogoprotobuf generated structures, like non-nullable non-scalar fields. Have you brought this up with grpc-gateway? I would think that there would be an easier interface to implement.

@tamird
Copy link
Contributor

@tamird tamird commented Sep 17, 2016

I haven't, but it's probably worthwhile to do so.

Having to copy marshalNonProtoField is really unfortunate.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Sep 19, 2016

Whoever opens this issue could CC me in on the issue. I would like to at least follow.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Oct 18, 2016

Doesn't seem like the issue was ever opened #212 (comment)

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Oct 28, 2016

We're currently getting a proto: no coders for int message printed on STDERR when returning grpc.Codes over grpc-gateway, is that because of this? We vendored @tamird's workaround and everything else works, but are still seeing those messages. Could someone with a better understanding explain what's going on and preferably raise that issue?

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Oct 29, 2016

I would report this issue if I was using grpc-gateway, but unfortunately I am not. Also I don't perfectly understand. I am guessing. I would suggest putting in a panic at the place this warning it being printed out to find out how this call is happening. And then report this issue.

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Oct 31, 2016

Sounds reasonable, I'll give it a go

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 1, 2016

For reference, I've managed to get a panic traceback, just posting it here before raising it against grpc-gateway:

2016/11/01 11:22:15 http2: panic serving 127.0.0.1:49722: proto: no coders for int
goroutine 427 [running]:
net/http.(*http2serverConn).runHandler.func1(0xc4201381c0, 0xc4201e7f5f, 0xc420378b40)
    /usr/local/go/src/net/http/h2_bundle.go:4304 +0xd1
panic(0x8ac880, 0xc4202a6770)
    /usr/local/go/src/runtime/panic.go:458 +0x243
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/proto.(*Properties).setEncAndDec(0xc42008ef20, 0xbfd8e0, 0x8ac140, 0xc4201e6eb0, 0x1)
    ~/myrepo/vendor/github.com/gogo/protobuf/proto/properties.go:381 +0x382
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/proto.(*Properties).init(0xc42008ef20, 0xbfd8e0, 0x8ac140, 0x88a710, 0x4, 0x88a720, 0x11, 0xc4201e6eb0, 0xc42008ef01)
    ~/myrepo/vendor/github.com/gogo/protobuf/proto/properties.go:713 +0xcb
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/proto.(*Properties).Init(0xc42008ef20, 0xbfd8e0, 0x8ac140, 0x88a710, 0x4, 0x88a720, 0x11, 0xc4201e6eb0)
    ~/myrepo/vendor/github.com/gogo/protobuf/proto/properties.go:699 +0x7f
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/jsonpb.jsonProperties(0x88a710, 0x4, 0x0, 0x0, 0xbfd8e0, 0x8ac140, 0x88a716, 0x28, 0x10, 0xc4202a65a8, ...)
    ~/myrepo/vendor/github.com/gogo/protobuf/jsonpb/jsonpb.go:892 +0xe1
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/jsonpb.(*Marshaler).marshalObject(0xc4202ccf80, 0xc4201e7518, 0x7f9012377670, 0xc4202ae2e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc4202ec000)
    ~/myrepo/vendor/github.com/gogo/protobuf/jsonpb/jsonpb.go:222 +0x406
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/jsonpb.(*Marshaler).Marshal(0xc4202ccf80, 0xbeecc0, 0xc420308380, 0x7f9012377670, 0xc4202ae2e0, 0x1, 0xc420308380)
    ~/myrepo/vendor/github.com/gogo/protobuf/jsonpb/jsonpb.go:79 +0xca
github.com/johanbrandhorst/myrepo/vendor/github.com/gogo/protobuf/jsonpb.(*Marshaler).Marshal-fm(0xbeecc0, 0xc420308380, 0x7f9012377670, 0xc4202ae2e0, 0x8f9501, 0xc4202a62c0)
    ~/myrepo/vendor/github.com/cockroachdb/cockroach/pkg/util/httputil/http.go:71 +0x52
github.com/johanbrandhorst/myrepo/vendor/github.com/cockroachdb/cockroach/pkg/util/protoutil.(*JSONPb).marshal(0xc4202ccf80, 0x8e4580, 0xc4202ae2e0, 0x411a68, 0x20, 0x8f9540, 0x96a701, 0xc4202ae2e0)
    ~/myrepo/vendor/github.com/cockroachdb/cockroach/pkg/util/protoutil/jsonpb_marshal.go:57 +0x118
github.com/johanbrandhorst/myrepo/vendor/github.com/cockroachdb/cockroach/pkg/util/protoutil.(*JSONPb).Marshal(0xc4202ccf80, 0x8e4580, 0xc4202ae2e0, 0x35, 0x10, 0x8ed040, 0xbf75c0, 0xc4202ac720)
    ~/myrepo/vendor/github.com/cockroachdb/cockroach/pkg/util/protoutil/jsonpb_marshal.go:48 +0x3f
github.com/johanbrandhorst/myrepo/vendor/github.com/grpc-ecosystem/grpc-gateway/runtime.DefaultHTTPError(0x7f90123337c0, 0xc4202ac720, 0xbf89a0, 0xc4202ccf80, 0xbf6b00, 0xc4201381c0, 0xc42037e960, 0xbef700, 0xc4202ae2c0)
    ~/myrepo/vendor/github.com/grpc-ecosystem/grpc-gateway/runtime/errors.go:91 +0x1cf
net/http.(*ServeMux).ServeHTTP(0xc420243680, 0xbf6b00, 0xc4201381c0, 0xc42037e960)
    /usr/local/go/src/net/http/server.go:2022 +0x7f
....
<my code>
....
created by net/http.(*http2serverConn).processHeaders
    /usr/local/go/src/net/http/h2_bundle.go:4092 +0x6e2
@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 1, 2016

On second thought, I'd love to have some input from @tamird and @mwitkow about the precise cause of the problem here so I can make an educated issue against grpc-gateway. Any chance you could elaborate a little bit?

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 1, 2016

Wow ok that trace is very interesting.
Could you also print out f *reflect.StructField

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 1, 2016

With

panic(fmt.Sprintf("proto: no coders for Kind: %v, StructField: %v", t1, f))
2016/11/01 12:48:56 http2: panic serving 127.0.0.1:52008: proto: no coders for Kind: int, StructField: &{Code  int protobuf:"bytes,2,name=code" json:"code" 16 [1] fals
e}
@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 1, 2016

Where is it finding this int field?

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 1, 2016

Using the debugger, I get this value for the valueField on ln 177 in gogo/protobuf/jsonpb/jsonpb.go:

valueField
<reflect.StructField>
Name:"Code"
PkgPath:""
Type:<reflect.Type>
Tag:"protobuf:"bytes,2,name=code" json:"code""
Offset:16
Index:<[]int> (length: 1, cap: 1)
Anonymous:false

The value of s is 386861805349424274, and i is 1

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 1, 2016

Ok I'm stumped. Where is this Code field coming from?

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 7, 2016

@tamird even without the use of gogo/protobuf generated files? I hadn't seen it in our logs before we made the switch but as I mentioned I haven't yet taken a closer look.

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 11, 2016

I was able to reproduce the proto: no coders for int using the example provided by grpc-gateway, so I've raised grpc-ecosystem/grpc-gateway#259 now. I think this can probably be closed unless there's anything else to add?

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 11, 2016

Good work

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 20, 2016

I guess since grpc-ecosystem/grpc-gateway#259 is closed this issue can also be closed?

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Nov 21, 2016

At least the proto: no coders for int message is fixed now. I think the original issue is probably slightly different but I wouldn't object to closing for now anyway as the cockroachdb workaround seems to work well.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Nov 21, 2016

@tamird This link is broken
For reference: https://github.com/cockroachdb/cockroach/blob/develop/util/jsonpb_marshal.go
Maybe someone still wants to bring an issue up with grpc-gateway about this interface?
Or maybe we could at least split out this code into a repo that others can use?

@tamird
Copy link
Contributor

@tamird tamird commented Nov 21, 2016

Updated the link.

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

The underlying issue here has reared it's head again in enum parsing in the grpc-gateway. See grpc-ecosystem/grpc-gateway#320. Again it is because gogo/protobuf RegisterEnum doesn't register against golang/protobuf and so when grpc-gateway attempts to map an enum it fails to find it.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

Should we create a plugin to allow you to register with both gogoprotobuf and golang/protobuf or something like that.
I would personally like to create a plugin that does not import either, maybe these use cases can overlap?
Obviously not importing either will break extensions and ptypes.Any, but everything else should work if its implemented correctly.

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

I would like something like that for better cross-compatibility.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

It would be useful for me (to try and to only build one new feature) if you could provide a repo which produces this problem, with an easy way for me to test.
Are you ok with no support for ptypes.Any and proto2 Extensions?

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

@awalterschulze Pinging you on the gopher slack for further discussion

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

Okay, my org would have no problems with a plugin that doesn't support ptypes.Any or proto2 extensions. What kind of example are you interested in? The reason it's not working right now with the grpc-gateway is because it looks in a map created by golang/proto.RegisterEnum while all our generated files call gogo/proto.RegisterEnum

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

@awalterschulze It looks at the map so that it can map from the enum string to the enum code and vice versa. It's to allow things like https://my.com/api/v1/method?enum_variable=MY_ENUM_VALUE to be translated into the correct enum numeric representation.

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

As a side note, if this is at all unpleasant to consider, @mwitkow sed commands can be run post-generation to fix this issue, so it doesn't need to implemented at this level.

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

Ah ok, so grpc-gateway uses this in a totally different way. Thats unfortunate.
That means it has to register with github.com/golang/protobuf and there is no way around it :(

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

Indeed

@awalterschulze
Copy link
Member

@awalterschulze awalterschulze commented Mar 2, 2017

Ok so you need a plugin that registers messages, enums, etc with both golang/protobuf and gogo/protobuf.
Maybe lets make a new issue for this?

@johanbrandhorst
Copy link
Member

@johanbrandhorst johanbrandhorst commented Mar 2, 2017

I'll get on it 😄

danhhz pushed a commit to danhhz/cockroach that referenced this issue May 28, 2019
Also move util/http.go to new util/httputil package.
This makes it much easier for downstream users to use the cockroachdb workaround for inconsistensies between grpc-gateway and gogo/protobuf.
See cockroachdb#10294 and gogo/protobuf#178 for more information.

(sapling split of 5769d35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.