Merge remote-tracking branch 'upstream/master' into newBadgesAdd

carols10cents · carols10cents · commit f8c700365149 · 2017-05-14T10:00:45.000-04:00
diff --git a/src/app.rs b/src/app.rs
@@ -49,10 +49,11 @@ impl App {
 
         let db_config = r2d2::Config::builder()
             .pool_size(if config.env == ::Env::Production {10} else {1})
+            .min_idle(if config.env == ::Env::Production {Some(5)} else {None})
             .helper_threads(if config.env == ::Env::Production {3} else {1})
             .build();
         let diesel_db_config = r2d2::Config::builder()
-            .pool_size(if config.env == ::Env::Production {50} else {1})
+            .pool_size(if config.env == ::Env::Production {30} else {1})
             .min_idle(if config.env == ::Env::Production {Some(5)} else {None})
             .helper_threads(if config.env == ::Env::Production {3} else {1})
             .build();
diff --git a/src/badge.rs b/src/badge.rs
@@ -1,12 +1,9 @@
-use Model;
 use krate::Crate;
 use schema::badges;
 use util::CargoResult;
 
 use diesel::pg::{Pg, PgConnection};
 use diesel::prelude::*;
-use pg::GenericConnection;
-use pg::rows::Row;
 use serde_json;
 use std::collections::HashMap;
 
@@ -59,17 +56,6 @@ impl Queryable<badges::SqlType, Pg> for Badge {
     }
 }
 
-impl Model for Badge {
-    fn from_row(row: &Row) -> Badge {
-        let badge_type: String = row.get("badge_type");
-        let attributes: serde_json::Value = row.get("attributes");
-        let json = json!({"badge_type": badge_type, "attributes": attributes});
-        serde_json::from_value(json)
-            .expect("Invalid CI badge in the database")
-    }
-    fn table_name(_: Option<Badge>) -> &'static str { "badges" }
-}
-
 impl Badge {
     pub fn encodable(self) -> EncodableBadge {
         serde_json::from_value(serde_json::to_value(self).unwrap()).unwrap()
@@ -127,37 +113,4 @@ impl Badge {
             Ok(invalid_badges)
         })
     }
-
-    pub fn update_crate_old(conn: &GenericConnection,
-                        krate: &Crate,
-                        badges: HashMap<String, HashMap<String, String>>)
-                        -> CargoResult<Vec<String>> {
-
-        let mut invalid_badges = vec![];
-
-        let badges: Vec<Badge> = badges.into_iter().filter_map(|(k, v)| {
-            let json = json!({"badge_type": k, "attributes": v});
-            serde_json::from_value(json)
-                .map_err(|_| invalid_badges.push(k))
-                .ok()
-        }).collect();
-
-        conn.execute("\
-            DELETE FROM badges \
-            WHERE crate_id = $1;",
-            &[&krate.id]
-        )?;
-
-        for badge in badges {
-            let json = serde_json::to_value(badge)?;
-            conn.execute("\
-                INSERT INTO badges (crate_id, badge_type, attributes) \
-                VALUES ($1, $2, $3) \
-                ON CONFLICT (crate_id, badge_type) DO UPDATE \
-                    SET attributes = EXCLUDED.attributes;",
-                &[&krate.id, &json["badge_type"].as_str(), &json["attributes"]]
-            )?;
-        }
-        Ok(invalid_badges)
-    }
 }
diff --git a/src/keyword.rs b/src/keyword.rs
@@ -1,20 +1,17 @@
 use std::ascii::AsciiExt;
-use std::collections::HashMap;
 use time::Timespec;
 
 use conduit::{Request, Response};
 use conduit_router::RequestParams;
 use diesel::pg::PgConnection;
 use diesel::prelude::*;
 use diesel;
-use pg::GenericConnection;
 use pg::rows::Row;
 
 use {Model, Crate};
 use db::RequestTransaction;
 use schema::*;
-use util::{RequestUtils, CargoResult, ChainError, internal};
-use util::errors::NotFound;
+use util::{RequestUtils, CargoResult};
 
 #[derive(Clone, Identifiable, Queryable)]
 pub struct Keyword {
@@ -43,12 +40,11 @@ pub struct EncodableKeyword {
 }
 
 impl Keyword {
-    pub fn find_by_keyword(conn: &GenericConnection, name: &str)
-                           -> CargoResult<Option<Keyword>> {
-        let stmt = conn.prepare("SELECT * FROM keywords \
-                                      WHERE keyword = LOWER($1)")?;
-        let rows = stmt.query(&[&name])?;
-        Ok(rows.iter().next().map(|r| Model::from_row(&r)))
+    pub fn find_by_keyword(conn: &PgConnection, name: &str)
+                           -> QueryResult<Keyword> {
+        keywords::table
+            .filter(keywords::keyword.eq(::lower(name)))
+            .first(&*conn)
     }
 
     pub fn find_or_create_all(conn: &PgConnection, names: &[&str]) -> QueryResult<Vec<Keyword>> {
@@ -60,58 +56,26 @@ impl Keyword {
         struct NewKeyword<'a> {
             keyword: &'a str,
         }
-        sql_function!(lower, lower_t, (x: ::diesel::types::Text) -> ::diesel::types::Text);
 
-        let (lowercase_names, new_keywords): (Vec<_>, Vec<_>) = names.iter()
-            .map(|s| (s.to_lowercase(), NewKeyword { keyword: *s }))
-            .unzip();
+        let lowercase_names: Vec<_> = names.iter()
+            .map(|s| s.to_lowercase())
+            .collect();
+
+        let new_keywords: Vec<_> = lowercase_names.iter()
+            .map(|s| NewKeyword { keyword: s })
+            .collect();
 
         // https://github.com/diesel-rs/diesel/issues/797
         if !new_keywords.is_empty() {
-            diesel::insert(&new_keywords.on_conflict_do_nothing()).into(keywords::table)
+            diesel::insert(&new_keywords.on_conflict_do_nothing())
+                .into(keywords::table)
                 .execute(conn)?;
         }
-        keywords::table.filter(lower(keywords::keyword).eq(any(lowercase_names)))
+        keywords::table
+            .filter(::lower(keywords::keyword).eq(any(&lowercase_names)))
             .load(conn)
     }
 
-    pub fn find_or_insert(conn: &GenericConnection, name: &str)
-                          -> CargoResult<Keyword> {
-        // TODO: racy (the select then insert is not atomic)
-        let stmt = conn.prepare("SELECT * FROM keywords
-                                      WHERE keyword = LOWER($1)")?;
-        for row in stmt.query(&[&name])?.iter() {
-            return Ok(Model::from_row(&row))
-        }
-
-        let stmt = conn.prepare("INSERT INTO keywords (keyword) VALUES (LOWER($1))
-                                      RETURNING *")?;
-        let rows = stmt.query(&[&name])?;
-        Ok(Model::from_row(&rows.iter().next().chain_error(|| {
-            internal("no version returned")
-        })?))
-    }
-
-    pub fn all(conn: &GenericConnection, sort: &str, limit: i64, offset: i64)
-               -> CargoResult<Vec<Keyword>> {
-
-        let sort_sql = match sort {
-           "crates" => "ORDER BY crates_cnt DESC",
-           _ => "ORDER BY keyword ASC",
-        };
-
-        let stmt = conn.prepare(&format!("SELECT * FROM keywords {}
-                                               LIMIT $1 OFFSET $2",
-                                         sort_sql))?;
-
-        let keywords: Vec<_> = stmt.query(&[&limit, &offset])?
-            .iter()
-            .map(|row| Model::from_row(&row))
-            .collect();
-
-        Ok(keywords)
-    }
-
     pub fn valid_name(name: &str) -> bool {
         if name.is_empty() { return false }
         name.chars().next().unwrap().is_alphanumeric() &&
@@ -144,47 +108,6 @@ impl Keyword {
             Ok(())
         })
     }
-
-    pub fn update_crate_old(conn: &GenericConnection,
-                        krate: &Crate,
-                        keywords: &[String]) -> CargoResult<()> {
-        let old_kws = krate.keywords(conn)?;
-        let old_kws = old_kws.iter().map(|kw| {
-            (&kw.keyword[..], kw)
-        }).collect::<HashMap<_, _>>();
-        let new_kws = keywords.iter().map(|k| {
-            let kw = Keyword::find_or_insert(conn, k)?;
-            Ok((k.as_str(), kw))
-        }).collect::<CargoResult<HashMap<_, _>>>()?;
-
-        let to_rm = old_kws.iter().filter(|&(kw, _)| {
-            !new_kws.contains_key(kw)
-        }).map(|(_, v)| v.id).collect::<Vec<_>>();
-        let to_add = new_kws.iter().filter(|&(kw, _)| {
-            !old_kws.contains_key(kw)
-        }).map(|(_, v)| v.id).collect::<Vec<_>>();
-
-        if !to_rm.is_empty() {
-            conn.execute("DELETE FROM crates_keywords
-                                WHERE keyword_id = ANY($1)
-                                  AND crate_id = $2",
-                         &[&to_rm, &krate.id])?;
-        }
-
-        if !to_add.is_empty() {
-            let insert = to_add.iter().map(|id| {
-                let crate_id: i32 = krate.id;
-                let id: i32 = *id;
-                format!("({}, {})", crate_id,  id)
-            }).collect::<Vec<_>>().join(", ");
-            conn.execute(&format!("INSERT INTO crates_keywords
-                                        (crate_id, keyword_id) VALUES {}",
-                                  insert),
-                         &[])?;
-        }
-
-        Ok(())
-    }
 }
 
 impl Model for Keyword {
@@ -201,34 +124,49 @@ impl Model for Keyword {
 
 /// Handles the `GET /keywords` route.
 pub fn index(req: &mut Request) -> CargoResult<Response> {
-    let conn = req.tx()?;
+    use diesel::expression::dsl::sql;
+    use diesel::types::BigInt;
+    use 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");
 
-    let keywords = Keyword::all(conn, sort, limit, offset)?;
-    let keywords = keywords.into_iter().map(Keyword::encodable).collect();
+    let mut query = keywords::table
+        .select((keywords::all_columns, sql::<BigInt>("COUNT(*) OVER ()")))
+        .limit(limit)
+        .offset(offset)
+        .into_boxed();
 
-    // Query for the total count of keywords
-    let total = Keyword::count(conn)?;
+    if sort == "crates" {
+        query = query.order(keywords::crates_cnt.desc());
+    } else {
+        query = query.order(keywords::keyword.asc());
+    }
+
+    let data = query.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::<Vec<_>>();
 
     #[derive(RustcEncodable)]
     struct R { keywords: Vec<EncodableKeyword>, meta: Meta }
     #[derive(RustcEncodable)]
     struct Meta { total: i64 }
 
     Ok(req.json(&R {
-        keywords: keywords,
+        keywords: kws,
         meta: Meta { total: total },
     }))
 }
 
 /// Handles the `GET /keywords/:keyword_id` route.
 pub fn show(req: &mut Request) -> CargoResult<Response> {
     let name = &req.params()["keyword_id"];
-    let conn = req.tx()?;
-    let kw = Keyword::find_by_keyword(conn, name)?;
-    let kw = kw.chain_error(|| NotFound)?;
+    let conn = req.db_conn()?;
+
+    let kw = Keyword::find_by_keyword(&conn, name)?;
 
     #[derive(RustcEncodable)]
     struct R { keyword: EncodableKeyword }
diff --git a/src/krate.rs b/src/krate.rs
@@ -609,11 +609,8 @@ impl Crate {
         Ok(rows.iter().map(|r| Model::from_row(&r)).collect())
     }
 
-    pub fn badges(&self, conn: &GenericConnection) -> CargoResult<Vec<Badge>> {
-        let stmt = conn.prepare("SELECT badges.* from badges \
-                                      WHERE badges.crate_id = $1")?;
-        let rows = stmt.query(&[&self.id])?;
-        Ok(rows.iter().map(|r| Model::from_row(&r)).collect())
+    pub fn badges(&self, conn: &PgConnection) -> QueryResult<Vec<Badge>> {
+        badges::table.filter(badges::crate_id.eq(self.id)).load(conn)
     }
 
     /// Returns (dependency, dependent crate name, dependent crate downloads)
@@ -709,7 +706,7 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
         query = query.filter(crates::id.eq_any(
             crates_keywords::table.select(crates_keywords::crate_id)
                 .inner_join(keywords::table)
-                .filter(lower(keywords::keyword).eq(lower(kw)))
+                .filter(::lower(keywords::keyword).eq(::lower(kw)))
         ));
     } else if let Some(letter) = params.get("letter") {
         let pattern = format!("{}%", letter.chars().next().unwrap()
@@ -1303,4 +1300,3 @@ pub fn reverse_dependencies(req: &mut Request) -> CargoResult<Response> {
 
 use diesel::types::Text;
 sql_function!(canon_crate_name, canon_crate_name_t, (x: Text) -> Text);
-sql_function!(lower, lower_t, (x: Text) -> Text);
diff --git a/src/lib.rs b/src/lib.rs
@@ -227,3 +227,5 @@ pub fn env(s: &str) -> String {
         panic!("must have `{}` defined", s)
     })
 }
+
+sql_function!(lower, lower_t, (x: ::diesel::types::Text) -> ::diesel::types::Text);
diff --git a/src/tests/all.rs b/src/tests/all.rs
@@ -32,7 +32,7 @@ use cargo_registry::krate::NewCrate;
 use cargo_registry::upload as u;
 use cargo_registry::user::NewUser;
 use cargo_registry::version::NewVersion;
-use cargo_registry::{User, Crate, Version, Keyword, Dependency, Category, Model, Replica};
+use cargo_registry::{User, Crate, Version, Dependency, Category, Model, Replica};
 use conduit::{Request, Method};
 use conduit_test::MockRequest;
 use diesel::pg::PgConnection;
@@ -296,10 +296,6 @@ fn mock_dep(req: &mut Request, version: &Version, krate: &Crate,
                        &target.map(|s| s.to_string())).unwrap()
 }
 
-fn mock_keyword(req: &mut Request, name: &str) -> Keyword {
-    Keyword::find_or_insert(req.tx().unwrap(), name).unwrap()
-}
-
 fn new_category<'a>(category: &'a str, slug: &'a str) -> NewCategory<'a> {
     NewCategory { category: category, slug: slug, ..NewCategory::default() }
 }
diff --git a/src/tests/badge.rs b/src/tests/badge.rs
diff --git a/src/tests/keyword.rs b/src/tests/keyword.rs

Original file line number	Diff line number	Diff line change
`@@ -227,3 +227,5 @@ pub fn env(s: &str) -> String {`
`227`	`227`	panic!("must have `{}` defined", s)
`228`	`228`	`})`
`229`	`229`	`}`
	`230`	`+`
	`231`	`+sql_function!(lower, lower_t, (x: ::diesel::types::Text) -> ::diesel::types::Text);`