fix(assertions): throw error or warn when `synth` is called multiple times on mutated construct tree by sumupitchayan · Pull Request #31865 · aws/aws-cdk

sumupitchayan · 2024-10-23T14:54:41Z

Reason for this change

Calling Template.fromStack(stack) twice on the same stack object will throw the error Unable to find artifact with id if the stack is mutated after the first Template.fromStack(stack) call. This is because synth should only be called once - from the comment on the synth function:

Once an assembly has been synthesized, it cannot be modified. Subsequent calls will return the same assembly.

Second call of Template.fromStack(stack) tries to find the mutated stack since app.synth() caches and returns the assembly from the first synth call.

Description of changes

This PR checks to see whether or not the construct tree has been each time synth() is called - if it has, then we either throw an error or a warning. We will only throw a warning if synth is being called from process.once('beforeExit').

Description of how you validated changes

Unit test with the customer's same example case, asserting than an error is thrown when synth is called twice after a stack has been mutated after the first synth call.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Signed-off-by: Sumu <sumughan@amazon.com>

…s are correctly merged) - move Template.stack calls after stack mutations Signed-off-by: Sumu <sumughan@amazon.com>

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan · 2024-10-28T16:11:54Z

packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts

    }),
  });

+  new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-2', {


This existing pipelines test had to be changed to not throw an error - all I did was move the stack mutations before the Template.fromStack() calls

That's perfect!

packages/aws-cdk-lib/core/lib/stage.ts

Signed-off-by: Sumu <sumughan@amazon.com>

packages/aws-cdk-lib/core/lib/stage.ts

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>

Signed-off-by: Sumu <sumughan@amazon.com>

packages/aws-cdk-lib/core/lib/stage.ts

mrgrain · 2024-10-29T10:37:24Z

packages/aws-cdk-lib/core/lib/stage.ts

+        throw new Error(errorMessage);
+      } else {
+        // eslint-disable-next-line no-console
+        console.error(errorMessage);


So we print this as a warning. And now what? The message text implies that we are in the warning situation and is trying to say the following:

Hey we have detected that you called synth() multiple and changed the construct tree after the first synth()

We are going to ignoring all changes, but are not failing the execution.

You should stop doing this, unless you really know what you do.

mrgrain · 2024-10-29T10:39:29Z

packages/aws-cdk-lib/core/lib/stage.ts

+    if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) {
+      const errorMessage = 'The construct tree has been modified after synthesis. Only the results of the first synth() call are written to disk, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called.';
+      if (options.errorOnDuplicateSynth ?? true) {
+        throw new Error(errorMessage);


Not sure we can really use the exact same message. the call to action is different when there is an error vs a warning. In my head, the error scenario is this:

Hey we have detected that you called synth() multiple times and changed the construct tree after the first synth()

You cannot do this, we are failing the execution.

If you really know what you are doing, you can set this flag to keep going:

mrgrain

Much improvement on the error messages. Still not 100% happy though.

Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>

Signed-off-by: Sumu <sumughan@amazon.com>

packages/aws-cdk-lib/core/lib/stage.ts

Signed-off-by: Sumu <sumughan@amazon.com>

packages/aws-cdk-lib/core/lib/stage.ts

Signed-off-by: Sumu <sumughan@amazon.com>

mrgrain

Conditionally approved until you've checked that addWarningV2() really doesn't work.

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan · 2024-10-29T22:00:03Z

Conditionally approved until you've checked that addWarningV2() really doesn't work.

Just tested and confirmed that addWarningV2() doesn't work - going with Rico's console.error() suggestion

aws-cdk-automation · 2024-10-29T22:31:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 4fec523
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-10-29T22:31:54Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-10-29T22:32:13Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

sumupitchayan added 2 commits October 23, 2024 10:49

WIP - add unit test to synthesis.test.ts

c4e1795

Signed-off-by: Sumu <sumughan@amazon.com>

remove assembly caching from stage.ts synth() function

fd13c07

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation requested a review from a team October 23, 2024 14:54

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Oct 23, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 23, 2024

aws-cdk-automation previously requested changes Oct 23, 2024

View reviewed changes

sumupitchayan added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Oct 23, 2024

sumupitchayan added 6 commits October 23, 2024 11:07

fix import order in synthesis.test.ts

f780751

Signed-off-by: Sumu <sumughan@amazon.com>

WIP - try different approach, comparing construct path sets

299733d

Signed-off-by: Sumu <sumughan@amazon.com>

fix linter errors

31afea1

Signed-off-by: Sumu <sumughan@amazon.com>

WIP - cache after running synth

e9cbc48

Signed-off-by: Sumu <sumughan@amazon.com>

fix pipelines test (environment variables specified in multiple place…

1c5cff1

…s are correctly merged) - move Template.stack calls after stack mutations Signed-off-by: Sumu <sumughan@amazon.com>

WIP - update unit test for this issue

c111479

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan commented Oct 28, 2024

View reviewed changes

mrgrain reviewed Oct 28, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Outdated Show resolved Hide resolved

mrgrain reviewed Oct 28, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Outdated Show resolved Hide resolved

sumupitchayan changed the title ~~fix(assertions): remove assembly caching from synth~~ fix(assertions): throw error or warn when synth is called multiple times on mutated construct tree Oct 28, 2024

sumupitchayan added 2 commits October 28, 2024 12:41

WIP - improve error/warning messages

f7e70bb

Signed-off-by: Sumu <sumughan@amazon.com>

typo

8e6a194

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan marked this pull request as ready for review October 28, 2024 16:42

Merge branch 'main' into sumughan/fix-assertions-unable-to-find-artifact

8798232

rix0rrr requested changes Oct 28, 2024

View reviewed changes

sumupitchayan and others added 5 commits October 28, 2024 14:47

Update packages/aws-cdk-lib/core/lib/stage.ts

de57d44

Co-authored-by: Rico Hermans <rix0rrr@gmail.com>

rename warnInsteadOfError to errorOnDuplicateSynth

af20ae7

Signed-off-by: Sumu <sumughan@amazon.com>

improve error message

9a2e089

Signed-off-by: Sumu <sumughan@amazon.com>

make 2 helper functions top-level

a78ab0e

Signed-off-by: Sumu <sumughan@amazon.com>

eslint comment to allow console.error statement

c7430d8

Signed-off-by: Sumu <sumughan@amazon.com>

mrgrain reviewed Oct 29, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Outdated Show resolved Hide resolved

mrgrain reviewed Oct 29, 2024

View reviewed changes

mrgrain requested changes Oct 29, 2024

View reviewed changes

rix0rrr approved these changes Oct 29, 2024

View reviewed changes

sumupitchayan and others added 4 commits October 29, 2024 11:43

Update packages/aws-cdk-lib/core/lib/stage.ts

ac524e1

Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>

rename parameter in recurse funciton

280e2e2

Signed-off-by: Sumu <sumughan@amazon.com>

WIP Momo feedback: improve err/warning messages

39cc2fd

Signed-off-by: Sumu <sumughan@amazon.com>

edit test err msg assertion

c133601

Signed-off-by: Sumu <sumughan@amazon.com>

mrgrain reviewed Oct 29, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Outdated Show resolved Hide resolved

mrgrain reviewed Oct 29, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Outdated Show resolved Hide resolved

sumupitchayan added 2 commits October 29, 2024 13:46

WIP err msgs again

02f405c

Signed-off-by: Sumu <sumughan@amazon.com>

WIP err msgs again x2

b68519a

Signed-off-by: Sumu <sumughan@amazon.com>

mrgrain reviewed Oct 29, 2024

View reviewed changes

packages/aws-cdk-lib/core/lib/stage.ts Show resolved Hide resolved

WIP err msgs 3x

e8b866b

Signed-off-by: Sumu <sumughan@amazon.com>

mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Oct 29, 2024

mrgrain approved these changes Oct 29, 2024

View reviewed changes

remove force option from error throwing in synth

6f098be

Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'main' into sumughan/fix-assertions-unable-to-find-artifact

4fec523

sumupitchayan removed the pr/do-not-merge This PR should not be merged at this time. label Oct 29, 2024

sumupitchayan self-assigned this Oct 29, 2024

mergify bot merged commit a261c9d into main Oct 29, 2024

mergify bot deleted the sumughan/fix-assertions-unable-to-find-artifact branch October 29, 2024 22:31

github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2024

Conversation

sumupitchayan commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

sumupitchayan Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

rix0rrr Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

mrgrain Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

sumupitchayan commented Oct 29, 2024

Uh oh!

aws-cdk-automation commented Oct 29, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Oct 29, 2024

Uh oh!

github-actions bot commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sumupitchayan commented Oct 23, 2024 •

edited

Loading