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 5ce90c882e8..41718d021a3 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, } @@ -43,6 +44,7 @@ impl PaginationOptions { PaginationOptionsBuilder { limit_page_numbers: false, enable_seek: false, + enable_seek_backward: false, enable_pages: true, } } @@ -54,6 +56,14 @@ impl PaginationOptions { None } } + + 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)] @@ -77,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, } @@ -85,6 +97,7 @@ pub(crate) struct PaginationOptionsBuilder { limit_page_numbers: bool, enable_pages: bool, enable_seek: bool, + enable_seek_backward: bool, } impl PaginationOptionsBuilder { @@ -103,6 +116,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 +162,18 @@ 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", + )); + } + 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")); + } + false => Page::Seek(RawSeekPayload(s)), } - - Page::Seek(RawSeekPayload(s)) } else { Page::Unspecified }; @@ -209,7 +235,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) } @@ -217,7 +243,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) @@ -228,14 +256,21 @@ impl Paginated { F: Fn(&T) -> S, S: Serialize, { - if self.is_explicit_page() || self.records_and_total.len() < self.options.per_page as usize + // 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.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::Unspecified | Page::Seek(_) => { + 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)?); } @@ -244,8 +279,33 @@ impl Paginated { Ok(Some(opts)) } - fn is_explicit_page(&self) -> bool { - matches!(&self.options.page, Page::Numeric(_)) + 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 { @@ -284,7 +344,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?; + + // This maintain consistent ordering from page to pagen + if options.is_backward() { + records_and_total.reverse(); + } Ok(Paginated { records_and_total, @@ -327,6 +392,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. @@ -432,7 +501,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?; + + // This maintain consistent ordering from page to pagen + if options.is_backward() { + records_and_total.reverse(); + } Ok(Paginated { records_and_total, @@ -522,9 +596,11 @@ macro_rules! seek { use crate::util::errors::AppResult; use crate::controllers::helpers::pagination::Page; impl $name { - pub fn after(&self, page: &Page) -> AppResult]>> { - let Page::Seek(ref encoded) = *page else { - return Ok(None); + pub fn decode(&self, page: &Page) -> AppResult]>> { + let encoded = match page { + Page::Seek(encoded) if !encoded.is_empty() => encoded, + Page::SeekBackward(encoded) if !encoded.is_empty() => encoded, + _ => return Ok(None), }; Ok(Some(match self { @@ -603,6 +679,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] @@ -615,6 +709,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] @@ -665,20 +766,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.after(&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() @@ -687,29 +795,59 @@ 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.after(&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); + } + + // 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)] diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 621c005a4af..61b6bd4fc13 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; @@ -116,14 +116,19 @@ 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) + .enable_seek_backward(true) + .gather(&req)?; + let is_forward = !pagination.is_backward(); + 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 +144,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 +161,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,41 +177,57 @@ 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 // 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 - .after(&pagination.page)? - .map(|s| filter_params.seek_after(&s)) + .decode(&pagination.page)? + .map(|s| filter_params.seek(&s, is_forward)) { query = query.filter(condition); } @@ -216,7 +246,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 { @@ -475,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); @@ -487,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 @@ -559,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() @@ -567,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"), @@ -622,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()), + ] + } } }; diff --git a/src/controllers/krate/versions.rs b/src/controllers/krate/versions.rs index 5b665e5d71c..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.after(&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( @@ -297,7 +293,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()) @@ -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); } @@ -432,7 +428,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)) } 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, diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 6f5d09d33fc..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,43 +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); - } - - 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(()) } @@ -1248,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() { @@ -1262,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());