fix parsing dependencies from release list

src/db/add_package.rs

@@ -1,17 +1,17 @@
 use crate::{
-    db::types::{BuildStatus, Feature, version::Version},
+    db::types::{
+        BuildId, BuildStatus, CrateId, Feature, ReleaseId, dependencies::ReleaseDependencyList,
+        version::Version,
+    },
     docbuilder::DocCoverage,
     error::Result,
     registry_api::{CrateData, CrateOwner, ReleaseData},
     storage::CompressionAlgorithm,
-    utils::{Dependency, MetadataPackage, rustc_version::parse_rustc_date},
+    utils::{MetadataPackage, rustc_version::parse_rustc_date},
     web::crate_details::{latest_release, releases_for_crate},
 };
 use anyhow::{Context, anyhow};
-use derive_more::{Deref, Display};
 use futures_util::stream::TryStreamExt;
-use semver::VersionReq;
-use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use slug::slugify;
 use std::{
@@ -22,56 +22,6 @@ use std::{
 };
 use tracing::{debug, error, info, instrument};
 
-#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
-#[sqlx(transparent)]
-pub struct CrateId(pub i32);
-
-#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
-#[sqlx(transparent)]
-pub struct ReleaseId(pub i32);
-
-#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
-#[sqlx(transparent)]
-pub struct BuildId(pub i32);
-
-type DepOut = (String, String, String, bool);
-type DepIn = (String, VersionReq, Option<String>, Option<bool>);
-
-/// A crate dependency in our internal representation for releases.dependencies json.
-#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Deref)]
-#[serde(from = "DepIn", into = "DepOut")]
-pub(crate) struct ReleaseDependency(Dependency);
-
-impl ReleaseDependency {
-    pub(crate) fn into_inner(self) -> Dependency {
-        self.0
-    }
-}
-
-impl From<DepIn> for ReleaseDependency {
-    fn from((name, req, kind, optional): DepIn) -> Self {
-        ReleaseDependency(Dependency {
-            name,
-            req,
-            kind,
-            optional: optional.unwrap_or(false),
-            rename: None,
-        })
-    }
-}
-
-impl From<ReleaseDependency> for DepOut {
-    fn from(rd: ReleaseDependency) -> Self {
-        let d = rd.0;
-        (
-            d.name,
-            d.req.to_string(),
-            d.kind.unwrap_or_else(|| "normal".into()),
-            d.optional,
-        )
-    }
-}
-
 /// Adds a package into database.
 ///
 /// Package must be built first.
@@ -98,7 +48,12 @@ pub(crate) async fn finish_release(
     source_size: u64,
 ) -> Result<()> {
     debug!("updating release data");
-    let dependencies = convert_dependencies(metadata_pkg)?;
+    let dependencies: ReleaseDependencyList = metadata_pkg
+        .dependencies
+        .iter()
+        .cloned()
+        .map(Into::into)
+        .collect();
     let rustdoc = get_rustdoc(metadata_pkg, source_dir).unwrap_or(None);
     let readme = get_readme(metadata_pkg, source_dir).unwrap_or(None);
     let features = get_features(metadata_pkg);
@@ -133,7 +88,7 @@ pub(crate) async fn finish_release(
            WHERE id = $1"#,
         release_id.0,
         registry_data.release_time,
-        dependencies,
+        serde_json::to_value(&dependencies)?,
         metadata_pkg.package_name(),
         registry_data.yanked,
         has_docs,
@@ -432,16 +387,6 @@ pub(crate) async fn initialize_build(
     Ok(build_id)
 }
 
-/// Convert dependencies into our own internal JSON representation
-fn convert_dependencies(pkg: &MetadataPackage) -> Result<serde_json::Value> {
-    let dependencies: Vec<_> = pkg
-        .dependencies
-        .iter()
-        .map(|dependency| ReleaseDependency(dependency.clone()))
-        .collect::<Vec<_>>();
-    Ok(serde_json::to_value(dependencies)?)
-}
-
 /// Reads features and converts them to Vec<Feature> with default being first
 fn get_features(pkg: &MetadataPackage) -> Vec<Feature> {
     let mut features = Vec::with_capacity(pkg.features.len());

src/db/mod.rs

@@ -4,17 +4,16 @@ use sqlx::migrate::{Migrate, Migrator};
 
 pub use self::add_package::update_latest_version_id;
 pub(crate) use self::add_package::{
-    ReleaseDependency, add_doc_coverage, finish_build, finish_release, initialize_build,
-    initialize_crate, initialize_release, update_build_with_error,
+    add_doc_coverage, finish_build, finish_release, initialize_build, initialize_crate,
+    initialize_release, update_build_with_error,
 };
 pub use self::{
-    add_package::{
-        BuildId, CrateId, ReleaseId, update_build_status, update_crate_data_in_database,
-    },
+    add_package::{update_build_status, update_crate_data_in_database},
     delete::{delete_crate, delete_version},
     file::{add_path_into_database, add_path_into_remote_archive},
     overrides::Overrides,
     pool::{AsyncPoolClient, Pool, PoolError},
+    types::{BuildId, CrateId, ReleaseId},
 };
 
 mod add_package;

src/db/types/dependencies.rs

@@ -0,0 +1,126 @@
+use crate::utils::Dependency;
+use derive_more::Deref;
+use semver::VersionReq;
+use serde::{Deserialize, Serialize};
+
+const DEFAULT_KIND: &str = "normal";
+
+/// A crate dependency in our internal representation for releases.dependencies json.
+#[derive(Debug, Clone, PartialEq, Deref)]
+pub(crate) struct ReleaseDependency(Dependency);
+
+impl<'de> Deserialize<'de> for ReleaseDependency {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        /// The three possible representations of a dependency in our internal JSON format
+        /// in the `releases.dependencies` column.
+        #[derive(Serialize, Deserialize)]
+        #[serde(untagged)]
+        enum Repr {
+            /// just [name, version]``
+            Basic((String, VersionReq)),
+            /// [name, version, kind]
+            WithKind((String, VersionReq, String)),
+            /// [name, version, kind, optional]
+            Full((String, VersionReq, String, bool)),
+        }
+
+        let src = Repr::deserialize(deserializer)?;
+        let (name, req, kind, optional) = match src {
+            Repr::Basic((name, req)) => (name, req, DEFAULT_KIND.into(), false),
+            Repr::WithKind((name, req, kind)) => (name, req, kind, false),
+            Repr::Full((name, req, kind, optional)) => (name, req, kind, optional),
+        };
+
+        Ok(ReleaseDependency(Dependency {
+            name,
+            req,
+            kind: Some(kind),
+            optional,
+            rename: None,
+        }))
+    }
+}
+
+impl Serialize for ReleaseDependency {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        let dep = &self.0;
+        let kind = dep.kind.as_deref().unwrap_or(DEFAULT_KIND);
+        (dep.name.as_str(), &dep.req, kind, dep.optional).serialize(serializer)
+    }
+}
+
+impl From<Dependency> for ReleaseDependency {
+    fn from(dep: Dependency) -> Self {
+        ReleaseDependency(dep)
+    }
+}
+
+impl From<ReleaseDependency> for Dependency {
+    fn from(dep: ReleaseDependency) -> Self {
+        dep.0
+    }
+}
+
+pub(crate) type ReleaseDependencyList = Vec<ReleaseDependency>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use anyhow::Result;
+    use test_case::test_case;
+
+    #[test_case("[]", "[]"; "empty")]
+    #[test_case(
+        r#"[["vec_map", "^0.0.1"]]"#,
+        r#"[["vec_map","^0.0.1","normal",false]]"#;
+        "2-tuple"
+    )]
+    #[test_case(
+        r#"[["vec_map", "^0.0.1", "normal" ]]"#,
+        r#"[["vec_map","^0.0.1","normal",false]]"#;
+        "3-tuple"
+    )]
+    #[test_case(
+        r#"[["rand", "^0.9", "normal", false], ["sdl3", "^0.16", "normal", false]]"#,
+        r#"[["rand","^0.9","normal",false],["sdl3","^0.16","normal",false]]"#;
+        "4-tuple"
+    )]
+    #[test_case(
+        r#"[["byteorder", "^0.5", "normal", false],["clippy", "^0", "normal", true]]"#,
+        r#"[["byteorder","^0.5","normal",false],["clippy","^0","normal",true]]"#;
+        "with optional"
+    )]
+    fn test_parse_release_dependency_json(input: &str, output: &str) -> Result<()> {
+        let deps: ReleaseDependencyList = serde_json::from_str(input)?;
+
+        assert_eq!(serde_json::to_string(&deps)?, output);
+        Ok(())
+    }
+
+    #[test_case(r#"[["vec_map", "^0.0.1"]]"#, "normal", false)]
+    #[test_case(r#"[["vec_map", "^0.0.1", "dev" ]]"#, "dev", false)]
+    #[test_case(r#"[["vec_map", "^0.0.1", "dev", true ]]"#, "dev", true)]
+    fn test_parse_dependency(
+        input: &str,
+        expected_kind: &str,
+        expected_optional: bool,
+    ) -> Result<()> {
+        let deps: ReleaseDependencyList = serde_json::from_str(input)?;
+        let [dep] = deps.as_slice() else {
+            panic!("expected exactly one dependency");
+        };
+
+        assert_eq!(dep.name, "vec_map");
+        assert_eq!(dep.req, VersionReq::parse("^0.0.1")?);
+        assert_eq!(dep.kind.as_deref(), Some(expected_kind));
+        assert_eq!(dep.optional, expected_optional);
+
+        Ok(())
+    }
+}

src/db/types/mod.rs

@@ -1,7 +1,21 @@
+use derive_more::Display;
 use serde::{Deserialize, Serialize};
 
+pub mod dependencies;
 pub mod version;
 
+#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
+#[sqlx(transparent)]
+pub struct CrateId(pub i32);
+
+#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
+#[sqlx(transparent)]
+pub struct ReleaseId(pub i32);
+
+#[derive(Debug, Clone, Copy, Display, PartialEq, Eq, Hash, Serialize, sqlx::Type)]
+#[sqlx(transparent)]
+pub struct BuildId(pub i32);
+
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, sqlx::Type)]
 #[sqlx(type_name = "feature")]
 pub struct Feature {

src/web/crate_details.rs

@@ -1,8 +1,8 @@
 use crate::{
     AsyncStorage,
     db::{
-        BuildId, CrateId, ReleaseDependency, ReleaseId,
-        types::{BuildStatus, version::Version},
+        BuildId, CrateId, ReleaseId,
+        types::{BuildStatus, dependencies::ReleaseDependencyList, version::Version},
     },
     impl_axum_webpage,
     registry_api::OwnerKind,
@@ -234,12 +234,17 @@ impl CrateDetails {
 
         let parsed_license = krate.license.as_deref().map(super::licenses::parse_license);
 
-        let dependencies = krate
+        let dependencies: Vec<Dependency> = krate
             .dependencies
-            .and_then(|value| serde_json::from_value::<Vec<ReleaseDependency>>(value).ok())
+            .map(serde_json::from_value::<ReleaseDependencyList>)
+            .transpose()
+            // NOTE: we sometimes have invalid semver-requirement strings the database
+            // (at the time writing, 14 releases out of 2 million).
+            // We silently ignore those here.
+            .unwrap_or_default()
             .unwrap_or_default()
             .into_iter()
-            .map(|rdep| rdep.into_inner())
+            .map(Into::into)
             .collect();
 
         let mut crate_details = CrateDetails {

src/web/rustdoc.rs

@@ -2674,6 +2674,7 @@ mod test {
                     .await
                     .get("/testing/0.1.0/testing/")
                     .await?
+                    .error_for_status()?
                     .text()
                     .await?
             ));