From 4952046545c1276dbaab0aacbc6b8e1251d4e6d4 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 18 Mar 2022 11:25:57 -0400 Subject: [PATCH 1/3] Add an environment variable to let us exclude crate IDs from the recent downloads list --- src/config.rs | 11 +++++++++++ src/controllers/krate/metadata.rs | 11 ++++++++++- src/tests/util/test_app.rs | 1 + 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 9353f488b23..c77a8a06c98 100644 --- a/src/config.rs +++ b/src/config.rs @@ -25,6 +25,7 @@ pub struct Server { pub blocked_traffic: Vec<(String, Vec)>, pub max_allowed_page_offset: u32, pub page_offset_ua_blocklist: Vec, + pub excluded_crate_ids: Vec, pub domain_name: String, pub allowed_origins: Vec, pub downloads_persist_interval_ms: usize, @@ -84,6 +85,15 @@ impl Default for Server { Some(s) => s.split(',').map(String::from).collect(), }; let base = Base::from_environment(); + let excluded_crate_ids: Vec = match env_optional::("EXCLUDED_CRATE_IDS") { + None => vec![], + Some(s) if s.is_empty() => vec![], + Some(s) => s + .split(',') + .map(|n| n.parse()) + .collect::, _>>() + .unwrap_or_default(), + }; Server { db: DatabasePools::full_from_environment(&base), base, @@ -97,6 +107,7 @@ impl Default for Server { blocked_traffic: blocked_traffic(), max_allowed_page_offset: env_optional("WEB_MAX_ALLOWED_PAGE_OFFSET").unwrap_or(200), page_offset_ua_blocklist, + excluded_crate_ids, domain_name: domain_name(), allowed_origins, downloads_persist_interval_ms: dotenv::var("DOWNLOADS_PERSIST_INTERVAL_MS") diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 0aac33c30ee..226eac9fe98 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -24,6 +24,9 @@ use crate::models::krate::ALL_COLUMNS; /// Handles the `GET /summary` route. pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { use crate::schema::crates::dsl::*; + use diesel::dsl::any; + + let config = &req.app().config; let conn = req.db_read_only()?; let num_crates: i64 = crates.count().get_result(&*conn)?; @@ -77,8 +80,14 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { .limit(10) .load(&*conn)?; - let most_recently_downloaded = crates + let mut most_recently_downloaded_query = crates .inner_join(recent_crate_downloads::table) + .into_boxed(); + if !config.excluded_crate_ids.is_empty() { + most_recently_downloaded_query = most_recently_downloaded_query + .filter(recent_crate_downloads::crate_id.ne(any(&config.excluded_crate_ids))); + } + let most_recently_downloaded = most_recently_downloaded_query .then_order_by(recent_crate_downloads::downloads.desc()) .select(selection) .limit(10) diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index b6ae2c48469..ccdbf3d0780 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -300,6 +300,7 @@ fn simple_config() -> config::Server { blocked_traffic: Default::default(), max_allowed_page_offset: 200, page_offset_ua_blocklist: vec![], + excluded_crate_ids: vec![], domain_name: "crates.io".into(), allowed_origins: Vec::new(), downloads_persist_interval_ms: 1000, From 17bc3b84a57b569a4686cadf02a27f751288c7ef Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 18 Mar 2022 14:10:58 -0400 Subject: [PATCH 2/3] Switch to excluding by crate name instead --- src/config.rs | 12 ++++------ src/controllers/krate/metadata.rs | 8 +++---- src/tests/krate/summary.rs | 39 +++++++++++++++++++++++++++++++ src/tests/util/test_app.rs | 2 +- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/config.rs b/src/config.rs index c77a8a06c98..dab909863aa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -25,7 +25,7 @@ pub struct Server { pub blocked_traffic: Vec<(String, Vec)>, pub max_allowed_page_offset: u32, pub page_offset_ua_blocklist: Vec, - pub excluded_crate_ids: Vec, + pub excluded_crate_names: Vec, pub domain_name: String, pub allowed_origins: Vec, pub downloads_persist_interval_ms: usize, @@ -85,14 +85,10 @@ impl Default for Server { Some(s) => s.split(',').map(String::from).collect(), }; let base = Base::from_environment(); - let excluded_crate_ids: Vec = match env_optional::("EXCLUDED_CRATE_IDS") { + let excluded_crate_names = match env_optional::("EXCLUDED_CRATE_NAMES") { None => vec![], Some(s) if s.is_empty() => vec![], - Some(s) => s - .split(',') - .map(|n| n.parse()) - .collect::, _>>() - .unwrap_or_default(), + Some(s) => s.split(',').map(String::from).collect(), }; Server { db: DatabasePools::full_from_environment(&base), @@ -107,7 +103,7 @@ impl Default for Server { blocked_traffic: blocked_traffic(), max_allowed_page_offset: env_optional("WEB_MAX_ALLOWED_PAGE_OFFSET").unwrap_or(200), page_offset_ua_blocklist, - excluded_crate_ids, + excluded_crate_names, domain_name: domain_name(), allowed_origins, downloads_persist_interval_ms: dotenv::var("DOWNLOADS_PERSIST_INTERVAL_MS") diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 226eac9fe98..375ed790fc9 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -24,7 +24,7 @@ use crate::models::krate::ALL_COLUMNS; /// Handles the `GET /summary` route. pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { use crate::schema::crates::dsl::*; - use diesel::dsl::any; + use diesel::dsl::all; let config = &req.app().config; @@ -83,9 +83,9 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { let mut most_recently_downloaded_query = crates .inner_join(recent_crate_downloads::table) .into_boxed(); - if !config.excluded_crate_ids.is_empty() { - most_recently_downloaded_query = most_recently_downloaded_query - .filter(recent_crate_downloads::crate_id.ne(any(&config.excluded_crate_ids))); + if !config.excluded_crate_names.is_empty() { + most_recently_downloaded_query = + most_recently_downloaded_query.filter(name.ne(all(&config.excluded_crate_names))); } let most_recently_downloaded = most_recently_downloaded_query .then_order_by(recent_crate_downloads::downloads.desc()) diff --git a/src/tests/krate/summary.rs b/src/tests/krate/summary.rs index 8eb8c0078e1..76d56b4a335 100644 --- a/src/tests/krate/summary.rs +++ b/src/tests/krate/summary.rs @@ -107,3 +107,42 @@ fn summary_new_crates() { assert_eq!(json.new_crates.len(), 5); } + +#[test] +fn excluded_crate_id() { + let (app, anon, user) = TestApp::init() + .with_config(|config| { + config.excluded_crate_names = vec![ + "most_recent_downloads".into(), + // make sure no error occurs with a crate name that doesn't exist and that the name + // matches are exact, not substrings + "downloads".into(), + ]; + }) + .with_user(); + let user = user.as_model(); + app.db(|conn| { + CrateBuilder::new("some_downloads", user.id) + .version(VersionBuilder::new("0.1.0")) + .description("description") + .keyword("popular") + .category("cat1") + .downloads(20) + .recent_downloads(10) + .expect_build(conn); + + CrateBuilder::new("most_recent_downloads", user.id) + .version(VersionBuilder::new("0.2.0")) + .keyword("popular") + .category("cat1") + .downloads(5000) + .recent_downloads(50) + .expect_build(conn); + }); + + let json: SummaryResponse = anon.get("/api/v1/summary").good(); + + assert_eq!(json.most_recently_downloaded.len(), 1); + assert_eq!(json.most_recently_downloaded[0].name, "some_downloads"); + assert_eq!(json.most_recently_downloaded[0].recent_downloads, Some(10)); +} diff --git a/src/tests/util/test_app.rs b/src/tests/util/test_app.rs index ccdbf3d0780..8c3b6f58dd8 100644 --- a/src/tests/util/test_app.rs +++ b/src/tests/util/test_app.rs @@ -300,7 +300,7 @@ fn simple_config() -> config::Server { blocked_traffic: Default::default(), max_allowed_page_offset: 200, page_offset_ua_blocklist: vec![], - excluded_crate_ids: vec![], + excluded_crate_names: vec![], domain_name: "crates.io".into(), allowed_origins: Vec::new(), downloads_persist_interval_ms: 1000, From bf912ecfe022a96e7eca2bff2e786d2461db5a0e Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 18 Mar 2022 14:15:14 -0400 Subject: [PATCH 3/3] Exclude crates from all time downloaded list too --- src/controllers/krate/metadata.rs | 9 +++++++-- src/tests/krate/summary.rs | 4 ++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 375ed790fc9..284dad97746 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -73,8 +73,13 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { .select(selection) .limit(10) .load(&*conn)?; - let most_downloaded = crates - .left_join(recent_crate_downloads::table) + + let mut most_downloaded_query = crates.left_join(recent_crate_downloads::table).into_boxed(); + if !config.excluded_crate_names.is_empty() { + most_downloaded_query = + most_downloaded_query.filter(name.ne(all(&config.excluded_crate_names))); + } + let most_downloaded = most_downloaded_query .then_order_by(downloads.desc()) .select(selection) .limit(10) diff --git a/src/tests/krate/summary.rs b/src/tests/krate/summary.rs index 76d56b4a335..ed95f663acd 100644 --- a/src/tests/krate/summary.rs +++ b/src/tests/krate/summary.rs @@ -142,6 +142,10 @@ fn excluded_crate_id() { let json: SummaryResponse = anon.get("/api/v1/summary").good(); + assert_eq!(json.most_downloaded.len(), 1); + assert_eq!(json.most_downloaded[0].name, "some_downloads"); + assert_eq!(json.most_downloaded[0].downloads, 20); + assert_eq!(json.most_recently_downloaded.len(), 1); assert_eq!(json.most_recently_downloaded[0].name, "some_downloads"); assert_eq!(json.most_recently_downloaded[0].recent_downloads, Some(10));