grpc-gateway calls golang/protobuf/jsonpb

[mwitkow]

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]

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]

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]

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 https://github.com/cockroachdb/cockroach/pull/6587/commits/2101119350ceaaba60e150f9e5fd3355acd9fefc

[awalterschulze]

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 https://github.com/cockroachdb/cockroach/pull/6587/commits/2101119350ceaaba60e150f9e5fd3355acd9fefc

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/178#issuecomment-219526886

[mwitkow]

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 < https://github.com/cockroachdb/cockroach/pull/6587/commits/2101119350ceaaba60e150f9e5fd3355acd9fefc

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/178#issuecomment-219526886

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/178#issuecomment-219528358

[awalterschulze]

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 https://github.com/cockroachdb/cockroach/pull/6587/commits/2101119350ceaaba60e150f9e5fd3355acd9fefc

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/178#issuecomment-219526886

[awalterschulze]

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 https://github.com/cockroachdb/cockroach/pull/6587/commits/2101119350ceaaba60e150f9e5fd3355acd9fefc

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/gogo/protobuf/issues/178#issuecomment-219526886

[awalterschulze]

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

[awalterschulze]

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?

[mwitkow]

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]

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]

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 https://github.com/gogo/protobuf/issues/178#issuecomment-246414580, or mute the thread https://github.com/notifications/unsubscribe-auth/AJNWo6RtqaHe1CLWL2srs9l4-cnwY8LRks5qpYTEgaJpZM4Id0EI .

[awalterschulze]

Ok so everything works in that case?

[tamird]

For reference: https://github.com/cockroachdb/cockroach/blob/995c4a0/pkg/util/protoutil/jsonpb_marshal.go

[awalterschulze]

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]

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

Having to copy marshalNonProtoField is really unfortunate.

[awalterschulze]

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

[awalterschulze]

Doesn't seem like the issue was ever opened https://github.com/gogo/protobuf/issues/212#issuecomment-253883168

[johanbrandhorst]

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]

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]

Sounds reasonable, I'll give it a go

[johanbrandhorst]

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]

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]

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

[johanbrandhorst]

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]

Where is it finding this int field?

[johanbrandhorst]

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]

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

[johanbrandhorst]

It's a gRPC codes.Code.

[awalterschulze]

That is a uint32 and not an int

[johanbrandhorst]

Ah yes, sorry, it's actually from https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/errors.go#L65:

type errorBody struct {
    Error string `protobuf:"bytes,1,name=error" json:"error"`
    Code  int    `protobuf:"bytes,2,name=code" json:"code"`
}

[awalterschulze]

This looks like a hack.

[johanbrandhorst]

I suppose it's meant to just marshal the int to a JSON integer without asking any questions? If there is an issue here against grpc-gateway I'll gladly raise it.

[awalterschulze]

I wonder how golang/protobuf does not have the same problem. Is it possibly because of this protoutil that only gogoprotobuf has this issue?

I still think they just make the int an int64 to make it a proper protobuf and this might be a raisable issue?

[johanbrandhorst]

I'd love for @tamird and @mwitkow to weigh in on this as I'm not sure exactly what causes the problem.

[johanbrandhorst]

I can reproduce the same problem using golang/protobuf/jsonpb as the grpc-gateway marshaler, so not sure where the error is coming from now.

[awalterschulze]

Well seems like it should now be easy to report to grpc-gateway or not?

[johanbrandhorst]

I'm still not sure this isn't somehow related to the use of gogo/protobuf generated protofiles, I'd like to just get a minimal sample project up and play around with it.

[awalterschulze]

Sounds like a good plan.

[tamird]

@johanbrandhorst it seems that this log manifests exactly whenever grpc-gateway is used to return an error from a grpc handler.

[johanbrandhorst]

@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]

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

[awalterschulze]

Good work

[awalterschulze]

I guess since https://github.com/grpc-ecosystem/grpc-gateway/issues/259 is closed this issue can also be closed?

[johanbrandhorst]

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]

@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]

Updated the link.

[johanbrandhorst]

The underlying issue here has reared it's head again in enum parsing in the grpc-gateway. See https://github.com/grpc-ecosystem/grpc-gateway/issues/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]

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]

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