add krate-name validation to rustdoc parameter extraction

Author: syphar

src/db/types/krate_name.rs

@@ -6,10 +6,10 @@ use sqlx::{
     Decode, Encode, Postgres,
     encode::IsNull,
     error::BoxDynError,
-    postgres::{PgArgumentBuffer, PgTypeInfo, PgValueRef},
+    postgres::{PgArgumentBuffer, PgHasArrayType, PgTypeInfo, PgValueRef},
     prelude::*,
 };
-use std::{io::Write, str::FromStr};
+use std::{borrow::Cow, io::Write, str::FromStr};
 
 /// validated crate name
 ///
@@ -31,7 +31,14 @@ use std::{io::Write, str::FromStr};
     SerializeDisplay,
     bincode::Encode,
 )]
-pub struct KrateName(String);
+pub struct KrateName(Cow<'static, str>);
+
+impl KrateName {
+    #[cfg(test)]
+    pub(crate) const fn from_static(s: &'static str) -> Self {
+        KrateName(Cow::Borrowed(s))
+    }
+}
 
 impl From<&KrateName> for KrateName {
     fn from(krate_name: &KrateName) -> Self {
@@ -44,7 +51,7 @@ impl FromStr for KrateName {
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         validate_crate_name("crate", s)?;
-        Ok(KrateName(s.to_string()))
+        Ok(KrateName(Cow::Owned(s.to_string())))
     }
 }
 
@@ -67,8 +74,21 @@ impl<'q> Encode<'q, Postgres> for KrateName {
 
 impl<'r> Decode<'r, Postgres> for KrateName {
     fn decode(value: PgValueRef<'r>) -> Result<Self, BoxDynError> {
+        // future improvement: we could also avoid the allocation here
+        // and return a Cow::Borrowed while reading from the DB.
+        // But this would mean the lifetime leaks into the whole codebase.
         let s: &str = Decode::<Postgres>::decode(value)?;
-        Ok(Self(s.parse()?))
+        Ok(Self(Cow::Owned(s.parse()?)))
+    }
+}
+
+impl PgHasArrayType for KrateName {
+    fn array_type_info() -> PgTypeInfo {
+        <&str as PgHasArrayType>::array_type_info()
+    }
+
+    fn array_compatible(ty: &PgTypeInfo) -> bool {
+        <&str as PgHasArrayType>::array_compatible(ty)
     }
 }
 

src/storage/mod.rs

@@ -45,7 +45,7 @@ use tokio::{
     runtime,
     sync::Mutex,
 };
-use tracing::{debug, error, info_span, instrument, trace, warn};
+use tracing::{debug, info_span, instrument, trace, warn};
 use walkdir::WalkDir;
 
 const ARCHIVE_INDEX_FILE_EXTENSION: &str = "index";

src/utils/cargo_metadata.rs

@@ -1,4 +1,6 @@
-use crate::{db::types::version::Version, error::Result};
+use crate::db::types::krate_name::KrateName;
+use crate::web::ReqVersion;
+use crate::{db::types::version::Version, error::Result, web::extractors::rustdoc::RustdocParams};
 use anyhow::{Context, bail};
 use rustwide::{Toolchain, Workspace, cmd::Command};
 use semver::VersionReq;
@@ -135,6 +137,19 @@ pub(crate) struct Dependency {
     pub(crate) optional: bool,
 }
 
+impl Dependency {
+    pub(crate) fn rustdoc_params(&self) -> RustdocParams {
+        RustdocParams::new(
+            // I validated in the database, which makes me assume that renames are
+            // handled before storing the deps into the column.
+            self.name
+                .parse::<KrateName>()
+                .expect("we validated that the dep name is always a valid KrateName"),
+        )
+        .with_req_version(ReqVersion::Semver(self.req.clone()))
+    }
+}
+
 impl bincode::Encode for Dependency {
     fn encode<E: bincode::enc::Encoder>(
         &self,

src/utils/html.rs

@@ -18,7 +18,7 @@ use lol_html::{element, errors::RewritingError};
 use std::sync::Arc;
 use tokio::{io::AsyncRead, task::JoinHandle};
 use tokio_util::io::ReaderStream;
-use tracing::{Span, error, instrument};
+use tracing::{Span, instrument};
 use tracing_futures::Instrument as _;
 
 const CHANNEL_SIZE: usize = 64;

src/web/build_details.rs

@@ -76,7 +76,7 @@ pub(crate) async fn build_details_handler(
         .into_canonical_req_version_or_else(|confirmed_name, version| {
             let params = params
                 .clone()
-                .with_confirmed_name(Some(confirmed_name))
+                .with_name(confirmed_name)
                 .with_req_version(version);
             AxumNope::Redirect(
                 params.build_details_url(id, build_params.filename.as_deref()),

src/web/builds.rs

@@ -1,4 +1,5 @@
 use crate::build_queue::PRIORITY_MANUAL_FROM_CRATES_IO;
+use crate::db::types::krate_name::KrateName;
 use crate::{
     AsyncBuildQueue, Config,
     db::{
@@ -70,7 +71,7 @@ pub(crate) async fn build_list_handler(
         .into_canonical_req_version_or_else(|confirmed_name, version| {
             let params = params
                 .clone()
-                .with_confirmed_name(Some(confirmed_name))
+                .with_name(confirmed_name)
                 .with_req_version(version);
             AxumNope::Redirect(
                 params.builds_url(),
@@ -105,7 +106,7 @@ pub(crate) async fn build_list_handler(
 
 async fn crate_version_exists(
     conn: &mut sqlx::PgConnection,
-    name: &String,
+    name: &KrateName,
     version: &Version,
 ) -> Result<bool, anyhow::Error> {
     let row = sqlx::query!(
@@ -125,7 +126,7 @@ async fn crate_version_exists(
 
 async fn build_trigger_check(
     conn: &mut sqlx::PgConnection,
-    name: &String,
+    name: &KrateName,
     version: &Version,
     build_queue: &Arc<AsyncBuildQueue>,
 ) -> AxumResult<impl IntoResponse> {
@@ -145,7 +146,7 @@ async fn build_trigger_check(
 }
 
 pub(crate) async fn build_trigger_rebuild_handler(
-    Path((name, version)): Path<(String, Version)>,
+    Path((name, version)): Path<(KrateName, Version)>,
     mut conn: DbConnection,
     Extension(build_queue): Extension<Arc<AsyncBuildQueue>>,
     Extension(config): Extension<Arc<Config>>,

src/web/crate_details.rs

@@ -489,7 +489,7 @@ pub(crate) async fn crate_details_handler(
         .into_canonical_req_version_or_else(|confirmed_name, version| {
             let params = params
                 .clone()
-                .with_confirmed_name(Some(confirmed_name))
+                .with_name(confirmed_name)
                 .with_req_version(version);
             AxumNope::Redirect(
                 params.crate_details_url(),
@@ -660,7 +660,7 @@ pub(crate) async fn get_all_platforms_inner(
             AxumNope::Redirect(
                 params
                     .clone()
-                    .with_confirmed_name(Some(corrected_name))
+                    .with_name(corrected_name)
                     .with_req_version(req_version)
                     .platforms_partial_url(),
                 CachePolicy::NoCaching,
@@ -669,7 +669,7 @@ pub(crate) async fn get_all_platforms_inner(
         .into_canonical_req_version_or_else(|confirmed_name, version| {
             let params = params
                 .clone()
-                .with_confirmed_name(Some(confirmed_name))
+                .with_name(confirmed_name)
                 .with_req_version(version);
             AxumNope::Redirect(
                 params.platforms_partial_url(),
@@ -2254,7 +2254,7 @@ path = "src/lib.rs"
                 .await?;
 
             let resp = env.web_app().await.get("/crate/dummy%3E").await?;
-            assert_eq!(resp.status(), StatusCode::NOT_FOUND);
+            assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
 
             Ok(())
         })

src/web/error.rs

@@ -10,7 +10,6 @@ use axum::{
     response::{IntoResponse, Response as AxumResponse},
 };
 use std::borrow::Cow;
-use tracing::error;
 
 #[derive(Debug, thiserror::Error)]
 pub enum AxumNope {