From c1497e6a7ce5721697c63c5da99fbf44e57ee081 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 17:05:24 +0400 Subject: [PATCH 1/3] fix!: a page token is only good for the request it was issued for The token's checksum covered only the cursor, so a client could change its `filter` between pages and keep its `page_token`: the server paged on from that position under a predicate the token was never cut for, returning pages that matched neither filter, without an error. AIP-158 requires every field but `page_token` and `page_size` to stay the same between pages, and `aip-go`'s `CalculateRequestChecksum` enforces it. The token now carries a digest of the request's fields -- whatever the caller passes, typically parent, filter and order_by -- next to the cursor. `parse` recomputes it from the request in hand and returns `CursorError::RequestMismatch` when they differ. The digest is kept in its own field so a changed request is told apart from a corrupted token, and the checksum covers it, so rewriting it is still `TokenChecksumMismatch`. Fields are length-prefixed, so moving text across a field boundary is a different request. The empty token is still the first page for any request. `CURSOR_TOKEN_CHECKSUM_MASK` is bumped, so tokens issued before this are rejected cleanly rather than misread. Closes #13 BREAKING CHANGE: `Cursor::encode` and `Cursor::parse` take the request's fields, `&[&str]`, in the same order on both sides. Tokens issued by earlier versions no longer parse. --- README.md | 19 +- crates/sqlx-query/src/clauses/pager.rs | 192 ++++++++++++++++---- crates/sqlx-query/tests/composer.rs | 2 +- crates/sqlx-query/tests/execute.rs | 8 +- crates/sqlx-query/tests/execute_postgres.rs | 4 +- 5 files changed, 180 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 10c4f65..b4765dc 100644 --- a/README.md +++ b/README.md @@ -237,8 +237,12 @@ into a `Page` — the rows, and the cursor to the next page if that extra row came back: ```rust +// Every request field but page_token and page_size: a token is only good for +// the request it was issued for. +let request = &[parent, filter, order_by_str]; + // The request's token; the empty one -- the first page -- is the empty cursor. -let cursor = Cursor::parse(&page_token)?; +let cursor = Cursor::parse(&page_token, request)?; let pager = Pager::new(Cursor::new(order_by.clone()), 50); let mut query = QueryComposer::::new(LIST_USERS); @@ -254,7 +258,7 @@ let page = pager.next_page(rows)?; // Page { rows: at most 50, cursor } // The cursor's values come off the page's last row, so no one has to know // each key's Rust type. On the last page it is the empty cursor, which // encodes as the empty token. -let next_page_token = page.cursor.encode(); +let next_page_token = page.cursor.encode(request); ``` The empty token and the empty cursor are each other's image, as AIP-158 reads @@ -272,9 +276,14 @@ against the column on the next page. The token is the cursor `postcard`-serialized, checksummed and base64'd, following `einride/aip-go`'s `pagination.PageToken` shape — opaque, and cleanly -rejected if hand-edited. A cursor carries the `order_by` it was built against, -so it doesn't have to be repeated; if it *is* set and disagrees, `compose()` -fails rather than paging through a different sort than the token was cut for. +rejected if hand-edited. It also carries a digest of the request fields it was +issued for, so a client that changes its `filter` (or anything else but +`page_token` and `page_size`) between pages gets `CursorError::RequestMismatch` +rather than a page that matches neither filter, as AIP-158 requires. + +A cursor carries the `order_by` it was built against, so it doesn't have to be +repeated; if it *is* set and disagrees, `compose()` fails rather than paging +through a different sort than the token was cut for. ## Binding diff --git a/crates/sqlx-query/src/clauses/pager.rs b/crates/sqlx-query/src/clauses/pager.rs index 24fde3a..b18de62 100644 --- a/crates/sqlx-query/src/clauses/pager.rs +++ b/crates/sqlx-query/src/clauses/pager.rs @@ -14,13 +14,22 @@ //! token's checksum will mismatch and be cleanly rejected rather than //! misread. //! -//! That checksum only guards against a corrupted or hand-edited token; it -//! is not a substitute for +//! The token also carries a digest of the request it was issued for — the +//! caller's own list of fields, typically the raw `filter` and `order_by` +//! strings and any scope such as the parent. AIP-158 requires every field +//! but `page_token` and `page_size` to stay the same between pages, as +//! `einride/aip-go`'s `CalculateRequestChecksum` does; without this, a +//! client could change its `filter` and keep its token, and be paged on from +//! that position under a predicate the token was never cut for. +//! [`Cursor::parse`] recomputes the digest from the request in hand and +//! rejects a token issued for a different one. +//! +//! Neither checksum stands in for //! [`QueryComposer::compose`](crate::QueryComposer::compose)'s -//! `CursorOrderByMismatch` check, which is what catches a client paging -//! with one `order_by` and then switching to another — that check compares -//! the actual decoded `OrderByClause`, a stronger guarantee than a hash -//! could give. +//! [`CursorMismatch`](crate::Error::CursorMismatch) check, which compares +//! the actual decoded `OrderByClause` against an explicit one — a stronger +//! guarantee than a hash could give, for the one field the cursor itself +//! depends on. //! //! [`Cursor::after_row`] fills values straight from a `sqlx::Row` (the //! last row of a page, once it's been fetched) instead of requiring the @@ -48,9 +57,15 @@ use crate::{OrderByClause, OrderDirection, Value, WhereClause}; /// payload, mirroring `einride/aip-go`'s `PageToken { Offset, checksum /// RequestChecksum }`. Private: callers only ever see the base64 string /// [`Cursor::encode`] returns, never this type. +/// +/// `request` is the digest of the request the token was issued for, kept +/// apart from `checksum` so [`Cursor::parse`] can tell a token issued for +/// another request from a corrupted one. `checksum` covers it too, so +/// editing it is corruption like any other. #[derive(Serialize, Deserialize)] struct CursorToken { checksum: u32, + request: u32, cursor: Cursor, } @@ -97,6 +112,9 @@ pub enum CursorError { #[error("cursor token checksum mismatch — the token was corrupted or tampered with")] TokenChecksumMismatch, + #[error("cursor token was issued for a different request — only page_token and page_size may change between pages")] + RequestMismatch, + #[error("row has no column named `{0}` for an order_by key")] RowColumnMissing(String), @@ -135,13 +153,19 @@ impl Cursor { /// An empty or all-whitespace token is the [empty](Self::is_empty) /// cursor, the first page, as AIP-158 reads it. /// + /// `request` is the current request's fields, in the order + /// [`encode`](Self::encode) was given them — every field but + /// `page_token` and `page_size`, e.g. `&[parent, filter, order_by]`. + /// /// # Errors /// /// [`CursorError::TokenInvalidBase64`] or /// [`CursorError::TokenMalformed`] for a token that isn't one this type - /// produced, and [`CursorError::TokenChecksumMismatch`] for one that was - /// corrupted or hand-edited after it was issued. - pub fn parse(token: &str) -> Result { + /// produced, [`CursorError::TokenChecksumMismatch`] for one that was + /// corrupted or hand-edited after it was issued, and + /// [`CursorError::RequestMismatch`] for one issued for a different + /// `request`. + pub fn parse(token: &str, request: &[&str]) -> Result { use base64::Engine as _; if token.trim().is_empty() { @@ -155,9 +179,12 @@ impl Cursor { let wire: CursorToken = postcard::from_bytes(&bytes).map_err(|_| CursorError::TokenMalformed)?; - if checksum_of(&wire.cursor) != wire.checksum { + if checksum_of(wire.request, &wire.cursor) != wire.checksum { return Err(CursorError::TokenChecksumMismatch); } + if digest_of(request) != wire.request { + return Err(CursorError::RequestMismatch); + } Ok(wire.cursor) } @@ -166,6 +193,11 @@ impl Cursor { /// back to the client as-is. See the module docs for the wire format /// and what the checksum does and doesn't guard against. /// + /// `request` is the fields of the request this page answers — every one + /// but `page_token` and `page_size`, e.g. `&[parent, filter, order_by]`. + /// [`parse`](Self::parse) only accepts the token back for the same + /// fields, in the same order. + /// /// The [empty](Self::is_empty) cursor encodes as the empty string, the /// token that says there is no next page. /// @@ -174,15 +206,17 @@ impl Cursor { /// If any key's value is unset — unreachable through this type's public /// API, same as [`to_where_clause`](Self::to_where_clause). #[must_use] - pub fn encode(&self) -> String { + pub fn encode(&self, request: &[&str]) -> String { use base64::Engine as _; if self.is_empty() { return String::new(); } + let request = digest_of(request); let token = CursorToken { - checksum: checksum_of(self), + checksum: checksum_of(request, self), + request, cursor: self.clone(), }; let bytes = postcard::to_allocvec(&token).expect("CursorToken serialization is infallible"); @@ -414,19 +448,30 @@ fn placeholder_index(keys: &[CursorKey], key: &CursorKey) -> usize { /// XORed into the checksum on both [`Cursor::encode`] and /// [`Cursor::parse`] — see the module docs for what bumping this buys. -const CURSOR_TOKEN_CHECKSUM_MASK: u32 = 0x5eed_c0de; - -/// `cursor`'s checksum, as stored in / verified against a -/// [`CursorToken`]: a CRC32 of `cursor`'s own `postcard` encoding, masked -/// by [`CURSOR_TOKEN_CHECKSUM_MASK`]. Deterministic — `postcard`'s output -/// for the same value is always the same bytes — so [`Cursor::parse`] can -/// recompute this straight from the decoded `cursor` field rather than -/// needing the original payload bytes kept around separately. -fn checksum_of(cursor: &Cursor) -> u32 { - let payload = postcard::to_allocvec(cursor).expect("Cursor serialization is infallible"); +/// Last bumped when [`CursorToken`] gained its `request` digest. +const CURSOR_TOKEN_CHECKSUM_MASK: u32 = 0x5eed_c0df; + +/// The token's checksum, as stored in / verified against a +/// [`CursorToken`]: a CRC32 of the `postcard` encoding of its `request` +/// digest and `cursor`, masked by [`CURSOR_TOKEN_CHECKSUM_MASK`]. +/// Deterministic — `postcard`'s output for the same value is always the +/// same bytes — so [`Cursor::parse`] can recompute this straight from the +/// decoded fields rather than needing the original payload bytes kept +/// around separately. +fn checksum_of(request: u32, cursor: &Cursor) -> u32 { + let payload = + postcard::to_allocvec(&(request, cursor)).expect("Cursor serialization is infallible"); crc32(&payload) ^ CURSOR_TOKEN_CHECKSUM_MASK } +/// The digest of a request's fields, as stored in a [`CursorToken`]: a +/// CRC32 of their `postcard` encoding, which prefixes each field with its +/// length — so `["a b"]`, `["a", "b"]` and `["ab", ""]` all differ. +fn digest_of(request: &[&str]) -> u32 { + let payload = postcard::to_allocvec(request).expect("&str serialization is infallible"); + crc32(&payload) +} + /// IEEE CRC32 (the same variant Go's `hash/crc32.ChecksumIEEE` computes), /// hand-rolled bitwise rather than table-driven — this only ever runs once /// per `encode`/`parse` call over a few dozen bytes, so the table's @@ -451,6 +496,10 @@ mod tests { use super::*; + /// A request's fields other than `page_token` and `page_size`: parent, + /// filter, `order_by`. + const REQUEST: &[&str] = &["publishers/1", "rank > 0", "rank desc"]; + #[test] fn builds_tuple_comparison_for_two_keys() { let order_by = OrderByClause::parse("rank desc, id asc").unwrap(); @@ -504,7 +553,7 @@ mod tests { .after(vec![Value::Int(42), Value::String("alice".into())]) .unwrap(); - let parsed = Cursor::parse(&cursor.encode()).unwrap(); + let parsed = Cursor::parse(&cursor.encode(REQUEST), REQUEST).unwrap(); assert_eq!(parsed.to_order_by_clause(), order_by); assert_eq!(parsed.to_where_clause(), cursor.to_where_clause()); @@ -528,7 +577,7 @@ mod tests { ]) .unwrap(); - let parsed = Cursor::parse(&cursor.encode()).unwrap(); + let parsed = Cursor::parse(&cursor.encode(REQUEST), REQUEST).unwrap(); assert_eq!( parsed.to_where_clause().values(), @@ -538,14 +587,14 @@ mod tests { #[test] fn parse_rejects_invalid_base64() { - let err = Cursor::parse("not valid base64!!").unwrap_err(); + let err = Cursor::parse("not valid base64!!", REQUEST).unwrap_err(); assert_eq!(err, CursorError::TokenInvalidBase64); } #[test] fn parse_rejects_garbage_bytes() { let token = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode([1u8, 2, 3]); - let err = Cursor::parse(&token).unwrap_err(); + let err = Cursor::parse(&token, REQUEST).unwrap_err(); assert_eq!(err, CursorError::TokenMalformed); } @@ -553,7 +602,7 @@ mod tests { fn parse_rejects_corrupted_token() { let order_by = OrderByClause::parse("rank desc").unwrap(); let cursor = Cursor::new(order_by).after(vec![Value::Int(42)]).unwrap(); - let token = cursor.encode(); + let token = cursor.encode(REQUEST); let mut wire = base64::engine::general_purpose::URL_SAFE_NO_PAD .decode(&token) @@ -562,7 +611,7 @@ mod tests { wire[last] ^= 0xFF; let tampered = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(&wire); - let err = Cursor::parse(&tampered).unwrap_err(); + let err = Cursor::parse(&tampered, REQUEST).unwrap_err(); assert!(matches!( err, CursorError::TokenChecksumMismatch | CursorError::TokenMalformed @@ -574,10 +623,13 @@ mod tests { #[test] fn the_empty_token_is_the_empty_cursor() { for token in ["", " "] { - assert!(Cursor::parse(token).unwrap().is_empty(), "{token:?}"); + assert!( + Cursor::parse(token, REQUEST).unwrap().is_empty(), + "{token:?}" + ); } assert!(Cursor::default().is_empty()); - assert_eq!(Cursor::default().encode(), ""); + assert_eq!(Cursor::default().encode(REQUEST), ""); assert!(Cursor::default().to_where_clause().is_empty()); assert!(Cursor::default().to_order_by_clause().keys().is_empty()); @@ -585,7 +637,7 @@ mod tests { .after(vec![Value::Int(42)]) .unwrap(); assert!(!positioned.is_empty()); - assert!(!positioned.encode().is_empty()); + assert!(!positioned.encode(REQUEST).is_empty()); } #[test] @@ -603,7 +655,81 @@ mod tests { let order_by = OrderByClause::parse("rank desc").unwrap(); let cursor = Cursor::new(order_by).after(vec![Value::Int(42)]).unwrap(); - let payload = postcard::to_allocvec(&cursor).unwrap(); - assert_ne!(checksum_of(&cursor), crc32(&payload)); + let request = digest_of(REQUEST); + let payload = postcard::to_allocvec(&(request, &cursor)).unwrap(); + assert_ne!(checksum_of(request, &cursor), crc32(&payload)); + } + + fn rank_token(request: &[&str]) -> String { + Cursor::new(OrderByClause::parse("rank desc").unwrap()) + .after(vec![Value::Int(42)]) + .unwrap() + .encode(request) + } + + /// AIP-158: every field but `page_token` and `page_size` stays the same + /// between pages, so a token is only good for the request it came from. + #[test] + fn parse_rejects_a_token_issued_for_another_request() { + let token = rank_token(REQUEST); + + for request in [ + &["publishers/1", "rank > 10", "rank desc"][..], + &["publishers/2", "rank > 0", "rank desc"], + &["publishers/1", "rank > 0"], + &[], + ] { + assert_eq!( + Cursor::parse(&token, request).unwrap_err(), + CursorError::RequestMismatch, + "{request:?}" + ); + } + } + + /// The fields are length-prefixed, so moving text across a field + /// boundary is a different request, not the same bytes. + #[test] + fn the_request_digest_keeps_field_boundaries() { + let token = rank_token(&["a b", ""]); + + assert!(Cursor::parse(&token, &["a b", ""]).is_ok()); + for request in [&["a", "b"][..], &["a b"], &["", "a b"], &["a b", "", ""]] { + assert_eq!( + Cursor::parse(&token, request).unwrap_err(), + CursorError::RequestMismatch, + "{request:?}" + ); + } + } + + /// Rewriting the token's request digest to match another request is + /// corruption like any other edit: the checksum covers it. + #[test] + fn parse_rejects_a_rewritten_request_digest() { + let token = rank_token(REQUEST); + let other = &["publishers/1", "rank > 10", "rank desc"]; + + let bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(&token) + .unwrap(); + let mut wire: CursorToken = postcard::from_bytes(&bytes).unwrap(); + wire.request = digest_of(other); + let rewritten = base64::engine::general_purpose::URL_SAFE_NO_PAD + .encode(postcard::to_allocvec(&wire).unwrap()); + + assert_eq!( + Cursor::parse(&rewritten, other).unwrap_err(), + CursorError::TokenChecksumMismatch + ); + } + + /// The first page has no position to carry, so the empty token is good + /// for any request. + #[test] + fn the_empty_token_is_good_for_any_request() { + assert!(Cursor::parse("", &["publishers/2", "rank > 10"]) + .unwrap() + .is_empty()); } } diff --git a/crates/sqlx-query/tests/composer.rs b/crates/sqlx-query/tests/composer.rs index fb76af8..1cd4702 100644 --- a/crates/sqlx-query/tests/composer.rs +++ b/crates/sqlx-query/tests/composer.rs @@ -438,7 +438,7 @@ fn an_empty_cursor_is_no_cursor() { let mut first = QueryComposer::::new(sql); first .push_order_by(OrderByClause::parse("rank desc").unwrap()) - .with_cursor(Cursor::parse("").unwrap()); + .with_cursor(Cursor::parse("", &[]).unwrap()); let mut untouched = QueryComposer::::new(sql); untouched.push_order_by(OrderByClause::parse("rank desc").unwrap()); diff --git a/crates/sqlx-query/tests/execute.rs b/crates/sqlx-query/tests/execute.rs index 2791634..76b43f1 100644 --- a/crates/sqlx-query/tests/execute.rs +++ b/crates/sqlx-query/tests/execute.rs @@ -155,13 +155,13 @@ async fn a_page_token_round_trips_into_a_running_query() { let token = Cursor::new(order_by) .after_row(rows.last().expect("a row")) .expect("cursor") - .encode(); + .encode(&["acme"]); let mut second = QueryComposer::::new(LIST_ORDERS); second .bind_value("acme") .bind_value(1i64) - .with_cursor(Cursor::parse(&token).expect("token parses")); + .with_cursor(Cursor::parse(&token, &["acme"]).expect("token parses")); let rows = second .build() .expect("composes") @@ -349,7 +349,7 @@ async fn pages(pool: &SqlitePool, size: usize) -> Vec> { .bind_value("acme") .bind_value(pager.limit()) .push_order_by(order_by.clone()) - .with_cursor(Cursor::parse(&token).expect("the token parses")); + .with_cursor(Cursor::parse(&token, &["acme"]).expect("the token parses")); let rows = query .build() @@ -360,7 +360,7 @@ async fn pages(pool: &SqlitePool, size: usize) -> Vec> { let page = pager.next_page(rows).expect("the row carries every key"); seen.push(page.rows.iter().map(|row| row.get("id")).collect()); - token = page.cursor.encode(); + token = page.cursor.encode(&["acme"]); if token.is_empty() { return seen; } diff --git a/crates/sqlx-query/tests/execute_postgres.rs b/crates/sqlx-query/tests/execute_postgres.rs index ec10399..ee4e92c 100644 --- a/crates/sqlx-query/tests/execute_postgres.rs +++ b/crates/sqlx-query/tests/execute_postgres.rs @@ -303,12 +303,12 @@ async fn a_cursor_pages_on_a_uuid_key() { let token = Cursor::new(order_by) .after_row(&rows[0]) .expect("a uuid key decodes") - .encode(); + .encode(&[]); let mut second = QueryComposer::::new(sql); second .bind_value(10i64) - .with_cursor(Cursor::parse(&token).expect("the token parses")); + .with_cursor(Cursor::parse(&token, &[]).expect("the token parses")); let rows = second .build() .expect("composes") From 0b4a864aac5887a44411c29b1da211c689e37cc9 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 17:11:33 +0400 Subject: [PATCH 2/3] fix!: check the filter against the cursor in the composer, like order_by Replaces the request digest in the page token. The cursor now carries the filter it was issued under, set with `Cursor::with_filter`, and `compose()` checks the `push_where` clauses against it the same way it checks an explicit `order_by`: a changed, added or dropped filter is `Error::CursorMismatch`. Comparing the decoded `WhereClause` is a stronger guarantee than a hash, and `encode`/`parse` keep their signatures. `with_filter` accumulates with `and`, as `push_where` does, so the same clauses in the same order combine to the same clause on both sides. `CURSOR_TOKEN_CHECKSUM_MASK` is bumped, so tokens issued before this are rejected cleanly rather than misread. Closes #13 BREAKING CHANGE: a cursor used with `push_where` must carry the same filter: build the pager's cursor with `Cursor::new(order_by) .with_filter(filter)`, or `compose()` returns `Error::CursorMismatch`. Tokens issued by earlier versions no longer parse. --- README.md | 31 +-- crates/sqlx-query/src/clauses/pager.rs | 280 +++++++++----------- crates/sqlx-query/src/composer.rs | 50 +++- crates/sqlx-query/tests/composer.rs | 75 +++++- crates/sqlx-query/tests/execute.rs | 56 +++- crates/sqlx-query/tests/execute_postgres.rs | 4 +- 6 files changed, 294 insertions(+), 202 deletions(-) diff --git a/README.md b/README.md index b4765dc..c633503 100644 --- a/README.md +++ b/README.md @@ -237,18 +237,17 @@ into a `Page` — the rows, and the cursor to the next page if that extra row came back: ```rust -// Every request field but page_token and page_size: a token is only good for -// the request it was issued for. -let request = &[parent, filter, order_by_str]; - // The request's token; the empty one -- the first page -- is the empty cursor. -let cursor = Cursor::parse(&page_token, request)?; -let pager = Pager::new(Cursor::new(order_by.clone()), 50); +let cursor = Cursor::parse(&page_token)?; +// The next page's cursor carries the filter, so that page can be checked +// against it. +let pager = Pager::new(Cursor::new(order_by.clone()).with_filter(filter.clone()), 50); let mut query = QueryComposer::::new(LIST_USERS); query .bind_value(tenant_id) .bind_value(pager.limit()) // 51: one past the page + .push_where(filter) .push_order_by(order_by) .with_cursor(cursor); // where the previous page left off, if anywhere @@ -258,7 +257,7 @@ let page = pager.next_page(rows)?; // Page { rows: at most 50, cursor } // The cursor's values come off the page's last row, so no one has to know // each key's Rust type. On the last page it is the empty cursor, which // encodes as the empty token. -let next_page_token = page.cursor.encode(request); +let next_page_token = page.cursor.encode(); ``` The empty token and the empty cursor are each other's image, as AIP-158 reads @@ -276,14 +275,16 @@ against the column on the next page. The token is the cursor `postcard`-serialized, checksummed and base64'd, following `einride/aip-go`'s `pagination.PageToken` shape — opaque, and cleanly -rejected if hand-edited. It also carries a digest of the request fields it was -issued for, so a client that changes its `filter` (or anything else but -`page_token` and `page_size`) between pages gets `CursorError::RequestMismatch` -rather than a page that matches neither filter, as AIP-158 requires. - -A cursor carries the `order_by` it was built against, so it doesn't have to be -repeated; if it *is* set and disagrees, `compose()` fails rather than paging -through a different sort than the token was cut for. +rejected if hand-edited. A cursor carries the `order_by` it was built against, +so it doesn't have to be repeated; if it *is* set and disagrees, `compose()` +fails rather than paging through a different sort than the token was cut for. + +It carries the filter too, given with `with_filter` — everything passed to +`push_where`, in the same order. AIP-158 holds the filter fixed between pages, +and a cursor is only a position within one filtered set, so `compose()` checks +the next page's `push_where` clauses against it and fails with +`Error::CursorMismatch` if the client changed, added or dropped a filter, +rather than returning pages that match neither. ## Binding diff --git a/crates/sqlx-query/src/clauses/pager.rs b/crates/sqlx-query/src/clauses/pager.rs index b18de62..13f48c0 100644 --- a/crates/sqlx-query/src/clauses/pager.rs +++ b/crates/sqlx-query/src/clauses/pager.rs @@ -14,22 +14,16 @@ //! token's checksum will mismatch and be cleanly rejected rather than //! misread. //! -//! The token also carries a digest of the request it was issued for — the -//! caller's own list of fields, typically the raw `filter` and `order_by` -//! strings and any scope such as the parent. AIP-158 requires every field -//! but `page_token` and `page_size` to stay the same between pages, as -//! `einride/aip-go`'s `CalculateRequestChecksum` does; without this, a -//! client could change its `filter` and keep its token, and be paged on from -//! that position under a predicate the token was never cut for. -//! [`Cursor::parse`] recomputes the digest from the request in hand and -//! rejects a token issued for a different one. -//! -//! Neither checksum stands in for +//! That checksum only guards against a corrupted or hand-edited token; it +//! is not a substitute for //! [`QueryComposer::compose`](crate::QueryComposer::compose)'s -//! [`CursorMismatch`](crate::Error::CursorMismatch) check, which compares -//! the actual decoded `OrderByClause` against an explicit one — a stronger -//! guarantee than a hash could give, for the one field the cursor itself -//! depends on. +//! [`CursorMismatch`](crate::Error::CursorMismatch) check, which is what +//! catches a client paging with one `order_by` or filter and then switching +//! to another — that check compares the actual decoded `OrderByClause` and +//! [`WhereClause`], a stronger guarantee than a hash could give. AIP-158 +//! requires both to stay the same between pages: a cursor positions within +//! one ordering of one filtered set, and carried into another it would +//! return pages that match neither. //! //! [`Cursor::after_row`] fills values straight from a `sqlx::Row` (the //! last row of a page, once it's been fetched) instead of requiring the @@ -57,15 +51,9 @@ use crate::{OrderByClause, OrderDirection, Value, WhereClause}; /// payload, mirroring `einride/aip-go`'s `PageToken { Offset, checksum /// RequestChecksum }`. Private: callers only ever see the base64 string /// [`Cursor::encode`] returns, never this type. -/// -/// `request` is the digest of the request the token was issued for, kept -/// apart from `checksum` so [`Cursor::parse`] can tell a token issued for -/// another request from a corrupted one. `checksum` covers it too, so -/// editing it is corruption like any other. #[derive(Serialize, Deserialize)] struct CursorToken { checksum: u32, - request: u32, cursor: Cursor, } @@ -93,6 +81,8 @@ struct CursorKey { #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Cursor { keys: Vec, + #[serde(with = "filter_wire")] + filter: WhereClause, } /// Errors [`Cursor`]'s methods can return: building one @@ -112,9 +102,6 @@ pub enum CursorError { #[error("cursor token checksum mismatch — the token was corrupted or tampered with")] TokenChecksumMismatch, - #[error("cursor token was issued for a different request — only page_token and page_size may change between pages")] - RequestMismatch, - #[error("row has no column named `{0}` for an order_by key")] RowColumnMissing(String), @@ -135,7 +122,37 @@ impl Cursor { .cloned() .map(|order| CursorKey { order, value: None }) .collect(); - Cursor { keys } + Cursor { + keys, + filter: WhereClause::default(), + } + } + + /// The filter the query this cursor pages through is filtered by — + /// everything passed to + /// [`QueryComposer::push_where`](crate::QueryComposer::push_where), in + /// the same order. Accumulates the same way: each call ANDs `filter` + /// onto what's already there, so a tenant scope and then a client + /// filter come out exactly as the composer combines them. + /// + /// The token carries it, and + /// [`QueryComposer::compose`](crate::QueryComposer::compose) checks it + /// against the next page's own `push_where` clauses, so a client that + /// changes its filter between pages gets + /// [`Error::CursorMismatch`](crate::Error::CursorMismatch) rather than a + /// page from a set the cursor was never positioned in. + #[must_use] + pub fn with_filter(mut self, filter: impl Into) -> Self { + self.filter = self.filter.and(filter.into()); + self + } + + /// The filter this cursor was issued under — see + /// [`with_filter`](Self::with_filter). Empty for a query with no + /// `push_where` clauses. + #[must_use] + pub fn filter(&self) -> &WhereClause { + &self.filter } /// Whether this is the empty cursor: no keys, so no position — the start @@ -146,26 +163,21 @@ impl Cursor { } /// Decodes a `page_token` string produced by [`encode`](Self::encode) - /// back into a `Cursor` carrying both its `order_by` and its boundary - /// values — no separate [`new`](Self::new)/[`after`](Self::after) call - /// needed, unlike building one from scratch. + /// back into a `Cursor` carrying its `order_by`, its + /// [filter](Self::with_filter) and its boundary values — no separate + /// [`new`](Self::new)/[`after`](Self::after) call needed, unlike building + /// one from scratch. /// /// An empty or all-whitespace token is the [empty](Self::is_empty) /// cursor, the first page, as AIP-158 reads it. /// - /// `request` is the current request's fields, in the order - /// [`encode`](Self::encode) was given them — every field but - /// `page_token` and `page_size`, e.g. `&[parent, filter, order_by]`. - /// /// # Errors /// /// [`CursorError::TokenInvalidBase64`] or /// [`CursorError::TokenMalformed`] for a token that isn't one this type - /// produced, [`CursorError::TokenChecksumMismatch`] for one that was - /// corrupted or hand-edited after it was issued, and - /// [`CursorError::RequestMismatch`] for one issued for a different - /// `request`. - pub fn parse(token: &str, request: &[&str]) -> Result { + /// produced, and [`CursorError::TokenChecksumMismatch`] for one that was + /// corrupted or hand-edited after it was issued. + pub fn parse(token: &str) -> Result { use base64::Engine as _; if token.trim().is_empty() { @@ -179,12 +191,9 @@ impl Cursor { let wire: CursorToken = postcard::from_bytes(&bytes).map_err(|_| CursorError::TokenMalformed)?; - if checksum_of(wire.request, &wire.cursor) != wire.checksum { + if checksum_of(&wire.cursor) != wire.checksum { return Err(CursorError::TokenChecksumMismatch); } - if digest_of(request) != wire.request { - return Err(CursorError::RequestMismatch); - } Ok(wire.cursor) } @@ -193,11 +202,6 @@ impl Cursor { /// back to the client as-is. See the module docs for the wire format /// and what the checksum does and doesn't guard against. /// - /// `request` is the fields of the request this page answers — every one - /// but `page_token` and `page_size`, e.g. `&[parent, filter, order_by]`. - /// [`parse`](Self::parse) only accepts the token back for the same - /// fields, in the same order. - /// /// The [empty](Self::is_empty) cursor encodes as the empty string, the /// token that says there is no next page. /// @@ -206,17 +210,15 @@ impl Cursor { /// If any key's value is unset — unreachable through this type's public /// API, same as [`to_where_clause`](Self::to_where_clause). #[must_use] - pub fn encode(&self, request: &[&str]) -> String { + pub fn encode(&self) -> String { use base64::Engine as _; if self.is_empty() { return String::new(); } - let request = digest_of(request); let token = CursorToken { - checksum: checksum_of(request, self), - request, + checksum: checksum_of(self), cursor: self.clone(), }; let bytes = postcard::to_allocvec(&token).expect("CursorToken serialization is infallible"); @@ -448,28 +450,44 @@ fn placeholder_index(keys: &[CursorKey], key: &CursorKey) -> usize { /// XORed into the checksum on both [`Cursor::encode`] and /// [`Cursor::parse`] — see the module docs for what bumping this buys. -/// Last bumped when [`CursorToken`] gained its `request` digest. +/// Last bumped when [`Cursor`] gained its filter. const CURSOR_TOKEN_CHECKSUM_MASK: u32 = 0x5eed_c0df; -/// The token's checksum, as stored in / verified against a -/// [`CursorToken`]: a CRC32 of the `postcard` encoding of its `request` -/// digest and `cursor`, masked by [`CURSOR_TOKEN_CHECKSUM_MASK`]. -/// Deterministic — `postcard`'s output for the same value is always the -/// same bytes — so [`Cursor::parse`] can recompute this straight from the -/// decoded fields rather than needing the original payload bytes kept -/// around separately. -fn checksum_of(request: u32, cursor: &Cursor) -> u32 { - let payload = - postcard::to_allocvec(&(request, cursor)).expect("Cursor serialization is infallible"); - crc32(&payload) ^ CURSOR_TOKEN_CHECKSUM_MASK +/// [`Cursor`]'s `filter` on the wire: its SQL text and bind values, the +/// two things a [`WhereClause`] is. Kept here rather than as a `serde` +/// impl on `WhereClause` itself, so the token stays the only place a +/// filter is ever serialized. +mod filter_wire { + use serde::{Deserialize, Deserializer, Serialize, Serializer}; + + use crate::{Value, WhereClause}; + + pub(super) fn serialize( + filter: &WhereClause, + serializer: S, + ) -> Result { + (filter.sql().as_str(), filter.values()).serialize(serializer) + } + + pub(super) fn deserialize<'de, D: Deserializer<'de>>( + deserializer: D, + ) -> Result { + let (sql, values) = <(String, Vec)>::deserialize(deserializer)?; + Ok(values + .into_iter() + .fold(WhereClause::new(sql), WhereClause::bind_value)) + } } -/// The digest of a request's fields, as stored in a [`CursorToken`]: a -/// CRC32 of their `postcard` encoding, which prefixes each field with its -/// length — so `["a b"]`, `["a", "b"]` and `["ab", ""]` all differ. -fn digest_of(request: &[&str]) -> u32 { - let payload = postcard::to_allocvec(request).expect("&str serialization is infallible"); - crc32(&payload) +/// `cursor`'s checksum, as stored in / verified against a +/// [`CursorToken`]: a CRC32 of `cursor`'s own `postcard` encoding, masked +/// by [`CURSOR_TOKEN_CHECKSUM_MASK`]. Deterministic — `postcard`'s output +/// for the same value is always the same bytes — so [`Cursor::parse`] can +/// recompute this straight from the decoded `cursor` field rather than +/// needing the original payload bytes kept around separately. +fn checksum_of(cursor: &Cursor) -> u32 { + let payload = postcard::to_allocvec(cursor).expect("Cursor serialization is infallible"); + crc32(&payload) ^ CURSOR_TOKEN_CHECKSUM_MASK } /// IEEE CRC32 (the same variant Go's `hash/crc32.ChecksumIEEE` computes), @@ -496,10 +514,6 @@ mod tests { use super::*; - /// A request's fields other than `page_token` and `page_size`: parent, - /// filter, `order_by`. - const REQUEST: &[&str] = &["publishers/1", "rank > 0", "rank desc"]; - #[test] fn builds_tuple_comparison_for_two_keys() { let order_by = OrderByClause::parse("rank desc, id asc").unwrap(); @@ -553,12 +567,35 @@ mod tests { .after(vec![Value::Int(42), Value::String("alice".into())]) .unwrap(); - let parsed = Cursor::parse(&cursor.encode(REQUEST), REQUEST).unwrap(); + let parsed = Cursor::parse(&cursor.encode()).unwrap(); assert_eq!(parsed.to_order_by_clause(), order_by); assert_eq!(parsed.to_where_clause(), cursor.to_where_clause()); } + /// The filter travels in the token, so the next page can be checked + /// against the one this page was cut from. + #[test] + fn encode_parse_round_trips_the_filter() { + let cursor = Cursor::new(OrderByClause::parse("rank desc").unwrap()) + .with_filter(WhereClause::new("tenant = $1").bind_value("acme")) + .with_filter(WhereClause::new("rank > $1").bind_value(10i64)) + .after(vec![Value::Int(42)]) + .unwrap(); + + let parsed = Cursor::parse(&cursor.encode()).unwrap(); + + assert_eq!(parsed.filter(), cursor.filter()); + assert_eq!( + parsed.filter().sql().as_str(), + "(tenant = $1) AND (rank > $2)" + ); + assert_eq!( + parsed.filter().values(), + &[Value::String("acme".into()), Value::Int(10)] + ); + } + #[test] fn encode_parse_round_trips_every_value_variant() { let order_by = @@ -577,7 +614,7 @@ mod tests { ]) .unwrap(); - let parsed = Cursor::parse(&cursor.encode(REQUEST), REQUEST).unwrap(); + let parsed = Cursor::parse(&cursor.encode()).unwrap(); assert_eq!( parsed.to_where_clause().values(), @@ -587,14 +624,14 @@ mod tests { #[test] fn parse_rejects_invalid_base64() { - let err = Cursor::parse("not valid base64!!", REQUEST).unwrap_err(); + let err = Cursor::parse("not valid base64!!").unwrap_err(); assert_eq!(err, CursorError::TokenInvalidBase64); } #[test] fn parse_rejects_garbage_bytes() { let token = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode([1u8, 2, 3]); - let err = Cursor::parse(&token, REQUEST).unwrap_err(); + let err = Cursor::parse(&token).unwrap_err(); assert_eq!(err, CursorError::TokenMalformed); } @@ -602,7 +639,7 @@ mod tests { fn parse_rejects_corrupted_token() { let order_by = OrderByClause::parse("rank desc").unwrap(); let cursor = Cursor::new(order_by).after(vec![Value::Int(42)]).unwrap(); - let token = cursor.encode(REQUEST); + let token = cursor.encode(); let mut wire = base64::engine::general_purpose::URL_SAFE_NO_PAD .decode(&token) @@ -611,7 +648,7 @@ mod tests { wire[last] ^= 0xFF; let tampered = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(&wire); - let err = Cursor::parse(&tampered, REQUEST).unwrap_err(); + let err = Cursor::parse(&tampered).unwrap_err(); assert!(matches!( err, CursorError::TokenChecksumMismatch | CursorError::TokenMalformed @@ -623,13 +660,10 @@ mod tests { #[test] fn the_empty_token_is_the_empty_cursor() { for token in ["", " "] { - assert!( - Cursor::parse(token, REQUEST).unwrap().is_empty(), - "{token:?}" - ); + assert!(Cursor::parse(token).unwrap().is_empty(), "{token:?}"); } assert!(Cursor::default().is_empty()); - assert_eq!(Cursor::default().encode(REQUEST), ""); + assert_eq!(Cursor::default().encode(), ""); assert!(Cursor::default().to_where_clause().is_empty()); assert!(Cursor::default().to_order_by_clause().keys().is_empty()); @@ -637,7 +671,7 @@ mod tests { .after(vec![Value::Int(42)]) .unwrap(); assert!(!positioned.is_empty()); - assert!(!positioned.encode(REQUEST).is_empty()); + assert!(!positioned.encode().is_empty()); } #[test] @@ -655,81 +689,7 @@ mod tests { let order_by = OrderByClause::parse("rank desc").unwrap(); let cursor = Cursor::new(order_by).after(vec![Value::Int(42)]).unwrap(); - let request = digest_of(REQUEST); - let payload = postcard::to_allocvec(&(request, &cursor)).unwrap(); - assert_ne!(checksum_of(request, &cursor), crc32(&payload)); - } - - fn rank_token(request: &[&str]) -> String { - Cursor::new(OrderByClause::parse("rank desc").unwrap()) - .after(vec![Value::Int(42)]) - .unwrap() - .encode(request) - } - - /// AIP-158: every field but `page_token` and `page_size` stays the same - /// between pages, so a token is only good for the request it came from. - #[test] - fn parse_rejects_a_token_issued_for_another_request() { - let token = rank_token(REQUEST); - - for request in [ - &["publishers/1", "rank > 10", "rank desc"][..], - &["publishers/2", "rank > 0", "rank desc"], - &["publishers/1", "rank > 0"], - &[], - ] { - assert_eq!( - Cursor::parse(&token, request).unwrap_err(), - CursorError::RequestMismatch, - "{request:?}" - ); - } - } - - /// The fields are length-prefixed, so moving text across a field - /// boundary is a different request, not the same bytes. - #[test] - fn the_request_digest_keeps_field_boundaries() { - let token = rank_token(&["a b", ""]); - - assert!(Cursor::parse(&token, &["a b", ""]).is_ok()); - for request in [&["a", "b"][..], &["a b"], &["", "a b"], &["a b", "", ""]] { - assert_eq!( - Cursor::parse(&token, request).unwrap_err(), - CursorError::RequestMismatch, - "{request:?}" - ); - } - } - - /// Rewriting the token's request digest to match another request is - /// corruption like any other edit: the checksum covers it. - #[test] - fn parse_rejects_a_rewritten_request_digest() { - let token = rank_token(REQUEST); - let other = &["publishers/1", "rank > 10", "rank desc"]; - - let bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD - .decode(&token) - .unwrap(); - let mut wire: CursorToken = postcard::from_bytes(&bytes).unwrap(); - wire.request = digest_of(other); - let rewritten = base64::engine::general_purpose::URL_SAFE_NO_PAD - .encode(postcard::to_allocvec(&wire).unwrap()); - - assert_eq!( - Cursor::parse(&rewritten, other).unwrap_err(), - CursorError::TokenChecksumMismatch - ); - } - - /// The first page has no position to carry, so the empty token is good - /// for any request. - #[test] - fn the_empty_token_is_good_for_any_request() { - assert!(Cursor::parse("", &["publishers/2", "rank > 10"]) - .unwrap() - .is_empty()); + let payload = postcard::to_allocvec(&cursor).unwrap(); + assert_ne!(checksum_of(&cursor), crc32(&payload)); } } diff --git a/crates/sqlx-query/src/composer.rs b/crates/sqlx-query/src/composer.rs index 18b523e..166724d 100644 --- a/crates/sqlx-query/src/composer.rs +++ b/crates/sqlx-query/src/composer.rs @@ -51,12 +51,18 @@ pub enum Error { #[error("a {name} clause was set, but the base query has no /* query.{name} */ slot")] MissingSlot { name: &'static str }, - /// `order_by` was set to something that doesn't match the - /// `OrderByClause` the [`Cursor`] passed to - /// [`QueryComposer::with_cursor`] was built against — almost always - /// means the client changed their sort between the request that - /// issued the page token and the one using it. - #[error("order_by doesn't match the order_by the cursor was built against")] + /// The query's `order_by` or filter doesn't match the one the + /// [`Cursor`] passed to [`QueryComposer::with_cursor`] was issued + /// under — almost always means the client changed their sort or their + /// filter between the request that issued the page token and the one + /// using it, which AIP-158 forbids. + /// + /// An explicit `order_by` must equal the cursor's + /// [`to_order_by_clause`](Cursor::to_order_by_clause); the + /// [`push_where`](QueryComposer::push_where) clauses, AND-ed together, + /// must equal its [`filter`](Cursor::filter) — no clauses at all only + /// matches a cursor with no filter. + #[error("order_by or filter doesn't match the one the cursor was issued under")] CursorMismatch, /// A base query for a `?`-style dialect contains a numbered `$N` @@ -171,6 +177,8 @@ impl QueryComposer { /// used directly; if it *is* set, [`compose`](Self::compose) checks the /// two match, since a mismatch almost always means the client's sort /// changed between the request that issued this cursor and this one. + /// The same goes for the filter, always: the `push_where` clauses must + /// match the cursor's [`filter`](Cursor::filter). /// Only one cursor makes sense per query, so unlike `push_where`/ /// `push_order_by` this doesn't accumulate — a second call replaces the /// first. @@ -240,8 +248,8 @@ impl QueryComposer { /// /// # Errors /// - /// [`Error::CursorMismatch`] if a cursor and an explicit `order_by` - /// are both set and disagree; [`Error::UnsupportedPlaceholder`] for a + /// [`Error::CursorMismatch`] if a cursor is set and an explicit + /// `order_by`, or the `push_where` clauses, disagree with it; [`Error::UnsupportedPlaceholder`] for a /// `$N` in a base query whose dialect spells placeholders `?`; /// [`Error::BindMismatch`] if the placeholders and the values don't /// correspond one-to-one; [`Error::MissingSlot`] for a clause with @@ -265,7 +273,7 @@ impl QueryComposer { } let order_by_sql = self.compose_order_by()?; - let (where_by_sql, where_by_values) = self.compose_where(placeholders); + let (where_by_sql, where_by_values) = self.compose_where(placeholders)?; let mut sql = String::with_capacity(self.sql.len()); let mut last = 0; @@ -413,13 +421,27 @@ impl QueryComposer { /// client-supplied filter, and pagination can all apply at once; none /// silently replaces another. Empty ones are dropped, and what's left /// is shifted past `offset`, the base query's own placeholders. - /// Unlike `order_by`, this can't fail: `where_by` values are always - /// AND-ed, never checked for equality against the cursor's. - fn compose_where(&self, offset: usize) -> (String, Vec) { - let where_by = self + /// + /// The cursor's tuple comparison is only meaningful within the filtered + /// set it was positioned in, so the `where_by` values, AND-ed together, + /// must equal the filter the cursor was issued under. Combined the same + /// way [`Cursor::with_filter`] combines them, so the same sequence of + /// clauses produces the same clause on both sides. + fn compose_where(&self, offset: usize) -> Result<(String, Vec), Error> { + let filter = self .where_by .iter() .cloned() + .fold(WhereClause::default(), WhereClause::and); + + if let Some(cursor) = &self.cursor { + if *cursor.filter() != filter { + return Err(Error::CursorMismatch); + } + } + + let where_by = Some(filter) + .into_iter() .chain(self.cursor.as_ref().map(Cursor::to_where_clause)) .reduce(WhereClause::and) .filter(|w| !w.is_empty()) @@ -434,7 +456,7 @@ impl QueryComposer { .as_ref() .map_or_else(String::new, |w| format!("({})", w.sql().as_str())); let values = where_by.map_or_else(Vec::new, |w| w.values().to_vec()); - (sql, values) + Ok((sql, values)) } /// The accumulated `order_by` if any `push_order_by()` calls were diff --git a/crates/sqlx-query/tests/composer.rs b/crates/sqlx-query/tests/composer.rs index 1cd4702..75d13af 100644 --- a/crates/sqlx-query/tests/composer.rs +++ b/crates/sqlx-query/tests/composer.rs @@ -347,6 +347,10 @@ fn build_produces_a_query_with_the_composed_sql() { assert!(built.sql().as_str().contains("(role = 'admin') AND TRUE")); } +fn rank_filter() -> WhereClause { + WhereClause::new("(rank) > ($1)").bind_value(10i64) +} + fn rank_cursor() -> Cursor { let order_by = OrderByClause::parse("rank desc, id asc").unwrap(); Cursor::new(order_by) @@ -401,15 +405,72 @@ fn cursor_with_mismatched_order_by_is_rejected() { )); } +/// A cursor is checked against every `push_where` clause, combined the way +/// `with_filter` combines them: the same clauses in the same order match. +#[test] +fn cursor_with_matching_filter_is_accepted() { + let sql = "SELECT * FROM t WHERE /* query.where AND */ TRUE"; + let tenant = WhereClause::new("tenant = $1").bind_value("acme"); + let mut query = QueryComposer::::new(sql); + query + .push_where(tenant.clone()) + .push_where(WhereClause::default()) + .push_where(rank_filter()) + .with_cursor(rank_cursor().with_filter(tenant).with_filter(rank_filter())); + + assert!(query.compose().is_ok()); +} + +/// AIP-158: the filter stays the same between pages. A cursor carried into +/// a different filter -- changed, added or dropped -- would page through a +/// set it was never positioned in, so it is rejected. +#[test] +fn cursor_with_mismatched_filter_is_rejected() { + let sql = "SELECT * FROM t WHERE /* query.where AND */ TRUE"; + let cases = [ + // The client changed the filter's value. + ( + rank_cursor().with_filter(rank_filter()), + vec![WhereClause::new("(rank) > ($1)").bind_value(20i64)], + ), + // The client added a filter. + (rank_cursor(), vec![rank_filter()]), + // The client dropped it. + (rank_cursor().with_filter(rank_filter()), vec![]), + // The caller left the tenant scope off the cursor. + ( + rank_cursor().with_filter(rank_filter()), + vec![ + WhereClause::new("tenant = $1").bind_value("acme"), + rank_filter(), + ], + ), + ]; + + for (cursor, filters) in cases { + let mut query = QueryComposer::::new(sql); + for filter in filters { + query.push_where(filter); + } + query.with_cursor(cursor); + + assert!(matches!( + query.compose().unwrap_err(), + Error::CursorMismatch + )); + } +} + /// A cursor and a plain filter both apply — pagination must not silently /// drop a filter the caller also set. #[test] fn cursor_and_filter_are_combined_with_and() { let sql = "SELECT * FROM t WHERE /* query.where AND */ TRUE"; let mut query = QueryComposer::::new(sql); + let filter = WhereClause::new("status = $1").bind_value("ACTIVE"); query - .push_where(WhereClause::new("status = $1").bind_value("ACTIVE")) - .with_cursor(rank_cursor()); + .push_where(filter.clone()) + .with_cursor(rank_cursor().with_filter(filter)); let statement = query.compose().unwrap(); let (sql, values) = (statement.sql(), values(&statement)); @@ -438,7 +499,7 @@ fn an_empty_cursor_is_no_cursor() { let mut first = QueryComposer::::new(sql); first .push_order_by(OrderByClause::parse("rank desc").unwrap()) - .with_cursor(Cursor::parse("", &[]).unwrap()); + .with_cursor(Cursor::parse("").unwrap()); let mut untouched = QueryComposer::::new(sql); untouched.push_order_by(OrderByClause::parse("rank desc").unwrap()); @@ -525,8 +586,8 @@ fn non_positional_binds_in_textual_order_not_base_then_fragments() { query .bind_value("acme") .bind_value(50i64) - .push_where(WhereClause::new("(rank) > ($1)").bind_value(10i64)) - .with_cursor(rank_cursor()); + .push_where(rank_filter()) + .with_cursor(rank_cursor().with_filter(rank_filter())); let statement = query.compose().unwrap(); let (sql, values) = (statement.sql(), values(&statement)); @@ -562,8 +623,8 @@ fn positional_keeps_numbering_and_declaration_order() { query .bind_value("acme") .bind_value(50i64) - .push_where(WhereClause::new("(rank) > ($1)").bind_value(10i64)) - .with_cursor(rank_cursor()); + .push_where(rank_filter()) + .with_cursor(rank_cursor().with_filter(rank_filter())); let statement = query.compose().unwrap(); let (sql, values) = (statement.sql(), values(&statement)); diff --git a/crates/sqlx-query/tests/execute.rs b/crates/sqlx-query/tests/execute.rs index 76b43f1..9c1bf33 100644 --- a/crates/sqlx-query/tests/execute.rs +++ b/crates/sqlx-query/tests/execute.rs @@ -155,13 +155,13 @@ async fn a_page_token_round_trips_into_a_running_query() { let token = Cursor::new(order_by) .after_row(rows.last().expect("a row")) .expect("cursor") - .encode(&["acme"]); + .encode(); let mut second = QueryComposer::::new(LIST_ORDERS); second .bind_value("acme") .bind_value(1i64) - .with_cursor(Cursor::parse(&token, &["acme"]).expect("token parses")); + .with_cursor(Cursor::parse(&token).expect("token parses")); let rows = second .build() .expect("composes") @@ -349,7 +349,7 @@ async fn pages(pool: &SqlitePool, size: usize) -> Vec> { .bind_value("acme") .bind_value(pager.limit()) .push_order_by(order_by.clone()) - .with_cursor(Cursor::parse(&token, &["acme"]).expect("the token parses")); + .with_cursor(Cursor::parse(&token).expect("the token parses")); let rows = query .build() @@ -360,7 +360,7 @@ async fn pages(pool: &SqlitePool, size: usize) -> Vec> { let page = pager.next_page(rows).expect("the row carries every key"); seen.push(page.rows.iter().map(|row| row.get("id")).collect()); - token = page.cursor.encode(&["acme"]); + token = page.cursor.encode(); if token.is_empty() { return seen; } @@ -385,3 +385,51 @@ async fn a_page_that_fills_exactly_has_no_cursor() { assert_eq!(pages(&pool, 3).await, [vec![1, 2, 3]]); } + +/// The pager's cursor carries the filter into each page's token, so the +/// next page is checked against it: the same filter pages on, and a +/// changed one is refused rather than paged through from the old position. +#[tokio::test] +async fn a_token_only_pages_on_under_its_own_filter() { + let pool = seed().await; + let shipped = || WhereClause::new("status = $1").bind_value("SHIPPED"); + let order_by = OrderByClause::default().asc("id"); + let pager = Pager::new(Cursor::new(order_by.clone()).with_filter(shipped()), 1); + + let query = |filter: WhereClause, token: &str| { + let mut query = QueryComposer::::new(LIST_ORDERS); + query + .bind_value("acme") + .bind_value(pager.limit()) + .push_where(filter) + .push_order_by(order_by.clone()) + .with_cursor(Cursor::parse(token).expect("the token parses")); + query + }; + + let rows = query(shipped(), "") + .build() + .expect("composes") + .fetch_all(&pool) + .await + .expect("runs"); + let page = pager.next_page(rows).expect("the row carries every key"); + let ids: Vec = page.rows.iter().map(|row| row.get("id")).collect(); + assert_eq!(ids, [1]); + let token = page.cursor.encode(); + + let rows = query(shipped(), &token) + .build() + .expect("composes") + .fetch_all(&pool) + .await + .expect("runs"); + let ids: Vec = rows.iter().map(|row| row.get("id")).collect(); + assert_eq!(ids, [3], "the next shipped order, skipping the pending one"); + + let pending = WhereClause::new("status = $1").bind_value("PENDING"); + assert!(matches!( + query(pending, &token).compose().unwrap_err(), + sqlx_query::Error::CursorMismatch + )); +} diff --git a/crates/sqlx-query/tests/execute_postgres.rs b/crates/sqlx-query/tests/execute_postgres.rs index ee4e92c..ec10399 100644 --- a/crates/sqlx-query/tests/execute_postgres.rs +++ b/crates/sqlx-query/tests/execute_postgres.rs @@ -303,12 +303,12 @@ async fn a_cursor_pages_on_a_uuid_key() { let token = Cursor::new(order_by) .after_row(&rows[0]) .expect("a uuid key decodes") - .encode(&[]); + .encode(); let mut second = QueryComposer::::new(sql); second .bind_value(10i64) - .with_cursor(Cursor::parse(&token, &[]).expect("the token parses")); + .with_cursor(Cursor::parse(&token).expect("the token parses")); let rows = second .build() .expect("composes") From 5c7e91d9aaa5ea2fa96182a2ac577bbb8f8bf5d6 Mon Sep 17 00:00:00 2001 From: Svetlin Ralchev Date: Tue, 6 Oct 2026 17:14:51 +0400 Subject: [PATCH 3/3] refactor!: carry a checksum of the filter in the token, not the filter The cursor never needs the filter itself, only to tell whether the next page's is the same, so a checksum is enough: tokens no longer grow with the filter, and its SQL no longer travels to the client. A checksum can't be ANDed onto, so `with_filter` chains one CRC32 per clause, and `compose()` folds the same chain over its `push_where` clauses -- the same clauses in the same order match, empty ones counting for nothing. `Cursor::filter()` goes, since there is no clause to return. The order_by keys stay in the token as they were: the cursor needs them to build its comparison and to supply the ORDER BY, and `compose()` already compares them directly. --- README.md | 15 +-- crates/sqlx-query/src/clauses/pager.rs | 155 +++++++++++++++---------- crates/sqlx-query/src/composer.rs | 29 ++--- 3 files changed, 111 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index c633503..20dd7f5 100644 --- a/README.md +++ b/README.md @@ -239,8 +239,8 @@ came back: ```rust // The request's token; the empty one -- the first page -- is the empty cursor. let cursor = Cursor::parse(&page_token)?; -// The next page's cursor carries the filter, so that page can be checked -// against it. +// The next page's cursor carries a checksum of the filter, so that page can be +// checked against it. let pager = Pager::new(Cursor::new(order_by.clone()).with_filter(filter.clone()), 50); let mut query = QueryComposer::::new(LIST_USERS); @@ -279,12 +279,13 @@ rejected if hand-edited. A cursor carries the `order_by` it was built against, so it doesn't have to be repeated; if it *is* set and disagrees, `compose()` fails rather than paging through a different sort than the token was cut for. -It carries the filter too, given with `with_filter` — everything passed to -`push_where`, in the same order. AIP-158 holds the filter fixed between pages, -and a cursor is only a position within one filtered set, so `compose()` checks -the next page's `push_where` clauses against it and fails with +It carries a checksum of the filter too, given with `with_filter` — the same +clauses passed to `push_where`, in the same order. AIP-158 holds the filter +fixed between pages, and a cursor is only a position within one filtered set, +so `compose()` checksums the next page's `push_where` clauses and fails with `Error::CursorMismatch` if the client changed, added or dropped a filter, -rather than returning pages that match neither. +rather than returning pages that match neither. Only the checksum travels: the +filter's SQL never ends up in the token. ## Binding diff --git a/crates/sqlx-query/src/clauses/pager.rs b/crates/sqlx-query/src/clauses/pager.rs index 13f48c0..d3d5145 100644 --- a/crates/sqlx-query/src/clauses/pager.rs +++ b/crates/sqlx-query/src/clauses/pager.rs @@ -19,11 +19,14 @@ //! [`QueryComposer::compose`](crate::QueryComposer::compose)'s //! [`CursorMismatch`](crate::Error::CursorMismatch) check, which is what //! catches a client paging with one `order_by` or filter and then switching -//! to another — that check compares the actual decoded `OrderByClause` and -//! [`WhereClause`], a stronger guarantee than a hash could give. AIP-158 -//! requires both to stay the same between pages: a cursor positions within -//! one ordering of one filtered set, and carried into another it would -//! return pages that match neither. +//! to another. AIP-158 requires both to stay the same between pages: a +//! cursor positions within one ordering of one filtered set, and carried +//! into another it would return pages that match neither. The `order_by` is +//! compared as the actual decoded `OrderByClause` — the cursor needs its +//! keys anyway, to build its comparison. The filter it never needs, so the +//! token carries only a checksum of it (see [`Cursor::with_filter`]): enough +//! to tell a changed filter from the same one, without the filter's SQL +//! growing every token or travelling to the client. //! //! [`Cursor::after_row`] fills values straight from a `sqlx::Row` (the //! last row of a page, once it's been fetched) instead of requiring the @@ -81,8 +84,7 @@ struct CursorKey { #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Cursor { keys: Vec, - #[serde(with = "filter_wire")] - filter: WhereClause, + filter: u32, } /// Errors [`Cursor`]'s methods can return: building one @@ -124,35 +126,33 @@ impl Cursor { .collect(); Cursor { keys, - filter: WhereClause::default(), + filter: FILTER_CHECKSUM_SEED, } } /// The filter the query this cursor pages through is filtered by — /// everything passed to - /// [`QueryComposer::push_where`](crate::QueryComposer::push_where), in - /// the same order. Accumulates the same way: each call ANDs `filter` - /// onto what's already there, so a tenant scope and then a client - /// filter come out exactly as the composer combines them. + /// [`QueryComposer::push_where`](crate::QueryComposer::push_where), one + /// call per `push_where` call, in the same order. An empty clause counts + /// for nothing, as it does there. /// - /// The token carries it, and - /// [`QueryComposer::compose`](crate::QueryComposer::compose) checks it - /// against the next page's own `push_where` clauses, so a client that - /// changes its filter between pages gets + /// Only a checksum of the filter is kept, and the token carries it; + /// [`QueryComposer::compose`](crate::QueryComposer::compose) computes + /// the same checksum over the next page's own `push_where` clauses, so a + /// client that changes its filter between pages gets /// [`Error::CursorMismatch`](crate::Error::CursorMismatch) rather than a /// page from a set the cursor was never positioned in. #[must_use] pub fn with_filter(mut self, filter: impl Into) -> Self { - self.filter = self.filter.and(filter.into()); + self.filter = filter_checksum(self.filter, &filter.into()); self } - /// The filter this cursor was issued under — see - /// [`with_filter`](Self::with_filter). Empty for a query with no - /// `push_where` clauses. - #[must_use] - pub fn filter(&self) -> &WhereClause { - &self.filter + /// Whether `where_by` — a query's `push_where` clauses, in order — is + /// the filter this cursor was issued under. See + /// [`with_filter`](Self::with_filter). + pub(crate) fn matches_filter(&self, where_by: &[WhereClause]) -> bool { + where_by.iter().fold(FILTER_CHECKSUM_SEED, filter_checksum) == self.filter } /// Whether this is the empty cursor: no keys, so no position — the start @@ -163,7 +163,7 @@ impl Cursor { } /// Decodes a `page_token` string produced by [`encode`](Self::encode) - /// back into a `Cursor` carrying its `order_by`, its + /// back into a `Cursor` carrying its `order_by`, the checksum of its /// [filter](Self::with_filter) and its boundary values — no separate /// [`new`](Self::new)/[`after`](Self::after) call needed, unlike building /// one from scratch. @@ -453,30 +453,26 @@ fn placeholder_index(keys: &[CursorKey], key: &CursorKey) -> usize { /// Last bumped when [`Cursor`] gained its filter. const CURSOR_TOKEN_CHECKSUM_MASK: u32 = 0x5eed_c0df; -/// [`Cursor`]'s `filter` on the wire: its SQL text and bind values, the -/// two things a [`WhereClause`] is. Kept here rather than as a `serde` -/// impl on `WhereClause` itself, so the token stays the only place a -/// filter is ever serialized. -mod filter_wire { - use serde::{Deserialize, Deserializer, Serialize, Serializer}; - - use crate::{Value, WhereClause}; - - pub(super) fn serialize( - filter: &WhereClause, - serializer: S, - ) -> Result { - (filter.sql().as_str(), filter.values()).serialize(serializer) - } - - pub(super) fn deserialize<'de, D: Deserializer<'de>>( - deserializer: D, - ) -> Result { - let (sql, values) = <(String, Vec)>::deserialize(deserializer)?; - Ok(values - .into_iter() - .fold(WhereClause::new(sql), WhereClause::bind_value)) +/// The checksum of no filter at all: a cursor's before any +/// [`Cursor::with_filter`] call, and a query's with no `push_where` clauses. +/// Zero, so it is also what [`Cursor`]'s derived `Default` starts from. +const FILTER_CHECKSUM_SEED: u32 = 0; + +/// `checksum` with one more filter clause folded in: a CRC32 of the +/// `postcard` encoding of the checksum so far and `clause`'s SQL text and +/// bind values. Chained rather than computed over the clauses `AND`ed +/// together, because a checksum can't be `AND`ed onto — so the same +/// clauses in the same order give the same checksum, on the cursor and in +/// the composer alike. `postcard` length-prefixes the text, so no two +/// clauses run together. An [empty](WhereClause::is_empty) clause leaves +/// it as it was, as `push_where` drops one. +fn filter_checksum(checksum: u32, clause: &WhereClause) -> u32 { + if clause.is_empty() { + return checksum; } + let payload = postcard::to_allocvec(&(checksum, clause.sql().as_str(), clause.values())) + .expect("WhereClause serialization is infallible"); + crc32(&payload) } /// `cursor`'s checksum, as stored in / verified against a @@ -573,27 +569,58 @@ mod tests { assert_eq!(parsed.to_where_clause(), cursor.to_where_clause()); } - /// The filter travels in the token, so the next page can be checked - /// against the one this page was cut from. - #[test] - fn encode_parse_round_trips_the_filter() { - let cursor = Cursor::new(OrderByClause::parse("rank desc").unwrap()) - .with_filter(WhereClause::new("tenant = $1").bind_value("acme")) - .with_filter(WhereClause::new("rank > $1").bind_value(10i64)) + fn tenant() -> WhereClause { + WhereClause::new("tenant = $1").bind_value("acme") + } + + fn above(rank: i64) -> WhereClause { + WhereClause::new("rank > $1").bind_value(rank) + } + + fn filtered(filters: Vec) -> Cursor { + filters + .into_iter() + .fold( + Cursor::new(OrderByClause::parse("rank desc").unwrap()), + Cursor::with_filter, + ) .after(vec![Value::Int(42)]) - .unwrap(); + .unwrap() + } + + /// The filter's checksum travels in the token, so the next page can be + /// checked against the filter this page was cut from. + #[test] + fn encode_parse_round_trips_the_filter_checksum() { + let cursor = filtered(vec![tenant(), above(10)]); let parsed = Cursor::parse(&cursor.encode()).unwrap(); - assert_eq!(parsed.filter(), cursor.filter()); - assert_eq!( - parsed.filter().sql().as_str(), - "(tenant = $1) AND (rank > $2)" - ); - assert_eq!( - parsed.filter().values(), - &[Value::String("acme".into()), Value::Int(10)] - ); + assert_eq!(parsed.filter, cursor.filter); + assert!(parsed.matches_filter(&[tenant(), above(10)])); + } + + /// The same clauses in the same order match, empty ones counting for + /// nothing; anything else is a different filter. + #[test] + fn a_cursor_matches_only_its_own_filter() { + let cursor = filtered(vec![tenant(), above(10)]); + + assert!(cursor.matches_filter(&[tenant(), WhereClause::default(), above(10)])); + for where_by in [ + vec![tenant(), above(20)], + vec![above(10), tenant()], + vec![tenant()], + vec![tenant(), above(10), above(10)], + vec![tenant().and(above(10))], + vec![], + ] { + assert!(!cursor.matches_filter(&where_by), "{where_by:?}"); + } + + assert!(Cursor::default().matches_filter(&[])); + assert!(Cursor::default().matches_filter(&[WhereClause::default()])); + assert!(!Cursor::default().matches_filter(&[tenant()])); } #[test] diff --git a/crates/sqlx-query/src/composer.rs b/crates/sqlx-query/src/composer.rs index 166724d..7cdebb8 100644 --- a/crates/sqlx-query/src/composer.rs +++ b/crates/sqlx-query/src/composer.rs @@ -59,9 +59,9 @@ pub enum Error { /// /// An explicit `order_by` must equal the cursor's /// [`to_order_by_clause`](Cursor::to_order_by_clause); the - /// [`push_where`](QueryComposer::push_where) clauses, AND-ed together, - /// must equal its [`filter`](Cursor::filter) — no clauses at all only - /// matches a cursor with no filter. + /// [`push_where`](QueryComposer::push_where) clauses must be the ones + /// given to its [`with_filter`](Cursor::with_filter), in the same order + /// — no clauses at all only matches a cursor with no filter. #[error("order_by or filter doesn't match the one the cursor was issued under")] CursorMismatch, @@ -178,7 +178,8 @@ impl QueryComposer { /// two match, since a mismatch almost always means the client's sort /// changed between the request that issued this cursor and this one. /// The same goes for the filter, always: the `push_where` clauses must - /// match the cursor's [`filter`](Cursor::filter). + /// be the ones the cursor was given with + /// [`with_filter`](Cursor::with_filter). /// Only one cursor makes sense per query, so unlike `push_where`/ /// `push_order_by` this doesn't accumulate — a second call replaces the /// first. @@ -423,25 +424,19 @@ impl QueryComposer { /// is shifted past `offset`, the base query's own placeholders. /// /// The cursor's tuple comparison is only meaningful within the filtered - /// set it was positioned in, so the `where_by` values, AND-ed together, - /// must equal the filter the cursor was issued under. Combined the same - /// way [`Cursor::with_filter`] combines them, so the same sequence of - /// clauses produces the same clause on both sides. + /// set it was positioned in, so the `where_by` values must be the filter + /// the cursor was issued under — see [`Cursor::with_filter`]. fn compose_where(&self, offset: usize) -> Result<(String, Vec), Error> { - let filter = self - .where_by - .iter() - .cloned() - .fold(WhereClause::default(), WhereClause::and); - if let Some(cursor) = &self.cursor { - if *cursor.filter() != filter { + if !cursor.matches_filter(&self.where_by) { return Err(Error::CursorMismatch); } } - let where_by = Some(filter) - .into_iter() + let where_by = self + .where_by + .iter() + .cloned() .chain(self.cursor.as_ref().map(Cursor::to_where_clause)) .reduce(WhereClause::and) .filter(|w| !w.is_empty())