From 74f395f5c5d90480afb3a8784db47552351bbaca Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 22 Jan 2024 00:50:07 +0800 Subject: [PATCH 1/6] Refactor pagination to prepare for expanding the scope of seek-based pagination on search --- src/controllers/helpers/pagination.rs | 25 ++++++++++++++++++++ src/controllers/krate/search.rs | 34 +++++++++++---------------- src/tests/routes/crates/list.rs | 3 ++- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 327b4bc3186..9817a0e7203 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -198,6 +198,31 @@ impl Paginated { Some(opts) } + pub(crate) fn next_seek_params(&self, f: F) -> AppResult>> + where + F: Fn(&T) -> S, + S: Serialize, + { + if self.is_explicit_page() || self.records_and_total.len() < self.options.per_page as usize + { + return Ok(None); + } + + let mut opts = IndexMap::new(); + match self.options.page { + Page::Unspecified | Page::Seek(_) => { + let seek = f(&self.records_and_total.last().unwrap().record); + opts.insert("seek".into(), encode_seek(seek)?); + } + Page::Numeric(_) => unreachable!(), + }; + 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 afcdba826a9..ea01bb544d8 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -4,7 +4,6 @@ use crate::auth::AuthCheck; use diesel::dsl::*; use diesel::sql_types::{Array, Text}; use diesel_full_text_search::*; -use indexmap::IndexMap; use once_cell::sync::OnceCell; use crate::controllers::cargo_prelude::*; @@ -149,7 +148,6 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { let (total, next_page, prev_page, data, conn) = if supports_seek && !explicit_page { // Equivalent of: // `WHERE name > (SELECT name FROM crates WHERE id = $1) LIMIT $2` - query = query.limit(pagination.per_page); if let Some(seek) = seek { let crate_name: String = crates::table .find(seek) @@ -164,26 +162,22 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { // // If this becomes a problem in the future the crates count could be denormalized, at least // for the filterless happy path. - let count_query = filter_params.make_query(&req, conn)?.count(); - let total: i64 = info_span!("db.query", message = "SELECT COUNT(*) FROM crates") - .in_scope(|| count_query.get_result(conn))?; - - let results: Vec<(Crate, bool, Option)> = - info_span!("db.query", message = "SELECT ... FROM crates") + let query = query.pages_pagination_with_count_query( + pagination, + filter_params.make_query(&req, conn)?.count(), + ); + let data: Paginated<(Crate, bool, Option)> = + info_span!("db.query", message = "SELECT ..., COUNT(*) FROM crates") .in_scope(|| query.load(conn))?; - let next_page = if let Some(last) = results.last() { - let mut params = IndexMap::new(); - params.insert( - "seek".into(), - crate::controllers::helpers::pagination::encode_seek(last.0.id)?, - ); - Some(req.query_with_params(params)) - } else { - None - }; - - (total, next_page, None, results, conn) + ( + data.total(), + data.next_seek_params(|last| last.0.id)? + .map(|p| req.query_with_params(p)), + None, + data.into_iter().collect::>(), + conn, + ) } else { let query = query.pages_pagination_with_count_query( pagination, diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index f370b50ed4e..8369d34d22e 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -794,12 +794,13 @@ fn seek_based_pagination() { 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); } else { assert_that!(resp.crates, empty()); + assert_eq!(resp.meta.total, 0); } assert_eq!(resp.meta.prev_page, None); - assert_eq!(resp.meta.total, 3); } assert_eq!(calls, 4); From e536a39a51043b9d8545ce0d7fa8b354923d9038 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 22 Jan 2024 13:16:36 +0800 Subject: [PATCH 2/6] controllers/helpers/pagination: Add `seek!` helper macro --- Cargo.lock | 7 ++ Cargo.toml | 1 + src/controllers/helpers/pagination.rs | 146 ++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 14fc59ed1a4..56640ce4c2c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1018,6 +1018,7 @@ dependencies = [ "once_cell", "p256", "parking_lot", + "paste", "prometheus", "rand", "regex", @@ -3051,6 +3052,12 @@ dependencies = [ "windows-targets 0.48.5", ] +[[package]] +name = "paste" +version = "1.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "de3145af08024dea9fa9914f381a17b8fc6034dfb00f3a84013f7ff43f29ed4c" + [[package]] name = "pem-rfc7468" version = "0.7.0" diff --git a/Cargo.toml b/Cargo.toml index 7bfaf0c69aa..e8eda828094 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -96,6 +96,7 @@ object_store = { version = "=0.9.0", features = ["aws"] } once_cell = "=1.19.0" p256 = "=0.13.2" parking_lot = "=0.12.1" +paste = "=1.0.14" prometheus = { version = "=0.13.3", default-features = false } rand = "=0.8.5" reqwest = { version = "=0.11.24", features = ["gzip", "json"] } diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 9817a0e7203..4c91289de45 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -397,6 +397,73 @@ impl PaginatedQueryWithCountSubq { } } +macro_rules! seek { + ( + $vis:vis enum $name:ident { + $( + $variant:ident($($(#[$field_meta:meta])? $ty:ty),*) + )* + } + ) => { + paste::item! { + $( + #[derive(Debug, Default, Deserialize, Serialize, PartialEq)] + $vis struct $variant($($(#[$field_meta])? pub(super) $ty),*); + )* + + #[derive(Debug, Deserialize, Serialize, PartialEq)] + #[serde(untagged)] + $vis enum [<$name Payload>] { + $( + $variant($variant), + )* + } + + #[derive(Debug, PartialEq)] + $vis enum $name { + $( + $variant, + )* + } + + $( + impl From<$variant> for [<$name Payload>] { + fn from(value: $variant) -> Self { + [<$name Payload>]::$variant(value) + } + } + )* + impl From<[<$name Payload>]> for $name { + fn from(value: [<$name Payload>]) -> Self { + match value { + $( + [<$name Payload>]::$variant(_) => $name::$variant, + )* + } + } + } + + 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); + }; + + Ok(Some(match self { + $( + $name::$variant => encoded.decode::<$variant>()?.into(), + )* + })) + } + } + } + }; +} + +pub(crate) use seek; + #[cfg(test)] mod tests { use super::*; @@ -501,6 +568,85 @@ mod tests { ); } + mod seek { + use chrono::naive::serde::ts_microseconds; + seek! { + pub(super) enum Seek { + Id(i32) + New(#[serde(with="ts_microseconds")] chrono::NaiveDateTime, i32) + RecentDownloads(Option, i32) + } + } + } + + #[test] + fn test_seek_macro_encode_and_decode() { + use chrono::{NaiveDate, NaiveDateTime}; + 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 seek = Seek::Id; + let payload = SeekPayload::Id(Id(1234)); + let query = format!("seek={}", encode_seek(&payload).unwrap()); + assert_decode_after(seek, &query, Some(payload)); + + let dt: NaiveDateTime = NaiveDate::from_ymd_opt(2016, 7, 8) + .unwrap() + .and_hms_opt(9, 10, 11) + .unwrap(); + let seek = Seek::New; + let payload = SeekPayload::New(New(dt, 1234)); + let query = format!("seek={}", encode_seek(&payload).unwrap()); + assert_decode_after(seek, &query, Some(payload)); + + let seek = Seek::RecentDownloads; + let payload = SeekPayload::RecentDownloads(RecentDownloads(Some(5678), 1234)); + let query = format!("seek={}", encode_seek(&payload).unwrap()); + assert_decode_after(seek, &query, Some(payload)); + + let seek = Seek::Id; + assert_decode_after(seek, "", None); + + let seek = Seek::Id; + let payload = SeekPayload::RecentDownloads(RecentDownloads(Some(5678), 1234)); + 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); + } + + #[test] + fn test_seek_macro_conv() { + use chrono::{NaiveDate, NaiveDateTime}; + use seek::*; + + assert_eq!(Seek::from(SeekPayload::Id(Id(1234))), Seek::Id); + + let dt: NaiveDateTime = NaiveDate::from_ymd_opt(2016, 7, 8) + .unwrap() + .and_hms_opt(9, 10, 11) + .unwrap(); + assert_eq!(Seek::from(SeekPayload::New(New(dt, 1234))), Seek::New); + + assert_eq!( + Seek::from(SeekPayload::RecentDownloads(RecentDownloads(None, 1234))), + Seek::RecentDownloads + ); + } + fn mock(query: &str) -> Request<()> { Request::builder() .method(Method::GET) From 7b8174e7bec9cfe89fcc61f0942e99c83f7470ed Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 22 Jan 2024 13:27:04 +0800 Subject: [PATCH 3/6] controllers/krate/search: Expand the scope of forward seek-based pagination --- src/controllers/krate/search.rs | 309 +++++++++++++++++++++++++------- src/tests/routes/crates/list.rs | 9 +- 2 files changed, 250 insertions(+), 68 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index ea01bb544d8..be609e1b2f8 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -2,7 +2,7 @@ use crate::auth::AuthCheck; use diesel::dsl::*; -use diesel::sql_types::{Array, Text}; +use diesel::sql_types::{Array, Bool, Text}; use diesel_full_text_search::*; use once_cell::sync::OnceCell; @@ -40,7 +40,8 @@ use crate::sql::{array_agg, canon_crate_name, lower}; /// for them. pub async fn search(app: AppState, req: Parts) -> AppResult> { spawn_blocking(move || { - use diesel::sql_types::Bool; + use diesel::sql_types::Float; + use seek::*; let params = req.query(); let option_param = |s| params.get(s).map(|v| v.as_str()); @@ -72,28 +73,20 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { ALL_COLUMNS, false.into_sql::(), recent_crate_downloads::downloads.nullable(), + 0_f32.into_sql::(), ); let conn = &mut *app.db_read()?; - let mut supports_seek = filter_params.supports_seek(); + let mut seek: Option = None; let mut query = filter_params .make_query(&req, conn)? .left_join(recent_crate_downloads::table) .select(selection); if let Some(q_string) = &q_string { - // Searching with a query string always puts the exact match at the start of the results, - // so we can't support seek-based pagination with it. - supports_seek = false; - if !q_string.is_empty() { let sort = sort.unwrap_or("relevance"); - query = query.select(( - ALL_COLUMNS, - Crate::with_name(q_string), - recent_crate_downloads::downloads.nullable(), - )); query = query.order(Crate::with_name(q_string).desc()); if sort == "relevance" { @@ -101,59 +94,70 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { .bind::(q_string) .sql(")"); let rank = ts_rank_cd(crates::textsearchable_index_col, q); + query = query.select(( + ALL_COLUMNS, + Crate::with_name(q_string), + recent_crate_downloads::downloads.nullable(), + rank.clone(), + )); + seek = Some(Seek::Relevance); query = query.then_order_by(rank.desc()) + } else { + query = query.select(( + ALL_COLUMNS, + Crate::with_name(q_string), + recent_crate_downloads::downloads.nullable(), + 0_f32.into_sql::(), + )); + seek = Some(Seek::Query); } } } // Any sort other than 'relevance' (default) would ignore exact crate name matches + // Seek-based pagination requires a unique ordering to avoid unexpected row skipping + // during pagination. + // Therefore, when the ordering isn't unique an auxiliary ordering column should be added + // to ensure predictable pagination behavior. if sort == Some("downloads") { - // Custom sorting is not supported yet with seek. - supports_seek = false; - - query = query.order(crates::downloads.desc()) + seek = Some(Seek::Downloads); + query = query.order((crates::downloads.desc(), crates::id.asc())) } else if sort == Some("recent-downloads") { - // Custom sorting is not supported yet with seek. - supports_seek = false; - - query = query.order(recent_crate_downloads::downloads.desc().nulls_last()) + seek = Some(Seek::RecentDownloads); + query = query.order(( + recent_crate_downloads::downloads.desc().nulls_last(), + crates::id.asc(), + )) } else if sort == Some("recent-updates") { - // Custom sorting is not supported yet with seek. - supports_seek = false; - - query = query.order(crates::updated_at.desc()); + seek = Some(Seek::RecentUpdates); + query = query.order((crates::updated_at.desc(), crates::id.asc())); } else if sort == Some("new") { - // Custom sorting is not supported yet with seek. - supports_seek = false; - - query = query.order(crates::created_at.desc()); + seek = Some(Seek::New); + query = query.order((crates::created_at.desc(), 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()) } let pagination: PaginationOptions = PaginationOptions::builder() .limit_page_numbers() - .enable_seek(supports_seek) + .enable_seek(true) .gather(&req)?; - let (explicit_page, seek) = match pagination.page { - Page::Numeric(_) => (true, None), - Page::Seek(ref s) => (false, Some(s.decode::()?)), - Page::Unspecified => (false, None), - }; + 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, conn) = if supports_seek && !explicit_page { - // Equivalent of: - // `WHERE name > (SELECT name FROM crates WHERE id = $1) LIMIT $2` - if let Some(seek) = seek { - let crate_name: String = crates::table - .find(seek) - .select(crates::name) - .get_result(conn)?; - query = query.filter(crates::name.gt(crate_name)); + let (total, next_page, prev_page, data, conn) = if !explicit_page && seek.is_some() { + let seek = seek.unwrap(); + if let Some(condition) = seek + .after(&pagination.page)? + .map(|s| filter_params.seek_after(&s)) + { + query = query.filter(condition); } // This does a full index-only scan over the crates table to gather how many crates were @@ -166,13 +170,13 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { pagination, filter_params.make_query(&req, conn)?.count(), ); - let data: Paginated<(Crate, bool, Option)> = + let data: Paginated<(Crate, bool, Option, f32)> = info_span!("db.query", message = "SELECT ..., COUNT(*) FROM crates") .in_scope(|| query.load(conn))?; ( data.total(), - data.next_seek_params(|last| last.0.id)? + data.next_seek_params(|last| seek.to_payload(last))? .map(|p| req.query_with_params(p)), None, data.into_iter().collect::>(), @@ -183,7 +187,7 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { pagination, filter_params.make_query(&req, conn)?.count(), ); - let data: Paginated<(Crate, bool, Option)> = + let data: Paginated<(Crate, bool, Option, f32)> = info_span!("db.query", message = "SELECT ..., COUNT(*) FROM crates") .in_scope(|| query.load(conn))?; ( @@ -195,12 +199,12 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { ) }; - let perfect_matches = data.iter().map(|&(_, b, _)| b).collect::>(); + let perfect_matches = data.iter().map(|&(_, b, _, _)| b).collect::>(); let recent_downloads = data .iter() - .map(|&(_, _, s)| s.unwrap_or(0)) + .map(|&(_, _, s, _)| s.unwrap_or(0)) .collect::>(); - let crates = data.into_iter().map(|(c, _, _)| c).collect::>(); + let crates = data.into_iter().map(|(c, _, _, _)| c).collect::>(); let versions: Vec = info_span!("db.query", message = "SELECT ... FROM versions") .in_scope(|| crates.versions().load(conn))?; @@ -279,20 +283,6 @@ impl<'a> FilterParams<'a> { }) } - fn supports_seek(&self) -> bool { - // Calculating the total number of results with filters is supported but paging is not supported yet. - !(self.q_string.is_some() - || self.category.is_some() - || self.all_keywords.is_some() - || self.keyword.is_some() - || self.letter.is_some() - || self.user_id.is_some() - || self.team_id.is_some() - || self.following - || self.has_ids - || !self.include_yanked) - } - fn make_query( &'a self, req: &Parts, @@ -404,6 +394,199 @@ impl<'a> FilterParams<'a> { Ok(query) } + + fn seek_after(&self, seek_payload: &seek::SeekPayload) -> BoxedCondition<'a> { + use seek::*; + + let crates_aliased = alias!(crates as crates_aliased); + let crate_name_by_id = |id: i32| { + crates_aliased + .find(id) + .select(crates_aliased.field(crates::name)) + .single_value() + }; + let conditions: Vec> = match *seek_payload { + SeekPayload::Name(Name(id)) => { + // Equivalent of: + // `WHERE name > name'` + vec![Box::new(crates::name.nullable().gt(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'` + vec![ + Box::new( + crates::created_at + .eq(created_at) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crates::created_at.lt(created_at).nullable()), + ] + } + SeekPayload::RecentUpdates(RecentUpdates(updated_at, id)) => { + // Equivalent of: + // `WHERE (updated_at = updated_at' AND id > id') OR updated_at < updated_at'` + vec![ + Box::new( + crates::updated_at + .eq(updated_at) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crates::updated_at.lt(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)` + // for recent_downloads is None: + // `WHERE (recent_downloads IS NULL AND id > id')` + match recent_downloads { + Some(dl) => { + vec![ + Box::new( + recent_crate_downloads::downloads + .eq(dl) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new( + recent_crate_downloads::downloads + .lt(dl) + .or(recent_crate_downloads::downloads.is_null()) + .nullable(), + ), + ] + } + None => { + vec![Box::new( + recent_crate_downloads::downloads + .is_null() + .and(crates::id.gt(id)) + .nullable(), + )] + } + } + } + SeekPayload::Downloads(Downloads(downloads, id)) => { + // Equivalent of: + // `WHERE (downloads = downloads' AND id > id') OR downloads < downloads'` + vec![ + Box::new( + crates::downloads + .eq(downloads) + .and(crates::id.gt(id)) + .nullable(), + ), + Box::new(crates::downloads.lt(downloads).nullable()), + ] + } + SeekPayload::Query(Query(exact_match, id)) => { + // Equivalent of: + // `WHERE (exact_match = exact_match' AND name > name') OR exact_match < + // exact_match'` + let q_string = self.q_string.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()), + ] + } + SeekPayload::Relevance(Relevance(exact, 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'` + let q_string = self.q_string.expect("q_string should not be None"); + let q = to_tsquery_with_search_config( + configuration::TsConfigurationByName("english"), + q_string, + ); + let rank = ts_rank_cd(crates::textsearchable_index_col, q); + let name_exact_match = Crate::with_name(q_string); + 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()), + ] + } + }; + + conditions + .into_iter() + .fold( + None, + |merged_condition: Option>, condition| { + Some(match merged_condition { + Some(merged) => Box::new(merged.or(condition)), + None => condition, + }) + }, + ) + .expect("should be a reduced BoxedCondition") + } +} + +mod seek { + use crate::controllers::helpers::pagination::seek; + use crate::models::Crate; + use chrono::naive::serde::ts_microseconds; + + seek! { + pub enum Seek { + Name(i32) + New(#[serde(with="ts_microseconds")] chrono::NaiveDateTime, i32) + RecentUpdates(#[serde(with="ts_microseconds")] chrono::NaiveDateTime, i32) + RecentDownloads(Option, i32) + Downloads(i32, i32) + Query(bool, i32) + Relevance(bool, f32, i32) + } + } + + impl Seek { + pub(crate) fn to_payload(&self, record: &(Crate, bool, Option, f32)) -> SeekPayload { + match *self { + Seek::Name => SeekPayload::Name(Name(record.0.id)), + Seek::New => SeekPayload::New(New(record.0.created_at, record.0.id)), + Seek::RecentUpdates => { + SeekPayload::RecentUpdates(RecentUpdates(record.0.updated_at, record.0.id)) + } + Seek::RecentDownloads => { + SeekPayload::RecentDownloads(RecentDownloads(record.2, record.0.id)) + } + Seek::Downloads => { + SeekPayload::Downloads(Downloads(record.0.downloads, record.0.id)) + } + Seek::Query => SeekPayload::Query(Query(record.1, record.0.id)), + Seek::Relevance => { + SeekPayload::Relevance(Relevance(record.1, record.3, record.0.id)) + } + } + } + } } +type BoxedCondition<'a> = Box< + dyn BoxableExpression< + LeftJoinQuerySource, + diesel::pg::Pg, + SqlType = diesel::sql_types::Nullable, + > + 'a, +>; + diesel::infix_operator!(Contains, "@>"); diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 8369d34d22e..777cf8820a8 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -732,17 +732,16 @@ fn pagination_links_included_if_applicable() { CrateBuilder::new("pagination_links_3", user.id).expect_build(conn); }); - // This uses a filter (`letter=p`) to disable seek-based pagination, as seek-based pagination - // does not return page numbers. If the test fails after expanding the scope of seek-based - // pagination replace the filter with something else still using pages. + // This uses a filter (`page=n`) to disable seek-based pagination, as seek-based pagination + // does not return page numbers. - let page1 = anon.search("letter=p&per_page=1"); + let page1 = anon.search("letter=p&page=1&per_page=1"); let page2 = anon.search("letter=p&page=2&per_page=1"); let page3 = anon.search("letter=p&page=3&per_page=1"); let page4 = anon.search("letter=p&page=4&per_page=1"); assert_eq!( - Some("?letter=p&per_page=1&page=2".to_string()), + Some("?letter=p&page=2&per_page=1".to_string()), page1.meta.next_page ); assert_eq!(None, page1.meta.prev_page); From c58e3e189dae906a02eb8b118146f92db7cdf721 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 22 Jan 2024 16:47:57 +0800 Subject: [PATCH 4/6] tests/routes/crates/list: Assert with both pagination strategies in the test suite --- src/tests/routes/crates/list.rs | 628 ++++++++++++++++++++------------ 1 file changed, 395 insertions(+), 233 deletions(-) diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 777cf8820a8..fe4e2a87194 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -7,13 +7,16 @@ use diesel::{dsl::*, prelude::*, update}; use googletest::prelude::*; use http::StatusCode; use insta::assert_json_snapshot; +use once_cell::sync::Lazy; +use regex::Regex; #[test] fn index() { let (app, anon) = TestApp::init().empty(); - let json = anon.search(""); - assert_eq!(json.crates.len(), 0); - assert_eq!(json.meta.total, 0); + for json in search_both(&anon, "") { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } let krate = app.db(|conn| { let u = new_user("foo") @@ -22,11 +25,12 @@ fn index() { CrateBuilder::new("fooindex", u.id).expect_build(conn) }); - let json = anon.search(""); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); - assert_eq!(json.crates[0].name, krate.name); - assert_eq!(json.crates[0].id, krate.name); + for json in search_both(&anon, "") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + assert_eq!(json.crates[0].name, krate.name); + assert_eq!(json.crates[0].id, krate.name); + } } #[test] @@ -57,64 +61,97 @@ fn index_queries() { (krate, krate2) }); - assert_eq!(anon.search("q=baz").meta.total, 0); + for json in search_both(&anon, "q=baz") { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } // All of these fields should be indexed/searched by the queries - assert_eq!(anon.search("q=foo").meta.total, 2); - assert_eq!(anon.search("q=kw1").meta.total, 3); - assert_eq!(anon.search("q=readme").meta.total, 1); - assert_eq!(anon.search("q=description").meta.total, 1); + for json in search_both(&anon, "q=foo") { + assert_eq!(json.crates.len(), 2); + assert_eq!(json.meta.total, 2); + } + + for json in search_both(&anon, "q=kw1") { + assert_eq!(json.crates.len(), 3); + assert_eq!(json.meta.total, 3); + } + + for json in search_both(&anon, "q=readme") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } + + for json in search_both(&anon, "q=description") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } // Query containing a space - assert_eq!(anon.search("q=foo%20kw3").meta.total, 1); + for json in search_both(&anon, "q=foo%20kw3") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } - let json = anon.search_by_user_id(user.id); - assert_eq!(json.crates.len(), 4); - assert_eq!(json.meta.total, 4); + for json in search_both_by_user_id(&anon, user.id) { + assert_eq!(json.crates.len(), 4); + assert_eq!(json.meta.total, 4); + } - let json = anon.search_by_user_id(0); - assert_eq!(json.crates.len(), 0); - assert_eq!(json.meta.total, 0); + for json in search_both_by_user_id(&anon, 0) { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } - let json = anon.search("letter=F"); - assert_eq!(json.crates.len(), 2); - assert_eq!(json.meta.total, 2); + for json in search_both(&anon, "letter=F") { + assert_eq!(json.crates.len(), 2); + assert_eq!(json.meta.total, 2); + } - let json = anon.search("letter=B"); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); + for json in search_both(&anon, "letter=B") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } - let json = anon.search("letter=b"); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); + for json in search_both(&anon, "letter=b") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } - let json = anon.search("letter=c"); - assert_eq!(json.crates.len(), 0); - assert_eq!(json.meta.total, 0); + for json in search_both(&anon, "letter=c") { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } - let json = anon.search("keyword=kw1"); - assert_eq!(json.crates.len(), 3); - assert_eq!(json.meta.total, 3); + for json in search_both(&anon, "keyword=kw1") { + assert_eq!(json.crates.len(), 3); + assert_eq!(json.meta.total, 3); + } - let json = anon.search("keyword=KW1"); - assert_eq!(json.crates.len(), 3); - assert_eq!(json.meta.total, 3); + for json in search_both(&anon, "keyword=KW1") { + assert_eq!(json.crates.len(), 3); + assert_eq!(json.meta.total, 3); + } - let json = anon.search("keyword=kw2"); - assert_eq!(json.crates.len(), 0); - assert_eq!(json.meta.total, 0); + for json in search_both(&anon, "keyword=kw2") { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } - let json = anon.search("all_keywords=kw1%20kw3"); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); + for json in search_both(&anon, "all_keywords=kw1%20kw3") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } - let json = anon.search("q=foo&keyword=kw1"); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); + for json in search_both(&anon, "q=foo&keyword=kw1") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } - let json = anon.search("q=foo2&keyword=kw1"); - assert_eq!(json.crates.len(), 0); - assert_eq!(json.meta.total, 0); + for json in search_both(&anon, "q=foo2&keyword=kw1") { + assert_eq!(json.crates.len(), 0); + assert_eq!(json.meta.total, 0); + } app.db(|conn| { new_category("Category 1", "cat1", "Category 1 crates") @@ -127,32 +164,40 @@ fn index_queries() { Category::update_crate(conn, &krate2, &["cat1::bar"]).unwrap(); }); - let cl = anon.search("category=cat1"); - assert_eq!(cl.crates.len(), 2); - assert_eq!(cl.meta.total, 2); + for cl in search_both(&anon, "category=cat1") { + assert_eq!(cl.crates.len(), 2); + assert_eq!(cl.meta.total, 2); + } - let cl = anon.search("category=cat1::bar"); - assert_eq!(cl.crates.len(), 1); - assert_eq!(cl.meta.total, 1); + for cl in search_both(&anon, "category=cat1::bar") { + assert_eq!(cl.crates.len(), 1); + assert_eq!(cl.meta.total, 1); + } - let cl = anon.search("keyword=cat2"); - assert_eq!(cl.crates.len(), 0); - assert_eq!(cl.meta.total, 0); + for cl in search_both(&anon, "keyword=cat2") { + assert_eq!(cl.crates.len(), 0); + assert_eq!(cl.meta.total, 0); + } - let cl = anon.search("q=readme&category=cat1"); - assert_eq!(cl.crates.len(), 1); - assert_eq!(cl.meta.total, 1); + for cl in search_both(&anon, "q=readme&category=cat1") { + assert_eq!(cl.crates.len(), 1); + assert_eq!(cl.meta.total, 1); + } - let cl = anon.search("keyword=kw1&category=cat1"); - assert_eq!(cl.crates.len(), 2); - assert_eq!(cl.meta.total, 2); + for cl in search_both(&anon, "keyword=kw1&category=cat1") { + assert_eq!(cl.crates.len(), 2); + assert_eq!(cl.meta.total, 2); + } - let cl = anon.search("keyword=kw3&category=cat1"); - assert_eq!(cl.crates.len(), 0); - assert_eq!(cl.meta.total, 0); + for cl in search_both(&anon, "keyword=kw3&category=cat1") { + assert_eq!(cl.crates.len(), 0); + assert_eq!(cl.meta.total, 0); + } // ignores 0x00 characters that Postgres does not support - assert_eq!(anon.search("q=k%00w1").meta.total, 3); + for cl in search_both(&anon, "q=k%00w1") { + assert_eq!(cl.meta.total, 3); + } } #[test] @@ -165,9 +210,10 @@ fn search_includes_crates_where_name_is_stopword() { .readme("crate which does things") .expect_build(conn); }); - let json = anon.search("q=which"); - assert_eq!(json.crates.len(), 1); - assert_eq!(json.meta.total, 1); + for json in search_both(&anon, "q=which") { + assert_eq!(json.crates.len(), 1); + assert_eq!(json.meta.total, 1); + } } #[test] @@ -193,23 +239,26 @@ fn exact_match_first_on_queries() { .expect_build(conn); }); - let json = anon.search("q=foo-exact"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "foo_exact"); - assert_eq!(json.crates[1].name, "baz_exact"); - assert_eq!(json.crates[2].name, "bar-exact"); - - let json = anon.search("q=bar_exact"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "bar-exact"); - assert_eq!(json.crates[1].name, "baz_exact"); - assert_eq!(json.crates[2].name, "foo_exact"); - - let json = anon.search("q=baz_exact"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "baz_exact"); - assert_eq!(json.crates[1].name, "bar-exact"); - assert_eq!(json.crates[2].name, "foo_exact"); + for json in search_both(&anon, "q=foo-exact") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "foo_exact"); + assert_eq!(json.crates[1].name, "baz_exact"); + assert_eq!(json.crates[2].name, "bar-exact"); + } + + for json in search_both(&anon, "q=bar_exact") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "bar-exact"); + assert_eq!(json.crates[1].name, "baz_exact"); + assert_eq!(json.crates[2].name, "foo_exact"); + } + + for json in search_both(&anon, "q=baz_exact") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "baz_exact"); + assert_eq!(json.crates[1].name, "bar-exact"); + assert_eq!(json.crates[2].name, "foo_exact"); + } } #[test] @@ -280,45 +329,85 @@ fn index_sorting() { }); // Sort by downloads - let json = anon.search("sort=downloads"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "foo_sort"); + for json in search_both(&anon, "sort=downloads") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "other_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + 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"); + 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); // Sort by recent-downloads - let json = anon.search("sort=recent-downloads"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "other_sort"); + for json in search_both(&anon, "sort=recent-downloads") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "foo_sort"); + assert_eq!(json.crates[1].name, "baz_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"); + 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[3].crates[0].name, "other_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Sort by recent-updates - let json = anon.search("sort=recent-updates"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "foo_sort"); + for json in search_both(&anon, "sort=recent-updates") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "other_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + 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"); + 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); // Sort by new - let json = anon.search("sort=new"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "bar_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "other_sort"); - assert_eq!(json.crates[3].name, "foo_sort"); + for json in search_both(&anon, "sort=new") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "bar_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[2].name, "other_sort"); + assert_eq!(json.crates[3].name, "foo_sort"); + } + let (resp, calls) = page_with_seek(&anon, "sort=new"); + assert_eq!(resp[0].crates[0].name, "bar_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, "foo_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); // Test for bug with showing null results first when sorting // by descending downloads - let json = anon.search("sort=recent-downloads"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "other_sort"); + for json in search_both(&anon, "sort=recent-downloads") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "foo_sort"); + assert_eq!(json.crates[1].name, "baz_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"); + 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[3].crates[0].name, "other_sort"); + assert_eq!(resp[3].meta.total, 4); + assert_eq!(calls, 5); } #[test] @@ -389,60 +478,68 @@ fn ignore_exact_match_on_queries_with_sort() { }); // Sort by downloads, order always the same no matter the crate name query - let json = anon.search("q=foo_sort&sort=downloads"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "baz_sort"); - assert_eq!(json.crates[1].name, "bar_sort"); - assert_eq!(json.crates[2].name, "foo_sort"); - - let json = anon.search("q=bar_sort&sort=downloads"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "baz_sort"); - assert_eq!(json.crates[1].name, "bar_sort"); - assert_eq!(json.crates[2].name, "foo_sort"); - - let json = anon.search("q=baz_sort&sort=downloads"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "baz_sort"); - assert_eq!(json.crates[1].name, "bar_sort"); - assert_eq!(json.crates[2].name, "foo_sort"); - - let json = anon.search("q=const&sort=downloads"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "foo_sort"); + for json in search_both(&anon, "q=foo_sort&sort=downloads") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "baz_sort"); + assert_eq!(json.crates[1].name, "bar_sort"); + assert_eq!(json.crates[2].name, "foo_sort"); + } + + for json in search_both(&anon, "q=bar_sort&sort=downloads") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "baz_sort"); + assert_eq!(json.crates[1].name, "bar_sort"); + assert_eq!(json.crates[2].name, "foo_sort"); + } + + for json in search_both(&anon, "q=baz_sort&sort=downloads") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "baz_sort"); + assert_eq!(json.crates[1].name, "bar_sort"); + assert_eq!(json.crates[2].name, "foo_sort"); + } + + for json in search_both(&anon, "q=const&sort=downloads") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "other_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[2].name, "bar_sort"); + assert_eq!(json.crates[3].name, "foo_sort"); + } // Sort by recent-downloads, order always the same no matter the crate name query - let json = anon.search("q=bar_sort&sort=recent-downloads"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); + for json in search_both(&anon, "q=bar_sort&sort=recent-downloads") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "foo_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[2].name, "bar_sort"); + } // Test for bug with showing null results first when sorting // by descending downloads - let json = anon.search("sort=recent-downloads"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); - assert_eq!(json.crates[3].name, "other_sort"); + for json in search_both(&anon, "sort=recent-downloads") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "foo_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[2].name, "bar_sort"); + assert_eq!(json.crates[3].name, "other_sort"); + } // Sort by recent-updates - let json = anon.search("q=bar_sort&sort=recent-updates"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "baz_sort"); - assert_eq!(json.crates[1].name, "bar_sort"); - assert_eq!(json.crates[2].name, "foo_sort"); + for json in search_both(&anon, "q=bar_sort&sort=recent-updates") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "baz_sort"); + assert_eq!(json.crates[1].name, "bar_sort"); + assert_eq!(json.crates[2].name, "foo_sort"); + } // Sort by new - let json = anon.search("q=bar_sort&sort=new"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "bar_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "foo_sort"); + for json in search_both(&anon, "q=bar_sort&sort=new") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "bar_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[2].name, "foo_sort"); + } } #[test] @@ -457,12 +554,15 @@ fn multiple_ids() { CrateBuilder::new("other", user.id).expect_build(conn); }); - let json = - anon.search("ids%5B%5D=foo&ids%5B%5D=bar&ids%5B%5D=baz&ids%5B%5D=baz&ids%5B%5D=unknown"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "bar"); - assert_eq!(json.crates[1].name, "baz"); - assert_eq!(json.crates[2].name, "foo"); + for json in search_both( + &anon, + "ids%5B%5D=foo&ids%5B%5D=bar&ids%5B%5D=baz&ids%5B%5D=baz&ids%5B%5D=unknown", + ) { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "bar"); + assert_eq!(json.crates[1].name, "baz"); + assert_eq!(json.crates[2].name, "foo"); + } } #[test] @@ -497,16 +597,18 @@ fn loose_search_order() { .expect_build(conn); vec![one, two, three, four] }); - let search_temp = anon.search("q=temp"); - assert_eq!(search_temp.meta.total, 4); - assert_eq!(search_temp.crates.len(), 4); - for (lhs, rhs) in search_temp.crates.iter().zip(ordered) { - assert_eq!(lhs.name, rhs.name); + for search_temp in search_both(&anon, "q=temp") { + assert_eq!(search_temp.meta.total, 4); + assert_eq!(search_temp.crates.len(), 4); + for (lhs, rhs) in search_temp.crates.iter().zip(&ordered) { + assert_eq!(lhs.name, rhs.name); + } } - let search_temp = anon.search("q=te"); - assert_eq!(search_temp.meta.total, 3); - assert_eq!(search_temp.crates.len(), 3); + for search_temp in search_both(&anon, "q=te") { + assert_eq!(search_temp.meta.total, 3); + assert_eq!(search_temp.crates.len(), 3); + } } #[test] @@ -537,19 +639,21 @@ fn index_include_yanked() { }); // Include fully yanked (all versions were yanked) crates - let json = anon.search("include_yanked=yes&sort=alphabetical"); - assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "all_yanked"); - assert_eq!(json.crates[1].name, "newest_yanked"); - assert_eq!(json.crates[2].name, "oldest_yanked"); - assert_eq!(json.crates[3].name, "unyanked"); + for json in search_both(&anon, "include_yanked=yes&sort=alphabetical") { + assert_eq!(json.meta.total, 4); + assert_eq!(json.crates[0].name, "all_yanked"); + assert_eq!(json.crates[1].name, "newest_yanked"); + assert_eq!(json.crates[2].name, "oldest_yanked"); + assert_eq!(json.crates[3].name, "unyanked"); + } // Do not include fully yanked (all versions were yanked) crates - let json = anon.search("include_yanked=no&sort=alphabetical"); - assert_eq!(json.meta.total, 3); - assert_eq!(json.crates[0].name, "newest_yanked"); - assert_eq!(json.crates[1].name, "oldest_yanked"); - assert_eq!(json.crates[2].name, "unyanked"); + for json in search_both(&anon, "include_yanked=no&sort=alphabetical") { + assert_eq!(json.meta.total, 3); + assert_eq!(json.crates[0].name, "newest_yanked"); + assert_eq!(json.crates[1].name, "oldest_yanked"); + assert_eq!(json.crates[2].name, "unyanked"); + } } #[test] @@ -565,9 +669,10 @@ fn yanked_versions_are_not_considered_for_max_version() { .expect_build(conn); }); - let json = anon.search("q=foo"); - assert_eq!(json.meta.total, 1); - assert_eq!(json.crates[0].max_version, "1.0.0"); + for json in search_both(&anon, "q=foo") { + assert_eq!(json.meta.total, 1); + assert_eq!(json.crates[0].max_version, "1.0.0"); + } } #[test] @@ -586,9 +691,10 @@ fn max_stable_version() { .expect_build(conn); }); - let json = anon.search("q=foo"); - assert_eq!(json.meta.total, 1); - assert_eq!(json.crates[0].max_stable_version, Some("1.0.0".to_string())); + for json in search_both(&anon, "q=foo") { + assert_eq!(json.meta.total, 1); + assert_eq!(json.crates[0].max_stable_version, Some("1.0.0".to_string())); + } } /// Given two crates, one with downloads less than 90 days ago, the @@ -617,18 +723,18 @@ fn test_recent_download_count() { .expect_build(conn); }); - let json = anon.search("sort=recent-downloads"); + for json in search_both(&anon, "sort=recent-downloads") { + assert_eq!(json.meta.total, 2); - assert_eq!(json.meta.total, 2); + assert_eq!(json.crates[0].name, "sweet_potato_snack"); + assert_eq!(json.crates[1].name, "green_ball"); - assert_eq!(json.crates[0].name, "sweet_potato_snack"); - assert_eq!(json.crates[1].name, "green_ball"); + assert_eq!(json.crates[0].recent_downloads, Some(2)); + assert_eq!(json.crates[0].downloads, 5); - assert_eq!(json.crates[0].recent_downloads, Some(2)); - assert_eq!(json.crates[0].downloads, 5); - - assert_eq!(json.crates[1].recent_downloads, Some(0)); - assert_eq!(json.crates[1].downloads, 10); + assert_eq!(json.crates[1].recent_downloads, Some(0)); + assert_eq!(json.crates[1].downloads, 10); + } } /// Given one crate with zero downloads, check that the crate @@ -648,11 +754,12 @@ fn test_zero_downloads() { .expect_build(conn); }); - let json = anon.search("sort=recent-downloads"); - assert_eq!(json.meta.total, 1); - assert_eq!(json.crates[0].name, "green_ball"); - assert_eq!(json.crates[0].recent_downloads, Some(0)); - assert_eq!(json.crates[0].downloads, 0); + for json in search_both(&anon, "sort=recent-downloads") { + assert_eq!(json.meta.total, 1); + assert_eq!(json.crates[0].name, "green_ball"); + assert_eq!(json.crates[0].recent_downloads, Some(0)); + assert_eq!(json.crates[0].downloads, 0); + } } /// Given two crates, one with more all-time downloads, the other with @@ -684,18 +791,18 @@ fn test_default_sort_recent() { // test that index for keywords is sorted by recent_downloads // by default - let json = anon.search("keyword=dog"); + for json in search_both(&anon, "keyword=dog") { + assert_eq!(json.meta.total, 2); - assert_eq!(json.meta.total, 2); + assert_eq!(json.crates[0].name, "green_ball"); + assert_eq!(json.crates[1].name, "sweet_potato_snack"); - assert_eq!(json.crates[0].name, "green_ball"); - assert_eq!(json.crates[1].name, "sweet_potato_snack"); + assert_eq!(json.crates[0].recent_downloads, Some(10)); + assert_eq!(json.crates[0].downloads, 10); - assert_eq!(json.crates[0].recent_downloads, Some(10)); - assert_eq!(json.crates[0].downloads, 10); - - assert_eq!(json.crates[1].recent_downloads, Some(0)); - assert_eq!(json.crates[1].downloads, 20); + assert_eq!(json.crates[1].recent_downloads, Some(0)); + assert_eq!(json.crates[1].downloads, 20); + } app.db(|conn| { new_category("Animal", "animal", "animal crates") @@ -707,18 +814,18 @@ fn test_default_sort_recent() { // test that index for categories is sorted by recent_downloads // by default - let json = anon.search("category=animal"); - - assert_eq!(json.meta.total, 2); + for json in search_both(&anon, "category=animal") { + assert_eq!(json.meta.total, 2); - assert_eq!(json.crates[0].name, "green_ball"); - assert_eq!(json.crates[1].name, "sweet_potato_snack"); + assert_eq!(json.crates[0].name, "green_ball"); + assert_eq!(json.crates[1].name, "sweet_potato_snack"); - assert_eq!(json.crates[0].recent_downloads, Some(10)); - assert_eq!(json.crates[0].downloads, 10); + assert_eq!(json.crates[0].recent_downloads, Some(10)); + assert_eq!(json.crates[0].downloads, 10); - assert_eq!(json.crates[1].recent_downloads, Some(0)); - assert_eq!(json.crates[1].downloads, 20); + assert_eq!(json.crates[1].recent_downloads, Some(0)); + assert_eq!(json.crates[1].downloads, 20); + } } #[test] @@ -880,9 +987,10 @@ fn crates_by_user_id() { CrateBuilder::new("foo_my_packages", id).expect_build(conn); }); - let response = user.search_by_user_id(id); - assert_eq!(response.crates.len(), 1); - assert_eq!(response.meta.total, 1); + for response in search_both_by_user_id(&user, id) { + assert_eq!(response.crates.len(), 1); + assert_eq!(response.meta.total, 1); + } } #[test] @@ -895,7 +1003,61 @@ fn crates_by_user_id_not_including_deleted_owners() { krate.owner_remove(conn, "foo").unwrap(); }); - let response = anon.search_by_user_id(user.id); - assert_eq!(response.crates.len(), 0); - assert_eq!(response.meta.total, 0); + for response in search_both_by_user_id(&anon, user.id) { + assert_eq!(response.crates.len(), 0); + assert_eq!(response.meta.total, 0); + } +} + +static PAGE_RE: Lazy = Lazy::new(|| Regex::new(r"((?:^page|&page|\?page)=\d+)").unwrap()); + +// search with both offset-based (prepend with `page=1` query) and seek-based pagination +fn search_both(anon: &U, query: &str) -> [crate::CrateList; 2] { + if PAGE_RE.is_match(query) { + panic!("url already contains page param"); + } + let (offset, seek) = (anon.search(&format!("page=1&{query}")), anon.search(query)); + assert!(offset + .meta + .next_page + .as_deref() + .unwrap_or("page=2") + .contains("page=2")); + assert!(seek + .meta + .next_page + .as_deref() + .unwrap_or("seek=") + .contains("seek=")); + [offset, seek] +} + +fn search_both_by_user_id(anon: &U, id: i32) -> [crate::CrateList; 2] { + let url = format!("user_id={id}"); + search_both(anon, &url) +} + +fn page_with_seek(anon: &U, query: &str) -> (Vec, i32) { + let mut url = Some(format!("?per_page=1&{query}")); + 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('?')); + calls += 1; + if calls > 200 { + panic!("potential infinite loop detected!") + } + + if let Some(ref new_url) = resp.meta.next_page { + assert!(new_url.contains("seek=")); + assert_that!(resp.crates, len(eq(1))); + url = Some(new_url.to_owned()); + assert_ne!(resp.meta.total, 0); + } else { + assert_that!(resp.crates, empty()); + assert_eq!(resp.meta.total, 0); + } + results.push(resp); + } + (results, calls) } From 06902c012038dde4e69aa7a28eb7a0b5b3570915 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 22 Jan 2024 18:21:13 +0800 Subject: [PATCH 5/6] tests/routes/crates/list: Ensure unique ordering of seek-based pagination is correct --- src/tests/routes/crates/list.rs | 110 ++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 20 deletions(-) diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index fe4e2a87194..5d0ef34f2b9 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -267,6 +267,8 @@ fn index_sorting() { let (app, anon, user) = TestApp::init().with_user(); let user = user.as_model(); + // To test that the unique ordering of seed-based pagination is correct, we need to + // set some columns to the same value. app.db(|conn| { let krate1 = CrateBuilder::new("foo_sort", user.id) .description("bar_sort baz_sort const") @@ -283,12 +285,12 @@ fn index_sorting() { let krate3 = CrateBuilder::new("baz_sort", user.id) .description("foo_sort bar_sort foo_sort bar_sort foo_sort bar_sort const") .downloads(100_000) - .recent_downloads(10) + .recent_downloads(50) .expect_build(conn); let krate4 = CrateBuilder::new("other_sort", user.id) .description("other_sort const") - .downloads(999_999) + .downloads(100_000) .expect_build(conn); // Set the created at column for each crate @@ -300,11 +302,7 @@ fn index_sorting() { .set(crates::created_at.eq(now - 1.weeks())) .execute(conn) .unwrap(); - update(&krate3) - .set(crates::created_at.eq(now - 2.weeks())) - .execute(conn) - .unwrap(); - update(&krate4) + update(crates::table.filter(crates::id.eq_any(vec![krate3.id, krate4.id]))) .set(crates::created_at.eq(now - 3.weeks())) .execute(conn) .unwrap(); @@ -314,14 +312,10 @@ fn index_sorting() { .set(crates::updated_at.eq(now - 3.weeks())) .execute(conn) .unwrap(); - update(&krate2) + update(crates::table.filter(crates::id.eq_any(vec![krate2.id, krate3.id]))) .set(crates::updated_at.eq(now - 5.days())) .execute(conn) .unwrap(); - update(&krate3) - .set(crates::updated_at.eq(now - 10.seconds())) - .execute(conn) - .unwrap(); update(&krate4) .set(crates::updated_at.eq(now)) .execute(conn) @@ -331,14 +325,14 @@ fn index_sorting() { // Sort by downloads for json in search_both(&anon, "sort=downloads") { assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); + assert_eq!(json.crates[0].name, "baz_sort"); + assert_eq!(json.crates[1].name, "other_sort"); 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"); - assert_eq!(resp[0].crates[0].name, "other_sort"); - assert_eq!(resp[1].crates[0].name, "baz_sort"); + assert_eq!(resp[0].crates[0].name, "baz_sort"); + assert_eq!(resp[1].crates[0].name, "other_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); @@ -364,14 +358,14 @@ fn index_sorting() { for json in search_both(&anon, "sort=recent-updates") { assert_eq!(json.meta.total, 4); assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "bar_sort"); + assert_eq!(json.crates[1].name, "bar_sort"); + assert_eq!(json.crates[2].name, "baz_sort"); assert_eq!(json.crates[3].name, "foo_sort"); } let (resp, calls) = page_with_seek(&anon, "sort=recent-updates"); 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[1].crates[0].name, "bar_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); @@ -392,6 +386,82 @@ fn index_sorting() { assert_eq!(resp[3].meta.total, 4); assert_eq!(calls, 5); + use std::cmp::Reverse; + + fn decode_seek serde::Deserialize<'a>>(seek: &str) -> anyhow::Result { + use base64::{engine::general_purpose, Engine}; + let decoded = serde_json::from_slice(&general_purpose::URL_SAFE_NO_PAD.decode(seek)?)?; + Ok(decoded) + } + + // Sort by alpha with query + for query in ["sort=alpha&q=bar_sort", "sort=alpha&q=sort"] { + let (resp, calls) = page_with_seek(&anon, query); + assert_eq!(calls, resp[0].meta.total + 1); + let decoded_seeks = resp + .iter() + .filter_map(|cl| { + cl.meta + .next_page + .as_ref() + .map(|next_page| (next_page, cl.crates[0].name.to_owned())) + }) + .filter_map(|(q, name)| { + let query = url::form_urlencoded::parse(q.trim_start_matches('?').as_bytes()) + .into_owned() + .collect::>(); + query.get("seek").map(|s| { + let d = decode_seek::<(bool, i32)>(s).unwrap(); + (d.0, name) + }) + }) + .collect::>(); + // ordering (exact match desc, name asc) + let mut sorted = decoded_seeks.to_vec(); + sorted.sort_by_key(|k| (Reverse(k.0), k.1.to_owned())); + assert_eq!(sorted, decoded_seeks); + for json in search_both(&anon, query) { + assert_eq!(json.meta.total, resp[0].meta.total); + for (c, r) in json.crates.iter().zip(&resp) { + assert_eq!(c.name, r.crates[0].name); + } + } + } + + // Sort by relevance + for query in ["q=foo_sort", "q=sort"] { + let (resp, calls) = page_with_seek(&anon, query); + assert_eq!(calls, resp[0].meta.total + 1); + let decoded_seeks = resp + .iter() + .filter_map(|cl| { + cl.meta + .next_page + .as_ref() + .map(|next_page| (next_page, cl.crates[0].name.to_owned())) + }) + .filter_map(|(q, name)| { + let query = url::form_urlencoded::parse(q.trim_start_matches('?').as_bytes()) + .into_owned() + .collect::>(); + query.get("seek").map(|s| { + let d = decode_seek::<(bool, f32, i32)>(s).unwrap(); + (d.0, (d.1 * 1e12) as i64, name) + }) + }) + .collect::>(); + // ordering (exact match desc, rank desc, name asc) + let mut sorted = decoded_seeks.clone(); + sorted.sort_by_key(|k| (Reverse(k.0), Reverse(k.1), k.2.to_owned())); + assert_eq!(sorted, decoded_seeks); + for json in search_both(&anon, query) { + assert_eq!(json.meta.total, resp[0].meta.total); + for (c, r) in json.crates.iter().zip(&resp) { + assert_eq!(c.name, r.crates[0].name); + } + } + } + // Test for bug with showing null results first when sorting // by descending downloads for json in search_both(&anon, "sort=recent-downloads") { From 548be214600d7d92caaa2aa2636977950df7d4e6 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Tue, 30 Jan 2024 18:33:21 +0800 Subject: [PATCH 6/6] controllers/krate/search: Reverse the sort order of the auxiliary column Since we all use descending order for sorting, it would be more intuitive to change all the auxiliary column from ascending to descending. --- src/controllers/krate/search.rs | 30 ++++++++++++------------- src/tests/routes/crates/list.rs | 40 ++++++++++++++++----------------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index be609e1b2f8..de2290e9ae2 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -121,19 +121,19 @@ pub async fn search(app: AppState, req: Parts) -> AppResult> { // to ensure predictable pagination behavior. if sort == Some("downloads") { seek = Some(Seek::Downloads); - query = query.order((crates::downloads.desc(), crates::id.asc())) + query = query.order((crates::downloads.desc(), crates::id.desc())) } else if sort == Some("recent-downloads") { seek = Some(Seek::RecentDownloads); query = query.order(( recent_crate_downloads::downloads.desc().nulls_last(), - crates::id.asc(), + crates::id.desc(), )) } else if sort == Some("recent-updates") { seek = Some(Seek::RecentUpdates); - query = query.order((crates::updated_at.desc(), crates::id.asc())); + query = query.order((crates::updated_at.desc(), crates::id.desc())); } else if sort == Some("new") { seek = Some(Seek::New); - query = query.order((crates::created_at.desc(), crates::id.asc())); + query = query.order((crates::created_at.desc(), crates::id.desc())); } else { seek = seek.or(Some(Seek::Name)); // Since the name is unique value, the inherent ordering becomes naturally unique. @@ -413,12 +413,12 @@ impl<'a> FilterParams<'a> { } SeekPayload::New(New(created_at, id)) => { // Equivalent of: - // `WHERE (created_at = created_at' AND id > id') OR created_at < created_at'` + // `WHERE (created_at = created_at' AND id < id') OR created_at < created_at'` vec![ Box::new( crates::created_at .eq(created_at) - .and(crates::id.gt(id)) + .and(crates::id.lt(id)) .nullable(), ), Box::new(crates::created_at.lt(created_at).nullable()), @@ -426,12 +426,12 @@ impl<'a> FilterParams<'a> { } SeekPayload::RecentUpdates(RecentUpdates(updated_at, id)) => { // Equivalent of: - // `WHERE (updated_at = updated_at' AND id > id') OR updated_at < updated_at'` + // `WHERE (updated_at = updated_at' AND id < id') OR updated_at < updated_at'` vec![ Box::new( crates::updated_at .eq(updated_at) - .and(crates::id.gt(id)) + .and(crates::id.lt(id)) .nullable(), ), Box::new(crates::updated_at.lt(updated_at).nullable()), @@ -440,17 +440,17 @@ impl<'a> FilterParams<'a> { SeekPayload::RecentDownloads(RecentDownloads(recent_downloads, id)) => { // Equivalent of: // for recent_downloads is not None: - // `WHERE (recent_downloads = recent_downloads' AND id > id') + // `WHERE (recent_downloads = recent_downloads' AND id < id') // OR (recent_downloads < recent_downloads' OR recent_downloads IS NULL)` // for recent_downloads is None: - // `WHERE (recent_downloads IS NULL AND id > id')` + // `WHERE (recent_downloads IS NULL AND id < id')` match recent_downloads { Some(dl) => { vec![ Box::new( recent_crate_downloads::downloads .eq(dl) - .and(crates::id.gt(id)) + .and(crates::id.lt(id)) .nullable(), ), Box::new( @@ -465,7 +465,7 @@ impl<'a> FilterParams<'a> { vec![Box::new( recent_crate_downloads::downloads .is_null() - .and(crates::id.gt(id)) + .and(crates::id.lt(id)) .nullable(), )] } @@ -473,12 +473,12 @@ impl<'a> FilterParams<'a> { } SeekPayload::Downloads(Downloads(downloads, id)) => { // Equivalent of: - // `WHERE (downloads = downloads' AND id > id') OR downloads < downloads'` + // `WHERE (downloads = downloads' AND id < id') OR downloads < downloads'` vec![ Box::new( crates::downloads .eq(downloads) - .and(crates::id.gt(id)) + .and(crates::id.lt(id)) .nullable(), ), Box::new(crates::downloads.lt(downloads).nullable()), @@ -486,7 +486,7 @@ impl<'a> FilterParams<'a> { } SeekPayload::Query(Query(exact_match, id)) => { // Equivalent of: - // `WHERE (exact_match = exact_match' AND name > name') OR exact_match < + // `WHERE (exact_match = exact_match' AND name < name') OR exact_match < // exact_match'` let q_string = self.q_string.expect("q_string should not be None"); let name_exact_match = Crate::with_name(q_string); diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 5d0ef34f2b9..b2a6b2a633e 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -325,14 +325,14 @@ fn index_sorting() { // Sort by downloads for json in search_both(&anon, "sort=downloads") { assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "baz_sort"); - assert_eq!(json.crates[1].name, "other_sort"); + assert_eq!(json.crates[0].name, "other_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); 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"); - assert_eq!(resp[0].crates[0].name, "baz_sort"); - assert_eq!(resp[1].crates[0].name, "other_sort"); + 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); @@ -341,14 +341,14 @@ fn index_sorting() { // Sort by recent-downloads for json in search_both(&anon, "sort=recent-downloads") { assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); + 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"); - assert_eq!(resp[0].crates[0].name, "foo_sort"); - assert_eq!(resp[1].crates[0].name, "baz_sort"); + 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); @@ -358,14 +358,14 @@ fn index_sorting() { for json in search_both(&anon, "sort=recent-updates") { assert_eq!(json.meta.total, 4); assert_eq!(json.crates[0].name, "other_sort"); - assert_eq!(json.crates[1].name, "bar_sort"); - assert_eq!(json.crates[2].name, "baz_sort"); + assert_eq!(json.crates[1].name, "baz_sort"); + 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"); 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, "baz_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); @@ -374,14 +374,14 @@ fn index_sorting() { for json in search_both(&anon, "sort=new") { assert_eq!(json.meta.total, 4); assert_eq!(json.crates[0].name, "bar_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); - assert_eq!(json.crates[2].name, "other_sort"); + assert_eq!(json.crates[1].name, "other_sort"); + 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"); assert_eq!(resp[0].crates[0].name, "bar_sort"); - assert_eq!(resp[1].crates[0].name, "baz_sort"); - assert_eq!(resp[2].crates[0].name, "other_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); @@ -466,14 +466,14 @@ fn index_sorting() { // by descending downloads for json in search_both(&anon, "sort=recent-downloads") { assert_eq!(json.meta.total, 4); - assert_eq!(json.crates[0].name, "foo_sort"); - assert_eq!(json.crates[1].name, "baz_sort"); + 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"); - assert_eq!(resp[0].crates[0].name, "foo_sort"); - assert_eq!(resp[1].crates[0].name, "baz_sort"); + 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);