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..ef3db8f9b75 --- /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; + +ALTER TABLE versions +ADD CONSTRAINT "fk_versions_published_by" +FOREIGN KEY (published_by) +REFERENCES users(id); diff --git a/src/bin/update-downloads.rs b/src/bin/update-downloads.rs index 25eb524e4ca..fe38771a0c3 100644 --- a/src/bin/update-downloads.rs +++ b/src/bin/update-downloads.rs @@ -146,6 +146,7 @@ mod test { None, None, 0, + user_id, ) .unwrap(); let version = version.save(conn, &[]).unwrap(); diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index c1991d4c9ad..031b297d6c5 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::{ @@ -106,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) @@ -146,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)) + .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(), @@ -185,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)) + .map(|(v, pb)| v.encodable(crate_name, pb)) .collect(); #[derive(Serialize)] @@ -218,10 +227,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)) + .map(|(version, krate_name, published_by)| version.encodable(&krate_name, published_by)) .collect(); #[derive(Serialize)] diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index b7e8931a098..237226e8221 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -146,6 +146,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 file_length as i32, + user.id, )? .save(&conn, &new_crate.authors)?; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index b185c9fa669..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)) + .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 8474ca6400a..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)) + .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), + version: version.encodable(&krate.name, published_by), })) } diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 9b8a0c35351..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), + version: version.encodable(&krate.name, published_by), })) } diff --git a/src/models/version.rs b/src/models/version.rs index 6efa01bc6c5..9e61d2e84ad 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}; @@ -23,6 +23,7 @@ pub struct Version { pub yanked: bool, pub license: Option, pub crate_size: Option, + pub published_by: Option, } #[derive(Insertable, Debug)] @@ -33,10 +34,11 @@ pub struct NewVersion { features: serde_json::Value, license: Option, crate_size: Option, + published_by: i32, } 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, @@ -68,6 +70,7 @@ impl Version { authors: format!("/api/v1/crates/{}/{}/authors", crate_name, num), }, crate_size, + published_by: published_by.map(|pb| pb.encodable_public()), } } @@ -107,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 { @@ -118,6 +130,7 @@ impl NewVersion { license: Option, license_file: Option<&str>, crate_size: i32, + published_by: i32, ) -> CargoResult { let features = serde_json::to_value(features)?; @@ -127,6 +140,7 @@ impl NewVersion { features, license, crate_size: Some(crate_size), + published_by, }; new_version.validate_license(license_file)?; diff --git a/src/schema.rs b/src/schema.rs index e7a7ced1628..2ea0a1d0f72 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -903,6 +903,12 @@ table! { /// /// (Automatically generated by Diesel.) crate_size -> Nullable, + /// The `published_by` column of the `versions` table. + /// + /// Its SQL type is `Nullable`. + /// + /// (Automatically generated by Diesel.) + published_by -> Nullable, } } @@ -927,6 +933,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, diff --git a/src/tests/builders.rs b/src/tests/builders.rs index 4e8d0870d17..832d06fe70b 100644 --- a/src/tests/builders.rs +++ b/src/tests/builders.rs @@ -68,7 +68,12 @@ impl<'a> VersionBuilder<'a> { 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 { @@ -83,6 +88,7 @@ impl<'a> VersionBuilder<'a> { license, self.license_file, self.size, + published_by, )? .save(connection, &[])?; @@ -119,10 +125,16 @@ impl<'a> VersionBuilder<'a> { /// # Panics /// /// Panics (and fails the test) if any part of inserting the version record fails. - pub fn expect_build(self, crate_id: i32, connection: &PgConnection) -> Version { - self.build(crate_id, connection).unwrap_or_else(|e| { - panic!("Unable to create version: {:?}", e); - }) + pub fn expect_build( + self, + crate_id: i32, + published_by: i32, + connection: &PgConnection, + ) -> Version { + self.build(crate_id, published_by, connection) + .unwrap_or_else(|e| { + panic!("Unable to create version: {:?}", e); + }) } } @@ -266,7 +278,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() { diff --git a/src/tests/krate.rs b/src/tests/krate.rs index a0e034448ec..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] @@ -1255,7 +1284,7 @@ fn dependencies() { app.db(|conn| { let c1 = CrateBuilder::new("foo_deps", user.id).expect_build(conn); - let v = VersionBuilder::new("1.0.0").expect_build(c1.id, conn); + let v = VersionBuilder::new("1.0.0").expect_build(c1.id, user.id, conn); let c2 = CrateBuilder::new("bar_deps", user.id).expect_build(conn); new_dependency(conn, &v, &c2); }); @@ -1752,6 +1781,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") diff --git a/src/tests/version.rs b/src/tests/version.rs index 26f8bb9aa45..fc5c6bc3ec5 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -47,15 +47,15 @@ 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, conn) + .expect_build(krate.id, user.id, conn) }); let url = format!("/api/v1/versions/{}", v.id); @@ -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(); @@ -87,7 +128,7 @@ fn record_rerendered_readme_time() { app.db(|conn| { let c = CrateBuilder::new("foo_authors", user.id).expect_build(conn); - let version = VersionBuilder::new("1.0.0").expect_build(c.id, conn); + let version = VersionBuilder::new("1.0.0").expect_build(c.id, user.id, conn); version.record_readme_rendering(conn).unwrap(); version.record_readme_rendering(conn).unwrap(); diff --git a/src/views/mod.rs b/src/views/mod.rs index bcb9ee4d8a1..00a14a4cddd 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: Option, } #[derive(Serialize, Deserialize, Debug)] @@ -304,6 +305,7 @@ mod tests { authors: "".to_string(), }, crate_size: Some(1234), + published_by: None, }; let json = serde_json::to_string(&ver).unwrap(); assert!(json