Merge pull request #10568 from Turbo87/new-user-cleanup

Simplify `NewUser` struct

Author: Turbo87

src/controllers/session.rs

@@ -3,14 +3,16 @@ use axum::Json;
 use axum_extra::json;
 use axum_extra::response::ErasedJson;
 use diesel::prelude::*;
-use diesel_async::{AsyncPgConnection, RunQueryDsl};
+use diesel_async::scoped_futures::ScopedFutureExt;
+use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
 use http::request::Parts;
 use oauth2::{AuthorizationCode, CsrfToken, Scope, TokenResponse};
 
 use crate::app::AppState;
+use crate::controllers::user::update::UserConfirmEmail;
 use crate::email::Emails;
 use crate::middleware::log_request::RequestLogExt;
-use crate::models::{NewUser, User};
+use crate::models::{NewEmail, NewUser, User};
 use crate::schema::users;
 use crate::util::diesel::is_read_only_error;
 use crate::util::errors::{bad_request, server_error, AppResult};
@@ -134,36 +136,70 @@ pub async fn authorize_session(
     super::user::me::get_authenticated_user(app, req).await
 }
 
-async fn save_user_to_database(
+pub async fn save_user_to_database(
     user: &GithubUser,
     access_token: &str,
     emails: &Emails,
     conn: &mut AsyncPgConnection,
-) -> AppResult<User> {
-    let new_user = NewUser::new(
-        user.id,
-        &user.login,
-        user.name.as_deref(),
-        user.avatar_url.as_deref(),
-        access_token,
-    );
-
-    match new_user
-        .create_or_update(user.email.as_deref(), emails, conn)
-        .await
-    {
+) -> QueryResult<User> {
+    let new_user = NewUser::builder()
+        .gh_id(user.id)
+        .gh_login(&user.login)
+        .maybe_name(user.name.as_deref())
+        .maybe_gh_avatar(user.avatar_url.as_deref())
+        .gh_access_token(access_token)
+        .build();
+
+    match create_or_update_user(&new_user, user.email.as_deref(), emails, conn).await {
         Ok(user) => Ok(user),
         Err(error) if is_read_only_error(&error) => {
             // If we're in read only mode, we can't update their details
             // just look for an existing user
-            find_user_by_gh_id(conn, user.id)
-                .await?
-                .ok_or_else(|| error.into())
+            find_user_by_gh_id(conn, user.id).await?.ok_or(error)
         }
-        Err(error) => Err(error.into()),
+        Err(error) => Err(error),
     }
 }
 
+/// Inserts the user into the database, or updates an existing one.
+///
+/// This method also inserts the email address into the `emails` table
+/// and sends a confirmation email to the user.
+async fn create_or_update_user(
+    new_user: &NewUser<'_>,
+    email: Option<&str>,
+    emails: &Emails,
+    conn: &mut AsyncPgConnection,
+) -> QueryResult<User> {
+    conn.transaction(|conn| {
+        async move {
+            let user = new_user.insert_or_update(conn).await?;
+
+            // To send the user an account verification email
+            if let Some(user_email) = email {
+                let new_email = NewEmail::builder()
+                    .user_id(user.id)
+                    .email(user_email)
+                    .build();
+
+                if let Some(token) = new_email.insert_if_missing(conn).await? {
+                    // Swallows any error. Some users might insert an invalid email address here.
+                    let email = UserConfirmEmail {
+                        user_name: &user.gh_login,
+                        domain: &emails.domain,
+                        token,
+                    };
+                    let _ = emails.send(user_email, email).await;
+                }
+            }
+
+            Ok(user)
+        }
+        .scope_boxed()
+    })
+    .await
+}
+
 async fn find_user_by_gh_id(conn: &mut AsyncPgConnection, gh_id: i32) -> QueryResult<Option<User>> {
     users::table
         .filter(users::gh_id.eq(gh_id))

src/models/user.rs

@@ -1,14 +1,15 @@
+use bon::Builder;
 use chrono::NaiveDateTime;
+use diesel::dsl::sql;
 use diesel::prelude::*;
-use diesel_async::scoped_futures::ScopedFutureExt;
-use diesel_async::{AsyncConnection, AsyncPgConnection, RunQueryDsl};
+use diesel::sql_types::Integer;
+use diesel::upsert::excluded;
+use diesel_async::{AsyncPgConnection, RunQueryDsl};
 
 use crate::app::App;
-use crate::controllers::user::update::UserConfirmEmail;
-use crate::email::Emails;
 use crate::util::errors::AppResult;
 
-use crate::models::{Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights};
+use crate::models::{Crate, CrateOwner, Email, Owner, OwnerKind, Rights};
 use crate::schema::{crate_owners, emails, users};
 use crates_io_diesel_helpers::lower;
 
@@ -106,7 +107,7 @@ impl User {
 }
 
 /// Represents a new user record insertable to the `users` table
-#[derive(Insertable, Debug, Default)]
+#[derive(Insertable, Debug, Builder)]
 #[diesel(table_name = users, check_for_backend(diesel::pg::Pg))]
 pub struct NewUser<'a> {
     pub gh_id: i32,
@@ -116,80 +117,36 @@ pub struct NewUser<'a> {
     pub gh_access_token: &'a str,
 }
 
-impl<'a> NewUser<'a> {
-    pub fn new(
-        gh_id: i32,
-        gh_login: &'a str,
-        name: Option<&'a str>,
-        gh_avatar: Option<&'a str>,
-        gh_access_token: &'a str,
-    ) -> Self {
-        NewUser {
-            gh_id,
-            gh_login,
-            name,
-            gh_avatar,
-            gh_access_token,
-        }
+impl NewUser<'_> {
+    /// Inserts the user into the database, or fails if the user already exists.
+    pub async fn insert(&self, conn: &mut AsyncPgConnection) -> QueryResult<User> {
+        diesel::insert_into(users::table)
+            .values(self)
+            .get_result(conn)
+            .await
     }
 
     /// Inserts the user into the database, or updates an existing one.
-    pub async fn create_or_update(
-        &self,
-        email: Option<&'a str>,
-        emails: &Emails,
-        conn: &mut AsyncPgConnection,
-    ) -> QueryResult<User> {
-        use diesel::dsl::sql;
-        use diesel::insert_into;
-        use diesel::pg::upsert::excluded;
-        use diesel::sql_types::Integer;
-
-        conn.transaction(|conn| {
-            async move {
-                let user: User = insert_into(users::table)
-                    .values(self)
-                    // We need the `WHERE gh_id > 0` condition here because `gh_id` set
-                    // to `-1` indicates that we were unable to find a GitHub ID for
-                    // the associated GitHub login at the time that we backfilled
-                    // GitHub IDs. Therefore, there are multiple records in production
-                    // that have a `gh_id` of `-1` so we need to exclude those when
-                    // considering uniqueness of `gh_id` values. The `> 0` condition isn't
-                    // necessary for most fields in the database to be used as a conflict
-                    // target :)
-                    .on_conflict(sql::<Integer>("(gh_id) WHERE gh_id > 0"))
-                    .do_update()
-                    .set((
-                        users::gh_login.eq(excluded(users::gh_login)),
-                        users::name.eq(excluded(users::name)),
-                        users::gh_avatar.eq(excluded(users::gh_avatar)),
-                        users::gh_access_token.eq(excluded(users::gh_access_token)),
-                    ))
-                    .get_result(conn)
-                    .await?;
-
-                // To send the user an account verification email
-                if let Some(user_email) = email {
-                    let new_email = NewEmail::builder()
-                        .user_id(user.id)
-                        .email(user_email)
-                        .build();
-
-                    if let Some(token) = new_email.insert_if_missing(conn).await? {
-                        // Swallows any error. Some users might insert an invalid email address here.
-                        let email = UserConfirmEmail {
-                            user_name: &user.gh_login,
-                            domain: &emails.domain,
-                            token,
-                        };
-                        let _ = emails.send(user_email, email).await;
-                    }
-                }
-
-                Ok(user)
-            }
-            .scope_boxed()
-        })
-        .await
+    pub async fn insert_or_update(&self, conn: &mut AsyncPgConnection) -> QueryResult<User> {
+        diesel::insert_into(users::table)
+            .values(self)
+            // We need the `WHERE gh_id > 0` condition here because `gh_id` set
+            // to `-1` indicates that we were unable to find a GitHub ID for
+            // the associated GitHub login at the time that we backfilled
+            // GitHub IDs. Therefore, there are multiple records in production
+            // that have a `gh_id` of `-1` so we need to exclude those when
+            // considering uniqueness of `gh_id` values. The `> 0` condition isn't
+            // necessary for most fields in the database to be used as a conflict
+            // target :)
+            .on_conflict(sql::<Integer>("(gh_id) WHERE gh_id > 0"))
+            .do_update()
+            .set((
+                users::gh_login.eq(excluded(users::gh_login)),
+                users::name.eq(excluded(users::name)),
+                users::gh_avatar.eq(excluded(users::gh_avatar)),
+                users::gh_access_token.eq(excluded(users::gh_access_token)),
+            ))
+            .get_result(conn)
+            .await
     }
 }

src/rate_limiter.rs

@@ -183,7 +183,6 @@ struct Bucket {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::schema::users;
     use crates_io_test_db::TestDatabase;
 
     #[tokio::test]
@@ -702,16 +701,14 @@ mod tests {
     async fn new_user(conn: &mut AsyncPgConnection, gh_login: &str) -> QueryResult<i32> {
         use crate::models::NewUser;
 
-        let user = NewUser {
-            gh_login,
-            ..NewUser::default()
-        };
-
-        diesel::insert_into(users::table)
-            .values(user)
-            .returning(users::id)
-            .get_result(conn)
+        NewUser::builder()
+            .gh_id(0)
+            .gh_login(gh_login)
+            .gh_access_token("some random token")
+            .build()
+            .insert(conn)
             .await
+            .map(|user| user.id)
     }
 
     async fn new_user_bucket(

src/tests/mod.rs

@@ -93,13 +93,11 @@ pub struct OwnerResp {
 }
 
 fn new_user(login: &str) -> NewUser<'_> {
-    NewUser {
-        gh_id: next_gh_id(),
-        gh_login: login,
-        name: None,
-        gh_avatar: None,
-        gh_access_token: "some random token",
-    }
+    NewUser::builder()
+        .gh_id(next_gh_id())
+        .gh_login(login)
+        .gh_access_token("some random token")
+        .build()
 }
 
 fn new_team(login: &str) -> NewTeam<'_> {

src/tests/owners.rs

@@ -8,7 +8,6 @@ use crate::views::{
     EncodableCrateOwnerInvitationV1, EncodableOwner, EncodablePublicUser, InvitationResponse,
 };
 
-use crate::schema::users;
 use chrono::Utc;
 use diesel::prelude::*;
 use diesel_async::RunQueryDsl;
@@ -770,17 +769,12 @@ async fn inactive_users_dont_get_invitations() {
     let invited_gh_login = "user_bar";
     let krate_name = "inactive_test";
 
-    let user = NewUser {
-        gh_id: -1,
-        gh_login: invited_gh_login,
-        name: None,
-        gh_avatar: None,
-        gh_access_token: "some random token",
-    };
-
-    diesel::insert_into(users::table)
-        .values(user)
-        .execute(&mut conn)
+    NewUser::builder()
+        .gh_id(-1)
+        .gh_login(invited_gh_login)
+        .gh_access_token("some random token")
+        .build()
+        .insert(&mut conn)
         .await
         .unwrap();
 

src/tests/routes/crates/list.rs

@@ -1,5 +1,5 @@
 use crate::models::Category;
-use crate::schema::{crates, users};
+use crate::schema::crates;
 use crate::tests::builders::{CrateBuilder, VersionBuilder};
 use crate::tests::util::{RequestHelper, TestApp};
 use crate::tests::{new_category, new_user};
@@ -22,11 +22,7 @@ async fn index() -> anyhow::Result<()> {
         assert_eq!(json.meta.total, 0);
     }
 
-    let user_id = insert_into(users::table)
-        .values(new_user("foo"))
-        .returning(users::id)
-        .get_result(&mut conn)
-        .await?;
+    let user_id = new_user("foo").insert(&mut conn).await?.id;
 
     let krate = CrateBuilder::new("fooindex", user_id)
         .expect_build(&mut conn)

src/tests/routes/me/email_notifications.rs

@@ -1,4 +1,4 @@
-use crate::schema::{crate_owners, users};
+use crate::schema::crate_owners;
 use crate::tests::builders::CrateBuilder;
 use crate::tests::new_user;
 use crate::tests::util::{RequestHelper, TestApp};
@@ -115,12 +115,11 @@ async fn test_update_email_notifications_not_owned() {
     let (app, _, user) = TestApp::init().with_user().await;
     let mut conn = app.db_conn().await;
 
-    let user_id = diesel::insert_into(users::table)
-        .values(new_user("arbitrary_username"))
-        .returning(users::id)
-        .get_result(&mut conn)
+    let user_id = new_user("arbitrary_username")
+        .insert(&mut conn)
         .await
-        .unwrap();
+        .unwrap()
+        .id;
 
     let not_my_crate = CrateBuilder::new("test_package", user_id)
         .expect_build(&mut conn)