Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(s3-assets): throw if path property is empty #29425

Merged
merged 2 commits into from
Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/aws-cdk-lib/aws-lambda/test/code.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ describe('code', () => {
});

describe('lambda.Code.fromAsset', () => {
test('fails if path is empty', () => {
// GIVEN
const fileAsset = lambda.Code.fromAsset('');

// THEN
expect(() => defineFunction(fileAsset)).toThrow(/Asset path cannot be empty/);
});
test('fails if path does not exist', () => {
// GIVEN
const fileAsset = lambda.Code.fromAsset('/path/not/found/' + Math.random() * 999999);

// THEN
expect(() => defineFunction(fileAsset)).toThrow(/Cannot find asset/);
});
test('fails if a non-zip asset is used', () => {
// GIVEN
const fileAsset = lambda.Code.fromAsset(path.join(__dirname, 'my-lambda-handler', 'index.py'));
Expand Down
4 changes: 4 additions & 0 deletions packages/aws-cdk-lib/aws-s3-assets/lib/asset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ export class Asset extends Construct implements cdk.IAsset {
constructor(scope: Construct, id: string, props: AssetProps) {
super(scope, id);

if (!props.path) {
throw new Error('Asset path cannot be empty');
}
Comment on lines +140 to +142
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be a good idea to check that cdk.out is excluded from the asset source, as Code.fromAsset('.') would cause the same reported issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call out, I'm good with the changes in this PR so I'll approve for now but feel free to add that check in a separate PR.


this.isBundled = props.bundling != null;

// stage the asset source (conditionally).
Expand Down
9 changes: 8 additions & 1 deletion packages/aws-cdk-lib/aws-s3-assets/test/asset.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,18 @@ test('"readers" or "grantRead" can be used to grant read permissions on the asse
});
});

test('fails if path is empty', () => {
const stack = new cdk.Stack();
expect(() => new Asset(stack, 'MyDirectory', {
path: '',
})).toThrow(/Asset path cannot be empty/);
});

test('fails if directory not found', () => {
const stack = new cdk.Stack();
expect(() => new Asset(stack, 'MyDirectory', {
path: '/path/not/found/' + Math.random() * 999999,
})).toThrow();
})).toThrow(/Cannot find asset/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just throws inherently, correct? Possible to catch this in asset.ts and add the path: Cannot find asset at path: <path> ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to

if (!fs.existsSync(this.sourcePath)) {
throw new Error(`Cannot find asset at ${this.sourcePath}`);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to refine the existing test to help differentiate between the two errors, since I was adding a new one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it now 👍🏼

});

test('multiple assets under the same parent', () => {
Expand Down
Loading