Conversation
Extend PageToken with a Cursor []any field and a NextCursor(msg, fields...) helper that reads named proto fields off a message (typically the last row of the current page) into Cursor via protoreflect. The existing Next(request) continues to advance Offset for offset-based pagination; NextCursor is an additional entry point for key-set pagination. Why carry Cursor alongside Offset on the same PageToken: Page tokens are server-opaque. AIP-158 specifies that the server defines the format and contents of page_token, and clients MUST treat them as opaque strings (https://google.aip.dev/158#page-tokens). Nothing in the AIP binds a resource to a single pagination strategy — offset-style `skip` (https://google.aip.dev/158#skipping-results) and key-set cursors are complementary, not mutually exclusive. A handler can issue offset tokens for early pages (cheap, simple) and transition to cursor tokens once it has a stable ordering key, all behind the same opaque token. Keeping both fields on one struct: - lets a handler switch between modes mid-stream without changing token type - preserves backward compatibility — gob decodes legacy {Offset, RequestChecksum} tokens with Cursor left nil (covered by the legacy-decode test) - keeps RequestChecksum as the single guard against request drift between pages NextCursor rejects repeated/map and message fields (ambiguous cursor encoding) and normalizes enums to int32 so gob can round-trip them.
Decode google.protobuf.Timestamp via AsTime() and google.protobuf.Duration via AsDuration() when building a key-set cursor, so callers can order by well-known time fields without hitting the generic "is a message (unsupported)" error. Other message types remain rejected.
|
@odsod @fredrikaverpil any chance to look at my PR? |
|
@odsod did you have a chance to look at this PR? Let me know if you are keen to accept it or not. |
|
@iamralch sorry for the delay in getting a review. We haven't been able to get to this one yet due to other priorities. I can't tell you right now if we want to adopt this, since it can be seen as a footgun to mix both the offset-based and cursor-based pagination into the same struct. We'll discuss this internally but unfortunately, I cannot give you a timeframe. You don't have to ping Oscar because he's no longer with the company. |
|
Hi @fredrikaverpil, thanks for the update and for the heads-up about Oscar. I definitely understand having other priorities! I was mostly just worried the PR had slipped through the cracks, so I appreciate the acknowledgment. I’m happy to be patient while you discuss it internally. Looking forward to hearing your thoughts on the approach whenever you have a moment. |
|
@fredrikaverpil did u have a chance to discuss the feature internally? |
alethenorio
left a comment
alethenorio
left a comment
There was a problem hiding this comment.
Hi @iamralch
I am starting to take a look at your PR and left some comments.
I am not I understand the use case for supporting "switching modes mid-stream". What is the use case? I do however see the value in a service switching from a offset-based to a cursor-based approach over time (maybe that's what you mean?) so I think potentially reusing the same struct might make some sense.
Also I think there may some issues with token decoding and encoding using the gob package when using non-native types such as time.Duration and time.Time so maybe add some tests for those?
| // Offset of the page. | ||
| Offset int64 | ||
| // Cursor for key set pagination. | ||
| Cursor []any |
There was a problem hiding this comment.
I'm thinking that we do not need to support a token going from cursor to offset midway but we do want to support a service switching implementations and being able to discern which type of token was used when receiving a request. Does it make sense to add some sort of enum field which we could validate against to know which types was used?
|
|
||
| // NextCursor returns the next page token for key set pagination by reading the | ||
| // named fields off msg (typically the last row of the current page) into Cursor. | ||
| func (p PageToken) NextCursor(msg proto.Message, fields ...string) (PageToken, error) { |
There was a problem hiding this comment.
Rather than using a generic string here, would it make sense to use a ordering.Field list? This way we could make it easier for the service using the package to derive this directly from the parsed orderBy value from the request, WDYT?
3 participants
Summary
Cursor []anytoPageTokenso a single opaque token can carry key-set state alongsideOffset.PageToken.NextCursor(msg proto.Message, fields ...string) (PageToken, error)— reads named fields offmsg(typically the last row of the current page) viaprotoreflectand stores them inCursor. Enums are normalized toint32; repeated/map/message fields are rejected.Next(request)continues to advanceOffsetfor offset-style pagination. The two modes are independent on the same token.Why both on one token
Per AIP-158,
page_tokenis server-opaque — the server defines its contents. Offset (viaskip) and key-set cursors are complementary, not mutually exclusive. A handler can issue offset tokens for early pages and switch to cursor tokens once a stable ordering key is available, all behind the same opaque string.Keeping both fields on one struct:
{Offset, RequestChecksum}tokens gob-decode withCursorleftnil(covered by an existing legacy-decode test)RequestChecksumas the single guard against request drift between pagesTest plan
go test ./pagination/— existing suite + new cursor round-trip,NextCursorpopulation, and unknown-field error tests passCursortoken shape still parses withCursor == nil)