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

Create new method with the FieldMask WKT #9

Closed
johanbrandhorst opened this Issue Mar 17, 2018 · 45 comments

Comments

Projects
None yet
4 participants
@johanbrandhorst
Member

johanbrandhorst commented Mar 17, 2018

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 17, 2018

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

  1. 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.
  2. I don't know if this even works as intended in the gRPC-Gateway today. See my comment on the PR that introduced this.
@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 17, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 18, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 27, 2018

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.

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 27, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

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?

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

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.
@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

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?

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

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.

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

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?

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

If it solves a problem, we merge it :)

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

@awalterschulze lets do it

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 28, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 28, 2018

Thank you so much. I really appreciate it.

@timonwong

This comment has been minimized.

timonwong commented Mar 29, 2018

@johanbrandhorst
It seems we will get more benefits by implementing XXX_MessageName:

  1. 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

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

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 29, 2018

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.

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 29, 2018

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

@timonwong

This comment has been minimized.

timonwong commented Mar 29, 2018

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

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

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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.

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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?

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

They don't happen with the cockroachdb Marshaler 🤔

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 29, 2018

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.
@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

  1. 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.

  2. If not then we can write some manual code in gogoprotobuf for the specific types that need them.

What do you think?

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 30, 2018

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.

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 30, 2018

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.

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

Oh that is very interesting. Hmmm.

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

In other words, will this work?

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

This comment has been minimized.

Member

johanbrandhorst commented Mar 30, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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.

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 30, 2018

Wow, that was fast!

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

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

@johanbrandhorst

This comment has been minimized.

Member

johanbrandhorst commented Mar 30, 2018

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?

@awalterschulze

This comment has been minimized.

Member

awalterschulze commented Mar 30, 2018

Done googleapis now has XXX_MessageName generated for each type.

@glerchundi

This comment has been minimized.

glerchundi commented Mar 31, 2018

Great and fast job guys! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment