From 09f12add14a49c4ee53da9328567fd4029d421e5 Mon Sep 17 00:00:00 2001 From: andy-bell Date: Sat, 24 Nov 2018 22:17:52 +0000 Subject: [PATCH 01/13] Adds in new migration to add the published_by field to the versions table. Also updates the schema.rb to reflect this table change. published by is a foreign key referencing the users table. --- .../down.sql | 3 +++ .../2018-11-01-223239_add_published_by_to_versions/up.sql | 7 +++++++ src/schema.rs | 7 +++++++ 3 files changed, 17 insertions(+) create mode 100644 migrations/2018-11-01-223239_add_published_by_to_versions/down.sql create mode 100644 migrations/2018-11-01-223239_add_published_by_to_versions/up.sql diff --git a/migrations/2018-11-01-223239_add_published_by_to_versions/down.sql b/migrations/2018-11-01-223239_add_published_by_to_versions/down.sql new file mode 100644 index 00000000000..c055cf9a95c --- /dev/null +++ b/migrations/2018-11-01-223239_add_published_by_to_versions/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE versions DROP CONSTRAINT fk_versions_published_by; + +ALTER TABLE versions DROP COLUMN published_by; diff --git a/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql b/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql new file mode 100644 index 00000000000..8f1464c6b30 --- /dev/null +++ b/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql @@ -0,0 +1,7 @@ +ALTER TABLE versions +ADD COLUMN published_by integer NOT NULL; + +ALTER TABLE versions +ADD CONSTRAINT "fk_versions_published_by" +FOREIGN KEY (published_by) +REFERENCES users(id); diff --git a/src/schema.rs b/src/schema.rs index d5436fcb094..a38e55c7e5a 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -897,6 +897,12 @@ table! { /// /// (Automatically generated by Diesel.) crate_size -> Nullable, + /// The `published_by` column of the `versions` table. + /// + /// Its SQL type is `Int4`. + /// + /// (Automatically generated by Diesel.) + published_by -> Int4, } } @@ -921,6 +927,7 @@ joinable!(version_authors -> users (user_id)); joinable!(version_authors -> versions (version_id)); joinable!(version_downloads -> versions (version_id)); joinable!(versions -> crates (crate_id)); +joinable!(versions -> users (published_by)); allow_tables_to_appear_in_same_query!( api_tokens, From acffea69e2ea7f3b910b7d52ced05b23d5098a74 Mon Sep 17 00:00:00 2001 From: andy-bell Date: Sat, 24 Nov 2018 22:19:28 +0000 Subject: [PATCH 02/13] Updates the Version and NewVersion models to add in the new field. --- src/models/version.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/models/version.rs b/src/models/version.rs index c8494f62aef..9f830d05c5d 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -27,6 +27,7 @@ pub struct Version { pub yanked: bool, pub license: Option, pub crate_size: Option, + pub published_by: i32, } #[derive(Insertable, Debug)] @@ -37,6 +38,7 @@ pub struct NewVersion { features: serde_json::Value, license: Option, crate_size: Option, + published_by: i32, } impl Version { @@ -51,6 +53,7 @@ impl Version { yanked, license, crate_size, + published_by, .. } = self; let num = num.to_string(); @@ -72,6 +75,7 @@ impl Version { authors: format!("/api/v1/crates/{}/{}/authors", crate_name, num), }, crate_size, + published_by, } } @@ -121,6 +125,7 @@ impl NewVersion { license: Option, license_file: Option<&str>, crate_size: Option, + published_by: i32, ) -> CargoResult { let features = serde_json::to_value(features)?; @@ -130,6 +135,7 @@ impl NewVersion { features, license, crate_size, + published_by, }; new_version.validate_license(license_file)?; From dc302cae28bafe6577345def2f44cff98957372a Mon Sep 17 00:00:00 2001 From: andy-bell Date: Sat, 24 Nov 2018 22:33:24 +0000 Subject: [PATCH 03/13] Makes the EncodeableVersion struct reference the match the Version Struct correctly. Fixes test in the same file broken by this change --- src/views/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/views/mod.rs b/src/views/mod.rs index 5ce2e3da790..08f60f72a8a 100644 --- a/src/views/mod.rs +++ b/src/views/mod.rs @@ -201,6 +201,7 @@ pub struct EncodableVersion { pub license: Option, pub links: EncodableVersionLinks, pub crate_size: Option, + pub published_by: i32, } #[derive(Serialize, Deserialize, Debug)] @@ -295,6 +296,7 @@ mod tests { authors: "".to_string(), }, crate_size: Some(1234), + published_by: 1, }; let json = serde_json::to_string(&ver).unwrap(); assert!( From 997f1e1f133b652a83e34253e9dd90ca9de38fcb Mon Sep 17 00:00:00 2001 From: andy-bell Date: Sat, 24 Nov 2018 22:34:18 +0000 Subject: [PATCH 04/13] Ensures the user id is being passed onto the creation of the new versions in the krate publish controller and in the update-downloads bin --- src/bin/update-downloads.rs | 1 + src/controllers/krate/publish.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/bin/update-downloads.rs b/src/bin/update-downloads.rs index 7b82a0b1efe..299762d3564 100644 --- a/src/bin/update-downloads.rs +++ b/src/bin/update-downloads.rs @@ -145,6 +145,7 @@ mod test { None, None, None, + user_id, ).unwrap(); let version = version.save(&conn, &[]).unwrap(); (krate, version) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index e6fa6feb72f..b8ebacb7f64 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -136,6 +136,7 @@ pub fn publish(req: &mut dyn Request) -> CargoResult { // Downcast is okay because the file length must be less than the max upload size // to get here, and max upload sizes are way less than i32 max Some(file_length as i32), + user.id, )?.save(&conn, &new_crate.authors)?; // Link this new version to all dependencies From e5ffc604df1072f682f6817f86c164a1c49b53f7 Mon Sep 17 00:00:00 2001 From: andy-bell Date: Sat, 24 Nov 2018 22:35:14 +0000 Subject: [PATCH 05/13] Fixes rest of thetests broken by the prior changes by adding in user_id arguments to pass it through. Unsure if this is the best way to solve this issue in the tests. --- src/tests/all.rs | 14 ++++++++------ src/tests/krate.rs | 2 +- src/tests/version.rs | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/tests/all.rs b/src/tests/all.rs index a7f2b6523bd..7aa115c1269 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -305,7 +305,7 @@ impl<'a> VersionBuilder<'a> { Self { yanked, ..self } } - fn build(self, crate_id: i32, connection: &PgConnection) -> CargoResult { + fn build(self, crate_id: i32, connection: &PgConnection, user_id: i32) -> CargoResult { use diesel::{insert_into, update}; let license = match self.license { @@ -320,6 +320,7 @@ impl<'a> VersionBuilder<'a> { license, self.license_file, None, + user_id, )?.save(connection, &[])?; if self.yanked { @@ -425,7 +426,7 @@ impl<'a> CrateBuilder<'a> { self } - fn build(mut self, connection: &PgConnection) -> CargoResult { + fn build(mut self, connection: &PgConnection, user_id: i32) -> CargoResult { use diesel::{insert_into, select, update}; let mut krate = self @@ -473,7 +474,7 @@ impl<'a> CrateBuilder<'a> { } for version_builder in self.versions { - version_builder.build(krate.id, connection)?; + version_builder.build(krate.id, connection, user_id)?; } if !self.keywords.is_empty() { @@ -485,15 +486,16 @@ impl<'a> CrateBuilder<'a> { fn expect_build(self, connection: &PgConnection) -> Crate { let name = self.krate.name; - self.build(connection).unwrap_or_else(|e| { + let owner = self.owner_id; + self.build(connection, owner).unwrap_or_else(|e| { panic!("Unable to create crate {}: {:?}", name, e); }) } } -fn new_version(crate_id: i32, num: &str, crate_size: Option) -> NewVersion { +fn new_version(crate_id: i32, num: &str, crate_size: Option, user_id: i32) -> NewVersion { let num = semver::Version::parse(num).unwrap(); - NewVersion::new(crate_id, &num, &HashMap::new(), None, None, crate_size).unwrap() + NewVersion::new(crate_id, &num, &HashMap::new(), None, None, crate_size, user_id).unwrap() } fn krate(name: &str) -> Crate { diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 46a6d536dd6..c35d220222c 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1248,7 +1248,7 @@ fn dependencies() { let conn = app.diesel_database.get().unwrap(); let user = new_user("foo").create_or_update(&conn).unwrap(); let c1 = CrateBuilder::new("foo_deps", user.id).expect_build(&conn); - let v = new_version(c1.id, "1.0.0", None).save(&conn, &[]).unwrap(); + let v = new_version(c1.id, "1.0.0", None, user.id).save(&conn, &[]).unwrap(); let c2 = CrateBuilder::new("bar_deps", user.id).expect_build(&conn); new_dependency(&conn, &v, &c2); } diff --git a/src/tests/version.rs b/src/tests/version.rs index f043a7eda75..f078902ef45 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -66,7 +66,7 @@ fn show() { let conn = app.diesel_database.get().unwrap(); let user = new_user("foo").create_or_update(&conn).unwrap(); let krate = CrateBuilder::new("foo_vers_show", user.id).expect_build(&conn); - new_version(krate.id, "2.0.0", Some(1234)) + new_version(krate.id, "2.0.0", Some(1234), user.id) .save(&conn, &[]) .unwrap() }; @@ -85,7 +85,7 @@ fn authors() { let conn = app.diesel_database.get().unwrap(); let u = new_user("foo").create_or_update(&conn).unwrap(); let c = CrateBuilder::new("foo_authors", u.id).expect_build(&conn); - new_version(c.id, "1.0.0", None).save(&conn, &[]).unwrap(); + new_version(c.id, "1.0.0", None, u.id).save(&conn, &[]).unwrap(); } let mut response = ok_resp!(middle.call(&mut req)); let mut data = Vec::new(); @@ -103,7 +103,7 @@ fn record_rerendered_readme_time() { let conn = app.diesel_database.get().unwrap(); let u = new_user("foo").create_or_update(&conn).unwrap(); let c = CrateBuilder::new("foo_authors", u.id).expect_build(&conn); - new_version(c.id, "1.0.0", None).save(&conn, &[]).unwrap() + new_version(c.id, "1.0.0", None, u.id).save(&conn, &[]).unwrap() }; { let conn = app.diesel_database.get().unwrap(); From d039b5d188aa665e37bc0df140a5b4885cbaebdc Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Tue, 27 Nov 2018 15:40:35 -0500 Subject: [PATCH 06/13] cargo fmt --- src/tests/all.rs | 10 +++++++++- src/tests/krate.rs | 4 +++- src/tests/version.rs | 8 ++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/tests/all.rs b/src/tests/all.rs index 502e0a2ce6d..76faef0e9b8 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -496,7 +496,15 @@ impl<'a> CrateBuilder<'a> { fn new_version(crate_id: i32, num: &str, crate_size: Option, user_id: i32) -> NewVersion { let num = semver::Version::parse(num).unwrap(); - NewVersion::new(crate_id, &num, &HashMap::new(), None, None, crate_size, user_id).unwrap() + NewVersion::new( + crate_id, + &num, + &HashMap::new(), + None, + None, + crate_size, + user_id, + ).unwrap() } fn krate(name: &str) -> Crate { diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 625da2e3739..4c5de6bf92f 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1144,7 +1144,9 @@ fn dependencies() { let conn = app.diesel_database.get().unwrap(); let user = new_user("foo").create_or_update(&conn).unwrap(); let c1 = CrateBuilder::new("foo_deps", user.id).expect_build(&conn); - let v = new_version(c1.id, "1.0.0", None, user.id).save(&conn, &[]).unwrap(); + let v = new_version(c1.id, "1.0.0", None, user.id) + .save(&conn, &[]) + .unwrap(); let c2 = CrateBuilder::new("bar_deps", user.id).expect_build(&conn); new_dependency(&conn, &v, &c2); } diff --git a/src/tests/version.rs b/src/tests/version.rs index b1794ba2ec1..86424ebeb42 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -83,7 +83,9 @@ fn authors() { let conn = app.diesel_database.get().unwrap(); let u = new_user("foo").create_or_update(&conn).unwrap(); let c = CrateBuilder::new("foo_authors", u.id).expect_build(&conn); - new_version(c.id, "1.0.0", None, u.id).save(&conn, &[]).unwrap(); + new_version(c.id, "1.0.0", None, u.id) + .save(&conn, &[]) + .unwrap(); } let mut response = ok_resp!(middle.call(&mut req)); let mut data = Vec::new(); @@ -101,7 +103,9 @@ fn record_rerendered_readme_time() { let conn = app.diesel_database.get().unwrap(); let u = new_user("foo").create_or_update(&conn).unwrap(); let c = CrateBuilder::new("foo_authors", u.id).expect_build(&conn); - new_version(c.id, "1.0.0", None, u.id).save(&conn, &[]).unwrap() + new_version(c.id, "1.0.0", None, u.id) + .save(&conn, &[]) + .unwrap() }; { let conn = app.diesel_database.get().unwrap(); From d7816ec051d233ef81425ab574eba636d67378f8 Mon Sep 17 00:00:00 2001 From: andy-bell Date: Tue, 27 Nov 2018 21:58:15 +0000 Subject: [PATCH 07/13] Remove NOT NULL from published by column. --- .../2018-11-01-223239_add_published_by_to_versions/up.sql | 2 +- src/schema.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql b/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql index 8f1464c6b30..ef3db8f9b75 100644 --- a/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql +++ b/migrations/2018-11-01-223239_add_published_by_to_versions/up.sql @@ -1,5 +1,5 @@ ALTER TABLE versions -ADD COLUMN published_by integer NOT NULL; +ADD COLUMN published_by integer; ALTER TABLE versions ADD CONSTRAINT "fk_versions_published_by" diff --git a/src/schema.rs b/src/schema.rs index a38e55c7e5a..097ca42a2ce 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -899,10 +899,10 @@ table! { crate_size -> Nullable, /// The `published_by` column of the `versions` table. /// - /// Its SQL type is `Int4`. + /// Its SQL type is `Nullable`. /// /// (Automatically generated by Diesel.) - published_by -> Int4, + published_by -> Nullable, } } From b92c9f29bd7e8b419099e489bcb694c389387676 Mon Sep 17 00:00:00 2001 From: andy-bell Date: Thu, 29 Nov 2018 22:24:36 +0000 Subject: [PATCH 08/13] Change models to reflect the fact that published by is a nullable field now. --- src/models/version.rs | 2 +- src/views/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/models/version.rs b/src/models/version.rs index 9f830d05c5d..3519756301c 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -27,7 +27,7 @@ pub struct Version { pub yanked: bool, pub license: Option, pub crate_size: Option, - pub published_by: i32, + pub published_by: Option, } #[derive(Insertable, Debug)] diff --git a/src/views/mod.rs b/src/views/mod.rs index 08f60f72a8a..d351bc60d49 100644 --- a/src/views/mod.rs +++ b/src/views/mod.rs @@ -201,7 +201,7 @@ pub struct EncodableVersion { pub license: Option, pub links: EncodableVersionLinks, pub crate_size: Option, - pub published_by: i32, + pub published_by: Option, } #[derive(Serialize, Deserialize, Debug)] @@ -296,7 +296,7 @@ mod tests { authors: "".to_string(), }, crate_size: Some(1234), - published_by: 1, + published_by: Some(1), }; let json = serde_json::to_string(&ver).unwrap(); assert!( From 3654052a7e0a4edcfd04e760e57f445df59c94ec Mon Sep 17 00:00:00 2001 From: andy-bell Date: Thu, 29 Nov 2018 22:27:59 +0000 Subject: [PATCH 09/13] Master changed VersionBuilder and CrateBuilder to be part of their own builders.rs file and this PR accidentally merged them back into the all.rs file when trying to resolve the merge conflict. This commit removes them from all.rs so that it reflects master, and then makes the minimal changes needed to the models in builders.rs to reflect the new database updates. --- src/tests/all.rs | 227 ------------------------------------------ src/tests/builders.rs | 5 +- 2 files changed, 3 insertions(+), 229 deletions(-) diff --git a/src/tests/all.rs b/src/tests/all.rs index 76faef0e9b8..f566030bef5 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -267,233 +267,6 @@ fn add_team_to_crate(t: &Team, krate: &Crate, u: &User, conn: &PgConnection) -> use cargo_registry::util::CargoResult; -struct VersionBuilder<'a> { - num: semver::Version, - license: Option<&'a str>, - license_file: Option<&'a str>, - features: HashMap>, - dependencies: Vec<(i32, Option<&'static str>)>, - yanked: bool, -} - -impl<'a> VersionBuilder<'a> { - fn new(num: &str) -> Self { - let num = semver::Version::parse(num).unwrap_or_else(|e| { - panic!("The version {} is not valid: {}", num, e); - }); - - VersionBuilder { - num, - license: None, - license_file: None, - features: HashMap::new(), - dependencies: Vec::new(), - yanked: false, - } - } - - fn license(mut self, license: Option<&'a str>) -> Self { - self.license = license; - self - } - - fn dependency(mut self, dependency: &Crate, target: Option<&'static str>) -> Self { - self.dependencies.push((dependency.id, target)); - self - } - - fn yanked(self, yanked: bool) -> Self { - Self { yanked, ..self } - } - - fn build(self, crate_id: i32, connection: &PgConnection, user_id: i32) -> CargoResult { - use diesel::{insert_into, update}; - - let license = match self.license { - Some(license) => Some(license.to_owned()), - None => None, - }; - - let mut vers = NewVersion::new( - crate_id, - &self.num, - &self.features, - license, - self.license_file, - None, - user_id, - )?.save(connection, &[])?; - - if self.yanked { - vers = update(&vers) - .set(versions::yanked.eq(true)) - .get_result(connection)?; - } - - let new_deps = self - .dependencies - .into_iter() - .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::::new()), - ) - }).collect::>(); - insert_into(dependencies::table) - .values(&new_deps) - .execute(connection)?; - - Ok(vers) - } -} - -impl<'a> From<&'a str> for VersionBuilder<'a> { - fn from(num: &'a str) -> Self { - VersionBuilder::new(num) - } -} - -struct CrateBuilder<'a> { - owner_id: i32, - krate: NewCrate<'a>, - downloads: Option, - recent_downloads: Option, - versions: Vec>, - keywords: Vec<&'a str>, -} - -impl<'a> CrateBuilder<'a> { - fn new(name: &str, owner_id: i32) -> CrateBuilder<'_> { - CrateBuilder { - owner_id, - krate: NewCrate { - name, - ..NewCrate::default() - }, - downloads: None, - recent_downloads: None, - versions: Vec::new(), - keywords: Vec::new(), - } - } - - fn description(mut self, description: &'a str) -> Self { - self.krate.description = Some(description); - self - } - - fn documentation(mut self, documentation: &'a str) -> Self { - self.krate.documentation = Some(documentation); - self - } - - fn homepage(mut self, homepage: &'a str) -> Self { - self.krate.homepage = Some(homepage); - self - } - - fn readme(mut self, readme: &'a str) -> Self { - self.krate.readme = Some(readme); - self - } - - fn max_upload_size(mut self, max_upload_size: i32) -> Self { - self.krate.max_upload_size = Some(max_upload_size); - self - } - - fn downloads(mut self, downloads: i32) -> Self { - self.downloads = Some(downloads); - self - } - - fn recent_downloads(mut self, recent_downloads: i32) -> Self { - self.recent_downloads = Some(recent_downloads); - self - } - - fn version>>(mut self, version: T) -> Self { - self.versions.push(version.into()); - self - } - - fn keyword(mut self, keyword: &'a str) -> Self { - self.keywords.push(keyword); - self - } - - fn build(mut self, connection: &PgConnection, user_id: i32) -> CargoResult { - use diesel::{insert_into, select, update}; - - let mut krate = self - .krate - .create_or_update(connection, None, self.owner_id)?; - - // Since we are using `NewCrate`, we can't set all the - // crate properties in a single DB call. - - let old_downloads = self.downloads.unwrap_or(0) - self.recent_downloads.unwrap_or(0); - let now = Utc::now(); - let old_date = now.naive_utc().date() - chrono::Duration::days(91); - - if let Some(downloads) = self.downloads { - let crate_download = CrateDownload { - crate_id: krate.id, - downloads: old_downloads, - date: old_date, - }; - - insert_into(crate_downloads::table) - .values(&crate_download) - .execute(connection)?; - krate.downloads = downloads; - update(&krate).set(&krate).execute(connection)?; - } - - if self.recent_downloads.is_some() { - let crate_download = CrateDownload { - crate_id: krate.id, - downloads: self.recent_downloads.unwrap(), - date: now.naive_utc().date(), - }; - - insert_into(crate_downloads::table) - .values(&crate_download) - .execute(connection)?; - - no_arg_sql_function!(refresh_recent_crate_downloads, ()); - select(refresh_recent_crate_downloads).execute(connection)?; - } - - if self.versions.is_empty() { - self.versions.push(VersionBuilder::new("0.99.0")); - } - - for version_builder in self.versions { - version_builder.build(krate.id, connection, user_id)?; - } - - if !self.keywords.is_empty() { - Keyword::update_crate(connection, &krate, &self.keywords)?; - } - - Ok(krate) - } - - fn expect_build(self, connection: &PgConnection) -> Crate { - let name = self.krate.name; - let owner = self.owner_id; - self.build(connection, owner).unwrap_or_else(|e| { - panic!("Unable to create crate {}: {:?}", name, e); - }) - } -} - fn new_version(crate_id: i32, num: &str, crate_size: Option, user_id: i32) -> NewVersion { let num = semver::Version::parse(num).unwrap(); NewVersion::new( diff --git a/src/tests/builders.rs b/src/tests/builders.rs index 82fbeafa014..699a3867902 100644 --- a/src/tests/builders.rs +++ b/src/tests/builders.rs @@ -66,7 +66,7 @@ impl<'a> VersionBuilder<'a> { Self { yanked, ..self } } - fn build(self, crate_id: i32, connection: &PgConnection) -> CargoResult { + fn build(self, crate_id: i32,published_by: i32, connection: &PgConnection) -> CargoResult { use diesel::{insert_into, update}; let license = match self.license { @@ -81,6 +81,7 @@ impl<'a> VersionBuilder<'a> { license, self.license_file, None, + published_by, )?.save(connection, &[])?; if self.yanked { @@ -251,7 +252,7 @@ impl<'a> CrateBuilder<'a> { } for version_builder in self.versions { - version_builder.build(krate.id, connection)?; + version_builder.build(krate.id, self.owner_id, connection)?; } if !self.keywords.is_empty() { From 477c73caa15eea9dcf27aa9b2fba30d961285130 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 1 Feb 2019 16:45:27 -0500 Subject: [PATCH 10/13] Have version encode take an optional User that published the crate Always pass None for the moment --- src/controllers/krate/metadata.rs | 6 +++--- src/controllers/user/me.rs | 2 +- src/controllers/version/deprecated.rs | 4 ++-- src/controllers/version/metadata.rs | 2 +- src/models/version.rs | 7 +++---- src/views/mod.rs | 4 ++-- 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index c1991d4c9ad..08d61e65a1f 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -148,7 +148,7 @@ pub fn show(req: &mut dyn Request) -> CargoResult { ), versions: versions .into_iter() - .map(|v| v.encodable(&krate.name)) + .map(|v| v.encodable(&krate.name, None)) .collect(), keywords: kws.into_iter().map(|k| k.encodable()).collect(), categories: cats.into_iter().map(|k| k.encodable()).collect(), @@ -189,7 +189,7 @@ pub fn versions(req: &mut dyn Request) -> CargoResult { versions.sort_by(|a, b| b.num.cmp(&a.num)); let versions = versions .into_iter() - .map(|v| v.encodable(crate_name)) + .map(|v| v.encodable(crate_name, None)) .collect(); #[derive(Serialize)] @@ -221,7 +221,7 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult { .select((versions::all_columns, crates::name)) .load::<(Version, String)>(&*conn)? .into_iter() - .map(|(version, krate_name)| version.encodable(&krate_name)) + .map(|(version, krate_name)| version.encodable(&krate_name, None)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index b185c9fa669..118b8dceea7 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -68,7 +68,7 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { let versions = data .into_iter() - .map(|((version, crate_name), _)| version.encodable(&crate_name)) + .map(|((version, crate_name), _)| version.encodable(&crate_name, None)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 8474ca6400a..3a5a45c9725 100644 --- a/src/controllers/version/deprecated.rs +++ b/src/controllers/version/deprecated.rs @@ -28,7 +28,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult { .filter(versions::id.eq(any(ids))) .load::<(Version, String)>(&*conn)? .into_iter() - .map(|(version, crate_name)| version.encodable(&crate_name)) + .map(|(version, crate_name)| version.encodable(&crate_name, None)) .collect(); #[derive(Serialize)] @@ -56,6 +56,6 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult { version: EncodableVersion, } Ok(req.json(&R { - version: version.encodable(&krate.name), + version: version.encodable(&krate.name, None), })) } diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 9b8a0c35351..e5b9535550c 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -74,6 +74,6 @@ pub fn show(req: &mut dyn Request) -> CargoResult { version: EncodableVersion, } Ok(req.json(&R { - version: version.encodable(&krate.name), + version: version.encodable(&krate.name, None), })) } diff --git a/src/models/version.rs b/src/models/version.rs index 07165bb4d47..ea9719f10f3 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -5,7 +5,7 @@ use diesel::prelude::*; use crate::util::{human, CargoResult}; -use crate::models::{Crate, Dependency}; +use crate::models::{Crate, Dependency, User}; use crate::schema::*; use crate::views::{EncodableVersion, EncodableVersionLinks}; @@ -38,7 +38,7 @@ pub struct NewVersion { } impl Version { - pub fn encodable(self, crate_name: &str) -> EncodableVersion { + pub fn encodable(self, crate_name: &str, published_by: Option) -> EncodableVersion { let Version { id, num, @@ -49,7 +49,6 @@ impl Version { yanked, license, crate_size, - published_by, .. } = self; let num = num.to_string(); @@ -71,7 +70,7 @@ impl Version { authors: format!("/api/v1/crates/{}/{}/authors", crate_name, num), }, crate_size, - published_by, + published_by: published_by.map(|pb| pb.encodable_public()), } } diff --git a/src/views/mod.rs b/src/views/mod.rs index 259610d3a46..00a14a4cddd 100644 --- a/src/views/mod.rs +++ b/src/views/mod.rs @@ -201,7 +201,7 @@ pub struct EncodableVersion { pub license: Option, pub links: EncodableVersionLinks, pub crate_size: Option, - pub published_by: Option, + pub published_by: Option, } #[derive(Serialize, Deserialize, Debug)] @@ -305,7 +305,7 @@ mod tests { authors: "".to_string(), }, crate_size: Some(1234), - published_by: Some(1), + published_by: None, }; let json = serde_json::to_string(&ver).unwrap(); assert!(json From 3b4bdef10fbc021904669a439c1f66d1d5e88f90 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 1 Feb 2019 21:04:49 -0500 Subject: [PATCH 11/13] Make a method to fetch the user the version was published by And use it in places where there's only one version --- src/controllers/version/metadata.rs | 4 ++- src/models/version.rs | 9 ++++++ src/tests/version.rs | 45 +++++++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index e5b9535550c..bacf03e3be3 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -68,12 +68,14 @@ pub fn authors(req: &mut dyn Request) -> CargoResult { /// API route to have. pub fn show(req: &mut dyn Request) -> CargoResult { let (version, krate) = version_and_crate(req)?; + let conn = req.db_conn()?; + let published_by = version.published_by(&conn); #[derive(Serialize)] struct R { version: EncodableVersion, } Ok(req.json(&R { - version: version.encodable(&krate.name, None), + version: version.encodable(&krate.name, published_by), })) } diff --git a/src/models/version.rs b/src/models/version.rs index ea9719f10f3..9e61d2e84ad 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -110,6 +110,15 @@ impl Version { .set(rendered_at.eq(now)) .execute(conn) } + + /// 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 { + match self.published_by { + Some(pb) => users::table.find(pb).first(conn).ok(), + None => None, + } + } } impl NewVersion { diff --git a/src/tests/version.rs b/src/tests/version.rs index b6a9fdb220c..fc5c6bc3ec5 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -47,12 +47,12 @@ fn index() { } #[test] -fn show() { +fn show_by_id() { let (app, anon, user) = TestApp::init().with_user(); let user = user.as_model(); let v = app.db(|conn| { - let krate = CrateBuilder::new("foo_vers_show", user.id).expect_build(conn); + let krate = CrateBuilder::new("foo_vers_show_id", user.id).expect_build(conn); VersionBuilder::new("2.0.0") .size(1234) .expect_build(krate.id, user.id, conn) @@ -64,6 +64,47 @@ fn show() { assert_eq!(json.version.crate_size, Some(1234)); } +#[test] +fn show_by_crate_name_and_semver_with_published_by() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + let v = app.db(|conn| { + let krate = CrateBuilder::new("foo_vers_show", user.id).expect_build(conn); + VersionBuilder::new("2.0.0") + .size(1234) + .expect_build(krate.id, user.id, conn) + }); + + let json: VersionResponse = anon.show_version("foo_vers_show", "2.0.0"); + assert_eq!(json.version.id, v.id); + assert_eq!(json.version.crate_size, Some(1234)); + assert_eq!(json.version.published_by.unwrap().login, user.gh_login); +} + +#[test] +fn show_by_crate_name_and_semver_no_published_by() { + use diesel::update; + + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + app.db(|conn| { + CrateBuilder::new("foo_vers_show_no_pb", user.id) + .version("1.0.0") + .expect_build(conn); + // Mimic a version published before we started recording who published versions + let none: Option = None; + update(versions::table) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + }); + + let json: VersionResponse = anon.show_version("foo_vers_show_no_pb", "1.0.0"); + assert!(json.version.published_by.is_none()); +} + #[test] fn authors() { let (app, anon, user) = TestApp::init().with_user(); From b9d15df5aa76cfced6969e41aba297e0cfdbf12e Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 1 Feb 2019 21:27:06 -0500 Subject: [PATCH 12/13] Query for published_by with queries for version and crate When we need published_by --- src/controllers/krate/metadata.rs | 14 +++++++--- src/controllers/user/me.rs | 13 +++++++-- src/controllers/version/deprecated.rs | 24 +++++++++++----- src/tests/krate.rs | 40 +++++++++++++++++++++++++++ src/tests/user.rs | 29 ++++++++++++++++++- 5 files changed, 105 insertions(+), 15 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 08d61e65a1f..9c735697ff6 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -6,7 +6,8 @@ use crate::controllers::prelude::*; use crate::models::{ - Category, Crate, CrateCategory, CrateDownload, CrateKeyword, CrateVersions, Keyword, Version, + Category, Crate, CrateCategory, CrateDownload, CrateKeyword, CrateVersions, Keyword, User, + Version, }; use crate::schema::*; use crate::views::{ @@ -218,10 +219,15 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult { let versions = versions::table .filter(versions::id.eq(any(version_ids))) .inner_join(crates::table) - .select((versions::all_columns, crates::name)) - .load::<(Version, String)>(&*conn)? + .left_outer_join(users::table) + .select(( + versions::all_columns, + crates::name, + users::all_columns.nullable(), + )) + .load::<(Version, String, Option)>(&*conn)? .into_iter() - .map(|(version, krate_name)| version.encodable(&krate_name, None)) + .map(|(version, krate_name, published_by)| version.encodable(&krate_name, published_by)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 118b8dceea7..b746c2356a2 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -55,11 +55,16 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { let followed_crates = Follow::belonging_to(user).select(follows::crate_id); let data = versions::table .inner_join(crates::table) + .left_outer_join(users::table) .filter(crates::id.eq(any(followed_crates))) .order(versions::created_at.desc()) - .select((versions::all_columns, crates::name)) + .select(( + versions::all_columns, + crates::name, + users::all_columns.nullable(), + )) .paginate(limit, offset) - .load::<((Version, String), i64)>(&*conn)?; + .load::<((Version, String, Option), i64)>(&*conn)?; let more = data .get(0) @@ -68,7 +73,9 @@ pub fn updates(req: &mut dyn Request) -> CargoResult { let versions = data .into_iter() - .map(|((version, crate_name), _)| version.encodable(&crate_name, None)) + .map(|((version, crate_name, published_by), _)| { + version.encodable(&crate_name, published_by) + }) .collect(); #[derive(Serialize)] diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 3a5a45c9725..346b089636c 100644 --- a/src/controllers/version/deprecated.rs +++ b/src/controllers/version/deprecated.rs @@ -7,7 +7,7 @@ use crate::controllers::prelude::*; -use crate::models::{Crate, Version}; +use crate::models::{Crate, User, Version}; use crate::schema::*; use crate::views::EncodableVersion; @@ -24,11 +24,16 @@ pub fn index(req: &mut dyn Request) -> CargoResult { let versions = versions::table .inner_join(crates::table) - .select((versions::all_columns, crates::name)) + .left_outer_join(users::table) + .select(( + versions::all_columns, + crates::name, + users::all_columns.nullable(), + )) .filter(versions::id.eq(any(ids))) - .load::<(Version, String)>(&*conn)? + .load::<(Version, String, Option)>(&*conn)? .into_iter() - .map(|(version, crate_name)| version.encodable(&crate_name, None)) + .map(|(version, crate_name, published_by)| version.encodable(&crate_name, published_by)) .collect(); #[derive(Serialize)] @@ -45,10 +50,15 @@ 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): (Version, Crate) = versions::table + let (version, krate, published_by): (Version, Crate, Option) = versions::table .find(id) .inner_join(crates::table) - .select((versions::all_columns, crate::models::krate::ALL_COLUMNS)) + .left_outer_join(users::table) + .select(( + versions::all_columns, + crate::models::krate::ALL_COLUMNS, + users::all_columns.nullable(), + )) .first(&*conn)?; #[derive(Serialize)] @@ -56,6 +66,6 @@ pub fn show_by_id(req: &mut dyn Request) -> CargoResult { version: EncodableVersion, } Ok(req.json(&R { - version: version.encodable(&krate.name, None), + version: version.encodable(&krate.name, published_by), })) } diff --git a/src/tests/krate.rs b/src/tests/krate.rs index ebe218d6e1a..e558b59b858 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -1752,6 +1752,46 @@ fn yanked_versions_not_included_in_reverse_dependencies() { assert_eq!(deps.meta.total, 0); } +#[test] +fn reverse_dependencies_includes_published_by_user_when_present() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + app.db(|conn| { + let c1 = CrateBuilder::new("c1", user.id) + .version("1.0.0") + .expect_build(conn); + CrateBuilder::new("c2", user.id) + .version(VersionBuilder::new("2.0.0").dependency(&c1, None)) + .expect_build(conn); + + // Make c2's version (and,incidentally, c1's, but that doesn't matter) mimic a version + // published before we started recording who published versions + let none: Option = None; + update(versions::table) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + + // c3's version will have the published by info recorded + CrateBuilder::new("c3", user.id) + .version(VersionBuilder::new("3.0.0").dependency(&c1, None)) + .expect_build(conn); + }); + + let deps = anon.reverse_dependencies("c1"); + assert_eq!(deps.versions.len(), 2); + + let c2_version = deps.versions.iter().find(|v| v.krate == "c2").unwrap(); + assert!(c2_version.published_by.is_none()); + + let c3_version = deps.versions.iter().find(|v| v.krate == "c3").unwrap(); + assert_eq!( + c3_version.published_by.as_ref().unwrap().login, + user.gh_login + ); +} + #[test] fn author_license_and_description_required() { let (_, _, _, token) = TestApp::init().with_token(); diff --git a/src/tests/user.rs b/src/tests/user.rs index b04933d0241..782ad4c4f40 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -142,6 +142,9 @@ fn crates_by_user_id_not_including_deleted_owners() { #[test] fn following() { + use cargo_registry::schema::versions; + use diesel::update; + #[derive(Deserialize)] struct R { versions: Vec, @@ -153,12 +156,21 @@ fn following() { } let (app, _, user) = TestApp::init().with_user(); - let user_id = user.as_model().id; + let user_model = user.as_model(); + let user_id = user_model.id; app.db(|conn| { CrateBuilder::new("foo_fighters", user_id) .version(VersionBuilder::new("1.0.0")) .expect_build(conn); + // Make foo_fighters's version mimic a version published before we started recording who + // published versions + let none: Option = None; + update(versions::table) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + CrateBuilder::new("bar_fighters", user_id) .version(VersionBuilder::new("1.0.0")) .expect_build(conn); @@ -176,6 +188,21 @@ fn following() { let r: R = user.get("/api/v1/me/updates").good(); assert_eq!(r.versions.len(), 2); assert_eq!(r.meta.more, false); + let foo_version = r + .versions + .iter() + .find(|v| v.krate == "foo_fighters") + .unwrap(); + assert!(foo_version.published_by.is_none()); + let bar_version = r + .versions + .iter() + .find(|v| v.krate == "bar_fighters") + .unwrap(); + assert_eq!( + bar_version.published_by.as_ref().unwrap().login, + user_model.gh_login + ); let r: R = user .get_with_query("/api/v1/me/updates", "per_page=1") From 62fdb18a0e79d6979853e6d5d4a62c8a44ab4bb8 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 1 Feb 2019 21:50:34 -0500 Subject: [PATCH 13/13] Query for published_by for multiple versions where needed --- src/controllers/krate/metadata.rs | 26 +++++++++++++++--------- src/tests/krate.rs | 33 +++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 9c735697ff6..031b297d6c5 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -107,9 +107,13 @@ pub fn show(req: &mut dyn Request) -> CargoResult { let conn = req.db_conn()?; let krate = Crate::by_name(name).first::(&*conn)?; - let mut versions = krate.all_versions().load::(&*conn)?; - versions.sort_by(|a, b| b.num.cmp(&a.num)); - let ids = versions.iter().map(|v| v.id).collect(); + let mut versions_and_publishers: Vec<(Version, Option)> = krate + .all_versions() + .left_outer_join(users::table) + .select((versions::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(); let kws = CrateKeyword::belonging_to(&krate) .inner_join(keywords::table) @@ -147,9 +151,9 @@ pub fn show(req: &mut dyn Request) -> CargoResult { false, recent_downloads, ), - versions: versions + versions: versions_and_publishers .into_iter() - .map(|v| v.encodable(&krate.name, None)) + .map(|(v, pb)| v.encodable(&krate.name, pb)) .collect(), keywords: kws.into_iter().map(|k| k.encodable()).collect(), categories: cats.into_iter().map(|k| k.encodable()).collect(), @@ -186,11 +190,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 = krate.all_versions().load::(&*conn)?; - versions.sort_by(|a, b| b.num.cmp(&a.num)); - let versions = versions + let mut versions_and_publishers: Vec<(Version, Option)> = krate + .all_versions() + .left_outer_join(users::table) + .select((versions::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 .into_iter() - .map(|v| v.encodable(crate_name, None)) + .map(|(v, pb)| v.encodable(crate_name, pb)) .collect(); #[derive(Serialize)] diff --git a/src/tests/krate.rs b/src/tests/krate.rs index e558b59b858..b6269c579a6 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -420,7 +420,7 @@ fn show() { let user = user.as_model(); let krate = app.db(|conn| { - CrateBuilder::new("foo_show", user.id) + let krate = CrateBuilder::new("foo_show", user.id) .description("description") .documentation("https://example.com") .homepage("http://example.com") @@ -430,7 +430,18 @@ fn show() { .keyword("kw1") .downloads(20) .recent_downloads(10) - .expect_build(conn) + .expect_build(conn); + + // Make version 1.0.0 mimic a version published before we started recording who published + // versions + let none: Option = None; + update(versions::table) + .filter(versions::num.eq("1.0.0")) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + + krate }); let json = anon.show_crate("foo_show"); @@ -448,6 +459,7 @@ fn show() { assert_eq!(json.versions[0].id, versions[0]); assert_eq!(json.versions[0].krate, json.krate.id); assert_eq!(json.versions[0].num, "1.0.0"); + assert!(json.versions[0].published_by.is_none()); let suffix = "/api/v1/crates/foo_show/1.0.0/download"; assert!( json.versions[0].dl_path.ends_with(suffix), @@ -459,6 +471,10 @@ fn show() { assert_eq!(json.versions[1].num, "0.5.1"); assert_eq!(json.versions[2].num, "0.5.0"); + assert_eq!( + json.versions[1].published_by.as_ref().unwrap().login, + user.gh_login + ); } #[test] @@ -489,6 +505,14 @@ fn versions() { .version("1.0.0") .version("0.5.0") .expect_build(conn); + // Make version 1.0.0 mimic a version published before we started recording who published + // versions + let none: Option = None; + update(versions::table) + .filter(versions::num.eq("1.0.0")) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); }); let json: VersionsList = anon.get("/api/v1/crates/foo_versions/versions").good(); @@ -497,6 +521,11 @@ fn versions() { assert_eq!(json.versions[0].num, "1.0.0"); assert_eq!(json.versions[1].num, "0.5.1"); assert_eq!(json.versions[2].num, "0.5.0"); + assert!(json.versions[0].published_by.is_none()); + assert_eq!( + json.versions[1].published_by.as_ref().unwrap().login, + user.gh_login + ); } #[test]