Create new method with the FieldMask WKT

Question

Create new method with the FieldMask WKT

Closed this issue 7 years ago · 45 comments

Need to find how to work around https://github.com/grpc-ecosystem/grpc-gateway/blob/58f78b988bc393694cef62b92c5cde77e4742ff5/runtime/query.go#L275 in gRPC-Gateway.

Answer 1 · 2018-03-17T21:51:42.000Z

I tried playing around with this and I've come to several conclusions for now:

This definitely won't work with 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.
I don't know if this even works as intended in the gRPC-Gateway today. See my comment on the PR that introduced this.

Answer 2 · 2018-03-17T21:53:48.000Z

Tagging @timonwong to see if he has anything to add to this discussion.

Answer 3 · 2018-03-18T09:25:22.000Z

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 :)

Answer 4 · 2018-03-27T21:46:23.000Z

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.

Answer 5 · 2018-03-27T21:47:17.000Z

I'm also continuing the discussion in grpc-ecosystem/grpc-gateway#529 (comment)

Answer 6 · 2018-03-28T09:35:34.000Z

Interesting development; if gogo/protobuf WKT implemented XXX_MessageName it could resolve the issue with using golang/protobuf.MessageName specifically: grpc-ecosystem/grpc-gateway#529 (comment). What are you thoughts here @awalterschulze?

Answer 7 · 2018-03-28T12:14:10.000Z

IF it really helps to solve the problem then:

I am more than open to adding that method to each message that needs it
I could even add a flag to allow this method to be generated or not, even for other messages
But I am worried that this still requires being registered in the correct registry.

Answer 8 · 2018-03-28T13:40:21.000Z

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?

Answer 9 · 2018-03-28T13:41:47.000Z

Ok so they are only using proto.MessageName and this fixes the compatibility issue?
Because then I am totally in!

Answer 10 · 2018-03-28T13:42:54.000Z

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.

Answer 11 · 2018-03-28T13:43:40.000Z

I think we should consider restricting it to the WKT though, since the XXX_Stuff is kind of restricted?

Answer 12 · 2018-03-28T13:46:41.000Z

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?

Answer 13 · 2018-03-28T13:47:19.000Z

If it solves a problem, we merge it :)

Answer 14 · 2018-03-28T13:50:47.000Z

@awalterschulze lets do it

Answer 15 · 2018-03-28T13:58:19.000Z

cool :)
Do you want to experiment with it locally or should I make a pull request?

Answer 16 · 2018-03-28T14:06:33.000Z

Uh, I suppose I could try it out first, sure, let me get back to you.

Answer 17 · 2018-03-28T14:07:53.000Z

Thank you so much. I really appreciate it.

Answer 18 · 2018-03-29T03:55:21.000Z

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

https://github.com/grpc/grpc-go/blob/f72b28a6d170d0938afd483331ba4e5d733785ae/status/status.go#L142-L158

It should also address this issue as well: grpc-ecosystem/grpc-gateway#576

Answer 19 · 2018-03-29T08:36:57.000Z

@timonwong I can investigate that at the same time, that would be huge.

Answer 20 · 2018-03-29T08:44:01.000Z

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.

Answer 21 · 2018-03-29T08:45:39.000Z

Looking at the code it looks like even UnmarshalAny should work.
This is huge

Answer 22 · 2018-03-29T09:41:46.000Z

Another caveat about jsonpb codec in grpc-gateway to handle google.rpc.* (eg. google.rpc.RetryInfo) correctly, we may have two options:

specify AnyResolver explicitly, because even we have XXX_MessageName implemented, it requires google/protobuf type registry (proto.MessageType)
import google.golang.org/genproto/googleapis/rpc/errdetails along with gogo/googleapis along

Answer 23 · 2018-03-29T10:28:32.000Z

The issue is handled by supplying a custom JSON marshaller, we'll never be able to work around the problem of proto.MessageType this easily.

Answer 24 · 2018-03-29T10:29:17.000Z

The latest merge to this repo shows how to use the gogo/googleapis types with the gRPC-Gateway already.

Answer 25 · 2018-03-29T20:51:04.000Z

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 ;).

Answer 26 · 2018-03-29T21:31:31.000Z

Replacing the cockroachdb Marshaler with https://github.com/abursavich/gogomarshal (from gogo/protobuf#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?

Answer 27 · 2018-03-29T21:35:24.000Z

They don't happen with the cockroachdb Marshaler 🤔

Answer 28 · 2018-03-29T21:59:41.000Z

I've just pushed up #11 which is a working example of using google.protobuf.FieldMask. I've switch marshaler and manually implemented XXX_MessageName on FieldMask.

Outstanding fixes:

Figure out why it outputs errors:

proto: no encoder for wall uint64 [GetProperties]
proto: no encoder for ext int64 [GetProperties]
proto: no encoder for loc *time.Location [GetProperties]

Properly implement XXX_MessageName on WKTs.

Answer 29 · 2018-03-30T12:05:13.000Z

@johanbrandhorst

On the warnings the solution seems to be in this issue
gogo/protobuf#216

And here is an original issue report
gogo/protobuf#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.

Answer 30 · 2018-03-30T12:08:55.000Z

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

Answer 31 · 2018-03-30T15:32:05.000Z

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.

Answer 32 · 2018-03-30T17:09:57.000Z

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.

Answer 33 · 2018-03-30T17:19:43.000Z

Oh that is very interesting. Hmmm.

Answer 34 · 2018-03-30T17:23:09.000Z

In the mean time. What exactly would the MessageName of fieldmask, just as an example.

Answer 35 · 2018-03-30T17:35:41.000Z

In other words, will this work?

func (*FieldMask) XXX_MessageName() string {
       return "google.protobuf.FieldMask"
}

Answer 36 · 2018-03-30T17:41:41.000Z

Yes, that works. It has to be the same as the name that is being registered.

Answer 37 · 2018-03-30T17:42:29.000Z

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.

Answer 38 · 2018-03-30T17:43:25.000Z

But I don't know how that is resulting in GetProperties being called.

Answer 39 · 2018-03-30T17:47:34.000Z

I know that it is stdtime=true that is causing this, since GetProperties is failing on time.Time

Answer 40 · 2018-03-30T17:52:01.000Z

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.
gogo/protobuf@1ef32a8

Answer 41 · 2018-03-30T18:12:14.000Z

Wow, that was fast!

Answer 42 · 2018-03-30T18:17:25.000Z

Just like you :)
It was easy, I just wanted to be sure before I did it :)

Answer 43 · 2018-03-30T19:21:58.000Z

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?

Answer 44 · 2018-03-30T19:45:21.000Z

Done googleapis now has XXX_MessageName generated for each type.

Answer 45 · 2018-03-31T13:09:44.000Z

Great and fast job guys! 👏