From 6d7ab2ea21dac6b1faa902386a6a37278bbc5547 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 5 Jun 2017 16:04:31 -0400 Subject: [PATCH] Refactor paginating queries to a trait This pattern gets duplicated quite a bit (and it should be duplicated even more since we still have places where we're doing more than one query to paginate). Additionally, using the `sql` function will cause a query to be removed from the prepared statement cache (since Diesel doesn't know if the string was generated dynamically or not). This moves it out into a trait, and represents a paginated query in a bit more of an abstract form from the SQL point of view. This will slightly change the SQL in the places that it's being used, so that it looks more like what we have in `src/krate_reverse_dependencies.sql`, applying the limit/offset as late as possible. This will not affect the query plan unless they are using `distinct`, or if `limit`/`offset` are called prior to `paginate`. Even if those cases do happen, this query is probably what we meant anyway. --- src/keyword.rs | 11 +++-------- src/krate.rs | 29 +++++++++++------------------ src/lib.rs | 2 ++ src/pagination.rs | 43 +++++++++++++++++++++++++++++++++++++++++++ src/user/mod.rs | 19 +++++++------------ 5 files changed, 66 insertions(+), 38 deletions(-) create mode 100644 src/pagination.rs diff --git a/src/keyword.rs b/src/keyword.rs index 0606fbc6f9c..3008a667fa3 100644 --- a/src/keyword.rs +++ b/src/keyword.rs @@ -10,6 +10,7 @@ use pg::rows::Row; use {Model, Crate}; use db::RequestTransaction; +use pagination::Paginate; use schema::*; use util::{RequestUtils, CargoResult}; @@ -138,8 +139,6 @@ impl Model for Keyword { /// Handles the `GET /keywords` route. pub fn index(req: &mut Request) -> CargoResult { - use diesel::expression::dsl::sql; - use diesel::types::BigInt; use schema::keywords; let conn = req.db_conn()?; @@ -147,11 +146,7 @@ pub fn index(req: &mut Request) -> CargoResult { let query = req.query(); let sort = query.get("sort").map(|s| &s[..]).unwrap_or("alpha"); - let mut query = keywords::table - .select((keywords::all_columns, sql::("COUNT(*) OVER ()"))) - .limit(limit) - .offset(offset) - .into_boxed(); + let mut query = keywords::table.into_boxed(); if sort == "crates" { query = query.order(keywords::crates_cnt.desc()); @@ -159,7 +154,7 @@ pub fn index(req: &mut Request) -> CargoResult { query = query.order(keywords::keyword.asc()); } - let data = query.load::<(Keyword, i64)>(&*conn)?; + 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()) diff --git a/src/krate.rs b/src/krate.rs index 87c0db5ad60..2f3d4deb783 100644 --- a/src/krate.rs +++ b/src/krate.rs @@ -29,6 +29,7 @@ use download::{VersionDownload, EncodableVersionDownload}; use git; use keyword::{EncodableKeyword, CrateKeyword}; use owner::{EncodableOwner, Owner, Rights, OwnerKind, Team, rights, CrateOwner}; +use pagination::Paginate; use schema::*; use upload; use user::RequestUser; @@ -781,8 +782,8 @@ impl Model for Crate { /// Handles the `GET /crates` route. #[allow(trivial_casts)] pub fn index(req: &mut Request) -> CargoResult { - use diesel::expression::dsl::sql; - use diesel::types::{BigInt, Bool}; + use diesel::expression::AsExpression; + use diesel::types::Bool; let conn = req.db_conn()?; let (offset, limit) = req.pagination(10, 100)?; @@ -790,13 +791,7 @@ pub fn index(req: &mut Request) -> CargoResult { let sort = params.get("sort").map(|s| &**s).unwrap_or("alpha"); let mut query = crates::table - .select(( - ALL_COLUMNS, - sql::("COUNT(*) OVER ()"), - sql::("false"), - )) - .limit(limit) - .offset(offset) + .select((ALL_COLUMNS, AsExpression::::as_expression(false))) .into_boxed(); if sort == "downloads" { @@ -813,11 +808,7 @@ pub fn index(req: &mut Request) -> CargoResult { ), )); - query = query.select(( - ALL_COLUMNS, - sql::("COUNT(*) OVER()"), - crates::name.eq(q_string), - )); + query = query.select((ALL_COLUMNS, crates::name.eq(q_string))); let perfect_match = crates::name.eq(q_string).desc(); if sort == "downloads" { query = query.order((perfect_match, crates::downloads.desc())); @@ -887,12 +878,14 @@ pub fn index(req: &mut Request) -> CargoResult { )); } - let data = query.load::<(Crate, i64, bool)>(&*conn)?; - let total = data.get(0).map(|&(_, t, _)| t).unwrap_or(0); + let data = query.paginate(limit, offset).load::<((Crate, bool), i64)>( + &*conn, + )?; + let total = data.first().map(|&(_, t)| t).unwrap_or(0); let crates = data.iter() - .map(|&(ref c, _, _)| c.clone()) + .map(|&((ref c, _), _)| c.clone()) .collect::>(); - let perfect_matches = data.into_iter().map(|(_, _, b)| b).collect::>(); + let perfect_matches = data.into_iter().map(|((_, b), _)| b).collect::>(); let versions = Version::belonging_to(&crates) .load::(&*conn)? diff --git a/src/lib.rs b/src/lib.rs index 5bcaefd2852..227435ce959 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -92,6 +92,8 @@ pub mod user; pub mod util; pub mod version; +mod pagination; + #[derive(PartialEq, Eq, Clone, Copy, Debug)] pub enum Env { Development, diff --git a/src/pagination.rs b/src/pagination.rs new file mode 100644 index 00000000000..06ab3926d51 --- /dev/null +++ b/src/pagination.rs @@ -0,0 +1,43 @@ +use diesel::prelude::*; +use diesel::query_builder::*; +use diesel::types::BigInt; +use diesel::pg::Pg; + +pub struct Paginated { + query: T, + limit: i64, + offset: i64, +} + +pub trait Paginate: AsQuery + Sized { + fn paginate(self, limit: i64, offset: i64) -> Paginated { + Paginated { + query: self.as_query(), + limit, + offset, + } + } +} + +impl Paginate for T {} + +impl Query for Paginated { + type SqlType = (T::SqlType, BigInt); +} + +impl QueryFragment for Paginated +where + T: QueryFragment, +{ + fn walk_ast(&self, mut out: AstPass) -> QueryResult<()> { + 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)?; + Ok(()) + } +} + +impl_query_id!(Paginated); diff --git a/src/user/mod.rs b/src/user/mod.rs index 333d5d24326..7f2ccfc93b5 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -11,6 +11,7 @@ use std::borrow::Cow; use app::RequestApp; use db::RequestTransaction; use krate::Follow; +use pagination::Paginate; use schema::*; use util::errors::NotFound; use util::{RequestUtils, CargoResult, internal, ChainError, human}; @@ -377,8 +378,7 @@ pub fn show_team(req: &mut Request) -> CargoResult { /// Handles the `GET /me/updates` route. pub fn updates(req: &mut Request) -> CargoResult { - use diesel::expression::dsl::{any, sql}; - use diesel::types::BigInt; + use diesel::expression::dsl::any; let user = req.user()?; let (offset, limit) = req.pagination(10, 100)?; @@ -389,21 +389,16 @@ pub fn updates(req: &mut Request) -> CargoResult { .inner_join(crates::table) .filter(crates::id.eq(any(followed_crates))) .order(versions::created_at.desc()) - .limit(limit) - .offset(offset) - .select(( - versions::all_columns, - crates::name, - sql::("COUNT(*) OVER ()"), - )) - .load::<(Version, String, i64)>(&*conn)?; + .select((versions::all_columns, crates::name)) + .paginate(limit, offset) + .load::<((Version, String), i64)>(&*conn)?; let more = data.get(0) - .map(|&(_, _, count)| count > offset + limit) + .map(|&(_, count)| count > offset + limit) .unwrap_or(false); let versions = data.into_iter() - .map(|(version, crate_name, _)| version.encodable(&crate_name)) + .map(|((version, crate_name), _)| version.encodable(&crate_name)) .collect(); #[derive(RustcEncodable)]