From 9b39d0382c086db9e292a6d3fdeb2f4cf73963f8 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sun, 18 Aug 2019 16:02:13 -0600 Subject: [PATCH 1/4] Refactor pagination to prepare for cursor-based pagination. This continues the changes made in #1763. Since that PR was merged, one of the non-code steps has been taken care of -- All users hitting any endpoint with `?page=20` (which is an arbitrary search pattern that seemed high enough to give any crawlers going through pagination) have been contacted about the change, with a PR opened against any that included a repo. (Intersting aside, there are *zero* records of this for any endpoint other than search, which perhaps implies we can get rid of a few of these endpoints, but that's a separate discussion). This PR does not change any functionality, but moves some code around to better encapsulate things for upcoming changes. Specifically: - Change our frontend to show "next/prev page" links on the all crates page - Stop returning the "total" meta item when the next/prev page links will be cursor based (which I'd actually just like to start omitting in general) The main goal of this change was to stop having any code outside of `Paginated` (which has been renamed to `PaginatedQuery`, as there's a new type called `Paginated`) care about how pagination occurs. This means other code can't care about *how* pagination happens (with the exception of `reverse_dependencies`, which uses raw SQL, and sorta has to... That was a bit of a rabbit hole, see https://github.com/diesel-rs/diesel/issues/2150 for details. Given the low traffic to that endpoint, I think we can safely ignore it). The distribution of responsibilities is as follows: - `PaginatedQuery`: Given the query params, decides how to paginate things, generates appropriate SQL, loads a `Paginated`. - `Paginated`: Handles providing an iterator to the records, getting the total count (to be removed in the near future), providing the next/prev page params - `Request`: Takes the pagination related query params, turns that into an actual URL (note: Due to jankiness in our router, this will only ever be a query string, we have no way of getting the actual path) The next step from here is to change our UI to stop showing page numbers, and then remove the `total` field. This PR will introduce a performance regression that was addressed by #1668. That PR was addressing "this will become a problem in a future", not "this is about to take the site down". Given the intent to remove the `total` field entirely, I think it is fine to cause this regression in the short term. If we aren't confident that the changes to remove this field will land in the near future, or want to be conservative about this, I can add some additional complexity/git churn to retain the previous performance characteristics --- src/controllers.rs | 29 ++--- src/controllers/category.rs | 9 +- src/controllers/helpers.rs | 4 +- src/controllers/helpers/pagination.rs | 174 +++++++++++++++++++++++--- src/controllers/keyword.rs | 11 +- src/controllers/krate/metadata.rs | 3 +- src/controllers/krate/search.rs | 68 ++-------- src/controllers/user/me.rs | 14 +-- src/models/helpers/with_count.rs | 8 +- src/models/krate.rs | 19 ++- src/tests/krate.rs | 3 +- 11 files changed, 216 insertions(+), 126 deletions(-) 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..dbc2f4237ff 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,15 @@ 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, options.per_page as i64, offset as i64)?; 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..be1f96d7bb0 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -2,33 +2,171 @@ use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; use diesel::sql_types::BigInt; +use diesel::query_dsl::LoadQuery; +use indexmap::IndexMap; +use crate::util::errors::*; +use crate::models::helpers::with_count::*; + +#[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.clone(); + 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::(&(self.options.per_page as i64))?; + if let Some(offset) = self.options.offset() { + out.push_sql(" OFFSET "); + out.push_bind_param::(&(offset as i64))?; + } Ok(()) } } diff --git a/src/controllers/keyword.rs b/src/controllers/keyword.rs index b1aaf00deed..c172c44b87e 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"); @@ -22,12 +21,12 @@ pub fn index(req: &mut dyn Request) -> CargoResult { } let data = query - .paginate(limit, offset) - .load::<(Keyword, i64)>(&*conn)?; - let total = data.get(0).map(|&(_, t)| t).unwrap_or(0); + .paginate(&req.query())? + .load::(&*conn)?; + let total = data.total(); let kws = data .into_iter() - .map(|(k, _)| k.encodable()) + .map(Keyword::encodable) .collect::>(); #[derive(Serialize)] @@ -37,7 +36,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..7d0cb06434f 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,17 +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), _)| { + .map(|(version, crate_name, published_by)| { version.encodable(&crate_name, published_by) }) .collect(); 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..1c4d031904d 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -4,6 +4,7 @@ use diesel::pg::Pg; use diesel::prelude::*; use diesel::sql_types::Bool; use url::Url; +use indexmap::IndexMap; use crate::app::App; use crate::util::{human, CargoResult}; @@ -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 diesel::sql_query; use diesel::sql_types::{BigInt, Integer}; - + use crate::controllers::helpers::pagination::*; + + // 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::(offset as i64) + .bind::(options.per_page as i64) .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); } From 5667d358ad84d62ebd543a6d0ed9aefd7140256e Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 19 Aug 2019 10:14:08 -0600 Subject: [PATCH 2/4] rustfmt --- src/controllers/helpers/pagination.rs | 20 ++++++++++---------- src/controllers/keyword.rs | 9 ++------- src/controllers/user/me.rs | 4 +--- src/models/krate.rs | 4 ++-- 4 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index be1f96d7bb0..057daca1f05 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -1,11 +1,11 @@ +use crate::models::helpers::with_count::*; +use crate::util::errors::*; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; -use diesel::sql_types::BigInt; use diesel::query_dsl::LoadQuery; +use diesel::sql_types::BigInt; use indexmap::IndexMap; -use crate::util::errors::*; -use crate::models::helpers::with_count::*; #[derive(Debug, Clone, Copy)] pub(crate) enum Page { @@ -87,10 +87,12 @@ pub struct Paginated { impl Paginated { pub(crate) fn total(&self) -> Option { - Some(self.records_and_total - .get(0) - .map(|row| row.total) - .unwrap_or_default()) + Some( + self.records_and_total + .get(0) + .map(|row| row.total) + .unwrap_or_default(), + ) } pub(crate) fn next_page_params(&self) -> Option> { @@ -120,9 +122,7 @@ impl Paginated { } pub(crate) fn iter(&self) -> impl Iterator { - self.records_and_total - .iter() - .map(|row| &row.record) + self.records_and_total.iter().map(|row| &row.record) } } diff --git a/src/controllers/keyword.rs b/src/controllers/keyword.rs index c172c44b87e..4edbc550b21 100644 --- a/src/controllers/keyword.rs +++ b/src/controllers/keyword.rs @@ -20,14 +20,9 @@ pub fn index(req: &mut dyn Request) -> CargoResult { query = query.order(keywords::keyword.asc()); } - let data = query - .paginate(&req.query())? - .load::(&*conn)?; + let data = query.paginate(&req.query())?.load::(&*conn)?; let total = data.total(); - let kws = data - .into_iter() - .map(Keyword::encodable) - .collect::>(); + let kws = data.into_iter().map(Keyword::encodable).collect::>(); #[derive(Serialize)] struct R { diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 7d0cb06434f..295e3259a3f 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -70,9 +70,7 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { 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/krate.rs b/src/models/krate.rs index 1c4d031904d..1d38aea9b59 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -3,8 +3,8 @@ use diesel::associations::Identifiable; use diesel::pg::Pg; use diesel::prelude::*; use diesel::sql_types::Bool; -use url::Url; use indexmap::IndexMap; +use url::Url; use crate::app::App; use crate::util::{human, CargoResult}; @@ -525,9 +525,9 @@ impl Crate { conn: &PgConnection, params: &IndexMap, ) -> CargoResult<(Vec, i64)> { + use crate::controllers::helpers::pagination::*; use diesel::sql_query; use diesel::sql_types::{BigInt, Integer}; - use crate::controllers::helpers::pagination::*; // FIXME: It'd be great to support this with `.paginate` directly, // and get cursor/id pagination for free. But Diesel doesn't currently From 1682066054eaf0f4ed6c583b9ae7bb0d7aca3568 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 19 Aug 2019 11:56:19 -0600 Subject: [PATCH 3/4] Make clippy happy --- src/controllers/category.rs | 2 +- src/controllers/helpers/pagination.rs | 4 ++-- src/models/krate.rs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/controllers/category.rs b/src/controllers/category.rs index dbc2f4237ff..0214f707db1 100644 --- a/src/controllers/category.rs +++ b/src/controllers/category.rs @@ -16,7 +16,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult { let offset = options.offset().unwrap_or_default(); let sort = query.get("sort").map_or("alpha", String::as_str); - let categories = Category::toplevel(&conn, sort, options.per_page as i64, offset as i64)?; + 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/pagination.rs b/src/controllers/helpers/pagination.rs index 057daca1f05..d94a309c8fc 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -174,10 +174,10 @@ where out.push_sql("SELECT *, COUNT(*) OVER () FROM ("); self.query.walk_ast(out.reborrow())?; out.push_sql(") t LIMIT "); - out.push_bind_param::(&(self.options.per_page as i64))?; + 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::(&(offset as i64))?; + out.push_bind_param::(&i64::from(offset))?; } Ok(()) } diff --git a/src/models/krate.rs b/src/models/krate.rs index 1d38aea9b59..e96c72cb242 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -537,8 +537,8 @@ impl Crate { let offset = options.offset().unwrap_or_default(); let rows = sql_query(include_str!("krate_reverse_dependencies.sql")) .bind::(self.id) - .bind::(offset as i64) - .bind::(options.per_page as i64) + .bind::(i64::from(offset)) + .bind::(i64::from(options.per_page)) .load::>(conn)?; Ok(rows.records_and_total()) From 8ef5179cd83a3e7de826cd432b066d37f099eb65 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Thu, 29 Aug 2019 08:22:19 -0600 Subject: [PATCH 4/4] Make clippy happy for real this time I swear I run this locally... --- src/controllers/category.rs | 3 ++- src/controllers/helpers/pagination.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/controllers/category.rs b/src/controllers/category.rs index 0214f707db1..c16aa74f49c 100644 --- a/src/controllers/category.rs +++ b/src/controllers/category.rs @@ -16,7 +16,8 @@ pub fn index(req: &mut dyn Request) -> CargoResult { let offset = options.offset().unwrap_or_default(); let sort = query.get("sort").map_or("alpha", String::as_str); - let categories = Category::toplevel(&conn, sort, i64::from(options.per_page), i64::from(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/pagination.rs b/src/controllers/helpers/pagination.rs index d94a309c8fc..f028833947f 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -146,7 +146,7 @@ impl PaginatedQuery { where Self: LoadQuery>, { - let options = self.options.clone(); + let options = self.options; let records_and_total = self.internal_load(conn)?; Ok(Paginated { records_and_total,