Reuse the code to perform version yank update

Signed-off-by: Rustin170506 <[email protected]>

Author: 0xPoe

src/controllers/version/metadata.rs

@@ -7,7 +7,7 @@
 use axum::extract::Path;
 use axum::Json;
 use crates_io_worker::BackgroundJob;
-use diesel::{ExpressionMethods, RunQueryDsl};
+use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl};
 use diesel_async::async_connection_wrapper::AsyncConnectionWrapper;
 use http::request::Parts;
 use http::StatusCode;
@@ -22,6 +22,7 @@ use crate::models::{
     insert_version_owner_action, Crate, Rights, Version, VersionAction, VersionOwnerAction,
 };
 use crate::rate_limiter::LimitedAction;
+use crate::schema::versions;
 use crate::tasks::spawn_blocking;
 use crate::util::diesel::Conn;
 use crate::util::errors::{bad_request, custom, version_not_found, AppResult};
@@ -142,14 +143,52 @@ fn apply_yank_update(
 ) -> AppResult<()> {
     // Try to update the yank state first, to avoid unnecessary checks.
     update_version_yank_state(version, update_data)?;
+    perform_version_yank_update(state, req, conn, version, krate)?;
 
-    // Add authentication check
+    Ok(())
+}
+
+fn update_version_yank_state(version: &mut Version, update_data: &VersionUpdate) -> AppResult<()> {
+    match (update_data.yanked, &update_data.yank_message) {
+        (Some(true), Some(message)) => {
+            version.yanked = true;
+            version.yank_message = Some(message.clone());
+        }
+        (Some(yanked), None) => {
+            version.yanked = yanked;
+            version.yank_message = None;
+        }
+        (Some(false), Some(_)) => {
+            return Err(bad_request("Cannot set yank message when unyanking"));
+        }
+        (None, Some(message)) => {
+            if version.yanked {
+                version.yank_message = Some(message.clone());
+            } else {
+                return Err(bad_request(
+                    "Cannot update yank message for a version that is not yanked",
+                ));
+            }
+        }
+        // If both yanked and yank_message are None, do nothing.
+        // This function only cares about updating the yanked state and yank message.
+        (None, None) => {}
+    }
+    Ok(())
+}
+
+pub fn perform_version_yank_update(
+    state: &AppState,
+    req: &Parts,
+    conn: &mut impl Conn,
+    version: &Version,
+    krate: &Crate,
+) -> AppResult<()> {
     let auth = AuthCheck::default()
         .with_endpoint_scope(EndpointScope::Yank)
         .for_crate(&krate.name)
         .check(req, conn)?;
 
-    // Add rate limiting check
     state
         .rate_limiter
         .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?;
@@ -158,68 +197,52 @@ fn apply_yank_update(
     let user = auth.user();
     let owners = krate.owners(conn)?;
 
-    // Check user rights
     if Handle::current().block_on(user.rights(state, &owners))? < Rights::Publish {
         if user.is_admin {
+            let action = if version.yanked {
+                "yanking"
+            } else {
+                "unyanking"
+            };
             warn!(
-                "Admin {} is updating {}@{}",
+                "Admin {} is {action} {}@{}",
                 user.gh_login, krate.name, version.num
             );
         } else {
             return Err(custom(
                 StatusCode::FORBIDDEN,
-                "must already be an owner to update version",
+                "must already be an owner to yank or unyank",
             ));
         }
     }
 
-    diesel::update(&*version)
+    // Check if the yanked state or yank message has changed
+    let (yanked, yank_message) = versions::table
+        .find(version.id)
+        .select((versions::yanked, versions::yank_message))
+        .first::<(bool, Option<String>)>(conn)?;
+
+    if yanked == version.yanked && yank_message == version.yank_message {
+        // No changes, return early
+        return Ok(());
+    }
+
+    diesel::update(version)
         .set((
-            crate::schema::versions::yanked.eq(version.yanked),
-            crate::schema::versions::yank_message.eq(&version.yank_message),
+            versions::yanked.eq(version.yanked),
+            versions::yank_message.eq(&version.yank_message),
         ))
         .execute(conn)?;
 
-    // Add version owner action
     let action = if version.yanked {
         VersionAction::Yank
     } else {
         VersionAction::Unyank
     };
     insert_version_owner_action(conn, version.id, user.id, api_token_id, action)?;
 
-    // Enqueue jobs
     jobs::enqueue_sync_to_index(&krate.name, conn)?;
     UpdateDefaultVersion::new(krate.id).enqueue(conn)?;
 
     Ok(())
 }
-
-fn update_version_yank_state(version: &mut Version, update_data: &VersionUpdate) -> AppResult<()> {
-    match (update_data.yanked, &update_data.yank_message) {
-        (Some(true), Some(message)) => {
-            version.yanked = true;
-            version.yank_message = Some(message.clone());
-        }
-        (Some(yanked), None) => {
-            version.yanked = yanked;
-            version.yank_message = None;
-        }
-        (Some(false), Some(_)) => {
-            return Err(bad_request("Cannot set yank message when unyanking"));
-        }
-        (None, Some(message)) => {
-            if version.yanked {
-                version.yank_message = Some(message.clone());
-            } else {
-                return Err(bad_request(
-                    "Cannot update yank message for a version that is not yanked",
-                ));
-            }
-        }
-        // If both yanked and yank_message are None, do nothing.
-        // This function only cares about updating the yanked state and yank message.
-        (None, None) => {}
-    }
-    Ok(())
-}

src/controllers/version/yank.rs

@@ -1,26 +1,15 @@
 //! Endpoints for yanking and unyanking specific versions of crates
 
+use super::metadata::perform_version_yank_update;
 use super::version_and_crate;
 use crate::app::AppState;
-use crate::auth::AuthCheck;
 use crate::controllers::helpers::ok_true;
-use crate::models::token::EndpointScope;
-use crate::models::Rights;
-use crate::models::{insert_version_owner_action, VersionAction};
-use crate::rate_limiter::LimitedAction;
-use crate::schema::versions;
 use crate::tasks::spawn_blocking;
-use crate::util::errors::{custom, version_not_found, AppResult};
-use crate::worker::jobs;
-use crate::worker::jobs::UpdateDefaultVersion;
+use crate::util::errors::{version_not_found, AppResult};
 use axum::extract::Path;
 use axum::response::Response;
-use crates_io_worker::BackgroundJob;
-use diesel::prelude::*;
 use diesel_async::async_connection_wrapper::AsyncConnectionWrapper;
 use http::request::Parts;
-use http::StatusCode;
-use tokio::runtime::Handle;
 
 /// Handles the `DELETE /crates/:crate_id/:version/yank` route.
 /// This does not delete a crate version, it makes the crate
@@ -66,57 +55,9 @@ async fn modify_yank(
     let conn = state.db_write().await?;
     spawn_blocking(move || {
         let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into();
-
-        let auth = AuthCheck::default()
-            .with_endpoint_scope(EndpointScope::Yank)
-            .for_crate(&crate_name)
-            .check(&req, conn)?;
-
-        state
-            .rate_limiter
-            .check_rate_limit(auth.user_id(), LimitedAction::YankUnyank, conn)?;
-
-        let (version, krate) = version_and_crate(conn, &crate_name, &version)?;
-        let api_token_id = auth.api_token_id();
-        let user = auth.user();
-        let owners = krate.owners(conn)?;
-
-        if Handle::current().block_on(user.rights(&state, &owners))? < Rights::Publish {
-            if user.is_admin {
-                let action = if yanked { "yanking" } else { "unyanking" };
-                warn!(
-                    "Admin {} is {action} {}@{}",
-                    user.gh_login, krate.name, version.num
-                );
-            } else {
-                return Err(custom(
-                    StatusCode::FORBIDDEN,
-                    "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 {
-            VersionAction::Unyank
-        };
-
-        insert_version_owner_action(conn, version.id, user.id, api_token_id, action)?;
-
-        jobs::enqueue_sync_to_index(&krate.name, conn)?;
-
-        UpdateDefaultVersion::new(krate.id).enqueue(conn)?;
-
+        let (mut version, krate) = version_and_crate(conn, &crate_name, &version)?;
+        version.yanked = yanked;
+        perform_version_yank_update(&state, &req, conn, &version, &krate)?;
         ok_true()
     })
     .await