Drop Email column (merge and deploy the fix for #1888 first!)

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
-        );
+        );
+

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();
+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();

migrations/2019-11-11-162609_drop_email_from_user/down.sql

@@ -0,0 +1 @@
+-- Not reversible

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;

src/controllers/user/me.rs

@@ -38,10 +38,10 @@ pub fn me(req: &mut dyn Request) -> CargoResult<Response> {
 
     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<Response> {
 /// Handles the `PUT /user/:user_id` route.
 pub fn update_user(req: &mut dyn Request) -> CargoResult<Response> {
     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<Response> {
     }
 
     conn.transaction::<_, Box<dyn CargoError>, _>(|| {
-        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,

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<dyn CargoError>| {
             // If we're in read only mode, we can't update their details

src/models/user.rs

@@ -13,20 +13,19 @@ use crate::views::{EncodablePrivateUser, EncodablePublicUser};
 #[derive(Clone, Debug, PartialEq, Eq, Queryable, Identifiable, AsChangeset, Associations)]
 pub struct User {
     pub id: i32,
-    pub email: Option<String>,
     pub gh_access_token: String,
     pub gh_login: String,
     pub name: Option<String>,
     pub gh_avatar: Option<String>,
     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,23 +35,26 @@ 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,
     ) -> Self {
         NewUser {
             gh_id,
             gh_login,
-            email,
             name,
             gh_avatar,
             gh_access_token: Cow::Borrowed(gh_access_token),
         }
     }
 
     /// Inserts the user into the database, or updates an existing one.
-    pub fn create_or_update(&self, conn: &PgConnection) -> QueryResult<User> {
+    pub fn create_or_update(
+        &self,
+        email: Option<&'a str>,
+        conn: &PgConnection,
+    ) -> QueryResult<User> {
+        use crate::schema::emails::columns::user_id;
         use crate::schema::users::dsl::*;
         use diesel::dsl::sql;
         use diesel::insert_into;
@@ -81,15 +83,17 @@ impl<'a> NewUser<'a> {
                 .get_result::<User>(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,
                 };
 
                 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::<String>(conn)
                     .optional()?;
@@ -168,6 +172,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<Option<String>> {
         Ok(Email::belonging_to(self)
             .select(emails::email)
@@ -179,19 +185,21 @@ impl User {
     /// Converts this `User` model into an `EncodablePrivateUser` for JSON serialization.
     pub fn encodable_private(
         self,
+        email: Option<String>,
+
         email_verified: bool,
         email_verification_sent: bool,
-    ) -> EncodablePrivateUser {
+    ) -> CargoResult<EncodablePrivateUser> {
         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 +208,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<Option<String>> {
+        Ok(Email::belonging_to(self)
+            .select(emails::email)
+            .first::<String>(&*conn)
+            .optional()?)
     }
 
     /// Converts this`User` model into an `EncodablePublicUser` for JSON serialization.

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)
     }
 

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<Varchar>`.
-        ///
-        /// (Automatically generated by Diesel.)
-        email -> Nullable<Varchar>,
         /// The `gh_access_token` column of the `users` table.
         ///
         /// Its SQL type is `Varchar`.

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"

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()
     }
 

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"),

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)
     });
 

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]

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("[email protected]"),
             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("[email protected]"),
             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::<String>(&*conn)
+            .unwrap()
+    });
 
     let new_github_email = "[email protected]";
 
@@ -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("[email protected]"),
             ..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("[email protected]"),
             ..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.

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 = "[email protected]";
-            user.email = Some(email.to_string());
+            let email = "[email protected]";
+            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),