diff --git a/.changeset/twelve-catalogs-revert.md b/.changeset/twelve-catalogs-revert.md new file mode 100644 index 0000000000..9bb7258526 --- /dev/null +++ b/.changeset/twelve-catalogs-revert.md @@ -0,0 +1,7 @@ +--- +"@pnpm/catalogs.config": patch +"@pnpm/installing.commands": patch +"pnpm": patch +--- + +Fix `pnpm install` reporting "Already up to date" after a catalog entry in `pnpm-workspace.yaml` was reverted to a previous version. After an update modified a catalog, the workspace state cache stored the pre-update catalog versions, so reverting the entry back to its original version was not detected as an outdated state [#12418](https://github.com/pnpm/pnpm/issues/12418). diff --git a/catalogs/config/src/index.ts b/catalogs/config/src/index.ts index 2f2143896e..edc1d800fa 100644 --- a/catalogs/config/src/index.ts +++ b/catalogs/config/src/index.ts @@ -1 +1,2 @@ export { getCatalogsFromWorkspaceManifest } from './getCatalogsFromWorkspaceManifest.js' +export { mergeCatalogs } from './mergeCatalogs.js' diff --git a/catalogs/config/src/mergeCatalogs.ts b/catalogs/config/src/mergeCatalogs.ts new file mode 100644 index 0000000000..ac26c2b2a9 --- /dev/null +++ b/catalogs/config/src/mergeCatalogs.ts @@ -0,0 +1,40 @@ +import type { Catalog, Catalogs } from '@pnpm/catalogs.types' + +/** + * Deep-merges catalog definitions, later arguments taking precedence over + * earlier ones at the individual entry level. Use it to fold the catalog + * changes produced by an install (`updatedCatalogs`) back into the catalogs + * read at startup, so the result reflects what was actually written to + * `pnpm-workspace.yaml`. + * + * Catalog and dependency names originate from `pnpm-workspace.yaml`, so the + * result is built from null-prototype records and entries are copied with + * `Object.defineProperty`. A name like `__proto__` then becomes an ordinary + * own property instead of mutating a prototype. + */ +export function mergeCatalogs (...catalogsList: Array): Catalogs { + const result = Object.create(null) as Record + for (const catalogs of catalogsList) { + if (catalogs == null) continue + for (const catalogName of Object.keys(catalogs)) { + const catalog = catalogs[catalogName] + if (catalog == null) continue + const target: Record = result[catalogName] ?? Object.create(null) + for (const dependencyName of Object.keys(catalog)) { + Object.defineProperty(target, dependencyName, { + value: catalog[dependencyName], + writable: true, + enumerable: true, + configurable: true, + }) + } + Object.defineProperty(result, catalogName, { + value: target, + writable: true, + enumerable: true, + configurable: true, + }) + } + } + return result +} diff --git a/catalogs/config/test/mergeCatalogs.test.ts b/catalogs/config/test/mergeCatalogs.test.ts new file mode 100644 index 0000000000..6456672662 --- /dev/null +++ b/catalogs/config/test/mergeCatalogs.test.ts @@ -0,0 +1,53 @@ +import { expect, test } from '@jest/globals' +import { mergeCatalogs } from '@pnpm/catalogs.config' + +test('returns an empty catalog when nothing is passed', () => { + expect(mergeCatalogs()).toEqual({}) + expect(mergeCatalogs(undefined, undefined)).toEqual({}) +}) + +test('later entries override earlier ones at the dependency level', () => { + expect(mergeCatalogs( + { + default: { foo: '1.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }, + { + default: { foo: '2.0.0' }, + } + )).toEqual({ + default: { foo: '2.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }) +}) + +test('adds catalog entries that did not exist in the base', () => { + expect(mergeCatalogs( + { default: { foo: '1.0.0' } }, + { default: { bar: '1.0.0' }, named: { baz: '1.0.0' } } + )).toEqual({ + default: { foo: '1.0.0', bar: '1.0.0' }, + named: { baz: '1.0.0' }, + }) +}) + +test('skips nullish catalog arguments and nullish named catalogs', () => { + expect(mergeCatalogs( + undefined, + { default: { foo: '1.0.0' }, named: undefined } + )).toEqual({ + default: { foo: '1.0.0' }, + }) +}) + +test('treats dangerous catalog and dependency names as ordinary own properties', () => { + // Catalogs parsed from YAML/JSON can carry `__proto__` as an own property, + // unlike an object literal where `{ __proto__: ... }` sets the prototype. + const malicious = JSON.parse('{"__proto__":{"polluted":"yes"},"default":{"__proto__":"1.0.0","constructor":"2.0.0"}}') + const merged = mergeCatalogs(malicious) + expect(({} as Record).polluted).toBeUndefined() + expect(Object.prototype.hasOwnProperty.call(merged, '__proto__')).toBe(true) + expect(Object.prototype.hasOwnProperty.call(merged.default, '__proto__')).toBe(true) + expect((merged.default as Record).__proto__).toBe('1.0.0') + expect((merged.default as Record).constructor).toBe('2.0.0') +}) diff --git a/installing/commands/package.json b/installing/commands/package.json index b5248094d3..4f106264f3 100644 --- a/installing/commands/package.json +++ b/installing/commands/package.json @@ -35,6 +35,7 @@ "@inquirer/prompts": "catalog:", "@pnpm/building.after-install": "workspace:*", "@pnpm/building.policy": "workspace:*", + "@pnpm/catalogs.config": "workspace:*", "@pnpm/catalogs.types": "workspace:*", "@pnpm/cli.command": "workspace:*", "@pnpm/cli.common-cli-options-help": "workspace:*", diff --git a/installing/commands/src/installDeps.ts b/installing/commands/src/installDeps.ts index b203f1e414..15df3a5a05 100644 --- a/installing/commands/src/installDeps.ts +++ b/installing/commands/src/installDeps.ts @@ -1,6 +1,8 @@ import path from 'node:path' import { buildProjects } from '@pnpm/building.after-install' +import { mergeCatalogs } from '@pnpm/catalogs.config' +import type { Catalogs } from '@pnpm/catalogs.types' import type { CommandHandler } from '@pnpm/cli.command' import { readProjectManifestOnly, @@ -426,7 +428,7 @@ export async function installDeps ( if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs), workspaceDir: opts.workspaceDir ?? opts.lockfileDir ?? opts.dir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, @@ -485,7 +487,7 @@ export async function installDeps ( selectedProjectsGraph, workspaceDir: opts.workspaceDir, // Otherwise TypeScript doesn't understand that is not undefined runPacquet, - }, 'install') + }, 'install', updatedCatalogs) if (opts.ignoreScripts) return @@ -508,7 +510,7 @@ export async function installDeps ( if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs), workspaceDir: opts.workspaceDir ?? opts.lockfileDir ?? opts.dir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, @@ -528,20 +530,36 @@ async function recursiveInstallThenUpdateWorkspaceState ( allProjects: Project[], params: string[], opts: RecursiveOptions & WorkspaceStateSettings, - cmdFullName: CommandFullName + cmdFullName: CommandFullName, + updatedCatalogs?: Catalogs ): Promise { const recursiveResult = await recursive(allProjects, params, opts, cmdFullName) if (!opts.lockfileOnly) { await updateWorkspaceState({ allProjects, - settings: opts, + settings: withUpdatedCatalogs(opts, updatedCatalogs, recursiveResult.updatedCatalogs), workspaceDir: opts.workspaceDir, pnpmfiles: opts.pnpmfile, filteredInstall: allProjects.length !== Object.keys(opts.selectedProjectsGraph ?? {}).length, configDependencies: opts.configDependencies, }) } - return recursiveResult + return recursiveResult.passed +} + +/** + * Folds the catalog entries written to `pnpm-workspace.yaml` during this + * install into the catalogs read at startup. The workspace state cache records + * these so a later install detects when a catalog entry was reverted; without + * this, the cache would keep the stale pre-install catalogs and report + * "Already up to date" even though the manifest changed. + */ +function withUpdatedCatalogs ( + settings: T, + ...updatedCatalogs: Array +): T { + if (updatedCatalogs.every((catalogs) => catalogs == null)) return settings + return { ...settings, catalogs: mergeCatalogs(settings.catalogs, ...updatedCatalogs) } } function severityStringToNumber (severity: VulnerabilitySeverity): number { diff --git a/installing/commands/src/recursive.ts b/installing/commands/src/recursive.ts index 93af6cfad1..a5eef2fc16 100755 --- a/installing/commands/src/recursive.ts +++ b/installing/commands/src/recursive.ts @@ -1,6 +1,7 @@ import { promises as fs } from 'node:fs' import path from 'node:path' +import { mergeCatalogs } from '@pnpm/catalogs.config' import type { Catalogs } from '@pnpm/catalogs.types' import type { CommandHandler } from '@pnpm/cli.command' import { @@ -144,21 +145,31 @@ export type RecursiveOptions = CreateStoreControllerOptions & Pick { +): Promise { if (allProjects.length === 0) { // It might make sense to throw an exception in this case - return false + return { passed: false } } const pkgs = Object.values(opts.selectedProjectsGraph).map((wsPkg) => wsPkg.package) if (pkgs.length === 0) { - return false + return { passed: false } } const manifestsByPath = getManifestsByPath(allProjects) @@ -225,7 +236,7 @@ export async function recursive ( const calculatedRepositoryRoot = await fs.realpath(calculateRepositoryRoot(opts.workspaceDir, importers.map(x => x.rootDir))) const isFromWorkspace = isSubdir.bind(null, calculatedRepositoryRoot) importers = await pFilter(importers, async ({ rootDirRealPath }) => isFromWorkspace(rootDirRealPath)) - if (importers.length === 0) return true + if (importers.length === 0) return { passed: true } let mutation: 'install' | 'installSome' | 'uninstallSome' switch (cmdFullName) { case 'remove': @@ -341,7 +352,7 @@ export async function recursive ( await Promise.all(promises) } await handleIgnoredBuilds(opts, ignoredBuilds) - return true + return { passed: true, updatedCatalogs } } const pkgPaths = (Object.keys(opts.selectedProjectsGraph) as ProjectRootDir[]).sort() @@ -459,8 +470,10 @@ export async function recursive ( if (opts.save !== false) { await writeProjectManifest(newManifest) if (newCatalogsAddition) { - updatedCatalogs ??= {} - Object.assign(updatedCatalogs, newCatalogsAddition) + // Per-project additions are partial maps keyed by catalog name then + // dependency. Merge at the dependency level so two projects updating + // different entries of the same catalog don't clobber each other. + updatedCatalogs = mergeCatalogs(updatedCatalogs, newCatalogsAddition) } } if (ignoredBuilds?.size) { @@ -527,7 +540,7 @@ export async function recursive ( 'None of the specified packages were found in the dependencies of any of the projects.') } - return true + return { passed: true, updatedCatalogs } } function calculateRepositoryRoot ( diff --git a/installing/commands/tsconfig.json b/installing/commands/tsconfig.json index 8d30b0d2d1..68fde6a9b4 100644 --- a/installing/commands/tsconfig.json +++ b/installing/commands/tsconfig.json @@ -27,6 +27,9 @@ { "path": "../../building/policy" }, + { + "path": "../../catalogs/config" + }, { "path": "../../catalogs/types" }, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1e71d6546d..130ae03484 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -5246,6 +5246,9 @@ importers: '@pnpm/building.policy': specifier: workspace:* version: link:../../building/policy + '@pnpm/catalogs.config': + specifier: workspace:* + version: link:../../catalogs/config '@pnpm/catalogs.types': specifier: workspace:* version: link:../../catalogs/types diff --git a/pnpm/test/catalogUpToDate.ts b/pnpm/test/catalogUpToDate.ts new file mode 100644 index 0000000000..14d4b99033 --- /dev/null +++ b/pnpm/test/catalogUpToDate.ts @@ -0,0 +1,48 @@ +import { expect, test } from '@jest/globals' +import { prepare } from '@pnpm/prepare' +import { addDistTag } from '@pnpm/testing.registry-mock' +import type { ProjectManifest } from '@pnpm/types' +import { writeYamlFileSync } from 'write-yaml-file' + +import { execPnpm } from './utils/index.js' + +test('reverting a catalog entry after updating it is detected as an outdated state (#12418)', async () => { + await addDistTag({ package: '@pnpm.e2e/foo', version: '100.1.0', distTag: 'latest' }) + + const manifest: ProjectManifest = { + name: 'test-catalog-up-to-date', + version: '0.0.0', + private: true, + dependencies: { + '@pnpm.e2e/foo': 'catalog:', + }, + } + const project = prepare(manifest) + + writeYamlFileSync('pnpm-workspace.yaml', { + packages: ['.'], + catalog: { + '@pnpm.e2e/foo': '100.0.0', + }, + }) + + await execPnpm(['install']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.0.0') + + await execPnpm(['update', '@pnpm.e2e/foo', '--latest']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.1.0') + + // Restore the catalog entry to the version it had before the update. A + // subsequent install must notice that the workspace state is no longer up to + // date and reinstall the previous version instead of reporting + // "Already up to date". + writeYamlFileSync('pnpm-workspace.yaml', { + packages: ['.'], + catalog: { + '@pnpm.e2e/foo': '100.0.0', + }, + }) + + await execPnpm(['install']) + expect(project.readCurrentLockfile().importers['.'].dependencies?.['@pnpm.e2e/foo'].version).toBe('100.0.0') +})