bun patch: select an aliased dependency by its package name too

src/install/PackageManager/patchPackage.rs

@@ -16,6 +16,7 @@ use crate::isolated_install::FileCopier;
 use crate::lockfile_real::package::{Package, PackageColumns as _};
 use crate::lockfile_real::tree;
 use crate::lockfile_real::{self as lockfile, Lockfile, PackageIndexEntry};
+use crate::package_installer::alias_is_safe_install_target;
 use crate::package_manager_real::PackageManager;
 use crate::package_manager_real::options::{LogLevel, PatchFeatures};
 use crate::package_manager_real::package_manager_directories::{
@@ -254,12 +255,12 @@ pub fn do_patch_commit(
         }
         PatchArgKind::NameAndVersion => 'brk: {
             let (name, version) = Dependency::split_name_and_maybe_version(argument);
-            let (pkg_id, node_modules_relative_path) =
+            let (pkg_id, node_modules_relative_path, folder_name) =
                 pkg_info_for_name_and_version(&lockfile, &mut iterator, argument, name, version);
 
             let changes_dir = resolve_path::join_z_buf::<platform::Auto>(
                 &mut pathbuf[..],
-                &[&node_modules_relative_path, name],
+                &[&node_modules_relative_path, &folder_name],
             )
             .as_bytes()
             .to_vec();
@@ -879,7 +880,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> {
                 let mut iterator = tree::Iterator::<{ tree::IteratorPathStyle::NodeModules }>::init(
                     &manager.lockfile,
                 );
-                let (pkg_id, folder_relative_path) = pkg_info_for_name_and_version(
+                let (pkg_id, folder_relative_path, folder_name) = pkg_info_for_name_and_version(
                     &manager.lockfile,
                     &mut iterator,
                     pkg_maybe_version_to_patch,
@@ -896,7 +897,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> {
                     write!(
                         &mut name_and_version,
                         "{}@{}",
-                        bstr::BStr::new(name),
+                        bstr::BStr::new(&pkg_name),
                         pkg.resolution.fmt(strbuf, PathSep::Posix)
                     )
                     .expect("unreachable");
@@ -926,7 +927,7 @@ pub fn prepare_patch(manager: &mut PackageManager) -> Result<(), crate::Error> {
                 let cache_dir_subpath = cache_result.cache_dir_subpath;
 
                 let module_folder_ =
-                    resolve_path::join::<platform::Auto>(&[&folder_relative_path, name]);
+                    resolve_path::join::<platform::Auto>(&[&folder_relative_path, &folder_name]);
                 #[cfg(windows)]
                 let buf =
                     resolve_path::path_to_posix_buf::<u8>(module_folder_, &mut win_normalizer[..])
@@ -1220,18 +1221,14 @@ type NodeModulesIterator<'a> = tree::Iterator<'a, { tree::IteratorPathStyle::Nod
 fn node_modules_folder_for_dependency_ids(
     iterator: &mut NodeModulesIterator<'_>,
     ids: &[IdPair],
-) -> Option<Vec<u8>> {
+) -> Option<(DependencyID, Vec<u8>)> {
     loop {
         let node_modules = iterator.next(None)?;
-        let mut found = false;
-        for id in ids {
-            if node_modules.dependencies.contains(&id.0) {
-                found = true;
-                break;
-            }
-        }
-        if found {
-            return Some(node_modules.relative_path.as_bytes().to_vec());
+        if let Some(id) = ids
+            .iter()
+            .find(|id| node_modules.dependencies.contains(&id.0))
+        {
+            return Some((id.0, node_modules.relative_path.as_bytes().to_vec()));
         }
     }
 }
@@ -1251,13 +1248,19 @@ fn node_modules_folder_for_dependency_id(
 
 type IdPair = (DependencyID, PackageID);
 
+/// Returns the package that `name` (and `version`, when given) selects, the node_modules folder
+/// that holds it, and the name of its folder there.
+///
+/// `name` selects a dependency by its name in package.json or by the name of the package it
+/// resolves to. An `npm:` alias makes the two differ: the first names the folder, the second keys
+/// the patch.
 fn pkg_info_for_name_and_version(
     lockfile: &Lockfile,
     iterator: &mut NodeModulesIterator<'_>,
     pkg_maybe_version_to_patch: &[u8],
     name: &[u8],
     version: Option<&[u8]>,
-) -> (PackageID, Vec<u8>) {
+) -> (PackageID, Vec<u8>, Vec<u8>) {
     let mut pairs: Vec<IdPair> = Vec::with_capacity(8);
 
     let name_hash = string_hash(name);
@@ -1266,15 +1269,28 @@ fn pkg_info_for_name_and_version(
 
     let mut resolution_label = Vec::new();
     let dependencies = lockfile.buffers.dependencies.as_slice();
+    let pkg_name_hashes = lockfile.packages.items_name_hash();
+    // The name becomes a path that `bun patch` deletes and writes, the same as an install.
+    let folder_name = |dep_id: DependencyID| {
+        let folder_name = dependencies[dep_id as usize].name.slice(strbuf);
+        if !alias_is_safe_install_target(folder_name) {
+            bun_core::pretty_errorln!(
+                "<r><red>error<r>: refusing to patch dependency with unsafe name <b>{}<r>",
+                bstr::BStr::new(folder_name),
+            );
+            Global::crash();
+        }
+        folder_name.to_vec()
+    };
 
     for (dep_id, dep) in dependencies.iter().enumerate() {
-        if dep.name_hash != name_hash {
-            continue;
-        }
         let pkg_id = lockfile.buffers.resolutions.as_slice()[dep_id];
         if pkg_id == invalid_package_id {
             continue;
         }
+        if dep.name_hash != name_hash && pkg_name_hashes[pkg_id as usize] != name_hash {
+            continue;
+        }
         let pkg = *lockfile.packages.get(pkg_id as usize);
         if let Some(v) = version {
             if print_resolution_label(&mut resolution_label, &pkg.resolution, strbuf) == v {
@@ -1285,6 +1301,10 @@ fn pkg_info_for_name_and_version(
         }
     }
 
+    // One node_modules folder can hold the package as `name` and under an alias. The folder lookup
+    // takes the first pair it finds there, so the folder that has `name` goes first.
+    pairs.sort_by_key(|&(dep_id, _)| dependencies[dep_id as usize].name_hash != name_hash);
+
     if pairs.is_empty() {
         bun_core::pretty_errorln!(
             "\n<r><red>error<r>: package <b>{}<r> not found<r>",
@@ -1307,14 +1327,14 @@ fn pkg_info_for_name_and_version(
                     Global::crash();
                 }
             };
-            return (pkg_id, folder);
+            return (pkg_id, folder, folder_name(dep_id));
         }
 
         // we found multiple dependents of the supplied pkg + version
         // the final package in the node_modules might be hoisted
         // so we are going to try looking for each dep id in node_modules
         let (_, pkg_id) = pairs[0];
-        let folder = match node_modules_folder_for_dependency_ids(iterator, &pairs) {
+        let (dep_id, folder) = match node_modules_folder_for_dependency_ids(iterator, &pairs) {
             Some(f) => f,
             None => {
                 bun_core::pretty_error!(
@@ -1325,7 +1345,7 @@ fn pkg_info_for_name_and_version(
             }
         };
 
-        return (pkg_id, folder);
+        return (pkg_id, folder, folder_name(dep_id));
     }
 
     // Otherwise the user did not supply a version, just the pkg name
@@ -1343,7 +1363,7 @@ fn pkg_info_for_name_and_version(
                 Global::crash();
             }
         };
-        return (pkg_id, folder);
+        return (pkg_id, folder, folder_name(dep_id));
     }
 
     // Otherwise we have multiple matches
@@ -1366,7 +1386,7 @@ fn pkg_info_for_name_and_version(
     // Disambiguate case a) from b)
     if count as usize == pairs.len() {
         // It may be hoisted, so we'll try the first one that matches
-        let folder = match node_modules_folder_for_dependency_ids(iterator, &pairs) {
+        let (dep_id, folder) = match node_modules_folder_for_dependency_ids(iterator, &pairs) {
             Some(f) => f,
             None => {
                 bun_core::pretty_error!(
@@ -1376,7 +1396,7 @@ fn pkg_info_for_name_and_version(
                 Global::crash();
             }
         };
-        return (pkg_id, folder);
+        return (pkg_id, folder, folder_name(dep_id));
     }
 
     bun_core::pretty_errorln!(

test/cli/install/bun-patch.test.ts

@@ -1,7 +1,7 @@
 import { $, ShellOutput } from "bun";
 import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test";
 import { lstatSync, readFileSync } from "fs";
-import { bunEnv, bunExe, isASAN, tempDir, VerdaccioRegistry } from "harness";
+import { bunEnv, bunExe, isASAN, readdirSorted, tempDir, VerdaccioRegistry } from "harness";
 import { isAbsolute, join, sep } from "path";
 
 const expectNoError = (o: ShellOutput) => expect(o.stderr.toString()).not.toContain("error");
@@ -1234,3 +1234,140 @@ describe.concurrent("bun patch --commit for non-registry dependencies", () => {
     expect(patchKey).toBe("pkg-to-patch@./dep.tgz");
   });
 });
+
+// `"my-alias": "npm:no-deps@1.0.0"` installs the package `no-deps` into `node_modules/my-alias`.
+// `bun patch` prints the package's own name and keys the patch by it (`no-deps@1.0.0`), so that
+// name has to select the package too, not only the alias.
+describe("an npm: aliased dependency", () => {
+  const registry = new VerdaccioRegistry();
+
+  beforeAll(async () => {
+    await registry.start();
+  });
+
+  afterAll(() => {
+    registry.stop();
+  });
+
+  // CI exports BUN_INSTALL_CACHE_DIR, which overrides the harness bunfig's per-test `cache`.
+  async function runBun(cwd: string, ...args: string[]) {
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), ...args],
+      cwd,
+      env: { ...bunEnv, BUN_INSTALL_CACHE_DIR: join(cwd, ".bun-cache") },
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    return { stdout, stderr, exitCode };
+  }
+
+  async function writeProject(dependencies: Record<string, string>) {
+    const { packageDir } = await registry.createTestDir({
+      bunfigOpts: { linker: "hoisted" },
+      files: { "package.json": JSON.stringify({ name: "root", dependencies }) },
+    });
+    return packageDir;
+  }
+
+  async function createProject(dependencies: Record<string, string>) {
+    const packageDir = await writeProject(dependencies);
+    const { stderr, exitCode } = await runBun(packageDir, "install");
+    expect(stderr).not.toContain("error:");
+    expect(exitCode).toBe(0);
+    return packageDir;
+  }
+
+  async function expectPrepared(packageDir: string, argument: string, folder: string) {
+    const { stdout, stderr, exitCode } = await runBun(packageDir, "patch", argument);
+    expect(stderr).not.toContain("error:");
+    expect(stdout).toContain(`To patch no-deps, edit the following folder:\n\n  ${folder}\n`);
+    expect(stdout).toContain(`bun patch --commit '${folder}'`);
+    expect(exitCode).toBe(0);
+  }
+
+  for (const argument of ["my-alias", "my-alias@1.0.0", "no-deps", "no-deps@1.0.0"]) {
+    test.concurrent(`bun patch ${argument}`, async () => {
+      const packageDir = await createProject({ "my-alias": "npm:no-deps@1.0.0" });
+      await expectPrepared(packageDir, argument, "node_modules/my-alias");
+      expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["my-alias"]);
+    });
+  }
+
+  test.concurrent("bun patch --commit <package name>", async () => {
+    const packageDir = await createProject({ "my-alias": "npm:no-deps@1.0.0" });
+    await expectPrepared(packageDir, "no-deps", "node_modules/my-alias");
+    await Bun.write(join(packageDir, "node_modules", "my-alias", "index.js"), "module.exports = 'patched';\n");
+
+    const { stderr, exitCode } = await runBun(packageDir, "patch", "--commit", "no-deps");
+    expect(stderr).not.toContain("error:");
+    expect(exitCode).toBe(0);
+    expect((await Bun.file(join(packageDir, "package.json")).json()).patchedDependencies).toEqual({
+      "no-deps@1.0.0": "patches/no-deps@1.0.0.patch",
+    });
+    expect(await Bun.file(join(packageDir, "patches", "no-deps@1.0.0.patch")).text()).toContain(
+      "+module.exports = 'patched';",
+    );
+    expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["my-alias"]);
+  });
+
+  // A second `bun patch` starts from the patched package, so the next commit keeps the first change.
+  for (const argument of ["my-alias", "no-deps"]) {
+    test.concurrent(`bun patch ${argument} of a package that is already patched`, async () => {
+      const packageDir = await createProject({ "my-alias": "npm:no-deps@1.0.0" });
+      const index = join(packageDir, "node_modules", "my-alias", "index.js");
+      await expectPrepared(packageDir, argument, "node_modules/my-alias");
+      await Bun.write(index, "module.exports = 'patched';\n");
+      const { stderr, exitCode } = await runBun(packageDir, "patch", "--commit", "node_modules/my-alias");
+      expect(stderr).not.toContain("error:");
+      expect(exitCode).toBe(0);
+
+      await expectPrepared(packageDir, argument, "node_modules/my-alias");
+      expect(await Bun.file(index).text()).toBe("module.exports = 'patched';\n");
+    });
+  }
+
+  test.concurrent("bun patch <package name> asks for a version when an alias installs another one", async () => {
+    const packageDir = await createProject({ "my-alias": "npm:no-deps@1.0.0", "no-deps": "2.0.0" });
+
+    const { stdout, stderr, exitCode } = await runBun(packageDir, "patch", "no-deps");
+    expect(stderr).toContain(
+      "error: Found multiple versions of no-deps, please specify a precise version from the following list:\n" +
+        "  no-deps@2.0.0\n" +
+        "  no-deps@1.0.0\n",
+    );
+    expect(stdout).not.toContain("To patch");
+    expect(exitCode).toBe(1);
+
+    await expectPrepared(packageDir, "no-deps@1.0.0", "node_modules/my-alias");
+    await expectPrepared(packageDir, "no-deps@2.0.0", "node_modules/no-deps");
+    await expectPrepared(packageDir, "my-alias", "node_modules/my-alias");
+  });
+
+  test.concurrent(
+    "bun patch <package name> prefers the folder with that name to an alias of the same package",
+    async () => {
+      const packageDir = await createProject({ "my-alias": "npm:no-deps@1.0.0", "no-deps": "1.0.0" });
+      await expectPrepared(packageDir, "no-deps", "node_modules/no-deps");
+      await expectPrepared(packageDir, "no-deps@1.0.0", "node_modules/no-deps");
+      await expectPrepared(packageDir, "my-alias", "node_modules/my-alias");
+    },
+  );
+
+  // The alias names the folder. `bun install` refuses to write `node_modules/a/b`, because only
+  // `@scope/name` can have a `/`, so `bun patch` does not create that folder either.
+  for (const argument of ["no-deps", "a/b"]) {
+    test.concurrent(`bun patch ${argument} refuses an alias that bun install refuses`, async () => {
+      const packageDir = await writeProject({ "a/b": "npm:no-deps@1.0.0" });
+      const install = await runBun(packageDir, "install");
+      expect(install.stderr).toContain("error: refusing to install dependency with unsafe name a/b\n");
+      expect(install.exitCode).toBe(1);
+
+      const { stdout, stderr, exitCode } = await runBun(packageDir, "patch", argument);
+      expect(stderr).toContain("error: refusing to patch dependency with unsafe name a/b\n");
+      expect(stdout).not.toContain("To patch");
+      expect(exitCode).toBe(1);
+      expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual([]);
+    });
+  }
+});