Rename a few poorly named methods, add some transactions

All the methods which do >1 query which modifies data should be wrapped
in a transaction

Author: sgrif

src/category.rs

@@ -97,19 +97,21 @@ impl Category {
                             slugs: &[&'a str]) -> QueryResult<Vec<&'a str>> {
         use diesel::expression::dsl::any;
 
-        let categories = categories::table
-            .filter(categories::slug.eq(any(slugs)))
-            .load::<Category>(conn)?;
-        let invalid_categories = slugs.iter().cloned()
-            .filter(|s| !categories.iter().any(|c| c.slug == *s))
-            .collect();
-        let crate_categories = categories.iter()
-            .map(|c| CrateCategory { category_id: c.id, crate_id: krate.id })
-            .collect::<Vec<_>>();
-
-        delete(CrateCategory::belonging_to(krate)).execute(conn)?;
-        insert(&crate_categories).into(crates_categories::table).execute(conn)?;
-        Ok(invalid_categories)
+        conn.transaction(|| {
+            let categories = categories::table
+                .filter(categories::slug.eq(any(slugs)))
+                .load::<Category>(conn)?;
+            let invalid_categories = slugs.iter().cloned()
+                .filter(|s| !categories.iter().any(|c| c.slug == *s))
+                .collect();
+            let crate_categories = categories.iter()
+                .map(|c| CrateCategory { category_id: c.id, crate_id: krate.id })
+                .collect::<Vec<_>>();
+
+            delete(CrateCategory::belonging_to(krate)).execute(conn)?;
+            insert(&crate_categories).into(crates_categories::table).execute(conn)?;
+            Ok(invalid_categories)
+        })
     }
 
     pub fn update_crate_old(conn: &GenericConnection,
@@ -237,7 +239,7 @@ pub struct NewCategory<'a> {
 }
 
 impl<'a> NewCategory<'a> {
-    pub fn create_or_update(&self, conn: &PgConnection) -> QueryResult<Category> {
+    pub fn find_or_create(&self, conn: &PgConnection) -> QueryResult<Category> {
         use schema::categories::dsl::*;
         use diesel::pg::upsert::*;
 

src/keyword.rs

@@ -52,7 +52,7 @@ impl Keyword {
         Ok(rows.iter().next().map(|r| Model::from_row(&r)))
     }
 
-    pub fn find_all(conn: &PgConnection, names: &[&str]) -> QueryResult<Vec<Keyword>> {
+    pub fn find_or_create_all(conn: &PgConnection, names: &[&str]) -> QueryResult<Vec<Keyword>> {
         use diesel::pg::upsert::*;
         use diesel::expression::dsl::any;
 
@@ -130,15 +130,17 @@ impl Keyword {
     pub fn update_crate(conn: &PgConnection,
                         krate: &Crate,
                         keywords: &[&str]) -> QueryResult<()> {
-        let keywords = Keyword::find_all(conn, keywords)?;
-        diesel::delete(CrateKeyword::belonging_to(krate))
-            .execute(conn)?;
-        let crate_keywords = keywords.into_iter().map(|kw| {
-            CrateKeyword { crate_id: krate.id, keyword_id: kw.id }
-        }).collect::<Vec<_>>();
-        diesel::insert(&crate_keywords).into(crates_keywords::table)
-            .execute(conn)?;
-        Ok(())
+        conn.transaction(|| {
+            let keywords = Keyword::find_or_create_all(conn, keywords)?;
+            diesel::delete(CrateKeyword::belonging_to(krate))
+                .execute(conn)?;
+            let crate_keywords = keywords.into_iter().map(|kw| {
+                CrateKeyword { crate_id: krate.id, keyword_id: kw.id }
+            }).collect::<Vec<_>>();
+            diesel::insert(&crate_keywords).into(crates_keywords::table)
+                .execute(conn)?;
+            Ok(())
+        })
     }
 
     pub fn update_crate_old(conn: &GenericConnection,

src/krate.rs

@@ -124,22 +124,24 @@ impl<'a> NewCrate<'a> {
         self.validate(license_file)?;
         self.ensure_name_not_reserved(conn)?;
 
-        // To avoid race conditions, we try to insert
-        // first so we know whether to add an owner
-        if let Some(krate) = self.save_new_crate(conn, uploader)? {
-            return Ok(krate)
-        }
-
-        // We don't want to change the max_upload_size
-        self.max_upload_size = None;
+        conn.transaction(|| {
+            // To avoid race conditions, we try to insert
+            // first so we know whether to add an owner
+            if let Some(krate) = self.save_new_crate(conn, uploader)? {
+                return Ok(krate)
+            }
 
-        let target = crates::table.filter(
-            canon_crate_name(crates::name)
-                .eq(canon_crate_name(self.name)));
-        update(target).set(&self)
-            .returning(ALL_COLUMNS)
-            .get_result(conn)
-            .map_err(Into::into)
+            // We don't want to change the max_upload_size
+            self.max_upload_size = None;
+
+            let target = crates::table.filter(
+                canon_crate_name(crates::name)
+                    .eq(canon_crate_name(self.name)));
+            update(target).set(&self)
+                .returning(ALL_COLUMNS)
+                .get_result(conn)
+                .map_err(Into::into)
+        })
     }
 
     fn validate(&mut self, license_file: Option<&str>) -> CargoResult<()> {
@@ -206,23 +208,25 @@ impl<'a> NewCrate<'a> {
         use schema::crates::dsl::*;
         use diesel::insert;
 
-        let maybe_inserted = insert(&self.on_conflict_do_nothing()).into(crates)
-            .returning(ALL_COLUMNS)
-            .get_result::<Crate>(conn)
-            .optional()?;
-
-        if let Some(ref krate) = maybe_inserted {
-            let owner = CrateOwner {
-                crate_id: krate.id,
-                owner_id: user_id,
-                created_by: user_id,
-                owner_kind: OwnerKind::User as i32,
-            };
-            insert(&owner).into(crate_owners::table)
-                .execute(conn)?;
-        }
+        conn.transaction(|| {
+            let maybe_inserted = insert(&self.on_conflict_do_nothing()).into(crates)
+                .returning(ALL_COLUMNS)
+                .get_result::<Crate>(conn)
+                .optional()?;
+
+            if let Some(ref krate) = maybe_inserted {
+                let owner = CrateOwner {
+                    crate_id: krate.id,
+                    owner_id: user_id,
+                    created_by: user_id,
+                    owner_kind: OwnerKind::User as i32,
+                };
+                insert(&owner).into(crate_owners::table)
+                    .execute(conn)?;
+            }
 
-        Ok(maybe_inserted)
+            Ok(maybe_inserted)
+        })
     }
 }
 

src/tests/krate.rs

@@ -143,8 +143,8 @@ fn index_queries() {
     let mut response = ok_resp!(middle.call(req.with_query("keyword=kw2")));
     assert_eq!(::json::<CrateList>(&mut response).crates.len(), 0);
 
-    ::new_category("cat1", "cat1").create_or_update(req.db_conn().unwrap()).unwrap();
-    ::new_category("cat1::bar", "cat1::bar").create_or_update(req.db_conn().unwrap()).unwrap();
+    ::new_category("cat1", "cat1").find_or_create(req.db_conn().unwrap()).unwrap();
+    ::new_category("cat1::bar", "cat1::bar").find_or_create(req.db_conn().unwrap()).unwrap();
     Category::update_crate(req.db_conn().unwrap(), &krate, &["cat1"]).unwrap();
     Category::update_crate(req.db_conn().unwrap(), &krate2, &["cat1::bar"]).unwrap();
     let mut response = ok_resp!(middle.call(req.with_query("category=cat1")));