Don't select the versions::published_by_email column ever

A workaround is needed to get the all columns pattern to work with the
boxed query pattern, see diesel-rs/diesel#1979

I don't like how many places needed to be changed, going to see if I can
refactor a bit.

Author: carols10cents

src/bin/delete-version.rs

@@ -47,6 +47,7 @@ fn delete(conn: &PgConnection) {
     let krate = Crate::by_name(&name).first::<Crate>(conn).unwrap();
     let v = Version::belonging_to(&krate)
         .filter(versions::num.eq(&version))
+        .select(cargo_registry::models::version::ALL_COLUMNS)
         .first::<Version>(conn)
         .unwrap();
     print!(

src/bin/render-readmes.rs

@@ -112,7 +112,7 @@ fn main() {
         let versions = versions::table
             .inner_join(crates::table)
             .filter(versions::id.eq(any(ids)))
-            .select((versions::all_columns, crates::name))
+            .select((cargo_registry::models::version::ALL_COLUMNS, crates::name))
             .load::<(Version, String)>(&conn)
             .expect("error loading versions");
 

src/bin/update-downloads.rs

@@ -288,6 +288,7 @@ mod test {
 
         let version_before = versions::table
             .find(version.id)
+            .select(cargo_registry::models::version::ALL_COLUMNS)
             .first::<Version>(&conn)
             .unwrap();
         let krate_before = Crate::all()
@@ -297,6 +298,7 @@ mod test {
         crate::update(&conn).unwrap();
         let version2 = versions::table
             .find(version.id)
+            .select(cargo_registry::models::version::ALL_COLUMNS)
             .first::<Version>(&conn)
             .unwrap();
         assert_eq!(version2.downloads, 2);
@@ -311,6 +313,7 @@ mod test {
         crate::update(&conn).unwrap();
         let version3 = versions::table
             .find(version.id)
+            .select(cargo_registry::models::version::ALL_COLUMNS)
             .first::<Version>(&conn)
             .unwrap();
         assert_eq!(version3.downloads, 2);

src/controllers/krate/metadata.rs

@@ -110,7 +110,10 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
     let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
         .all_versions()
         .left_outer_join(users::table)
-        .select((versions::all_columns, users::all_columns.nullable()))
+        .select((
+            crate::models::version::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();
@@ -193,7 +196,10 @@ pub fn versions(req: &mut dyn Request) -> CargoResult<Response> {
     let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
         .all_versions()
         .left_outer_join(users::table)
-        .select((versions::all_columns, users::all_columns.nullable()))
+        .select((
+            crate::models::version::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
@@ -229,7 +235,7 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult<Response> {
         .inner_join(crates::table)
         .left_outer_join(users::table)
         .select((
-            versions::all_columns,
+            crate::models::version::ALL_COLUMNS,
             crates::name,
             users::all_columns.nullable(),
         ))

src/controllers/user/me.rs

@@ -59,7 +59,7 @@ pub fn updates(req: &mut dyn Request) -> CargoResult<Response> {
         .filter(crates::id.eq(any(followed_crates)))
         .order(versions::created_at.desc())
         .select((
-            versions::all_columns,
+            crate::models::version::ALL_COLUMNS,
             crates::name,
             users::all_columns.nullable(),
         ))

src/controllers/version/deprecated.rs

@@ -26,7 +26,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
         .inner_join(crates::table)
         .left_outer_join(users::table)
         .select((
-            versions::all_columns,
+            crate::models::version::ALL_COLUMNS,
             crates::name,
             users::all_columns.nullable(),
         ))
@@ -55,7 +55,7 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult<Response> {
         .inner_join(crates::table)
         .left_outer_join(users::table)
         .select((
-            versions::all_columns,
+            crate::models::version::ALL_COLUMNS,
             crate::models::krate::ALL_COLUMNS,
             users::all_columns.nullable(),
         ))

src/models/krate.rs

@@ -1,7 +1,9 @@
 use chrono::{NaiveDate, NaiveDateTime};
 use diesel::associations::Identifiable;
+use diesel::dsl::SqlTypeOf;
 use diesel::pg::Pg;
 use diesel::prelude::*;
+use diesel::query_builder::BoxedSelectStatement;
 use url::Url;
 
 use crate::app::App;
@@ -559,28 +561,42 @@ mod tests {
     }
 }
 
+// This type definition is a workaround to make the BoxedQuery `all_version` methods on Crate work
+// with the version `AllColumns` type definition; see this diesel issue:
+// https://github.com/diesel-rs/diesel/issues/1979
+pub type BoxedVersionQuery<'a, Backend> = BoxedSelectStatement<
+    'a,
+    SqlTypeOf<crate::models::version::AllColumns>,
+    versions::table,
+    Backend,
+>;
+
 pub trait CrateVersions {
-    fn versions(&self) -> versions::BoxedQuery<'_, Pg> {
+    fn versions(&self) -> BoxedVersionQuery<'_, Pg> {
         self.all_versions().filter(versions::yanked.eq(false))
     }
 
-    fn all_versions(&self) -> versions::BoxedQuery<'_, Pg>;
+    fn all_versions(&self) -> BoxedVersionQuery<'_, Pg>;
 }
 
 impl CrateVersions for Crate {
-    fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
-        Version::belonging_to(self).into_boxed()
+    fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
+        Version::belonging_to(self)
+            .select(crate::models::version::ALL_COLUMNS)
+            .into_boxed()
     }
 }
 
 impl CrateVersions for Vec<Crate> {
-    fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
+    fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
         self.as_slice().all_versions()
     }
 }
 
 impl CrateVersions for [Crate] {
-    fn all_versions(&self) -> versions::BoxedQuery<'_, Pg> {
-        Version::belonging_to(self).into_boxed()
+    fn all_versions(&self) -> BoxedVersionQuery<'_, Pg> {
+        Version::belonging_to(self)
+            .select(crate::models::version::ALL_COLUMNS)
+            .into_boxed()
     }
 }

src/models/mod.rs

@@ -30,4 +30,4 @@ mod rights;
 mod team;
 mod token;
 mod user;
-mod version;
+pub mod version;

src/models/version.rs

@@ -26,6 +26,41 @@ pub struct Version {
     pub published_by: Option<i32>,
 }
 
+/// The database stores the verified email address of the person who published the version at the
+/// time the version was published, so that even if someone removes the verified email address from
+/// their account, we still have a way to try to contact them in case of a DMCA complaint. However,
+/// this information is private, only for crates.io administrator access, and should never be
+/// returned from any API calls. This type excludes the `published_by_email` column.
+pub type AllColumns = (
+    versions::id,
+    versions::crate_id,
+    versions::num,
+    versions::updated_at,
+    versions::created_at,
+    versions::downloads,
+    versions::features,
+    versions::yanked,
+    versions::license,
+    versions::crate_size,
+    versions::published_by,
+);
+
+pub const ALL_COLUMNS: AllColumns = (
+    versions::id,
+    versions::crate_id,
+    versions::num,
+    versions::updated_at,
+    versions::created_at,
+    versions::downloads,
+    versions::features,
+    versions::yanked,
+    versions::license,
+    versions::crate_size,
+    versions::published_by,
+);
+
+type All = diesel::dsl::Select<versions::table, AllColumns>;
+
 #[derive(Insertable, Debug)]
 #[table_name = "versions"]
 pub struct NewVersion {
@@ -38,6 +73,10 @@ pub struct NewVersion {
 }
 
 impl Version {
+    pub fn all() -> All {
+        versions::table.select(ALL_COLUMNS)
+    }
+
     pub fn encodable(self, crate_name: &str, published_by: Option<User>) -> EncodableVersion {
         let Version {
             id,
@@ -114,10 +153,8 @@ impl Version {
     /// Gets the User who ran `cargo publish` for this version, if recorded.
     /// Not for use when you have a group of versions you need the publishers for.
     pub fn published_by(&self, conn: &PgConnection) -> Option<User> {
-        match self.published_by {
-            Some(pb) => users::table.find(pb).first(conn).ok(),
-            None => None,
-        }
+        self.published_by
+            .and_then(|pb| users::table.find(pb).first(conn).ok())
     }
 }
 
@@ -168,6 +205,7 @@ impl NewVersion {
 
             let version = insert_into(versions)
                 .values(self)
+                .returning(ALL_COLUMNS)
                 .get_result::<Version>(conn)?;
 
             let new_authors = authors

src/tests/builders.rs

@@ -95,6 +95,7 @@ impl<'a> VersionBuilder<'a> {
         if self.yanked {
             vers = update(&vers)
                 .set(versions::yanked.eq(true))
+                .returning(cargo_registry::models::version::ALL_COLUMNS)
                 .get_result(connection)?;
         }