Applied all the changes necessary for pre-migration

hbina · hbina · commit ea51376af883 · 2019-11-21T17:56:01.000-05:00
Every queries out of `User`'stable should now be converted into an
intermediate type called `UserNoEmailType` (could be renamed, I
guess). Conversion from this type to `User` is simply a copy except
that `email` returned will always be `None`.

I think its possible to remove `email` from User directly because
its not actually the mirror of `User`'s table (the mirror struct is actually
`NewUser`
diff --git a/src/bin/transfer-crates.rs b/src/bin/transfer-crates.rs
@@ -7,7 +7,7 @@
 
 use cargo_registry::{
     db,
-    models::{Crate, OwnerKind, User},
+    models::{user, Crate, OwnerKind, User},
     schema::{crate_owners, crates, users},
 };
 use std::{
@@ -17,14 +17,15 @@ use std::{
 };
 
 use diesel::prelude::*;
+use cargo_registry::models::user::UserNoEmailType;
 
 fn main() {
     let conn = db::connect_now().unwrap();
     conn.transaction::<_, diesel::result::Error, _>(|| {
         transfer(&conn);
         Ok(())
     })
-    .unwrap()
+        .unwrap()
 }
 
 fn transfer(conn: &PgConnection) {
@@ -44,12 +45,16 @@ fn transfer(conn: &PgConnection) {
     };
 
     let from = users::table
+        .select(user::ALL_COLUMNS)
         .filter(users::gh_login.eq(from))
-        .first::<User>(conn)
+        .first::<UserNoEmailType>(conn)
+        .map(User::from)
         .unwrap();
     let to = users::table
+        .select(user::ALL_COLUMNS)
         .filter(users::gh_login.eq(to))
-        .first::<User>(conn)
+        .first::<UserNoEmailType>(conn)
+        .map(User::from)
         .unwrap();
 
     if from.gh_id != to.gh_id {
diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs
@@ -8,8 +8,7 @@ use crate::controllers::prelude::*;
 use crate::models::user;
 use crate::models::user::UserNoEmailType;
 use crate::models::{
-    Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads,
-    User, Version,
+    Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads, Version,
 };
 use crate::schema::*;
 use crate::views::{
@@ -107,10 +106,10 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
     let conn = req.db_conn()?;
     let krate = Crate::by_name(name).first::<Crate>(&*conn)?;
 
-    let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
+    let mut versions_and_publishers: Vec<(Version, Option<UserNoEmailType>)> = krate
         .all_versions()
         .left_outer_join(users::table)
-        .select((versions::all_columns, users::all_columns.nullable()))
+        .select((versions::all_columns, user::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 +152,7 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
         ),
         versions: versions_and_publishers
             .into_iter()
-            .map(|(v, pb)| v.encodable(&krate.name, pb))
+            .map(|(v, pb)| v.encodable(&krate.name, pb.map(From::from)))
             .collect(),
         keywords: kws.into_iter().map(Keyword::encodable).collect(),
         categories: cats.into_iter().map(Category::encodable).collect(),
@@ -189,15 +188,15 @@ pub fn versions(req: &mut dyn Request) -> CargoResult<Response> {
     let crate_name = &req.params()["crate_id"];
     let conn = req.db_conn()?;
     let krate = Crate::by_name(crate_name).first::<Crate>(&*conn)?;
-    let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
+    let mut versions_and_publishers: Vec<(Version, Option<UserNoEmailType>)> = krate
         .all_versions()
         .left_outer_join(users::table)
-        .select((versions::all_columns, users::all_columns.nullable()))
+        .select((versions::all_columns, user::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(|(v, pb)| v.encodable(crate_name, pb.map(From::from)))
         .collect();
 
     #[derive(Serialize)]
@@ -236,22 +235,7 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult<Response> {
         ))
         .load::<(Version, String, Option<UserNoEmailType>)>(&*conn)?
         .into_iter()
-        .map(|(version, krate_name, user)| {
-            if let Some(user) = user {
-                let published_by = User {
-                    id: user.0,
-                    email: None,
-                    gh_access_token: user.1,
-                    gh_login: user.2,
-                    name: user.3,
-                    gh_avatar: user.4,
-                    gh_id: user.5,
-                };
-                version.encodable(&krate_name, Some(published_by))
-            } else {
-                version.encodable(&krate_name, None)
-            }
-        })
+        .map(|(version, krate_name, user)| version.encodable(&krate_name, user.map(From::from)))
         .collect();
 
     #[derive(Serialize)]
diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs
@@ -43,18 +43,9 @@ pub fn me(req: &mut dyn Request) -> CargoResult<Response> {
     //          Will modify this so that we don't need this kind of conversion anymore...
     //          In fact, the PR will use the email that we obtained from the above SQL queries
     //          and pass it along the encodable_private function below.
-    let user = User {
-        id: user.0,
-        email,
-        gh_access_token: user.1,
-        gh_login: user.2,
-        name: user.3,
-        gh_avatar: user.4,
-        gh_id: user.5,
-    };
 
     Ok(req.json(&EncodableMe {
-        user: user.encodable_private(verified, verification_sent),
+        user: User::from(user).encodable_private(email, verified, verification_sent),
     }))
 }
 
@@ -80,22 +71,8 @@ pub fn updates(req: &mut dyn Request) -> CargoResult<Response> {
     let versions = data
         .into_iter()
         .map(|(version, crate_name, published_by)| {
-            if let Some(user) = published_by {
-                let published_by = User {
-                    id: user.0,
-                    email: None,
-                    gh_access_token: user.1,
-                    gh_login: user.2,
-                    name: user.3,
-                    gh_avatar: user.4,
-                    gh_id: user.5,
-                };
-                version.encodable(&crate_name, Some(published_by))
-            } else {
-                version.encodable(&crate_name, None)
-            }
+            version.encodable(&crate_name, published_by.map(From::from))
         })
-        // version.encodable(&crate_name, published_by))
         .collect();
 
     #[derive(Serialize)]
diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs
@@ -3,6 +3,8 @@ use crate::controllers::prelude::*;
 use crate::models::{OwnerKind, User};
 use crate::schema::{crate_owners, crates, users};
 use crate::views::EncodablePublicUser;
+use crate::models::user::UserNoEmailType;
+use crate::models::user;
 
 /// Handles the `GET /users/:user_id` route.
 pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
@@ -11,16 +13,17 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
     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::<User>(&*conn)?;
+        .first::<UserNoEmailType>(&*conn)?;
 
     #[derive(Serialize)]
     struct R {
         user: EncodablePublicUser,
     }
     Ok(req.json(&R {
-        user: user.encodable_public(),
+        user: User::from(user).encodable_public(),
     }))
 }
 
diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs
@@ -4,9 +4,11 @@ use crate::github;
 use conduit_cookie::RequestSession;
 use oauth2::{prelude::*, AuthorizationCode, TokenResponse};
 
+use crate::models::user;
 use crate::models::{NewUser, User};
 use crate::schema::users;
 use crate::util::errors::{CargoError, ReadOnlyMode};
+use crate::models::user::UserNoEmailType;
 
 /// Handles the `GET /authorize_url` route.
 ///
@@ -123,21 +125,22 @@ impl GithubUser {
             self.avatar_url.as_ref().map(|s| &s[..]),
             access_token,
         )
-        .create_or_update(conn)
-        .map_err(Into::into)
-        .or_else(|e: Box<dyn CargoError>| {
-            // If we're in read only mode, we can't update their details
-            // just look for an existing user
-            if e.is::<ReadOnlyMode>() {
-                users::table
-                    .filter(users::gh_id.eq(self.id))
-                    .first(conn)
-                    .optional()?
-                    .ok_or(e)
-            } else {
-                Err(e)
-            }
-        })
+            .create_or_update(conn)
+            .map_err(Into::into)
+            .or_else(|e: Box<dyn CargoError>| {
+                // If we're in read only mode, we can't update their details
+                // just look for an existing user
+                if e.is::<ReadOnlyMode>() {
+                    users::table
+                        .select(user::ALL_COLUMNS)
+                        .filter(users::gh_id.eq(self.id))
+                        .first::<UserNoEmailType>(conn)
+                        .map(User::from)
+                        .map_err(|_| e)
+                } else {
+                    Err(e)
+                }
+            })
     }
 }
 
diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs
@@ -7,7 +7,9 @@
 
 use crate::controllers::prelude::*;
 
-use crate::models::{Crate, User, Version};
+use crate::models::user;
+use crate::models::user::UserNoEmailType;
+use crate::models::{Crate, Version};
 use crate::schema::*;
 use crate::views::EncodableVersion;
 
@@ -28,12 +30,12 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
         .select((
             versions::all_columns,
             crates::name,
-            users::all_columns.nullable(),
+            user::ALL_COLUMNS.nullable(),
         ))
         .filter(versions::id.eq(any(ids)))
-        .load::<(Version, String, Option<User>)>(&*conn)?
+        .load::<(Version, String, Option<UserNoEmailType>)>(&*conn)?
         .into_iter()
-        .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by))
+        .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by.map(From::from)))
         .collect();
 
     #[derive(Serialize)]
@@ -50,14 +52,14 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult<Response> {
     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<User>) = versions::table
+    let (version, krate, published_by): (Version, Crate, Option<UserNoEmailType>) = versions::table
         .find(id)
         .inner_join(crates::table)
         .left_outer_join(users::table)
         .select((
             versions::all_columns,
             crate::models::krate::ALL_COLUMNS,
-            users::all_columns.nullable(),
+            user::ALL_COLUMNS.nullable(),
         ))
         .first(&*conn)?;
 
@@ -66,6 +68,6 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult<Response> {
         version: EncodableVersion,
     }
     Ok(req.json(&R {
-        version: version.encodable(&krate.name, published_by),
+        version: version.encodable(&krate.name, published_by.map(From::from)),
     }))
 }
diff --git a/src/models/krate.rs b/src/models/krate.rs
@@ -409,16 +409,7 @@ impl Crate {
             .filter(crate_owners::owner_kind.eq(OwnerKind::User as i32))
             .load::<UserNoEmailType>(conn)?
             .into_iter()
-            .map(|user| User {
-                id: user.0,
-                email: None, // <-- Because user _should not_ contain the email column.
-                // This hack should be fixed in the future.
-                gh_access_token: user.1,
-                gh_login: user.2,
-                name: user.3,
-                gh_avatar: user.4,
-                gh_id: user.5,
-            })
+            .map(User::from)
             .map(Owner::User);
         let teams = base_query
             .inner_join(teams::table)
diff --git a/src/models/owner.rs b/src/models/owner.rs
@@ -58,15 +58,7 @@ impl Owner {
                 .select(ALL_COLUMNS)
                 .filter(users::gh_login.eq(name))
                 .first::<UserNoEmailType>(conn)
-                .map(|user| User {
-                    id: user.0,
-                    email: None,
-                    gh_access_token: user.1,
-                    gh_login: user.2,
-                    name: user.3,
-                    gh_avatar: user.4,
-                    gh_id: user.5,
-                })
+                .map(User::from)
                 .map(Owner::User)
                 .map_err(|_| human(&format_args!("could not find user with login `{}`", name)))
         }
diff --git a/src/models/user.rs b/src/models/user.rs
diff --git a/src/models/version.rs b/src/models/version.rs
