From 8cd0ec5bed8616903beddadef85a88ac0bdb1aad Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Fri, 22 Nov 2024 11:03:27 +0100 Subject: [PATCH] controllers/krate/owners: Convert remaining endpoints to async/await --- src/controllers/krate/owners.rs | 230 ++++++++++++++------------- src/models/crate_owner_invitation.rs | 44 ++--- src/models/krate.rs | 17 +- src/models/owner.rs | 20 +-- src/models/team.rs | 20 +-- src/models/user.rs | 7 +- src/tests/owners.rs | 6 +- src/tests/routes/crates/list.rs | 3 +- src/tests/routes/me/get.rs | 6 +- src/tests/routes/users/stats.rs | 3 +- src/tests/team.rs | 3 +- 11 files changed, 187 insertions(+), 172 deletions(-) diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index dcada53a248..9e1bd6e3c41 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -2,7 +2,6 @@ use crate::models::{krate::NewOwnerInvite, token::EndpointScope}; use crate::models::{Crate, Owner, Rights, Team, User}; -use crate::tasks::spawn_blocking; use crate::util::diesel::prelude::*; use crate::util::errors::{bad_request, crate_not_found, custom, AppResult}; use crate::views::EncodableOwner; @@ -12,11 +11,11 @@ use axum::extract::Path; use axum::Json; use axum_extra::json; use axum_extra::response::ErasedJson; -use diesel_async::async_connection_wrapper::AsyncConnectionWrapper; +use diesel_async::scoped_futures::ScopedFutureExt; +use diesel_async::AsyncConnection; use http::request::Parts; use http::StatusCode; use secrecy::{ExposeSecret, SecretString}; -use tokio::runtime::Handle; /// Handles the `GET /crates/:crate_id/owners` route. pub async fn owners(state: AppState, Path(crate_name): Path) -> AppResult { @@ -62,25 +61,23 @@ pub async fn owner_team(state: AppState, Path(crate_name): Path) -> AppR /// Handles the `GET /crates/:crate_id/owner_user` route. pub async fn owner_user(state: AppState, Path(crate_name): Path) -> AppResult { - let conn = state.db_read().await?; - spawn_blocking(move || { - use diesel::RunQueryDsl; + use diesel_async::RunQueryDsl; - let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); + let mut conn = state.db_read().await?; - let krate: Crate = Crate::by_name(&crate_name) - .first(conn) - .optional()? - .ok_or_else(|| crate_not_found(&crate_name))?; + let krate: Crate = Crate::by_name(&crate_name) + .first(&mut conn) + .await + .optional()? + .ok_or_else(|| crate_not_found(&crate_name))?; - let owners = User::owning(&krate, conn)? - .into_iter() - .map(Owner::into) - .collect::>(); + let owners = User::owning(&krate, &mut conn) + .await? + .into_iter() + .map(Owner::into) + .collect::>(); - Ok(json!({ "users": owners })) - }) - .await? + Ok(json!({ "users": owners })) } /// Handles the `PUT /crates/:crate_id/owners` route. @@ -116,6 +113,8 @@ async fn modify_owners( body: ChangeOwnersRequest, add: bool, ) -> AppResult { + use diesel_async::RunQueryDsl; + let logins = body.owners; // Bound the number of invites processed per request to limit the cost of @@ -132,121 +131,124 @@ async fn modify_owners( .for_crate(&crate_name) .check(&parts, &mut conn) .await?; - spawn_blocking(move || { - use diesel::RunQueryDsl; - - let conn: &mut AsyncConnectionWrapper<_> = &mut conn.into(); - - let user = auth.user(); - - // The set of emails to send out after invite processing is complete and - // the database transaction has committed. - let mut emails = Vec::with_capacity(logins.len()); - - let comma_sep_msg = conn.transaction(|conn| { - let krate: Crate = Crate::by_name(&crate_name) - .first(conn) - .optional()? - .ok_or_else(|| crate_not_found(&crate_name))?; - - let owners = krate.owners(conn)?; - match Handle::current().block_on(user.rights(&app, &owners))? { - Rights::Full => {} - // Yes! - Rights::Publish => { - return Err(custom( - StatusCode::FORBIDDEN, - "team members don't have permission to modify owners", - )); - } - Rights::None => { - return Err(custom( - StatusCode::FORBIDDEN, - "only owners have permission to modify owners", - )); + let user = auth.user(); + + let (comma_sep_msg, emails) = conn + .transaction(|conn| { + let app = app.clone(); + async move { + let krate: Crate = Crate::by_name(&crate_name) + .first(conn) + .await + .optional()? + .ok_or_else(|| crate_not_found(&crate_name))?; + + let owners = krate.async_owners(conn).await?; + + match user.rights(&app, &owners).await? { + Rights::Full => {} + // Yes! + Rights::Publish => { + return Err(custom( + StatusCode::FORBIDDEN, + "team members don't have permission to modify owners", + )); + } + Rights::None => { + return Err(custom( + StatusCode::FORBIDDEN, + "only owners have permission to modify owners", + )); + } } - } - let comma_sep_msg = if add { - let mut msgs = Vec::with_capacity(logins.len()); - for login in &logins { - let login_test = - |owner: &Owner| owner.login().to_lowercase() == *login.to_lowercase(); - if owners.iter().any(login_test) { - return Err(bad_request(format_args!("`{login}` is already an owner"))); - } + // The set of emails to send out after invite processing is complete and + // the database transaction has committed. + let mut emails = Vec::with_capacity(logins.len()); + + let comma_sep_msg = if add { + let mut msgs = Vec::with_capacity(logins.len()); + for login in &logins { + let login_test = + |owner: &Owner| owner.login().to_lowercase() == *login.to_lowercase(); + if owners.iter().any(login_test) { + return Err(bad_request(format_args!("`{login}` is already an owner"))); + } - match krate.owner_add(&app, conn, user, login) { - // 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 - // acceptance. - Ok(NewOwnerInvite::User(invitee, token)) => { - msgs.push(format!( - "user {} has been invited to be an owner of crate {}", - invitee.gh_login, krate.name, - )); - - if let Some(recipient) = invitee.verified_email(conn).ok().flatten() { - emails.push(OwnerInviteEmail { - recipient_email_address: recipient, - inviter: user.gh_login.clone(), - domain: app.emails.domain.clone(), - crate_name: krate.name.clone(), - token, - }); + match krate.owner_add(&app, conn, user, 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 + // acceptance. + Ok(NewOwnerInvite::User(invitee, token)) => { + msgs.push(format!( + "user {} has been invited to be an owner of crate {}", + invitee.gh_login, krate.name, + )); + + if let Some(recipient) = + invitee.async_verified_email(conn).await.ok().flatten() + { + emails.push(OwnerInviteEmail { + recipient_email_address: recipient, + inviter: user.gh_login.clone(), + domain: app.emails.domain.clone(), + crate_name: krate.name.clone(), + token, + }); + } } - } - // A team was successfully invited. They are immediately - // added, and do not have an invite token. - Ok(NewOwnerInvite::Team(team)) => msgs.push(format!( - "team {} has been added as an owner of crate {}", - team.login, krate.name - )), + // A team was successfully invited. They are immediately + // added, and do not have an invite token. + Ok(NewOwnerInvite::Team(team)) => msgs.push(format!( + "team {} has been added as an owner of crate {}", + team.login, krate.name + )), - // This user has a pending invite. - Err(OwnerAddError::AlreadyInvited(user)) => msgs.push(format!( + // This user has a pending invite. + Err(OwnerAddError::AlreadyInvited(user)) => msgs.push(format!( "user {} already has a pending invitation to be an owner of crate {}", user.gh_login, krate.name )), - // An opaque error occurred. - Err(OwnerAddError::AppError(e)) => return Err(e), + // An opaque error occurred. + Err(OwnerAddError::AppError(e)) => return Err(e), + } } - } - msgs.join(",") - } else { - for login in &logins { - krate.owner_remove(conn, login)?; - } - if User::owning(&krate, conn)?.is_empty() { - return Err(bad_request( - "cannot remove all individual owners of a crate. \ + msgs.join(",") + } else { + for login in &logins { + krate.owner_remove(conn, login).await?; + } + if User::owning(&krate, conn).await?.is_empty() { + return Err(bad_request( + "cannot remove all individual owners of a crate. \ Team member don't have permission to modify owners, so \ at least one individual owner is required.", - )); - } - "owners successfully removed".to_owned() - }; + )); + } + "owners successfully removed".to_owned() + }; - Ok(comma_sep_msg) - })?; + Ok((comma_sep_msg, emails)) + } + .scope_boxed() + }) + .await?; - // Send the accumulated invite emails now the database state has - // committed. - for email in emails { - let addr = email.recipient_email_address().to_string(); + // Send the accumulated invite emails now the database state has + // committed. + for email in emails { + let addr = email.recipient_email_address().to_string(); - if let Err(e) = app.emails.send(&addr, email) { - warn!("Failed to send co-owner invite email: {e}"); - } + if let Err(e) = app.emails.async_send(&addr, email).await { + warn!("Failed to send co-owner invite email: {e}"); } + } - Ok(json!({ "msg": comma_sep_msg, "ok": true })) - }) - .await? + Ok(json!({ "msg": comma_sep_msg, "ok": true })) } pub struct OwnerInviteEmail { diff --git a/src/models/crate_owner_invitation.rs b/src/models/crate_owner_invitation.rs index 17c0518b8ef..5b2aceec85f 100644 --- a/src/models/crate_owner_invitation.rs +++ b/src/models/crate_owner_invitation.rs @@ -1,5 +1,6 @@ use chrono::{NaiveDateTime, Utc}; -use diesel_async::AsyncPgConnection; +use diesel_async::scoped_futures::ScopedFutureExt; +use diesel_async::{AsyncConnection, AsyncPgConnection}; use http::StatusCode; use secrecy::SecretString; @@ -7,7 +8,6 @@ use crate::config; use crate::models::{CrateOwner, OwnerKind}; use crate::schema::{crate_owner_invitations, crate_owners, crates}; use crate::util::diesel::prelude::*; -use crate::util::diesel::Conn; use crate::util::errors::{custom, AppResult}; #[derive(Debug)] @@ -30,14 +30,14 @@ pub struct CrateOwnerInvitation { } impl CrateOwnerInvitation { - pub fn create( + pub async fn create( invited_user_id: i32, invited_by_user_id: i32, crate_id: i32, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, config: &config::Server, ) -> QueryResult { - use diesel::RunQueryDsl; + use diesel_async::RunQueryDsl; #[derive(Insertable, Clone, Copy, Debug)] #[diesel(table_name = crate_owner_invitations, check_for_backend(diesel::pg::Pg))] @@ -50,22 +50,27 @@ impl CrateOwnerInvitation { // Before actually creating the invite, check if an expired invitation already exists // and delete it from the database. This allows obtaining a new invite if the old one // expired, instead of returning "already exists". - conn.transaction(|conn| -> QueryResult<()> { - // This does a SELECT FOR UPDATE + DELETE instead of a DELETE with a WHERE clause to - // use the model's `is_expired` method, centralizing our expiration checking logic. - let existing: Option = crate_owner_invitations::table - .find((invited_user_id, crate_id)) - .for_update() - .first(conn) - .optional()?; - - if let Some(existing) = existing { - if existing.is_expired(config) { - diesel::delete(&existing).execute(conn)?; + conn.transaction(|conn| { + async move { + // This does a SELECT FOR UPDATE + DELETE instead of a DELETE with a WHERE clause to + // use the model's `is_expired` method, centralizing our expiration checking logic. + let existing: Option = crate_owner_invitations::table + .find((invited_user_id, crate_id)) + .for_update() + .first(conn) + .await + .optional()?; + + if let Some(existing) = existing { + if existing.is_expired(config) { + diesel::delete(&existing).execute(conn).await?; + } } + QueryResult::Ok(()) } - Ok(()) - })?; + .scope_boxed() + }) + .await?; let res: Option = diesel::insert_into(crate_owner_invitations::table) .values(&NewRecord { @@ -78,6 +83,7 @@ impl CrateOwnerInvitation { // deleted before doing this INSERT. .on_conflict_do_nothing() .get_result(conn) + .await .optional()?; Ok(match res { diff --git a/src/models/krate.rs b/src/models/krate.rs index 5c0cb1469b9..9e26a5e01f4 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -415,22 +415,23 @@ impl Crate { /// Invite `login` as an owner of this crate, returning the created /// [`NewOwnerInvite`]. - pub fn owner_add( + pub async fn owner_add( &self, app: &App, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, req_user: &User, login: &str, ) -> Result { use diesel::insert_into; - use diesel::RunQueryDsl; + use diesel_async::RunQueryDsl; - let owner = Owner::find_or_create_by_login(app, conn, req_user, login)?; + 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 creation_ret = CrateOwnerInvitation::create(user.id, req_user.id, self.id, conn, &app.config) + .await .map_err(BoxedAppError::from)?; match creation_ret { @@ -456,6 +457,7 @@ impl Crate { .do_update() .set(crate_owners::deleted.eq(false)) .execute(conn) + .await .map_err(BoxedAppError::from)?; Ok(NewOwnerInvite::Team(team)) @@ -463,8 +465,8 @@ impl Crate { } } - pub fn owner_remove(&self, conn: &mut impl Conn, login: &str) -> AppResult<()> { - use diesel::RunQueryDsl; + pub async fn owner_remove(&self, conn: &mut AsyncPgConnection, login: &str) -> AppResult<()> { + use diesel_async::RunQueryDsl; let query = diesel::sql_query( r#"WITH crate_owners_with_login AS ( @@ -497,7 +499,8 @@ impl Crate { let num_updated_rows = query .bind::(self.id) .bind::(login) - .execute(conn)?; + .execute(conn) + .await?; if num_updated_rows == 0 { let error = format!("could not find owner with login `{login}`"); diff --git a/src/models/owner.rs b/src/models/owner.rs index d6279e61380..05d57fe77e8 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -1,13 +1,12 @@ -use diesel::pg::Pg; -use diesel::prelude::*; - use crate::app::App; use crate::util::errors::{bad_request, AppResult}; +use diesel::pg::Pg; +use diesel::prelude::*; +use diesel_async::AsyncPgConnection; use crate::models::{Crate, Team, User}; use crate::schema::crate_owners; use crate::sql::pg_enum; -use crate::util::diesel::Conn; #[derive(Insertable, Associations, Identifiable, Debug, Clone, Copy)] #[diesel( @@ -61,18 +60,19 @@ impl Owner { /// /// May be a user's GH login or a full team name. This is case /// sensitive. - pub fn find_or_create_by_login( + pub async fn find_or_create_by_login( app: &App, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, req_user: &User, name: &str, ) -> AppResult { if name.contains(':') { - Ok(Owner::Team(Team::create_or_update( - app, conn, name, req_user, - )?)) + Ok(Owner::Team( + Team::create_or_update(app, conn, name, req_user).await?, + )) } else { - User::find_by_login(conn, name) + User::async_find_by_login(conn, name) + .await .optional()? .map(Owner::User) .ok_or_else(|| bad_request(format_args!("could not find user with login `{name}`"))) diff --git a/src/models/team.rs b/src/models/team.rs index b7d2bc4be92..2c562d5fe94 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -7,7 +7,6 @@ use crate::util::errors::{bad_request, custom, AppResult}; use crates_io_github::GitHubError; use oauth2::AccessToken; -use tokio::runtime::Handle; use crate::models::{Crate, CrateOwner, Owner, OwnerKind, User}; use crate::schema::{crate_owners, teams}; @@ -88,9 +87,9 @@ impl Team { /// # Panics /// /// This function will panic if login contains less than 2 `:` characters. - pub fn create_or_update( + pub async fn create_or_update( app: &App, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, login: &str, req_user: &User, ) -> AppResult { @@ -116,6 +115,7 @@ impl Team { team, req_user, ) + .await } _ => Err(bad_request( "unknown organization handler, \ @@ -127,9 +127,9 @@ impl Team { /// Tries to create or update a Github Team. Assumes `org` and `team` are /// correctly parsed out of the full `name`. `name` is passed as a /// convenience to avoid rebuilding it. - fn create_or_update_github_team( + async fn create_or_update_github_team( app: &App, - conn: &mut impl Conn, + conn: &mut AsyncPgConnection, login: &str, org_name: &str, team_name: &str, @@ -151,8 +151,7 @@ impl Team { } let token = AccessToken::new(req_user.gh_access_token.clone()); - let team = Handle::current() - .block_on(app.github.team_by_name(org_name, team_name, &token)) + let team = app.github.team_by_name(org_name, team_name, &token).await .map_err(|_| { bad_request(format_args!( "could not find the github team {org_name}/{team_name}. \ @@ -163,14 +162,14 @@ impl Team { let org_id = team.organization.id; - if !Handle::current().block_on(can_add_team(app, org_id, team.id, req_user))? { + if !can_add_team(app, org_id, team.id, req_user).await? { return Err(custom( StatusCode::FORBIDDEN, "only members of a team or organization owners can add it as an owner", )); } - let org = Handle::current().block_on(app.github.org_by_name(org_name, &token))?; + let org = app.github.org_by_name(org_name, &token).await?; NewTeam::builder() .login(&login.to_lowercase()) @@ -179,7 +178,8 @@ impl Team { .maybe_name(team.name.as_deref()) .maybe_avatar(org.avatar_url.as_deref()) .build() - .create_or_update(conn) + .async_create_or_update(conn) + .await .map_err(Into::into) } diff --git a/src/models/user.rs b/src/models/user.rs index 938a41bc944..dbd60433c19 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -60,14 +60,15 @@ impl User { .await } - pub fn owning(krate: &Crate, conn: &mut impl Conn) -> QueryResult> { - use diesel::RunQueryDsl; + pub async fn owning(krate: &Crate, conn: &mut AsyncPgConnection) -> QueryResult> { + use diesel_async::RunQueryDsl; let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) .select(User::as_select()) .filter(crate_owners::crate_id.eq(krate.id)) - .load(conn)? + .load(conn) + .await? .into_iter() .map(Owner::User); diff --git a/src/tests/owners.rs b/src/tests/owners.rs index 26fdd47a2c2..28377182679 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -384,14 +384,16 @@ async fn add_existing_team() { #[tokio::test(flavor = "multi_thread")] async fn deleted_ownership_isnt_in_owner_user() { let (app, anon, user) = TestApp::init().with_user().await; - let mut conn = app.db_conn(); let mut async_conn = app.async_db_conn().await; let user = user.as_model(); let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut async_conn) .await; - krate.owner_remove(&mut conn, &user.gh_login).unwrap(); + krate + .owner_remove(&mut async_conn, &user.gh_login) + .await + .unwrap(); let json: UserResponse = anon .get("/api/v1/crates/foo_my_packages/owner_user") diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index ca0050b7ea7..b762376fe83 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1175,14 +1175,13 @@ async fn crates_by_user_id() { #[tokio::test(flavor = "multi_thread")] async fn crates_by_user_id_not_including_deleted_owners() { let (app, anon, user) = TestApp::init().with_user().await; - let mut conn = app.db_conn(); let mut async_conn = app.async_db_conn().await; let user = user.as_model(); let krate = CrateBuilder::new("foo_my_packages", user.id) .expect_build(&mut async_conn) .await; - krate.owner_remove(&mut conn, "foo").unwrap(); + krate.owner_remove(&mut async_conn, "foo").await.unwrap(); for response in search_both_by_user_id(&anon, user.id).await { assert_eq!(response.crates.len(), 0); diff --git a/src/tests/routes/me/get.rs b/src/tests/routes/me/get.rs index 601c5190994..5283ef412c8 100644 --- a/src/tests/routes/me/get.rs +++ b/src/tests/routes/me/get.rs @@ -42,14 +42,16 @@ async fn me() { #[tokio::test(flavor = "multi_thread")] async fn test_user_owned_crates_doesnt_include_deleted_ownership() { let (app, _, user) = TestApp::init().with_user().await; - let mut conn = app.db_conn(); let mut async_conn = app.async_db_conn().await; let user_model = user.as_model(); let krate = CrateBuilder::new("foo_my_packages", user_model.id) .expect_build(&mut async_conn) .await; - krate.owner_remove(&mut conn, &user_model.gh_login).unwrap(); + krate + .owner_remove(&mut async_conn, &user_model.gh_login) + .await + .unwrap(); let json = user.show_me().await; assert_eq!(json.owned_crates.len(), 0); diff --git a/src/tests/routes/users/stats.rs b/src/tests/routes/users/stats.rs index f15af6dad70..959a81ad1e2 100644 --- a/src/tests/routes/users/stats.rs +++ b/src/tests/routes/users/stats.rs @@ -52,7 +52,8 @@ async fn user_total_downloads() { .execute(&mut conn) .unwrap(); no_longer_my_krate - .owner_remove(&mut conn, &user.gh_login) + .owner_remove(&mut async_conn, &user.gh_login) + .await .unwrap(); let url = format!("/api/v1/users/{}/stats", user.id); diff --git a/src/tests/team.rs b/src/tests/team.rs index 80550b27a8a..abc5a11f29b 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -477,7 +477,6 @@ async fn crates_by_team_id() { #[tokio::test(flavor = "multi_thread")] async fn crates_by_team_id_not_including_deleted_owners() { let (app, anon) = TestApp::init().empty().await; - let mut conn = app.db_conn(); let mut async_conn = app.async_db_conn().await; let user = app.db_new_user("user-all-teams").await; let user = user.as_model(); @@ -499,7 +498,7 @@ async fn crates_by_team_id_not_including_deleted_owners() { add_team_to_crate(&t, &krate, user, &mut async_conn) .await .unwrap(); - krate.owner_remove(&mut conn, &t.login).unwrap(); + krate.owner_remove(&mut async_conn, &t.login).await.unwrap(); let json = anon.search(&format!("team_id={}", t.id)).await; assert_eq!(json.crates.len(), 0);