diff --git a/src/controllers.rs b/src/controllers.rs index 06cbafa0600..a8f947f1282 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -24,7 +24,7 @@ mod prelude { fn json(&self, t: &T) -> Response; fn query(&self) -> IndexMap; fn wants_json(&self) -> bool; - fn pagination(&self, default: usize, max: usize) -> CargoResult<(i64, i64)>; + fn query_with_params(&self, params: IndexMap) -> String; } impl<'a> RequestUtils for dyn Request + 'a { @@ -55,26 +55,13 @@ mod prelude { .unwrap_or(false) } - fn pagination(&self, default: usize, max: usize) -> CargoResult<(i64, i64)> { - let query = self.query(); - let page = query - .get("page") - .and_then(|s| s.parse::().ok()) - .unwrap_or(1); - let limit = query - .get("per_page") - .and_then(|s| s.parse::().ok()) - .unwrap_or(default); - if limit > max { - return Err(human(&format_args!( - "cannot request more than {} items", - max - ))); - } - if page == 0 { - return Err(human("page indexing starts from 1, page 0 is invalid")); - } - Ok((((page - 1) * limit) as i64, limit as i64)) + fn query_with_params(&self, new_params: IndexMap) -> String { + let mut params = self.query().clone(); + params.extend(new_params); + let query_string = url::form_urlencoded::Serializer::new(String::new()) + .extend_pairs(params) + .finish(); + format!("?{}", query_string) } } } diff --git a/src/controllers/category.rs b/src/controllers/category.rs index 30860203a4e..c16aa74f49c 100644 --- a/src/controllers/category.rs +++ b/src/controllers/category.rs @@ -1,3 +1,4 @@ +use super::helpers::pagination::*; use super::prelude::*; use crate::models::Category; @@ -7,11 +8,16 @@ use crate::views::{EncodableCategory, EncodableCategoryWithSubcategories}; /// Handles the `GET /categories` route. pub fn index(req: &mut dyn Request) -> CargoResult { let conn = req.db_conn()?; - let (offset, limit) = req.pagination(10, 100)?; let query = req.query(); + // FIXME: There are 69 categories, 47 top level. This isn't going to + // grow by an OoM. We need a limit for /summary, but we don't need + // to paginate this. + let options = PaginationOptions::new(&query)?; + let offset = options.offset().unwrap_or_default(); let sort = query.get("sort").map_or("alpha", String::as_str); - let categories = Category::toplevel(&conn, sort, limit, offset)?; + let categories = + Category::toplevel(&conn, sort, i64::from(options.per_page), i64::from(offset))?; let categories = categories.into_iter().map(Category::encodable).collect(); // Query for the total count of categories diff --git a/src/controllers/helpers.rs b/src/controllers/helpers.rs index 3fc53e213a9..1f81c956eba 100644 --- a/src/controllers/helpers.rs +++ b/src/controllers/helpers.rs @@ -1,9 +1,9 @@ use crate::util::{json_response, CargoResult}; use conduit::Response; -pub mod pagination; +pub(crate) mod pagination; -pub use self::pagination::Paginate; +pub(crate) use self::pagination::Paginate; pub fn ok_true() -> CargoResult { #[derive(Serialize)] diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 76e9cc8a0bb..f028833947f 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -1,34 +1,172 @@ +use crate::models::helpers::with_count::*; +use crate::util::errors::*; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; +use diesel::query_dsl::LoadQuery; use diesel::sql_types::BigInt; +use indexmap::IndexMap; + +#[derive(Debug, Clone, Copy)] +pub(crate) enum Page { + Numeric(u32), + Unspecified, +} + +impl Page { + fn new(params: &IndexMap) -> CargoResult { + if let Some(s) = params.get("page") { + let numeric_page = s.parse()?; + if numeric_page < 1 { + return Err(human(&format_args!( + "page indexing starts from 1, page {} is invalid", + numeric_page, + ))); + } + + Ok(Page::Numeric(numeric_page)) + } else { + Ok(Page::Unspecified) + } + } +} + +#[derive(Debug, Clone, Copy)] +pub(crate) struct PaginationOptions { + page: Page, + pub(crate) per_page: u32, +} + +impl PaginationOptions { + pub(crate) fn new(params: &IndexMap) -> CargoResult { + const DEFAULT_PER_PAGE: u32 = 10; + const MAX_PER_PAGE: u32 = 100; + + let per_page = params + .get("per_page") + .map(|s| s.parse()) + .unwrap_or(Ok(DEFAULT_PER_PAGE))?; + + if per_page > MAX_PER_PAGE { + return Err(human(&format_args!( + "cannot request more than {} items", + MAX_PER_PAGE, + ))); + } + + Ok(Self { + page: Page::new(params)?, + per_page, + }) + } + + pub(crate) fn offset(&self) -> Option { + if let Page::Numeric(p) = self.page { + Some((p - 1) * self.per_page) + } else { + None + } + } +} + +pub(crate) trait Paginate: Sized { + fn paginate(self, params: &IndexMap) -> CargoResult> { + Ok(PaginatedQuery { + query: self, + options: PaginationOptions::new(params)?, + }) + } +} + +impl Paginate for T {} -#[derive(Debug, QueryId)] pub struct Paginated { - query: T, - limit: i64, - offset: i64, + records_and_total: Vec>, + options: PaginationOptions, } -pub trait Paginate: AsQuery + Sized { - fn paginate(self, limit: i64, offset: i64) -> Paginated { - Paginated { - query: self.as_query(), - limit, - offset, +impl Paginated { + pub(crate) fn total(&self) -> Option { + Some( + self.records_and_total + .get(0) + .map(|row| row.total) + .unwrap_or_default(), + ) + } + + pub(crate) fn next_page_params(&self) -> Option> { + if self.records_and_total.len() < self.options.per_page as usize { + return None; } + + let mut opts = IndexMap::new(); + match self.options.page { + Page::Numeric(n) => opts.insert("page".into(), (n + 1).to_string()), + Page::Unspecified => opts.insert("page".into(), 2.to_string()), + }; + Some(opts) + } + + pub(crate) fn prev_page_params(&self) -> Option> { + if let Page::Numeric(1) | Page::Unspecified = self.options.page { + return None; + } + + let mut opts = IndexMap::new(); + match self.options.page { + Page::Numeric(n) => opts.insert("page".into(), (n - 1).to_string()), + Page::Unspecified => unreachable!(), + }; + Some(opts) + } + + pub(crate) fn iter(&self) -> impl Iterator { + self.records_and_total.iter().map(|row| &row.record) + } +} + +impl IntoIterator for Paginated { + type IntoIter = Box>; + type Item = T; + + fn into_iter(self) -> Self::IntoIter { + Box::new(self.records_and_total.into_iter().map(|row| row.record)) } } -impl Paginate for T {} +#[derive(Debug)] +pub(crate) struct PaginatedQuery { + query: T, + options: PaginationOptions, +} -impl Query for Paginated { +impl PaginatedQuery { + pub(crate) fn load(self, conn: &PgConnection) -> QueryResult> + where + Self: LoadQuery>, + { + let options = self.options; + let records_and_total = self.internal_load(conn)?; + Ok(Paginated { + records_and_total, + options, + }) + } +} + +impl QueryId for PaginatedQuery { + const HAS_STATIC_QUERY_ID: bool = false; + type QueryId = (); +} + +impl Query for PaginatedQuery { type SqlType = (T::SqlType, BigInt); } -impl RunQueryDsl for Paginated {} +impl RunQueryDsl for PaginatedQuery {} -impl QueryFragment for Paginated +impl QueryFragment for PaginatedQuery where T: QueryFragment, { @@ -36,9 +174,11 @@ where out.push_sql("SELECT *, COUNT(*) OVER () FROM ("); self.query.walk_ast(out.reborrow())?; out.push_sql(") t LIMIT "); - out.push_bind_param::(&self.limit)?; - out.push_sql(" OFFSET "); - out.push_bind_param::(&self.offset)?; + out.push_bind_param::(&i64::from(self.options.per_page))?; + if let Some(offset) = self.options.offset() { + out.push_sql(" OFFSET "); + out.push_bind_param::(&i64::from(offset))?; + } Ok(()) } } diff --git a/src/controllers/keyword.rs b/src/controllers/keyword.rs index b1aaf00deed..4edbc550b21 100644 --- a/src/controllers/keyword.rs +++ b/src/controllers/keyword.rs @@ -9,7 +9,6 @@ pub fn index(req: &mut dyn Request) -> CargoResult { use crate::schema::keywords; let conn = req.db_conn()?; - let (offset, limit) = req.pagination(10, 100)?; let query = req.query(); let sort = query.get("sort").map(|s| &s[..]).unwrap_or("alpha"); @@ -21,14 +20,9 @@ pub fn index(req: &mut dyn Request) -> CargoResult { query = query.order(keywords::keyword.asc()); } - let data = query - .paginate(limit, offset) - .load::<(Keyword, i64)>(&*conn)?; - let total = data.get(0).map(|&(_, t)| t).unwrap_or(0); - let kws = data - .into_iter() - .map(|(k, _)| k.encodable()) - .collect::>(); + let data = query.paginate(&req.query())?.load::(&*conn)?; + let total = data.total(); + let kws = data.into_iter().map(Keyword::encodable).collect::>(); #[derive(Serialize)] struct R { @@ -37,7 +31,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult { } #[derive(Serialize)] struct Meta { - total: i64, + total: Option, } Ok(req.json(&R { diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index df9b9389c86..d53741c3c7a 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -212,8 +212,7 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult { let name = &req.params()["crate_id"]; let conn = req.db_conn()?; let krate = Crate::by_name(name).first::(&*conn)?; - let (offset, limit) = req.pagination(10, 100)?; - let (rev_deps, total) = krate.reverse_dependencies(&*conn, offset, limit)?; + let (rev_deps, total) = krate.reverse_dependencies(&*conn, &req.query())?; let rev_deps: Vec<_> = rev_deps .into_iter() .map(|dep| dep.encodable(&krate.name)) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 2250fbb15ad..552cb7d6efe 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -1,7 +1,6 @@ //! Endpoint for searching and discovery functionality use diesel::dsl::*; -use diesel::sql_types::{NotNull, Nullable}; use diesel_full_text_search::*; use crate::controllers::helpers::Paginate; @@ -37,13 +36,11 @@ pub fn search(req: &mut dyn Request) -> CargoResult { use diesel::sql_types::{Bool, Text}; let conn = req.db_conn()?; - let (offset, limit) = req.pagination(10, 100)?; let params = req.query(); let sort = params .get("sort") .map(|s| &**s) .unwrap_or("recent-downloads"); - let mut has_filter = false; let include_yanked = params .get("include_yanked") .map(|s| s == "yes") @@ -60,7 +57,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { .into_boxed(); if let Some(q_string) = params.get("q") { - has_filter = true; if !q_string.is_empty() { let sort = params.get("sort").map(|s| &**s).unwrap_or("relevance"); @@ -88,7 +84,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } if let Some(cat) = params.get("category") { - has_filter = true; query = query.filter( crates::id.eq_any( crates_categories::table @@ -104,7 +99,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } if let Some(kw) = params.get("keyword") { - has_filter = true; query = query.filter( crates::id.eq_any( crates_keywords::table @@ -114,7 +108,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ), ); } else if let Some(letter) = params.get("letter") { - has_filter = true; let pattern = format!( "{}%", letter @@ -126,7 +119,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ); query = query.filter(canon_crate_name(crates::name).like(pattern)); } else if let Some(user_id) = params.get("user_id").and_then(|s| s.parse::().ok()) { - has_filter = true; query = query.filter( crates::id.eq_any( crate_owners::table @@ -137,7 +129,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ), ); } else if let Some(team_id) = params.get("team_id").and_then(|s| s.parse::().ok()) { - has_filter = true; query = query.filter( crates::id.eq_any( crate_owners::table @@ -148,7 +139,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ), ); } else if params.get("following").is_some() { - has_filter = true; query = query.filter( crates::id.eq_any( follows::table @@ -159,7 +149,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } if !include_yanked { - has_filter = true; query = query.filter(exists( versions::table .filter(versions::crate_id.eq(crates::id)) @@ -177,32 +166,20 @@ pub fn search(req: &mut dyn Request) -> CargoResult { query = query.then_order_by(crates::name.asc()) } - // The database query returns a tuple within a tuple, with the root - // tuple containing 3 items. - let data = if has_filter { - query - .paginate(limit, offset) - .load::<((Crate, bool, Option), i64)>(&*conn)? - } else { - sql_function!(fn coalesce(value: Nullable, default: T) -> T); - query - .select(( - // FIXME: Use `query.selection()` if that feature ends up in - // Diesel 2.0 - selection, - coalesce(crates::table.count().single_value(), 0), - )) - .limit(limit) - .offset(offset) - .load(&*conn)? - }; - let total = data.first().map(|&(_, t)| t).unwrap_or(0); - let perfect_matches = data.iter().map(|&((_, b, _), _)| b).collect::>(); + let data = query + .paginate(&req.query())? + .load::<(Crate, bool, Option)>(&*conn)?; + let total = data.total(); + + let next_page = data.next_page_params().map(|p| req.query_with_params(p)); + let prev_page = data.prev_page_params().map(|p| req.query_with_params(p)); + + 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 = crates .versions() @@ -235,27 +212,6 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ) .collect(); - let mut next_page = None; - let mut prev_page = None; - let page_num = params.get("page").map(|s| s.parse()).unwrap_or(Ok(1))?; - - let url_for_page = |num: i64| { - let mut params = req.query(); - params.insert("page".into(), num.to_string()); - let query_string = url::form_urlencoded::Serializer::new(String::new()) - .clear() - .extend_pairs(params) - .finish(); - Some(format!("?{}", query_string)) - }; - - if offset + limit < total { - next_page = url_for_page(page_num + 1); - } - if page_num != 1 { - prev_page = url_for_page(page_num - 1); - }; - #[derive(Serialize)] struct R { crates: Vec, @@ -263,7 +219,7 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } #[derive(Serialize)] struct Meta { - total: i64, + total: Option, next_page: Option, prev_page: Option, } diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index d8100eded9e..295e3259a3f 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -1,6 +1,6 @@ use crate::controllers::prelude::*; -use crate::controllers::helpers::Paginate; +use crate::controllers::helpers::*; use crate::email; use crate::util::bad_request; use crate::util::errors::CargoError; @@ -50,7 +50,6 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { use diesel::dsl::any; let user = req.user()?; - let (offset, limit) = req.pagination(10, 100)?; let conn = req.db_conn()?; let followed_crates = Follow::belonging_to(user).select(follows::crate_id); @@ -64,19 +63,14 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { crates::name, users::all_columns.nullable(), )) - .paginate(limit, offset) - .load::<((Version, String, Option), i64)>(&*conn)?; + .paginate(&req.query())? + .load::<(Version, String, Option)>(&*conn)?; - let more = data - .get(0) - .map(|&(_, count)| count > offset + limit) - .unwrap_or(false); + let more = data.next_page_params().is_some(); let versions = data .into_iter() - .map(|((version, crate_name, published_by), _)| { - version.encodable(&crate_name, published_by) - }) + .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by)) .collect(); #[derive(Serialize)] diff --git a/src/models/helpers/with_count.rs b/src/models/helpers/with_count.rs index 097a43b526c..a63d7ca5701 100644 --- a/src/models/helpers/with_count.rs +++ b/src/models/helpers/with_count.rs @@ -1,9 +1,9 @@ -#[derive(QueryableByName, Debug)] +#[derive(QueryableByName, Queryable, Debug)] pub struct WithCount { - #[sql_type = "::diesel::sql_types::BigInt"] - total: i64, #[diesel(embed)] - record: T, + pub(crate) record: T, + #[sql_type = "::diesel::sql_types::BigInt"] + pub(crate) total: i64, } pub trait WithCountExtension { diff --git a/src/models/krate.rs b/src/models/krate.rs index 327665108be..e96c72cb242 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -3,6 +3,7 @@ use diesel::associations::Identifiable; use diesel::pg::Pg; use diesel::prelude::*; use diesel::sql_types::Bool; +use indexmap::IndexMap; use url::Url; use crate::app::App; @@ -522,16 +523,22 @@ impl Crate { pub fn reverse_dependencies( &self, conn: &PgConnection, - offset: i64, - limit: i64, - ) -> QueryResult<(Vec, i64)> { + params: &IndexMap, + ) -> CargoResult<(Vec, i64)> { + use crate::controllers::helpers::pagination::*; use diesel::sql_query; use diesel::sql_types::{BigInt, Integer}; + // FIXME: It'd be great to support this with `.paginate` directly, + // and get cursor/id pagination for free. But Diesel doesn't currently + // have great support for abstracting over "Is this using `Queryable` + // or `QueryableByName` to load things?" + let options = PaginationOptions::new(params)?; + let offset = options.offset().unwrap_or_default(); let rows = sql_query(include_str!("krate_reverse_dependencies.sql")) .bind::(self.id) - .bind::(offset) - .bind::(limit) + .bind::(i64::from(offset)) + .bind::(i64::from(options.per_page)) .load::>(conn)?; Ok(rows.records_and_total()) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index d88fe49c7f1..0a38a875525 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -2230,11 +2230,12 @@ fn pagination_links_included_if_applicable() { let page1 = anon.search("per_page=1"); let page2 = anon.search("page=2&per_page=1"); let page3 = anon.search("page=3&per_page=1"); + let page4 = anon.search("page=4&per_page=1"); assert_eq!(Some("?per_page=1&page=2".to_string()), page1.meta.next_page); assert_eq!(None, page1.meta.prev_page); assert_eq!(Some("?page=3&per_page=1".to_string()), page2.meta.next_page); assert_eq!(Some("?page=1&per_page=1".to_string()), page2.meta.prev_page); - assert_eq!(None, page3.meta.next_page); + assert_eq!(None, page4.meta.next_page); assert_eq!(Some("?page=2&per_page=1".to_string()), page3.meta.prev_page); }