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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73cc174
fix(s3-assets): throw if path property is empty
nmussy Mar 10, 2024
74ecaa4
Merge branch 'main' into fix-s3-assets-throw-empty-path
mergify[bot] Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/aws-cdk-lib/aws-lambda/test/code.test.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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

aws-cdk/packages/aws-cdk-lib/core/lib/asset-staging.ts

Lines 174 to 176 in e63c777

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