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

runtime: support FieldMask as query param #529

Merged
merged 1 commit into from Jan 21, 2018

Conversation

Projects
None yet
3 participants
@achew22

This comment has been minimized.

Collaborator

achew22 commented Jan 21, 2018

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

@glerchundi

This comment has been minimized.

Contributor

glerchundi commented Jan 21, 2018

@glerchundi glerchundi force-pushed the glerchundi:feature/fieldmask-in-query branch from 6582bd9 to 4d594b6 Jan 21, 2018

@glerchundi

This comment has been minimized.

Contributor

glerchundi commented Jan 21, 2018

ready to review @achew22!

@achew22

This comment has been minimized.

Collaborator

achew22 commented Jan 21, 2018

I love being reviewed!

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

@achew22 achew22 merged commit 5e029d0 into grpc-ecosystem:master Jan 21, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@glerchundi

This comment has been minimized.

Contributor

glerchundi commented Jan 21, 2018

@johanbrandhorst

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 17, 2018

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

This comment has been minimized.

Contributor

glerchundi commented Mar 19, 2018

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.

@glerchundi glerchundi deleted the glerchundi:feature/fieldmask-in-query branch Mar 19, 2018

@johanbrandhorst

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 19, 2018

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

@johanbrandhorst

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 27, 2018

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

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

This comment has been minimized.

Contributor

glerchundi commented Mar 27, 2018

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

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 28, 2018

@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

This comment has been minimized.

Contributor

glerchundi commented Mar 28, 2018

@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

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 28, 2018

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

This comment has been minimized.

Contributor

glerchundi commented Mar 28, 2018

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

This comment has been minimized.

Collaborator

johanbrandhorst commented Mar 28, 2018

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.

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