From 521beb744195a77b4907c2f1fc46c7af5464f731 Mon Sep 17 00:00:00 2001 From: mugoh Date: Tue, 12 Nov 2019 07:14:09 +0300 Subject: [PATCH 1/7] remove uses of email field on User --- .../20140924113530_dumped_migration_1/up.sql | 3 +- .../down.sql | 1 + .../up.sql | 2 ++ src/controllers/user/me.rs | 11 ++----- src/controllers/user/session.rs | 3 +- src/models/user.rs | 33 +++++++++++++------ src/publish_rate_limit.rs | 2 +- src/schema.rs | 6 ---- src/tasks/update_downloads.rs | 4 +-- src/views.rs | 2 +- 10 files changed, 36 insertions(+), 31 deletions(-) create mode 100644 migrations/2019-11-11-162609_drop_email_from_user/down.sql create mode 100644 migrations/2019-11-11-162609_drop_email_from_user/up.sql diff --git a/migrations/20140924113530_dumped_migration_1/up.sql b/migrations/20140924113530_dumped_migration_1/up.sql index 84c269232ac..72de307bfd6 100644 --- a/migrations/20140924113530_dumped_migration_1/up.sql +++ b/migrations/20140924113530_dumped_migration_1/up.sql @@ -3,4 +3,5 @@ CREATE TABLE users ( email VARCHAR NOT NULL UNIQUE, gh_access_token VARCHAR NOT NULL, api_token VARCHAR NOT NULL - ); \ No newline at end of file + ); + diff --git a/migrations/2019-11-11-162609_drop_email_from_user/down.sql b/migrations/2019-11-11-162609_drop_email_from_user/down.sql new file mode 100644 index 00000000000..2a2ec6d386a --- /dev/null +++ b/migrations/2019-11-11-162609_drop_email_from_user/down.sql @@ -0,0 +1 @@ +-- Not reversible diff --git a/migrations/2019-11-11-162609_drop_email_from_user/up.sql b/migrations/2019-11-11-162609_drop_email_from_user/up.sql new file mode 100644 index 00000000000..ae40642a5cb --- /dev/null +++ b/migrations/2019-11-11-162609_drop_email_from_user/up.sql @@ -0,0 +1,2 @@ +-- Your SQL goes here +ALTER TABLE users DROP COLUMN email CASCADE; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 295e3259a3f..b0caf74d649 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -38,10 +38,10 @@ pub fn me(req: &mut dyn Request) -> CargoResult { let verified = verified.unwrap_or(false); let verification_sent = verified || verification_sent; - let user = User { email, ..user }; + let user = User { ..user }; Ok(req.json(&EncodableMe { - user: user.encodable_private(verified, verification_sent), + user: user.encodable_private(email, verified, verification_sent)?, })) } @@ -91,8 +91,7 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { /// Handles the `PUT /user/:user_id` route. pub fn update_user(req: &mut dyn Request) -> CargoResult { use self::emails::user_id; - use self::users::dsl::{email, gh_login, users}; - use diesel::{insert_into, update}; + use diesel::insert_into; let mut body = String::new(); req.body().read_to_string(&mut body)?; @@ -130,10 +129,6 @@ pub fn update_user(req: &mut dyn Request) -> CargoResult { } conn.transaction::<_, Box, _>(|| { - update(users.filter(gh_login.eq(&user.gh_login))) - .set(email.eq(user_email)) - .execute(&*conn)?; - let new_email = NewEmail { user_id: user.id, email: user_email, diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index fdbdd5f099d..219790b158a 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -118,12 +118,11 @@ impl GithubUser { NewUser::new( self.id, &self.login, - self.email.as_ref().map(|s| &s[..]), self.name.as_ref().map(|s| &s[..]), self.avatar_url.as_ref().map(|s| &s[..]), access_token, ) - .create_or_update(conn) + .create_or_update(self.email.as_ref().map(|s| &s[..]), conn) .map_err(Into::into) .or_else(|e: Box| { // If we're in read only mode, we can't update their details diff --git a/src/models/user.rs b/src/models/user.rs index 2f22c20a19e..f6c0e46485c 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -13,7 +13,6 @@ use crate::views::{EncodablePrivateUser, EncodablePublicUser}; #[derive(Clone, Debug, PartialEq, Eq, Queryable, Identifiable, AsChangeset, Associations)] pub struct User { pub id: i32, - pub email: Option, pub gh_access_token: String, pub gh_login: String, pub name: Option, @@ -21,12 +20,12 @@ pub struct User { pub gh_id: i32, } +/// Represents a new user record insertible to the `users` table #[derive(Insertable, Debug, Default)] #[table_name = "users"] pub struct NewUser<'a> { pub gh_id: i32, pub gh_login: &'a str, - pub email: Option<&'a str>, pub name: Option<&'a str>, pub gh_avatar: Option<&'a str>, pub gh_access_token: Cow<'a, str>, @@ -36,7 +35,6 @@ impl<'a> NewUser<'a> { pub fn new( gh_id: i32, gh_login: &'a str, - email: Option<&'a str>, name: Option<&'a str>, gh_avatar: Option<&'a str>, gh_access_token: &'a str, @@ -44,7 +42,6 @@ impl<'a> NewUser<'a> { NewUser { gh_id, gh_login, - email, name, gh_avatar, gh_access_token: Cow::Borrowed(gh_access_token), @@ -52,7 +49,11 @@ impl<'a> NewUser<'a> { } /// Inserts the user into the database, or updates an existing one. - pub fn create_or_update(&self, conn: &PgConnection) -> QueryResult { + pub fn create_or_update( + &self, + email: Option<&'a str>, + conn: &PgConnection, + ) -> QueryResult { use crate::schema::users::dsl::*; use diesel::dsl::sql; use diesel::insert_into; @@ -81,7 +82,7 @@ impl<'a> NewUser<'a> { .get_result::(conn)?; // To send the user an account verification email... - if let Some(user_email) = user.email.as_ref() { + if let Some(user_email) = email { let new_email = NewEmail { user_id: user.id, email: user_email, @@ -168,6 +169,8 @@ impl User { Ok(best) } + /// Queries the database for the verified emails + /// belonging to a given user pub fn verified_email(&self, conn: &PgConnection) -> CargoResult> { Ok(Email::belonging_to(self) .select(emails::email) @@ -179,19 +182,21 @@ impl User { /// Converts this `User` model into an `EncodablePrivateUser` for JSON serialization. pub fn encodable_private( self, + email: Option, + email_verified: bool, email_verification_sent: bool, - ) -> EncodablePrivateUser { + ) -> CargoResult { let User { id, - email, name, gh_login, gh_avatar, .. } = self; let url = format!("https://github.com/{}", gh_login); - EncodablePrivateUser { + + Ok(EncodablePrivateUser { id, email, email_verified, @@ -200,7 +205,15 @@ impl User { login: gh_login, name, url: Some(url), - } + }) + } + + /// Queries for the email belonging to a particular user + pub fn email(&self, conn: &PgConnection) -> CargoResult> { + Ok(Email::belonging_to(self) + .select(emails::email) + .first::(&*conn) + .optional()?) } /// Converts this`User` model into an `EncodablePublicUser` for JSON serialization. diff --git a/src/publish_rate_limit.rs b/src/publish_rate_limit.rs index d21d47ab574..53c95dfe713 100644 --- a/src/publish_rate_limit.rs +++ b/src/publish_rate_limit.rs @@ -325,7 +325,7 @@ mod tests { gh_login, ..NewUser::default() } - .create_or_update(conn)?; + .create_or_update(None, conn)?; Ok(user.id) } diff --git a/src/schema.rs b/src/schema.rs index 6b692eeada9..c4051902f9c 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -745,12 +745,6 @@ table! { /// /// (Automatically generated by Diesel.) id -> Int4, - /// The `email` column of the `users` table. - /// - /// Its SQL type is `Nullable`. - /// - /// (Automatically generated by Diesel.) - email -> Nullable, /// The `gh_access_token` column of the `users` table. /// /// Its SQL type is `Varchar`. diff --git a/src/tasks/update_downloads.rs b/src/tasks/update_downloads.rs index 166ad2540d7..6feb218b3dc 100644 --- a/src/tasks/update_downloads.rs +++ b/src/tasks/update_downloads.rs @@ -93,8 +93,8 @@ mod test { } fn user(conn: &PgConnection) -> User { - NewUser::new(2, "login", None, None, None, "access_token") - .create_or_update(conn) + NewUser::new(2, "login", None, None, "access_token") + .create_or_update(None, conn) .unwrap() } diff --git a/src/views.rs b/src/views.rs index 9506ac00a05..9f6555742b4 100644 --- a/src/views.rs +++ b/src/views.rs @@ -161,10 +161,10 @@ pub struct EncodableMe { pub struct EncodablePrivateUser { pub id: i32, pub login: String, - pub email: Option, pub email_verified: bool, pub email_verification_sent: bool, pub name: Option, + pub email: Option, pub avatar: Option, pub url: Option, } From 5ef002dc2cdc781892b83ec0a708c4e3e30e0591 Mon Sep 17 00:00:00 2001 From: mugoh Date: Tue, 12 Nov 2019 08:37:01 +0300 Subject: [PATCH 2/7] update tests to match altered fields --- src/tests/all.rs | 1 - src/tests/krate.rs | 2 +- src/tests/token.rs | 2 +- src/tests/user.rs | 34 ++++++++++++++++++---------------- src/tests/util.rs | 7 ++++--- 5 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/tests/all.rs b/src/tests/all.rs index 7be4db4f9b7..84c1d8f7d25 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -220,7 +220,6 @@ fn new_user(login: &str) -> NewUser<'_> { NewUser { gh_id: NEXT_GH_ID.fetch_add(1, Ordering::SeqCst) as i32, gh_login: login, - email: None, name: None, gh_avatar: None, gh_access_token: Cow::Borrowed("some random token"), diff --git a/src/tests/krate.rs b/src/tests/krate.rs index accaa6579c2..7369d598c82 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -86,7 +86,7 @@ fn index() { assert_eq!(json.meta.total, 0); let krate = app.db(|conn| { - let u = new_user("foo").create_or_update(conn).unwrap(); + let u = new_user("foo").create_or_update(None, conn).unwrap(); CrateBuilder::new("fooindex", u.id).expect_build(conn) }); diff --git a/src/tests/token.rs b/src/tests/token.rs index e878f96de84..0cc92314332 100644 --- a/src/tests/token.rs +++ b/src/tests/token.rs @@ -267,7 +267,7 @@ fn token_gives_access_to_me() { anon.get(url).assert_forbidden(); let json: UserShowPrivateResponse = token.get(url).good(); - assert_eq!(json.user.email, user.as_model().email); + assert_eq!(json.user.name, user.as_model().name); } #[test] diff --git a/src/tests/user.rs b/src/tests/user.rs index db90c51e106..9a5382041f7 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -110,7 +110,7 @@ fn me() { let user = app.db_new_user("foo"); let json = user.show_me(); - assert_eq!(json.user.email, user.as_model().email); + assert_eq!(json.user.name, user.as_model().name); } #[test] @@ -141,21 +141,19 @@ fn show_latest_user_case_insensitively() { t!(NewUser::new( 1, "foobar", - Some("foo@bar.com"), Some("I was first then deleted my github account"), None, "bar" ) - .create_or_update(conn)); + .create_or_update(None, conn)); t!(NewUser::new( 2, "FOOBAR", - Some("later-foo@bar.com"), Some("I was second, I took the foobar username on github"), None, "bar" ) - .create_or_update(conn)); + .create_or_update(None, conn)); }); let json: UserShowPublicResponse = anon.get("api/v1/users/fOObAr").good(); @@ -324,7 +322,7 @@ fn updating_existing_user_doesnt_change_api_token() { let user = app.db(|conn| { // Reuse gh_id but use new gh_login and gh_access_token - t!(NewUser::new(gh_id, "bar", None, None, None, "bar_token").create_or_update(conn)); + t!(NewUser::new(gh_id, "bar", None, None, "bar_token").create_or_update(None, conn)); // Use the original API token to find the now updated user t!(User::find_by_api_token(conn, token)) @@ -353,7 +351,7 @@ fn github_without_email_does_not_overwrite_email() { // Don't use app.db_new_user because it adds a verified email. let user_without_github_email = app.db(|conn| { let u = new_user("arbitrary_username"); - let u = u.create_or_update(conn).unwrap(); + let u = u.create_or_update(None, conn).unwrap(); MockCookieUser::new(&app, u) }); let user_without_github_email_model = user_without_github_email.as_model(); @@ -373,7 +371,7 @@ fn github_without_email_does_not_overwrite_email() { // new_user uses a None email; the rest of the fields are arbitrary ..new_user("arbitrary_username") }; - let u = u.create_or_update(conn).unwrap(); + let u = u.create_or_update(None, conn).unwrap(); MockCookieUser::new(&app, u) }); @@ -386,9 +384,16 @@ fn github_without_email_does_not_overwrite_email() { */ #[test] fn github_with_email_does_not_overwrite_email() { + use cargo_registry::schema::emails; + let (app, _, user) = TestApp::init().with_user(); let model = user.as_model(); - let original_email = &model.email; + let original_email = app.db(|conn| { + Email::belonging_to(model) + .select(emails::email) + .first::(&*conn) + .unwrap() + }); let new_github_email = "new-email-in-github@example.com"; @@ -397,16 +402,15 @@ fn github_with_email_does_not_overwrite_email() { let u = NewUser { // Use the same github ID to link to the existing account gh_id: model.gh_id, - email: Some(new_github_email), // the rest of the fields are arbitrary ..new_user("arbitrary_username") }; - let u = u.create_or_update(conn).unwrap(); + let u = u.create_or_update(Some(new_github_email), conn).unwrap(); MockCookieUser::new(&app, u) }); let json = user_with_different_email_in_github.show_me(); - assert_eq!(json.user.email, *original_email); + assert_eq!(json.user.email, Some(original_email)); } /* Given a crates.io user, check that the user's email can be @@ -512,10 +516,9 @@ fn test_confirm_user_email() { // email directly into the database and we want to test the verification flow here. let user = app.db(|conn| { let u = NewUser { - email: Some("potato2@example.com"), ..new_user("arbitrary_username") }; - let u = u.create_or_update(conn).unwrap(); + let u = u.create_or_update(None, conn).unwrap(); MockCookieUser::new(&app, u) }); let user_model = user.as_model(); @@ -551,10 +554,9 @@ fn test_existing_user_email() { // email directly into the database and we want to test the verification flow here. let user = app.db(|conn| { let u = NewUser { - email: Some("potahto@example.com"), ..new_user("arbitrary_username") }; - let u = u.create_or_update(conn).unwrap(); + let u = u.create_or_update(None, conn).unwrap(); update(Email::belonging_to(&u)) // Users created before we added verification will have // `NULL` in the `token_generated_at` column. diff --git a/src/tests/util.rs b/src/tests/util.rs index a17b171da45..25fb09e1d57 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -135,9 +135,10 @@ impl TestApp { use diesel::prelude::*; let user = self.db(|conn| { - let mut user = crate::new_user(username).create_or_update(conn).unwrap(); - let email = "something@example.com"; - user.email = Some(email.to_string()); + let email = "cow@mammals.milk"; + let user = crate::new_user(username) + .create_or_update(Some(email), conn) + .unwrap(); diesel::insert_into(emails::table) .values(( emails::user_id.eq(user.id), From 3544b91086e822607cfeadda57b261e66112ba52 Mon Sep 17 00:00:00 2001 From: mugoh Date: Wed, 13 Nov 2019 15:58:23 +0300 Subject: [PATCH 3/7] remove email field from dump - `do_nothing` on `emails` insertion violates unique constraint on `user_id` --- migrations/20161115111957_dumped_migration_119/up.sql | 2 +- src/models/user.rs | 5 ++++- src/tasks/dump_db/dump-db.toml | 1 - 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/migrations/20161115111957_dumped_migration_119/up.sql b/migrations/20161115111957_dumped_migration_119/up.sql index 02593daa376..00193b6b223 100644 --- a/migrations/20161115111957_dumped_migration_119/up.sql +++ b/migrations/20161115111957_dumped_migration_119/up.sql @@ -1 +1 @@ -CREATE FUNCTION update_categories_crates_cnt() RETURNS trigger AS $$ BEGIN IF (TG_OP = 'INSERT') THEN UPDATE categories SET crates_cnt = crates_cnt + 1 WHERE id = NEW.category_id; return NEW; ELSIF (TG_OP = 'DELETE') THEN UPDATE categories SET crates_cnt = crates_cnt - 1 WHERE id = OLD.category_id; return OLD; END IF; END $$ LANGUAGE plpgsql; CREATE TRIGGER trigger_update_categories_crates_cnt BEFORE INSERT OR DELETE ON crates_categories FOR EACH ROW EXECUTE PROCEDURE update_categories_crates_cnt(); CREATE TRIGGER touch_crate_on_modify_categories AFTER INSERT OR DELETE ON crates_categories FOR EACH ROW EXECUTE PROCEDURE touch_crate(); \ No newline at end of file +CREATE FUNCTION update_categories_crates_cnt() RETURNS trigger AS $$ BEGIN IF (TG_OP = 'INSERT') THEN UPDATE categories SET crates_cnt = crates_cnt + 1 WHERE id = NEW.category_id; return NEW; ELSIF (TG_OP = 'DELETE') THEN UPDATE categories SET crates_cnt = crates_cnt - 1 WHERE id = OLD.category_id; return OLD; END IF; END $$ LANGUAGE plpgsql; CREATE TRIGGER trigger_update_categories_crates_cnt BEFORE INSERT OR DELETE ON crates_categories FOR EACH ROW EXECUTE PROCEDURE update_categories_crates_cnt(); CREATE TRIGGER touch_crate_on_modify_categories AFTER INSERT OR DELETE ON crates_categories FOR EACH ROW EXECUTE PROCEDURE touch_crate(); diff --git a/src/models/user.rs b/src/models/user.rs index f6c0e46485c..eb35abcb252 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -54,6 +54,7 @@ impl<'a> NewUser<'a> { email: Option<&'a str>, conn: &PgConnection, ) -> QueryResult { + use crate::schema::emails::columns::user_id; use crate::schema::users::dsl::*; use diesel::dsl::sql; use diesel::insert_into; @@ -90,7 +91,9 @@ impl<'a> NewUser<'a> { let token = insert_into(emails::table) .values(&new_email) - .on_conflict_do_nothing() + .on_conflict(user_id) + .do_update() + .set(&new_email) .returning(emails::token) .get_result::(conn) .optional()?; diff --git a/src/tasks/dump_db/dump-db.toml b/src/tasks/dump_db/dump-db.toml index 12302c891ba..ee3d5900231 100644 --- a/src/tasks/dump_db/dump-db.toml +++ b/src/tasks/dump_db/dump-db.toml @@ -165,7 +165,6 @@ id in ( )""" [users.columns] id = "public" -email = "private" gh_access_token = "private" gh_login = "public" name = "public" From fc01ad47d47ffa285c24f017522dd43b09d8bbff Mon Sep 17 00:00:00 2001 From: mugoh Date: Fri, 15 Nov 2019 14:47:02 +0300 Subject: [PATCH 4/7] add requested changes --- migrations/2019-11-11-162609_drop_email_from_user/up.sql | 3 +-- src/controllers/user/me.rs | 3 +-- src/models/user.rs | 9 ++++----- src/tests/util.rs | 3 ++- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/migrations/2019-11-11-162609_drop_email_from_user/up.sql b/migrations/2019-11-11-162609_drop_email_from_user/up.sql index ae40642a5cb..fef46f3e4e4 100644 --- a/migrations/2019-11-11-162609_drop_email_from_user/up.sql +++ b/migrations/2019-11-11-162609_drop_email_from_user/up.sql @@ -1,2 +1 @@ --- Your SQL goes here -ALTER TABLE users DROP COLUMN email CASCADE; +ALTER TABLE users DROP COLUMN email CASCADE; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index b0caf74d649..563201a92e4 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -38,10 +38,9 @@ pub fn me(req: &mut dyn Request) -> CargoResult { let verified = verified.unwrap_or(false); let verification_sent = verified || verification_sent; - let user = User { ..user }; Ok(req.json(&EncodableMe { - user: user.encodable_private(email, verified, verification_sent)?, + user: user.encodable_private(email, verified, verification_sent), })) } diff --git a/src/models/user.rs b/src/models/user.rs index eb35abcb252..2e25fa34388 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -20,7 +20,7 @@ pub struct User { pub gh_id: i32, } -/// Represents a new user record insertible to the `users` table +/// Represents a new user record insertable to the `users` table #[derive(Insertable, Debug, Default)] #[table_name = "users"] pub struct NewUser<'a> { @@ -186,10 +186,9 @@ impl User { pub fn encodable_private( self, email: Option, - email_verified: bool, email_verification_sent: bool, - ) -> CargoResult { + ) -> EncodablePrivateUser { let User { id, name, @@ -199,7 +198,7 @@ impl User { } = self; let url = format!("https://github.com/{}", gh_login); - Ok(EncodablePrivateUser { + EncodablePrivateUser { id, email, email_verified, @@ -208,7 +207,7 @@ impl User { login: gh_login, name, url: Some(url), - }) + } } /// Queries for the email belonging to a particular user diff --git a/src/tests/util.rs b/src/tests/util.rs index 25fb09e1d57..14d94dc2263 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -136,8 +136,9 @@ impl TestApp { let user = self.db(|conn| { let email = "cow@mammals.milk"; + let user = crate::new_user(username) - .create_or_update(Some(email), conn) + .create_or_update(None, conn) .unwrap(); diesel::insert_into(emails::table) .values(( From 3be2b553194b7004b034ffb7f5b6efc6c075be3d Mon Sep 17 00:00:00 2001 From: mugoh Date: Fri, 15 Nov 2019 15:27:41 +0300 Subject: [PATCH 5/7] retain original email - retain email registered with when github email is modified --- src/models/user.rs | 5 +---- src/tests/user.rs | 9 ++++++--- src/tests/util.rs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/models/user.rs b/src/models/user.rs index 2e25fa34388..c12213d4d4d 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -54,7 +54,6 @@ impl<'a> NewUser<'a> { email: Option<&'a str>, conn: &PgConnection, ) -> QueryResult { - use crate::schema::emails::columns::user_id; use crate::schema::users::dsl::*; use diesel::dsl::sql; use diesel::insert_into; @@ -91,9 +90,7 @@ impl<'a> NewUser<'a> { let token = insert_into(emails::table) .values(&new_email) - .on_conflict(user_id) - .do_update() - .set(&new_email) + .on_conflict_do_nothing() .returning(emails::token) .get_result::(conn) .optional()?; diff --git a/src/tests/user.rs b/src/tests/user.rs index 9a5382041f7..18184bb9505 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -514,11 +514,13 @@ fn test_confirm_user_email() { // Simulate logging in via GitHub. Don't use app.db_new_user because it inserts a verified // email directly into the database and we want to test the verification flow here. + let email = "cow@mammals@milk"; + let user = app.db(|conn| { let u = NewUser { ..new_user("arbitrary_username") }; - let u = u.create_or_update(None, conn).unwrap(); + let u = u.create_or_update(Some(email), conn).unwrap(); MockCookieUser::new(&app, u) }); let user_model = user.as_model(); @@ -533,7 +535,7 @@ fn test_confirm_user_email() { user.confirm_email(&email_token); let json = user.show_me(); - assert_eq!(json.user.email.unwrap(), "potato2@example.com"); + assert_eq!(json.user.email.unwrap(), "cow@mammals@milk"); assert!(json.user.email_verified); assert!(json.user.email_verification_sent); } @@ -552,11 +554,12 @@ fn test_existing_user_email() { // Simulate logging in via GitHub. Don't use app.db_new_user because it inserts a verified // email directly into the database and we want to test the verification flow here. + let email = "potahto@example.com"; let user = app.db(|conn| { let u = NewUser { ..new_user("arbitrary_username") }; - let u = u.create_or_update(None, conn).unwrap(); + let u = u.create_or_update(Some(email), conn).unwrap(); update(Email::belonging_to(&u)) // Users created before we added verification will have // `NULL` in the `token_generated_at` column. diff --git a/src/tests/util.rs b/src/tests/util.rs index 14d94dc2263..054a44d92da 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -135,7 +135,7 @@ impl TestApp { use diesel::prelude::*; let user = self.db(|conn| { - let email = "cow@mammals.milk"; + let email = "something@example.com"; let user = crate::new_user(username) .create_or_update(None, conn) From 59f7169b3f62d4ea8e515bd83396d792c82a881d Mon Sep 17 00:00:00 2001 From: mugoh Date: Sat, 16 Nov 2019 18:29:37 +0300 Subject: [PATCH 6/7] use valid email pattern in test --- src/tests/user.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/user.rs b/src/tests/user.rs index 18184bb9505..0826dcd056c 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -514,7 +514,7 @@ fn test_confirm_user_email() { // Simulate logging in via GitHub. Don't use app.db_new_user because it inserts a verified // email directly into the database and we want to test the verification flow here. - let email = "cow@mammals@milk"; + let email = "potato2@example.com"; let user = app.db(|conn| { let u = NewUser { @@ -535,7 +535,7 @@ fn test_confirm_user_email() { user.confirm_email(&email_token); let json = user.show_me(); - assert_eq!(json.user.email.unwrap(), "cow@mammals@milk"); + assert_eq!(json.user.email.unwrap(), "potato2@example.com"); assert!(json.user.email_verified); assert!(json.user.email_verification_sent); } From 2410f44669769da12e45d946d88de230c75c42f6 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 2 Dec 2019 11:27:50 -0500 Subject: [PATCH 7/7] Remove more temporary code that was needed to remove user.email --- src/bin/transfer-crates.rs | 11 ++--- src/controllers/krate/metadata.rs | 22 ++++----- src/controllers/user/me.rs | 19 ++++---- src/controllers/user/other.rs | 7 +-- src/controllers/user/session.rs | 9 ++-- src/controllers/version/deprecated.rs | 18 +++---- src/middleware/current_user.rs | 7 +-- src/models/krate.rs | 7 +-- src/models/owner.rs | 5 +- src/models/user.rs | 68 +++------------------------ src/models/version.rs | 9 +--- 11 files changed, 47 insertions(+), 135 deletions(-) diff --git a/src/bin/transfer-crates.rs b/src/bin/transfer-crates.rs index 527937f9feb..76fd187c03c 100644 --- a/src/bin/transfer-crates.rs +++ b/src/bin/transfer-crates.rs @@ -7,7 +7,7 @@ use cargo_registry::{ db, - models::{user, Crate, OwnerKind, User}, + models::{Crate, OwnerKind, User}, schema::{crate_owners, crates, users}, }; use std::{ @@ -16,7 +16,6 @@ use std::{ process::exit, }; -use cargo_registry::models::user::UserNoEmailType; use diesel::prelude::*; fn main() { @@ -45,16 +44,12 @@ fn transfer(conn: &PgConnection) { }; let from = users::table - .select(user::ALL_COLUMNS) .filter(users::gh_login.eq(from)) - .first::(conn) - .map(User::from) + .first::(conn) .unwrap(); let to = users::table - .select(user::ALL_COLUMNS) .filter(users::gh_login.eq(to)) - .first::(conn) - .map(User::from) + .first::(conn) .unwrap(); if from.gh_id != to.gh_id { diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 3d063eb1389..9e02718f8c3 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -5,11 +5,9 @@ //! `Cargo.toml` file. use crate::controllers::prelude::*; -use crate::models::user; -use crate::models::user::UserNoEmailType; use crate::models::{ Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads, - Version, + User, Version, }; use crate::schema::*; use crate::views::{ @@ -107,10 +105,10 @@ pub fn show(req: &mut dyn Request) -> AppResult { let conn = req.db_conn()?; let krate = Crate::by_name(name).first::(&*conn)?; - let mut versions_and_publishers: Vec<(Version, Option)> = krate + let mut versions_and_publishers: Vec<(Version, Option)> = krate .all_versions() .left_outer_join(users::table) - .select((versions::all_columns, user::ALL_COLUMNS.nullable())) + .select((versions::all_columns, users::all_columns.nullable())) .load(&*conn)?; versions_and_publishers.sort_by(|a, b| b.0.num.cmp(&a.0.num)); let ids = versions_and_publishers.iter().map(|v| v.0.id).collect(); @@ -153,7 +151,7 @@ pub fn show(req: &mut dyn Request) -> AppResult { ), versions: versions_and_publishers .into_iter() - .map(|(v, pb)| v.encodable(&krate.name, pb.map(From::from))) + .map(|(v, pb)| v.encodable(&krate.name, pb)) .collect(), keywords: kws.into_iter().map(Keyword::encodable).collect(), categories: cats.into_iter().map(Category::encodable).collect(), @@ -189,15 +187,15 @@ pub fn versions(req: &mut dyn Request) -> AppResult { let crate_name = &req.params()["crate_id"]; let conn = req.db_conn()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; - let mut versions_and_publishers: Vec<(Version, Option)> = krate + let mut versions_and_publishers: Vec<(Version, Option)> = krate .all_versions() .left_outer_join(users::table) - .select((versions::all_columns, user::ALL_COLUMNS.nullable())) + .select((versions::all_columns, users::all_columns.nullable())) .load(&*conn)?; versions_and_publishers.sort_by(|a, b| b.0.num.cmp(&a.0.num)); let versions = versions_and_publishers .into_iter() - .map(|(v, pb)| v.encodable(crate_name, pb.map(From::from))) + .map(|(v, pb)| v.encodable(crate_name, pb)) .collect(); #[derive(Serialize)] @@ -229,11 +227,11 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> AppResult { .select(( versions::all_columns, crates::name, - user::ALL_COLUMNS.nullable(), + users::all_columns.nullable(), )) - .load::<(Version, String, Option)>(&*conn)? + .load::<(Version, String, Option)>(&*conn)? .into_iter() - .map(|(version, krate_name, user)| version.encodable(&krate_name, user.map(From::from))) + .map(|(version, krate_name, user)| version.encodable(&krate_name, user)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index a7e43dca31e..000bf273591 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -7,7 +7,6 @@ use crate::email; use crate::util::bad_request; use crate::util::errors::AppError; -use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; use crate::models::{CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version}; use crate::schema::{crate_owners, crates, emails, follows, users, versions}; use crate::views::{EncodableMe, EncodableVersion, OwnedCrate}; @@ -32,12 +31,12 @@ pub fn me(req: &mut dyn Request) -> AppResult { .find(user_id) .left_join(emails::table) .select(( - ALL_COLUMNS, + users::all_columns, emails::verified.nullable(), emails::email.nullable(), emails::token_generated_at.nullable().is_not_null(), )) - .first::<(UserNoEmailType, Option, Option, bool)>(&*conn)?; + .first::<(User, Option, Option, bool)>(&*conn)?; let owned_crates = crate_owners::table .inner_join(crates::table) @@ -57,7 +56,7 @@ pub fn me(req: &mut dyn Request) -> AppResult { let verified = verified.unwrap_or(false); let verification_sent = verified || verification_sent; Ok(req.json(&EncodableMe { - user: User::from(user).encodable_private(email, verified, verification_sent), + user: user.encodable_private(email, verified, verification_sent), owned_crates, })) } @@ -75,17 +74,19 @@ pub fn updates(req: &mut dyn Request) -> AppResult { .left_outer_join(users::table) .filter(crates::id.eq(any(followed_crates))) .order(versions::created_at.desc()) - .select((versions::all_columns, crates::name, ALL_COLUMNS.nullable())) + .select(( + versions::all_columns, + crates::name, + users::all_columns.nullable(), + )) .paginate(&req.query())? - .load::<(Version, String, Option)>(&*conn)?; + .load::<(Version, String, Option)>(&*conn)?; let more = data.next_page_params().is_some(); let versions = data .into_iter() - .map(|(version, crate_name, published_by)| { - version.encodable(&crate_name, published_by.map(From::from)) - }) + .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index 839e11d353b..b2e9cb598d5 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -1,7 +1,5 @@ use crate::controllers::prelude::*; -use crate::models::user; -use crate::models::user::UserNoEmailType; use crate::models::{OwnerKind, User}; use crate::schema::{crate_owners, crates, users}; use crate::views::EncodablePublicUser; @@ -13,17 +11,16 @@ pub fn show(req: &mut dyn Request) -> AppResult { let name = &req.params()["user_id"].to_lowercase(); let conn = req.db_conn()?; let user = users - .select(user::ALL_COLUMNS) .filter(crate::lower(gh_login).eq(name)) .order(id.desc()) - .first::(&*conn)?; + .first::(&*conn)?; #[derive(Serialize)] struct R { user: EncodablePublicUser, } Ok(req.json(&R { - user: User::from(user).encodable_public(), + user: user.encodable_public(), })) } diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 81f7e1f7371..be0dc8a86ac 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -4,8 +4,6 @@ use crate::github; use conduit_cookie::RequestSession; use oauth2::{prelude::*, AuthorizationCode, TokenResponse}; -use crate::models::user; -use crate::models::user::UserNoEmailType; use crate::models::{NewUser, User}; use crate::schema::users; use crate::util::errors::{AppError, ReadOnlyMode}; @@ -131,11 +129,10 @@ impl GithubUser { // just look for an existing user if e.is::() { users::table - .select(user::ALL_COLUMNS) .filter(users::gh_id.eq(self.id)) - .first::(conn) - .map(User::from) - .map_err(|_| e) + .first(conn) + .optional()? + .ok_or(e) } else { Err(e) } diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index c40709da68a..9246a9b8a1f 100644 --- a/src/controllers/version/deprecated.rs +++ b/src/controllers/version/deprecated.rs @@ -7,9 +7,7 @@ use crate::controllers::prelude::*; -use crate::models::user; -use crate::models::user::UserNoEmailType; -use crate::models::{Crate, Version}; +use crate::models::{Crate, User, Version}; use crate::schema::*; use crate::views::EncodableVersion; @@ -30,14 +28,12 @@ pub fn index(req: &mut dyn Request) -> AppResult { .select(( versions::all_columns, crates::name, - user::ALL_COLUMNS.nullable(), + users::all_columns.nullable(), )) .filter(versions::id.eq(any(ids))) - .load::<(Version, String, Option)>(&*conn)? + .load::<(Version, String, Option)>(&*conn)? .into_iter() - .map(|(version, crate_name, published_by)| { - version.encodable(&crate_name, published_by.map(From::from)) - }) + .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by)) .collect(); #[derive(Serialize)] @@ -54,14 +50,14 @@ pub fn show_by_id(req: &mut dyn Request) -> AppResult { let id = &req.params()["version_id"]; let id = id.parse().unwrap_or(0); let conn = req.db_conn()?; - let (version, krate, published_by): (Version, Crate, Option) = versions::table + let (version, krate, published_by): (Version, Crate, Option) = versions::table .find(id) .inner_join(crates::table) .left_outer_join(users::table) .select(( versions::all_columns, crate::models::krate::ALL_COLUMNS, - user::ALL_COLUMNS.nullable(), + users::all_columns.nullable(), )) .first(&*conn)?; @@ -70,6 +66,6 @@ pub fn show_by_id(req: &mut dyn Request) -> AppResult { version: EncodableVersion, } Ok(req.json(&R { - version: version.encodable(&krate.name, published_by.map(From::from)), + version: version.encodable(&krate.name, published_by), })) } diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index 4fb42a3fb23..1bdd6f14366 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -6,7 +6,6 @@ use diesel::prelude::*; use crate::db::RequestTransaction; use crate::util::errors::{std_error, AppResult, ChainError, Unauthorized}; -use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; use crate::models::User; use crate::schema::users; @@ -32,13 +31,9 @@ impl Middleware for CurrentUser { if let Some(id) = id { // If it did, look for a user in the database with the given `user_id` - let maybe_user = users::table - .select(ALL_COLUMNS) - .find(id) - .first::(&*conn); + let maybe_user = users::table.find(id).first::(&*conn); drop(conn); if let Ok(user) = maybe_user { - let user = User::from(user); // Attach the `User` model from the database to the request req.mut_extensions().insert(user); req.mut_extensions() diff --git a/src/models/krate.rs b/src/models/krate.rs index f290e4a03f6..a6839b19e92 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -8,8 +8,6 @@ use url::Url; use crate::app::App; use crate::email; -use crate::models::user; -use crate::models::user::UserNoEmailType; use crate::util::{cargo_err, AppResult}; use crate::models::{ @@ -402,10 +400,9 @@ impl Crate { let users = CrateOwner::by_owner_kind(OwnerKind::User) .filter(crate_owners::crate_id.eq(self.id)) .inner_join(users::table) - .select(user::ALL_COLUMNS) - .load::(conn)? + .select(users::all_columns) + .load(conn)? .into_iter() - .map(User::from) .map(Owner::User); let teams = CrateOwner::by_owner_kind(OwnerKind::Team) .filter(crate_owners::crate_id.eq(self.id)) diff --git a/src/models/owner.rs b/src/models/owner.rs index c3847c91884..2d6a2ac3412 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -5,7 +5,6 @@ use crate::app::App; use crate::github; use crate::util::{cargo_err, AppResult}; -use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; use crate::models::{Crate, Team, User}; use crate::schema::{crate_owners, users}; use crate::views::EncodableOwner; @@ -72,10 +71,8 @@ impl Owner { )?)) } else { users::table - .select(ALL_COLUMNS) .filter(users::gh_login.eq(name)) - .first::(conn) - .map(User::from) + .first(conn) .map(Owner::User) .map_err(|_| cargo_err(&format_args!("could not find user with login `{}`", name))) } diff --git a/src/models/user.rs b/src/models/user.rs index 0d17853fd88..2bfbe86f24f 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -31,41 +31,6 @@ pub struct NewUser<'a> { pub gh_access_token: Cow<'a, str>, } -pub type UserNoEmailType = ( - // pub id: - i32, - // pub email: - // Option, - // pub gh_access_token: - String, - // pub gh_login: - String, - // pub name: - Option, - // pub gh_avatar: - Option, - // pub gh_id: - i32, -); - -pub type AllColumns = ( - users::id, - users::gh_access_token, - users::gh_login, - users::name, - users::gh_avatar, - users::gh_id, -); - -pub const ALL_COLUMNS: AllColumns = ( - users::id, - users::gh_access_token, - users::gh_login, - users::name, - users::gh_avatar, - users::gh_id, -); - impl<'a> NewUser<'a> { pub fn new( gh_id: i32, @@ -114,13 +79,12 @@ impl<'a> NewUser<'a> { gh_avatar.eq(excluded(gh_avatar)), gh_access_token.eq(excluded(gh_access_token)), )) - .returning(ALL_COLUMNS) - .get_result::(conn)?; + .get_result::(conn)?; // To send the user an account verification email if let Some(user_email) = email { let new_email = NewEmail { - user_id: user.0, + user_id: user.id, email: user_email, }; @@ -132,11 +96,11 @@ impl<'a> NewUser<'a> { .optional()?; if let Some(token) = token { - crate::email::send_user_confirm_email(user_email, &user.2, &token); + crate::email::send_user_confirm_email(user_email, &user.gh_login, &token); } } - Ok(User::from(user)) + Ok(user) }) } } @@ -162,21 +126,16 @@ impl User { }) .or_else(|_| tokens.select(user_id).first(conn))?; - users::table - .select(ALL_COLUMNS) - .find(user_id_) - .first::(conn) - .map(User::from) + users::table.find(user_id_).first(conn) } pub fn owning(krate: &Crate, conn: &PgConnection) -> AppResult> { let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) - .select(ALL_COLUMNS) + .select(users::all_columns) .filter(crate_owners::crate_id.eq(krate.id)) - .load::(conn)? + .load(conn)? .into_iter() - .map(User::from) .map(Owner::User); Ok(users.collect()) @@ -274,16 +233,3 @@ impl User { } } } - -impl From for User { - fn from(user: UserNoEmailType) -> Self { - User { - id: user.0, - gh_access_token: user.1, - gh_login: user.2, - name: user.3, - gh_avatar: user.4, - gh_id: user.5, - } - } -} diff --git a/src/models/version.rs b/src/models/version.rs index e685967e1a9..7ec7746c050 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -5,8 +5,6 @@ use diesel::prelude::*; use crate::util::{cargo_err, AppResult}; -use crate::models::user; -use crate::models::user::UserNoEmailType; use crate::models::{Crate, Dependency, User}; use crate::schema::*; use crate::views::{EncodableVersion, EncodableVersionLinks}; @@ -117,12 +115,7 @@ impl Version { /// Not for use when you have a group of versions you need the publishers for. pub fn published_by(&self, conn: &PgConnection) -> Option { match self.published_by { - Some(pb) => users::table - .find(pb) - .select(user::ALL_COLUMNS) - .first::(conn) - .map(User::from) - .ok(), + Some(pb) => users::table.find(pb).first(conn).ok(), None => None, } }