fix(lockfiles) trailing zero yaml parse (#6296)

### Description Fixes #6232 This PR allows us to properly deserialize semver versions from YAML in `yarn.lock`. Previously we would fail at parsing ranges with trailing zeros e.g. `(0.10f32).to_string() == "0.1"`. The approach taken in this PR is due to some outstanding quirks in `serde_yaml`: - dtolnay/serde-yaml#165 (comment) - dtolnay/serde-yaml#388 Our usage of `#[serde(flatten)]` in `LockfileData` caused attempting to parse `2` or `0.10` as a `String` to fail. To avoid this we first parse the document as a map with a union of the metadata/package entries and then convert this to the expected structure. This provides us the type safety of the old implementation, but at the cost of rebuilding the map. As a minor thing, I removed all unused `Serialize`/`Deserialize` implementations to make it clear which codepaths actually get used. ### Testing Instructions Existing unit tests pass. I changed out the old unit tests for `SemverString` to be captured by the new `berry_semver.lock` test fixture which covers the same cases. We do this because even if parsing versions works when invoked directly, adding `#[serde(flatten)]`/`#[serde(untagged)]` to any containing structure changes the behavior. Closes TURBO-1540 --------- Co-authored-by: Chris Olszewski <Chris Olszewski>
vercel · Oct 30, 2023 · 8af1b94 · 8af1b94
1 parent 23efa9a
commit 8af1b94
Show file tree

Hide file tree

Showing 6 changed files with 219 additions and 80 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/turborepo-lockfiles/Cargo.toml b/crates/turborepo-lockfiles/Cargo.toml
@@ -14,7 +14,7 @@ regex = "1"
 semver = "1.0.17"
 serde = { version = "1.0.126", features = ["derive", "rc"] }
 serde_json = "1.0.86"
-serde_yaml = "0.9"
+serde_yaml = "0.9.27"
 thiserror = "1.0.38"
 turbopath = { path = "../turborepo-paths" }
 

diff --git a/crates/turborepo-lockfiles/fixtures/berry_semver.lock b/crates/turborepo-lockfiles/fixtures/berry_semver.lock
@@ -0,0 +1,31 @@
+# This file is generated by running "yarn install" inside your project.
+# Manual changes might be lost - proceed with caution!
+
+__metadata:
+  version: 6
+  cacheKey: 8
+
+"file-source@npm:2":
+  version: 2.6.1
+  resolution: "file-source@npm:2.6.1"
+  dependencies:
+    stream-source: 0.10
+  checksum: db27232df214b27ddd7026bbd202caf0bde31123811ebac0a9772d6a41735efdc3ee8050661f56d02d43811bde79938adb5ed8cbfe5f813c7cd09b86d19b6c84
+  languageName: node
+  linkType: hard
+
+"foo@workspace:packages/foo":
+  version: 0.0.0-use.local
+  resolution: "foo@workspace:packages/foo"
+  dependencies:
+    file-source: 2
+  languageName: unknown
+  linkType: soft
+
+"stream-source@npm:0.10":
+  version: 0.10.12
+  resolution: "stream-source@npm:0.10.12"
+  checksum: 65a0e0a38014dcfa6e219e92d83bde5fe716cf16df339e9d7e951525c0d129771416de4d77c3389c38cdd20a43d9575f90cc91041d6fa0c2f053b989780e3591
+  languageName: node
+  linkType: hard
+
diff --git a/crates/turborepo-lockfiles/src/berry/de.rs b/crates/turborepo-lockfiles/src/berry/de.rs
@@ -1,63 +1,150 @@
-use serde::{Deserialize, Serialize};
+use std::collections::BTreeMap;
 
-// Newtype used for correct deserialization
-#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Default, Clone)]
-pub struct SemverString(pub String);
+use serde::Deserialize;
 
-impl From<SemverString> for String {
-    fn from(value: SemverString) -> Self {
-        value.0
-    }
+use super::{BerryPackage, DependencyMeta, LockfileData, Metadata};
+
+const METADATA_KEY: &str = "__metadata";
+
+/// Union type of yarn.lock metadata entry and package entries.
+/// Only as a workaround for serde_yaml behavior around parsing numbers as
+/// strings.
+// In the ideal world this would be an enum, but serde_yaml currently has behavior
+// where using `#[serde(untagged)]` or `#[serde(flatten)]` affects how it handles
+// YAML numbers being parsed as Strings.
+// If these macros are present, then it will refuse to parse 1 or 1.0 as a String
+// and will instead only parse them as an int/float respectively.
+// If these macros aren't present, then it will happily parse 1 or 1.0 as
+// "1" and "1.0".
+#[derive(Debug, Deserialize, Default)]
+#[serde(rename_all = "camelCase")]
+pub(crate) struct Entry {
+    version: String,
+    language_name: Option<String>,
+    dependencies: Option<BTreeMap<String, String>>,
+    peer_dependencies: Option<BTreeMap<String, String>>,
+    dependencies_meta: Option<BTreeMap<String, DependencyMeta>>,
+    peer_dependencies_meta: Option<BTreeMap<String, DependencyMeta>>,
+    bin: Option<BTreeMap<String, String>>,
+    link_type: Option<String>,
+    resolution: Option<String>,
+    checksum: Option<String>,
+    conditions: Option<String>,
+    cache_key: Option<String>,
 }
 
-impl AsRef<str> for SemverString {
-    fn as_ref(&self) -> &str {
-        self.0.as_str()
-    }
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("missing resolution for entry {0}")]
+    MissingResolution(String),
+    #[error("multiple entry {0} has fields that should only appear in metadata")]
+    InvalidMetadataFields(String),
+    #[error("lockfile missing {METADATA_KEY} entry")]
+    MissingMetadata,
 }
 
-impl<'de> Deserialize<'de> for SemverString {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: serde::Deserializer<'de>,
-    {
-        // We use this to massage numerical semver versions to strings
-        // e.g. 2
-        #[derive(Deserialize)]
-        #[serde(untagged)]
-        enum StringOrNum {
-            String(String),
-            Int(u64),
-            Float(f32),
-        }
+impl TryFrom<BTreeMap<String, Entry>> for LockfileData {
+    type Error = Error;
 
-        match StringOrNum::deserialize(deserializer)? {
-            StringOrNum::String(s) => Ok(SemverString(s)),
-            StringOrNum::Int(x) => Ok(SemverString(x.to_string())),
-            StringOrNum::Float(f) => Ok(SemverString(f.to_string())),
+    fn try_from(mut value: BTreeMap<String, Entry>) -> Result<Self, Self::Error> {
+        let Entry {
+            version, cache_key, ..
+        } = value.remove(METADATA_KEY).ok_or(Error::MissingMetadata)?;
+        let metadata = Metadata { version, cache_key };
+        let mut packages = BTreeMap::new();
+        for (key, entry) in value {
+            let Entry {
+                version,
+                language_name,
+                dependencies,
+                peer_dependencies,
+                dependencies_meta,
+                peer_dependencies_meta,
+                bin,
+                link_type,
+                resolution,
+                checksum,
+                conditions,
+                cache_key,
+            } = entry;
+            if cache_key.is_some() {
+                return Err(Error::InvalidMetadataFields(key));
+            }
+            let resolution = resolution.ok_or_else(|| Error::MissingResolution(key.clone()))?;
+            packages.insert(
+                key,
+                BerryPackage {
+                    version,
+                    language_name,
+                    dependencies,
+                    peer_dependencies,
+                    dependencies_meta,
+                    peer_dependencies_meta,
+                    bin,
+                    link_type,
+                    resolution,
+                    checksum,
+                    conditions,
+                },
+            );
         }
+
+        Ok(LockfileData { metadata, packages })
     }
 }
 
 #[cfg(test)]
 mod test {
-    use std::collections::HashMap;
-
     use super::*;
 
     #[test]
-    fn test_semver() {
-        let input = "foo: 1.2.3
-bar: 2
-baz: latest
-oop: 1.3
-";
-
-        let result: HashMap<String, SemverString> = serde_yaml::from_str(input).unwrap();
-
-        assert_eq!(result["foo"].as_ref(), "1.2.3");
-        assert_eq!(result["bar"].as_ref(), "2");
-        assert_eq!(result["baz"].as_ref(), "latest");
-        assert_eq!(result["oop"].as_ref(), "1.3")
+    fn test_requires_metadata() {
+        let data = BTreeMap::new();
+        assert!(LockfileData::try_from(data).is_err());
+    }
+
+    #[test]
+    fn test_rejects_cache_key_in_packages() {
+        let mut data = BTreeMap::new();
+        data.insert(
+            METADATA_KEY.to_string(),
+            Entry {
+                version: "1".into(),
+                cache_key: Some("8".into()),
+                ..Default::default()
+            },
+        );
+        data.insert(
+            "foo".to_string(),
+            Entry {
+                version: "1".into(),
+                resolution: Some("resolved".into()),
+                cache_key: Some("8".into()),
+                ..Default::default()
+            },
+        );
+        assert!(LockfileData::try_from(data).is_err());
+    }
+
+    #[test]
+    fn test_requires_resolution() {
+        let mut data = BTreeMap::new();
+        data.insert(
+            METADATA_KEY.to_string(),
+            Entry {
+                version: "1".into(),
+                cache_key: Some("8".into()),
+                ..Default::default()
+            },
+        );
+        data.insert(
+            "foo".to_string(),
+            Entry {
+                version: "1".into(),
+                resolution: None,
+                ..Default::default()
+            },
+        );
+        assert!(LockfileData::try_from(data).is_err());
     }
 }