From 2afe32e99cccfe7d20bb45b561e31ddf5023e89f Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 21 Jun 2017 19:25:00 -0400 Subject: [PATCH 1/2] Switch several category routes over to diesel --- src/category.rs | 96 +++++++++++++++++++------------------ src/tests/all.rs | 37 +-------------- src/tests/category.rs | 108 ++++++++++++++++++++++++++++-------------- src/tests/krate.rs | 6 +-- 4 files changed, 126 insertions(+), 121 deletions(-) diff --git a/src/category.rs b/src/category.rs index 90a8839bc70..3156364736e 100644 --- a/src/category.rs +++ b/src/category.rs @@ -193,17 +193,18 @@ impl Category { Ok(invalid_categories) } - pub fn count_toplevel(conn: &GenericConnection) -> CargoResult { - let sql = format!( + pub fn count_toplevel(conn: &PgConnection) -> CargoResult { + use diesel::expression::*; + use diesel::types::*; + + let query = sql::( "\ - SELECT COUNT(*) \ - FROM {} \ - WHERE category NOT LIKE '%::%'", - Model::table_name(None::) + 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,27 +270,31 @@ impl Category { Ok(categories) } - pub fn subcategories(&self, conn: &GenericConnection) -> CargoResult> { - 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> { + 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::(&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, @@ -297,20 +302,15 @@ pub struct NewCategory<'a> { } impl<'a> NewCategory<'a> { - pub fn find_or_create(&self, conn: &PgConnection) -> QueryResult { - use schema::categories::dsl::*; + /// Inserts the category into the database, or updates an existing one. + pub fn create_or_update(&self, conn: &PgConnection) -> CargoResult { + 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 { - 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 { /// Handles the `GET /categories/:category_id` route. pub fn show(req: &mut Request) -> CargoResult { - 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::(&*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, diff --git a/src/tests/all.rs b/src/tests/all.rs index b93af52ad38..281cd7afecf 100644 --- a/src/tests/all.rs +++ b/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::().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::(); } diff --git a/src/tests/category.rs b/src/tests/category.rs index 511b0b2c131..57111bba01c 100644 --- a/src/tests/category.rs +++ b/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::(&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); diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 83fa3600d0b..ee476c02dc5 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -189,10 +189,10 @@ fn index_queries() { { let conn = app.diesel_database.get().unwrap(); ::new_category("Category 1", "cat1") - .find_or_create(&conn) + .create_or_update(&conn) .unwrap(); ::new_category("Category 1::Ba'r", "cat1::bar") - .find_or_create(&conn) + .create_or_update(&conn) .unwrap(); Category::update_crate(&conn, &krate, &["cat1"]).unwrap(); Category::update_crate(&conn, &krate2, &["cat1::bar"]).unwrap(); @@ -1510,7 +1510,7 @@ fn good_categories() { { let conn = app.diesel_database.get().unwrap(); ::new_category("Category 1", "cat1") - .find_or_create(&conn) + .create_or_update(&conn) .unwrap(); } let mut response = ok_resp!(middle.call(&mut req)); From 60fe8ed27a180dd8da8aff787e2d0bf6b5723aa0 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 21 Jun 2017 20:12:09 -0400 Subject: [PATCH 2/2] `cargo fmt` --- src/category.rs | 9 +++++---- src/tests/category.rs | 31 ++++++++++++------------------- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/category.rs b/src/category.rs index 3156364736e..294980fd421 100644 --- a/src/category.rs +++ b/src/category.rs @@ -201,7 +201,7 @@ impl Category { "\ SELECT COUNT(*) \ FROM categories \ - WHERE category NOT LIKE '%::%'" + WHERE category NOT LIKE '%::%'", ); let count = query.get_result(&*conn)?; Ok(count) @@ -366,9 +366,10 @@ pub fn show(req: &mut Request) -> CargoResult { let id = &req.params()["category_id"]; let conn = req.db_conn()?; let cat = categories.filter(slug.eq(id)).first::(&*conn)?; - let subcats = cat.subcategories(&*conn)?.into_iter().map(|s| { - s.encodable() - }).collect(); + let subcats = cat.subcategories(&*conn)? + .into_iter() + .map(|s| s.encodable()) + .collect(); let cat = cat.encodable(); let cat_with_subcats = EncodableCategoryWithSubcategories { diff --git a/src/tests/category.rs b/src/tests/category.rs index 57111bba01c..13eed196da5 100644 --- a/src/tests/category.rs +++ b/src/tests/category.rs @@ -35,8 +35,12 @@ fn index() { // Create a category and a subcategory { 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(); + ::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); @@ -89,8 +93,7 @@ fn update_crate() { 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) + ::CrateBuilder::new("foo_crate", user.id).expect_build(&conn) }; // Updating with no categories has no effect @@ -128,11 +131,7 @@ fn update_crate() { // Adding 2 categories { let conn = t!(app.diesel_database.get()); - Category::update_crate( - &conn, - &krate, - &["cat1", "category-2"], - ).unwrap(); + Category::update_crate(&conn, &krate, &["cat1", "category-2"]).unwrap(); } assert_eq!(cnt(&mut req, "cat1"), 1); assert_eq!(cnt(&mut req, "category-2"), 1); @@ -148,11 +147,7 @@ fn update_crate() { // Attempting to add one valid category and one invalid category let invalid_categories = { let conn = t!(app.diesel_database.get()); - Category::update_crate( - &conn, - &krate, - &["cat1", "catnope"], - ).unwrap() + Category::update_crate(&conn, &krate, &["cat1", "catnope"]).unwrap() }; assert_eq!(invalid_categories, vec!["catnope".to_string()]); assert_eq!(cnt(&mut req, "cat1"), 1); @@ -177,12 +172,10 @@ fn update_crate() { // Add a category and its subcategory { let conn = t!(app.diesel_database.get()); - t!(::new_category("cat1::bar", "cat1::bar").create_or_update(&conn)); - Category::update_crate( + t!(::new_category("cat1::bar", "cat1::bar").create_or_update( &conn, - &krate, - &["cat1", "cat1::bar"], - ).unwrap(); + )); + Category::update_crate(&conn, &krate, &["cat1", "cat1::bar"]).unwrap(); } assert_eq!(cnt(&mut req, "cat1"), 1); assert_eq!(cnt(&mut req, "cat1::bar"), 1);