From 63ac89a092f89f9d413ce876b8f6bdc4e9f8e50d Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Thu, 29 Feb 2024 20:20:20 +0100 Subject: [PATCH 1/2] fetch CrateDetails via MatchedRelease to save one SQL query per request --- ...53aa03ae0b38047e0e58c034a3d34f8e29631.json | 22 ---- src/web/crate_details.rs | 118 +++++++++--------- src/web/mod.rs | 11 +- src/web/rustdoc.rs | 33 ++--- 4 files changed, 78 insertions(+), 106 deletions(-) delete mode 100644 .sqlx/query-908827d37c731be4a6611eacd2e53aa03ae0b38047e0e58c034a3d34f8e29631.json diff --git a/.sqlx/query-908827d37c731be4a6611eacd2e53aa03ae0b38047e0e58c034a3d34f8e29631.json b/.sqlx/query-908827d37c731be4a6611eacd2e53aa03ae0b38047e0e58c034a3d34f8e29631.json deleted file mode 100644 index eb2800d85..000000000 --- a/.sqlx/query-908827d37c731be4a6611eacd2e53aa03ae0b38047e0e58c034a3d34f8e29631.json +++ /dev/null @@ -1,22 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT releases.rustdoc_status\n FROM releases\n WHERE releases.id = $1\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "rustdoc_status", - "type_info": "Bool" - } - ], - "parameters": { - "Left": [ - "Int4" - ] - }, - "nullable": [ - false - ] - }, - "hash": "908827d37c731be4a6611eacd2e53aa03ae0b38047e0e58c034a3d34f8e29631" -} diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 453746464..812d2bf64 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -9,7 +9,7 @@ use crate::{ encode_url_path, error::{AxumNope, AxumResult}, extractors::{DbConnection, Path}, - ReqVersion, + MatchedRelease, ReqVersion, }, AsyncStorage, }; @@ -98,11 +98,27 @@ pub struct Release { impl CrateDetails { #[tracing::instrument(skip(conn))] - pub async fn new( + pub(crate) async fn from_matched_release( + conn: &mut sqlx::PgConnection, + release: &MatchedRelease, + ) -> Result { + Ok(Self::new( + conn, + &release.name, + release.version(), + Some(release.req_version.clone()), + release.all_releases.clone(), + ) + .await? + .unwrap()) + } + + async fn new( conn: &mut sqlx::PgConnection, name: &str, version: &Version, req_version: Option, + prefetched_releases: Vec, ) -> Result, anyhow::Error> { let krate = match sqlx::query!( r#"SELECT @@ -163,9 +179,6 @@ impl CrateDetails { None => return Ok(None), }; - // get releases, sorted by semver - let releases = releases_for_crate(conn, krate.crate_id).await?; - let repository_metadata = krate.repo_host.map(|_| RepositoryMetadata { issues: krate.repo_issues.unwrap(), stars: krate.repo_stars.unwrap(), @@ -205,7 +218,7 @@ impl CrateDetails { keywords: krate.keywords, have_examples: krate.have_examples, target_name: krate.target_name, - releases, + releases: prefetched_releases, repository_metadata, metadata, is_library: krate.is_library, @@ -399,7 +412,7 @@ pub(crate) async fn crate_details_handler( ) })?; - let version = match_version(&mut conn, ¶ms.name, &req_version) + let matched_release = match_version(&mut conn, ¶ms.name, &req_version) .await? .assume_exact_name()? .into_canonical_req_version_or_else(|version| { @@ -407,13 +420,9 @@ pub(crate) async fn crate_details_handler( format!("/crate/{}/{}", ¶ms.name, version), CachePolicy::ForeverInCdn, ) - })? - .into_version(); + })?; - let mut details = - CrateDetails::new(&mut conn, ¶ms.name, &version, Some(req_version.clone())) - .await? - .ok_or(AxumNope::VersionNotFound)?; + let mut details = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; match details.fetch_readme(&storage).await { Ok(readme) => details.readme = readme.or(details.readme), @@ -670,11 +679,36 @@ mod tests { assert_cache_control, assert_redirect, assert_redirect_cached, async_wrapper, wrapper, TestDatabase, TestEnvironment, }; - use anyhow::{Context, Error}; + use anyhow::Error; use kuchikiki::traits::TendrilSink; use semver::Version; use std::collections::HashMap; + async fn crate_details( + conn: &mut sqlx::PgConnection, + name: &str, + version: &str, + req_version: Option, + ) -> CrateDetails { + let crate_id: i32 = sqlx::query_scalar!("SELECT id FROM crates WHERE name = $1", name) + .fetch_one(&mut *conn) + .await + .unwrap(); + + let releases = releases_for_crate(&mut *conn, crate_id).await.unwrap(); + + CrateDetails::new( + &mut *conn, + name, + &Version::parse(version).unwrap(), + req_version, + releases, + ) + .await + .unwrap() + .unwrap() + } + async fn assert_last_successful_build_equals( db: &TestDatabase, package: &str, @@ -682,11 +716,7 @@ mod tests { expected_last_successful_build: Option<&str>, ) -> Result<(), Error> { let mut conn = db.async_conn().await; - let details = - CrateDetails::new(&mut conn, package, &Version::parse(version).unwrap(), None) - .await - .with_context(|| anyhow::anyhow!("could not fetch crate details"))? - .unwrap(); + let details = crate_details(&mut conn, package, version, None).await; assert_eq!( details.last_successful_build, @@ -851,10 +881,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &"0.2.0".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", "0.2.0", None).await }); assert_eq!( @@ -972,10 +999,7 @@ mod tests { for version in &["0.0.1", "0.0.2", "0.0.3"] { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &version.parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", version, None).await }); assert_eq!( details.latest_release().unwrap().version, @@ -1002,10 +1026,7 @@ mod tests { for version in &["0.0.1", "0.0.2", "0.0.3-pre.1"] { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &version.parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", version, None).await }); assert_eq!( details.latest_release().unwrap().version, @@ -1033,10 +1054,7 @@ mod tests { for version in &["0.0.1", "0.0.2", "0.0.3"] { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &(version.parse().unwrap()), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", version, None).await }); assert_eq!( details.latest_release().unwrap().version, @@ -1072,10 +1090,7 @@ mod tests { for version in &["0.0.1", "0.0.2", "0.0.3"] { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &version.parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", version, None).await }); assert_eq!( details.latest_release().unwrap().version, @@ -1132,10 +1147,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &"0.0.1".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", "0.0.1", None).await }); assert_eq!( details.owners, @@ -1158,10 +1170,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &"0.0.1".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", "0.0.1", None).await }); let mut owners = details.owners; owners.sort(); @@ -1185,10 +1194,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &"0.0.1".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", "0.0.1", None).await }); assert_eq!( details.owners, @@ -1207,10 +1213,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = db.async_conn().await; - CrateDetails::new(&mut conn, "foo", &"0.0.1".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "foo", "0.0.1", None).await }); assert_eq!( details.owners, @@ -1654,10 +1657,7 @@ mod tests { let details = env.runtime().block_on(async move { let mut conn = env.async_db().await.async_conn().await; - CrateDetails::new(&mut conn, "dummy", &"0.5.0".parse().unwrap(), None) - .await - .unwrap() - .unwrap() + crate_details(&mut conn, "dummy", "0.5.0", None).await }); assert!(matches!( env.runtime() diff --git a/src/web/mod.rs b/src/web/mod.rs index 8533cd76b..0e94290b5 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -111,7 +111,7 @@ impl FromStr for ReqVersion { } #[derive(Debug)] -struct MatchedRelease { +pub(crate) struct MatchedRelease { /// crate name pub name: String, @@ -125,6 +125,9 @@ struct MatchedRelease { /// the matched release pub release: crate_details::Release, + + /// all releases since we have them anyways and so we can pass them to CrateDetails + all_releases: Vec, } impl MatchedRelease { @@ -274,6 +277,7 @@ async fn match_version( corrected_name, req_version: input_version.clone(), release: release.clone(), + all_releases: releases, }); } @@ -302,6 +306,7 @@ async fn match_version( corrected_name, req_version: input_version.clone(), release: release.clone(), + all_releases: releases, }); } @@ -311,11 +316,13 @@ async fn match_version( if req_semver == VersionReq::STAR { return releases .first() + .cloned() .map(|release| MatchedRelease { name: name.to_owned(), corrected_name: corrected_name.clone(), req_version: input_version.clone(), - release: release.clone(), + release, + all_releases: releases, }) .ok_or(AxumNope::VersionNotFound); } diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index bec925b3e..be6fed59b 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -180,12 +180,9 @@ pub(crate) async fn rustdoc_redirector_handler( rendering_time.step("serve JS for crate"); return async { - let version = matched_release.into_version(); + let krate = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; - let krate = - CrateDetails::new(&mut conn, &crate_name, &version, params.version.clone()) - .await? - .ok_or(AxumNope::ResourceNotFound)?; + let version = matched_release.into_version(); rendering_time.step("fetch from storage"); @@ -409,7 +406,7 @@ pub(crate) async fn rustdoc_html_server_handler( // * If both the name and the version are an exact match, return the version of the crate. // * If there is an exact match, but the requested crate name was corrected (dashes vs. underscores), redirect to the corrected name. // * If there is a semver (but not exact) match, redirect to the exact version. - let version = match_version(&mut conn, ¶ms.name, ¶ms.version) + let matched_release = match_version(&mut conn, ¶ms.name, ¶ms.version) .await? .into_exactly_named_or_else(|corrected_name, req_version| { AxumNope::Redirect( @@ -432,22 +429,14 @@ pub(crate) async fn rustdoc_html_server_handler( )), CachePolicy::ForeverInCdn, ) - })? - .into_version(); + })?; trace!("crate details"); rendering_time.step("crate details"); // Get the crate's details from the database - // NOTE: we know this crate must exist because we just checked it above (or else `match_version` is buggy) - let krate = CrateDetails::new( - &mut conn, - ¶ms.name, - &version, - Some(params.version.clone()), - ) - .await? - .ok_or(AxumNope::ResourceNotFound)?; + let krate = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; + let version = matched_release.into_version(); if !krate.rustdoc_status { rendering_time.step("redirect to crate"); @@ -733,14 +722,12 @@ pub(crate) async fn target_redirect_handler( mut conn: DbConnection, Extension(storage): Extension>, ) -> AxumResult { - let version = match_version(&mut conn, &name, &req_version) + let matched_release = match_version(&mut conn, &name, &req_version) .await? - .into_canonical_req_version_or_else(|_| AxumNope::VersionNotFound)? - .into_version(); + .into_canonical_req_version_or_else(|_| AxumNope::VersionNotFound)?; - let crate_details = CrateDetails::new(&mut conn, &name, &version, Some(req_version.clone())) - .await? - .ok_or(AxumNope::VersionNotFound)?; + let crate_details = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; + let version = matched_release.into_version(); // We're trying to find the storage location // for the requested path in the target-redirect. From 2b99b0346dd721a13abe856451b45db23c38d739 Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Fri, 1 Mar 2024 13:06:18 +0100 Subject: [PATCH 2/2] take param by value in CrateDetails::from_matched_release --- src/web/crate_details.rs | 12 ++++++------ src/web/rustdoc.rs | 33 +++++++++++++++++---------------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 812d2bf64..5d71671ef 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -32,7 +32,7 @@ use std::sync::Arc; #[derive(Debug, Clone, PartialEq, Serialize)] pub struct CrateDetails { name: String, - version: Version, + pub version: Version, description: Option, owners: Vec<(String, String)>, dependencies: Option, @@ -100,14 +100,14 @@ impl CrateDetails { #[tracing::instrument(skip(conn))] pub(crate) async fn from_matched_release( conn: &mut sqlx::PgConnection, - release: &MatchedRelease, + release: MatchedRelease, ) -> Result { Ok(Self::new( conn, &release.name, - release.version(), - Some(release.req_version.clone()), - release.all_releases.clone(), + &release.release.version, + Some(release.req_version), + release.all_releases, ) .await? .unwrap()) @@ -422,7 +422,7 @@ pub(crate) async fn crate_details_handler( ) })?; - let mut details = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; + let mut details = CrateDetails::from_matched_release(&mut conn, matched_release).await?; match details.fetch_readme(&storage).await { Ok(readme) => details.readme = readme.or(details.readme), diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index be6fed59b..b44b55ae1 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -180,16 +180,14 @@ pub(crate) async fn rustdoc_redirector_handler( rendering_time.step("serve JS for crate"); return async { - let krate = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; - - let version = matched_release.into_version(); + let krate = CrateDetails::from_matched_release(&mut conn, matched_release).await?; rendering_time.step("fetch from storage"); match storage .fetch_rustdoc_file( &crate_name, - &version.to_string(), + &krate.version.to_string(), krate.latest_build_id.unwrap_or(0), target, krate.archive_storage, @@ -435,8 +433,7 @@ pub(crate) async fn rustdoc_html_server_handler( rendering_time.step("crate details"); // Get the crate's details from the database - let krate = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; - let version = matched_release.into_version(); + let krate = CrateDetails::from_matched_release(&mut conn, matched_release).await?; if !krate.rustdoc_status { rendering_time.step("redirect to crate"); @@ -452,7 +449,7 @@ pub(crate) async fn rustdoc_html_server_handler( if req_path.first().copied() == Some(&krate.metadata.default_target) { return redirect( ¶ms.name, - &version, + &krate.version, &req_path[1..], CachePolicy::ForeverInCdn, ); @@ -476,7 +473,7 @@ pub(crate) async fn rustdoc_html_server_handler( let blob = match storage .fetch_rustdoc_file( ¶ms.name, - &version.to_string(), + &krate.version.to_string(), krate.latest_build_id.unwrap_or(0), &storage_path, krate.archive_storage, @@ -498,14 +495,19 @@ pub(crate) async fn rustdoc_html_server_handler( return if storage .rustdoc_file_exists( ¶ms.name, - &version.to_string(), + &krate.version.to_string(), krate.latest_build_id.unwrap_or(0), &storage_path, krate.archive_storage, ) .await? { - redirect(¶ms.name, &version, &req_path, CachePolicy::ForeverInCdn) + redirect( + ¶ms.name, + &krate.version, + &req_path, + CachePolicy::ForeverInCdn, + ) } else if req_path.first().map_or(false, |p| p.contains('-')) { // This is a target, not a module; it may not have been built. // Redirect to the default target and show a search page instead of a hard 404. @@ -523,7 +525,7 @@ pub(crate) async fn rustdoc_html_server_handler( if storage_path == format!("{}/index.html", krate.target_name) { error!( krate = params.name, - version = version.to_string(), + version = krate.version.to_string(), original_path = original_path.as_ref(), storage_path, "Couldn't find crate documentation root on storage. @@ -553,8 +555,8 @@ pub(crate) async fn rustdoc_html_server_handler( // Get the latest version of the crate let latest_version = latest_release.version.clone(); - let is_latest_version = latest_version == version; - let is_prerelease = !(version.pre.is_empty()); + let is_latest_version = latest_version == krate.version; + let is_prerelease = !(krate.version.pre.is_empty()); // The path within this crate version's rustdoc output let (target, inner_path) = { @@ -726,8 +728,7 @@ pub(crate) async fn target_redirect_handler( .await? .into_canonical_req_version_or_else(|_| AxumNope::VersionNotFound)?; - let crate_details = CrateDetails::from_matched_release(&mut conn, &matched_release).await?; - let version = matched_release.into_version(); + let crate_details = CrateDetails::from_matched_release(&mut conn, matched_release).await?; // We're trying to find the storage location // for the requested path in the target-redirect. @@ -757,7 +758,7 @@ pub(crate) async fn target_redirect_handler( let (redirect_path, query_args) = if storage .rustdoc_file_exists( &name, - &version.to_string(), + &crate_details.version.to_string(), crate_details.latest_build_id.unwrap_or(0), &storage_location_for_path, crate_details.archive_storage,