Closed johanbrandhorst closed 6 years ago
I tried playing around with this and I've come to several conclusions for now:
gogo/protobuf
as it is currently implemented in the gRPC-Gateway. This is because as is evident from the source, it only checks if the proto.MessageName
equals a certain value against those registered with golang/protobuf
. Attempting to use the golang/protobuf
generated protofile isn't an option here either, as gogo/protobuf
assumes nested fields implement Marshal
and Unmarshal
.Tagging @timonwong to see if he has anything to add to this discussion.
Nice. Yes lets first see how it should work with golang/protobuf and then try to get it working with gogo/protobuf. Thanks so much for pushing this forward :)
I was doing some more testing of field masks in the grpc-gateway, and surprisingly it works with protoc-gen-go, so there's potentially something here that protoc-gen-gogo can do differently, but I'm not entirely sure what yet.
I created https://github.com/johanbrandhorst/field-mask-test as an example of using the gRPC gateway with a field mask, and it works (???). I will try and reproduce the same thing in the gogo/grpc-example repo and see where it breaks next.
I'm also continuing the discussion in https://github.com/grpc-ecosystem/grpc-gateway/pull/529#issuecomment-376684843
Interesting development; if gogo/protobuf WKT implemented XXX_MessageName
it could resolve the issue with using golang/protobuf.MessageName
specifically: https://github.com/grpc-ecosystem/grpc-gateway/pull/529#issuecomment-376822766. What are you thoughts here @awalterschulze?
IF it really helps to solve the problem then:
It would only solve it in the proto.MessageName
case, it doesn't solve the general proto.MessageType
lookup case. I think it would be worthwhile implementing for this as I can't seem to find anywhere the gRPC-Gateway uses proto.MessageType directly. Shall I raise an issue?
Ok so they are only using proto.MessageName and this fixes the compatibility issue? Because then I am totally in!
I did a quick search for proto.MessageType and couldn't find any uses, so yeah, perhaps? It doesn't solve the problem of the jsonpb marshaler using golang/protobuf but that's completely configurable by the user at least.
I think we should consider restricting it to the WKT though, since the XXX_Stuff
is kind of restricted?
Ok what about we make a pull request on gogoprotobuf types folder and add XXX_MessageName methods to all the wkts and see if fieldmask starts working?
If it solves a problem, we merge it :)
@awalterschulze lets do it
cool :) Do you want to experiment with it locally or should I make a pull request?
Uh, I suppose I could try it out first, sure, let me get back to you.
Thank you so much. I really appreciate it.
@johanbrandhorst
It seems we will get more benefits by implementing XXX_MessageName
:
grpc.Status.WithDetails
should work correctly since it invokes ptypes.MarshalAny()
function, previously we will get wrong type url:@timonwong I can investigate that at the same time, that would be huge.
Oh wow
// MarshalAny takes the protocol buffer and encodes it into google.protobuf.Any.
func MarshalAny(pb proto.Message) (*Any, error) {
value, err := proto.Marshal(pb)
if err != nil {
return nil, err
}
return &Any{TypeUrl: googleApis + proto.MessageName(pb), Value: value}, nil
}
Seems like that should work :)
So then generating XXX_MessageName for all messages should solve MarshalAny.
Looking at the code it looks like even UnmarshalAny should work. This is huge
Another caveat about jsonpb codec in grpc-gateway to handle google.rpc.*
(eg. google.rpc.RetryInfo) correctly, we may have two options:
AnyResolver
explicitly, because even we have XXX_MessageName
implemented, it requires google/protobuf type registry (proto.MessageType
)google.golang.org/genproto/googleapis/rpc/errdetails
along with gogo/googleapis
alongThe issue is handled by supplying a custom JSON marshaller, we'll never be able to work around the problem of proto.MessageType
this easily.
The latest merge to this repo shows how to use the gogo/googleapis
types with the gRPC-Gateway already.
I've made some progress on this. In addition to adding XXX_MessageName() string
to the FieldMask type, I've had to patch the JSON marshaller I was using (the cockroachdb one). It wasn't able to unmarshal pointers to types that implement the proto.Message
. I've not yet figured it out exactly what needs doing as I'm still getting some marshalling errors, but I think we can go ahead and start work on defining XXX_MessageName() string
for the WKT types @awalterschulze. I'm not sure what it would entail exactly, and I consider that your area of expertise ;).
Replacing the cockroachdb Marshaler with https://github.com/abursavich/gogomarshal (from https://github.com/gogo/protobuf/issues/166) works out of the box (when XXX_MessageName() string
is implemented in FieldMask
).
It does output these worrying warnings though:
proto: no encoder for wall uint64 [GetProperties]
proto: no encoder for ext int64 [GetProperties]
proto: no encoder for loc *time.Location [GetProperties]
Now I'm sure I've seen this before, but I can't remember where or why. @awalterschulze any ideas about what causes this?
They don't happen with the cockroachdb Marshaler :thinking:
I've just pushed up https://github.com/gogo/grpc-example/pull/11 which is a working example of using google.protobuf.FieldMask. I've switch marshaler and manually implemented XXX_MessageName
on FieldMask.
Outstanding fixes:
proto: no encoder for wall uint64 [GetProperties]
proto: no encoder for ext int64 [GetProperties]
proto: no encoder for loc *time.Location [GetProperties]
XXX_MessageName
on WKTs.@johanbrandhorst
On the warnings the solution seems to be in this issue https://github.com/gogo/protobuf/issues/216
And here is an original issue report https://github.com/gogo/protobuf/issues/212
The solution seems to have something to do with
defaultMarshaler = &JSONPb{OrigName: true}
But, since I am not gRPC user :( I never confirmed it.
@johanbrandhorst on XXX_MessageName
I just want to confirm, this won't only be needed for FieldMask, but potentially for all user defined protobuf messages.
If so then I am thinking of adding another message/file option messagename=true
that will result in a XXX_MessageName
method being generated for the enabled messages.
If not then we can write some manual code in gogoprotobuf for the specific types that need them.
What do you think?
Yeah I'm not sure which yet, this could potentially be an alternative to goproto_registration. Implement it as an option to start with I think. I guess there's no harm in users turning it on for their messages or files.
Doesn't seem like either of those issue figured out why the messages appeared, so I'll do some more digging. Setting OrigName: true
does not solve the issue.
Oh that is very interesting. Hmmm.
In the mean time. What exactly would the MessageName of fieldmask, just as an example.
In other words, will this work?
func (*FieldMask) XXX_MessageName() string {
return "google.protobuf.FieldMask"
}
Yes, that works. It has to be the same as the name that is being registered.
It seems as though cockroachdb is calling jsonpb.Unmarshal https://github.com/cockroachdb/cockroach/blob/master/pkg/util/protoutil/jsonpb_marshal.go#L107
While gogomarshal is calling encoding/json Unmarshal https://github.com/abursavich/gogomarshal/blob/master/jsonpb.go#L106
That is probably a bug.
But I don't know how that is resulting in GetProperties being called.
I know that it is stdtime=true
that is causing this, since GetProperties is failing on time.Time
the option to generate XXX_MessageName has now been pushed to master and I have also generated it for all the well known types, including fieldmask. https://github.com/gogo/protobuf/commit/1ef32a8b9fc3f8ec940126907cedb5998f6318e4
Wow, that was fast!
Just like you :) It was easy, I just wanted to be sure before I did it :)
With the FieldMask now officially implementing MessageName() string
and the gogo/gateway
marshaler, FieldMask requests work out of the box! I will clean up the MR a little and add some more tests, but then we can close this issue.
The next question is if the MessageName() string
means we can use grpc/status
instead of gogo/status
. @awalterschulze any chance we can regenerate gogo/googleapis
packages with the MessageName() string
?
Done googleapis now has XXX_MessageName generated for each type.
Great and fast job guys! 👏
Need to find how to work around https://github.com/grpc-ecosystem/grpc-gateway/blob/58f78b988bc393694cef62b92c5cde77e4742ff5/runtime/query.go#L275 in gRPC-Gateway.