From 0048f169f905a784ad41751e13b488f4389b74f3 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 23 Dec 2017 11:21:06 -0700 Subject: [PATCH] Minor cleanup on dependency insertion and loading 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. --- src/dependency.rs | 78 +++++++++++++++++++++++------------------------ src/tests/all.rs | 38 ++++++++++++----------- src/upload.rs | 14 +++++++++ 3 files changed, 72 insertions(+), 58 deletions(-) diff --git a/src/dependency.rs b/src/dependency.rs index a3336749154..10e1fe785e8 100644 --- a/src/dependency.rs +++ b/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) -> EncodableDependency { @@ -101,9 +88,10 @@ impl ReverseDependency { pub fn add_dependencies( conn: &PgConnection, deps: &[::upload::CrateDependency], - version_id: i32, + target_version_id: i32, ) -> CargoResult> { use diesel::insert_into; + use self::dependencies::dsl::*; let git_and_new_dependencies = deps.iter() .map(|dep| { @@ -120,28 +108,27 @@ 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::, _>>()?; @@ -149,13 +136,34 @@ pub fn add_dependencies( 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 for Kind { + fn build_from_row>(row: &mut R) -> Result> { + Self::from_sql(row.take()) + } +} + +impl FromSql for Kind { + fn from_sql(bytes: Option<&[u8]>) -> Result> { + match >::from_sql(bytes)? { + 0 => Ok(Kind::Normal), + 1 => Ok(Kind::Build), + 2 => Ok(Kind::Dev), + n => Err(format!("unknown kind: {}", n).into()), + } + } +} + impl Queryable for Dependency { type Row = ( i32, @@ -166,7 +174,7 @@ impl Queryable for Dependency { bool, Vec, Option, - i32, + Kind, ); fn build(row: Self::Row) -> Self { @@ -179,12 +187,7 @@ impl Queryable 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, } } } @@ -204,12 +207,7 @@ impl QueryableByName for Dependency { default_features: row.get::, _>("default_features")?, features: row.get::, _>("features")?, target: row.get::, _>("target")?, - kind: match row.get::, _>("kind")? { - 0 => Kind::Normal, - 1 => Kind::Build, - 2 => Kind::Dev, - n => return Err(format!("unknown kind: {}", n).into()), - }, + kind: row.get::, _>("kind")?, }) } } diff --git a/src/tests/all.rs b/src/tests/all.rs index 2c7536dbdba..6e0fb4fa023 100644 --- a/src/tests/all.rs +++ b/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::*; @@ -313,13 +312,15 @@ 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() - } + ( + 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::::new()), + ) }) .collect::>(); insert_into(dependencies::table) @@ -505,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::::new()), + )) .get_result(conn) .unwrap() } diff --git a/src/upload.rs b/src/upload.rs index f712089f08f..64bd4480216 100644 --- a/src/upload.rs +++ b/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 for Feature { + fn to_sql( + &self, + out: &mut ToSqlOutput, + ) -> Result> { + ToSql::::to_sql(&**self, out) + } +}