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

fix(assertions): stack overflow while parsing template #26767

Merged
merged 2 commits into from
Aug 18, 2023
Merged

fix(assertions): stack overflow while parsing template #26767

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8469648
fix(assertions): stack overflow while parsing template
Chriscbr Aug 15, 2023
55f3239
Merge branch 'main' into fix-cyclic
mergify[bot] Aug 18, 2023
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
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/assertions/lib/private/cyclic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,12 @@ function findCycle(deps: ReadonlyMap<string, ReadonlySet<string>>): string[] {

function recurse(node: string, path: string[]): string[] | undefined {
for (const dep of deps.get(node) ?? []) {
if (dep === path[0]) { return [...path, dep]; }
if (path.includes(dep)) { return [...path, dep]; }

const cycle = recurse(dep, [...path, dep]);
if (cycle) { return cycle; }
}

return undefined;
}
}
}
23 changes: 23 additions & 0 deletions packages/aws-cdk-lib/assertions/test/template.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,29 @@ describe('Template', () => {
}).toThrow(/dependency cycle/);
});

test('throws when given a more complex template with cyclic dependencies', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

While i can see how the change in cyclic.ts makes the recurse function more efficient, this test isn't going to show that one way or another right? the test will succeed with the old code as well?

I'm prepared to just approve as is anyway, because this looks low risk and is hard to test, but want to make sure i have the whole picture first.

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 initially ran the test with the old code (unchanged) and it fails for me because of a stackoverflow error.
Screenshot 2023-08-17 at 5 46 06 PM

I think the call stack in the test case I added ends up looking like this (without the fix applied):

findCycle(deps)
recurse("Res1", ["Res1"])
recurse("Res2", ["Res1", "Res2"])
recurse("Res3", ["Res1", "Res2", "Res3"])
recurse("Res2", ["Res1", "Res2", "Res3", "Res2"])
recurse("Res3", ["Res1", "Res2", "Res3", "Res2", "Res3"])
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah i see

expect(() => {
Template.fromJSON({
Resources: {
Res1: {
Type: 'Foo',
Properties: {
Thing: { Ref: 'Res2' },
},
},
Res2: {
Type: 'Foo',
DependsOn: ['Res3'],
},
Res3: {
Type: 'Foo',
DependsOn: ['Res2'],
},
},
});
}).toThrow(/dependency cycle/);
});

test('does not throw when given a template with cyclic dependencies if check is skipped', () => {
expect(() => {
Template.fromJSON({
Expand Down