From c4ecd9636087332d8ae9bc5e120d890e8c677f35 Mon Sep 17 00:00:00 2001 From: Mike Date: Tue, 14 Dec 2021 08:50:40 -0600 Subject: [PATCH] fix(aws-lambda-nodejs): use closest lockfile when autodetecting (#16629) fixes #15847 A bug in the automatic lockfile finding logic causes lockfiles higher in the directory tree to be used over lower/closer ones. This is because the code traverses the tree once per lockfile type in series, stopping when it finds one: https://github.com/aws/aws-cdk/blob/58fda9104ad884026d578dc0602f7d64dd533f6d/packages/%40aws-cdk/aws-lambda-nodejs/lib/function.ts#L137-L139 This updates the code to traverse the tree once looking for all the lockfile types at the same time and stop when one or more is found. If multiple are found at the same level, an error is thrown (per https://github.com/aws/aws-cdk/issues/15847#issuecomment-903830384). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-lambda-nodejs/lib/function.ts | 17 ++++--- .../@aws-cdk/aws-lambda-nodejs/lib/util.ts | 25 ++++++++--- .../aws-lambda-nodejs/test/util.test.ts | 45 ++++++++++++++++++- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts index 0bb00a2c35e5c..171df8ccbf385 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts @@ -5,7 +5,7 @@ import { Architecture } from '@aws-cdk/aws-lambda'; import { Bundling } from './bundling'; import { PackageManager } from './package-manager'; import { BundlingOptions } from './types'; -import { callsites, findUp } from './util'; +import { callsites, findUpMultiple } from './util'; // keep this import separate from other imports to reduce chance for merge conflicts with v2-main // eslint-disable-next-line no-duplicate-imports, import/order @@ -137,15 +137,20 @@ function findLockFile(depsLockFilePath?: string): string { return path.resolve(depsLockFilePath); } - const lockFile = findUp(PackageManager.PNPM.lockFile) - ?? findUp(PackageManager.YARN.lockFile) - ?? findUp(PackageManager.NPM.lockFile); + const lockFiles = findUpMultiple([ + PackageManager.PNPM.lockFile, + PackageManager.YARN.lockFile, + PackageManager.NPM.lockFile, + ]); - if (!lockFile) { + if (lockFiles.length === 0) { throw new Error('Cannot find a package lock file (`pnpm-lock.yaml`, `yarn.lock` or `package-lock.json`). Please specify it with `depsFileLockPath`.'); } + if (lockFiles.length > 1) { + throw new Error(`Multiple package lock files found: ${lockFiles.join(', ')}. Please specify the desired one with \`depsFileLockPath\`.`); + } - return lockFile; + return lockFiles[0]; } /** diff --git a/packages/@aws-cdk/aws-lambda-nodejs/lib/util.ts b/packages/@aws-cdk/aws-lambda-nodejs/lib/util.ts index bc754185cf7a6..0ececb74ab95f 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/lib/util.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/lib/util.ts @@ -35,19 +35,34 @@ export function callsites(): CallSite[] { * Find a file by walking up parent directories */ export function findUp(name: string, directory: string = process.cwd()): string | undefined { + return findUpMultiple([name], directory)[0]; +} + +/** + * Find the lowest of multiple files by walking up parent directories. If + * multiple files exist at the same level, they will all be returned. + */ +export function findUpMultiple(names: string[], directory: string = process.cwd()): string[] { const absoluteDirectory = path.resolve(directory); - const file = path.join(directory, name); - if (fs.existsSync(file)) { - return file; + const files = []; + for (const name of names) { + const file = path.join(directory, name); + if (fs.existsSync(file)) { + files.push(file); + } + } + + if (files.length > 0) { + return files; } const { root } = path.parse(absoluteDirectory); if (absoluteDirectory === root) { - return undefined; + return []; } - return findUp(name, path.dirname(absoluteDirectory)); + return findUpMultiple(names, path.dirname(absoluteDirectory)); } /** diff --git a/packages/@aws-cdk/aws-lambda-nodejs/test/util.test.ts b/packages/@aws-cdk/aws-lambda-nodejs/test/util.test.ts index ea68bbffa156b..d2249f4f59118 100644 --- a/packages/@aws-cdk/aws-lambda-nodejs/test/util.test.ts +++ b/packages/@aws-cdk/aws-lambda-nodejs/test/util.test.ts @@ -1,7 +1,7 @@ import * as child_process from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; -import { callsites, exec, extractDependencies, findUp } from '../lib/util'; +import { callsites, exec, extractDependencies, findUp, findUpMultiple } from '../lib/util'; beforeEach(() => { jest.clearAllMocks(); @@ -33,6 +33,49 @@ describe('findUp', () => { }); }); +describe('findUpMultiple', () => { + test('Starting at process.cwd()', () => { + const files = findUpMultiple(['README.md', 'package.json']); + expect(files).toHaveLength(2); + expect(files[0]).toMatch(/aws-lambda-nodejs\/README\.md$/); + expect(files[1]).toMatch(/aws-lambda-nodejs\/package\.json$/); + }); + + test('Non existing files', () => { + expect(findUpMultiple(['non-existing-file.unknown', 'non-existing-file.unknown2'])).toEqual([]); + }); + + test('Existing and non existing files', () => { + const files = findUpMultiple(['non-existing-file.unknown', 'README.md']); + expect(files).toHaveLength(1); + expect(files[0]).toMatch(/aws-lambda-nodejs\/README\.md$/); + }); + + test('Starting at a specific path', () => { + const files = findUpMultiple(['util.test.ts', 'function.test.ts'], path.join(__dirname, 'integ-handlers')); + expect(files).toHaveLength(2); + expect(files[0]).toMatch(/aws-lambda-nodejs\/test\/util\.test\.ts$/); + expect(files[1]).toMatch(/aws-lambda-nodejs\/test\/function\.test\.ts$/); + }); + + test('Non existing files starting at a non existing relative path', () => { + expect(findUpMultiple(['not-to-be-found.txt', 'not-to-be-found2.txt'], 'non-existing/relative/path')).toEqual([]); + }); + + test('Starting at a relative path', () => { + const files = findUpMultiple(['util.test.ts', 'function.test.ts'], 'test/integ-handlers'); + expect(files).toHaveLength(2); + expect(files[0]).toMatch(/aws-lambda-nodejs\/test\/util\.test\.ts$/); + expect(files[1]).toMatch(/aws-lambda-nodejs\/test\/function\.test\.ts$/); + }); + + test('Files on multiple levels', () => { + const files = findUpMultiple(['README.md', 'util.test.ts'], path.join(__dirname, 'integ-handlers')); + expect(files).toHaveLength(1); + expect(files[0]).toMatch(/aws-lambda-nodejs\/test\/util\.test\.ts$/); + }); +}); + describe('exec', () => { test('normal execution', () => { const spawnSyncMock = jest.spyOn(child_process, 'spawnSync').mockReturnValue({