Skip to content

Add detail for gRPC/HTTP transcoding #922

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.

Sign up for GitHub

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

Open
jskeet wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add detail for gRPC/HTTP transcoding #922

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c4ffa4
Add detail for gRPC/HTTP transcoding
jskeet Jul 21, 2022
1747ffe
Address review comments
jskeet Jul 22, 2022
c397ff5
Merge branch 'master' into transcoding
GregFurmanek Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions aip/general/0127.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ streaming.
### HTTP method and path

When using protocol buffers, each RPC **must** define the HTTP method and path
using the `google.api.http` annotation:
using the `google.api.http` annotation. This annotation specifies an
[HttpRule][] for the method. (The comments in the linked proto give more
exhaustive details than this page.)

```proto
rpc CreateBook(CreateBookRequest) returns (Book) {
Expand Down Expand Up @@ -68,7 +70,7 @@ message CreateBookRequest {
- RPCs **should** use the prescribed HTTP verb for custom methods, as
discussed in [AIP-136][].
- RPCs **should not** use `put` or `custom`.
- The corresponding value represents the URI.
- The corresponding value represents the path in the URI.
Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Jul 21, 2022

Choose a reason for hiding this comment

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

(URN, in the nomenclature of https://en.wikipedia.org/wiki/Uniform_Resource_Identifier, though it seems this term is not commonly used)

Copy link
Copy Markdown
Contributor Author

@jskeet jskeet Jul 22, 2022

Choose a reason for hiding this comment

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

I suspect that far more readers will understand "path in the URI" than "URN".

Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Aug 30, 2022

Choose a reason for hiding this comment

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

Agreed. (Hence the parentheses)

- URIs **must** use the `{foo=bar/*}` syntax to represent a variable that
should be populated in the request proto. When extracting a [resource
name][aip-122], the variable **must** include the entire resource name, not
Expand All @@ -92,6 +94,17 @@ message CreateBookRequest {
- The `body` **must not** be a `repeated` field.
- Fields **should not** use the `json_name` annotation to alter the field
name in JSON, unless doing so for backwards-compatibility reasons.
- In any given HTTP request, if a field is represented in both the body
and the URI, the values **must** be identical. The server behavior when
this constraint is violated is unspecified.
- Eligible fields which are not represented in the URI path or the body are mapped
to URI query parameters.
Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Jul 21, 2022

Choose a reason for hiding this comment

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

It might be useful to include a reference to https://developers.google.com/protocol-buffers/docs/proto3#json, which has more details (and may get updated based on our parallel work)

Copy link
Copy Markdown
Contributor Author

@jskeet jskeet Jul 22, 2022

Choose a reason for hiding this comment

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

That's already referenced earlier in the body part: "The request body is encoded as JSON
as defined by protocol buffers' canonical JSON encoding" where "JSON encoding" is a link.

- Only singular primitive, repeated primitive, and singular message type fields
Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Jul 21, 2022

Choose a reason for hiding this comment

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

It would be good to link to the source of truth https://github.com/googleapis/googleapis/blob/dbfbfdb38a2f891384779a7ee31deb15adba6659/google/api/http.proto#L61 .... but is there a web-friendly, auto-up-to-date view of this somewhere we can use?

Copy link
Copy Markdown
Contributor Author

@jskeet jskeet Jul 22, 2022

Choose a reason for hiding this comment

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

I've added a link to the proto (in the master branch, which will be redirected accordingly if we rename it to main) earlier - I think that's probably clearer than trying to link to individual bits of documentation for each section.

are eligible to be represented as query parameters.
Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Jul 21, 2022

Choose a reason for hiding this comment

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

nit: Consider simpler wording: s/are eligible to be/may be/

Copy link
Copy Markdown
Contributor Author

@jskeet jskeet Jul 22, 2022

Choose a reason for hiding this comment

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

I'd prefer to keep it as "eligible" as "may" suggests something that's optional - a client might or might not decide to encode some fields as URI query parameters. In reality, a client must do that, because the fields are eligible to be encoded. Happy to noodle on alternatives to "eligible" - I'd just prefer to avoid "may".

Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g Aug 30, 2022

Choose a reason for hiding this comment

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

Oh, I see what you mean. On a naive read, "eligible" registered to me as something you can choose to do, i.e. "may". But you are using it in an algorithmic sense, tying back to your previous line.

Could we say instead:

Fields which are not represented in the URI path or the body are mapped to URI query parameters if and only if they 
are singular primitive, repeated primitive, or singular message type fields.
- Repeated primitive...
- For message-type fields....

And that also brings up a question to clarify: what about repeated message fields? Should it be an error for there to be an http binding that does not map them to the body, since they can't be represented in the path or query params and so would incur data loss if non-empty?

- Repeated primitive fields are represented as repeated query parameters,
e.g. `...?param=A&param=B`.
- For message type fields, each eligible leaf field is represented as a
separate query parameter, e.g. `...?foo.a=A&foo.b=B&foo.c=C`.

**Note:** Bi-directional streaming RPCs should not include a `google.api.http`
annotation at all. If feasible, the service **should** provide non-streaming
Expand Down Expand Up @@ -127,11 +140,14 @@ rpc CreateBook(CreateBookRequest) returns (Book) {

## Changelog

- **2022-07-21**: Added clarification around field duplication between the
body and URI, and added query parameter mapping.
- **2021-01-06**: Added clarification around `body` and nested fields.
- **2019-09-23**: Added a statement about request body encoding, and guidance
discouraging `json_name`.

<!-- prettier-ignore-start -->
[HttpRule]: https://github.com/googleapis/googleapis/blob/master/google/api/http.proto
[aip-121]: ./0121.md
[aip-122]: ./0122.md
[aip-131]: ./0131.md
Expand Down