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

Impossible to adhere to AIP suggestions for AIP-136 when using google.api.HttpBody and have values in request path #1207

Closed
achew22 opened this issue Jul 23, 2023 · 7 comments · Fixed by #1445

Comments

@achew22
Copy link
Contributor

achew22 commented Jul 23, 2023

In AIP-136, and in the associated linter entry it is suggested that the following would not be allowed:

// Incorrect.
rpc CheckoutBook(CheckoutBookRequest) returns (CheckoutBookResponse) {
  option (google.api.http) = {
    post: "/v1/{name=publishers/*/books}:checkout"
    // `body: "*"` should be included.
  };
}

I believe the implied form of CheckoutBookRequest would be something like:

message CheckoutBookRequest {
  string name = 1;
  string some_other_field = 2;
  // even more fields
}

This seems reasonable in the general case. There is, however, an edge case in the somewhat bizarre event where you're using google.api.HttpBody to provide a webhook endpoint where you can't control the caller to send you structured data. Unfortunately I can't even force JSON, I'm going to be getting application/x-www-form-urlencoded encoded data posted as the whole body. I could put it into a body: "string_field" directly, but then I would be asking the parser to infer the encoding, which feels unnecessary since there is a nice way to get the content_type. I believe this functionally is akin to HTTPOverRPC from days of old (sorry for bringing that up if you remember it). This may be considered a thing that is beyond the scope for AIP, but I believe the only correct form for using this would be:

// Possibly incorrect?
rpc HandleWebhook(HandleWebhookRequest) returns (google.api.HttpBody) {
  option (google.api.http) = {
    post: "/v1/{name=webhooks/*}:handle"
    body: "request"
  };
}

message HandleWebhookRequest {
  string name = 1;
  google.api.HttpBody request = 2;
}

Unfortunately this causes a lint error. I could then add a lint exception for api-linter: core::0136::http-body=disabled to the HandleWebhook RPC, but I want to make sure that I'm not missing something obvious.

Is this what you would expect?

@loeffel-io
Copy link
Contributor

great one @achew22, same here
would be great to allow this

@noahdietz
Copy link
Collaborator

noahdietz commented Nov 7, 2024

@loeffel-io @achew22 so the ask is to specifically allow a non-* POST body for Custom Methods when the field name specified refers to a field of type google.api.HttpBody?

If so, I actually made this change internally and just haven't ported it to the OSS version (oops). LMK and I can make that change no problem

@achew22
Copy link
Contributor Author

achew22 commented Nov 8, 2024

@noahdietz, if you have a modification that could be brought into the oss repo it would be greatly appreciated! As a rule I don't care what the linter says as long as it is consistent, which I expect this would be. Thank you in advance!

@noahdietz
Copy link
Collaborator

Right-o, just opened #1444 !

@noahdietz noahdietz self-assigned this Nov 8, 2024
@noahdietz noahdietz linked a pull request Nov 8, 2024 that will close this issue
@noahdietz
Copy link
Collaborator

Merged, will cut a release on Monday

@noahdietz
Copy link
Collaborator

This should now be available in v1.67.6

@loeffel-io
Copy link
Contributor

Thank you @noahdietz 🙏

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

Successfully merging a pull request may close this issue.

3 participants