diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index 3b5b178f8..d4f7dfe3c 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/crates/socket-patch-cli/tests/in_process_redirect.rs @@ -5408,6 +5408,95 @@ async fn yarn_berry_unwind_merges_into_a_registry_entry_added_after_pinning() { } } +/// #1435: a hosted berry pin routes each descriptor through its own +/// `resolutions` selector. `yarn remove` in one workspace member, while +/// another still uses the package, keeps the URL-keyed entry and both +/// selectors. `rollback` and `remove` must rebuild the restored key from +/// the descriptors the lock still declares, not from every selector, or +/// every `yarn install --immutable` fails YN0028. A descriptor another +/// entry still declares stays in the key. +#[tokio::test] +#[serial] +async fn yarn_berry_unwind_drops_a_descriptor_no_entry_declares() { + let server = MockServer::start().await; + mock_discovery(&server).await; + let hosted_url = HOSTED_URL.replace("http://patch.test", &server.uri()); + mock_reference_with_berry_url(&server, &hosted_url).await; + mock_view(&server).await; + mock_npm_registry_advertising( + &server, + &vlt_hosted_common::sha512_sri(&upstream_tarball()), + &format!( + "{}/npm-registry/{NAME}/-/{NAME}-{VERSION}.tgz", + server.uri() + ), + ) + .await; + // A workspace member that declares the exact version: its entry is + // yarn's record that the second descriptor is still in use. + let member = format!( + "\n\"member@workspace:packages/member\":\n version: 0.0.0-use.local\n \ + resolution: \"member@workspace:packages/member\"\n dependencies:\n \ + {NAME}: \"npm:{VERSION}\"\n languageName: unknown\n linkType: soft\n" + ); + for (declared, key) in [ + (false, format!("\"{NAME}@npm:^{VERSION}\":")), + ( + true, + format!("\"{NAME}@npm:{VERSION}, {NAME}@npm:^{VERSION}\":"), + ), + ] { + for command in ["rollback", "remove"] { + let tmp = tempfile::tempdir().unwrap(); + write_berry_project(tmp.path()); + let env = run_redirect_subprocess_with( + tmp.path(), + &server.uri(), + &["--patch-server-url", &server.uri()], + ); + assert_eq!(vlt_hosted_common::redirected(&env), 1, "{env:#}"); + // The selector a since-removed member's descriptor left behind. + let pkg_path = tmp.path().join("package.json"); + let mut pkg: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&pkg_path).unwrap()).unwrap(); + pkg["resolutions"][format!("{NAME}@npm:{VERSION}")] = + serde_json::Value::String(hosted_url.clone()); + let lock_path = tmp.path().join("yarn.lock"); + if declared { + pkg["workspaces"] = serde_json::json!(["packages/*"]); + let dir = tmp.path().join("packages").join("member"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("package.json"), + format!( + r#"{{ "name": "member", "version": "0.0.0", "dependencies": {{ "{NAME}": "{VERSION}" }} }}"# + ), + ) + .unwrap(); + let pinned = std::fs::read_to_string(&lock_path).unwrap(); + std::fs::write(&lock_path, format!("{pinned}{member}")).unwrap(); + } + std::fs::write(&pkg_path, serde_json::to_string_pretty(&pkg).unwrap()).unwrap(); + + let args: &[&str] = if command == "rollback" { + &["rollback"] + } else { + &["remove", PURL] + }; + let (code, env) = hosted_unwind_json(tmp.path(), &server, &server.uri(), args); + assert_eq!(code, Some(0), "{command} declared={declared}: {env:#}"); + let restored = std::fs::read_to_string(&lock_path).unwrap(); + assert!( + restored.contains(&format!("\n{key}\n")), + "{command} declared={declared}: restored key must be {key}:\n{restored}" + ); + assert!(!restored.contains(&hosted_url), "{restored}"); + let pkg = std::fs::read_to_string(&pkg_path).unwrap(); + assert!(!pkg.contains("resolutions"), "{pkg}"); + } + } +} + /// #908: the restore reads `dist.tarball` from the registry the project /// resolves against (`.yarnrc.yml` `npmRegistryServer`), not from the /// default registry. A mirror whose tarball URLs are off the conventional diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs index 688aa9947..c4406a58c 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs @@ -757,6 +757,8 @@ async fn restore_berry( url_pin: bool, } let mut hits: Vec = Vec::new(); + // What the lock still declares, read once for the URL-keyed hits. + let mut declared: Option> = None; for (i, block) in blocks.iter().enumerate() { let lines = stanza_lines(block); let Some(resolution) = berry_field(&lines, "resolution").map(str::to_string) else { @@ -825,6 +827,20 @@ async fn restore_berry( }) .cloned() .collect(); + // Only descriptors some entry still declares key the restored + // entry: `yarn remove` in one workspace member leaves its + // selector behind while another member keeps the URL entry + // live, and yarn keys the entry by what is declared (#1435). + // The selectors are all dropped below either way. A lock that + // declares none of them (a spelling not read here) keeps every + // selector's descriptor, as before. + let declared = declared.get_or_insert_with(|| berry_declared_descriptors(&blocks)); + if descriptors + .iter() + .any(|d| berry_descriptor_declared(declared, d)) + { + descriptors.retain(|d| berry_descriptor_declared(declared, d)); + } if descriptors.is_empty() { result.refuse( &uuid, @@ -1051,6 +1067,52 @@ async fn restore_berry( view.write(rel, doc.render(&moved)); } +/// The descriptors a berry lock's `stanzas` still declare: every +/// entry's `dependencies:` map, plus the descriptor a `patch:` descriptor +/// wraps (`name@patch:name@npm%3A1.3.0#…` wraps `name@npm:1.3.0`), in +/// either a dependency map or a key. Yarn 4 spells a registry range +/// `npm:range`, yarn 2 and 3 a bare `range`; see +/// [`berry_descriptor_declared`]. +fn berry_declared_descriptors(stanzas: &[String]) -> BTreeSet { + use crate::formats::yarn::berry_prune::dependency_descriptors; + use crate::formats::yarn::patterns::split_berry_key_patterns; + use crate::formats::yarn::stanzas::stanza_key; + + let mut out = BTreeSet::new(); + for stanza in stanzas { + let keys = stanza_key(stanza) + .map(split_berry_key_patterns) + .unwrap_or_default(); + for descriptor in dependency_descriptors(stanza) { + if let Some(inner) = berry_patch_inner(&descriptor) { + out.insert(inner); + } + out.insert(descriptor); + } + out.extend(keys.iter().filter_map(|k| berry_patch_inner(k))); + } + out +} + +/// The descriptor a `name@patch:#` descriptor wraps, with +/// yarn's percent-encoding of the inner `:` undone. +fn berry_patch_inner(descriptor: &str) -> Option { + let (_, rest) = descriptor.split_once("@patch:")?; + let inner = rest.split('#').next().unwrap_or(rest); + Some(inner.replace("%3A", ":")) +} + +/// Whether the `name@npm:range` `descriptor` is in `declared` +/// ([`berry_declared_descriptors`]), in yarn 4's spelling or yarn 2/3's +/// bare `name@range`. +fn berry_descriptor_declared(declared: &BTreeSet, descriptor: &str) -> bool { + use crate::formats::yarn::patterns::split_pattern; + declared.contains(descriptor) + || split_pattern(descriptor) + .and_then(|(name, range)| Some(format!("{name}@{}", range.strip_prefix("npm:")?))) + .is_some_and(|bare| declared.contains(&bare)) +} + /// Retire the leftover hosted `resolutions` selectors of a yarn berry root /// `package.json` (#1203): `yarn remove` / `yarn up` deleted the lock entry /// a hosted pin keyed by its tarball URL but left its selector, which now @@ -2723,6 +2785,55 @@ mod tests { }; use crate::patch::redirect::upstream::client::NpmDist; + /// #1435: what still declares a descriptor the hosted restore keys + /// the registry entry by. + #[test] + fn berry_descriptor_declared_reads_what_the_lock_still_uses() { + let declared = |stanzas: &[&str], d: &str| { + let stanzas: Vec = stanzas.iter().map(|s| s.to_string()).collect(); + super::berry_descriptor_declared(&super::berry_declared_descriptors(&stanzas), d) + }; + let url_entry = "\"left-pad@http://patch.test/left-pad-1.3.0.tgz\":\n version: 1.3.0\n \ + resolution: \"left-pad@http://patch.test/left-pad-1.3.0.tgz\""; + let member = |dep: &str| { + format!( + "\"b@workspace:packages/b\":\n version: 0.0.0-use.local\n dependencies:\n \ + {dep}\n linkType: soft" + ) + }; + // Yarn 4 spells the range `npm:range`; yarn 2 and 3 a bare one. + let yarn4 = member("left-pad: \"npm:^1.3.0\""); + let yarn3 = member("left-pad: ^1.3.0"); + assert!(declared(&[url_entry, &yarn4], "left-pad@npm:^1.3.0")); + assert!(declared(&[url_entry, &yarn3], "left-pad@npm:^1.3.0")); + // The member a `yarn remove` emptied declares nothing. + assert!(!declared(&[url_entry, &yarn4], "left-pad@npm:1.3.0")); + assert!(!declared(&[url_entry, &yarn3], "left-pad@npm:1.3.0")); + // A `yarn patch` of the descriptor still uses it, in a dependency + // map or a key. + let patch = "left-pad@patch:left-pad@npm%3A1.3.0#~/.yarn/patches/x.patch"; + let patched = format!("\"{patch}\":\n version: 1.3.0"); + assert!(declared(&[url_entry, &patched], "left-pad@npm:1.3.0")); + let patch_dep = member(&format!("left-pad: \"{}\"", &patch["left-pad@".len()..])); + assert!(declared(&[url_entry, &patch_dep], "left-pad@npm:1.3.0")); + // An alias, a scoped namesake and a longer range declare other + // descriptors. + for other in [ + member("lp: \"npm:left-pad@1.3.0\""), + member("\"@acme/left-pad\": \"npm:1.3.0\""), + member("left-pad: \"npm:1.3.0-rc.1\""), + "\"@acme/left-pad@patch:@acme/left-pad@npm%3A1.3.0#x.patch\":\n version: 1.3.0" + .to_string(), + "\"left-pad@patch:left-pad@npm%3A1.3.0-rc.1#x.patch\":\n version: 1.3.0-rc.1" + .to_string(), + ] { + assert!( + !declared(&[url_entry, &other], "left-pad@npm:1.3.0"), + "{other}" + ); + } + } + fn files(npmrc: &[(&str, bool)], bunfig: &[(&str, bool)]) -> super::BunConfigFiles { let layer = |list: &[(&str, bool)]| { list.iter()