Switch several category routes over to diesel

Author: jtgeibel

src/category.rs

@@ -193,17 +193,18 @@ impl Category {
         Ok(invalid_categories)
     }
 
-    pub fn count_toplevel(conn: &GenericConnection) -> CargoResult<i64> {
-        let sql = format!(
+    pub fn count_toplevel(conn: &PgConnection) -> CargoResult<i64> {
+        use diesel::expression::*;
+        use diesel::types::*;
+
+        let query = sql::<BigInt>(
             "\
-             SELECT COUNT(*) \
-             FROM {} \
-             WHERE category NOT LIKE '%::%'",
-            Model::table_name(None::<Self>)
+            SELECT COUNT(*) \
+            FROM categories \
+            WHERE category NOT LIKE '%::%'"
         );
-        let stmt = conn.prepare(&sql)?;
-        let rows = stmt.query(&[])?;
-        Ok(rows.iter().next().unwrap().get("count"))
+        let count = query.get_result(&*conn)?;
+        Ok(count)
     }
 
     pub fn toplevel(
@@ -269,48 +270,47 @@ impl Category {
         Ok(categories)
     }
 
-    pub fn subcategories(&self, conn: &GenericConnection) -> CargoResult<Vec<Category>> {
-        let stmt = conn.prepare(
-            "\
-             SELECT c.id, c.category, c.slug, c.description, c.created_at, \
-             COALESCE (( \
-             SELECT sum(c2.crates_cnt)::int \
-             FROM categories as c2 \
-             WHERE c2.slug = c.slug \
-             OR c2.slug LIKE c.slug || '::%' \
-             ), 0) as crates_cnt \
-             FROM categories as c \
-             WHERE c.category ILIKE $1 || '::%' \
-             AND c.category NOT ILIKE $1 || '::%::%'",
-        )?;
+    pub fn subcategories(&self, conn: &PgConnection) -> CargoResult<Vec<Category>> {
+        use diesel::expression::*;
+        use diesel::types::*;
 
-        let rows = stmt.query(&[&self.category])?;
-        Ok(rows.iter().map(|r| Model::from_row(&r)).collect())
+        let query = sql::<(Integer, Text, Text, Text, Integer, Timestamp)>(
+            "\
+            SELECT c.id, c.category, c.slug, c.description, \
+            COALESCE (( \
+                SELECT sum(c2.crates_cnt)::int \
+                FROM categories as c2 \
+                WHERE c2.slug = c.slug \
+                OR c2.slug LIKE c.slug || '::%' \
+            ), 0) as crates_cnt, \
+            c.created_at \
+            FROM categories as c \
+            WHERE c.category ILIKE $1 || '::%' \
+            AND c.category NOT ILIKE $1 || '::%::%'",
+        ).bind::<Text, _>(&self.category);
+
+        let rows = query.get_results(conn)?;
+        Ok(rows)
     }
 }
 
-#[derive(Insertable, Default, Debug)]
+#[derive(Insertable, AsChangeset, Default, Debug)]
 #[table_name = "categories"]
 pub struct NewCategory<'a> {
     pub category: &'a str,
     pub slug: &'a str,
 }
 
 impl<'a> NewCategory<'a> {
-    pub fn find_or_create(&self, conn: &PgConnection) -> QueryResult<Category> {
-        use schema::categories::dsl::*;
+    /// Inserts the category into the database, or updates an existing one.
+    pub fn create_or_update(&self, conn: &PgConnection) -> CargoResult<Category> {
+        use diesel::insert;
         use diesel::pg::upsert::*;
 
-        let maybe_inserted = insert(&self.on_conflict_do_nothing())
-            .into(categories)
+        insert(&self.on_conflict(categories::slug, do_update().set(self)))
+            .into(categories::table)
             .get_result(conn)
-            .optional()?;
-
-        if let Some(c) = maybe_inserted {
-            return Ok(c);
-        }
-
-        categories.filter(slug.eq(self.slug)).first(conn)
+            .map_err(Into::into)
     }
 }
 
@@ -332,16 +332,16 @@ impl Model for Category {
 
 /// Handles the `GET /categories` route.
 pub fn index(req: &mut Request) -> CargoResult<Response> {
-    let conn = req.tx()?;
+    let conn = req.db_conn()?;
     let (offset, limit) = req.pagination(10, 100)?;
     let query = req.query();
     let sort = query.get("sort").map_or("alpha", String::as_str);
 
-    let categories = Category::toplevel_old(conn, sort, limit, offset)?;
+    let categories = Category::toplevel(&conn, sort, limit, offset)?;
     let categories = categories.into_iter().map(Category::encodable).collect();
 
     // Query for the total count of categories
-    let total = Category::count_toplevel(conn)?;
+    let total = Category::count_toplevel(&conn)?;
 
     #[derive(RustcEncodable)]
     struct R {
@@ -361,13 +361,15 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
 
 /// Handles the `GET /categories/:category_id` route.
 pub fn show(req: &mut Request) -> CargoResult<Response> {
-    let slug = &req.params()["category_id"];
-    let conn = req.tx()?;
-    let cat = Category::find_by_slug(conn, slug)?;
-    let subcats = cat.subcategories(conn)?
-        .into_iter()
-        .map(|s| s.encodable())
-        .collect();
+    use self::categories::dsl::{categories, slug};
+
+    let id = &req.params()["category_id"];
+    let conn = req.db_conn()?;
+    let cat = categories.filter(slug.eq(id)).first::<Category>(&*conn)?;
+    let subcats = cat.subcategories(&*conn)?.into_iter().map(|s| {
+        s.encodable()
+    }).collect();
+
     let cat = cat.encodable();
     let cat_with_subcats = EncodableCategoryWithSubcategories {
         id: cat.id,

src/tests/all.rs

@@ -35,7 +35,7 @@ use cargo_registry::user::NewUser;
 use cargo_registry::owner::{CrateOwner, NewTeam, Team};
 use cargo_registry::version::NewVersion;
 use cargo_registry::user::AuthenticationSource;
-use cargo_registry::{User, Crate, Version, Dependency, Category, Model, Replica};
+use cargo_registry::{User, Crate, Version, Dependency, Replica};
 use conduit::{Request, Method};
 use conduit_test::MockRequest;
 use diesel::pg::PgConnection;
@@ -467,29 +467,6 @@ fn sign_in(req: &mut Request, app: &App) {
     sign_in_as(req, &user);
 }
 
-fn mock_crate(req: &mut Request, krate: Crate) -> (Crate, Version) {
-    mock_crate_vers(req, krate, &semver::Version::parse("1.0.0").unwrap())
-}
-
-fn mock_crate_vers(req: &mut Request, krate: Crate, v: &semver::Version) -> (Crate, Version) {
-    let user = req.extensions().find::<User>().unwrap();
-    let mut krate = Crate::find_or_insert(
-        req.tx().unwrap(),
-        &krate.name,
-        user.id,
-        &krate.description,
-        &krate.homepage,
-        &krate.documentation,
-        &krate.readme,
-        &krate.repository,
-        &krate.license,
-        &None,
-        krate.max_upload_size,
-    ).unwrap();
-    let v = krate.add_version(req.tx().unwrap(), v, &HashMap::new(), &[]);
-    (krate, v.unwrap())
-}
-
 fn new_dependency(conn: &PgConnection, version: &Version, krate: &Crate) -> Dependency {
     use diesel::insert;
     use cargo_registry::schema::dependencies;
@@ -515,18 +492,6 @@ fn new_category<'a>(category: &'a str, slug: &'a str) -> NewCategory<'a> {
     }
 }
 
-fn mock_category(req: &mut Request, name: &str, slug: &str) -> Category {
-    let conn = req.tx().unwrap();
-    let stmt = conn.prepare(
-        " \
-         INSERT INTO categories (category, slug) \
-         VALUES ($1, $2) \
-         RETURNING *",
-    ).unwrap();
-    let rows = stmt.query(&[&name, &slug]).unwrap();
-    Model::from_row(&rows.iter().next().unwrap())
-}
-
 fn logout(req: &mut Request) {
     req.mut_extensions().pop::<User>();
 }

src/tests/category.rs

@@ -1,7 +1,6 @@
 use conduit::{Handler, Method};
 use conduit_test::MockRequest;
 
-use cargo_registry::db::RequestTransaction;
 use cargo_registry::category::{Category, EncodableCategory, EncodableCategoryWithSubcategories};
 
 #[derive(RustcDecodable)]
@@ -25,7 +24,7 @@ struct CategoryWithSubcategories {
 #[test]
 fn index() {
     let (_b, app, middle) = ::app();
-    let mut req = ::req(app, Method::Get, "/api/v1/categories");
+    let mut req = ::req(app.clone(), Method::Get, "/api/v1/categories");
 
     // List 0 categories if none exist
     let mut response = ok_resp!(middle.call(&mut req));
@@ -34,9 +33,11 @@ fn index() {
     assert_eq!(json.meta.total, 0);
 
     // Create a category and a subcategory
-    ::mock_category(&mut req, "foo", "foo");
-    ::mock_category(&mut req, "foo::bar", "foo::bar");
-
+    {
+        let conn = t!(app.diesel_database.get());
+        ::new_category("foo", "foo").create_or_update(&conn).unwrap();
+        ::new_category("foo::bar", "foo::bar").create_or_update(&conn).unwrap();
+    }
     let mut response = ok_resp!(middle.call(&mut req));
     let json: CategoryList = ::json(&mut response);
 
@@ -51,13 +52,18 @@ fn show() {
     let (_b, app, middle) = ::app();
 
     // Return not found if a category doesn't exist
-    let mut req = ::req(app, Method::Get, "/api/v1/categories/foo-bar");
+    let mut req = ::req(app.clone(), Method::Get, "/api/v1/categories/foo-bar");
     let response = t_resp!(middle.call(&mut req));
     assert_eq!(response.status.0, 404);
 
     // Create a category and a subcategory
-    ::mock_category(&mut req, "Foo Bar", "foo-bar");
-    ::mock_category(&mut req, "Foo Bar::Baz", "foo-bar::baz");
+    {
+        let conn = t!(app.diesel_database.get());
+
+        // Create a category and a subcategory
+        t!(::new_category("Foo Bar", "foo-bar").create_or_update(&conn));
+        t!(::new_category("Foo Bar::Baz", "foo-bar::baz").create_or_update(&conn));
+    }
 
     // The category and its subcategories should be in the json
     let mut response = ok_resp!(middle.call(&mut req));
@@ -71,57 +77,83 @@ fn show() {
 #[test]
 fn update_crate() {
     let (_b, app, middle) = ::app();
-    let mut req = ::req(app, Method::Get, "/api/v1/categories/foo");
+    let mut req = ::req(app.clone(), Method::Get, "/api/v1/categories/foo");
     let cnt = |req: &mut MockRequest, cat: &str| {
         req.with_path(&format!("/api/v1/categories/{}", cat));
         let mut response = ok_resp!(middle.call(req));
         ::json::<GoodCategory>(&mut response).category.crates_cnt as usize
     };
-    ::mock_user(&mut req, ::user("foo"));
-    let (krate, _) = ::mock_crate(&mut req, ::krate("foocat"));
-    ::mock_category(&mut req, "cat1", "cat1");
-    ::mock_category(&mut req, "Category 2", "category-2");
+
+    let krate = {
+        let conn = t!(app.diesel_database.get());
+        let user = t!(::new_user("foo").create_or_update(&conn));
+        t!(::new_category("cat1", "cat1").create_or_update(&conn));
+        t!(::new_category("Category 2", "category-2").create_or_update(&conn));
+        ::CrateBuilder::new("foo_crate", user.id)
+            .expect_build(&conn)
+    };
 
     // Updating with no categories has no effect
-    Category::update_crate_old(req.tx().unwrap(), &krate, &[]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &[]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 0);
     assert_eq!(cnt(&mut req, "category-2"), 0);
 
     // Happy path adding one category
-    Category::update_crate_old(req.tx().unwrap(), &krate, &["cat1".to_string()]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &["cat1"]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 1);
     assert_eq!(cnt(&mut req, "category-2"), 0);
 
     // Replacing one category with another
-    Category::update_crate_old(req.tx().unwrap(), &krate, &["category-2".to_string()]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &["category-2"]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 0);
     assert_eq!(cnt(&mut req, "category-2"), 1);
 
     // Removing one category
-    Category::update_crate_old(req.tx().unwrap(), &krate, &[]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &[]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 0);
     assert_eq!(cnt(&mut req, "category-2"), 0);
 
     // Adding 2 categories
-    Category::update_crate_old(
-        req.tx().unwrap(),
-        &krate,
-        &["cat1".to_string(), "category-2".to_string()],
-    ).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(
+            &conn,
+            &krate,
+            &["cat1", "category-2"],
+        ).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 1);
     assert_eq!(cnt(&mut req, "category-2"), 1);
 
     // Removing all categories
-    Category::update_crate_old(req.tx().unwrap(), &krate, &[]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &[]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 0);
     assert_eq!(cnt(&mut req, "category-2"), 0);
 
     // Attempting to add one valid category and one invalid category
-    let invalid_categories = Category::update_crate_old(
-        req.tx().unwrap(),
-        &krate,
-        &["cat1".to_string(), "catnope".to_string()],
-    ).unwrap();
+    let invalid_categories = {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(
+            &conn,
+            &krate,
+            &["cat1", "catnope"],
+        ).unwrap()
+    };
     assert_eq!(invalid_categories, vec!["catnope".to_string()]);
     assert_eq!(cnt(&mut req, "cat1"), 1);
     assert_eq!(cnt(&mut req, "category-2"), 0);
@@ -135,17 +167,23 @@ fn update_crate() {
     assert_eq!(json.meta.total, 2);
 
     // Attempting to add a category by display text; must use slug
-    Category::update_crate_old(req.tx().unwrap(), &krate, &["Category 2".to_string()]).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        Category::update_crate(&conn, &krate, &["Category 2"]).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 0);
     assert_eq!(cnt(&mut req, "category-2"), 0);
 
     // Add a category and its subcategory
-    ::mock_category(&mut req, "cat1::bar", "cat1::bar");
-    Category::update_crate_old(
-        req.tx().unwrap(),
-        &krate,
-        &["cat1".to_string(), "cat1::bar".to_string()],
-    ).unwrap();
+    {
+        let conn = t!(app.diesel_database.get());
+        t!(::new_category("cat1::bar", "cat1::bar").create_or_update(&conn));
+        Category::update_crate(
+            &conn,
+            &krate,
+            &["cat1", "cat1::bar"],
+        ).unwrap();
+    }
     assert_eq!(cnt(&mut req, "cat1"), 1);
     assert_eq!(cnt(&mut req, "cat1::bar"), 1);
     assert_eq!(cnt(&mut req, "category-2"), 0);