From 9786da64dc7078d346650ccf876d8f3514c4abe0 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 17 Feb 2025 04:36:13 +0800 Subject: [PATCH 1/5] controllers/version: Load `Crate` and `Version` in single query Instead of loading them individually, this gets both in one query. I think that in most cases, the request to query for `Crate` and `Version` will have valid input, so this seems like a better approach, even though it might only give a small performance boost. --- src/controllers/version.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/controllers/version.rs b/src/controllers/version.rs index 8c8885e6cbb..d5dd4359dc6 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -7,14 +7,15 @@ pub mod update; pub mod yank; use axum::extract::{FromRequestParts, Path}; -use diesel_async::AsyncPgConnection; +use diesel::prelude::*; +use diesel_async::{AsyncPgConnection, RunQueryDsl}; use serde::de::Error; use serde::{Deserialize, Deserializer}; use utoipa::IntoParams; -use crate::controllers::krate::load_crate; use crate::models::{Crate, Version}; -use crate::util::errors::{AppResult, version_not_found}; +use crate::schema::{crates, versions}; +use crate::util::errors::{AppResult, crate_not_found, version_not_found}; #[derive(Deserialize, FromRequestParts, IntoParams)] #[into_params(parameter_in = Path)] @@ -46,12 +47,19 @@ async fn version_and_crate( crate_name: &str, semver: &str, ) -> AppResult<(Version, Crate)> { - let krate = load_crate(conn, crate_name).await?; - let version = krate - .find_version(conn, semver) - .await? - .ok_or_else(|| version_not_found(crate_name, semver))?; + let (krate, version) = Crate::by_name(crate_name) + .left_join( + versions::table.on(crates::id + .eq(versions::crate_id) + .and(versions::num.eq(semver))), + ) + .select(<(Crate, Option)>::as_select()) + .first(conn) + .await + .optional()? + .ok_or_else(|| crate_not_found(crate_name))?; + let version = version.ok_or_else(|| version_not_found(crate_name, semver))?; Ok((version, krate)) } From 2da492f90d98392b5aa7d66871d0009fce243b91 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 17 Feb 2025 19:04:35 +0800 Subject: [PATCH 2/5] controllers/version: Extract SQL query into a function for reusability --- src/controllers/version.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/controllers/version.rs b/src/controllers/version.rs index d5dd4359dc6..fa7bc5b8bb3 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -47,12 +47,9 @@ async fn version_and_crate( crate_name: &str, semver: &str, ) -> AppResult<(Version, Crate)> { - let (krate, version) = Crate::by_name(crate_name) - .left_join( - versions::table.on(crates::id - .eq(versions::crate_id) - .and(versions::num.eq(semver))), - ) + use ext::*; + + let (krate, version) = crate_and_version_query(crate_name, semver) .select(<(Crate, Option)>::as_select()) .first(conn) .await @@ -68,3 +65,19 @@ fn deserialize_version<'de, D: Deserializer<'de>>(deserializer: D) -> Result(crate_name: &'a str, semver: &'a str) -> _ { + crates::table + .left_join( + versions::table.on(crates::id + .eq(versions::crate_id) + .and(versions::num.eq(semver))), + ) + .filter(canon_crate_name(crates::name).eq(canon_crate_name(crate_name))) + } +} From 837c67ba12313b0c9c8837652c18eb4d9dae76e0 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 17 Feb 2025 19:05:20 +0800 Subject: [PATCH 3/5] controllers/version: Add the `CrateVersionPathExt` trait and provide `crate_and_version()` fn Since most endpoints don't need all of the columns for `crates` and `versions`, it would be great to only load the columns we actually need to make things faster. This will enable other endpoints to use the function to make a base query and customize the needed columns. --- src/controllers/version.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/controllers/version.rs b/src/controllers/version.rs index fa7bc5b8bb3..e0adb1d52bb 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -80,4 +80,14 @@ mod ext { ) .filter(canon_crate_name(crates::name).eq(canon_crate_name(crate_name))) } + + pub trait CrateVersionPathExt { + fn crate_and_version(&self) -> crate_and_version_query<'_>; + } + + impl CrateVersionPathExt for CrateVersionPath { + fn crate_and_version(&self) -> crate_and_version_query<'_> { + crate_and_version_query(&self.name, &self.version) + } + } } From d9c370934a74ec9b12a6104ba7ce213411b65c28 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 17 Feb 2025 19:07:08 +0800 Subject: [PATCH 4/5] controllers/version: Add a helper trait to simplify validation --- src/controllers/version.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/controllers/version.rs b/src/controllers/version.rs index e0adb1d52bb..e5900bc2af3 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -15,7 +15,7 @@ use utoipa::IntoParams; use crate::models::{Crate, Version}; use crate::schema::{crates, versions}; -use crate::util::errors::{AppResult, crate_not_found, version_not_found}; +use crate::util::errors::AppResult; #[derive(Deserialize, FromRequestParts, IntoParams)] #[into_params(parameter_in = Path)] @@ -54,9 +54,7 @@ async fn version_and_crate( .first(conn) .await .optional()? - .ok_or_else(|| crate_not_found(crate_name))?; - - let version = version.ok_or_else(|| version_not_found(crate_name, semver))?; + .gather(crate_name, semver)?; Ok((version, krate)) } @@ -68,6 +66,7 @@ fn deserialize_version<'de, D: Deserializer<'de>>(deserializer: D) -> Result { + fn gather(self, crate_name: &str, semver: &str) -> AppResult<(C, V)>; + fn gather_from_path(self, path: &CrateVersionPath) -> AppResult<(C, V)>; + } + + impl CrateVersionHelper for Option<(C, Option)> { + fn gather(self, crate_name: &str, semver: &str) -> AppResult<(C, V)> { + let (krate, version) = self.ok_or_else(|| crate_not_found(crate_name))?; + let version = version.ok_or_else(|| version_not_found(crate_name, semver))?; + Ok((krate, version)) + } + + fn gather_from_path(self, path: &CrateVersionPath) -> AppResult<(C, V)> { + self.gather(&path.name, &path.version) + } + } } From 102892993bccc5cde72bb51cd2e26ea2ee6ee0b3 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 17 Feb 2025 20:20:55 +0800 Subject: [PATCH 5/5] controllers/version: Avoid loading entire `Crate` within the `load_version()` fn --- src/controllers/version.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/controllers/version.rs b/src/controllers/version.rs index e5900bc2af3..2274b7aa73e 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -31,7 +31,16 @@ pub struct CrateVersionPath { impl CrateVersionPath { pub async fn load_version(&self, conn: &mut AsyncPgConnection) -> AppResult { - Ok(self.load_version_and_crate(conn).await?.0) + use ext::*; + + let (_, version) = self + .crate_and_version() + .select((crates::id, Option::::as_select())) + .first::<(i32, _)>(conn) + .await + .optional()? + .gather_from_path(self)?; + Ok(version) } pub async fn load_version_and_crate(