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

fix!: use resource field in http body for Updates #1198

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

noahdietz
Copy link
Collaborator

Fixes the google.api.http body fields for UpdateUser, UpdateBlurb, and UpdateRoom to use the field in the request representing the resource, instead of *.

This also adds support for parsing some of the well known protobuf types that may be present as JSON encoded query parameters and that have special JSON encoding formats (see https://developers.google.com/protocol-buffers/docs/proto3#json). This API only makes use of Timestamp, Duration, and FieldMask.

fix: support parsing well known types in query params
@noahdietz noahdietz requested a review from a team as a code owner August 31, 2022 23:48
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

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

I don't understand why we have to change body to NOT be *. Could you explain? Everything else lgtm.

@@ -58,7 +58,7 @@ service Identity {
rpc UpdateUser(UpdateUserRequest) returns (User) {
option (google.api.http) = {
patch: "/v1beta1/{user.name=users/*}"
body: "*"
body: "user"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have * so that the user can specify which fields to PATCH? It seems like this is one of the prime uses cases for field masks.

Copy link
Collaborator Author

@noahdietz noahdietz Sep 1, 2022

Choose a reason for hiding this comment

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

I'm not sure what you mean. The update_mask will end up in the query params. We don't lose any functionality here, just fixing a non-compliant binding (https://google.aip.dev/134) so that we can test a real use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. "There must be a body key in the google.api.http annotation, and it must map to the resource field in the request message. All remaining fields should map to URI query parameters."

OK, my bad. LGTM!

util/genrest/resttools/populatefield.go Outdated Show resolved Hide resolved
@noahdietz noahdietz requested a review from vchudnov-g September 1, 2022 20:24
@@ -58,7 +58,7 @@ service Identity {
rpc UpdateUser(UpdateUserRequest) returns (User) {
option (google.api.http) = {
patch: "/v1beta1/{user.name=users/*}"
body: "*"
body: "user"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. "There must be a body key in the google.api.http annotation, and it must map to the resource field in the request message. All remaining fields should map to URI query parameters."

OK, my bad. LGTM!

@noahdietz
Copy link
Collaborator Author

Ok so I am going to remove the Breaking Change indicator from the commit because I don't want to release this as a major version (at some point we really should make a v1 though).

@noahdietz noahdietz merged commit 48a2632 into googleapis:main Sep 1, 2022
@noahdietz noahdietz deleted the regapic-wkt branch September 1, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants