Merge pull request #10563 from Turbo87/model-krate-deps

models/krate: Move out code unrelated to database interaction

Author: Turbo87

src/controllers/krate/metadata.rs

@@ -11,7 +11,9 @@ use crate::models::{
     Version, VersionOwnerAction,
 };
 use crate::schema::*;
-use crate::util::errors::{bad_request, crate_not_found, AppResult, BoxedAppError};
+use crate::util::errors::{
+    bad_request, crate_not_found, version_not_found, AppResult, BoxedAppError,
+};
 use crate::views::{EncodableCategory, EncodableCrate, EncodableKeyword, EncodableVersion};
 use axum::extract::{FromRequestParts, Query};
 use axum_extra::json;
@@ -123,6 +125,8 @@ pub async fn find_crate(
         .filter(|_| include.default_version && !include.versions)
     {
         let version = krate.find_version(&mut conn, default_version).await?;
+        let version = version.ok_or_else(|| version_not_found(&krate.name, default_version))?;
+
         let (actions, published_by) = tokio::try_join!(
             VersionOwnerAction::by_version(&mut conn, &version),
             version.published_by(&mut conn),

src/controllers/krate/owners.rs

@@ -1,21 +1,28 @@
 //! All routes related to managing owners of a crate
 
 use crate::controllers::krate::CratePath;
-use crate::models::{krate::NewOwnerInvite, token::EndpointScope};
+use crate::models::krate::OwnerRemoveError;
+use crate::models::{
+    krate::NewOwnerInvite, token::EndpointScope, CrateOwner, NewCrateOwnerInvitation,
+    NewCrateOwnerInvitationOutcome, OwnerKind,
+};
 use crate::models::{Crate, Owner, Rights, Team, User};
-use crate::util::errors::{bad_request, crate_not_found, custom, AppResult};
+use crate::util::errors::{bad_request, crate_not_found, custom, AppResult, BoxedAppError};
 use crate::views::EncodableOwner;
-use crate::{app::AppState, models::krate::OwnerAddError};
+use crate::{app::AppState, App};
 use crate::{auth::AuthCheck, email::Email};
 use axum::Json;
 use axum_extra::json;
 use axum_extra::response::ErasedJson;
+use chrono::Utc;
+use crates_io_database::schema::crate_owners;
 use diesel::prelude::*;
 use diesel_async::scoped_futures::ScopedFutureExt;
-use diesel_async::{AsyncConnection, RunQueryDsl};
+use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
 use http::request::Parts;
 use http::StatusCode;
 use secrecy::{ExposeSecret, SecretString};
+use thiserror::Error;
 
 /// List crate owners.
 #[utoipa::path(
@@ -199,7 +206,7 @@ async fn modify_owners(
                             return Err(bad_request(format_args!("`{login}` is already an owner")));
                         }
 
-                        match krate.owner_add(&app, conn, user, login).await {
+                        match add_owner(&app, conn, user, &krate, login).await {
                             // A user was successfully invited, and they must accept
                             // the invite, and a best-effort attempt should be made
                             // to email them the invite token for one-click
@@ -274,6 +281,96 @@ async fn modify_owners(
     Ok(json!({ "msg": comma_sep_msg, "ok": true }))
 }
 
+/// Invite `login` as an owner of this crate, returning the created
+/// [`NewOwnerInvite`].
+async fn add_owner(
+    app: &App,
+    conn: &mut AsyncPgConnection,
+    req_user: &User,
+    krate: &Crate,
+    login: &str,
+) -> Result<NewOwnerInvite, OwnerAddError> {
+    use diesel::insert_into;
+
+    let owner = Owner::find_or_create_by_login(app, conn, req_user, login).await?;
+    match owner {
+        // Users are invited and must accept before being added
+        Owner::User(user) => {
+            let expires_at = Utc::now() + app.config.ownership_invitations_expiration;
+            let invite = NewCrateOwnerInvitation {
+                invited_user_id: user.id,
+                invited_by_user_id: req_user.id,
+                crate_id: krate.id,
+                expires_at,
+            };
+
+            let creation_ret = invite.create(conn).await.map_err(BoxedAppError::from)?;
+
+            match creation_ret {
+                NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => {
+                    Ok(NewOwnerInvite::User(user, plaintext_token))
+                }
+                NewCrateOwnerInvitationOutcome::AlreadyExists => {
+                    Err(OwnerAddError::AlreadyInvited(Box::new(user)))
+                }
+            }
+        }
+        // Teams are added as owners immediately
+        Owner::Team(team) => {
+            insert_into(crate_owners::table)
+                .values(&CrateOwner {
+                    crate_id: krate.id,
+                    owner_id: team.id,
+                    created_by: req_user.id,
+                    owner_kind: OwnerKind::Team,
+                    email_notifications: true,
+                })
+                .on_conflict(crate_owners::table.primary_key())
+                .do_update()
+                .set(crate_owners::deleted.eq(false))
+                .execute(conn)
+                .await
+                .map_err(BoxedAppError::from)?;
+
+            Ok(NewOwnerInvite::Team(team))
+        }
+    }
+}
+
+/// Error results from a [`add_owner()`] model call.
+#[derive(Debug, Error)]
+enum OwnerAddError {
+    /// An opaque [`BoxedAppError`].
+    #[error("{0}")] // AppError does not impl Error
+    AppError(BoxedAppError),
+
+    /// The requested invitee already has a pending invite.
+    ///
+    /// Note: Teams are always immediately added, so they cannot have a pending
+    /// invite to cause this error.
+    #[error("user already has pending invite")]
+    AlreadyInvited(Box<User>),
+}
+
+/// A [`BoxedAppError`] does not impl [`std::error::Error`] so it needs a manual
+/// [`From`] impl.
+impl From<BoxedAppError> for OwnerAddError {
+    fn from(value: BoxedAppError) -> Self {
+        Self::AppError(value)
+    }
+}
+
+impl From<OwnerRemoveError> for BoxedAppError {
+    fn from(error: OwnerRemoveError) -> Self {
+        match error {
+            OwnerRemoveError::Diesel(error) => error.into(),
+            OwnerRemoveError::NotFound { login } => {
+                bad_request(format!("could not find owner with login `{login}`"))
+            }
+        }
+    }
+}
+
 pub struct OwnerInviteEmail {
     /// The destination email address for this email.
     recipient_email_address: String,

src/controllers/version.rs

@@ -14,7 +14,7 @@ use utoipa::IntoParams;
 
 use crate::controllers::krate::load_crate;
 use crate::models::{Crate, Version};
-use crate::util::errors::AppResult;
+use crate::util::errors::{version_not_found, AppResult};
 
 #[derive(Deserialize, FromRequestParts, IntoParams)]
 #[into_params(parameter_in = Path)]
@@ -47,7 +47,10 @@ async fn version_and_crate(
     semver: &str,
 ) -> AppResult<(Version, Crate)> {
     let krate = load_crate(conn, crate_name).await?;
-    let version = krate.find_version(conn, semver).await?;
+    let version = krate
+        .find_version(conn, semver)
+        .await?
+        .ok_or_else(|| version_not_found(crate_name, semver))?;
 
     Ok((version, krate))
 }

src/models/krate.rs

@@ -1,4 +1,4 @@
-use chrono::{NaiveDateTime, Utc};
+use chrono::NaiveDateTime;
 use diesel::associations::Identifiable;
 use diesel::dsl;
 use diesel::pg::Pg;
@@ -11,13 +11,8 @@ use thiserror::Error;
 
 use crate::models::helpers::with_count::*;
 use crate::models::version::TopVersions;
-use crate::models::{
-    CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, Owner, OwnerKind,
-    ReverseDependency, User, Version,
-};
+use crate::models::{CrateOwner, Owner, OwnerKind, ReverseDependency, User, Version};
 use crate::schema::*;
-use crate::util::errors::{bad_request, version_not_found, AppResult};
-use crate::{app::App, util::errors::BoxedAppError};
 use crates_io_diesel_helpers::canon_crate_name;
 
 use super::Team;
@@ -208,14 +203,13 @@ impl Crate {
         &self,
         conn: &mut AsyncPgConnection,
         version: &str,
-    ) -> AppResult<Version> {
+    ) -> QueryResult<Option<Version>> {
         Version::belonging_to(self)
             .filter(versions::num.eq(version))
             .select(Version::as_select())
             .first(conn)
             .await
-            .optional()?
-            .ok_or_else(|| version_not_found(&self.name, version))
+            .optional()
     }
 
     // Validates the name is a valid crate name.
@@ -385,63 +379,11 @@ impl Crate {
         Ok(users.chain(teams).collect())
     }
 
-    /// Invite `login` as an owner of this crate, returning the created
-    /// [`NewOwnerInvite`].
-    pub async fn owner_add(
+    pub async fn owner_remove(
         &self,
-        app: &App,
         conn: &mut AsyncPgConnection,
-        req_user: &User,
         login: &str,
-    ) -> Result<NewOwnerInvite, OwnerAddError> {
-        use diesel::insert_into;
-
-        let owner = Owner::find_or_create_by_login(app, conn, req_user, login).await?;
-        match owner {
-            // Users are invited and must accept before being added
-            Owner::User(user) => {
-                let expires_at = Utc::now() + app.config.ownership_invitations_expiration;
-                let invite = NewCrateOwnerInvitation {
-                    invited_user_id: user.id,
-                    invited_by_user_id: req_user.id,
-                    crate_id: self.id,
-                    expires_at,
-                };
-
-                let creation_ret = invite.create(conn).await.map_err(BoxedAppError::from)?;
-
-                match creation_ret {
-                    NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => {
-                        Ok(NewOwnerInvite::User(user, plaintext_token))
-                    }
-                    NewCrateOwnerInvitationOutcome::AlreadyExists => {
-                        Err(OwnerAddError::AlreadyInvited(Box::new(user)))
-                    }
-                }
-            }
-            // Teams are added as owners immediately
-            Owner::Team(team) => {
-                insert_into(crate_owners::table)
-                    .values(&CrateOwner {
-                        crate_id: self.id,
-                        owner_id: team.id,
-                        created_by: req_user.id,
-                        owner_kind: OwnerKind::Team,
-                        email_notifications: true,
-                    })
-                    .on_conflict(crate_owners::table.primary_key())
-                    .do_update()
-                    .set(crate_owners::deleted.eq(false))
-                    .execute(conn)
-                    .await
-                    .map_err(BoxedAppError::from)?;
-
-                Ok(NewOwnerInvite::Team(team))
-            }
-        }
-    }
-
-    pub async fn owner_remove(&self, conn: &mut AsyncPgConnection, login: &str) -> AppResult<()> {
+    ) -> Result<(), OwnerRemoveError> {
         let query = diesel::sql_query(
             r#"WITH crate_owners_with_login AS (
                 SELECT
@@ -477,8 +419,7 @@ impl Crate {
             .await?;
 
         if num_updated_rows == 0 {
-            let error = format!("could not find owner with login `{login}`");
-            return Err(bad_request(error));
+            return Err(OwnerRemoveError::not_found(login));
         }
 
         Ok(())
@@ -518,26 +459,18 @@ pub enum NewOwnerInvite {
     Team(Team),
 }
 
-/// Error results from a [`Crate::owner_add()`] model call.
 #[derive(Debug, Error)]
-pub enum OwnerAddError {
-    /// An opaque [`BoxedAppError`].
-    #[error("{0}")] // AppError does not impl Error
-    AppError(BoxedAppError),
-
-    /// The requested invitee already has a pending invite.
-    ///
-    /// Note: Teams are always immediately added, so they cannot have a pending
-    /// invite to cause this error.
-    #[error("user already has pending invite")]
-    AlreadyInvited(Box<User>),
+pub enum OwnerRemoveError {
+    #[error(transparent)]
+    Diesel(#[from] diesel::result::Error),
+    #[error("Could not find owner with login `{login}`")]
+    NotFound { login: String },
 }
 
-/// A [`BoxedAppError`] does not impl [`std::error::Error`] so it needs a manual
-/// [`From`] impl.
-impl From<BoxedAppError> for OwnerAddError {
-    fn from(value: BoxedAppError) -> Self {
-        Self::AppError(value)
+impl OwnerRemoveError {
+    pub fn not_found(login: &str) -> Self {
+        let login = login.to_string();
+        Self::NotFound { login }
     }
 }