fix up patch-commit handling of unconventional tarballs

Author: spanishpear

.yarn/versions/34906c89.yml

@@ -1,6 +1,7 @@
 releases:
   "@yarnpkg/cli": patch
   "@yarnpkg/plugin-npm": patch
+  "@yarnpkg/plugin-patch": minor
 
 declined:
   - "@yarnpkg/plugin-compat"
@@ -12,7 +13,6 @@ declined:
   - "@yarnpkg/plugin-nm"
   - "@yarnpkg/plugin-npm-cli"
   - "@yarnpkg/plugin-pack"
-  - "@yarnpkg/plugin-patch"
   - "@yarnpkg/plugin-pnp"
   - "@yarnpkg/plugin-pnpm"
   - "@yarnpkg/plugin-stage"

packages/acceptance-tests/pkg-tests-specs/sources/commands/patchCommit.test.ts

@@ -1,5 +1,10 @@
 import {Filename, npath, ppath, xfs} from '@yarnpkg/fslib';
 
+const {
+  tests: {startPackageServer},
+  fs: {writeFile},
+} = require(`pkg-tests-core`);
+
 describe(`Commands`, () => {
   describe(`patch-commit`, () => {
     test(
@@ -200,5 +205,100 @@ describe(`Commands`, () => {
         });
       }),
     );
+
+    test(
+      `it should preserve __archiveUrl parameters in patch URLs`,
+      makeTemporaryEnv({
+        dependencies: {
+          [`unconventional-tarball`]: `npm:1.0.0::__archiveUrl=https://registry.example.org/unconventional-tarball/tralala/unconventional-tarball-1.0.0.tgz`,
+        },
+      }, async ({path, run}) => {
+        await run(`install`);
+
+        const {stdout} = await run(`patch`, `unconventional-tarball`, `--json`);
+        const {path: updateFolderN} = JSON.parse(stdout);
+
+        const updateFolder = npath.toPortablePath(updateFolderN);
+        const updateFile = ppath.join(updateFolder, `index.js`);
+
+        const fileSource = await xfs.readFilePromise(updateFile, `utf8`);
+        const fileUser = fileSource.replace(
+          `module.exports = require(\`./package.json\`);`,
+          `module.exports = require(\`./package.json\`);\n\nmodule.exports.hello = \`world\`;`,
+        );
+        await xfs.writeFilePromise(updateFile, fileUser);
+
+        await run(`patch-commit`, `-s`, npath.fromPortablePath(updateFolder));
+
+        const manifest = await xfs.readJsonPromise(ppath.join(path, Filename.manifest));
+
+        expect(manifest.dependencies).toEqual({
+          [`unconventional-tarball`]: expect.stringMatching(/^patch:unconventional-tarball@npm%3A1\.0\.0#.*::__archiveUrl=https%3A%2F%2Fregistry\.example\.org/),
+        });
+
+        // Ensure the patch URL format is correct (not malformed)
+        expect(manifest.dependencies[`unconventional-tarball`]).not.toMatch(/^patch:.*::.*#/);
+      }),
+    );
+
+    test(
+      `it should generate proper patch URLs for packages with unconventional tarball urls`,
+      makeTemporaryEnv(
+        {
+          dependencies: {
+            [`unconventional-tarball`]: `npm:1.0.0::__archiveUrl=https://registry.example.org/unconventional-tarball/tralala/unconventional-tarball-1.0.0.tgz`,
+          },
+        },
+        async ({path, run, source}) => {
+          const url = await startPackageServer({type: `https`});
+
+          await writeFile(ppath.join(path, `.yarnrc.yml`), [
+            `npmScopes:`,
+            `  private:`,
+            `    npmRegistryServer: "${url}"`,
+          ].join(`\n`));
+
+          await run(`install`);
+
+          // Use yarn patch to get a temporary folder
+          const {stdout} = await run(`patch`, `unconventional-tarball`, `--json`);
+          const {path: updateFolderN} = JSON.parse(stdout);
+
+          const updateFolder = npath.toPortablePath(updateFolderN);
+          const updateFile = ppath.join(updateFolder, `index.js`);
+
+          // Make the same modification as our patch
+          const fileSource = await xfs.readFilePromise(updateFile, `utf8`);
+          const fileUser = fileSource.replace(
+            `module.exports = require(\`./package.json\`);`,
+            `module.exports = require(\`./package.json\`);\n\nmodule.exports.hello = \`world\`;`,
+          );
+          await xfs.writeFilePromise(updateFile, fileUser);
+
+          // Commit the patch
+          await run(`patch-commit`, `-s`, npath.fromPortablePath(updateFolder));
+
+          // Read the manifest to check the generated patch URL format
+          const manifest = await xfs.readJsonPromise(ppath.join(path, Filename.manifest));
+          const patchUrl = manifest.dependencies[`unconventional-tarball`];
+
+          // Verify the patch URL has the correct format: patch:source#selector::params
+          expect(patchUrl).toMatch(/^patch:unconventional-tarball@npm%3A1\.0\.0#.*::__archiveUrl=/);
+
+          // Verify it's NOT the malformed format: patch:source::params#selector
+          expect(patchUrl).not.toMatch(/^patch:unconventional-tarball@npm%3A1\.0\.0::.*#.*$/);
+
+          // Run install again to apply the patch
+          await run(`install`);
+
+          // Verify the patch was applied correctly
+          await expect(source(`require('unconventional-tarball')`)).resolves.toMatchObject({
+            name: `unconventional-tarball`,
+            version: `1.0.0`,
+            hello: `world`,
+          });
+        },
+      ),
+    );
   });
 });

packages/acceptance-tests/pkg-tests-specs/sources/protocols/patch.test.ts

@@ -238,5 +238,24 @@ describe(`Protocols`, () => {
         },
       ),
     );
+    test(
+      `it should apply patches on packages with unconventional tarball urls (__archiveUrl)`,
+      makeTemporaryEnv(
+        {
+          dependencies: {[`unconventional-tarball`]: `patch:unconventional-tarball@npm:1.0.0#${PATCH_NAME}::__archiveUrl=https://registry.example.org/unconventional-tarball/tralala/unconventional-tarball-1.0.0.tgz`},
+        },
+        async ({path, run, source}) => {
+          await xfs.writeFilePromise(ppath.join(path, PATCH_NAME), NO_DEPS_PATCH);
+
+          await run(`install`);
+
+          await expect(source(`require('unconventional-tarball')`)).resolves.toMatchObject({
+            name: `unconventional-tarball`,
+            version: `1.0.0`,
+            hello: `world`,
+          });
+        },
+      ),
+    );
   });
 });

packages/plugin-npm/tests/NpmTarballResolver.test.ts

@@ -0,0 +1,70 @@
+import {structUtils}        from '@yarnpkg/core';
+
+import {NpmTarballResolver} from '../sources/NpmTarballResolver';
+
+describe(`NpmTarballResolver`, () => {
+  describe(`supportsDescriptor`, () => {
+    it(`should support npm descriptors with __archiveUrl parameters`, () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const descriptor = structUtils.makeDescriptor(ident, `npm:1.0.0::__archiveUrl=https://registry.example.org/test-package-1.0.0.tgz`);
+
+      expect(resolver.supportsDescriptor(descriptor, null as any)).toBe(true);
+    });
+
+    it(`should not support npm descriptors without __archiveUrl parameters`, () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const descriptor = structUtils.makeDescriptor(ident, `npm:1.0.0`);
+
+      expect(resolver.supportsDescriptor(descriptor, null as any)).toBe(false);
+    });
+
+    it(`should not support non-npm descriptors`, () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const descriptor = structUtils.makeDescriptor(ident, `file:./local-package.tgz`);
+
+      expect(resolver.supportsDescriptor(descriptor, null as any)).toBe(false);
+    });
+  });
+
+  describe(`supportsLocator`, () => {
+    it(`should not support any locators (handled by NpmSemverResolver)`, () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const locator = structUtils.makeLocator(ident, `npm:1.0.0::__archiveUrl=https://registry.example.org/test-package-1.0.0.tgz`);
+
+      expect(resolver.supportsLocator(locator, null as any)).toBe(false);
+    });
+  });
+
+  describe(`getCandidates`, () => {
+    it(`should convert descriptor to locator`, async () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const descriptor = structUtils.makeDescriptor(ident, `npm:1.0.0::__archiveUrl=https://registry.example.org/test-package-1.0.0.tgz`);
+
+      const candidates = await resolver.getCandidates(descriptor, {}, null as any);
+
+      expect(candidates.length).toBe(1);
+      expect(candidates[0].identHash).toBe(descriptor.identHash);
+    });
+  });
+
+  describe(`getSatisfying`, () => {
+    it(`should filter locators that match the descriptor`, async () => {
+      const resolver = new NpmTarballResolver();
+      const ident = structUtils.makeIdent(null, `test-package`);
+      const descriptor = structUtils.makeDescriptor(ident, `npm:1.0.0::__archiveUrl=https://registry.example.org/test-package-1.0.0.tgz`);
+      const locator1 = structUtils.makeLocator(ident, `npm:1.0.0::__archiveUrl=https://registry.example.org/test-package-1.0.0.tgz`);
+      const locator2 = structUtils.makeLocator(ident, `npm:2.0.0::__archiveUrl=https://registry.example.org/test-package-2.0.0.tgz`);
+
+      const result = await resolver.getSatisfying(descriptor, {}, [locator1, locator2], null as any);
+
+      expect(result.locators.length).toBe(1);
+      expect(result.locators[0].locatorHash).toBe(locator1.locatorHash);
+      expect(result.sorted).toBe(false);
+    });
+  });
+});

packages/plugin-patch/sources/patchUtils.ts

@@ -1,5 +1,6 @@
 import {Cache, structUtils, Locator, Descriptor, Ident, Project, ThrowReport, miscUtils, FetchOptions, Package, execUtils, FetchResult, semverUtils, hashUtils} from '@yarnpkg/core';
 import {npath, PortablePath, xfs, ppath, NativePath, CwdFS}                                                                                                     from '@yarnpkg/fslib';
+import * as querystring                                                                                                                                         from 'querystring';
 
 import {CACHE_VERSION}                                                                                                                                          from './constants';
 import {Hooks as PatchHooks}                                                                                                                                    from './index';
@@ -18,7 +19,7 @@ function parseSpec<T>(spec: string, sourceParser: (source: string) => T) {
     throw new Error(`Patch locators must explicitly define their source`);
 
   const patchPaths = selector
-    ? selector.split(/&/).map(path => npath.toPortablePath(path))
+    ? selector.split(/&/).map(npath.toPortablePath)
     : [];
 
   const parentLocator = params && typeof params.locator === `string`
@@ -29,7 +30,18 @@ function parseSpec<T>(spec: string, sourceParser: (source: string) => T) {
     ? params.version
     : null;
 
-  const sourceItem = sourceParser(source);
+  // Separate patch-specific from source-specific parameters
+  const patchParams = new Set([`locator`, `version`, `hash`]);
+  const sourceParams = params
+    ? Object.fromEntries(Object.entries(params).filter(([key]) => !patchParams.has(key)))
+    : {};
+
+  // Reconstruct source with its parameters
+  const sourceWithParams = Object.keys(sourceParams).length > 0
+    ? `${source}::${querystring.stringify(normalizeParams(sourceParams))}`
+    : source;
+
+  const sourceItem = sourceParser(sourceWithParams);
 
   return {parentLocator, sourceItem, patchPaths, sourceVersion};
 }
@@ -91,11 +103,15 @@ function makeSpec<T>({parentLocator, sourceItem, patchPaths, sourceVersion, patc
     ? {hash: patchHash}
     : {} as {};
 
+  const sourceString = sourceStringifier(sourceItem);
+  const {params: sourceParams} = structUtils.parseRange(sourceString);
+
   return structUtils.makeRange({
     protocol: `patch:`,
-    source: sourceStringifier(sourceItem),
+    source: sourceString,
     selector: patchPaths.join(`&`),
     params: {
+      ...normalizeParams(sourceParams),
       ...sourceVersionSpread,
       ...patchHashSpread,
       ...parentLocatorSpread,
@@ -343,3 +359,25 @@ export function makePatchHash(
 
   return hashUtils.makeHash(`${CACHE_VERSION}`, ...parts).slice(0, 6);
 }
+
+/**
+ * Normalizes ParsedUrlQuery to string-only parameters.
+ *
+ * Node.js querystring methods don't provide this conversion directly:
+ * - Filters out undefined values
+ * - Joins arrays with commas (not multiple key=value pairs)
+ * - Ensures all values are strings
+ */
+export function normalizeParams(params: ReturnType<typeof structUtils.parseRange>[`params`]): {[key: string]: string} {
+  if (!params) return {};
+
+  const result: {[key: string]: string} = {};
+
+  for (const [key, value] of Object.entries(params)) {
+    if (value !== undefined) {
+      result[key] = Array.isArray(value) ? value.join(`,`) : String(value);
+    }
+  }
+
+  return result;
+}