search

gogo / grpc-example

An example of using Go gRPC and tools from the greater gRPC ecosystem together with the GoGo Protobuf Project.
Other
454 stars 88 forks source link

FieldMask demo is wrong #31

Closed tzachshabtay closed 4 years ago

tzachshabtay commented 4 years ago

In your example, try updating a user's role for example. It doesn't update it.

This is because you've set allow_patch_update=false which means grpc-gateway will not automatically fill the field mask for you, but you also put user instead of * in your service definition which means that swagger won't include the field mask so the user can't set it. In other words, the update endpoint can't do anything currently.

Changing to * will allow the user to set the field mask which will work but not user friendly.

We were looking for a way to make gogo-proto and allow_patch_update=true to work successfully together (which is how I found your example in the first place), but we didn't find anything on that topic (the field mask type is different from go-protobuf). Any idea?

johanbrandhorst commented 4 years ago

Hi! Yeah I must've forgotten to change this when I merged it, you're right that the patch feature doesn't work with gogoproto, allow_patch_update=false is required to avoid a compilation error (as you noticed). The only option if you want to use gogoproto is to expose the fieldmask and user type explicitly. I'm gonna change the signature to fix this.

tzachshabtay commented 4 years ago

@johanbrandhorst I was hoping you'll find a way to make it work with the patch feature (at least for us, and I'm guessing other companies too, grpc-gateway is meant for letting operations interact with the services with the likes of Postman, and in that case exposing the field mask to the user is dangerous, as a human being might forget to set it).

johanbrandhorst commented 4 years ago

Feel free to experiment with it, but I just don't think it's possible. I don't have the time to do it myself. I nowadays recommend users avoid gogoproto for this and other reasons.

tzachshabtay commented 4 years ago

@johanbrandhorst thanks for the response. I'm curious about why you recommend people to avoid gogoproto (except for this issue), do you have a post about this somewhere I can read? Thanks.

johanbrandhorst commented 4 years ago

https://jbrandhorst.com/post/gogoproto/ was a post I wrote a while a go saying "it's not so bad" but really I've come to change my mind, the benefits aren't really worth the costs to me anymore. It's cases like this that show it'll never be a first class citizen in the ecosystem.