Merge #1213

1213: Minor cleanup on dependency insertion and loading r=jtgeibel

I'm going through some random code as I'm looking to point to this repo
more in Diesel's guides for some actual examples. My goal was actually
to do the following:

- `#[derive(Insertable)]` on `upload::CrateDependency` and use that
  directly
- `#[derive(Queryable)]` on `git::Dependency` and construct it from a
  returning clause

However, doing the first piece would require skipping the `crate_name`
field or pulling it out of that struct, and the second piece requires
some way to tell Diesel that a subselect returns a single row (which is
not hard to do, but is not code that should live in this repo)

Either way, while I was experimenting I did a bit of cleanup here which
I think is worthwhile on its own.

- Removed an `Insertable` struct in favor of tuple inserts, since
  `Insertable` is really only mean to be used when your input comes from
  another source (like `Deserialize`).
- Made some defaults explicit in the tests. It felt sketchy having the
  default impl that was only used in tests. I thought about pushing
  these defaults into the database, but that felt sketchy for the same
  reason
- Removed a potential panic
- Removed a few unneccessary iterations/allocations over the features
  vec.

Author: bors-voyager[bot]

src/dependency.rs

@@ -60,19 +60,6 @@ pub enum Kind {
     // if you add a kind here, be sure to update `from_row` below.
 }
 
-#[derive(Default, Insertable, Debug)]
-#[table_name = "dependencies"]
-pub struct NewDependency<'a> {
-    pub version_id: i32,
-    pub crate_id: i32,
-    pub req: String,
-    pub optional: bool,
-    pub default_features: bool,
-    pub features: Vec<&'a str>,
-    pub target: Option<&'a str>,
-    pub kind: i32,
-}
-
 impl Dependency {
     // `downloads` need only be specified when generating a reverse dependency
     pub fn encodable(self, crate_name: &str, downloads: Option<i32>) -> EncodableDependency {
@@ -101,9 +88,10 @@ impl ReverseDependency {
 pub fn add_dependencies(
     conn: &PgConnection,
     deps: &[::upload::CrateDependency],
-    version_id: i32,
+    target_version_id: i32,
 ) -> CargoResult<Vec<git::Dependency>> {
     use diesel::insert_into;
+    use self::dependencies::dsl::*;
 
     let git_and_new_dependencies = deps.iter()
         .map(|dep| {
@@ -118,42 +106,62 @@ pub fn add_dependencies(
                      information",
                 ));
             }
-            let features: Vec<_> = dep.features.iter().map(|s| &**s).collect();
 
             Ok((
                 git::Dependency {
                     name: dep.name.to_string(),
                     req: dep.version_req.to_string(),
-                    features: features.iter().map(|s| s.to_string()).collect(),
+                    features: dep.features.iter().map(|s| s.to_string()).collect(),
                     optional: dep.optional,
                     default_features: dep.default_features,
                     target: dep.target.clone(),
                     kind: dep.kind.or(Some(Kind::Normal)),
                 },
-                NewDependency {
-                    version_id: version_id,
-                    crate_id: krate.id,
-                    req: dep.version_req.to_string(),
-                    kind: dep.kind.unwrap_or(Kind::Normal) as i32,
-                    optional: dep.optional,
-                    default_features: dep.default_features,
-                    features: features,
-                    target: dep.target.as_ref().map(|s| &**s),
-                },
+                (
+                    version_id.eq(target_version_id),
+                    crate_id.eq(krate.id),
+                    req.eq(dep.version_req.to_string()),
+                    dep.kind.map(|k| kind.eq(k as i32)),
+                    optional.eq(dep.optional),
+                    default_features.eq(dep.default_features),
+                    features.eq(&dep.features),
+                    target.eq(dep.target.as_ref().map(|s| &**s)),
+                ),
             ))
         })
         .collect::<Result<Vec<_>, _>>()?;
 
     let (git_deps, new_dependencies): (Vec<_>, Vec<_>) =
         git_and_new_dependencies.into_iter().unzip();
 
-    insert_into(dependencies::table)
+    insert_into(dependencies)
         .values(&new_dependencies)
         .execute(conn)?;
 
     Ok(git_deps)
 }
 
+use diesel::types::{FromSql, FromSqlRow, Integer};
+use diesel::row::Row;
+
+// FIXME: Replace with `#[derive(FromSqlRow)]` in Diesel 1.1
+impl FromSqlRow<Integer, Pg> for Kind {
+    fn build_from_row<R: Row<Pg>>(row: &mut R) -> Result<Self, Box<Error + Send + Sync>> {
+        Self::from_sql(row.take())
+    }
+}
+
+impl FromSql<Integer, Pg> for Kind {
+    fn from_sql(bytes: Option<&[u8]>) -> Result<Self, Box<Error + Send + Sync>> {
+        match <i32 as FromSql<Integer, Pg>>::from_sql(bytes)? {
+            0 => Ok(Kind::Normal),
+            1 => Ok(Kind::Build),
+            2 => Ok(Kind::Dev),
+            n => Err(format!("unknown kind: {}", n).into()),
+        }
+    }
+}
+
 impl Queryable<dependencies::SqlType, Pg> for Dependency {
     type Row = (
         i32,
@@ -164,7 +172,7 @@ impl Queryable<dependencies::SqlType, Pg> for Dependency {
         bool,
         Vec<String>,
         Option<String>,
-        i32,
+        Kind,
     );
 
     fn build(row: Self::Row) -> Self {
@@ -177,12 +185,7 @@ impl Queryable<dependencies::SqlType, Pg> for Dependency {
             default_features: row.5,
             features: row.6,
             target: row.7,
-            kind: match row.8 {
-                0 => Kind::Normal,
-                1 => Kind::Build,
-                2 => Kind::Dev,
-                n => panic!("unknown kind: {}", n),
-            },
+            kind: row.8,
         }
     }
 }
@@ -202,12 +205,7 @@ impl QueryableByName<Pg> for Dependency {
             default_features: row.get::<SqlTypeOf<default_features>, _>("default_features")?,
             features: row.get::<SqlTypeOf<features>, _>("features")?,
             target: row.get::<SqlTypeOf<target>, _>("target")?,
-            kind: match row.get::<SqlTypeOf<kind>, _>("kind")? {
-                0 => Kind::Normal,
-                1 => Kind::Build,
-                2 => Kind::Dev,
-                n => return Err(format!("unknown kind: {}", n).into()),
-            },
+            kind: row.get::<SqlTypeOf<kind>, _>("kind")?,
         })
     }
 }

src/tests/all.rs

@@ -30,7 +30,6 @@ use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
 
 use cargo_registry::app::App;
 use cargo_registry::category::NewCategory;
-use cargo_registry::dependency::NewDependency;
 use cargo_registry::keyword::Keyword;
 use cargo_registry::krate::{CrateDownload, EncodableCrate, NewCrate};
 use cargo_registry::schema::*;
@@ -312,12 +311,16 @@ impl<'a> VersionBuilder<'a> {
 
         let new_deps = self.dependencies
             .into_iter()
-            .map(|(crate_id, target)| NewDependency {
-                version_id: vers.id,
-                req: ">= 0".into(),
-                crate_id,
-                target,
-                ..Default::default()
+            .map(|(crate_id, target)| {
+                (
+                    dependencies::version_id.eq(vers.id),
+                    dependencies::req.eq(">= 0"),
+                    dependencies::crate_id.eq(crate_id),
+                    dependencies::target.eq(target),
+                    dependencies::optional.eq(false),
+                    dependencies::default_features.eq(false),
+                    dependencies::features.eq(Vec::<String>::new()),
+                )
             })
             .collect::<Vec<_>>();
         insert_into(dependencies::table)
@@ -503,16 +506,17 @@ fn sign_in(req: &mut Request, app: &App) -> User {
 
 fn new_dependency(conn: &PgConnection, version: &Version, krate: &Crate) -> Dependency {
     use diesel::insert_into;
-    use cargo_registry::schema::dependencies;
-
-    insert_into(dependencies::table)
-        .values(&NewDependency {
-            version_id: version.id,
-            crate_id: krate.id,
-            req: ">= 0".into(),
-            optional: false,
-            ..Default::default()
-        })
+    use cargo_registry::schema::dependencies::dsl::*;
+
+    insert_into(dependencies)
+        .values((
+            version_id.eq(version.id),
+            crate_id.eq(krate.id),
+            req.eq(">= 0"),
+            optional.eq(false),
+            default_features.eq(false),
+            features.eq(Vec::<String>::new()),
+        ))
         .get_result(conn)
         .unwrap()
 }

src/upload.rs

@@ -250,3 +250,17 @@ impl Deref for CategoryList {
         &self.0
     }
 }
+
+use diesel::pg::Pg;
+use diesel::types::{IsNull, Text, ToSql, ToSqlOutput};
+use std::error::Error;
+use std::io::Write;
+
+impl ToSql<Text, Pg> for Feature {
+    fn to_sql<W: Write>(
+        &self,
+        out: &mut ToSqlOutput<W, Pg>,
+    ) -> Result<IsNull, Box<Error + Send + Sync>> {
+        ToSql::<Text, Pg>::to_sql(&**self, out)
+    }
+}