From 3c4d3ccd9171113dd050a5a4ab3de8e771b9e157 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 12 Nov 2024 15:02:49 +0100 Subject: [PATCH 1/2] admin/delete_crate: Simplify SQL query code by deriving `Queryable` and `Selectable` --- src/bin/crates-admin/delete_crate.rs | 106 +++++++++++++-------------- 1 file changed, 51 insertions(+), 55 deletions(-) diff --git a/src/bin/crates-admin/delete_crate.rs b/src/bin/crates-admin/delete_crate.rs index b0a3658e844..091d329a126 100644 --- a/src/bin/crates-admin/delete_crate.rs +++ b/src/bin/crates-admin/delete_crate.rs @@ -6,8 +6,9 @@ use crates_io::worker::jobs; use crates_io::{db, schema::crates}; use crates_io_worker::BackgroundJob; use diesel::dsl::sql; +use diesel::expression::SqlLiteral; +use diesel::prelude::*; use diesel::sql_types::{Array, BigInt, Text}; -use diesel::{ExpressionMethods, QueryDsl}; use diesel_async::RunQueryDsl; use futures_util::TryStreamExt; use std::collections::HashMap; @@ -40,50 +41,14 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { let existing_crates = crates::table .inner_join(crate_downloads::table) .filter(crates::name.eq_any(&crate_names)) - .select(( - crates::name, - crates::id, - crate_downloads::downloads, - sql::>( - r#" - ARRAY( - SELECT - CASE WHEN crate_owners.owner_kind = 1 THEN - teams.login - ELSE - users.gh_login - END - FROM crate_owners - LEFT JOIN teams ON teams.id = crate_owners.owner_id - LEFT JOIN users ON users.id = crate_owners.owner_id - WHERE crate_owners.crate_id = crates.id - ) - "#, - ), - sql::( - // This is an incorrect reverse dependencies query, since it - // includes the `dependencies` rows for all versions, not just - // the "default version" per crate. However, it's good enough - // for our purposes here. - r#" - ( - SELECT COUNT(*) - FROM dependencies - WHERE dependencies.crate_id = crates.id - ) - "#, - ), - )) - .load_stream::<(String, i32, i64, Vec, i64)>(&mut conn) + .select(CrateInfo::as_select()) + .load_stream::(&mut conn) .await .context("Failed to look up crate name from the database")? - .try_fold( - HashMap::new(), - |mut map, (name, id, downloads, owners, rev_deps)| { - map.insert(name, CrateInfo::new(id, downloads, owners, rev_deps)); - futures_util::future::ready(Ok(map)) - }, - ) + .try_fold(HashMap::new(), |mut map, info| { + map.insert(info.name.clone(), info); + futures_util::future::ready(Ok(map)) + }) .await?; println!("Deleting the following crates:"); @@ -138,25 +103,20 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { Ok(()) } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Queryable, Selectable)] struct CrateInfo { + #[diesel(select_expression = crates::columns::name)] + name: String, + #[diesel(select_expression = crates::columns::id)] id: i32, + #[diesel(select_expression = crate_downloads::columns::downloads)] downloads: i64, + #[diesel(select_expression = owners_subquery())] owners: Vec, + #[diesel(select_expression = rev_deps_subquery())] rev_deps: i64, } -impl CrateInfo { - pub fn new(id: i32, downloads: i64, owners: Vec, rev_deps: i64) -> Self { - Self { - id, - downloads, - owners, - rev_deps, - } - } -} - impl Display for CrateInfo { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let id = self.id; @@ -175,3 +135,39 @@ impl Display for CrateInfo { Ok(()) } } + +/// A subquery that returns the owners of a crate as an array of strings. +#[diesel::dsl::auto_type] +fn owners_subquery() -> SqlLiteral> { + sql(r#" + ARRAY( + SELECT + CASE WHEN crate_owners.owner_kind = 1 THEN + teams.login + ELSE + users.gh_login + END + FROM crate_owners + LEFT JOIN teams ON teams.id = crate_owners.owner_id + LEFT JOIN users ON users.id = crate_owners.owner_id + WHERE crate_owners.crate_id = crates.id + ) + "#) +} + +/// A subquery that returns the number of reverse dependencies of a crate. +/// +/// **Warning:** this is an incorrect reverse dependencies query, since it +/// includes the `dependencies` rows for all versions, not just the +/// "default version" per crate. However, it's good enough for our +/// purposes here. +#[diesel::dsl::auto_type] +fn rev_deps_subquery() -> SqlLiteral { + sql(r#" + ( + SELECT COUNT(*) + FROM dependencies + WHERE dependencies.crate_id = crates.id + ) + "#) +} From 3aeff80ff79416938d9b5037be01adfb68605243 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 12 Nov 2024 15:09:37 +0100 Subject: [PATCH 2/2] admin/delete_crate: Remove unnecessary `HashMap` The number of deleted crates is usually no more than a dozen or two, so the extra complexity from the `HashMap` isn't really worth it here. --- src/bin/crates-admin/delete_crate.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/bin/crates-admin/delete_crate.rs b/src/bin/crates-admin/delete_crate.rs index 091d329a126..0eee655293d 100644 --- a/src/bin/crates-admin/delete_crate.rs +++ b/src/bin/crates-admin/delete_crate.rs @@ -10,8 +10,6 @@ use diesel::expression::SqlLiteral; use diesel::prelude::*; use diesel::sql_types::{Array, BigInt, Text}; use diesel_async::RunQueryDsl; -use futures_util::TryStreamExt; -use std::collections::HashMap; use std::fmt::Display; #[derive(clap::Parser, Debug)] @@ -42,19 +40,14 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { .inner_join(crate_downloads::table) .filter(crates::name.eq_any(&crate_names)) .select(CrateInfo::as_select()) - .load_stream::(&mut conn) + .load::(&mut conn) .await - .context("Failed to look up crate name from the database")? - .try_fold(HashMap::new(), |mut map, info| { - map.insert(info.name.clone(), info); - futures_util::future::ready(Ok(map)) - }) - .await?; + .context("Failed to look up crate name from the database")?; println!("Deleting the following crates:"); println!(); for name in &crate_names { - match existing_crates.get(name) { + match existing_crates.iter().find(|info| info.name == *name) { Some(info) => println!(" - {} ({info})", name.bold()), None => println!(" - {name} (⚠️ crate not found)"), } @@ -68,7 +61,7 @@ pub async fn run(opts: Opts) -> anyhow::Result<()> { } for name in &crate_names { - if let Some(crate_info) = existing_crates.get(name) { + if let Some(crate_info) = existing_crates.iter().find(|info| info.name == *name) { let id = crate_info.id; info!("{name}: Deleting crate from the database…");