diff --git a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs index d962d5919..0ae7fd482 100644 --- a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs +++ b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs @@ -53,13 +53,17 @@ pub(crate) fn with_manifest_dep_maps(lines: &[String], pkg: &Value) -> Vec Vec { - unlocked_among( - blocks, - manifest_dep_maps(pkg) - .into_iter() - .flat_map(|(_, deps)| deps) - .map(|(name, range)| format!("{name}@{range}")), - ) + unlocked_among(blocks, manifest_descriptors(pkg)) +} + +/// [`unlocked_descriptors`] against `locked`, every pattern the lock's +/// keys hold: for a caller that checks many manifests against one lock +/// and splits its keys once, not once per manifest (#1423). +pub(crate) fn unlocked_descriptors_in( + locked: &std::collections::HashSet<&str>, + pkg: &Value, +) -> Vec { + sorted_missing(manifest_descriptors(pkg), |d| locked.contains(d)) } /// Every descriptor of `descriptors` no block of `blocks` is keyed by, @@ -72,9 +76,23 @@ pub(crate) fn unlocked_among( .iter() .flat_map(|b| split_key_patterns(&b.key)) .collect(); - let mut missing: Vec = descriptors - .filter(|descriptor| !locked.contains(descriptor)) - .collect(); + sorted_missing(descriptors, |d| locked.contains(d)) +} + +/// The descriptors `pkg`'s dependency maps declare, as `name@range`. +fn manifest_descriptors(pkg: &Value) -> impl Iterator { + manifest_dep_maps(pkg) + .into_iter() + .flat_map(|(_, deps)| deps) + .map(|(name, range)| format!("{name}@{range}")) +} + +/// The descriptors `is_locked` rejects, sorted and deduplicated. +fn sorted_missing( + descriptors: impl Iterator, + is_locked: impl Fn(&str) -> bool, +) -> Vec { + let mut missing: Vec = descriptors.filter(|d| !is_locked(d)).collect(); missing.sort(); missing.dedup(); missing @@ -213,6 +231,31 @@ is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n" assert!(unlocked_descriptors(&blocks, &serde_json::json!({})).is_empty()); } + /// #1423: the pre-split form agrees with [`unlocked_descriptors`]. + #[test] + fn unlocked_descriptors_in_matches_the_block_scan() { + let blocks = scan_blocks(LOCK); + let patterns: Vec = blocks + .iter() + .flat_map(|b| split_key_patterns(&b.key)) + .collect(); + let locked: std::collections::HashSet<&str> = patterns.iter().map(String::as_str).collect(); + for pkg in [ + serde_json::json!({ + "dependencies": {"is-number": "^7.0.0", "wow": "^1.0.0"}, + "optionalDependencies": {"@scope/opt": "^2.0.0"}, + }), + serde_json::json!({"dependencies": {"is-number": "^6.0.0"}}), + serde_json::json!({}), + ] { + assert_eq!( + unlocked_descriptors_in(&locked, &pkg), + unlocked_descriptors(&blocks, &pkg), + "{pkg}" + ); + } + } + #[test] fn block_dep_descriptors_reads_the_blocks_own_sub_maps() { // #1379: a block a pre-v5 vendor rewrote to a patched manifest diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 88a7fdecd..14e67b34d 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -19,7 +19,7 @@ //! network: every host effect (locking, probes, record fetches, the commit //! of the rewritten files) stays with the caller. -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use serde::{Deserialize, Serialize}; @@ -1212,40 +1212,82 @@ pub fn yarn_classic_artifact_targets<'a>( if crate::patch::redirect::yarn_classic_hosted_refused(files, yarn_outer) { return Vec::new(); } + select_classic_artifacts(candidates, lock, classic_registry_copies) +} + +/// [`yarn_classic_artifact_targets`] over a classic `lock`, reading its +/// registry copies through `read_copies`. The lock is read in one pass for +/// the artifacts it pins already, once a dep with a sha1 asks; the +/// registry-copy test reads every block, so it runs after that, at most +/// once per lock, and only when some dep still needs it (#1423): a rescan +/// whose grants all carry a sha1 and are pinned already never reads the +/// blocks. +fn select_classic_artifacts<'a>( + candidates: &'a [Candidate], + lock: &str, + read_copies: impl FnOnce(&str) -> HashSet<(String, String)>, +) -> Vec<&'a DepOverride> { + let mut pinned: Option> = None; + let mut read_copies = Some(read_copies); + let mut registry_copies: Option> = None; let mut seen = BTreeSet::new(); candidates .iter() .map(|c| &c.dep) .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) .filter(|dep| { - classic_locks_registry_copy(lock, &crate::patch::redirect::full_name(dep), &dep.version) + dep.integrity.sha1.is_none() + || !pinned + .get_or_insert_with(|| quoted_fragment_urls(lock)) + .contains(dep.artifact_url.as_str()) }) .filter(|dep| { - dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) + registry_copies + .get_or_insert_with(|| read_copies.take().map_or_else(HashSet::new, |r| r(lock))) + .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() } -/// Whether a classic `lock` has a registry block of `name@version`: the -/// only copy a hosted pin rewrites (a git, `file:`, `link:` or remote +/// Every `` that `text` writes as `"#…`: an opening quote, then +/// text with no quote, `#` or line break, then a `#` (a `resolved` URL +/// with its fragment). One pass, so asking whether the lock pins an +/// artifact is a set lookup rather than a search of the whole lock. +fn quoted_fragment_urls(text: &str) -> HashSet<&str> { + let mut urls = HashSet::new(); + let mut rest = text; + while let Some(open) = rest.find('"') { + let after = &rest[open + 1..]; + let end = after.find(['"', '#', '\n']).unwrap_or(after.len()); + if after[end..].starts_with('#') { + urls.insert(&after[..end]); + } + rest = &after[end..]; + } + urls +} + +/// The `(name, version)` of every registry block in a classic `lock`: the +/// only copies a hosted pin rewrites (a git, `file:`, `link:` or remote /// tarball copy is skipped by name, so it needs no served tarball). Read /// by the blocks' real names, as the rewriter does, so `lodash` never /// matches a `lodash.debounce` block. -fn classic_locks_registry_copy(lock: &str, name: &str, version: &str) -> bool { +fn classic_registry_copies(lock: &str) -> HashSet<(String, String)> { use crate::formats::yarn::blocks::{classic_field, scan_blocks}; use crate::formats::yarn::patterns::{classic_key_real_name, split_key_patterns}; use crate::formats::yarn::source::{classic_copy_source, CopySource}; - if !lock.contains(name) { - return false; - } - scan_blocks(lock).iter().any(|block| { - let patterns = split_key_patterns(&block.key); - classic_key_real_name(&patterns) == Some(name) - && classic_field(&block.lines, "version") == Some(version) - && classic_copy_source(&patterns, classic_field(&block.lines, "resolved")) - == CopySource::Registry - }) + scan_blocks(lock) + .iter() + .filter_map(|block| { + let patterns = split_key_patterns(&block.key); + let name = classic_key_real_name(&patterns)?; + let version = classic_field(&block.lines, "version")?; + (classic_copy_source(&patterns, classic_field(&block.lines, "resolved")) + == CopySource::Registry) + .then(|| (name.to_string(), version.to_string())) + }) + .collect() } /// Record what the served tarball at `url` yielded: its sha1 on every @@ -3088,15 +3130,104 @@ mod tests { resolved \"git+https://github.com/x/left-pad.git#abc\"\n\n\ is-odd@^3.0.0:\n version \"3.0.1\"\n \ resolved \"https://registry.yarnpkg.com/is-odd/-/is-odd-3.0.1.tgz#bb\"\n"; - assert!(!classic_locks_registry_copy(lock, "lodash", "4.17.21")); - assert!(!classic_locks_registry_copy(lock, "left-pad", "1.3.0")); - assert!(!classic_locks_registry_copy(lock, "is-odd", "3.0.0")); - assert!(classic_locks_registry_copy(lock, "is-odd", "3.0.1")); - assert!(classic_locks_registry_copy( - lock, - "lodash.debounce", - "4.17.21" - )); + let copies = classic_registry_copies(lock); + let has = |name: &str, version: &str| copies.contains(&(name.into(), version.into())); + assert!(!has("lodash", "4.17.21")); + assert!(!has("left-pad", "1.3.0")); + assert!(!has("is-odd", "3.0.0")); + assert!(has("is-odd", "3.0.1")); + assert!(has("lodash.debounce", "4.17.21")); + assert_eq!(copies.len(), 2); + } + + fn classic_candidate(name: &str, version: &str, sha1: Option<&str>) -> Candidate { + let url = format!("https://h/serve/{GRANT}/{name}-{version}.tgz"); + let mut dep = grant_dep("npm", url); + dep.name = name.into(); + dep.version = version.into(); + dep.integrity.sha512 = Some("sha512-x".into()); + dep.integrity.sha1 = sha1.map(Into::into); + Candidate { + purl: format!("pkg:npm/{name}@{version}"), + dep, + } + } + + #[test] + fn quoted_fragment_urls_reads_each_resolved_url() { + let lock = "a@^1:\n resolved \"https://r/a.tgz#aa\"\n\ + b@^1:\n resolved \"https://r/b.tgz\"\n\ + \"c@^1\", \"c@^2\":\n resolved \"https://r/c.tgz#cc\"\n"; + let urls = quoted_fragment_urls(lock); + assert_eq!( + urls, + HashSet::from(["https://r/a.tgz", "https://r/c.tgz"]), + "{urls:?}" + ); + } + + /// #1423: the served tarball is read only for a dep the lock locks as + /// a registry copy and doesn't pin to the artifact yet. The lock's + /// blocks are read at most once however many deps there are, and not + /// at all on a rescan whose deps are all pinned already. + #[test] + fn classic_artifact_targets_read_the_lock_blocks_at_most_once() { + let mut lock = String::from("# yarn lockfile v1\n\n"); + let n = 200; + for i in 0..n { + lock.push_str(&format!( + "pkg-{i}@^1.0.0:\n version \"1.0.{i}\"\n \ + resolved \"https://registry.yarnpkg.com/pkg-{i}/-/pkg-{i}-1.0.{i}.tgz#aa\"\n\n" + )); + } + lock.push_str( + "gitdep@git+https://github.com/x/gitdep.git:\n version \"2.0.0\"\n \ + resolved \"git+https://github.com/x/gitdep.git#abc\"\n", + ); + let mut candidates: Vec = (0..n) + .map(|i| classic_candidate(&format!("pkg-{i}"), &format!("1.0.{i}"), Some("s1"))) + .collect(); + let reads = std::cell::Cell::new(0); + let counted = |lock: &str| { + reads.set(reads.get() + 1); + classic_registry_copies(lock) + }; + + // Every dep pinned to its artifact already: nothing to fetch, and + // the blocks are never read. + let mut pinned = lock.clone(); + for c in &candidates { + pinned.push_str(&format!("# \"{}#s1\"\n", c.dep.artifact_url)); + } + assert!(select_classic_artifacts(&candidates, &pinned, counted).is_empty()); + assert_eq!(reads.get(), 0); + + // Unpinned: every registry copy is selected once per artifact; a + // version the lock doesn't lock, a name it doesn't have, a git copy + // and a non-npm dep never. One read of the blocks for all of them. + candidates.push(classic_candidate("pkg-0", "9.9.9", None)); + candidates.push(classic_candidate("absent", "1.0.0", None)); + candidates.push(classic_candidate("gitdep", "2.0.0", None)); + candidates.push(classic_candidate("pkg-1", "1.0.1", Some("s1"))); + let mut pypi = classic_candidate("pkg-2", "1.0.2", None); + pypi.dep.ecosystem = "pypi".into(); + candidates.push(pypi); + let got: Vec = select_classic_artifacts(&candidates, &lock, counted) + .into_iter() + .map(|d| format!("{}@{}", d.name, d.version)) + .collect(); + assert_eq!(reads.get(), 1); + assert_eq!(got.len(), n); + assert_eq!(got[0], "pkg-0@1.0.0"); + assert_eq!(got[n - 1], format!("pkg-{}@1.0.{}", n - 1, n - 1)); + + // The public entry point selects the same deps from the files. + let files = BTreeMap::from([("yarn.lock".to_string(), lock)]); + let outer = OuterYarnMirror::default(); + assert_eq!( + yarn_classic_artifact_targets(&candidates, &files, &outer).len(), + n + ); } #[test] diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 7a8d54452..ea8687474 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -3967,6 +3967,9 @@ fn rewrite_yarn_classic_with( // byte span once, at the end: re-scanning and re-copying the whole lock // per pinned block is quadratic in a large lock. let mut pinned_blocks: Vec = vec![false; blocks.len()]; + // Every pattern the keys hold, for the served-manifest check (#591): + // gathered from `heads` once, when the first dep needs it (#1423). + let mut locked_patterns: Option> = None; let mut segments: Option = None; let mut changed = false; let mut any_pinned = false; @@ -4007,7 +4010,13 @@ fn rewrite_yarn_classic_with( .and_then(|text| serde_json::from_str::(text).ok()) .filter(Value::is_object); if let Some(pkg) = &served_manifest { - let missing = crate::formats::yarn::classic_deps::unlocked_descriptors(&blocks, pkg); + let locked = locked_patterns.get_or_insert_with(|| { + heads + .iter() + .flat_map(|(patterns, _)| patterns.iter().map(String::as_str)) + .collect() + }); + let missing = crate::formats::yarn::classic_deps::unlocked_descriptors_in(locked, pkg); if !missing.is_empty() { result .refused_yarn_classic_uuids