From a484cea3f2564a80ce8c3171d433f3d8c3e714ef Mon Sep 17 00:00:00 2001 From: 3w36zj6 <52315048+3w36zj6@users.noreply.github.com> Date: Mon, 26 Jan 2026 09:13:06 +0900 Subject: [PATCH] fix(npm-resolver): request full metadata for optional dependencies (#10455) close #9950 --------- Co-authored-by: Zoltan Kochan --- .changeset/optional-deps-full-metadata.md | 8 + .changeset/refactor-fullmetadata-options.md | 5 + network/fetch/src/fetchFromRegistry.ts | 3 +- network/fetch/test/fetchFromRegistry.test.ts | 77 ++++++++- network/fetching-types/src/index.ts | 1 + pkg-manager/package-requester/test/index.ts | 59 +++++++ pnpm/test/dlx.ts | 7 +- pnpm/test/install/supportedArchitectures.ts | 2 +- resolving/npm-resolver/src/fetch.ts | 14 +- resolving/npm-resolver/src/index.ts | 8 +- resolving/npm-resolver/src/pickPackage.ts | 26 +++- .../test/optionalDependencies.test.ts | 146 ++++++++++++++++++ resolving/resolver-base/src/index.ts | 2 +- 13 files changed, 334 insertions(+), 24 deletions(-) create mode 100644 .changeset/optional-deps-full-metadata.md create mode 100644 .changeset/refactor-fullmetadata-options.md create mode 100644 resolving/npm-resolver/test/optionalDependencies.test.ts diff --git a/.changeset/optional-deps-full-metadata.md b/.changeset/optional-deps-full-metadata.md new file mode 100644 index 0000000000..e3d89193d6 --- /dev/null +++ b/.changeset/optional-deps-full-metadata.md @@ -0,0 +1,8 @@ +--- +"@pnpm/npm-resolver": patch +"@pnpm/fetching-types": patch +"@pnpm/package-requester": patch +"pnpm": patch +--- + +Fixed optional dependencies to request full metadata from the registry to get the `libc` field, which is required for proper platform compatibility checks [#9950](https://github.com/pnpm/pnpm/issues/9950). diff --git a/.changeset/refactor-fullmetadata-options.md b/.changeset/refactor-fullmetadata-options.md new file mode 100644 index 0000000000..ec4444cfe7 --- /dev/null +++ b/.changeset/refactor-fullmetadata-options.md @@ -0,0 +1,5 @@ +--- +"@pnpm/fetch": major +--- + +Refactored `fullMetadata` option handling. The `fullMetadata` option is no longer accepted by `createFetchFromRegistry()` at construction time - it should only be passed at call time via the fetch options. diff --git a/network/fetch/src/fetchFromRegistry.ts b/network/fetch/src/fetchFromRegistry.ts index 019843e7dd..09d7b6405c 100644 --- a/network/fetch/src/fetchFromRegistry.ts +++ b/network/fetch/src/fetchFromRegistry.ts @@ -35,7 +35,6 @@ export function fetchWithAgent (url: RequestInfo, opts: FetchWithAgentOptions): export type { AgentOptions } export interface CreateFetchFromRegistryOptions extends AgentOptions { - fullMetadata?: boolean userAgent?: string sslConfigs?: Record } @@ -46,7 +45,7 @@ export function createFetchFromRegistry (defaultOpts: CreateFetchFromRegistryOpt 'user-agent': USER_AGENT, ...getHeaders({ auth: opts?.authHeaderValue, - fullMetadata: defaultOpts.fullMetadata, + fullMetadata: opts?.fullMetadata, userAgent: defaultOpts.userAgent, }), } diff --git a/network/fetch/test/fetchFromRegistry.test.ts b/network/fetch/test/fetchFromRegistry.test.ts index 26c240b5f5..427ba84ff9 100644 --- a/network/fetch/test/fetchFromRegistry.test.ts +++ b/network/fetch/test/fetchFromRegistry.test.ts @@ -16,8 +16,8 @@ test('fetchFromRegistry', async () => { }) test('fetchFromRegistry fullMetadata', async () => { - const fetchFromRegistry = createFetchFromRegistry({ fullMetadata: true }) - const res = await fetchFromRegistry('https://registry.npmjs.org/is-positive') + const fetchFromRegistry = createFetchFromRegistry({}) + const res = await fetchFromRegistry('https://registry.npmjs.org/is-positive', { fullMetadata: true }) const metadata = await res.json() as any // eslint-disable-line expect(metadata.name).toEqual('is-positive') expect(metadata.versions['1.0.0'].scripts).toBeTruthy() @@ -33,7 +33,7 @@ test('authorization headers are removed before redirection if the target is on a .get('/is-positive') .reply(200, { ok: true }) - const fetchFromRegistry = createFetchFromRegistry({ fullMetadata: true }) + const fetchFromRegistry = createFetchFromRegistry({}) const res = await fetchFromRegistry( 'http://registry.pnpm.io/is-positive', { authHeaderValue: 'Bearer 123' } @@ -55,7 +55,7 @@ test('authorization headers are not removed before redirection if the target is .get('/is-positive-new') .reply(200, { ok: true }) - const fetchFromRegistry = createFetchFromRegistry({ fullMetadata: true }) + const fetchFromRegistry = createFetchFromRegistry({}) const res = await fetchFromRegistry( 'http://registry.pnpm.io/is-positive', { authHeaderValue: 'Bearer 123' } @@ -66,7 +66,7 @@ test('authorization headers are not removed before redirection if the target is }) test('switch to the correct agent for requests on redirect from http: to https:', async () => { - const fetchFromRegistry = createFetchFromRegistry({ fullMetadata: true }) + const fetchFromRegistry = createFetchFromRegistry({}) // We can test this on any endpoint that redirects from http: to https: const { status } = await fetchFromRegistry('http://pnpm.io/pnpm.js') @@ -138,3 +138,70 @@ test('fail if the client certificate is not provided', async () => { } expect(err?.code).toMatch(/ECONNRESET|ERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED/) }) + +test('redirect to protocol-relative URL', async () => { + nock('http://registry.pnpm.io/') + .get('/foo') + .reply(302, '', { location: '//registry.other.org/foo' }) + nock('http://registry.other.org/') + .get('/foo') + .reply(200, { ok: true }) + + const fetchFromRegistry = createFetchFromRegistry({}) + const res = await fetchFromRegistry( + 'http://registry.pnpm.io/foo' + ) + + expect(await res.json()).toStrictEqual({ ok: true }) + expect(nock.isDone()).toBeTruthy() +}) + +test('redirect to relative URL', async () => { + nock('http://registry.pnpm.io/') + .get('/bar/baz') + .reply(302, '', { location: '../foo' }) + nock('http://registry.pnpm.io/') + .get('/foo') + .reply(200, { ok: true }) + + const fetchFromRegistry = createFetchFromRegistry({}) + const res = await fetchFromRegistry( + 'http://registry.pnpm.io/bar/baz' + ) + + expect(await res.json()).toStrictEqual({ ok: true }) + expect(nock.isDone()).toBeTruthy() +}) + +test('redirect to relative URL when request pkg.pr.new link', async () => { + nock('https://pkg.pr.new/') + .get('/vue@14175') + .reply(302, '', { location: '/vuejs/core/vue@14182' }) + + nock('https://pkg.pr.new/') + .get('/vuejs/core/vue@14182') + .reply(302, '', { location: '/vuejs/core/vue@82a13bb6faaa9f77a06b57e69e0934b9f620f333' }) + + nock('https://pkg.pr.new/') + .get('/vuejs/core/vue@82a13bb6faaa9f77a06b57e69e0934b9f620f333') + .reply(200, { ok: true }) + + const fetchFromRegistry = createFetchFromRegistry({}) + const res = await fetchFromRegistry( + 'https://pkg.pr.new/vue@14175' + ) + + expect(await res.json()).toStrictEqual({ ok: true }) + expect(nock.isDone()).toBeTruthy() +}) + +test('redirect without location header throws error', async () => { + nock('http://registry.pnpm.io/') + .get('/missing-location') + .reply(302, 'found') + + const fetchFromRegistry = createFetchFromRegistry({}) + await expect(fetchFromRegistry( + 'http://registry.pnpm.io/missing-location' + )).rejects.toThrow(/Redirect location header missing/) +}) diff --git a/network/fetching-types/src/index.ts b/network/fetching-types/src/index.ts index 6c2f2c470e..c9789d520a 100644 --- a/network/fetching-types/src/index.ts +++ b/network/fetching-types/src/index.ts @@ -13,6 +13,7 @@ export type FetchFromRegistry = ( opts?: RequestInit & { authHeaderValue?: string compress?: boolean + fullMetadata?: boolean retry?: RetryTimeoutOptions timeout?: number } diff --git a/pkg-manager/package-requester/test/index.ts b/pkg-manager/package-requester/test/index.ts index 5dfb11d659..e98eaab9be 100644 --- a/pkg-manager/package-requester/test/index.ts +++ b/pkg-manager/package-requester/test/index.ts @@ -1174,3 +1174,62 @@ test('HTTP tarball without integrity gets integrity computed during fetch', asyn expect(pkgResponse.body.resolution).toHaveProperty('integrity') expect((pkgResponse.body.resolution as { integrity?: string }).integrity).toMatch(/^sha512-/) }) + +test('should pass optional flag to resolve function', async () => { + const storeDir = temporaryDirectory() + const cafs = createCafsStore(storeDir) + + let capturedOptional: boolean | undefined + const mockResolve: typeof resolve = async (wantedDependency, _options) => { + capturedOptional = wantedDependency.optional + return resolve(wantedDependency, _options) + } + + const requestPackage = createPackageRequester({ + resolve: mockResolve, + fetchers, + cafs, + networkConcurrency: 1, + storeDir, + verifyStoreIntegrity: true, + virtualStoreDirMaxLength: 120, + }) + + const projectDir = temporaryDirectory() + + await requestPackage( + { alias: 'is-positive', bareSpecifier: '1.0.0', optional: true }, + { + downloadPriority: 0, + lockfileDir: projectDir, + preferredVersions: {}, + projectDir, + } + ) + + expect(capturedOptional).toBe(true) + + await requestPackage( + { alias: 'is-positive', bareSpecifier: '1.0.0', optional: false }, + { + downloadPriority: 0, + lockfileDir: projectDir, + preferredVersions: {}, + projectDir, + } + ) + + expect(capturedOptional).toBe(false) + + await requestPackage( + { alias: 'is-positive', bareSpecifier: '1.0.0' }, + { + downloadPriority: 0, + lockfileDir: projectDir, + preferredVersions: {}, + projectDir, + } + ) + + expect(capturedOptional).toBeUndefined() +}) diff --git a/pnpm/test/dlx.ts b/pnpm/test/dlx.ts index 6184b75550..c4d4626003 100644 --- a/pnpm/test/dlx.ts +++ b/pnpm/test/dlx.ts @@ -351,12 +351,13 @@ describeOnLinuxOnly('dlx with supportedArchitectures CLI options', () => { '@pnpm.e2e/only-darwin-arm64', '@pnpm.e2e/only-darwin-x64', '@pnpm.e2e/only-linux-arm64-glibc', - '@pnpm.e2e/only-linux-arm64-musl', '@pnpm.e2e/only-linux-x64-glibc', - '@pnpm.e2e/only-linux-x64-musl', '@pnpm.e2e/only-win32-arm64', '@pnpm.e2e/only-win32-x64', - ], []], + ], [ + '@pnpm.e2e/only-linux-arm64-musl', + '@pnpm.e2e/only-linux-x64-musl', + ]], ] as Case[])('%p', async (cliOpts, installed, notInstalled) => { prepareEmpty() diff --git a/pnpm/test/install/supportedArchitectures.ts b/pnpm/test/install/supportedArchitectures.ts index 286f4e6c49..220b038764 100644 --- a/pnpm/test/install/supportedArchitectures.ts +++ b/pnpm/test/install/supportedArchitectures.ts @@ -28,12 +28,12 @@ type Case = [ const TEST_CASES: Case[] = [ [[], undefined, [ 'only-linux-x64-glibc', - 'only-linux-x64-musl', ], [ 'only-darwin-arm64', 'only-darwin-x64', 'only-linux-arm64-glibc', 'only-linux-arm64-musl', + 'only-linux-x64-musl', 'only-win32-arm64', 'only-win32-x64', ]], diff --git a/resolving/npm-resolver/src/fetch.ts b/resolving/npm-resolver/src/fetch.ts index d806ba3295..027ce55e85 100644 --- a/resolving/npm-resolver/src/fetch.ts +++ b/resolving/npm-resolver/src/fetch.ts @@ -49,11 +49,20 @@ export interface FetchMetadataFromFromRegistryOptions { fetchWarnTimeoutMs: number } +export interface FetchMetadataOptions { + registry: string + authHeaderValue?: string + fullMetadata?: boolean +} + export async function fetchMetadataFromFromRegistry ( fetchOpts: FetchMetadataFromFromRegistryOptions, pkgName: string, - registry: string, - authHeaderValue?: string + { + authHeaderValue, + fullMetadata, + registry, + }: FetchMetadataOptions ): Promise { const uri = toUri(pkgName, registry) const op = retry.operation(fetchOpts.retry) @@ -65,6 +74,7 @@ export async function fetchMetadataFromFromRegistry ( response = await fetchOpts.fetch(uri, { authHeaderValue, compress: true, + fullMetadata, retry: fetchOpts.retry, timeout: fetchOpts.timeout, }) as RegistryResponse diff --git a/resolving/npm-resolver/src/index.ts b/resolving/npm-resolver/src/index.ts index 65b1e107d8..7015c8b1f9 100644 --- a/resolving/npm-resolver/src/index.ts +++ b/resolving/npm-resolver/src/index.ts @@ -1,5 +1,4 @@ import path from 'path' -import { FULL_META_DIR, FULL_FILTERED_META_DIR, ABBREVIATED_META_DIR } from '@pnpm/constants' import { PnpmError } from '@pnpm/error' import { type FetchFromRegistry, @@ -149,7 +148,7 @@ export interface WorkspaceResolveResult extends ResolveResult { } export type NpmResolver = ( - wantedDependency: WantedDependency, + wantedDependency: WantedDependency & { optional?: boolean }, opts: ResolveFromNpmOptions ) => Promise @@ -179,9 +178,9 @@ export function createNpmResolver ( getAuthHeaderValueByURI: getAuthHeader, pickPackage: pickPackage.bind(null, { fetch, + fullMetadata: opts.fullMetadata, filterMetadata: opts.filterMetadata, metaCache, - metaDir: opts.fullMetadata ? (opts.filterMetadata ? FULL_FILTERED_META_DIR : FULL_META_DIR) : ABBREVIATED_META_DIR, offline: opts.offline, preferOffline: opts.preferOffline, cacheDir: opts.cacheDir, @@ -233,7 +232,7 @@ export type ResolveFromNpmOptions = { async function resolveNpm ( ctx: ResolveFromNpmContext, - wantedDependency: WantedDependency, + wantedDependency: WantedDependency & { optional?: boolean }, opts: ResolveFromNpmOptions ): Promise { const defaultTag = opts.defaultTag ?? 'latest' @@ -276,6 +275,7 @@ async function resolveNpm ( preferredVersionSelectors: opts.preferredVersions?.[spec.name], registry, updateToLatest: opts.update === 'latest', + optional: wantedDependency.optional, }) } catch (err: any) { // eslint-disable-line if ((workspacePackages != null) && opts.projectDir) { diff --git a/resolving/npm-resolver/src/pickPackage.ts b/resolving/npm-resolver/src/pickPackage.ts index e76f004956..7a564a3f16 100644 --- a/resolving/npm-resolver/src/pickPackage.ts +++ b/resolving/npm-resolver/src/pickPackage.ts @@ -1,5 +1,6 @@ import { promises as fs } from 'fs' import path from 'path' +import { ABBREVIATED_META_DIR, FULL_META_DIR, FULL_FILTERED_META_DIR } from '@pnpm/constants' import { createHexHash } from '@pnpm/crypto.hash' import { PnpmError } from '@pnpm/error' import { logger } from '@pnpm/logger' @@ -66,6 +67,7 @@ export interface PickPackageOptions extends PickPackageFromMetaOptions { registry: string dryRun: boolean updateToLatest?: boolean + optional?: boolean } const pickPackageFromMetaUsingTimeStrict = pickPackageFromMeta.bind(null, pickVersionByVersionRange) @@ -84,8 +86,8 @@ function pickPackageFromMetaUsingTime ( export async function pickPackage ( ctx: { - fetch: (pkgName: string, registry: string, authHeaderValue?: string) => Promise - metaDir: string + fetch: (pkgName: string, opts: { registry: string, authHeaderValue?: string, fullMetadata?: boolean }) => Promise + fullMetadata?: boolean metaCache: PackageMetaCache cacheDir: string offline?: boolean @@ -125,7 +127,15 @@ export async function pickPackage ( validatePackageName(spec.name) - const cachedMeta = ctx.metaCache.get(spec.name) + // Use full metadata for optional dependencies to get libc field. + // See: https://github.com/pnpm/pnpm/issues/9950 + const fullMetadata = opts.optional === true || ctx.fullMetadata === true + const metaDir = fullMetadata + ? (ctx.filterMetadata ? FULL_FILTERED_META_DIR : FULL_META_DIR) + : ABBREVIATED_META_DIR + // Cache key includes fullMetadata to avoid returning abbreviated metadata when full metadata is requested. + const cacheKey = fullMetadata ? `${spec.name}:full` : spec.name + const cachedMeta = ctx.metaCache.get(cacheKey) if (cachedMeta != null) { return { meta: cachedMeta, @@ -134,7 +144,7 @@ export async function pickPackage ( } const registryName = getRegistryName(opts.registry) - const pkgMirror = path.join(ctx.cacheDir, ctx.metaDir, registryName, `${encodePkgName(spec.name)}.json`) + const pkgMirror = path.join(ctx.cacheDir, metaDir, registryName, `${encodePkgName(spec.name)}.json`) return runLimited(pkgMirror, async (limit) => { let metaCachedInStore: PackageMeta | null | undefined @@ -201,13 +211,17 @@ export async function pickPackage ( } try { - let meta = await ctx.fetch(spec.name, opts.registry, opts.authHeaderValue) + let meta = await ctx.fetch(spec.name, { + authHeaderValue: opts.authHeaderValue, + fullMetadata, + registry: opts.registry, + }) if (ctx.filterMetadata) { meta = clearMeta(meta) } meta.cachedAt = Date.now() // only save meta to cache, when it is fresh - ctx.metaCache.set(spec.name, meta) + ctx.metaCache.set(cacheKey, meta) if (!opts.dryRun) { // We stringify this meta here to avoid saving any mutations that could happen to the meta object. const stringifiedMeta = JSON.stringify(meta) diff --git a/resolving/npm-resolver/test/optionalDependencies.test.ts b/resolving/npm-resolver/test/optionalDependencies.test.ts new file mode 100644 index 0000000000..7e7a8b1090 --- /dev/null +++ b/resolving/npm-resolver/test/optionalDependencies.test.ts @@ -0,0 +1,146 @@ +/// +import { createFetchFromRegistry } from '@pnpm/fetch' +import { createNpmResolver } from '@pnpm/npm-resolver' +import { type Registries } from '@pnpm/types' +import nock from 'nock' +import tempy from 'tempy' + +const registries = { + default: 'https://registry.npmjs.org/', +} satisfies Registries + +const fetch = createFetchFromRegistry({}) +const getAuthHeader = () => undefined +const createResolveFromNpm = createNpmResolver.bind(null, fetch, getAuthHeader) + +afterEach(() => { + nock.cleanAll() + nock.disableNetConnect() +}) + +beforeEach(() => { + nock.enableNetConnect() +}) + +describe('optional dependencies', () => { + test('optional dependencies receive full metadata with libc field', async () => { + // This test verifies the fix for https://github.com/pnpm/pnpm/issues/9950 + // Optional dependencies need full metadata to get the libc field for platform compatibility checks. + const packageMeta = { + name: 'platform-pkg', + 'dist-tags': { latest: '1.0.0' }, + versions: { + '1.0.0': { + name: 'platform-pkg', + version: '1.0.0', + os: ['linux'], + cpu: ['x64'], + libc: ['glibc'], + dist: { + tarball: 'https://registry.npmjs.org/platform-pkg/-/platform-pkg-1.0.0.tgz', + integrity: 'sha512-test1234567890123456789012345678901234567890123456789012345678', + }, + }, + }, + } + + // Verify that full metadata is requested (no abbreviated Accept header) + const scope = nock(registries.default) + .get('/platform-pkg') + .matchHeader('accept', (value) => !value.includes('application/vnd.npm.install-v1+json')) + .reply(200, packageMeta) + + const { resolveFromNpm } = createResolveFromNpm({ + cacheDir: tempy.directory(), + registries, + }) + + const result = await resolveFromNpm( + { + alias: 'platform-pkg', + bareSpecifier: '1.0.0', + optional: true, + }, + {} + ) + + expect(result!.manifest!.libc).toEqual(['glibc']) + expect(result!.manifest!.os).toEqual(['linux']) + expect(result!.manifest!.cpu).toEqual(['x64']) + expect(scope.isDone()).toBe(true) + }) + + test('abbreviated and full metadata are cached separately', async () => { + // Abbreviated metadata doesn't include scripts, full metadata does. + // When resolving the same package first as regular, then as optional, + // we should get different metadata from each request. + const abbreviatedMeta = { + name: 'cache-test', + 'dist-tags': { latest: '1.0.0' }, + versions: { + '1.0.0': { + name: 'cache-test', + version: '1.0.0', + dist: { + tarball: 'https://registry.npmjs.org/cache-test/-/cache-test-1.0.0.tgz', + integrity: 'sha512-test1234567890123456789012345678901234567890123456789012345678', + }, + }, + }, + } + const fullMeta = { + name: 'cache-test', + 'dist-tags': { latest: '1.0.0' }, + versions: { + '1.0.0': { + name: 'cache-test', + version: '1.0.0', + scripts: { + test: 'jest', + build: 'tsc', + }, + dist: { + tarball: 'https://registry.npmjs.org/cache-test/-/cache-test-1.0.0.tgz', + integrity: 'sha512-test1234567890123456789012345678901234567890123456789012345678', + }, + }, + }, + } + + // First request: abbreviated metadata for regular dependency + const abbreviatedScope = nock(registries.default) + .get('/cache-test') + .matchHeader('accept', /application\/vnd\.npm\.install-v1\+json/) + .reply(200, abbreviatedMeta) + + // Second request: full metadata for optional dependency + const fullScope = nock(registries.default) + .get('/cache-test') + .matchHeader('accept', (value) => !value.includes('application/vnd.npm.install-v1+json')) + .reply(200, fullMeta) + + const cacheDir = tempy.directory() + + const { resolveFromNpm } = createResolveFromNpm({ + cacheDir, + registries, + }) + + // Resolve as regular dependency - should get abbreviated metadata + const regularResult = await resolveFromNpm( + { alias: 'cache-test', bareSpecifier: '1.0.0' }, + {} + ) + expect(regularResult!.manifest!.scripts).toBeUndefined() + + // Resolve as optional dependency - should get full metadata (separate cache entry) + const optionalResult = await resolveFromNpm( + { alias: 'cache-test', bareSpecifier: '1.0.0', optional: true }, + {} + ) + expect(optionalResult!.manifest!.scripts).toEqual({ test: 'jest', build: 'tsc' }) + + expect(abbreviatedScope.isDone()).toBe(true) + expect(fullScope.isDone()).toBe(true) + }) +}) diff --git a/resolving/resolver-base/src/index.ts b/resolving/resolver-base/src/index.ts index a585b9a99b..d4295423d6 100644 --- a/resolving/resolver-base/src/index.ts +++ b/resolving/resolver-base/src/index.ts @@ -137,4 +137,4 @@ export type WantedDependency = { bareSpecifier?: string }) -export type ResolveFunction = (wantedDependency: WantedDependency, opts: ResolveOptions) => Promise +export type ResolveFunction = (wantedDependency: WantedDependency & { optional?: boolean }, opts: ResolveOptions) => Promise