fix: use normalized version constraints (#38)

Cargo.toml

@@ -14,7 +14,7 @@ path = "tests/spec_test.rs"
 harness = false
 
 [dependencies]
-deno_semver = "0.5.11"
+deno_semver = "0.5.12"
 serde = { version = "1.0.149", features = ["derive"] }
 serde_json = "1.0.85"
 thiserror = "1.0.40"

src/error.rs

@@ -1,37 +1,33 @@
 // Copyright 2018-2024 the Deno authors. MIT license.
 
+use deno_semver::jsr::JsrDepPackageReqParseError;
 use deno_semver::package::PackageNv;
 use thiserror::Error;
 
 use crate::transforms::TransformError;
 
-#[derive(Debug)]
+#[derive(Debug, Error)]
+#[error("Failed reading lockfile at '{file_path}'")]
 pub struct LockfileError {
   pub file_path: String,
-  pub reason: LockfileErrorReason,
-}
-
-impl std::error::Error for LockfileError {}
-
-impl std::fmt::Display for LockfileError {
-  fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-    match &self.reason {
-      LockfileErrorReason::Empty => write!(f, "Unable to read lockfile. Lockfile was empty at '{}'.", self.file_path),
-      LockfileErrorReason::ParseError(e) => write!(f, "Unable to parse contents of lockfile '{}': {:#}.", self.file_path, e),
-      LockfileErrorReason::DeserializationError(e) => write!(f, "Unable to deserialize lockfile '{}': {:#}.", self.file_path, e),
-      LockfileErrorReason::UnsupportedVersion { version } => write!(f, "Unsupported lockfile version '{}'. Try upgrading Deno or recreating the lockfile at '{}'.", version, self.file_path),
-      LockfileErrorReason::TransformError(e) => write!(f, "Unable to upgrade old lockfile '{}': {:#}.", self.file_path, e),
-    }
-  }
+  #[source]
+  pub source: LockfileErrorReason,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Error)]
 pub enum LockfileErrorReason {
+  #[error("Lockfile was empty")]
   Empty,
+  #[error("Failed parsing. Lockfile may be corrupt")]
   ParseError(serde_json::Error),
-  DeserializationError(DeserializationError),
+  #[error("Failed deserializing. Lockfile may be corrupt")]
+  DeserializationError(#[source] DeserializationError),
+  #[error("Unsupported lockfile version '{version}'. Try upgrading Deno or recreating the lockfile")]
   UnsupportedVersion { version: String },
-  TransformError(TransformError),
+  #[error(
+    "Failed upgrading lockfile to latest version. Lockfile may be corrupt"
+  )]
+  TransformError(#[source] TransformError),
 }
 
 impl From<TransformError> for LockfileErrorReason {
@@ -44,19 +40,21 @@ impl From<TransformError> for LockfileErrorReason {
 pub enum DeserializationError {
   #[error("Invalid {0} section: {1:#}")]
   FailedDeserializing(&'static str, serde_json::Error),
-  #[error("Invalid npm package '{0}'. Lockfile may be corrupt or you might be using an old version of Deno.")]
+  #[error("Invalid npm package '{0}'")]
   InvalidNpmPackageId(String),
-  #[error("Invalid npm package dependency '{0}'. Lockfile may be corrupt or you might be using an old version of Deno.")]
+  #[error("Invalid npm package dependency '{0}'")]
   InvalidNpmPackageDependency(String),
-  #[error("Invalid package specifier '{0}'. Lockfile may be corrupt or you might be using an old version of Deno.")]
-  InvalidPackageSpecifier(String),
-  #[error("Invalid package specifier version '{version}' for '{specifier}'. Lockfile may be corrupt or you might be using an old version of Deno.")]
+  #[error(transparent)]
+  InvalidPackageSpecifier(#[from] JsrDepPackageReqParseError),
+  #[error("Invalid package specifier version '{version}' for '{specifier}'")]
   InvalidPackageSpecifierVersion { specifier: String, version: String },
-  #[error("Invalid jsr dependency '{dependency}' for '{package}'. Lockfile may be corrupt or you might be using an old version of Deno.")]
+  #[error("Invalid jsr dependency '{dependency}' for '{package}'")]
   InvalidJsrDependency {
     package: PackageNv,
     dependency: String,
   },
-  #[error("npm package '{0}' was not found and could not have its version resolved. Lockfile may be corrupt.")]
+  #[error(
+    "npm package '{0}' was not found and could not have its version resolved"
+  )]
   MissingPackage(String),
 }

src/lib.rs

@@ -243,10 +243,7 @@ impl LockfileContent {
         let mut specifiers =
           HashMap::with_capacity(deserialized_specifiers.len());
         for (key, value) in deserialized_specifiers {
-          let dep = JsrDepPackageReq::from_str(&key).map_err(|_err| {
-            // todo(dsherret): surface internal error here
-            DeserializationError::InvalidPackageSpecifier(key.to_string())
-          })?;
+          let dep = JsrDepPackageReq::from_str(&key)?;
           specifiers.insert(dep, value);
         }
 
@@ -458,14 +455,14 @@ impl Lockfile {
     if opts.content.trim().is_empty() {
       return Err(Box::new(LockfileError {
         file_path: opts.file_path.display().to_string(),
-        reason: LockfileErrorReason::Empty,
+        source: LockfileErrorReason::Empty,
       }));
     }
 
     let content =
       load_content(opts.content).map_err(|reason| LockfileError {
         file_path: opts.file_path.display().to_string(),
-        reason,
+        source: reason,
       })?;
     Ok(Lockfile {
       overwrite: opts.overwrite,
@@ -850,18 +847,18 @@ mod tests {
   #[test]
   fn future_version_unsupported() {
     let file_path = PathBuf::from("lockfile.json");
-    assert_eq!(
-      Lockfile::new(
-        NewLockfileOptions {
-        file_path,
-        content: "{ \"version\": \"2000\" }",
-        overwrite: false,
-        }
-      )
-      .err()
-      .unwrap().to_string(),
-      "Unsupported lockfile version '2000'. Try upgrading Deno or recreating the lockfile at 'lockfile.json'.".to_string()
-    );
+    let err = Lockfile::new(NewLockfileOptions {
+      file_path,
+      content: "{ \"version\": \"2000\" }",
+      overwrite: false,
+    })
+    .unwrap_err();
+    match err.source {
+      LockfileErrorReason::UnsupportedVersion { version } => {
+        assert_eq!(version, "2000")
+      }
+      _ => unreachable!(),
+    }
   }
 
   #[test]
@@ -1154,8 +1151,8 @@ mod tests {
       r#"{
   "version": "4",
   "specifiers": {
-    "jsr:@foo/bar@^2": "jsr:@foo/[email protected]",
-    "jsr:path": "jsr:@std/[email protected]"
+    "jsr:@foo/bar@2": "jsr:@foo/[email protected]",
+    "jsr:path@*": "jsr:@std/[email protected]"
   }
 }
 "#,
@@ -1283,9 +1280,6 @@ mod tests {
     })
     .err()
     .unwrap();
-    assert_eq!(
-      err.to_string(),
-      "Unable to read lockfile. Lockfile was empty at 'lockfile.json'."
-    );
+    assert!(matches!(err.source, LockfileErrorReason::Empty));
   }
 }

src/printer.rs

@@ -29,34 +29,47 @@ struct SerializedNpmPkg<'a> {
   dependencies: Vec<Cow<'a, str>>,
 }
 
+// WARNING: It's important to implement Ord/PartialOrd on the final
+// normalized string so that sorting works according to the final
+// output and so that's why this is used rather than JsrDepPackageReq
+// directly.
+#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Serialize)]
+struct SerializedJsrDepPackageReq(String);
+
+impl SerializedJsrDepPackageReq {
+  pub fn new(dep_req: &JsrDepPackageReq) -> Self {
+    Self(dep_req.to_string_normalized())
+  }
+}
+
 #[derive(Debug, Default, Serialize)]
 #[serde(rename_all = "camelCase")]
-struct SerializedLockfilePackageJsonContent<'a> {
+struct SerializedLockfilePackageJsonContent {
   #[serde(default)]
   #[serde(skip_serializing_if = "Vec::is_empty")]
-  pub dependencies: Vec<&'a JsrDepPackageReq>,
+  pub dependencies: Vec<SerializedJsrDepPackageReq>,
 }
 
-impl<'a> SerializedLockfilePackageJsonContent<'a> {
+impl SerializedLockfilePackageJsonContent {
   pub fn is_empty(&self) -> bool {
     self.dependencies.is_empty()
   }
 }
 
 #[derive(Debug, Default, Serialize)]
 #[serde(rename_all = "camelCase")]
-struct SerializedWorkspaceMemberConfigContent<'a> {
+struct SerializedWorkspaceMemberConfigContent {
   #[serde(skip_serializing_if = "Vec::is_empty")]
   #[serde(default)]
-  pub dependencies: Vec<&'a JsrDepPackageReq>,
+  pub dependencies: Vec<SerializedJsrDepPackageReq>,
   #[serde(
     skip_serializing_if = "SerializedLockfilePackageJsonContent::is_empty"
   )]
   #[serde(default)]
-  pub package_json: SerializedLockfilePackageJsonContent<'a>,
+  pub package_json: SerializedLockfilePackageJsonContent,
 }
 
-impl<'a> SerializedWorkspaceMemberConfigContent<'a> {
+impl SerializedWorkspaceMemberConfigContent {
   pub fn is_empty(&self) -> bool {
     self.dependencies.is_empty() && self.package_json.is_empty()
   }
@@ -66,10 +79,10 @@ impl<'a> SerializedWorkspaceMemberConfigContent<'a> {
 #[serde(rename_all = "camelCase")]
 struct SerializedWorkspaceConfigContent<'a> {
   #[serde(default, flatten)]
-  pub root: SerializedWorkspaceMemberConfigContent<'a>,
+  pub root: SerializedWorkspaceMemberConfigContent,
   #[serde(skip_serializing_if = "BTreeMap::is_empty")]
   #[serde(default)]
-  pub members: BTreeMap<&'a str, SerializedWorkspaceMemberConfigContent<'a>>,
+  pub members: BTreeMap<&'a str, SerializedWorkspaceMemberConfigContent>,
 }
 
 impl<'a> SerializedWorkspaceConfigContent<'a> {
@@ -83,7 +96,7 @@ struct LockfileV4<'a> {
   // order these based on auditability
   version: &'static str,
   #[serde(skip_serializing_if = "BTreeMap::is_empty")]
-  specifiers: BTreeMap<String, &'a String>,
+  specifiers: BTreeMap<SerializedJsrDepPackageReq, &'a String>,
   #[serde(skip_serializing_if = "BTreeMap::is_empty")]
   jsr: BTreeMap<&'a PackageNv, SerializedJsrPkg<'a>>,
   #[serde(skip_serializing_if = "BTreeMap::is_empty")]
@@ -137,7 +150,7 @@ pub fn print_v4_content(content: &LockfileContent) -> String {
                   if has_single_specifier {
                     format!("{}{}", dep.kind.scheme_with_colon(), dep.req.name)
                   } else {
-                    dep.to_string()
+                    dep.to_string_normalized()
                   }
                 })
                 .collect::<Vec<_>>();
@@ -211,7 +224,11 @@ pub fn print_v4_content(content: &LockfileContent) -> String {
   fn handle_pkg_json_content(
     content: &LockfilePackageJsonContent,
   ) -> SerializedLockfilePackageJsonContent {
-    let mut dependencies = content.dependencies.iter().collect::<Vec<_>>();
+    let mut dependencies = content
+      .dependencies
+      .iter()
+      .map(SerializedJsrDepPackageReq::new)
+      .collect::<Vec<_>>();
     dependencies.sort();
     SerializedLockfilePackageJsonContent { dependencies }
   }
@@ -221,7 +238,11 @@ pub fn print_v4_content(content: &LockfileContent) -> String {
   ) -> SerializedWorkspaceMemberConfigContent {
     SerializedWorkspaceMemberConfigContent {
       dependencies: {
-        let mut member = member.dependencies.iter().collect::<Vec<_>>();
+        let mut member = member
+          .dependencies
+          .iter()
+          .map(SerializedJsrDepPackageReq::new)
+          .collect::<Vec<_>>();
         member.sort();
         member
       },
@@ -246,7 +267,7 @@ pub fn print_v4_content(content: &LockfileContent) -> String {
   let mut specifiers = BTreeMap::new();
   for (key, value) in &content.packages.specifiers {
     // insert a string to ensure proper sorting
-    specifiers.insert(key.to_string(), value);
+    specifiers.insert(SerializedJsrDepPackageReq::new(key), value);
   }
 
   let lockfile = LockfileV4 {

tests/specs/config_changes/DenoIssue22250.txt

@@ -944,7 +944,7 @@
 {
   "version": "4",
   "specifiers": {
-    "npm:@spotify/web-api-ts-sdk": "1.2.0",
+    "npm:@spotify/web-api-ts-sdk@*": "1.2.0",
     "npm:[email protected]": "[email protected]",
     "npm:[email protected]": "[email protected]",
     "npm:[email protected]": "8.4.31"
@@ -1414,7 +1414,7 @@
   },
   "workspace": {
     "dependencies": [
-      "npm:@spotify/web-api-ts-sdk"
+      "npm:@spotify/web-api-ts-sdk@*"
     ]
   }
 }

tests/specs/config_changes/NoConfigMaintains.txt

@@ -206,11 +206,11 @@
 {
   "version": "4",
   "specifiers": {
-    "jsr:@scope/package_a": "0.0.1",
-    "jsr:@scope/package_b": "0.0.1",
-    "jsr:@scope/package_c": "0.0.1",
-    "jsr:@scope/package_orphan": "0.0.1",
-    "npm:ts-morph": "21.0.1"
+    "jsr:@scope/package_a@*": "0.0.1",
+    "jsr:@scope/package_b@*": "0.0.1",
+    "jsr:@scope/package_c@*": "0.0.1",
+    "jsr:@scope/package_orphan@*": "0.0.1",
+    "npm:ts-morph@*": "21.0.1"
   },
   "jsr": {
     "@scope/[email protected]": {
@@ -376,11 +376,11 @@
   },
   "workspace": {
     "dependencies": [
-      "jsr:@scope/package_a"
+      "jsr:@scope/package_a@*"
     ],
     "packageJson": {
       "dependencies": [
-        "npm:ts-morph"
+        "npm:ts-morph@*"
       ]
     }
   }

tests/specs/config_changes/NoConfigMaintains_v4.txt

@@ -190,11 +190,11 @@
 {
   "version": "4",
   "specifiers": {
-    "jsr:@scope/package_a@^0.0": "0.0.1",
-    "jsr:@scope/package_b": "0.0.1",
-    "jsr:@scope/package_c": "0.0.1",
-    "jsr:@scope/package_orphan": "0.0.1",
-    "npm:ts-morph": "21.0.1"
+    "jsr:@scope/[email protected]": "0.0.1",
+    "jsr:@scope/package_b@*": "0.0.1",
+    "jsr:@scope/package_c@*": "0.0.1",
+    "jsr:@scope/package_orphan@*": "0.0.1",
+    "npm:ts-morph@*": "21.0.1"
   },
   "jsr": {
     "@scope/[email protected]": {
@@ -360,11 +360,11 @@
   },
   "workspace": {
     "dependencies": [
-      "jsr:@scope/package_a"
+      "jsr:@scope/package_a@*"
     ],
     "packageJson": {
       "dependencies": [
-        "npm:ts-morph"
+        "npm:ts-morph@*"
       ]
     }
   }