migrate crate-details handler to axum

syphar · syphar · commit 0eb570daa2c3 · 2022-12-01T18:14:49.000+01:00
diff --git a/src/utils/mod.rs b/src/utils/mod.rs
@@ -107,9 +107,27 @@ where
     F: FnOnce() -> Result<R> + Send + 'static,
     R: Send + 'static,
 {
-    tokio::task::spawn_blocking(f)
+    let result = tokio::task::spawn_blocking(f)
         .await
-        .context("failed to join thread")?
+        .context("failed to join thread")?;
+
+    if cfg!(debug_assertions) {
+        // Since `AxumNope` can also be silently converted into
+        // `anyhow::Error`, someone accidentally using this method
+        // instead of `web::error::spawn_blocking_web` can lead to
+        // a more specific AxumNope error with a specific status code
+        // being silently converted into an anyhow::Error, so a server
+        // error with status 500.
+        if let Err(ref err) = result {
+            use crate::web::error::AxumNope;
+            assert!(
+                err.downcast_ref::<AxumNope>().is_none(),
+                "AxumNope found inside anyhow::Error, this is likely a problem."
+            );
+        }
+    }
+
+    result
 }
 
 #[cfg(test)]
diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs
@@ -1,19 +1,26 @@
-use super::{markdown, match_version, redirect_base, MatchSemver, MetaData};
-use crate::utils::{get_correct_docsrs_style_file, report_error};
+use super::error::spawn_blocking_web;
+use super::{markdown, match_version, MatchSemver, MetaData};
+use crate::utils::{get_correct_docsrs_style_file, report_error, spawn_blocking};
 use crate::{
     db::Pool,
-    impl_webpage,
+    impl_axum_webpage,
     repositories::RepositoryStatsUpdater,
-    web::{cache::CachePolicy, page::WebPage},
+    web::{
+        cache::CachePolicy,
+        error::{AxumNope, AxumResult},
+    },
+};
+use anyhow::{anyhow, Context as _};
+use axum::{
+    extract::{Extension, Path},
+    response::{IntoResponse, Response as AxumResponse},
 };
-use anyhow::anyhow;
 use chrono::{DateTime, Utc};
-use iron::prelude::*;
-use iron::Url;
 use postgres::GenericClient;
-use router::Router;
+use serde::Deserialize;
 use serde::{ser::Serializer, Serialize};
 use serde_json::Value;
+use std::sync::Arc;
 
 // TODO: Add target name and versions
 
@@ -293,75 +300,85 @@ struct CrateDetailsPage {
     details: CrateDetails,
 }
 
-impl_webpage! {
+impl_axum_webpage! {
     CrateDetailsPage = "crate/details.html",
 }
 
-pub fn crate_details_handler(req: &mut Request) -> IronResult<Response> {
-    let router = extension!(req, Router);
-    // this handler must always called with a crate name
-    let name = cexpect!(req, router.find("name"));
-    let req_version = router.find("version");
+#[derive(Deserialize, Clone, Debug)]
+pub(crate) struct CrateDetailHandlerParams {
+    name: String,
+    version: Option<String>,
+}
 
-    if req_version.is_none() {
-        let url = ctry!(
-            req,
-            Url::parse(&format!("{}/crate/{}/latest", redirect_base(req), name,)),
-        );
-        return Ok(super::cached_redirect(url, CachePolicy::ForeverInCdn));
+#[tracing::instrument]
+pub(crate) async fn crate_details_handler(
+    Path(params): Path<CrateDetailHandlerParams>,
+    Extension(pool): Extension<Pool>,
+    Extension(repository_stats_updater): Extension<Arc<RepositoryStatsUpdater>>,
+) -> AxumResult<AxumResponse> {
+    // this handler must always called with a crate name
+    if params.version.is_none() {
+        return Ok(super::axum_cached_redirect(
+            &format!("/crate/{}/latest", params.name),
+            CachePolicy::ForeverInCdn,
+        )
+        .into_response());
     }
 
-    let mut conn = extension!(req, Pool).get()?;
+    let found_version = spawn_blocking_web({
+        let pool = pool.clone();
+        let params = params.clone();
+        move || {
+            let mut conn = pool.get().context("could not get connection from pool")?;
+            match_version(&mut conn, &params.name, params.version.as_deref())
+                .and_then(|m| m.assume_exact())
+                .map_err(Into::<AxumNope>::into)
+        }
+    })
+    .await?;
 
-    let found_version =
-        match_version(&mut conn, name, req_version).and_then(|m| m.assume_exact())?;
     let (version, version_or_latest, is_latest_url) = match found_version {
         MatchSemver::Exact((version, _)) => (version.clone(), version, false),
         MatchSemver::Latest((version, _)) => (version, "latest".to_string(), true),
         MatchSemver::Semver((version, _)) => {
-            let url = ctry!(
-                req,
-                Url::parse(&format!(
-                    "{}/crate/{}/{}",
-                    redirect_base(req),
-                    name,
-                    version
-                )),
-            );
-
-            return Ok(super::cached_redirect(url, CachePolicy::ForeverInCdn));
+            return Ok(super::axum_cached_redirect(
+                &format!("/crate/{}/{}", &params.name, version),
+                CachePolicy::ForeverInCdn,
+            )
+            .into_response());
         }
     };
 
-    let updater = extension!(req, RepositoryStatsUpdater);
-    let details = cexpect!(
-        req,
-        ctry!(
-            req,
-            CrateDetails::new(
-                &mut *conn,
-                name,
-                &version,
-                &version_or_latest,
-                Some(updater)
-            )
+    let details = spawn_blocking(move || {
+        let mut conn = pool.get()?;
+        CrateDetails::new(
+            &mut *conn,
+            &params.name,
+            &version,
+            &version_or_latest,
+            Some(&repository_stats_updater),
         )
-    );
-
-    let mut res = CrateDetailsPage { details }.into_response(req)?;
-    res.extensions.insert::<CachePolicy>(if is_latest_url {
-        CachePolicy::ForeverInCdn
-    } else {
-        CachePolicy::ForeverInCdnAndStaleInBrowser
-    });
-    Ok(res)
+    })
+    .await?
+    .ok_or(AxumNope::VersionNotFound)?;
+
+    let mut res = CrateDetailsPage { details }.into_response();
+    res.extensions_mut()
+        .insert::<CachePolicy>(if is_latest_url {
+            CachePolicy::ForeverInCdn
+        } else {
+            CachePolicy::ForeverInCdnAndStaleInBrowser
+        });
+    Ok(res.into_response())
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
     use crate::index::api::CrateOwner;
-    use crate::test::{assert_cache_control, assert_redirect_cached, wrapper, TestDatabase};
+    use crate::test::{
+        assert_cache_control, assert_redirect, assert_redirect_cached, wrapper, TestDatabase,
+    };
     use anyhow::{Context, Error};
     use kuchiki::traits::TendrilSink;
     use std::collections::HashMap;
@@ -1035,13 +1052,7 @@ mod tests {
             assert!(body.contains("<a href=\"/crate/dummy/latest/source/\""));
             assert!(body.contains("<a href=\"/crate/dummy/latest\""));
 
-            assert_redirect_cached(
-                "/crate/dummy/latest/",
-                "/crate/dummy/latest",
-                CachePolicy::NoCaching,
-                web,
-                &env.config(),
-            )?;
+            assert_redirect("/crate/dummy/latest/", "/crate/dummy/latest", web)?;
             assert_redirect_cached(
                 "/crate/dummy",
                 "/crate/dummy/latest",
diff --git a/src/web/error.rs b/src/web/error.rs
@@ -4,6 +4,7 @@ use crate::{
     db::PoolError,
     web::{page::WebPage, releases::Search, AxumErrorPage, ErrorPage},
 };
+use anyhow::Context as _;
 use axum::{
     http::StatusCode,
     response::{IntoResponse, Response as AxumResponse},
@@ -234,8 +235,60 @@ impl IntoResponse for AxumNope {
     }
 }
 
+impl From<Nope> for AxumNope {
+    fn from(err: Nope) -> Self {
+        match err {
+            Nope::ResourceNotFound => AxumNope::ResourceNotFound,
+            Nope::BuildNotFound => AxumNope::BuildNotFound,
+            Nope::CrateNotFound => AxumNope::CrateNotFound,
+            Nope::OwnerNotFound => AxumNope::OwnerNotFound,
+            Nope::VersionNotFound => AxumNope::VersionNotFound,
+            Nope::NoResults => todo!(),
+            Nope::InternalServerError => AxumNope::InternalServerError,
+        }
+    }
+}
+
 pub(crate) type AxumResult<T> = Result<T, AxumNope>;
 
+/// spawn-blocking helper for usage in axum requests.
+///
+/// Should be used when spawning tasks from inside web-handlers
+/// that return AxumNope as error. The join-error will also be
+/// converted into an `anyhow::Error`. Any standard AxumNope
+/// will be left intact so the error-handling can generate the
+/// correct status-page with the correct status code.
+///
+/// Examples:
+///
+/// with standard `tokio::task::spawn_blocking`:
+/// ```ignore
+/// let data = spawn_blocking(move || -> Result<_, AxumNope> {
+///     let data = get_the_data()?;
+///     Ok(data)
+/// })
+/// .await
+/// .context("failed to join thread")??;
+/// ```
+///
+/// with this helper function:
+/// ```ignore
+/// let data = spawn_blocking(move || {
+///     let data = get_the_data()?;
+///     Ok(data)
+/// })
+/// .await?
+/// ```
+pub(crate) async fn spawn_blocking_web<F, R>(f: F) -> AxumResult<R>
+where
+    F: FnOnce() -> AxumResult<R> + Send + 'static,
+    R: Send + 'static,
+{
+    tokio::task::spawn_blocking(f)
+        .await
+        .context("failed to join thread")?
+}
+
 #[cfg(test)]
 mod tests {
     use crate::test::wrapper;
diff --git a/src/web/mod.rs b/src/web/mod.rs
@@ -77,7 +77,7 @@ mod builds;
 pub(crate) mod cache;
 pub(crate) mod crate_details;
 mod csp;
-mod error;
+pub(crate) mod error;
 mod extensions;
 mod features;
 mod file;
@@ -97,7 +97,9 @@ use anyhow::Error;
 use axum::{
     extract::Extension,
     http::{uri::Authority, StatusCode},
-    middleware, Router as AxumRouter,
+    middleware,
+    response::IntoResponse,
+    Router as AxumRouter,
 };
 use chrono::{DateTime, Utc};
 use csp::CspMiddleware;
@@ -327,7 +329,7 @@ fn match_version(
                  WHERE normalize_crate_name(name) = normalize_crate_name($1)",
                 &[&name],
             )
-            .unwrap();
+            .unwrap(); // FIXME: remove this unwrap when all handlers using it are migrated to axum
 
         let row = rows.get(0).ok_or(Nope::CrateNotFound)?;
 
@@ -443,6 +445,7 @@ pub(crate) fn build_axum_app(
             .layer(Extension(context.metrics()?))
             .layer(Extension(context.config()?))
             .layer(Extension(context.storage()?))
+            .layer(Extension(context.repository_stats_updater()?))
             .layer(Extension(template_data))
             .layer(middleware::from_fn(csp::csp_middleware))
             .layer(middleware::from_fn(
@@ -536,12 +539,28 @@ fn redirect(url: Url) -> Response {
     resp
 }
 
+fn axum_redirect(url: &str) -> impl IntoResponse {
+    (
+        StatusCode::FOUND,
+        [(
+            http::header::LOCATION,
+            http::HeaderValue::try_from(url).expect("invalid url for redirect"),
+        )],
+    )
+}
+
 fn cached_redirect(url: Url, cache_policy: cache::CachePolicy) -> Response {
     let mut resp = Response::with((status::Found, Redirect(url)));
     resp.extensions.insert::<cache::CachePolicy>(cache_policy);
     resp
 }
 
+fn axum_cached_redirect(url: &str, cache_policy: cache::CachePolicy) -> impl IntoResponse {
+    let mut resp = axum_redirect(url).into_response();
+    resp.extensions_mut().insert(cache_policy);
+    resp
+}
+
 fn redirect_base(req: &Request) -> String {
     // Try to get the scheme from CloudFront first, and then from iron
     let scheme = req
diff --git a/src/web/routes.rs b/src/web/routes.rs
@@ -105,6 +105,14 @@ pub(super) fn build_axum_routes() -> AxumRouter {
             "/releases/failures/:page",
             get_internal(super::releases::releases_failures_by_stars_handler),
         )
+        .route(
+            "/crate/:name",
+            get_internal(super::crate_details::crate_details_handler),
+        )
+        .route(
+            "/crate/:name/:version",
+            get_internal(super::crate_details::crate_details_handler),
+        )
 }
 
 // REFACTOR: Break this into smaller initialization functions
@@ -152,11 +160,6 @@ pub(super) fn build_routes() -> Routes {
     routes.internal_page("/releases/search", super::releases::search_handler);
     routes.internal_page("/releases/queue", super::releases::build_queue_handler);
 
-    routes.internal_page("/crate/:name", super::crate_details::crate_details_handler);
-    routes.internal_page(
-        "/crate/:name/:version",
-        super::crate_details::crate_details_handler,
-    );
     routes.internal_page(
         "/crate/:name/:version/builds",
         super::builds::build_list_handler,
