Skip to content

Commit a0a168a

Browse files
committed
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.
1 parent f99c829 commit a0a168a

File tree

5 files changed

+64
-38
lines changed

5 files changed

+64
-38
lines changed

src/keyword.rs

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use pg::rows::Row;
1010

1111
use {Model, Crate};
1212
use db::RequestTransaction;
13+
use pagination::Paginate;
1314
use schema::*;
1415
use util::{RequestUtils, CargoResult};
1516

@@ -138,28 +139,22 @@ impl Model for Keyword {
138139

139140
/// Handles the `GET /keywords` route.
140141
pub fn index(req: &mut Request) -> CargoResult<Response> {
141-
use diesel::expression::dsl::sql;
142-
use diesel::types::BigInt;
143142
use schema::keywords;
144143

145144
let conn = req.db_conn()?;
146145
let (offset, limit) = req.pagination(10, 100)?;
147146
let query = req.query();
148147
let sort = query.get("sort").map(|s| &s[..]).unwrap_or("alpha");
149148

150-
let mut query = keywords::table
151-
.select((keywords::all_columns, sql::<BigInt>("COUNT(*) OVER ()")))
152-
.limit(limit)
153-
.offset(offset)
154-
.into_boxed();
149+
let mut query = keywords::table.into_boxed();
155150

156151
if sort == "crates" {
157152
query = query.order(keywords::crates_cnt.desc());
158153
} else {
159154
query = query.order(keywords::keyword.asc());
160155
}
161156

162-
let data = query.load::<(Keyword, i64)>(&*conn)?;
157+
let data = query.paginate(limit, offset).load::<(Keyword, i64)>(&*conn)?;
163158
let total = data.get(0).map(|&(_, t)| t).unwrap_or(0);
164159
let kws = data.into_iter()
165160
.map(|(k, _)| k.encodable())

src/krate.rs

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ use download::{VersionDownload, EncodableVersionDownload};
2929
use git;
3030
use keyword::{EncodableKeyword, CrateKeyword};
3131
use owner::{EncodableOwner, Owner, Rights, OwnerKind, Team, rights, CrateOwner};
32+
use pagination::Paginate;
3233
use schema::*;
3334
use upload;
3435
use user::RequestUser;
@@ -781,22 +782,16 @@ impl Model for Crate {
781782
/// Handles the `GET /crates` route.
782783
#[allow(trivial_casts)]
783784
pub fn index(req: &mut Request) -> CargoResult<Response> {
784-
use diesel::expression::dsl::sql;
785-
use diesel::types::{BigInt, Bool};
785+
use diesel::expression::AsExpression;
786+
use diesel::types::Bool;
786787

787788
let conn = req.db_conn()?;
788789
let (offset, limit) = req.pagination(10, 100)?;
789790
let params = req.query();
790791
let sort = params.get("sort").map(|s| &**s).unwrap_or("alpha");
791792

792793
let mut query = crates::table
793-
.select((
794-
ALL_COLUMNS,
795-
sql::<BigInt>("COUNT(*) OVER ()"),
796-
sql::<Bool>("false"),
797-
))
798-
.limit(limit)
799-
.offset(offset)
794+
.select((ALL_COLUMNS, AsExpression::<Bool>::as_expression(false)))
800795
.into_boxed();
801796

802797
if sort == "downloads" {
@@ -813,11 +808,7 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
813808
),
814809
));
815810

816-
query = query.select((
817-
ALL_COLUMNS,
818-
sql::<BigInt>("COUNT(*) OVER()"),
819-
crates::name.eq(q_string),
820-
));
811+
query = query.select((ALL_COLUMNS, crates::name.eq(q_string)));
821812
let perfect_match = crates::name.eq(q_string).desc();
822813
if sort == "downloads" {
823814
query = query.order((perfect_match, crates::downloads.desc()));
@@ -887,12 +878,10 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
887878
));
888879
}
889880

890-
let data = query.load::<(Crate, i64, bool)>(&*conn)?;
891-
let total = data.get(0).map(|&(_, t, _)| t).unwrap_or(0);
892-
let crates = data.iter()
893-
.map(|&(ref c, _, _)| c.clone())
894-
.collect::<Vec<_>>();
895-
let perfect_matches = data.into_iter().map(|(_, _, b)| b).collect::<Vec<_>>();
881+
let data = query.paginate(limit, offset).load::<((Crate, bool), i64)>(&*conn)?;
882+
let total = data.first().map(|&(_, t)| t).unwrap_or(0);
883+
let crates = data.iter().map(|&((ref c, _), _)| c.clone()).collect::<Vec<_>>();
884+
let perfect_matches = data.into_iter().map(|((_, b), _)| b).collect::<Vec<_>>();
896885

897886
let versions = Version::belonging_to(&crates)
898887
.load::<Version>(&*conn)?

src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ pub mod user;
9292
pub mod util;
9393
pub mod version;
9494

95+
mod pagination;
96+
9597
#[derive(PartialEq, Eq, Clone, Copy)]
9698
pub enum Env {
9799
Development,

src/pagination.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
use diesel::prelude::*;
2+
use diesel::query_builder::*;
3+
use diesel::types::BigInt;
4+
use diesel::pg::Pg;
5+
6+
pub struct Paginated<T> {
7+
query: T,
8+
limit: i64,
9+
offset: i64,
10+
}
11+
12+
pub trait Paginate: AsQuery + Sized {
13+
fn paginate(self, limit: i64, offset: i64) -> Paginated<Self::Query> {
14+
Paginated {
15+
query: self.as_query(),
16+
limit,
17+
offset,
18+
}
19+
}
20+
}
21+
22+
impl<T: AsQuery> Paginate for T {}
23+
24+
impl<T: Query> Query for Paginated<T> {
25+
type SqlType = (T::SqlType, BigInt);
26+
}
27+
28+
impl<T> QueryFragment<Pg> for Paginated<T> where
29+
T: QueryFragment<Pg>,
30+
{
31+
fn walk_ast(&self, mut out: AstPass<Pg>) -> QueryResult<()> {
32+
out.push_sql("SELECT *, COUNT(*) OVER () FROM (");
33+
self.query.walk_ast(out.reborrow())?;
34+
out.push_sql(") t LIMIT ");
35+
out.push_bind_param::<BigInt, _>(&self.limit)?;
36+
out.push_sql(" OFFSET ");
37+
out.push_bind_param::<BigInt, _>(&self.offset)?;
38+
Ok(())
39+
}
40+
}
41+
42+
impl_query_id!(Paginated<T>);

src/user/mod.rs

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use std::borrow::Cow;
1111
use app::RequestApp;
1212
use db::RequestTransaction;
1313
use krate::Follow;
14+
use pagination::Paginate;
1415
use schema::*;
1516
use util::errors::NotFound;
1617
use util::{RequestUtils, CargoResult, internal, ChainError, human};
@@ -377,8 +378,7 @@ pub fn show_team(req: &mut Request) -> CargoResult<Response> {
377378

378379
/// Handles the `GET /me/updates` route.
379380
pub fn updates(req: &mut Request) -> CargoResult<Response> {
380-
use diesel::expression::dsl::{any, sql};
381-
use diesel::types::BigInt;
381+
use diesel::expression::dsl::any;
382382

383383
let user = req.user()?;
384384
let (offset, limit) = req.pagination(10, 100)?;
@@ -389,22 +389,20 @@ pub fn updates(req: &mut Request) -> CargoResult<Response> {
389389
.inner_join(crates::table)
390390
.filter(crates::id.eq(any(followed_crates)))
391391
.order(versions::created_at.desc())
392-
.limit(limit)
393-
.offset(offset)
394392
.select((
395393
versions::all_columns,
396394
crates::name,
397-
sql::<BigInt>("COUNT(*) OVER ()"),
398395
))
399-
.load::<(Version, String, i64)>(&*conn)?;
396+
.paginate(limit, offset)
397+
.load::<((Version, String), i64)>(&*conn)?;
400398

401399
let more = data.get(0)
402-
.map(|&(_, _, count)| count > offset + limit)
400+
.map(|&(_, count)| count > offset + limit)
403401
.unwrap_or(false);
404402

405-
let versions = data.into_iter()
406-
.map(|(version, crate_name, _)| version.encodable(&crate_name))
407-
.collect();
403+
let versions = data.into_iter().map(|((version, crate_name), _)| {
404+
version.encodable(&crate_name)
405+
}).collect();
408406

409407
#[derive(RustcEncodable)]
410408
struct R {

0 commit comments

Comments
 (0)