diff --git a/README.md b/README.md index 10c4f65..20dd7f5 100644 --- a/README.md +++ b/README.md @@ -239,12 +239,15 @@ came back: ```rust // The request's token; the empty one -- the first page -- is the empty cursor. let cursor = Cursor::parse(&page_token)?; -let pager = Pager::new(Cursor::new(order_by.clone()), 50); +// 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); 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 @@ -276,6 +279,14 @@ 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 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. Only the checksum travels: the +filter's SQL never ends up in the token. + ## Binding Two ways to supply a value for one of the base query's own placeholders. diff --git a/crates/sqlx-query/src/clauses/pager.rs b/crates/sqlx-query/src/clauses/pager.rs index 24fde3a..d3d5145 100644 --- a/crates/sqlx-query/src/clauses/pager.rs +++ b/crates/sqlx-query/src/clauses/pager.rs @@ -17,10 +17,16 @@ //! That checksum only guards against a corrupted or hand-edited token; it //! is not a substitute 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 is what +//! catches a client paging with one `order_by` or filter and then switching +//! 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 @@ -78,6 +84,7 @@ struct CursorKey { #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Cursor { keys: Vec, + filter: u32, } /// Errors [`Cursor`]'s methods can return: building one @@ -117,7 +124,35 @@ impl Cursor { .cloned() .map(|order| CursorKey { order, value: None }) .collect(); - Cursor { keys } + Cursor { + keys, + 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), one + /// call per `push_where` call, in the same order. An empty clause counts + /// for nothing, as it does there. + /// + /// 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 = filter_checksum(self.filter, &filter.into()); + self + } + + /// 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 @@ -128,9 +163,10 @@ 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`, 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. /// /// An empty or all-whitespace token is the [empty](Self::is_empty) /// cursor, the first page, as AIP-158 reads it. @@ -414,7 +450,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; +/// Last bumped when [`Cursor`] gained its filter. +const CURSOR_TOKEN_CHECKSUM_MASK: u32 = 0x5eed_c0df; + +/// 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 /// [`CursorToken`]: a CRC32 of `cursor`'s own `postcard` encoding, masked @@ -510,6 +569,60 @@ mod tests { assert_eq!(parsed.to_where_clause(), cursor.to_where_clause()); } + 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() + } + + /// 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!(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] fn encode_parse_round_trips_every_value_variant() { let order_by = diff --git a/crates/sqlx-query/src/composer.rs b/crates/sqlx-query/src/composer.rs index 18b523e..7cdebb8 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 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, /// A base query for a `?`-style dialect contains a numbered `$N` @@ -171,6 +177,9 @@ 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 + /// 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. @@ -240,8 +249,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 +274,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,9 +422,17 @@ 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) { + /// + /// The cursor's tuple comparison is only meaningful within the filtered + /// 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> { + if let Some(cursor) = &self.cursor { + if !cursor.matches_filter(&self.where_by) { + return Err(Error::CursorMismatch); + } + } + let where_by = self .where_by .iter() @@ -434,7 +451,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 fb76af8..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)); @@ -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 2791634..9c1bf33 100644 --- a/crates/sqlx-query/tests/execute.rs +++ b/crates/sqlx-query/tests/execute.rs @@ -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 + )); +}