Auto merge of #4120 - Turbo87:yank-db, r=Turbo87

worker::git: Move database update out of `yank()` job

When publishing, we also save the new version (and crate) directly in the database, so we should be able to do the same when yanking/unyanking, and avoid potentially returning incorrect data right after the request and before the background job has finished.

Author: bors

src/controllers/version/yank.rs

@@ -6,6 +6,7 @@ use super::{extract_crate_name_and_semver, version_and_crate};
 use crate::controllers::cargo_prelude::*;
 use crate::models::Rights;
 use crate::models::{insert_version_owner_action, VersionAction};
+use crate::schema::versions;
 use crate::worker;
 
 /// Handles the `DELETE /crates/:crate_id/:version/yank` route.
@@ -42,6 +43,16 @@ fn modify_yank(req: &mut dyn RequestExt, yanked: bool) -> EndpointResult {
     if user.rights(req.app(), &owners)? < Rights::Publish {
         return Err(cargo_err("must already be an owner to yank or unyank"));
     }
+
+    if version.yanked == yanked {
+        // The crate is already in the state requested, nothing to do
+        return ok_true();
+    }
+
+    diesel::update(&version)
+        .set(versions::yanked.eq(yanked))
+        .execute(&*conn)?;
+
     let action = if yanked {
         VersionAction::Yank
     } else {

src/worker/git.rs

@@ -1,7 +1,6 @@
 use crate::background_jobs::Environment;
 use crate::git::{Crate, Credentials};
 use crate::models::Version;
-use crate::schema::versions;
 use chrono::Utc;
 use std::fs::{self, OpenOptions};
 use std::io::prelude::*;
@@ -31,60 +30,40 @@ pub fn add_crate(env: &Environment, krate: Crate) -> Result<(), PerformError> {
 /// push the changes.
 #[swirl::background_job]
 pub fn yank(
-    conn: &PgConnection,
     env: &Environment,
     krate: String,
     version: Version,
     yanked: bool,
 ) -> Result<(), PerformError> {
-    use diesel::prelude::*;
-
     let repo = env.lock_index()?;
     let dst = repo.index_file(&krate);
 
-    conn.transaction(|| {
-        let yanked_in_db: bool = versions::table
-            .find(version.id)
-            .select(versions::yanked)
-            .for_update()
-            .first(&*conn)?;
-
-        if yanked_in_db == yanked {
-            // The crate is alread in the state requested, nothing to do
-            return Ok(());
-        }
+    let prev = fs::read_to_string(&dst)?;
+    let new = prev
+        .lines()
+        .map(|line| {
+            let mut git_crate = serde_json::from_str::<Crate>(line)
+                .map_err(|_| format!("couldn't decode: `{}`", line))?;
+            if git_crate.name != krate || git_crate.vers != version.num {
+                return Ok(line.to_string());
+            }
+            git_crate.yanked = Some(yanked);
+            Ok(serde_json::to_string(&git_crate)?)
+        })
+        .collect::<Result<Vec<_>, PerformError>>();
+    let new = new?.join("\n") + "\n";
+    fs::write(&dst, new.as_bytes())?;
+
+    let message: String = format!(
+        "{} crate `{}#{}`",
+        if yanked { "Yanking" } else { "Unyanking" },
+        krate,
+        version.num
+    );
+
+    repo.commit_and_push(&message, &repo.relative_index_file(&krate))?;
 
-        let prev = fs::read_to_string(&dst)?;
-        let new = prev
-            .lines()
-            .map(|line| {
-                let mut git_crate = serde_json::from_str::<Crate>(line)
-                    .map_err(|_| format!("couldn't decode: `{}`", line))?;
-                if git_crate.name != krate || git_crate.vers != version.num {
-                    return Ok(line.to_string());
-                }
-                git_crate.yanked = Some(yanked);
-                Ok(serde_json::to_string(&git_crate)?)
-            })
-            .collect::<Result<Vec<_>, PerformError>>();
-        let new = new?.join("\n") + "\n";
-        fs::write(&dst, new.as_bytes())?;
-
-        let message: String = format!(
-            "{} crate `{}#{}`",
-            if yanked { "Yanking" } else { "Unyanking" },
-            krate,
-            version.num
-        );
-
-        repo.commit_and_push(&message, &repo.relative_index_file(&krate))?;
-
-        diesel::update(&version)
-            .set(versions::yanked.eq(yanked))
-            .execute(&*conn)?;
-
-        Ok(())
-    })
+    Ok(())
 }
 
 /// Collapse the index into a single commit, archiving the current history in a snapshot branch.