Skip to content

Commit

Permalink
fix(aws-lambda-nodejs): use closest lockfile when autodetecting (#16629)
Browse files Browse the repository at this point in the history
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 #15847 (comment)).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
yo1dog committed Dec 14, 2021
1 parent 2fc6895 commit c4ecd96
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 12 deletions.
17 changes: 11 additions & 6 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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];
}

/**
Expand Down
25 changes: 20 additions & 5 deletions packages/@aws-cdk/aws-lambda-nodejs/lib/util.ts
Expand Up @@ -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));
}

/**
Expand Down
45 changes: 44 additions & 1 deletion 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();
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit c4ecd96

Please sign in to comment.