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.

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

Jump to bottom

feat (AIP-136) Update guidance of billable stateless methods #1124

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

feat (AIP-136) Update guidance of billable stateless methods #1124

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c9105e7
Update 0136.md for guidance of billable stateless methods
xdtdaniel May 31, 2023
4871799
Merge branch 'master' into master
toumorokoshi Jun 2, 2023
7fd7b34
Clarify on the why the cacheable requests should use GET
xdtdaniel Jun 3, 2023
c43b4c5
Update 0136.md
xdtdaniel Jun 7, 2023
85f1829
Update 0136.md to reflect current date
xdtdaniel Jun 7, 2023
4426da2
Merge branch 'master' into master
xdtdaniel Jun 10, 2023
0bcef51
Merge branch 'master' into master
toumorokoshi Jul 5, 2023
b2dcccc
Update 0136.md
xdtdaniel Jul 5, 2023
dd59db2
Merge branch 'master' into master
toumorokoshi Aug 7, 2023
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
13 changes: 11 additions & 2 deletions aip/general/0136.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
id: 136
state: approved
created: 2019-01-25
updated: 2023-03-02
updated: 2023-06-06
placement:
category: operations
order: 100
Expand Down Expand Up @@ -127,7 +127,6 @@ rpc TranslateText(TranslateTextRequest) returns (TranslateTextResponse) {
- The URI **should** place both the verb and noun after the `:` separator
(avoid a "faux collection key" in the URI in this case, as there is no
collection). For example, `:translateText` is preferable to `text:translate`.
- Stateless methods **must** use `POST` if they involve billing.

### Declarative-friendly resources

Expand Down Expand Up @@ -174,8 +173,18 @@ RPC name itself causes confusion, and can even cause issues for client libraries
which generate both synchronous and asynchronous methods to call the RPC in some
languages.

### Choice of HTTP methods for stateless methods

HTTP clients such as browsers often cache `GET` responses as `GET` implies that
it is doing data retrieval. Stateless methods that are cacheable and send no
Comment on lines +178 to +179
Copy link
Collaborator

Choose a reason for hiding this comment

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

HTTP clients such as browsers often cache `GET` responses as `GET` implies that
it is doing data retrieval.

This is rationale, not guidance

payload **should** use `GET` HTTP method due to its cache-friendly nature.
Copy link
Collaborator

@noahdietz noahdietz Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

due to its cache-friendly nature.

This is rationale, not guidance.

This can reduce the costs for both the server and the client.
Copy link

@loudej loudej Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, I don't believe cacheability to informs the choice of method for rest api.

On one hand, per RFC the response returned by non-GET operations may carry an entity that has cacheable implications.

https://datatracker.ietf.org/doc/html/rfc7231#section-7.2

   In a successful response to a state-changing request, validator
   fields describe the new representation that has replaced the prior
   selected representation as a result of processing the request.

   For example, an ETag header field in a 201 (Created) response
   communicates the entity-tag of the newly created resource's
   representation, so that it can be used in later conditional requests
   to prevent the "lost update" problem [[RFC7232](https://datatracker.ietf.org/doc/html/rfc7232)].

So the litmus test is less like "is your response cacheable?" and more like "is the URI (path+query) the address of a data or of a function?" --- and if the URI is the address of data --- do all of the idempotent and non-idempotent methods acting on that URI comply with the RFC necessary to be cacheable?

And that's the other hand, our API are actually miles away from cacheable and would take effort to change that... Like:

  • return an etag json field has no semantic meaning - the response would need to contain an ETag response header instead
  • the format of our ETag isn't right. it doesn't matter if it's a guid or not - but the string value must contain starting and ending with double-quote characters
  • both the mutating and non-mutating URI would need to have the same path+query for cache hits, cache invalidation, and optimistic concurrency. the way we have processing-instruction parameters appear as query arguments (like update_mask=xxx and etag=yyy) means that PATCH and DELETE operations are correlated to different cache entries than the GET's response represents
  • and speaking of ?etag=yyy precondition field --- for browser and httpclient cacheability what we'd need to accept are the If-Match yyy and If-None-Match: yyy request header parameters.

(I think those would be great to do --- but without that using a cache-enabled api httpclient does more harm than good. w/out etag and 304 response code it can actively prevent you from reaching past stale data --- especially if PUT/PATCH/DELETE aren't invalidating cache entries by URI properly)

(Also worth noting this isn't unusual --- many (most?) (almost all??) systems with REST API interfaces don't go down the road of http-defined cachability. I suppose that's because (unlike a 70k js or png file) by the time you've paid for the round-trip, done the mandatory authn/authz for the operation, and drawn sufficient info from the DB to known that the etag verifier is up-to-date ---- returning 304 and saving the typically smaller response of an average API doesn't have much ROI left)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Louis for the comment. I think it makes sense to emphasize the part of how uri informs the operation - is it more of a data retrieval or something more complicated than that. Let me rephrase this part. I will wait for Noah's comment as well to make the change together. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can reduce the costs for both the server and the client.

This is rationale, not guidance.


xdtdaniel marked this conversation as resolved.
Show resolved Hide resolved
## Changelog

- **2023-06-06:** Remove the guidance that stateless methods involving
billing should use POST. Clarify the choice of HTTP methods for stateless
methods.
- **2023-05-16:** Added prohibition of the term "async" within RPC names.
- **2023-05-09:** Adding guidance for POST and GET, require parent instead of
the resource singular.
Expand Down