From 0f770fd8ec9197526cdb8a0f19f0f6fdd78a3ab0 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 11 Feb 2025 12:42:12 +0100 Subject: [PATCH 1/3] model/invitation: Make `NewRecord` a top-level struct --- src/models/crate_owner_invitation.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/models/crate_owner_invitation.rs b/src/models/crate_owner_invitation.rs index 65a187903e2..e2e57f16153 100644 --- a/src/models/crate_owner_invitation.rs +++ b/src/models/crate_owner_invitation.rs @@ -16,6 +16,14 @@ pub enum NewCrateOwnerInvitationOutcome { InviteCreated { plaintext_token: SecretString }, } +#[derive(Clone, Debug, Insertable)] +#[diesel(table_name = crate_owner_invitations, check_for_backend(diesel::pg::Pg))] +pub struct NewCrateOwnerInvitation { + pub invited_user_id: i32, + pub invited_by_user_id: i32, + pub crate_id: i32, +} + /// The model representing a row in the `crate_owner_invitations` database table. #[derive(Clone, Debug, Identifiable, Queryable)] #[diesel(primary_key(invited_user_id, crate_id))] @@ -36,14 +44,6 @@ impl CrateOwnerInvitation { conn: &mut AsyncPgConnection, config: &config::Server, ) -> QueryResult { - #[derive(Insertable, Clone, Copy, Debug)] - #[diesel(table_name = crate_owner_invitations, check_for_backend(diesel::pg::Pg))] - struct NewRecord { - invited_user_id: i32, - invited_by_user_id: i32, - crate_id: i32, - } - // 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". @@ -70,7 +70,7 @@ impl CrateOwnerInvitation { .await?; let res: Option = diesel::insert_into(crate_owner_invitations::table) - .values(&NewRecord { + .values(&NewCrateOwnerInvitation { invited_user_id, invited_by_user_id, crate_id, From ee7e60a726b41e57765f7b7dfff43ad9923a6fa5 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 11 Feb 2025 12:47:35 +0100 Subject: [PATCH 2/3] model/invitation: Simplify `CrateOwnerInvitation::create()` arguments Passing in three `i32` arguments seems a bit error-prone... --- src/models.rs | 4 +++- src/models/crate_owner_invitation.rs | 12 +++--------- src/models/krate.rs | 17 +++++++++++------ 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/models.rs b/src/models.rs index 9580b921245..03bfc833d0e 100644 --- a/src/models.rs +++ b/src/models.rs @@ -1,6 +1,8 @@ pub use self::action::{NewVersionOwnerAction, VersionAction, VersionOwnerAction}; pub use self::category::{Category, CrateCategory, NewCategory}; -pub use self::crate_owner_invitation::{CrateOwnerInvitation, NewCrateOwnerInvitationOutcome}; +pub use self::crate_owner_invitation::{ + CrateOwnerInvitation, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, +}; pub use self::default_versions::{update_default_version, verify_default_version}; pub use self::deleted_crate::NewDeletedCrate; pub use self::dependency::{Dependency, DependencyKind, ReverseDependency}; diff --git a/src/models/crate_owner_invitation.rs b/src/models/crate_owner_invitation.rs index e2e57f16153..381ef8e9e20 100644 --- a/src/models/crate_owner_invitation.rs +++ b/src/models/crate_owner_invitation.rs @@ -38,9 +38,7 @@ pub struct CrateOwnerInvitation { impl CrateOwnerInvitation { pub async fn create( - invited_user_id: i32, - invited_by_user_id: i32, - crate_id: i32, + invite: &NewCrateOwnerInvitation, conn: &mut AsyncPgConnection, config: &config::Server, ) -> QueryResult { @@ -52,7 +50,7 @@ impl CrateOwnerInvitation { // 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)) + .find((invite.invited_user_id, invite.crate_id)) .for_update() .first(conn) .await @@ -70,11 +68,7 @@ impl CrateOwnerInvitation { .await?; let res: Option = diesel::insert_into(crate_owner_invitations::table) - .values(&NewCrateOwnerInvitation { - invited_user_id, - invited_by_user_id, - crate_id, - }) + .values(invite) // The ON CONFLICT DO NOTHING clause results in not creating the invite if another one // already exists. This does not cause problems with expired invitation as those are // deleted before doing this INSERT. diff --git a/src/models/krate.rs b/src/models/krate.rs index 1e98dd2b78c..43b2c9129a0 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -13,8 +13,8 @@ use crate::controllers::helpers::pagination::*; use crate::models::helpers::with_count::*; use crate::models::version::TopVersions; use crate::models::{ - CrateOwner, CrateOwnerInvitation, NewCrateOwnerInvitationOutcome, Owner, OwnerKind, - ReverseDependency, User, Version, + CrateOwner, CrateOwnerInvitation, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, + Owner, OwnerKind, ReverseDependency, User, Version, }; use crate::schema::*; use crate::util::errors::{bad_request, version_not_found, AppResult}; @@ -401,10 +401,15 @@ impl Crate { 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)?; + let invite = NewCrateOwnerInvitation { + invited_user_id: user.id, + invited_by_user_id: req_user.id, + crate_id: self.id, + }; + + let creation_ret = CrateOwnerInvitation::create(&invite, conn, &app.config) + .await + .map_err(BoxedAppError::from)?; match creation_ret { NewCrateOwnerInvitationOutcome::InviteCreated { plaintext_token } => { From b0057f35841c6637d87b6d655fdf2482a8836296 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 11 Feb 2025 12:50:34 +0100 Subject: [PATCH 3/3] model/invitation: Move `CrateOwnerInvitation::create()` fn onto `NewCrateOwnerInvitation` struct --- src/models/crate_owner_invitation.rs | 34 +++++++++++++++------------- src/models/krate.rs | 7 +++--- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/models/crate_owner_invitation.rs b/src/models/crate_owner_invitation.rs index 381ef8e9e20..94c5be77643 100644 --- a/src/models/crate_owner_invitation.rs +++ b/src/models/crate_owner_invitation.rs @@ -24,21 +24,9 @@ pub struct NewCrateOwnerInvitation { pub crate_id: i32, } -/// The model representing a row in the `crate_owner_invitations` database table. -#[derive(Clone, Debug, Identifiable, Queryable)] -#[diesel(primary_key(invited_user_id, crate_id))] -pub struct CrateOwnerInvitation { - pub invited_user_id: i32, - pub invited_by_user_id: i32, - pub crate_id: i32, - pub created_at: NaiveDateTime, - #[diesel(deserialize_as = String)] - pub token: SecretString, -} - -impl CrateOwnerInvitation { +impl NewCrateOwnerInvitation { pub async fn create( - invite: &NewCrateOwnerInvitation, + &self, conn: &mut AsyncPgConnection, config: &config::Server, ) -> QueryResult { @@ -50,7 +38,7 @@ impl CrateOwnerInvitation { // 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((invite.invited_user_id, invite.crate_id)) + .find((self.invited_user_id, self.crate_id)) .for_update() .first(conn) .await @@ -68,7 +56,7 @@ impl CrateOwnerInvitation { .await?; let res: Option = diesel::insert_into(crate_owner_invitations::table) - .values(invite) + .values(self) // The ON CONFLICT DO NOTHING clause results in not creating the invite if another one // already exists. This does not cause problems with expired invitation as those are // deleted before doing this INSERT. @@ -84,7 +72,21 @@ impl CrateOwnerInvitation { None => NewCrateOwnerInvitationOutcome::AlreadyExists, }) } +} +/// The model representing a row in the `crate_owner_invitations` database table. +#[derive(Clone, Debug, Identifiable, Queryable)] +#[diesel(primary_key(invited_user_id, crate_id))] +pub struct CrateOwnerInvitation { + pub invited_user_id: i32, + pub invited_by_user_id: i32, + pub crate_id: i32, + pub created_at: NaiveDateTime, + #[diesel(deserialize_as = String)] + pub token: SecretString, +} + +impl CrateOwnerInvitation { pub async fn find_by_id( user_id: i32, crate_id: i32, diff --git a/src/models/krate.rs b/src/models/krate.rs index 43b2c9129a0..8f60f639479 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -13,8 +13,8 @@ use crate::controllers::helpers::pagination::*; use crate::models::helpers::with_count::*; use crate::models::version::TopVersions; use crate::models::{ - CrateOwner, CrateOwnerInvitation, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, - Owner, OwnerKind, ReverseDependency, User, Version, + CrateOwner, NewCrateOwnerInvitation, NewCrateOwnerInvitationOutcome, Owner, OwnerKind, + ReverseDependency, User, Version, }; use crate::schema::*; use crate::util::errors::{bad_request, version_not_found, AppResult}; @@ -407,7 +407,8 @@ impl Crate { crate_id: self.id, }; - let creation_ret = CrateOwnerInvitation::create(&invite, conn, &app.config) + let creation_ret = invite + .create(conn, &app.config) .await .map_err(BoxedAppError::from)?;