feat(158): suggest has_changed flag and clarify expectations about stale paginated list data #278

gregrperkins-roblox · 2025-02-14T20:34:02Z

The pagination AEP-158 doesn't give clear recommendations about what to do if the underlying data has changed during the course of a paged request. Normally the recommended cursor-based pagination strategy would continue serving stale data.

This PR suggests some changes to clarify this expectation, and provide a workaround when data freshness is critical.

Specifically, it adds a has_changed boolean to the list response, which clients that need up-to-date data can use to show an indicator and add a refresh listing action to their user interface.

🍱 Types of changes

Enhancement
New proposal
Migrated from google.aip.dev
Chore / Quick Fix

General

Basic Guidance is met.
Ensure that your PR
references AEPs
correctly.
My code has been formatted
(usually prettier -w .)

💝 Thank you!

The pagination AIP-158 doesn't give clear recommendations about what to do if the underlying data has changed during the course of a paged request. Normally the recommended cursor-based pagination strategy would continue serving stale data. This PR suggests some language to clarify this expectation, and provide a workaround when data freshness is critical. Specifically, it adds an optional `has_changed` boolean to the response, which clients that need up-to-date data can use to show an indicator and add a refresh listing action to their user interface. (roblox internal DSA-4035)

…alSize

gregrperkins-roblox · 2025-02-14T20:37:07Z

aep/general/0158/aep.md.j2

-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.


Prettier had some things to say about the main branch version of these files. I assume including its output verbatim is the preferred behavior, since https://aep.dev/contributing/#formatting is a broken link (no "formatting" section on that page).

@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.

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.

I used VSCode's integrated prettier by flipping the file type to markdown.

I think the difference has to do with the prettier version. Here's a PR to upgrade it: #280.

This should all be fixed at this point.

rofrankel · 2025-02-14T20:49:02Z

aep/general/0132/aep.md.j2

@@ -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.


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?

Ah, you're right that it is worded wrong. My intention was that has_changed would be marked on all requests after there was a change. Services whose databases use insensitive (snapshot-based) cursors, like Postgres, could currently keep serving stale results from their db snapshot -- assuming they can keep track of the db cursor based on the next_page_tokens.

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:

keep an updated_time db field

store a timestamp with the db cursor in the application layer that can be looked up based on the next_page_token

rerun the underlying list query every request but with a constraint to check if any of the fields had been updated since the db cursor was created

...but this doesn't handle deletions... that might require something like postgres logical replication or some oracle aq beast of a system

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.

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.

rofrankel · 2025-02-14T20:52:46Z

aep/general/0158/aep.md.j2

-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.


@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.

rofrankel · 2025-02-14T20:53:27Z

aep/general/0158/aep.md.j2

  - 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


Suggested change

protobuf) `totalSize` field, providing the user with the total number of

protobuf) `total_size` field, providing the user with the total number of

Protobuf fields are in snake_case.

rofrankel · 2025-02-14T20:55:39Z

aep/general/0158/aep.md.j2

+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


Suggested change

If the service is capable of detecting underlying changes in the listing since

If the service is capable of detecting underlying changes in the list of results since

Not sure about the term "the listing" in this context...sounds to me like a marketplace term.

rofrankel · 2025-02-14T20:56:41Z

aep/general/0158/aep.md.j2

+using an in-memory storage mechanism without snapshots, it **may** return stale
+data or phantom reads with no indication.


Should it document that this may happen?

Yes, should would maintain backward compatibility, so that sounds like a good idea.

rofrankel · 2025-02-14T20:59:17Z

aep/general/0132/aep.md.j2

@@ -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


has_changed reads as a little vague to me. Maybe results_changed?

I kind of wanted to suggest something like page_token_expired, but I think that's overloading the concept. :/

Later this has

the API may expire the page token.

results_changed sounds better, yeah

gregrperkins-roblox and others added 7 commits February 14, 2025 11:46

Also add relevant suggestions to AEP-132 listing

4287844

Prettier; field name consistency between AEP-158 and AEP-132

91c7d13

Add example per style guidelines

a3508ca

total_size would be a breaking change, gotta normalize it back to tot…

e3683bd

…alSize

Undo irrelevant casing changes, leave inconsistency

50b635f

Oops missed one

f95aef4

gregrperkins-roblox requested a review from a team as a code owner February 14, 2025 20:34

gregrperkins-roblox commented Feb 14, 2025

View reviewed changes

Remove incorrect filter AEP reference

8b41a1d

rofrankel reviewed Feb 14, 2025

View reviewed changes

	protobuf) `totalSize` field, providing the user with the total number of
	protobuf) `total_size` field, providing the user with the total number of

	If the service is capable of detecting underlying changes in the listing since
	If the service is capable of detecting underlying changes in the list of results since

		using an in-memory storage mechanism without snapshots, it may return stale
		data or phantom reads with no indication.

feat(158): suggest has_changed flag and clarify expectations about stale paginated list data #278

Are you sure you want to change the base?

feat(158): suggest has_changed flag and clarify expectations about stale paginated list data #278

Uh oh!

Conversation

gregrperkins-roblox commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🍱 Types of changes

General

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregrperkins-roblox Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregrperkins-roblox commented Feb 14, 2025 •

edited

Loading

gregrperkins-roblox Feb 16, 2025 •

edited

Loading