fix(lock): Additive lock file (denoland#16500)

This commit changes lockfile to be "additive" - ie. integrity check only fails if
file/package is already specified in the lockfile, but its integrity doesn't match.

If file/package is not present in the lockfile, it will be added to the lockfile and
the lockfile will be written to disk.

cli/lockfile.rs

@@ -75,15 +75,16 @@ pub struct LockfileContent {
 
 #[derive(Debug, Clone)]
 pub struct Lockfile {
-  pub write: bool,
+  pub overwrite: bool,
+  pub has_content_changed: bool,
   pub content: LockfileContent,
   pub filename: PathBuf,
 }
 
 impl Lockfile {
-  pub fn new(filename: PathBuf, write: bool) -> Result<Lockfile, AnyError> {
+  pub fn new(filename: PathBuf, overwrite: bool) -> Result<Lockfile, AnyError> {
     // Writing a lock file always uses the new format.
-    let content = if write {
+    let content = if overwrite {
       LockfileContent {
         version: "2".to_string(),
         remote: BTreeMap::new(),
@@ -114,15 +115,16 @@ impl Lockfile {
     };
 
     Ok(Lockfile {
-      write,
+      overwrite,
+      has_content_changed: false,
       content,
       filename,
     })
   }
 
   // Synchronize lock file to disk - noop if --lock-write file is not specified.
   pub fn write(&self) -> Result<(), AnyError> {
-    if !self.write {
+    if !self.has_content_changed && !self.overwrite {
       return Ok(());
     }
 
@@ -148,39 +150,40 @@ impl Lockfile {
     specifier: &str,
     code: &str,
   ) -> bool {
-    if self.write {
+    if self.overwrite {
       // In case --lock-write is specified check always passes
       self.insert(specifier, code);
       true
     } else {
-      self.check(specifier, code)
+      self.check_or_insert(specifier, code)
     }
   }
 
   pub fn check_or_insert_npm_package(
     &mut self,
     package: &NpmResolutionPackage,
   ) -> Result<(), LockfileError> {
-    if self.write {
+    if self.overwrite {
       // In case --lock-write is specified check always passes
-      self.insert_npm_package(package);
+      self.insert_npm(package);
       Ok(())
     } else {
-      self.check_npm_package(package)
+      self.check_or_insert_npm(package)
     }
   }
 
-  /// Checks the given module is included.
-  /// Returns Ok(true) if check passed.
-  fn check(&mut self, specifier: &str, code: &str) -> bool {
+  /// Checks the given module is included, if so verify the checksum. If module
+  /// is not included, insert it.
+  fn check_or_insert(&mut self, specifier: &str, code: &str) -> bool {
     if specifier.starts_with("file:") {
       return true;
     }
     if let Some(lockfile_checksum) = self.content.remote.get(specifier) {
       let compiled_checksum = crate::checksum::gen(&[code.as_bytes()]);
       lockfile_checksum == &compiled_checksum
     } else {
-      false
+      self.insert(specifier, code);
+      true
     }
   }
 
@@ -190,9 +193,10 @@ impl Lockfile {
     }
     let checksum = crate::checksum::gen(&[code.as_bytes()]);
     self.content.remote.insert(specifier.to_string(), checksum);
+    self.has_content_changed = true;
   }
 
-  fn check_npm_package(
+  fn check_or_insert_npm(
     &mut self,
     package: &NpmResolutionPackage,
   ) -> Result<(), LockfileError> {
@@ -211,12 +215,14 @@ impl Lockfile {
           package.id, integrity, package_info.integrity
         )));
       }
+    } else {
+      self.insert_npm(package);
     }
 
     Ok(())
   }
 
-  fn insert_npm_package(&mut self, package: &NpmResolutionPackage) {
+  fn insert_npm(&mut self, package: &NpmResolutionPackage) {
     let dependencies = package
       .dependencies
       .iter()
@@ -235,19 +241,19 @@ impl Lockfile {
         dependencies,
       },
     );
+    self.has_content_changed = true;
   }
 
   pub fn insert_npm_specifier(
     &mut self,
     package_req: &NpmPackageReq,
     version: String,
   ) {
-    if self.write {
-      self.content.npm.specifiers.insert(
-        package_req.to_string(),
-        format!("{}@{}", package_req.name, version),
-      );
-    }
+    self.content.npm.specifiers.insert(
+      package_req.to_string(),
+      format!("{}@{}", package_req.name, version),
+    );
+    self.has_content_changed = true;
   }
 }
 
@@ -290,8 +296,12 @@ pub fn as_maybe_locker(
 #[cfg(test)]
 mod tests {
   use super::*;
+  use crate::npm::NpmPackageId;
+  use crate::npm::NpmPackageVersionDistInfo;
+  use crate::npm::NpmVersion;
   use deno_core::serde_json;
   use deno_core::serde_json::json;
+  use std::collections::HashMap;
   use std::fs::File;
   use std::io::prelude::*;
   use std::io::Write;
@@ -309,7 +319,16 @@ mod tests {
       },
       "npm": {
         "specifiers": {},
-        "packages": {}
+        "packages": {
+          "[email protected]": {
+            "integrity": "sha512-MqBkQh/OHTS2egovRtLk45wEyNXwF+cokD+1YPf9u5VfJiRdAiRwB2froX5Co9Rh20xs4siNPm8naNotSD6RBw==",
+            "dependencies": {}
+          },
+          "[email protected]": {
+            "integrity": "sha512-foobar",
+            "dependencies": {}
+          },
+        }
       }
     });
 
@@ -424,7 +443,7 @@ mod tests {
   }
 
   #[test]
-  fn check_or_insert_lockfile_false() {
+  fn check_or_insert_lockfile() {
     let temp_dir = TempDir::new();
     let file_path = setup(&temp_dir);
 
@@ -443,8 +462,86 @@ mod tests {
 
     let check_false = lockfile.check_or_insert_remote(
       "https://deno.land/[email protected]/textproto/mod.ts",
-      "This is new Source code",
+      "Here is some NEW source code",
     );
     assert!(!check_false);
+
+    // Not present in lockfile yet, should be inserted and check passed.
+    let check_true = lockfile.check_or_insert_remote(
+      "https://deno.land/[email protected]/http/file_server.ts",
+      "This is new Source code",
+    );
+    assert!(check_true);
+  }
+
+  #[test]
+  fn check_or_insert_lockfile_npm() {
+    let temp_dir = TempDir::new();
+    let file_path = setup(&temp_dir);
+
+    let mut lockfile = Lockfile::new(file_path, false).unwrap();
+
+    let npm_package = NpmResolutionPackage {
+      id: NpmPackageId {
+        name: "nanoid".to_string(),
+        version: NpmVersion::parse("3.3.4").unwrap(),
+      },
+      dist: NpmPackageVersionDistInfo {
+        tarball: "foo".to_string(), 
+        shasum: "foo".to_string(), 
+        integrity: Some("sha512-MqBkQh/OHTS2egovRtLk45wEyNXwF+cokD+1YPf9u5VfJiRdAiRwB2froX5Co9Rh20xs4siNPm8naNotSD6RBw==".to_string())
+      },
+      dependencies: HashMap::new(),
+    };
+    let check_ok = lockfile.check_or_insert_npm_package(&npm_package);
+    assert!(check_ok.is_ok());
+
+    let npm_package = NpmResolutionPackage {
+      id: NpmPackageId {
+        name: "picocolors".to_string(),
+        version: NpmVersion::parse("1.0.0").unwrap(),
+      },
+      dist: NpmPackageVersionDistInfo {
+        tarball: "foo".to_string(), 
+        shasum: "foo".to_string(), 
+        integrity: Some("sha512-1fygroTLlHu66zi26VoTDv8yRgm0Fccecssto+MhsZ0D/DGW2sm8E8AjW7NU5VVTRt5GxbeZ5qBuJr+HyLYkjQ==".to_string())
+      },
+      dependencies: HashMap::new(),
+    };
+    // Integrity is borked in the loaded lockfile
+    let check_err = lockfile.check_or_insert_npm_package(&npm_package);
+    assert!(check_err.is_err());
+
+    let npm_package = NpmResolutionPackage {
+      id: NpmPackageId {
+        name: "source-map-js".to_string(),
+        version: NpmVersion::parse("1.0.2").unwrap(),
+      },
+      dist: NpmPackageVersionDistInfo {
+        tarball: "foo".to_string(), 
+        shasum: "foo".to_string(), 
+        integrity: Some("sha512-R0XvVJ9WusLiqTCEiGCmICCMplcCkIwwR11mOSD9CR5u+IXYdiseeEuXCVAjS54zqwkLcPNnmU4OeJ6tUrWhDw==".to_string())
+      },
+      dependencies: HashMap::new(),
+    };
+    // Not present in lockfile yet, should be inserted and check passed.
+    let check_ok = lockfile.check_or_insert_npm_package(&npm_package);
+    assert!(check_ok.is_ok());
+
+    let npm_package = NpmResolutionPackage {
+      id: NpmPackageId {
+        name: "source-map-js".to_string(),
+        version: NpmVersion::parse("1.0.2").unwrap(),
+      },
+      dist: NpmPackageVersionDistInfo {
+        tarball: "foo".to_string(),
+        shasum: "foo".to_string(),
+        integrity: Some("sha512-foobar".to_string()),
+      },
+      dependencies: HashMap::new(),
+    };
+    // Now present in lockfile, should file due to borked integrity
+    let check_err = lockfile.check_or_insert_npm_package(&npm_package);
+    assert!(check_err.is_err());
   }
 }

cli/npm/mod.rs

@@ -7,7 +7,11 @@ mod resolvers;
 mod semver;
 mod tarball;
 
+#[cfg(test)]
+pub use self::semver::NpmVersion;
 pub use cache::NpmCache;
+#[cfg(test)]
+pub use registry::NpmPackageVersionDistInfo;
 pub use registry::NpmRegistryApi;
 pub use resolution::NpmPackageId;
 pub use resolution::NpmPackageReference;

cli/proc_state.rs

@@ -236,7 +236,8 @@ impl ProcState {
       cli_options.cache_setting(),
       progress_bar.clone(),
     );
-    let maybe_lockfile = lockfile.as_ref().filter(|l| !l.lock().write).cloned();
+    let maybe_lockfile =
+      lockfile.as_ref().filter(|l| !l.lock().overwrite).cloned();
     let mut npm_resolver = NpmPackageResolver::new(
       npm_cache.clone(),
       api,