From f697740feab9c666aa2b27d637694da837ca21b4 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 3 Mar 2025 18:24:21 +0800 Subject: [PATCH 01/13] controllers/helpers/pagination: Rename `Seek::after()` to `Seek::decode()` --- src/controllers/helpers/pagination.rs | 6 +++--- src/controllers/krate/search.rs | 2 +- src/controllers/krate/versions.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 5ce90c882e8..1d6bd42ad6a 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -522,7 +522,7 @@ macro_rules! seek { use crate::util::errors::AppResult; use crate::controllers::helpers::pagination::Page; impl $name { - pub fn after(&self, page: &Page) -> AppResult]>> { + pub fn decode(&self, page: &Page) -> AppResult]>> { let Page::Seek(ref encoded) = *page else { return Ok(None); }; @@ -670,7 +670,7 @@ mod tests { .enable_seek(true) .gather(&mock(query)) .unwrap(); - let decoded = seek.after(&pagination.page).unwrap(); + let decoded = seek.decode(&pagination.page).unwrap(); assert_eq!(decoded, expect); }; @@ -706,7 +706,7 @@ mod tests { .enable_seek(true) .gather(&mock(&query)) .unwrap(); - let error = seek.after(&pagination.page).unwrap_err(); + let error = seek.decode(&pagination.page).unwrap_err(); assert_eq!(error.to_string(), "invalid seek parameter"); let response = error.response(); assert_eq!(response.status(), StatusCode::BAD_REQUEST); diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 621c005a4af..9d43083519e 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -196,7 +196,7 @@ pub async fn list_crates( let (total, next_page, prev_page, data) = if !explicit_page && seek.is_some() { let seek = seek.unwrap(); if let Some(condition) = seek - .after(&pagination.page)? + .decode(&pagination.page)? .map(|s| filter_params.seek_after(&s)) { query = query.filter(condition); diff --git a/src/controllers/krate/versions.rs b/src/controllers/krate/versions.rs index 5b665e5d71c..b5fac917130 100644 --- a/src/controllers/krate/versions.rs +++ b/src/controllers/krate/versions.rs @@ -159,7 +159,7 @@ async fn list_by_date( !matches!(&options.page, Page::Numeric(_)), "?page= is not supported" ); - if let Some(SeekPayload::Date(Date { created_at, id })) = Seek::Date.after(&options.page)? { + if let Some(SeekPayload::Date(Date { created_at, id })) = Seek::Date.decode(&options.page)? { query = query.filter( versions::created_at .eq(created_at) @@ -297,7 +297,7 @@ async fn list_by_semver( }); let mut idx = Some(0); - if let Some(SeekPayload::Semver(Semver { id })) = Seek::Semver.after(&options.page)? { + if let Some(SeekPayload::Semver(Semver { id })) = Seek::Semver.decode(&options.page)? { idx = sorted_versions .get_index_of(&id) .filter(|i| i + 1 < sorted_versions.len()) From d1dc40bb84e7930ef05b22495a246e92d11ef49c Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 5 Mar 2025 17:00:07 +0800 Subject: [PATCH 02/13] controllers/helpers/pagination: Add an option to enable seeking backward --- src/controllers/helpers/pagination.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 1d6bd42ad6a..2ca4d5423a9 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -43,6 +43,7 @@ impl PaginationOptions { PaginationOptionsBuilder { limit_page_numbers: false, enable_seek: false, + enable_seek_backward: false, enable_pages: true, } } @@ -85,6 +86,7 @@ pub(crate) struct PaginationOptionsBuilder { limit_page_numbers: bool, enable_pages: bool, enable_seek: bool, + enable_seek_backward: bool, } impl PaginationOptionsBuilder { @@ -103,6 +105,12 @@ impl PaginationOptionsBuilder { self } + #[allow(dead_code)] + pub(crate) fn enable_seek_backward(mut self, enable: bool) -> Self { + self.enable_seek_backward = enable; + self + } + pub(crate) fn gather(self, parts: &Parts) -> AppResult { use axum::extract::Query; @@ -143,11 +151,19 @@ impl PaginationOptionsBuilder { Page::Numeric(numeric_page) } else if let Some(s) = params.seek { - if !self.enable_seek { - return Err(bad_request("?seek= is not supported for this request")); + match s.starts_with('-') { + true if !self.enable_seek_backward => { + return Err(bad_request( + "seek backward ?seek=- is not supported for this request", + )); + } + // TODO: add a varaint for seek backward + true => unimplemented!("seek backward is not yet implemented"), + false if !self.enable_seek => { + return Err(bad_request("?seek= is not supported for this request")); + } + false => Page::Seek(RawSeekPayload(s)), } - - Page::Seek(RawSeekPayload(s)) } else { Page::Unspecified }; From 77eb338b22b727d2192201a222345534ae92da94 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 5 Mar 2025 17:41:23 +0800 Subject: [PATCH 03/13] controllers/helpers/pagination: Add `Page::SeekBackward()` variant --- src/controllers/crate_owner_invitation.rs | 1 + src/controllers/helpers/pagination.rs | 117 ++++++++++++++++------ src/controllers/krate/versions.rs | 2 +- 3 files changed, 87 insertions(+), 33 deletions(-) diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 24547c0e650..e2d33b057d0 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -212,6 +212,7 @@ async fn prepare_list( .load(conn) .await? } + Page::SeekBackward(_) => unreachable!("seek-backward is disabled"), Page::Numeric(_) => unreachable!("page-based pagination is disabled"), }; let next_page = if raw_invitations.len() > pagination.per_page as usize { diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 2ca4d5423a9..77d35871bdc 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -29,6 +29,7 @@ const MAX_PER_PAGE: i64 = 100; pub(crate) enum Page { Numeric(u32), Seek(RawSeekPayload), + SeekBackward(RawSeekPayload), Unspecified, } @@ -157,8 +158,7 @@ impl PaginationOptionsBuilder { "seek backward ?seek=- is not supported for this request", )); } - // TODO: add a varaint for seek backward - true => unimplemented!("seek backward is not yet implemented"), + true => Page::SeekBackward(RawSeekPayload(s.trim_start_matches('-').into())), false if !self.enable_seek => { return Err(bad_request("?seek= is not supported for this request")); } @@ -225,7 +225,7 @@ impl Paginated { match self.options.page { Page::Numeric(n) => opts.insert("page".into(), (n + 1).to_string()), Page::Unspecified => opts.insert("page".into(), 2.to_string()), - Page::Seek(_) => return None, + Page::Seek(_) | Page::SeekBackward(_) => return None, }; Some(opts) } @@ -233,7 +233,9 @@ impl Paginated { pub(crate) fn prev_page_params(&self) -> Option> { let mut opts = IndexMap::new(); match self.options.page { - Page::Numeric(1) | Page::Unspecified | Page::Seek(_) => return None, + Page::Numeric(1) | Page::Unspecified | Page::Seek(_) | Page::SeekBackward(_) => { + return None; + } Page::Numeric(n) => opts.insert("page".into(), (n - 1).to_string()), }; Some(opts) @@ -244,6 +246,7 @@ impl Paginated { F: Fn(&T) -> S, S: Serialize, { + // TODO: handle this more properly when seek backward comes into play. if self.is_explicit_page() || self.records_and_total.len() < self.options.per_page as usize { return Ok(None); @@ -251,7 +254,7 @@ impl Paginated { let mut opts = IndexMap::new(); match self.options.page { - Page::Unspecified | Page::Seek(_) => { + Page::Unspecified | Page::Seek(_) | Page::SeekBackward(_) => { let seek = f(&self.records_and_total.last().unwrap().record); opts.insert("seek".into(), encode_seek(seek)?); } @@ -302,6 +305,8 @@ impl PaginatedQuery { async move { let records_and_total = future.await?.try_collect().await?; + // TODO: maintain consistent ordering from page to pagen + Ok(Paginated { records_and_total, options, @@ -450,6 +455,8 @@ impl PaginatedQueryWithCountSubq { async move { let records_and_total = future.await?.try_collect().await?; + // TODO: maintain consistent ordering from page to pagen + Ok(Paginated { records_and_total, options, @@ -539,8 +546,10 @@ macro_rules! seek { use crate::controllers::helpers::pagination::Page; impl $name { pub fn decode(&self, page: &Page) -> AppResult]>> { - let Page::Seek(ref encoded) = *page else { - return Ok(None); + let encoded = match page { + Page::Seek(encoded) => encoded, + Page::SeekBackward(encoded) => encoded, + _ => return Ok(None), }; Ok(Some(match self { @@ -619,6 +628,24 @@ mod tests { pagination.page ); } + + // for backward + let error = pagination_error(PaginationOptions::builder(), "seek=-OTg"); + assert_snapshot!(error, @"seek backward ?seek=- is not supported for this request"); + + let pagination = PaginationOptions::builder() + .enable_seek_backward(true) + .gather(&mock("seek=-OTg")) + .unwrap(); + + if let Page::SeekBackward(raw) = pagination.page { + assert_ok_eq!(raw.decode::(), 98); + } else { + panic!( + "did not parse a seek page, parsed {:?} instead", + pagination.page + ); + } } #[test] @@ -631,6 +658,13 @@ mod tests { "page=1&seek=OTg", ); assert_snapshot!(error, @"providing both ?page= and ?seek= is unsupported"); + + // for backward + let error = pagination_error( + PaginationOptions::builder().enable_seek_backward(true), + "page=1&seek=-OTg", + ); + assert_snapshot!(error, @"providing both ?page= and ?seek= is unsupported"); } #[test] @@ -681,20 +715,27 @@ mod tests { use chrono::serde::ts_microseconds; use seek::*; - let assert_decode_after = |seek: Seek, query: &str, expect| { - let pagination = PaginationOptions::builder() - .enable_seek(true) - .gather(&mock(query)) - .unwrap(); - let decoded = seek.decode(&pagination.page).unwrap(); - assert_eq!(decoded, expect); + let assert_decode = |seek: Seek, payload: Option<_>| { + for (param, prefix) in [("seek", ""), ("seek", "-")] { + let query = if let Some(ref s) = payload { + &format!("{param}={prefix}{}", encode_seek(s).unwrap()) + } else { + "" + }; + let pagination = PaginationOptions::builder() + .enable_seek(true) + .enable_seek_backward(true) + .gather(&mock(query)) + .unwrap(); + let decoded = seek.decode(&pagination.page).unwrap(); + assert_eq!(decoded.as_ref(), payload.as_ref()); + } }; let id = 1234; let seek = Seek::Id; let payload = SeekPayload::Id(Id { id }); - let query = format!("seek={}", encode_seek(&payload).unwrap()); - assert_decode_after(seek, &query, Some(payload)); + assert_decode(seek, Some(payload)); let dt = NaiveDate::from_ymd_opt(2016, 7, 8) .unwrap() @@ -703,29 +744,41 @@ mod tests { .and_utc(); let seek = Seek::New; let payload = SeekPayload::New(New { dt, id }); - let query = format!("seek={}", encode_seek(&payload).unwrap()); - assert_decode_after(seek, &query, Some(payload)); + assert_decode(seek, Some(payload)); let downloads = Some(5678); let seek = Seek::RecentDownloads; let payload = SeekPayload::RecentDownloads(RecentDownloads { downloads, id }); - let query = format!("seek={}", encode_seek(&payload).unwrap()); - assert_decode_after(seek, &query, Some(payload)); + assert_decode(seek, Some(payload)); let seek = Seek::Id; - assert_decode_after(seek, "", None); + assert_decode(seek, None); - let seek = Seek::Id; - let payload = SeekPayload::RecentDownloads(RecentDownloads { downloads, id }); - let query = format!("seek={}", encode_seek(payload).unwrap()); - let pagination = PaginationOptions::builder() - .enable_seek(true) - .gather(&mock(&query)) - .unwrap(); - let error = seek.decode(&pagination.page).unwrap_err(); - assert_eq!(error.to_string(), "invalid seek parameter"); - let response = error.response(); - assert_eq!(response.status(), StatusCode::BAD_REQUEST); + // invalid seek payload + { + let seek = Seek::Id; + let payload = SeekPayload::RecentDownloads(RecentDownloads { downloads, id }); + let query = format!("seek={}", encode_seek(&payload).unwrap()); + let pagination = PaginationOptions::builder() + .enable_seek(true) + .gather(&mock(&query)) + .unwrap(); + let error = seek.decode(&pagination.page).unwrap_err(); + assert_eq!(error.to_string(), "invalid seek parameter"); + let response = error.response(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + + // for backward + let query = format!("seek=-{}", encode_seek(&payload).unwrap()); + let pagination = PaginationOptions::builder() + .enable_seek_backward(true) + .gather(&mock(&query)) + .unwrap(); + let error = seek.decode(&pagination.page).unwrap_err(); + assert_eq!(error.to_string(), "invalid seek parameter"); + let response = error.response(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + } // Ensures it still encodes compactly with a field struct #[derive(Debug, Default, Serialize, PartialEq)] diff --git a/src/controllers/krate/versions.rs b/src/controllers/krate/versions.rs index b5fac917130..24d9b50dff2 100644 --- a/src/controllers/krate/versions.rs +++ b/src/controllers/krate/versions.rs @@ -432,7 +432,7 @@ where let seek = f(records.last().unwrap()); opts.insert("seek".into(), encode_seek(seek)?); } - Page::Numeric(_) => unreachable!(), + Page::Numeric(_) | Page::SeekBackward(_) => unreachable!(), }; Ok(Some(opts)) } From 84f9152561d3929b7708c7071b795bba7bdcb7e1 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Sun, 9 Mar 2025 17:50:15 +0800 Subject: [PATCH 04/13] controllers/helpers/pagination: Consider empty `RawSeekPayload` as valid This makes "seek=" and "seek=-" valid, and "seek=-" could be treated as paginating backward from the tail. --- src/controllers/helpers/pagination.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 77d35871bdc..ffb36b2ce52 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -348,6 +348,10 @@ impl RawSeekPayload { pub(crate) fn decode Deserialize<'a>>(&self) -> AppResult { decode_seek(&self.0).map_err(|_| bad_request("invalid seek parameter")) } + + pub(crate) fn is_empty(&self) -> bool { + self.0.is_empty() + } } /// Function to check if the request is blocked. @@ -547,8 +551,8 @@ macro_rules! seek { impl $name { pub fn decode(&self, page: &Page) -> AppResult]>> { let encoded = match page { - Page::Seek(encoded) => encoded, - Page::SeekBackward(encoded) => encoded, + Page::Seek(encoded) if !encoded.is_empty() => encoded, + Page::SeekBackward(encoded) if !encoded.is_empty() => encoded, _ => return Ok(None), }; @@ -780,6 +784,24 @@ mod tests { assert_eq!(response.status(), StatusCode::BAD_REQUEST); } + // empty string + { + let seek = Seek::Id; + let pagination = PaginationOptions::builder() + .enable_seek(true) + .gather(&mock("seek=")) + .unwrap(); + assert_eq!(seek.decode(&pagination.page).unwrap(), None); + + // for backward + let seek = Seek::Id; + let pagination = PaginationOptions::builder() + .enable_seek_backward(true) + .gather(&mock("seek=-")) + .unwrap(); + assert_eq!(seek.decode(&pagination.page).unwrap(), None); + } + // Ensures it still encodes compactly with a field struct #[derive(Debug, Default, Serialize, PartialEq)] struct NewTuple( From 624ab10944253dd74dc9fea4dfff6931ae122ee3 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 5 Mar 2025 19:01:57 +0800 Subject: [PATCH 05/13] controllers/krate/search: Build query ordering based on direction --- src/controllers/krate/search.rs | 63 ++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 9d43083519e..18beff0a9ac 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -116,14 +116,18 @@ pub async fn list_crates( .left_join(versions::table.on(default_versions::version_id.eq(versions::id))) .select(selection); + let pagination: PaginationOptions = PaginationOptions::builder() + .limit_page_numbers() + .enable_seek(true) + .gather(&req)?; + let is_forward = !matches!(pagination.page, Page::SeekBackward(_)); + if let Some(q_string) = &filter_params.q_string { if !q_string.is_empty() { let q_string = q_string.as_str(); let sort = sort.unwrap_or("relevance"); - query = query.order(Crate::with_name(q_string).desc()); - if sort == "relevance" { let q = plainto_tsquery_with_search_config(TsConfigurationByName("english"), q_string); @@ -139,7 +143,11 @@ pub async fn list_crates( default_versions::num_versions.nullable(), )); seek = Some(Seek::Relevance); - query = query.then_order_by(rank.desc()) + query = if is_forward { + query.order((Crate::with_name(q_string).desc(), rank.desc())) + } else { + query.order((Crate::with_name(q_string).asc(), rank.asc())) + } } else { query = query.select(( ALL_COLUMNS, @@ -152,6 +160,11 @@ pub async fn list_crates( default_versions::num_versions.nullable(), )); seek = Some(Seek::Query); + query = if is_forward { + query.order(Crate::with_name(q_string).desc()) + } else { + query.order(Crate::with_name(q_string).asc()) + } } } } @@ -163,31 +176,49 @@ pub async fn list_crates( // to ensure predictable pagination behavior. if sort == Some("downloads") { seek = Some(Seek::Downloads); - query = query.order((crate_downloads::downloads.desc(), crates::id.desc())) + query = if is_forward { + query.order((crate_downloads::downloads.desc(), crates::id.desc())) + } else { + query.order((crate_downloads::downloads.asc(), crates::id.asc())) + }; } else if sort == Some("recent-downloads") { seek = Some(Seek::RecentDownloads); - query = query.order(( - recent_crate_downloads::downloads.desc().nulls_last(), - crates::id.desc(), - )) + query = if is_forward { + query.order(( + recent_crate_downloads::downloads.desc().nulls_last(), + crates::id.desc(), + )) + } else { + query.order(( + recent_crate_downloads::downloads.asc().nulls_first(), + crates::id.asc(), + )) + }; } else if sort == Some("recent-updates") { seek = Some(Seek::RecentUpdates); - query = query.order((crates::updated_at.desc(), crates::id.desc())); + query = if is_forward { + query.order((crates::updated_at.desc(), crates::id.desc())) + } else { + query.order((crates::updated_at.asc(), crates::id.asc())) + }; } else if sort == Some("new") { seek = Some(Seek::New); - query = query.order((crates::created_at.desc(), crates::id.desc())); + query = if is_forward { + query.order((crates::created_at.desc(), crates::id.desc())) + } else { + query.order((crates::created_at.asc(), crates::id.asc())) + }; } else { seek = seek.or(Some(Seek::Name)); // Since the name is unique value, the inherent ordering becomes naturally unique. // Therefore, an additional auxiliary ordering column is unnecessary in this case. - query = query.then_order_by(crates::name.asc()) + query = if is_forward { + query.then_order_by(crates::name.asc()) + } else { + query.then_order_by(crates::name.desc()) + }; } - let pagination: PaginationOptions = PaginationOptions::builder() - .limit_page_numbers() - .enable_seek(true) - .gather(&req)?; - let explicit_page = matches!(pagination.page, Page::Numeric(_)); // To avoid breaking existing users, seek-based pagination is only used if an explicit page has From 0e21380cdcb0e31a3fd331e7c5a92d656a5e021c Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 5 Mar 2025 19:45:23 +0800 Subject: [PATCH 06/13] controllers/krate/search: Build query filter for seek-based pagination ...based on direction Most of this should be intuitive, as it simply flips the comparison operator. To make it easier to identify the difference, I chose to implement it in a single function and place it next to the forward ordering conditions. The only part that requires a bit of thought is the case involving nulls last. --- src/controllers/krate/search.rs | 293 +++++++++++++++++++++----------- 1 file changed, 196 insertions(+), 97 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 18beff0a9ac..21231f4f7c2 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -228,7 +228,7 @@ pub async fn list_crates( let seek = seek.unwrap(); if let Some(condition) = seek .decode(&pagination.page)? - .map(|s| filter_params.seek_after(&s)) + .map(|s| filter_params.seek(&s, is_forward)) { query = query.filter(condition); } @@ -506,7 +506,7 @@ impl FilterParams { query } - fn seek_after(&self, seek_payload: &seek::SeekPayload) -> BoxedCondition<'_> { + fn seek(&self, seek_payload: &seek::SeekPayload, is_forward: bool) -> BoxedCondition<'_> { use seek::*; let crates_aliased = alias!(crates as crates_aliased); @@ -518,63 +518,85 @@ impl FilterParams { }; let conditions: Vec> = match *seek_payload { SeekPayload::Name(Name { id }) => { - // Equivalent of: - // ``` - // WHERE name > name' - // ORDER BY name ASC - // ``` - vec![Box::new(crates::name.nullable().gt(crate_name_by_id(id)))] + if is_forward { + // Equivalent of: + // ``` + // WHERE name > name' + // ORDER BY name ASC + // ``` + vec![Box::new(crates::name.nullable().gt(crate_name_by_id(id)))] + } else { + vec![Box::new(crates::name.nullable().lt(crate_name_by_id(id)))] + } } SeekPayload::New(New { created_at, id }) => { - // Equivalent of: - // ``` - // WHERE (created_at = created_at' AND id < id') OR created_at < created_at' - // ORDER BY created_at DESC, id DESC - // ``` - vec![ - Box::new( - crates::created_at - .eq(created_at) - .and(crates::id.lt(id)) - .nullable(), - ), - Box::new(crates::created_at.lt(created_at).nullable()), - ] + if is_forward { + // Equivalent of: + // ``` + // WHERE (created_at = created_at' AND id < id') OR created_at < created_at' + // ORDER BY created_at DESC, id DESC + // ``` + vec![ + Box::new( + crates::created_at + .eq(created_at) + .and(crates::id.lt(id)) + .nullable(), + ), + Box::new(crates::created_at.lt(created_at).nullable()), + ] + } else { + vec![ + Box::new( + crates::created_at + .eq(created_at) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crates::created_at.gt(created_at).nullable()), + ] + } } SeekPayload::RecentUpdates(RecentUpdates { updated_at, id }) => { - // Equivalent of: - // ``` - // WHERE (updated_at = updated_at' AND id < id') OR updated_at < updated_at' - // ORDER BY updated_at DESC, id DESC - // ``` - vec![ - Box::new( - crates::updated_at - .eq(updated_at) - .and(crates::id.lt(id)) - .nullable(), - ), - Box::new(crates::updated_at.lt(updated_at).nullable()), - ] + if is_forward { + // Equivalent of: + // ``` + // WHERE (updated_at = updated_at' AND id < id') OR updated_at < updated_at' + // ORDER BY updated_at DESC, id DESC + // ``` + vec![ + Box::new( + crates::updated_at + .eq(updated_at) + .and(crates::id.lt(id)) + .nullable(), + ), + Box::new(crates::updated_at.lt(updated_at).nullable()), + ] + } else { + vec![ + Box::new( + crates::updated_at + .eq(updated_at) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crates::updated_at.gt(updated_at).nullable()), + ] + } } SeekPayload::RecentDownloads(RecentDownloads { recent_downloads, id, }) => { - // Equivalent of: - // for recent_downloads is not None: - // ``` - // WHERE (recent_downloads = recent_downloads' AND id < id') - // OR (recent_downloads < recent_downloads' OR recent_downloads IS NULL) - // ORDER BY recent_downloads DESC NULLS LAST, id DESC - // ``` - // for recent_downloads is None: - // ``` - // WHERE (recent_downloads IS NULL AND id < id') - // ORDER BY recent_downloads DESC NULLS LAST, id DESC - // ``` - match recent_downloads { - Some(dl) => { + match (recent_downloads, is_forward) { + (Some(dl), true) => { + // Equivalent of: + // ``` + // WHERE (recent_downloads = recent_downloads' AND id < id') + // OR (recent_downloads < recent_downloads' OR recent_downloads IS NULL) + // ORDER BY recent_downloads DESC NULLS LAST, id DESC + // ``` vec![ Box::new( recent_crate_downloads::downloads @@ -590,7 +612,12 @@ impl FilterParams { ), ] } - None => { + (None, true) => { + // Equivalent of: + // ``` + // WHERE (recent_downloads IS NULL AND id < id') + // ORDER BY recent_downloads DESC NULLS LAST, id DESC + // ``` vec![Box::new( recent_crate_downloads::downloads .is_null() @@ -598,54 +625,105 @@ impl FilterParams { .nullable(), )] } + (Some(dl), false) => { + // Equivalent of: + // ``` + // WHERE (recent_downloads = recent_downloads' AND id > id') + // OR (recent_downloads > recent_downloads') + // ORDER BY recent_downloads ASC NULLS FIRST, id ASC + // ``` + vec![ + Box::new( + recent_crate_downloads::downloads + .eq(dl) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(recent_crate_downloads::downloads.gt(dl).nullable()), + ] + } + (None, false) => { + // Equivalent of: + // ``` + // WHERE (recent_downloads IS NULL AND id > id') + // OR (recent_downloads IS NOT NULL) + // ORDER BY recent_downloads ASC NULLS FIRST, id ASC + // ``` + vec![ + Box::new( + recent_crate_downloads::downloads + .is_null() + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(recent_crate_downloads::downloads.is_not_null().nullable()), + ] + } } } SeekPayload::Downloads(Downloads { downloads, id }) => { - // Equivalent of: - // ``` - // WHERE (downloads = downloads' AND id < id') OR downloads < downloads' - // ORDER BY downloads DESC, id DESC - // ``` - vec![ - Box::new( - crate_downloads::downloads - .eq(downloads) - .and(crates::id.lt(id)) - .nullable(), - ), - Box::new(crate_downloads::downloads.lt(downloads).nullable()), - ] + if is_forward { + // Equivalent of: + // ``` + // WHERE (downloads = downloads' AND id < id') OR downloads < downloads' + // ORDER BY downloads DESC, id DESC + // ``` + vec![ + Box::new( + crate_downloads::downloads + .eq(downloads) + .and(crates::id.lt(id)) + .nullable(), + ), + Box::new(crate_downloads::downloads.lt(downloads).nullable()), + ] + } else { + vec![ + Box::new( + crate_downloads::downloads + .eq(downloads) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crate_downloads::downloads.gt(downloads).nullable()), + ] + } } SeekPayload::Query(Query { exact_match, id }) => { - // Equivalent of: - // ``` - // WHERE (exact_match = exact_match' AND name > name') OR exact_match < exact_match' - // ORDER BY exact_match DESC, NAME ASC - // ``` let q_string = self.q_string.as_ref().expect("q_string should not be None"); let name_exact_match = Crate::with_name(q_string); - vec![ - Box::new( - name_exact_match - .eq(exact_match) - .and(crates::name.nullable().gt(crate_name_by_id(id))) - .nullable(), - ), - Box::new(name_exact_match.lt(exact_match).nullable()), - ] + if is_forward { + // Equivalent of: + // ``` + // WHERE (exact_match = exact_match' AND name > name') OR exact_match < exact_match' + // ORDER BY exact_match DESC, NAME ASC + // ``` + vec![ + Box::new( + name_exact_match + .eq(exact_match) + .and(crates::name.nullable().gt(crate_name_by_id(id))) + .nullable(), + ), + Box::new(name_exact_match.lt(exact_match).nullable()), + ] + } else { + vec![ + Box::new( + name_exact_match + .eq(exact_match) + .and(crates::name.nullable().lt(crate_name_by_id(id))) + .nullable(), + ), + Box::new(name_exact_match.gt(exact_match).nullable()), + ] + } } SeekPayload::Relevance(Relevance { exact_match: exact, rank: rank_in, id, }) => { - // Equivalent of: - // ``` - // WHERE (exact_match = exact_match' AND rank = rank' AND name > name') - // OR (exact_match = exact_match' AND rank < rank') - // OR exact_match < exact_match' - // ORDER BY exact_match DESC, rank DESC, name ASC - // ``` let q_string = self.q_string.as_ref().expect("q_string should not be None"); let q = plainto_tsquery_with_search_config( TsConfigurationByName("english"), @@ -653,17 +731,38 @@ impl FilterParams { ); let rank = ts_rank_cd(crates::textsearchable_index_col, q); let name_exact_match = Crate::with_name(q_string.as_str()); - vec![ - Box::new( - name_exact_match - .eq(exact) - .and(rank.eq(rank_in)) - .and(crates::name.nullable().gt(crate_name_by_id(id))) - .nullable(), - ), - Box::new(name_exact_match.eq(exact).and(rank.lt(rank_in)).nullable()), - Box::new(name_exact_match.lt(exact).nullable()), - ] + if is_forward { + // Equivalent of: + // ``` + // WHERE (exact_match = exact_match' AND rank = rank' AND name > name') + // OR (exact_match = exact_match' AND rank < rank') + // OR exact_match < exact_match' + // ORDER BY exact_match DESC, rank DESC, name ASC + // ``` + vec![ + Box::new( + name_exact_match + .eq(exact) + .and(rank.eq(rank_in)) + .and(crates::name.nullable().gt(crate_name_by_id(id))) + .nullable(), + ), + Box::new(name_exact_match.eq(exact).and(rank.lt(rank_in)).nullable()), + Box::new(name_exact_match.lt(exact).nullable()), + ] + } else { + vec![ + Box::new( + name_exact_match + .eq(exact) + .and(rank.eq(rank_in)) + .and(crates::name.nullable().lt(crate_name_by_id(id))) + .nullable(), + ), + Box::new(name_exact_match.eq(exact).and(rank.gt(rank_in)).nullable()), + Box::new(name_exact_match.gt(exact).nullable()), + ] + } } }; From 6bb6793dc2fefe92cbbe4e7da317f862e28e0aee Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 6 Mar 2025 20:40:49 +0800 Subject: [PATCH 07/13] controllers/helpers/pagination: Add `PaginationOptions::is_backward()` fn --- src/controllers/helpers/pagination.rs | 4 ++++ src/controllers/krate/search.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index ffb36b2ce52..b5fccfcdf89 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -56,6 +56,10 @@ impl PaginationOptions { None } } + + pub(crate) fn is_backward(&self) -> bool { + matches!(self.page, Page::SeekBackward(_)) + } } #[derive(Debug, Deserialize, FromRequestParts, utoipa::IntoParams)] diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 21231f4f7c2..a5d3741064d 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -120,7 +120,7 @@ pub async fn list_crates( .limit_page_numbers() .enable_seek(true) .gather(&req)?; - let is_forward = !matches!(pagination.page, Page::SeekBackward(_)); + let is_forward = !pagination.is_backward(); if let Some(q_string) = &filter_params.q_string { if !q_string.is_empty() { From 7b91ca37652033ac6a8459c257e5ee986e3c3498 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 6 Mar 2025 20:41:40 +0800 Subject: [PATCH 08/13] controllers/helpers/pagination: Add `PaginationOptions::is_explicit()` fn This also remove the obsolete `Paginated::is_explicit_page()`. --- src/controllers/helpers/pagination.rs | 11 ++++++----- src/controllers/krate/search.rs | 6 ++---- src/controllers/krate/versions.rs | 16 ++++++---------- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index b5fccfcdf89..74ece40c0d9 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -60,6 +60,10 @@ impl PaginationOptions { pub(crate) fn is_backward(&self) -> bool { matches!(self.page, Page::SeekBackward(_)) } + + pub(crate) fn is_explicit(&self) -> bool { + matches!(self.page, Page::Numeric(_)) + } } #[derive(Debug, Deserialize, FromRequestParts, utoipa::IntoParams)] @@ -251,7 +255,8 @@ impl Paginated { S: Serialize, { // TODO: handle this more properly when seek backward comes into play. - if self.is_explicit_page() || self.records_and_total.len() < self.options.per_page as usize + if self.options.is_explicit() + || self.records_and_total.len() < self.options.per_page as usize { return Ok(None); } @@ -267,10 +272,6 @@ impl Paginated { Ok(Some(opts)) } - fn is_explicit_page(&self) -> bool { - matches!(&self.options.page, Page::Numeric(_)) - } - pub(crate) fn iter(&self) -> impl Iterator { self.records_and_total.iter().map(|row| &row.record) } diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index a5d3741064d..02ea6489177 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -21,7 +21,7 @@ use crate::schema::*; use crate::util::errors::{AppResult, bad_request}; use crate::views::EncodableCrate; -use crate::controllers::helpers::pagination::{Page, PaginationOptions, PaginationQueryParams}; +use crate::controllers::helpers::pagination::{PaginationOptions, PaginationQueryParams}; use crate::models::krate::ALL_COLUMNS; use crate::util::RequestUtils; use crate::util::string_excl_null::StringExclNull; @@ -219,12 +219,10 @@ pub async fn list_crates( }; } - let explicit_page = matches!(pagination.page, Page::Numeric(_)); - // To avoid breaking existing users, seek-based pagination is only used if an explicit page has // not been provided. This way clients relying on meta.next_page will use the faster seek-based // paginations, while client hardcoding pages handling will use the slower offset-based code. - let (total, next_page, prev_page, data) = if !explicit_page && seek.is_some() { + let (total, next_page, prev_page, data) = if !pagination.is_explicit() && seek.is_some() { let seek = seek.unwrap(); if let Some(condition) = seek .decode(&pagination.page)? diff --git a/src/controllers/krate/versions.rs b/src/controllers/krate/versions.rs index 24d9b50dff2..6c07ce91f17 100644 --- a/src/controllers/krate/versions.rs +++ b/src/controllers/krate/versions.rs @@ -155,11 +155,10 @@ async fn list_by_date( let mut query = make_base_query(); if let Some(options) = options { - assert!( - !matches!(&options.page, Page::Numeric(_)), - "?page= is not supported" - ); - if let Some(SeekPayload::Date(Date { created_at, id })) = Seek::Date.decode(&options.page)? { + assert!(!options.is_explicit(), "?page= is not supported"); + if let Some(SeekPayload::Date(Date { created_at, id })) = + Seek::Date.decode(&options.page)? + { query = query.filter( versions::created_at .eq(created_at) @@ -282,10 +281,7 @@ async fn list_by_semver( sorted_versions .sort_unstable_by(|_, (semver_a, _, _), _, (semver_b, _, _)| semver_b.cmp(semver_a)); - assert!( - !matches!(&options.page, Page::Numeric(_)), - "?page= is not supported" - ); + assert!(!options.is_explicit(), "?page= is not supported"); let release_tracks = include.release_tracks.then(|| { ReleaseTracks::from_sorted_semver_iter( @@ -422,7 +418,7 @@ where F: Fn(&T) -> S, S: serde::Serialize, { - if matches!(options.page, Page::Numeric(_)) || records.len() < options.per_page as usize { + if options.is_explicit() || records.len() < options.per_page as usize { return Ok(None); } From 454f35da6e1f15bb94b0feac0d4a86108b67f56b Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Thu, 6 Mar 2025 21:54:33 +0800 Subject: [PATCH 09/13] controllers/helpers/pagination: Handle null `next_page` for seek-based pagination ...when seeking backward involved. --- src/controllers/helpers/pagination.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 74ece40c0d9..e2a5dbbaf52 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -254,15 +254,20 @@ impl Paginated { F: Fn(&T) -> S, S: Serialize, { - // TODO: handle this more properly when seek backward comes into play. + // When the data size is smaller than the page size, we would expect the next page to be + // available during backward pagination but unavailable during forward pagination. if self.options.is_explicit() - || self.records_and_total.len() < self.options.per_page as usize + || self.records_and_total.is_empty() + || (self.records_and_total.len() < self.options.per_page as usize + && !self.options.is_backward()) { return Ok(None); } + // We also like to return None for next page when it's the first backward pagination. let mut opts = IndexMap::new(); match self.options.page { + Page::SeekBackward(ref raw) if raw.is_empty() => return Ok(None), Page::Unspecified | Page::Seek(_) | Page::SeekBackward(_) => { let seek = f(&self.records_and_total.last().unwrap().record); opts.insert("seek".into(), encode_seek(seek)?); From 193a57d0d4293ccb07d05035f0bb6cc89c855c29 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Sun, 9 Mar 2025 18:55:42 +0800 Subject: [PATCH 10/13] controllers/helpers/pagination: Maintain consistent ordering from page to page For instance, given rows with the following: | a | b | c | ... | 1 | 2 | 3 | When performing forward seek pagination with page size as 3 to the last page, let's say we have the last page as 1, 2, 3. When performing backward seek pagination, we reverse the order of the query so that we can retrieve the data from the tail. This results in 3, 2, 1. However, we would like to maintain consistent ordering as we view it with forward pagination, namely 1, 2, 3. Therefore, we need to reverse the results again to maintain the original ordering. --- src/controllers/helpers/pagination.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index e2a5dbbaf52..62cf44062b6 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -313,9 +313,12 @@ impl PaginatedQuery { let future = self.internal_load(conn); async move { - let records_and_total = future.await?.try_collect().await?; + let mut records_and_total: Vec<_> = future.await?.try_collect().await?; - // TODO: maintain consistent ordering from page to pagen + // This maintain consistent ordering from page to pagen + if options.is_backward() { + records_and_total.reverse(); + } Ok(Paginated { records_and_total, @@ -467,9 +470,12 @@ impl PaginatedQueryWithCountSubq { let future = self.internal_load(conn); async move { - let records_and_total = future.await?.try_collect().await?; + let mut records_and_total: Vec<_> = future.await?.try_collect().await?; - // TODO: maintain consistent ordering from page to pagen + // This maintain consistent ordering from page to pagen + if options.is_backward() { + records_and_total.reverse(); + } Ok(Paginated { records_and_total, From 847e8bce0898de9c2850e9c120b595dc6065402b Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Sun, 9 Mar 2025 19:58:41 +0800 Subject: [PATCH 11/13] controllers/helpers/pagination: Generate `prev_page` for seek-based pagination --- src/controllers/helpers/pagination.rs | 29 +++++++++++++++++++++++++++ src/controllers/krate/search.rs | 3 ++- src/tests/routes/crates/list.rs | 4 +++- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 62cf44062b6..0800675bb4b 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -277,6 +277,35 @@ impl Paginated { Ok(Some(opts)) } + pub(crate) fn prev_seek_params(&self, f: F) -> AppResult>> + where + F: Fn(&T) -> S, + S: Serialize, + { + // When the data size is smaller than the page size, we would expect the prev page to be + // unavailable during backward pagination but available during forward pagination. + if self.options.is_explicit() + || self.records_and_total.is_empty() + || (self.records_and_total.len() < self.options.per_page as usize + && self.options.is_backward()) + { + return Ok(None); + } + + // We also like to return None for prev page when it's the first forward pagination. + let mut opts = IndexMap::new(); + match self.options.page { + Page::Unspecified => return Ok(None), + Page::Seek(ref raw) if raw.is_empty() => return Ok(None), + Page::Seek(_) | Page::SeekBackward(_) => { + let seek = f(&self.records_and_total.first().unwrap().record); + opts.insert("seek".into(), format!("-{}", encode_seek(seek)?)); + } + Page::Numeric(_) => unreachable!(), + }; + Ok(Some(opts)) + } + pub(crate) fn iter(&self) -> impl Iterator { self.records_and_total.iter().map(|row| &row.record) } diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 02ea6489177..0b7fb345df4 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -245,7 +245,8 @@ pub async fn list_crates( data.total(), data.next_seek_params(|last| seek.to_payload(last))? .map(|p| req.query_with_params(p)), - None, + data.prev_seek_params(|first| seek.to_payload(first))? + .map(|p| req.query_with_params(p)), data.into_iter().collect::>(), ) } else { diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 6f5d09d33fc..01cff0e8b25 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1077,7 +1077,9 @@ async fn seek_based_pagination() -> anyhow::Result<()> { assert_eq!(resp.meta.total, 0); } - assert_eq!(resp.meta.prev_page, None); + if calls == 1 { + assert_eq!(resp.meta.prev_page, None); + } } assert_eq!(calls, 4); From 3f142f8f4497040f26551c4c254ea34636dff072 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 10 Mar 2025 02:00:05 +0800 Subject: [PATCH 12/13] controllers/krate/search: Enable seeking backward for search --- src/controllers/krate/search.rs | 1 + src/tests/routes/crates/list.rs | 170 +++++++++++++++++++++++--------- 2 files changed, 127 insertions(+), 44 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 0b7fb345df4..61b6bd4fc13 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -119,6 +119,7 @@ pub async fn list_crates( let pagination: PaginationOptions = PaginationOptions::builder() .limit_page_numbers() .enable_seek(true) + .enable_seek_backward(true) .gather(&req)?; let is_forward = !pagination.is_backward(); diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 01cff0e8b25..7ac56544374 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -352,13 +352,20 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(json.crates[2].name, "bar_sort"); assert_eq!(json.crates[3].name, "foo_sort"); } - let (resp, calls) = page_with_seek(&anon, "sort=downloads").await; + let (resp, calls) = page_with_seek_forward(&anon, "sort=downloads").await; assert_eq!(resp[0].crates[0].name, "other_sort"); assert_eq!(resp[1].crates[0].name, "baz_sort"); assert_eq!(resp[2].crates[0].name, "bar_sort"); assert_eq!(resp[3].crates[0].name, "foo_sort"); assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, "sort=downloads").await; + assert_eq!(resp[0].crates[0].name, "foo_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "baz_sort"); + assert_eq!(resp[3].crates[0].name, "other_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by recent-downloads for json in search_both(&anon, "sort=recent-downloads").await { @@ -368,13 +375,20 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(json.crates[2].name, "bar_sort"); assert_eq!(json.crates[3].name, "other_sort"); } - let (resp, calls) = page_with_seek(&anon, "sort=recent-downloads").await; + let (resp, calls) = page_with_seek_forward(&anon, "sort=recent-downloads").await; assert_eq!(resp[0].crates[0].name, "baz_sort"); assert_eq!(resp[1].crates[0].name, "foo_sort"); assert_eq!(resp[2].crates[0].name, "bar_sort"); assert_eq!(resp[3].crates[0].name, "other_sort"); assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, "sort=recent-downloads").await; + assert_eq!(resp[0].crates[0].name, "other_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "foo_sort"); + assert_eq!(resp[3].crates[0].name, "baz_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by recent-updates for json in search_both(&anon, "sort=recent-updates").await { @@ -384,13 +398,20 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(json.crates[2].name, "bar_sort"); assert_eq!(json.crates[3].name, "foo_sort"); } - let (resp, calls) = page_with_seek(&anon, "sort=recent-updates").await; + let (resp, calls) = page_with_seek_forward(&anon, "sort=recent-updates").await; assert_eq!(resp[0].crates[0].name, "other_sort"); assert_eq!(resp[1].crates[0].name, "baz_sort"); assert_eq!(resp[2].crates[0].name, "bar_sort"); assert_eq!(resp[3].crates[0].name, "foo_sort"); assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, "sort=recent-updates").await; + assert_eq!(resp[0].crates[0].name, "foo_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "baz_sort"); + assert_eq!(resp[3].crates[0].name, "other_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by new for json in search_both(&anon, "sort=new").await { @@ -400,18 +421,25 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(json.crates[2].name, "baz_sort"); assert_eq!(json.crates[3].name, "foo_sort"); } - let (resp, calls) = page_with_seek(&anon, "sort=new").await; + let (resp, calls) = page_with_seek_forward(&anon, "sort=new").await; assert_eq!(resp[0].crates[0].name, "bar_sort"); assert_eq!(resp[1].crates[0].name, "other_sort"); assert_eq!(resp[2].crates[0].name, "baz_sort"); assert_eq!(resp[3].crates[0].name, "foo_sort"); assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, "sort=new").await; + assert_eq!(resp[0].crates[0].name, "foo_sort"); + assert_eq!(resp[1].crates[0].name, "baz_sort"); + assert_eq!(resp[2].crates[0].name, "other_sort"); + assert_eq!(resp[3].crates[0].name, "bar_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by alpha with query // ordering (exact match desc, name asc) let query = "sort=alpha&q=bar_sort"; - let (resp, calls) = page_with_seek(&anon, query).await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 3); assert_eq!(resp[0].crates[0].name, "bar_sort"); @@ -419,9 +447,15 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(resp[2].crates[0].name, "foo_sort"); } assert_eq!(calls, 4); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "foo_sort"); + assert_eq!(resp[1].crates[0].name, "baz_sort"); + assert_eq!(resp[2].crates[0].name, "bar_sort"); + assert_eq!(resp[2].meta.total, 3); + assert_eq!(calls, 4); let query = "sort=alpha&q=sort"; - let (resp, calls) = page_with_seek(&anon, query).await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 4); assert_eq!(resp[0].crates[0].name, "bar_sort"); @@ -430,11 +464,18 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(resp[3].crates[0].name, "other_sort"); } assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "other_sort"); + assert_eq!(resp[1].crates[0].name, "foo_sort"); + assert_eq!(resp[2].crates[0].name, "baz_sort"); + assert_eq!(resp[3].crates[0].name, "bar_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by relevance // ordering (exact match desc, rank desc, name asc) let query = "q=foo_sort"; - let (resp, calls) = page_with_seek(&anon, query).await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 3); assert_eq!(resp[0].crates[0].name, "foo_sort"); @@ -443,13 +484,19 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(resp[2].crates[0].name, "baz_sort"); } assert_eq!(calls, 4); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "baz_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "foo_sort"); + assert_eq!(resp[2].meta.total, 3); + assert_eq!(calls, 4); let ranks = querystring_rank(&mut conn, "foo_sort").await; assert_eq!(ranks.get("bar_sort"), ranks.get("baz_sort")); // Add query containing a space to ensure tsquery works // "foo_sort" and "foo sort" would generate same tsquery let query = "q=foo%20sort"; - let (resp, calls) = page_with_seek(&anon, query).await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 3); assert_eq!(resp[0].crates[0].name, "foo_sort"); @@ -458,11 +505,17 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(resp[2].crates[0].name, "baz_sort"); } assert_eq!(calls, 4); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "baz_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "foo_sort"); + assert_eq!(resp[2].meta.total, 3); + assert_eq!(calls, 4); let ranks = querystring_rank(&mut conn, "foo%20sort").await; assert_eq!(ranks.get("bar_sort"), ranks.get("baz_sort")); let query = "q=sort"; - let (resp, calls) = page_with_seek(&anon, query).await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 4); // by rank desc (items with more "sort" should have a hider rank value) @@ -472,6 +525,13 @@ async fn index_sorting() -> anyhow::Result<()> { assert_eq!(resp[3].crates[0].name, "other_sort"); } assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "other_sort"); + assert_eq!(resp[1].crates[0].name, "foo_sort"); + assert_eq!(resp[2].crates[0].name, "bar_sort"); + assert_eq!(resp[3].crates[0].name, "baz_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); let ranks = querystring_rank(&mut conn, "sort").await; assert_eq!( ranks.keys().collect::>(), @@ -480,20 +540,28 @@ async fn index_sorting() -> anyhow::Result<()> { // Test for bug with showing null results first when sorting // by descending downloads - for json in search_both(&anon, "sort=recent-downloads").await { + let query = "sort=recent-downloads"; + for json in search_both(&anon, query).await { assert_eq!(json.meta.total, 4); assert_eq!(json.crates[0].name, "baz_sort"); assert_eq!(json.crates[1].name, "foo_sort"); assert_eq!(json.crates[2].name, "bar_sort"); assert_eq!(json.crates[3].name, "other_sort"); } - let (resp, calls) = page_with_seek(&anon, "sort=recent-downloads").await; + let (resp, calls) = page_with_seek_forward(&anon, query).await; assert_eq!(resp[0].crates[0].name, "baz_sort"); assert_eq!(resp[1].crates[0].name, "foo_sort"); assert_eq!(resp[2].crates[0].name, "bar_sort"); assert_eq!(resp[3].crates[0].name, "other_sort"); assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + let (resp, calls) = page_with_seek_backward(&anon, query).await; + assert_eq!(resp[0].crates[0].name, "other_sort"); + assert_eq!(resp[1].crates[0].name, "bar_sort"); + assert_eq!(resp[2].crates[0].name, "foo_sort"); + assert_eq!(resp[3].crates[0].name, "baz_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); Ok(()) } @@ -1052,45 +1120,35 @@ async fn seek_based_pagination() -> anyhow::Result<()> { .expect_build(&mut conn) .await; - let mut url = Some("?per_page=1".to_string()); - let mut results = Vec::new(); - let mut calls = 0; - while let Some(current_url) = url.take() { - let resp = anon.search(current_url.trim_start_matches('?')).await; - calls += 1; - - results.append( - &mut resp - .crates - .iter() - .map(|res| res.name.clone()) - .collect::>(), - ); - - if let Some(new_url) = resp.meta.next_page { - assert_that!(resp.crates, len(eq(1))); - url = Some(new_url); - assert_eq!(resp.meta.total, 3); - assert!(default_versions_iter(&resp.crates).all(Option::is_some)); - } else { - assert_that!(resp.crates, empty()); - assert_eq!(resp.meta.total, 0); - } - - if calls == 1 { - assert_eq!(resp.meta.prev_page, None); - } + fn names(resp: &[crate::tests::CrateList]) -> std::vec::Vec<&str> { + resp.iter() + .flat_map(|r| r.crates.iter().map(|c| c.name.as_str())) + .collect::>() } + let (resp, calls) = page_with_seek_forward(&anon, "").await; assert_eq!(calls, 4); assert_eq!( vec![ "pagination_links_1", "pagination_links_2", - "pagination_links_3" + "pagination_links_3", + ], + names(&resp) + ); + assert_eq!(resp[0].meta.prev_page, None); + + let (resp, calls) = page_with_seek_backward(&anon, "").await; + assert_eq!(calls, 4); + assert_eq!( + vec![ + "pagination_links_3", + "pagination_links_2", + "pagination_links_1", ], - results + names(&resp) ); + assert_eq!(resp[0].meta.next_page, None); Ok(()) } @@ -1250,11 +1308,29 @@ async fn search_both_by_user_id( search_both(anon, &url).await } -async fn page_with_seek( +async fn page_with_seek_forward( + anon: &U, + query: &str, +) -> (Vec, i32) { + _page_with_seek(anon, query, true).await +} + +async fn page_with_seek_backward( anon: &U, query: &str, ) -> (Vec, i32) { - let mut url = Some(format!("?per_page=1&{query}")); + _page_with_seek(anon, query, false).await +} + +async fn _page_with_seek( + anon: &U, + query: &str, + forward: bool, +) -> (Vec, i32) { + let mut url = Some(format!( + "?per_page=1&{query}{}", + (!forward).then_some("&seek=-").unwrap_or_default() + )); let mut results = Vec::new(); let mut calls = 0; while let Some(current_url) = url.take() { @@ -1264,7 +1340,13 @@ async fn page_with_seek( panic!("potential infinite loop detected!") } - if let Some(ref new_url) = resp.meta.next_page { + let next_url = if forward { + resp.meta.next_page.as_ref() + } else { + resp.meta.prev_page.as_ref() + }; + + if let Some(new_url) = next_url { assert!(new_url.contains("seek=")); assert_that!(resp.crates, len(eq(1))); url = Some(new_url.to_owned()); From 44d5e670ac84107832c1086e50cba635cac252f2 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 10 Mar 2025 17:39:03 +0800 Subject: [PATCH 13/13] controllers/helpers/pagination: Update the `seek` parameter descriptions --- src/controllers/helpers/pagination.rs | 4 +++- .../crates_io__openapi__tests__openapi_snapshot.snap | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 0800675bb4b..41718d021a3 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -87,7 +87,9 @@ pub struct PaginationQueryParams { /// all requests. /// /// The seek key can usually be found in the `meta.next_page` field of - /// paginated responses. + /// paginated responses. It can also be found in the `meta.prev_page` field + /// when the endpoint supports backward pagination, in which case the value + /// starts with a `-`. pub seek: Option, } diff --git a/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap b/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap index cb7a074036a..3fdcebf0e10 100644 --- a/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap +++ b/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap @@ -1232,7 +1232,7 @@ expression: response.json() } }, { - "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses.", + "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses. It can also be found in the `meta.prev_page` field\nwhen the endpoint supports backward pagination, in which case the value\nstarts with a `-`.", "in": "query", "name": "seek", "required": false, @@ -1450,7 +1450,7 @@ expression: response.json() } }, { - "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses.", + "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses. It can also be found in the `meta.prev_page` field\nwhen the endpoint supports backward pagination, in which case the value\nstarts with a `-`.", "in": "query", "name": "seek", "required": false, @@ -1758,7 +1758,7 @@ expression: response.json() } }, { - "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses.", + "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses. It can also be found in the `meta.prev_page` field\nwhen the endpoint supports backward pagination, in which case the value\nstarts with a `-`.", "in": "query", "name": "seek", "required": false, @@ -2666,7 +2666,7 @@ expression: response.json() } }, { - "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses.", + "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses. It can also be found in the `meta.prev_page` field\nwhen the endpoint supports backward pagination, in which case the value\nstarts with a `-`.", "in": "query", "name": "seek", "required": false, @@ -3279,7 +3279,7 @@ expression: response.json() } }, { - "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses.", + "description": "The seek key to request.\n\nThis parameter is mutually exclusive with `page` and not supported for\nall requests.\n\nThe seek key can usually be found in the `meta.next_page` field of\npaginated responses. It can also be found in the `meta.prev_page` field\nwhen the endpoint supports backward pagination, in which case the value\nstarts with a `-`.", "in": "query", "name": "seek", "required": false,