Skip to content
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

Updated Patch behavior #806

Closed
wants to merge 8 commits into from

Conversation

razamiDev
Copy link
Contributor

This PR is a cleaner version of PR #671
This was a collaborative effort by @razamiDev and @dmacthedestroyer

this addresses #379
If a binding is mapped to patch and the request message has exactly one FieldMask message in it, additional code is rendered for the gateway handler that will populate the FieldMask based on the request body.
This handles two scenarios:
The FieldMask is hidden from the REST request (as in the UpdateV2 example). In this case, the FieldMask is updated from the request body and set in the gRPC request message.
The FieldMask is exposed to the REST request (as in the PatchWithFieldMaskInBody example). For this case, a check is made as to whether the FieldMask is nil/empty prior to populating with the request body. If it's not nil, then it converts the FieldMask paths from the REST (snake_case) to gRPC (PascalCase) format. Otherwise, it acts like the previous case.
Additional comments of interest are inline on this PR.

@johanbrandhorst
Copy link
Collaborator

@razamiDev Thanks a lot for picking this up. It's very hotly anticipated feature among the maintainers :). Looks like we've got some bazel failures, something about the field_mask.proto file not being found. @achew22 @drigz maybe you can help here? I think it's a new dependency for the test files.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work here, can't wait to merge this.

}
}()
getField(obj, path).Set(newValue)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is already working, but with my own experimentation with field masks I found the structs package very helpful: https://github.com/fatih/structs. See https://github.com/gogo/grpc-example/blob/master/server/server.go#L138 for an example of its use for this kind of thing.

@drigz
Copy link
Contributor

drigz commented Nov 14, 2018

@razamiDev Could you install Bazel and run bazel run :gazelle_fix?

@johanbrandhorst I'll try to make this clearer in the CI logs (#807, in progress). Do you think we need an invocation that works with docker like in CONTRIBUTING.md, or is it OK to ask contributors to install Bazel?

@johanbrandhorst
Copy link
Collaborator

@drigz I definitely would like to see a docker-based solution, and I think we should be able to put together a oneliner with the docker image we're using in circleCI.

@drigz
Copy link
Contributor

drigz commented Nov 14, 2018

@johanbrandhorst We need to use a different image to get Bazel - CircleCI uses l.gcr.io/google/bazel - so we'll need two docker run commands. The simplest I could come up with is:

docker run -itv $(pwd):/grpc-gateway -w /grpc-gateway --entrypoint /bin/bash --rm \
    l.gcr.io/google/bazel -c 'bazel run :gazelle_fix; bazel run :buildifier'

@razamiDev If you don't have Bazel installed, perhaps you could try that command and let me know if it works for you?

@johanbrandhorst
Copy link
Collaborator

Nice job @drigz, lets see if we can get that into the contribution guidelines with #807?

drigz and others added 3 commits November 14, 2018 06:51
* Make Bazel CI failures clearer

- don't try to run tests if the BUILD files are out-of-date
- run buildifier even if the tests fail, as it may still be useful
- use a parameter to hold Bazel config to simplify config.yml
- ignore the vendor/ directory when running Gazelle
  (otherwise it will fail if you've used `dep` to create vendor/)

* Add Bazel invocation to CONTRIBUTING.md
@razamiDev
Copy link
Contributor Author

I appreciate the help as I am new to bazel. @johanbrandhorst I installed bazel and ran bazel run :gazelle_fix and it didn't give me any errors it said Build completed successfully.

@drigz I also ran the docker command and it said Build completed successfully.
I added all the new BUILD.bazel files and see that it still failed... giving an error not findingfastuuid

@drigz
Copy link
Contributor

drigz commented Nov 15, 2018

@razamiDev Thanks for following up! It looks like gazelle has rewritten the dependencies to point an vendor, which isn't what we want. Could you rebase on top of master to pull in #807? That tells gazelle to ignore vendor.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@razamiDev
Copy link
Contributor Author

@johanbrandhorst I am not sure why the googlebot failed... I am okay with my work being contributed to this project.

@johanbrandhorst
Copy link
Collaborator

So it looks like your merged master into this branch instead of rebasing on top of master. This meant this branch now contains commits that weren't made by you, and it's confusing the Google bot. I can't overrule the Google bot, do the easiest thing for you is to try and rebase on master again (not merge!). If you can't do that, the next easiest is probably to open a new PR in a new branch.

@drigz
Copy link
Contributor

drigz commented Nov 15, 2018

@razamiDev I think the CLA bot is unhappy because some commits from master have made it into the pull request:

image

You may be able to fix it by running:

git checkout -b feature/patch2-rebased master
git cherry-pick 398e4356300c19959d4cc65668975b2254b06b51
bazel run :gazelle_fix
git commit -am "Regenerate BUILD files"

As @johanbrandhorst you may need to create a new pull request so the CLA bot can run again.

@razamiDev razamiDev mentioned this pull request Nov 15, 2018
@razamiDev
Copy link
Contributor Author

@johanbrandhorst @drigz thank you both for the help!
I have created a new PR (#812) and this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants