runtime: support FieldMask as query param

[glerchundi]

In order to support FieldMask in query params, as Google proposes, this should be treated as another known type.

/cc @achew22 @tmc

As a reference:
https://cloud.google.com/iam/reference/rest/v1/organizations.roles/patch
https://github.com/googleapis/googleapis/blob/f704d14a7224a140bca5cc26835fae471eaf7281/google/iam/admin/v1/iam.proto#L157
https://github.com/googleapis/googleapis/blob/f704d14a7224a140bca5cc26835fae471eaf7281/google/iam/admin/v1/iam.proto#L614-L626

[achew22]

Can you add a test to verify this functionality? I'd be very happy to merge once that happens. Thanks!

[glerchundi]

Yeah, will do!

…

On Sun, 21 Jan 2018 at 04:56, Andrew Z Allen ***@***.***> wrote: Can you add a test to verify this functionality? I'd be very happy to merge once that happens. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIPlgK7bkV_RT-dop_ySAt2MrwAJgi3ks5tMrVfgaJpZM4Rix5-> .

[glerchundi]

ready to review @achew22!

[achew22]

I love being reviewed!

On a more serious note. LGTM, let's ship it. Thanks for contributing!

[glerchundi]

Haha, thanks!

…

On Sun, 21 Jan 2018 at 12:36, Andrew Z Allen ***@***.***> wrote: Merged #529 <#529>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#529 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIPlv_afB9Hc-1Z6N8vPx9HhA_1-fXcks5tMyFJgaJpZM4Rix5-> .

[johanbrandhorst]

I've tried playing around with this a bit, and I can't seem to get this to work the way I think it should work. Given the example and the code in this PR, I'm assuming the field mask should be provided as a query parameter like so:

curl -k -X PATCH "https://localhost:11000/api/v1/user/1?update_mask=role" -H  "accept: application/json" -H  "Content-Type: application/json" -d "{\"role\": \"ADMIN\"}"

Given the following definitions:

rpc UpdateUser(UpdateUserRequest) returns (User) {
    option (google.api.http) = {
        patch: "/api/v1/user/{user.id}"
        body: "user"
    };
}

message UpdateUserRequest {
    User user = 1;
    google.protobuf.FieldMask update_mask = 2;
}

Unfortunately, this kind of definition doesn't even survive generation; the generated code tries to decode into a **User:

if err := marshaler.NewDecoder(req.Body).Decode(&protoReq.User); err != nil {

Is there an example of this implemented a used successfully in the wild? The query test "example" is basically useless as far as I'm concerned.

[glerchundi]

I've tested with this proto:

 // Update an existing user.
    rpc UpdateUser(UpdateUserRequest) returns (User) {
        option (google.api.http) = {
            patch: "/api/v1/{user.name=installations/*/users/*}"
            body: "user"
        };
    }

[...]

message UpdateUserRequest {
    // The user resource which replaces the resource on the server.
    User user = 2;

    // The update mask applies to the resource. For the `FieldMask` definition,
    // see https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#fieldmask
    google.protobuf.FieldMask update_mask = 3;
}

[...]

message User {
    // Relative resource name.
    string name = 1;

    // Display name.
    string display_name = 3;

    // Description.
    string description = 4;
}

And this worked:

curl -H "Content-Type: application/json" --request PATCH -d@./update_user.json localhost:8080/api/v1/installations/1/users/user2?update_mask=display_name

Where update_user.json is:

{
  "display_name": "user3" 
}

So i firmly believe that something is not working on your side. Are you using the standard go generator or something like gogo? Give us a full example and perhaps we can go further.

[johanbrandhorst]

@glerchundi thanks for that, sounds like my use case should work, I will do some more testing and report back.

[johanbrandhorst]

Update: my initial problems were indeed because I was using GoGo Protobuf. My intentions are actually to investigate whether this works with gogoprotobuf, but I wanted to get it working first, so thanks for pointing me in the right direction. I will see what would need to be done on the gogo/protobuf side, but I expect this will flat out not work with gogo/protobuf until an alternative implementation for

grpc-gateway/runtime/query.go

Line 275 in 58f78b9

switch proto.MessageName(gwkt) {

can be made.

Out of interest; since the update_mask specifies fields as strings, what reflect magic do you use to figure out which fields of the struct to update?

[glerchundi]

Good investigation.

Update: my initial problems were indeed because I was using GoGo Protobuf. My intentions are actually to investigate whether this works with gogoprotobuf, but I wanted to get it working first, so thanks for pointing me in the right direction. I will see what would need to be done on the gogo/protobuf side, but I expect this will flat out not work with gogo/protobuf until an alternative implementation for

But I don't know why it's not working, proto.MessageName should work because gogo's fieldmask registers itself under the same name that the official fieldmak does:

official: https://github.com/google/go-genproto/blob/master/protobuf/field_mask/field_mask.pb.go#L249
gogo: https://github.com/gogo/protobuf/blob/master/types/field_mask.pb.go#L244

Also the structs have the same parameters (just one) with the same signature which means the reflection should work as well. And proto.MessageName seems to behave in the same way...

Out of interest; since the update_mask specifies fields as strings, what reflect magic do you use to figure out which fields of the struct to update?

We're pre-generating everything, updateable fields which we then map to database fields, no reflection magic at all. With something like this:

message User {
string display_name = 3  [metadata.updateable = true];
}

Would be interesting to see your progress.

[johanbrandhorst]

@glerchundi I'm afraid you're making an incorrect assumption about how proto.MessageName works. golang/proto.MessageName only returns messages that are registered with the golang/protobuf type registry. gogo/protobuf is forced to maintain its own type registry, which leads to this problem as well as many other issues. The links you're providing ignores the fact that one registeres with gogo/protobuf.RegisterType and one with golang/protobuf.RegisterType.

If you want more information about this kind of thing, you're welcome to read my blogpost on gogo/protobuf compatibility issues: https://jbrandhorst.com/post/gogoproto/.

There are basically two solutions to this problem:

1. Make all gogo/protobuf files register with both type registries.
   This is undesireable because gogo/protobuf shouldn't have to import golang/protobuf (for what I think should be obvious reasons).
2. We implement some way in the gRPC-Gateway that accesses both registries, presumably with golang/protobuf taking precedence.
   2.1 Alternatively, allow users of the gRPC-Gateway runtime to select a proto.MessageName resolver.

Anyway, as I mentioned, there seems to be another problem here with the gogo/protobuf generated message, so I will look into that before suggesting any changes to the gRPC-Gateway.

[glerchundi]

@glerchundi I'm afraid you're making an incorrect assumption about how proto.MessageName works. golang/proto.MessageName only returns messages that are registered with the golang/protobuf type registry. gogo/protobuf is forced to maintain its own type registry, which leads to this problem as well as many other issues. The links you're providing ignores the fact that one registeres with gogo/protobuf.RegisterType and one with golang/protobuf.RegisterType.

Right.

2.1 Alternatively, allow users of the gRPC-Gateway runtime to select a proto.MessageName resolver.

What about gogo implementing XXX_MessageName method in its types? Would solve without messing the runtime with resolvers here and there.

https://github.com/golang/protobuf/blob/master/proto/properties.go#L846-L855

[johanbrandhorst]

Hm, that is a fair point, I didn't know about that method. It would solve the problem with proto.MessageName specifically, but if that name was used to look up any types in proto.MessageType, it would still error.

[glerchundi]

Hm, that is a fair point, I didn't know about that method. It would solve the problem with proto.MessageName specifically, but if that name was used to look up any types in proto.MessageType, it would still error.

Better than nothing. I think the problem of how to interoperate is much bigger than these concrete methods. I still think that in this case using interfaces is the way to go and would fix your current issue, what do you think if start by fixing this in gogo types and file/comment in another issue(s) for the interoperability between libraries (i'm quite sure there are already issues in this regard).

[johanbrandhorst]

I agree, and as I said there are other issues here with gogo/protobuf which I will investigate at the same time. Feel free to follow the discussion in gogo/grpc-example#9 if you wish.