-
Notifications
You must be signed in to change notification settings - Fork 21
feat(158): suggest has_changed flag and clarify expectations about stale paginated list data #278
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
base: main
Are you sure you want to change the base?
Changes from all commits
9bf3714
4287844
91c7d13
a3508ca
e3683bd
50b635f
f95aef4
8b41a1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,7 @@ when the REST/JSON interface is used. | |
|
|
||
| `List` operations **must** return a page of results, with each individual | ||
| result being a resource. | ||
|
|
||
| - The array of resources **must** be named `results` and contain resources with | ||
| no additional wrapping. | ||
| - The `string nextPageToken` field **must** be included in the list response | ||
|
|
@@ -112,6 +113,9 @@ result being a resource. | |
| - The response message **must** include a field corresponding to the resources | ||
| being returned, named for the English plural term for the resource, and | ||
| **should not** include any other repeated fields. | ||
| - The response message **may** include a `bool has_changed` field, to indicate | ||
| that the underlying data has changed since the `next_page_token` was | ||
| generated. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is ignored, i.e. the client just keeps paginating rather than starting over, is it set on every subsequent request as well? I can see why that would not really make sense; it would imply that the API continues to respond with next page tokens that reflect the previous state of the world, which is likely infeasible most of the time. But I'm wondering about the behavior of client libraries that wrap pagination with an iterator (or even just a sync method that concatenates a number of paged responses). How would those handle this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you're right that it is worded wrong. My intention was that As worded it would be extremely uncommon to be able to support this, of course, since you'd have to detect changes since the prior request rather than the initial request. So that wording would need to be changed -- as a client you really want to know whether the data has changed since the initial request. However, even beyond the wording, I also didn't really consider how such services would detect that the results had changed since their initial request, and... well, that doesn't seem like it's something that db systems normally support, nor does it seem reasonable to build in the application layer. The best implementation direction that is coming to me right now would be:
If detecting whether there were changes since the snapshot isn't really feasible, then both of the recommendations here aren't either. Anyone know of any db systems that have some cute trick up their sleeve to detect whether data would have changed since the snapshot? Assuming this isn't feasible, this PR would just collapse down to clarifying that APIs are usually built in a way that might return stale data when using the recommended paging cursors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the thoughtful analysis! Agreed that it's generally uncommon for databases to be able to tell you that the data was modified while it's reading in rows for the request. I think the clarification makes sense - we do have a "rationale" section that is available in aeps to explain why a particular choice was made: https://aep.dev/8/#document-structure. I'm thinking about the ramifications to the clients a little more - and I believe that inherently there is some understanding that any request point-in-time can quickly become stale. This is why, for example, HTTP has the concept of an ETAG for a resource to verify whether a resource has changed: https://aep.dev/154/. Declarative providers like Terraform perform a get to validate state before and after. UIs often provide a refresh button. So although I think it is an implicit expectation of clients to possibly read stale data, I think it's definitely worth a clarification, especially for list / pagination. |
||
| - Fields providing metadata about the list request (such as | ||
| `string next_page_token` or `int32 total_size`) **must** be included on the | ||
| response (not as part of the resource itself). | ||
|
|
@@ -156,11 +160,11 @@ return `200 OK` with an empty results array, and not `404 Not Found`. | |
|
|
||
| ### Advanced operations | ||
|
|
||
| `List` methods **may** allow an extended set of functionality to allow a user to | ||
| specify the resources that are returned in a response. | ||
| `List` methods **may** allow an extended set of functionality to allow a user | ||
| to specify the resources that are returned in a response. | ||
|
|
||
| The following table summarizes the applicable AEPs, ordered by the precedence of | ||
| the operation on the results. | ||
| The following table summarizes the applicable AEPs, ordered by the precedence | ||
| of the operation on the results. | ||
|
|
||
| | Operation | AEP | | ||
| | ---------------------------- | --------------------------------- | | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,9 +8,9 @@ be paginated. | |||||
|
|
||||||
| ## Guidance | ||||||
|
|
||||||
| Methods returning collections of data **must** provide pagination _at the outset_, | ||||||
| as it is a [backwards-incompatible change](#backwards-compatibility) to add | ||||||
| pagination to an existing method. | ||||||
| Methods returning collections of data **must** provide pagination _at the | ||||||
| outset_, as it is a [backwards-incompatible change](#backwards-compatibility) | ||||||
| to add pagination to an existing method. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prettier had some things to say about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rambleraptor re broken link on the site And yeah I'm surprised this got checked in, given that we have a prettierrc specifying 79-char columns and GH workflow that looks like it runs a prettier check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which version of prettier are you using? it didn't format these files for me, and it's good to dig into why. re the broken link I think we just need to fix the markdown, I'll see if I can file a PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used VSCode's integrated prettier by flipping the file type to markdown. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the difference has to do with the prettier version. Here's a PR to upgrade it: #280. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should all be fixed at this point. |
||||||
|
|
||||||
| {% tab proto %} | ||||||
|
|
||||||
|
|
@@ -50,8 +50,8 @@ message ListBooksResponse { | |||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| - The field containing pagination results **should** be the first field in | ||||||
| the message and have a field number of `1`. | ||||||
| - The field containing pagination results **should** be the first field in the | ||||||
| message and have a field number of `1`. | ||||||
|
|
||||||
| {% tab oas %} | ||||||
|
|
||||||
|
|
@@ -60,39 +60,39 @@ message ListBooksResponse { | |||||
| {% endtabs %} | ||||||
|
|
||||||
| - Request messages for collections **should** define an integer (`int32` for | ||||||
| protobuf) `max_page_size` field, allowing users to specify the maximum number of | ||||||
| results to return. | ||||||
| protobuf) `max_page_size` field, allowing users to specify the maximum number | ||||||
| of results to return. | ||||||
| - If the user does not specify `max_page_size` (or specifies `0`), the API | ||||||
| chooses an appropriate default, which the API **should** document. The API | ||||||
| **must not** return an error. | ||||||
| - If the user specifies `max_page_size` greater than the maximum permitted by the | ||||||
| API, the API **should** coerce down to the maximum permitted page size. | ||||||
| - If the user specifies a negative value for `max_page_size`, the API **must** | ||||||
| send an `INVALID_ARGUMENT` error. | ||||||
| - If the user specifies `max_page_size` greater than the maximum permitted by | ||||||
| the API, the API **should** coerce down to the maximum permitted page size. | ||||||
| - If the user specifies a negative value for `max_page_size`, the API | ||||||
| **must** send an `INVALID_ARGUMENT` error. | ||||||
| - The API **may** return fewer results than the number requested (including | ||||||
| zero results), even if not at the end of the collection. | ||||||
| - Request schemas for collections **should** define a `string page_token` | ||||||
| field, allowing users to advance to the next page in the collection. | ||||||
| - The `page_token` field **must not** be required. | ||||||
| - If the user changes the `max_page_size` in a request for subsequent pages, the | ||||||
| service **must** honor the new page size. | ||||||
| - If the user changes the `max_page_size` in a request for subsequent pages, | ||||||
| the service **must** honor the new page size. | ||||||
| - The user is expected to keep all other arguments to the method the same; if | ||||||
| any arguments are different, the API **should** send an `INVALID_ARGUMENT` | ||||||
| error. | ||||||
| - The response **must not** be a streaming response. | ||||||
| - Response messages for collections **must** define a | ||||||
| `string next_page_token` field, providing the user with a page token that may | ||||||
| be used to retrieve the next page. | ||||||
| - The field containing pagination results **must** be a repeated | ||||||
| field containing a list of resources constituting a single page of results. | ||||||
| - Response messages for collections **must** define a `string next_page_token` | ||||||
| field, providing the user with a page token that may be used to retrieve the | ||||||
| next page. | ||||||
| - The field containing pagination results **must** be a repeated field | ||||||
| containing a list of resources constituting a single page of results. | ||||||
| - If the end of the collection has been reached, the `next_page_token` field | ||||||
| **must** be empty. This is the _only_ way to communicate | ||||||
| "end-of-collection" to users. | ||||||
| - If the end of the collection has not been reached (or if the API can not | ||||||
| determine in time), the API **must** provide a `next_page_token`. | ||||||
| - Response messages for collections **may** provide an integer (`int32` for | ||||||
| protobuf) `total_size` field, providing the user with the total number of items | ||||||
| in the list. | ||||||
| protobuf) `totalSize` field, providing the user with the total number of | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Protobuf fields are in snake_case. |
||||||
| items in the list. | ||||||
| - This total **may** be an estimate. If so, the API **should** explicitly | ||||||
| document that. | ||||||
|
|
||||||
|
|
@@ -148,10 +148,30 @@ used. It is not necessary to document this behavior. | |||||
| **Note:** While a reasonable time may vary between APIs, a good rule of thumb | ||||||
| is three days. | ||||||
|
|
||||||
| ### Freshness | ||||||
|
|
||||||
| Since a request with the same filter and `page_token` **should** return the | ||||||
| same results unless the page token has expired, items returned **may** have | ||||||
| been deleted or modified since the initial request, and paging through the list | ||||||
| **may** not include all current items. | ||||||
|
|
||||||
| If the service is capable of detecting underlying changes in the listing since | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Not sure about the term "the listing" in this context...sounds to me like a marketplace term. |
||||||
| a given `page_token` was generated: | ||||||
|
|
||||||
| - The response **may** include a `has_changed` boolean. If this boolean is | ||||||
| supported but `false` or unset, the data **must** be up to date, and paging | ||||||
| through the list **must** include all current items. | ||||||
| - If no `has_changed` boolean is included, the API **may** expire the page | ||||||
| token. | ||||||
|
|
||||||
| If the service is incapable of detecting such changes, for example if it is | ||||||
| using an in-memory storage mechanism without snapshots, it **may** return stale | ||||||
| data or phantom reads with no indication. | ||||||
|
Comment on lines
+168
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it document that this may happen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, should would maintain backward compatibility, so that sounds like a good idea. |
||||||
|
|
||||||
| ### Backwards compatibility | ||||||
|
|
||||||
| Adding pagination to an existing method is a backwards-incompatible change. This | ||||||
| may seem strange; adding fields to proto messages is generally backwards | ||||||
| Adding pagination to an existing method is a backwards-incompatible change. | ||||||
| This may seem strange; adding fields to proto messages is generally backwards | ||||||
| compatible. However, this change is _behaviorally_ incompatible. | ||||||
|
|
||||||
| Consider a user whose collection has 75 resources, and who has already written | ||||||
|
|
@@ -174,4 +194,4 @@ paginate) is reasonable for initially-small collections. | |||||
|
|
||||||
| ## Changelog | ||||||
|
|
||||||
| - **2024-04-14**: Imported from https://google.aip.dev/158. | ||||||
| - **2024-04-14**: Imported from https://google.aip.dev/158. | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_changedreads as a little vague to me. Mayberesults_changed?I kind of wanted to suggest something like
page_token_expired, but I think that's overloading the concept. :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later this has
results_changedsounds better, yeah