fix: support missing variant of json structure in release.dependencies

Author: syphar

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,7 @@ 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.clone().into();
     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 +83,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 +382,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,106 @@
+use crate::utils::Dependency;
+use derive_more::Deref;
+use semver::VersionReq;
+use serde::{Deserialize, Serialize};
+
+const DEFAULT_KIND: &str = "normal";
+
+/// The three possible representations of a dependency in our internal JSON format
+/// in the `releases.dependencies` column.
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(untagged)]
+enum Dep {
+    Two((String, VersionReq)),
+    Three((String, VersionReq, String)),
+    Four((String, VersionReq, String, bool)),
+}
+
+/// A crate dependency in our internal representation for releases.dependencies json.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Deref)]
+#[serde(from = "Dep", into = "Dep")]
+pub(crate) struct ReleaseDependency(Dependency);
+
+impl From<Dep> for ReleaseDependency {
+    fn from(src: Dep) -> Self {
+        let (name, req, kind, optional) = match src {
+            Dep::Two((name, req)) => (name, req, DEFAULT_KIND.into(), false),
+            Dep::Three((name, req, kind)) => (name, req, kind, false),
+            Dep::Four((name, req, kind, optional)) => (name, req, kind, optional),
+        };
+
+        ReleaseDependency(Dependency {
+            name,
+            req,
+            kind: Some(kind),
+            optional,
+            rename: None,
+        })
+    }
+}
+
+impl From<ReleaseDependency> for Dep {
+    // dependency serialization for new releases.
+    fn from(rd: ReleaseDependency) -> Self {
+        let d = rd.0;
+        Dep::Four((
+            d.name,
+            d.req,
+            d.kind.unwrap_or_else(|| DEFAULT_KIND.into()),
+            d.optional,
+        ))
+    }
+}
+
+#[derive(Debug, Clone, Serialize, Deserialize, Default, Deref)]
+#[serde(transparent)]
+pub(crate) struct ReleaseDependencyList(Vec<ReleaseDependency>);
+
+impl<I> From<I> for ReleaseDependencyList
+where
+    I: IntoIterator<Item = Dependency>,
+{
+    fn from(deps: I) -> Self {
+        Self(deps.into_iter().map(ReleaseDependency).collect())
+    }
+}
+
+impl ReleaseDependencyList {
+    pub(crate) fn into_iter_dependencies(self) -> impl Iterator<Item = Dependency> {
+        self.0.into_iter().map(|rd| rd.0)
+    }
+}
+
+#[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(())
+    }
+}

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,13 +234,19 @@ impl CrateDetails {
 
         let parsed_license = krate.license.as_deref().map(super::licenses::parse_license);
 
-        let dependencies = krate
-            .dependencies
-            .and_then(|value| serde_json::from_value::<Vec<ReleaseDependency>>(value).ok())
-            .unwrap_or_default()
-            .into_iter()
-            .map(|rdep| rdep.into_inner())
-            .collect();
+        let dependencies = if let Some(value) = krate.dependencies {
+            match serde_json::from_value::<ReleaseDependencyList>(value) {
+                Ok(list) => list.into_iter_dependencies().collect(),
+                Err(_) => {
+                    // 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.
+                    Vec::new()
+                }
+            }
+        } else {
+            Vec::new()
+        };
 
         let mut crate_details = CrateDetails {
             name: krate.name,

src/web/rustdoc.rs

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