From 2edc0dd21f326a75d407749eb4fa8b817e20f086 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 25 Nov 2019 13:05:15 -0500 Subject: [PATCH 1/2] Revert "Auto merge of #1926 - integer32llc:fix-master, r=carols10cents" This reverts commit e08605e3b8e3a4efef99be75fb700836d545657f, reversing changes made to 92edac3d0843509a1acb1391c91c622455c5c633. --- src/bin/transfer-crates.rs | 11 ++-- src/controllers/krate/metadata.rs | 22 ++++---- src/controllers/user/me.rs | 24 +++++---- src/controllers/user/other.rs | 7 ++- src/controllers/user/session.rs | 9 ++-- src/controllers/version/deprecated.rs | 18 ++++--- src/middleware/current_user.rs | 6 ++- src/models.rs | 2 +- src/models/krate.rs | 7 ++- src/models/owner.rs | 5 +- src/models/user.rs | 73 +++++++++++++++++++++++---- src/models/version.rs | 9 +++- 12 files changed, 142 insertions(+), 51 deletions(-) diff --git a/src/bin/transfer-crates.rs b/src/bin/transfer-crates.rs index 76fd187c03c..527937f9feb 100644 --- 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::{ @@ -16,6 +16,7 @@ use std::{ process::exit, }; +use cargo_registry::models::user::UserNoEmailType; use diesel::prelude::*; fn main() { @@ -44,12 +45,16 @@ fn transfer(conn: &PgConnection) { }; let from = users::table + .select(user::ALL_COLUMNS) .filter(users::gh_login.eq(from)) - .first::(conn) + .first::(conn) + .map(User::from) .unwrap(); let to = users::table + .select(user::ALL_COLUMNS) .filter(users::gh_login.eq(to)) - .first::(conn) + .first::(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 index d53741c3c7a..3a9aa90e298 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -5,9 +5,11 @@ //! `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, - User, Version, + Version, }; use crate::schema::*; use crate::views::{ @@ -105,10 +107,10 @@ pub fn show(req: &mut dyn Request) -> CargoResult { 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, 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(); @@ -151,7 +153,7 @@ pub fn show(req: &mut dyn Request) -> CargoResult { ), 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(), @@ -187,15 +189,15 @@ pub fn versions(req: &mut dyn Request) -> CargoResult { 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, 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)] @@ -227,11 +229,11 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult { .select(( versions::all_columns, crates::name, - users::all_columns.nullable(), + user::ALL_COLUMNS.nullable(), )) - .load::<(Version, String, Option)>(&*conn)? + .load::<(Version, String, Option)>(&*conn)? .into_iter() - .map(|(version, krate_name, published_by)| version.encodable(&krate_name, published_by)) + .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 index d7b36c2d60f..f8f6241920c 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -7,6 +7,7 @@ use crate::email; use crate::util::bad_request; use crate::util::errors::CargoError; +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}; @@ -31,12 +32,12 @@ pub fn me(req: &mut dyn Request) -> CargoResult { .find(user_id) .left_join(emails::table) .select(( - users::all_columns, + ALL_COLUMNS, emails::verified.nullable(), emails::email.nullable(), emails::token_generated_at.nullable().is_not_null(), )) - .first::<(User, Option, Option, bool)>(&*conn)?; + .first::<(UserNoEmailType, Option, Option, bool)>(&*conn)?; let owned_crates = crate_owners::table .inner_join(crates::table) @@ -55,10 +56,13 @@ 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 }; + // PR :: https://github.com/rust-lang/crates.io/pull/1891 + // 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. Ok(req.json(&EncodableMe { - user: user.encodable_private(verified, verification_sent), + user: User::from(user).encodable_private(email, verified, verification_sent), owned_crates, })) } @@ -76,19 +80,17 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { .left_outer_join(users::table) .filter(crates::id.eq(any(followed_crates))) .order(versions::created_at.desc()) - .select(( - versions::all_columns, - crates::name, - users::all_columns.nullable(), - )) + .select((versions::all_columns, crates::name, 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(|(version, crate_name, published_by)| { + version.encodable(&crate_name, published_by.map(From::from)) + }) .collect(); #[derive(Serialize)] diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index cbdf4180010..9ec66bb9b92 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -1,5 +1,7 @@ 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; @@ -11,16 +13,17 @@ pub fn show(req: &mut dyn Request) -> CargoResult { 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.encodable_public(), + user: User::from(user).encodable_public(), })) } diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index fdbdd5f099d..33c03febcbd 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -4,6 +4,8 @@ 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::{CargoError, ReadOnlyMode}; @@ -130,10 +132,11 @@ 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) - .optional()? - .ok_or(e) + .first::(conn) + .map(User::from) + .map_err(|_| e) } else { Err(e) } diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 346b089636c..71739034dd7 100644 --- 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,14 @@ pub fn index(req: &mut dyn Request) -> CargoResult { .select(( versions::all_columns, crates::name, - users::all_columns.nullable(), + user::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(|(version, crate_name, published_by)| { + version.encodable(&crate_name, published_by.map(From::from)) + }) .collect(); #[derive(Serialize)] @@ -50,14 +54,14 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult { 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, - users::all_columns.nullable(), + user::ALL_COLUMNS.nullable(), )) .first(&*conn)?; @@ -66,6 +70,6 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult { 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/middleware/current_user.rs b/src/middleware/current_user.rs index ad760c88857..0bd08e1ec56 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -6,6 +6,7 @@ use diesel::prelude::*; use crate::db::RequestTransaction; use crate::util::errors::{std_error, CargoResult, ChainError, Unauthorized}; +use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; use crate::models::User; use crate::schema::users; @@ -31,7 +32,10 @@ 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.find(id).first::(&*conn); + let maybe_user = users::table + .select(ALL_COLUMNS) + .find(id) + .first::(&*conn); drop(conn); if let Ok(user) = maybe_user { // Attach the `User` model from the database to the request diff --git a/src/models.rs b/src/models.rs index 806d7c91dc9..f11eb84e9d4 100644 --- a/src/models.rs +++ b/src/models.rs @@ -31,5 +31,5 @@ mod owner; mod rights; mod team; mod token; -mod user; +pub mod user; mod version; diff --git a/src/models/krate.rs b/src/models/krate.rs index e19cc219735..969de414a98 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -8,6 +8,8 @@ use url::Url; use crate::app::App; use crate::email; +use crate::models::user; +use crate::models::user::UserNoEmailType; use crate::util::{human, CargoResult}; use crate::models::{ @@ -399,9 +401,10 @@ impl Crate { let users = CrateOwner::by_owner_kind(OwnerKind::User) .filter(crate_owners::crate_id.eq(self.id)) .inner_join(users::table) - .select(users::all_columns) - .load(conn)? + .select(user::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 a1f83586a28..88d8c7afb92 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -5,6 +5,7 @@ use crate::app::App; use crate::github; use crate::util::{human, CargoResult}; +use crate::models::user::{UserNoEmailType, ALL_COLUMNS}; use crate::models::{Crate, Team, User}; use crate::schema::{crate_owners, users}; use crate::views::EncodableOwner; @@ -71,8 +72,10 @@ impl Owner { )?)) } else { users::table + .select(ALL_COLUMNS) .filter(users::gh_login.eq(name)) - .first(conn) + .first::(conn) + .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 index 8fb9c59cd57..5ff3cb987d5 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -32,6 +32,41 @@ 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, @@ -78,12 +113,13 @@ impl<'a> NewUser<'a> { gh_avatar.eq(excluded(gh_avatar)), gh_access_token.eq(excluded(gh_access_token)), )) - .get_result::(conn)?; + .returning(ALL_COLUMNS) + .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) = self.email { let new_email = NewEmail { - user_id: user.id, + user_id: user.0, email: user_email, }; @@ -95,11 +131,11 @@ impl<'a> NewUser<'a> { .optional()?; if let Some(token) = token { - crate::email::send_user_confirm_email(user_email, &user.gh_login, &token); + crate::email::send_user_confirm_email(user_email, &user.2, &token); } } - Ok(user) + Ok(User::from(user)) }) } } @@ -125,16 +161,21 @@ impl User { }) .or_else(|_| tokens.select(user_id).first(conn))?; - users::table.find(user_id_).first(conn) + users::table + .select(ALL_COLUMNS) + .find(user_id_) + .first::(conn) + .map(User::from) } pub fn owning(krate: &Crate, conn: &PgConnection) -> CargoResult> { let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) - .select(users::all_columns) + .select(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()) @@ -178,12 +219,12 @@ 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 { let User { id, - email, name, gh_login, gh_avatar, @@ -221,3 +262,17 @@ impl User { } } } + +impl From for User { + fn from(user: UserNoEmailType) -> Self { + 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, + } + } +} diff --git a/src/models/version.rs b/src/models/version.rs index 4be66b41a21..0194b55e096 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -5,6 +5,8 @@ use diesel::prelude::*; use crate::util::{human, CargoResult}; +use crate::models::user; +use crate::models::user::UserNoEmailType; use crate::models::{Crate, Dependency, User}; use crate::schema::*; use crate::views::{EncodableVersion, EncodableVersionLinks}; @@ -115,7 +117,12 @@ 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).first(conn).ok(), + Some(pb) => users::table + .find(pb) + .select(user::ALL_COLUMNS) + .first::(conn) + .map(User::from) + .ok(), None => None, } } From fcf5e963f28a5b22bcb11ca34e9fdd082637af37 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 25 Nov 2019 11:25:54 -0500 Subject: [PATCH 2/2] Missed a conversion from UserNoEmailType to User --- src/middleware/current_user.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index 0bd08e1ec56..a6c72695263 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -38,6 +38,7 @@ impl Middleware for CurrentUser { .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()