From c4e17956459140bc74d53ad022a3e5dc35ccfc64 Mon Sep 17 00:00:00 2001 From: Sumu Date: Wed, 23 Oct 2024 10:49:08 -0400 Subject: [PATCH 01/23] WIP - add unit test to synthesis.test.ts Signed-off-by: Sumu --- .../aws-cdk-lib/core/test/synthesis.test.ts | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index 8b67b371e0be8..bdfb607a582d8 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -6,6 +6,7 @@ import { Construct } from 'constructs'; import * as cxschema from '../../cloud-assembly-schema'; import * as cxapi from '../../cx-api'; import * as cdk from '../lib'; +import { Template } from '../../assertions'; import { synthesize } from '../lib/private/synthesis'; function createModernApp() { @@ -362,6 +363,26 @@ describe('synthesis', () => { }); + test('can call synth multiple times without caching assembly', () => { + const app = new cdk.App(); + + const stages = [ + { + stage: 'PROD', + }, + { + stage: 'BETA', + }, + ]; + + stages.forEach(({ stage }) => { + const stack = new cdk.Stack(app, `${stage}-Stack`, {}); + // Assert that `Unable to find artifact with id` error is not thrown + expect(() => { + Template.fromStack(stack); + }).not.toThrow(); + }); + }); }); function list(outdir: string) { From fd13c0704a92265851a82753aab5c042cf0a3f4e Mon Sep 17 00:00:00 2001 From: Sumu Date: Wed, 23 Oct 2024 10:50:01 -0400 Subject: [PATCH 02/23] remove assembly caching from stage.ts synth() function Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index f48ff158b9b54..14b1b6fa9176c 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -141,11 +141,6 @@ export class Stage extends Construct { */ public readonly parentStage?: Stage; - /** - * The cached assembly if it was already built - */ - private assembly?: cxapi.CloudAssembly; - /** * Validation plugins to run during synthesis. If any plugin reports any violation, * synthesis will be interrupted and the report displayed to the user. @@ -210,14 +205,10 @@ export class Stage extends Construct { * calls will return the same assembly. */ public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly { - if (!this.assembly || options.force) { - this.assembly = synthesize(this, { - skipValidation: options.skipValidation, - validateOnSynthesis: options.validateOnSynthesis, - }); - } - - return this.assembly; + return synthesize(this, { + skipValidation: options.skipValidation, + validateOnSynthesis: options.validateOnSynthesis, + }); } private createBuilder(outdir?: string) { From f780751f704056f312aca11d87b912348e857326 Mon Sep 17 00:00:00 2001 From: Sumu Date: Wed, 23 Oct 2024 11:07:25 -0400 Subject: [PATCH 03/23] fix import order in synthesis.test.ts Signed-off-by: Sumu --- packages/aws-cdk-lib/core/test/synthesis.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index bdfb607a582d8..a5af571b696f7 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -3,10 +3,10 @@ import * as os from 'os'; import * as path from 'path'; import { testDeprecated } from '@aws-cdk/cdk-build-tools'; import { Construct } from 'constructs'; +import { Template } from '../../assertions'; import * as cxschema from '../../cloud-assembly-schema'; import * as cxapi from '../../cx-api'; import * as cdk from '../lib'; -import { Template } from '../../assertions'; import { synthesize } from '../lib/private/synthesis'; function createModernApp() { From 299733d5530c2acbebcf2e9e1f41ae25716da38b Mon Sep 17 00:00:00 2001 From: Sumu Date: Thu, 24 Oct 2024 21:05:53 -0400 Subject: [PATCH 04/23] WIP - try different approach, comparing construct path sets Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/app.ts | 2 +- packages/aws-cdk-lib/core/lib/stage.ts | 66 ++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/app.ts b/packages/aws-cdk-lib/core/lib/app.ts index d24ec829f00df..adbbf2a7b1be1 100644 --- a/packages/aws-cdk-lib/core/lib/app.ts +++ b/packages/aws-cdk-lib/core/lib/app.ts @@ -192,7 +192,7 @@ export class App extends Stage { if (autoSynth) { // synth() guarantees it will only execute once, so a default of 'true' // doesn't bite manual calling of the function. - process.once('beforeExit', () => this.synth()); + process.once('beforeExit', () => this.synth({warnInsteadOfError: true})); } this._treeMetadata = props.treeMetadata ?? true; diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 14b1b6fa9176c..0ed6a0bf5de67 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -4,6 +4,7 @@ import { PermissionsBoundary } from './permissions-boundary'; import { synthesize } from './private/synthesis'; import { IPolicyValidationPluginBeta1 } from './validation'; import * as cxapi from '../../cx-api'; +import { Annotations } from './annotations'; const STAGE_SYMBOL = Symbol.for('@aws-cdk/core.Stage'); @@ -141,6 +142,16 @@ export class Stage extends Construct { */ public readonly parentStage?: Stage; + /** + * The cached assembly if it was already built + */ + private assembly?: cxapi.CloudAssembly; + + /** + * The cached set of construct paths. Empty if assembly was not yet built. + */ + private constructPathsCache: Set; + /** * Validation plugins to run during synthesis. If any plugin reports any violation, * synthesis will be interrupted and the report displayed to the user. @@ -158,6 +169,7 @@ export class Stage extends Construct { Object.defineProperty(this, STAGE_SYMBOL, { value: true }); + this.constructPathsCache = new Set(); this.parentStage = Stage.of(this); this.region = props.env?.region ?? this.parentStage?.region; @@ -205,10 +217,49 @@ export class Stage extends Construct { * calls will return the same assembly. */ public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly { - return synthesize(this, { - skipValidation: options.skipValidation, - validateOnSynthesis: options.validateOnSynthesis, - }); + + const newConstructPaths = new Set(); + recurseAndlistAllConstructPaths(this); + + // Lists all construct paths + function recurseAndlistAllConstructPaths(construct: IConstruct) { + newConstructPaths.add(construct.node.path); + for (const child of construct.node.children) { + if (!Stage.isStage(child)) { + recurseAndlistAllConstructPaths(child); + } + } + } + + // If the assembly cache is uninitiazed, run synthesize. + if (this.constructPathsCache.size == 0 || !this.assembly || options.force) { + this.constructPathsCache = newConstructPaths; + this.assembly = synthesize(this, { + skipValidation: options.skipValidation, + validateOnSynthesis: options.validateOnSynthesis, + }); + } + + // If the construct paths set has changed + if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { + if (options.warnInsteadOfError) { + Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'Construct paths have been modified since the last synth call.'); + } else { + throw new Error('Synth was called since the last call.'); + } + } + + function constructPathSetsAreEqual(set1: Set, set2: Set): boolean { + if (set1.size !== set2.size) return false; + for (const id of set1) { + if (!set2.has(id)) { + return false; + } + } + return true; + } + + return this.assembly; } private createBuilder(outdir?: string) { @@ -250,4 +301,11 @@ export interface StageSynthesisOptions { * @default false */ readonly force?: boolean; + + /** + * Whether or not to throw a warning instead of an error if the construct tree has + * been mutated since the last synth. + * @default false + */ + readonly warnInsteadOfError?: boolean; } From 31afea10c17ef6368e69e71c3ee5f91c4d7c3e35 Mon Sep 17 00:00:00 2001 From: Sumu Date: Fri, 25 Oct 2024 10:34:25 -0400 Subject: [PATCH 05/23] fix linter errors Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/app.ts | 2 +- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/app.ts b/packages/aws-cdk-lib/core/lib/app.ts index adbbf2a7b1be1..a1be81ba46ddc 100644 --- a/packages/aws-cdk-lib/core/lib/app.ts +++ b/packages/aws-cdk-lib/core/lib/app.ts @@ -192,7 +192,7 @@ export class App extends Stage { if (autoSynth) { // synth() guarantees it will only execute once, so a default of 'true' // doesn't bite manual calling of the function. - process.once('beforeExit', () => this.synth({warnInsteadOfError: true})); + process.once('beforeExit', () => this.synth({ warnInsteadOfError: true })); } this._treeMetadata = props.treeMetadata ?? true; diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 0ed6a0bf5de67..2de7923ad8894 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -1,10 +1,10 @@ import { IConstruct, Construct, Node } from 'constructs'; +import { Annotations } from './annotations'; import { Environment } from './environment'; import { PermissionsBoundary } from './permissions-boundary'; import { synthesize } from './private/synthesis'; import { IPolicyValidationPluginBeta1 } from './validation'; import * as cxapi from '../../cx-api'; -import { Annotations } from './annotations'; const STAGE_SYMBOL = Symbol.for('@aws-cdk/core.Stage'); From e9cbc48ada86b3064061a0a511920aed0bffd45f Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 11:32:09 -0400 Subject: [PATCH 06/23] WIP - cache after running synth Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 2de7923ad8894..8f204bfa4948c 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -221,6 +221,16 @@ export class Stage extends Construct { const newConstructPaths = new Set(); recurseAndlistAllConstructPaths(this); + // If the assembly cache is uninitiazed, run synthesize and re-run list all construvt paths + if (this.constructPathsCache.size == 0 || !this.assembly || options.force) { + this.assembly = synthesize(this, { + skipValidation: options.skipValidation, + validateOnSynthesis: options.validateOnSynthesis, + }); + recurseAndlistAllConstructPaths(this); + this.constructPathsCache = newConstructPaths; + } + // Lists all construct paths function recurseAndlistAllConstructPaths(construct: IConstruct) { newConstructPaths.add(construct.node.path); @@ -231,15 +241,6 @@ export class Stage extends Construct { } } - // If the assembly cache is uninitiazed, run synthesize. - if (this.constructPathsCache.size == 0 || !this.assembly || options.force) { - this.constructPathsCache = newConstructPaths; - this.assembly = synthesize(this, { - skipValidation: options.skipValidation, - validateOnSynthesis: options.validateOnSynthesis, - }); - } - // If the construct paths set has changed if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { if (options.warnInsteadOfError) { @@ -259,6 +260,9 @@ export class Stage extends Construct { return true; } + // Reset construct paths + this.constructPathsCache = newConstructPaths; + return this.assembly; } From 1c5cff1283143cbbe7c4350b476ecc18496530b5 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 11:56:02 -0400 Subject: [PATCH 07/23] fix pipelines test (environment variables specified in multiple places are correctly merged) - move Template.stack calls after stack mutations Signed-off-by: Sumu --- .../pipelines/test/compliance/synths.test.ts | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts b/packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts index ce52f6a2df0a8..a0555f1a28800 100644 --- a/packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts +++ b/packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts @@ -186,6 +186,27 @@ test('CodeBuild: environment variables specified in multiple places are correctl }), }); + new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-2', { + synth: new cdkp.CodeBuildStep('Synth', { + input: cdkp.CodePipelineSource.gitHub('test/test', 'main'), + primaryOutputDirectory: '.', + env: { + SOME_ENV_VAR: 'SomeValue', + }, + installCommands: [ + 'install1', + 'install2', + ], + commands: ['synth'], + buildEnvironment: { + environmentVariables: { + INNER_VAR: { value: 'InnerValue' }, + }, + privileged: true, + }, + }), + }); + // THEN Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodeBuild::Project', { Environment: Match.objectLike({ @@ -217,27 +238,6 @@ test('CodeBuild: environment variables specified in multiple places are correctl }, }); - new ModernTestGitHubNpmPipeline(pipelineStack, 'Cdk-2', { - synth: new cdkp.CodeBuildStep('Synth', { - input: cdkp.CodePipelineSource.gitHub('test/test', 'main'), - primaryOutputDirectory: '.', - env: { - SOME_ENV_VAR: 'SomeValue', - }, - installCommands: [ - 'install1', - 'install2', - ], - commands: ['synth'], - buildEnvironment: { - environmentVariables: { - INNER_VAR: { value: 'InnerValue' }, - }, - privileged: true, - }, - }), - }); - // THEN Template.fromStack(pipelineStack).hasResourceProperties('AWS::CodeBuild::Project', { Environment: Match.objectLike({ From c11147981af98df8ddd340f9c140102a46e53143 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 12:01:33 -0400 Subject: [PATCH 08/23] WIP - update unit test for this issue Signed-off-by: Sumu --- .../aws-cdk-lib/core/test/synthesis.test.ts | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index a5af571b696f7..1d2611e058897 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -363,7 +363,7 @@ describe('synthesis', () => { }); - test('can call synth multiple times without caching assembly', () => { + test('calling synth multiple times errors if construct tree is mutated', () => { const app = new cdk.App(); const stages = [ @@ -375,13 +375,17 @@ describe('synthesis', () => { }, ]; - stages.forEach(({ stage }) => { - const stack = new cdk.Stack(app, `${stage}-Stack`, {}); - // Assert that `Unable to find artifact with id` error is not thrown - expect(() => { - Template.fromStack(stack); - }).not.toThrow(); - }); + // THEN - no error the first time synth is called + let stack = new cdk.Stack(app, `${stages[0].stage}-Stack`, {}); + expect(() => { + Template.fromStack(stack); + }).not.toThrow(); + + // THEN - error is thrown since synth was called with mutated stack name + stack = new cdk.Stack(app, `${stages[1].stage}-Stack`, {}); + expect(() => { + Template.fromStack(stack); + }).toThrow('Synth was called since the last call'); }); }); From f7e70bbf5feeab08e04355a6bdefbd6449652b13 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 12:41:18 -0400 Subject: [PATCH 09/23] WIP - improve error/warning messages Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 6 +++--- packages/aws-cdk-lib/core/test/synthesis.test.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 8f204bfa4948c..55fac8a27b43e 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -244,9 +244,9 @@ export class Stage extends Construct { // If the construct paths set has changed if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { if (options.warnInsteadOfError) { - Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'Construct paths have been modified since the last synth call.'); + Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'The construct tree was modified since the last synth call. Re-synthesizing with modifications can lead to inconsistent states. For reliable results, avoid construct mutations between synth calls.'); } else { - throw new Error('Synth was called since the last call.'); + throw new Error('The construct tree was modified after the initial synthesis (synth) call. Avoid modifying the construct tree between synth calls as this can cause unexpected behavior.'); } } @@ -260,7 +260,7 @@ export class Stage extends Construct { return true; } - // Reset construct paths + // Reset construct paths cache this.constructPathsCache = newConstructPaths; return this.assembly; diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index 1d2611e058897..2a718071e5fe4 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -385,7 +385,7 @@ describe('synthesis', () => { stack = new cdk.Stack(app, `${stages[1].stage}-Stack`, {}); expect(() => { Template.fromStack(stack); - }).toThrow('Synth was called since the last call'); + }).toThrow('The construct tree was modified after the initial synthesis (synth) call.'); }); }); From 8e6a1941c2ab8f6f446f79a1803eb03d00bf43ed Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 12:42:26 -0400 Subject: [PATCH 10/23] typo Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 55fac8a27b43e..723527fc3688c 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -221,7 +221,7 @@ export class Stage extends Construct { const newConstructPaths = new Set(); recurseAndlistAllConstructPaths(this); - // If the assembly cache is uninitiazed, run synthesize and re-run list all construvt paths + // If the assembly cache is uninitiazed, run synthesize and reset construct paths cache if (this.constructPathsCache.size == 0 || !this.assembly || options.force) { this.assembly = synthesize(this, { skipValidation: options.skipValidation, From de57d44d910f29d0581089e2dedf015f6bdb5946 Mon Sep 17 00:00:00 2001 From: Sumu Pitchayan <35242245+sumupitchayan@users.noreply.github.com> Date: Mon, 28 Oct 2024 14:47:25 -0400 Subject: [PATCH 11/23] Update packages/aws-cdk-lib/core/lib/stage.ts Co-authored-by: Rico Hermans --- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 723527fc3688c..f309e846f51e5 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -311,5 +311,5 @@ export interface StageSynthesisOptions { * been mutated since the last synth. * @default false */ - readonly warnInsteadOfError?: boolean; + readonly errorOnDuplicateSynth?: boolean; } From af20ae7c8e0f2526ac79b3927455d2f254753a09 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 15:04:45 -0400 Subject: [PATCH 12/23] rename warnInsteadOfError to errorOnDuplicateSynth Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/app.ts | 2 +- packages/aws-cdk-lib/core/lib/stage.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/app.ts b/packages/aws-cdk-lib/core/lib/app.ts index a1be81ba46ddc..019919c8b85d4 100644 --- a/packages/aws-cdk-lib/core/lib/app.ts +++ b/packages/aws-cdk-lib/core/lib/app.ts @@ -192,7 +192,7 @@ export class App extends Stage { if (autoSynth) { // synth() guarantees it will only execute once, so a default of 'true' // doesn't bite manual calling of the function. - process.once('beforeExit', () => this.synth({ warnInsteadOfError: true })); + process.once('beforeExit', () => this.synth({ errorOnDuplicateSynth: false })); } this._treeMetadata = props.treeMetadata ?? true; diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index f309e846f51e5..1606ca12ed0ee 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -243,10 +243,10 @@ export class Stage extends Construct { // If the construct paths set has changed if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { - if (options.warnInsteadOfError) { - Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'The construct tree was modified since the last synth call. Re-synthesizing with modifications can lead to inconsistent states. For reliable results, avoid construct mutations between synth calls.'); - } else { + if (options.errorOnDuplicateSynth) { throw new Error('The construct tree was modified after the initial synthesis (synth) call. Avoid modifying the construct tree between synth calls as this can cause unexpected behavior.'); + } else { + Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'The construct tree was modified since the last synth call. Re-synthesizing with modifications can lead to inconsistent states. For reliable results, avoid construct mutations between synth calls.'); } } @@ -309,7 +309,7 @@ export interface StageSynthesisOptions { /** * Whether or not to throw a warning instead of an error if the construct tree has * been mutated since the last synth. - * @default false + * @default true */ readonly errorOnDuplicateSynth?: boolean; } From 9a2e0896c66f44699a1dd4c6fb6a2ec0453a93c9 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 15:17:48 -0400 Subject: [PATCH 13/23] improve error message Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 8 ++++---- packages/aws-cdk-lib/core/test/synthesis.test.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 1606ca12ed0ee..420e869231dda 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -1,5 +1,4 @@ import { IConstruct, Construct, Node } from 'constructs'; -import { Annotations } from './annotations'; import { Environment } from './environment'; import { PermissionsBoundary } from './permissions-boundary'; import { synthesize } from './private/synthesis'; @@ -243,10 +242,11 @@ export class Stage extends Construct { // If the construct paths set has changed if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { - if (options.errorOnDuplicateSynth) { - throw new Error('The construct tree was modified after the initial synthesis (synth) call. Avoid modifying the construct tree between synth calls as this can cause unexpected behavior.'); + 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); } else { - Annotations.of(this).addWarningV2('@aws-cdk/core:synth-warning', 'The construct tree was modified since the last synth call. Re-synthesizing with modifications can lead to inconsistent states. For reliable results, avoid construct mutations between synth calls.'); + console.error(errorMessage); } } diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index 2a718071e5fe4..642fdbd4abe09 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -385,7 +385,7 @@ describe('synthesis', () => { stack = new cdk.Stack(app, `${stages[1].stage}-Stack`, {}); expect(() => { Template.fromStack(stack); - }).toThrow('The construct tree was modified after the initial synthesis (synth) call.'); + }).toThrow('The construct tree has been modified after synthesis.'); }); }); From a78ab0e2006f4db28444fc297cb3b92eebf1b525 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 15:29:13 -0400 Subject: [PATCH 14/23] make 2 helper functions top-level Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 53 ++++++++++++++------------ 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 420e869231dda..181432af0ce49 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -217,8 +217,7 @@ export class Stage extends Construct { */ public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly { - const newConstructPaths = new Set(); - recurseAndlistAllConstructPaths(this); + let newConstructPaths = this.listAllConstructPaths(this); // If the assembly cache is uninitiazed, run synthesize and reset construct paths cache if (this.constructPathsCache.size == 0 || !this.assembly || options.force) { @@ -226,22 +225,12 @@ export class Stage extends Construct { skipValidation: options.skipValidation, validateOnSynthesis: options.validateOnSynthesis, }); - recurseAndlistAllConstructPaths(this); + newConstructPaths = this.listAllConstructPaths(this); this.constructPathsCache = newConstructPaths; } - // Lists all construct paths - function recurseAndlistAllConstructPaths(construct: IConstruct) { - newConstructPaths.add(construct.node.path); - for (const child of construct.node.children) { - if (!Stage.isStage(child)) { - recurseAndlistAllConstructPaths(child); - } - } - } - // If the construct paths set has changed - if (!constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { + 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); @@ -250,22 +239,38 @@ export class Stage extends Construct { } } - function constructPathSetsAreEqual(set1: Set, set2: Set): boolean { - if (set1.size !== set2.size) return false; - for (const id of set1) { - if (!set2.has(id)) { - return false; - } - } - return true; - } - // Reset construct paths cache this.constructPathsCache = newConstructPaths; return this.assembly; } + // Function that lists all construct paths and returns them as a set + private listAllConstructPaths(construct: IConstruct): Set { + const paths = new Set(); + function recurse(construct: IConstruct) { + paths.add(construct.node.path); + for (const child of construct.node.children) { + if (!Stage.isStage(child)) { + recurse(child); + } + } + } + recurse(construct); + return paths; + } + + // Checks if sets of construct paths are equal + private constructPathSetsAreEqual(set1: Set, set2: Set): boolean { + if (set1.size !== set2.size) return false; + for (const id of set1) { + if (!set2.has(id)) { + return false; + } + } + return true; + } + private createBuilder(outdir?: string) { // cannot specify "outdir" if we are a nested stage if (this.parentStage && outdir) { From c7430d82706e0c1ad8ea020b641fd549d14dc278 Mon Sep 17 00:00:00 2001 From: Sumu Date: Mon, 28 Oct 2024 15:31:23 -0400 Subject: [PATCH 15/23] eslint comment to allow console.error statement Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 181432af0ce49..8bbaf9578e272 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -235,6 +235,7 @@ export class Stage extends Construct { if (options.errorOnDuplicateSynth ?? true) { throw new Error(errorMessage); } else { + // eslint-disable-next-line no-console console.error(errorMessage); } } From ac524e106ad9719ea49eefeba6724cb3ebc2d723 Mon Sep 17 00:00:00 2001 From: Sumu Pitchayan <35242245+sumupitchayan@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:43:41 -0400 Subject: [PATCH 16/23] Update packages/aws-cdk-lib/core/lib/stage.ts Co-authored-by: Momo Kornher --- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 8bbaf9578e272..4c4fe3b7f9f7f 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -231,7 +231,7 @@ export class Stage extends Construct { // If the construct paths set has changed 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.'; + const errorMessage = 'The construct tree has been modified after synthesis. Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called.'; if (options.errorOnDuplicateSynth ?? true) { throw new Error(errorMessage); } else { From 280e2e28f91efed6bf2e0b4f146945c9f2b1ed0f Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 12:08:21 -0400 Subject: [PATCH 17/23] rename parameter in recurse funciton Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 4c4fe3b7f9f7f..a8184ac058de4 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -249,9 +249,9 @@ export class Stage extends Construct { // Function that lists all construct paths and returns them as a set private listAllConstructPaths(construct: IConstruct): Set { const paths = new Set(); - function recurse(construct: IConstruct) { - paths.add(construct.node.path); - for (const child of construct.node.children) { + function recurse(root: IConstruct) { + paths.add(root.node.path); + for (const child of root.node.children) { if (!Stage.isStage(child)) { recurse(child); } From 39cc2fd36c58fd2106b429bfbd3efb40fbeb3987 Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 13:27:35 -0400 Subject: [PATCH 18/23] WIP Momo feedback: improve err/warning messages Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index a8184ac058de4..f48dd3bb1c61b 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -231,12 +231,11 @@ export class Stage extends Construct { // If the construct paths set has changed 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 used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called.'; - if (options.errorOnDuplicateSynth ?? true) { - throw new Error(errorMessage); + if ((options.errorOnDuplicateSynth ?? true) && !options.force) { + throw new Error('Synthesis has been called multiple times and the construct tree was modified after the first synthesis. This is not allowed: if you intended to do this, then call synthesis with the --force flag set to true.'); } else { // eslint-disable-next-line no-console - console.error(errorMessage); + console.error('Synthesis has been called mulitple times and the construct tree was modified after the first synthesis. Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); } } From c133601083b5f735b7edecf4836f07d3979dd82e Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 13:29:52 -0400 Subject: [PATCH 19/23] edit test err msg assertion Signed-off-by: Sumu --- packages/aws-cdk-lib/core/test/synthesis.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/test/synthesis.test.ts b/packages/aws-cdk-lib/core/test/synthesis.test.ts index 642fdbd4abe09..90760a19d05da 100644 --- a/packages/aws-cdk-lib/core/test/synthesis.test.ts +++ b/packages/aws-cdk-lib/core/test/synthesis.test.ts @@ -385,7 +385,7 @@ describe('synthesis', () => { stack = new cdk.Stack(app, `${stages[1].stage}-Stack`, {}); expect(() => { Template.fromStack(stack); - }).toThrow('The construct tree has been modified after synthesis.'); + }).toThrow('Synthesis has been called multiple times and the construct tree was modified after the first synthesis'); }); }); From 02f405c7682d101dc86f0da4771997fdc4561827 Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 13:46:20 -0400 Subject: [PATCH 20/23] WIP err msgs again Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index f48dd3bb1c61b..1d890d7340aff 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -231,11 +231,12 @@ export class Stage extends Construct { // If the construct paths set has changed if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { + const errorMessage = 'Synthesis has been called multiple times and the construct tree was modified after the first synthesis.'; if ((options.errorOnDuplicateSynth ?? true) && !options.force) { - throw new Error('Synthesis has been called multiple times and the construct tree was modified after the first synthesis. This is not allowed: if you intended to do this, then call synthesis with the --force flag set to true.'); + throw new Error(errorMessage + 'This is not allowed: if you intended to do this, then call synth() with `errorOnDuplicateSynth` set to `false`.'); } else { // eslint-disable-next-line no-console - console.error('Synthesis has been called mulitple times and the construct tree was modified after the first synthesis. Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); + console.error(errorMessage + 'Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); } } From b68519a74e31fcefc325bcd3fdadf318a8d1744e Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 13:47:26 -0400 Subject: [PATCH 21/23] WIP err msgs again x2 Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index 1d890d7340aff..d3c7dedd2ffc7 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -233,10 +233,10 @@ export class Stage extends Construct { if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { const errorMessage = 'Synthesis has been called multiple times and the construct tree was modified after the first synthesis.'; if ((options.errorOnDuplicateSynth ?? true) && !options.force) { - throw new Error(errorMessage + 'This is not allowed: if you intended to do this, then call synth() with `errorOnDuplicateSynth` set to `false`.'); + throw new Error(errorMessage + ' This is not allowed.'); } else { // eslint-disable-next-line no-console - console.error(errorMessage + 'Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); + console.error(errorMessage + ' Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); } } From e8b866b23a70db5760eaeea208abb3a19bc14838 Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 13:52:57 -0400 Subject: [PATCH 22/23] WIP err msgs 3x Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index d3c7dedd2ffc7..f5be2b1f6188e 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -233,7 +233,7 @@ export class Stage extends Construct { if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { const errorMessage = 'Synthesis has been called multiple times and the construct tree was modified after the first synthesis.'; if ((options.errorOnDuplicateSynth ?? true) && !options.force) { - throw new Error(errorMessage + ' This is not allowed.'); + throw new Error(errorMessage + ' This is not allowed. Remove multple synth() calls and do not modify the construct tree after the first synth().'); } else { // eslint-disable-next-line no-console console.error(errorMessage + ' Only the results of the first synth() call are used, and modifications done after it are ignored. Avoid construct tree mutations after synth() has been called unless this is intentional.'); From 6f098befab3fbe8d6519a2157f21d1f66988afd9 Mon Sep 17 00:00:00 2001 From: Sumu Date: Tue, 29 Oct 2024 17:57:06 -0400 Subject: [PATCH 23/23] remove force option from error throwing in synth Signed-off-by: Sumu --- packages/aws-cdk-lib/core/lib/stage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk-lib/core/lib/stage.ts b/packages/aws-cdk-lib/core/lib/stage.ts index f5be2b1f6188e..c3f9acf4a0563 100644 --- a/packages/aws-cdk-lib/core/lib/stage.ts +++ b/packages/aws-cdk-lib/core/lib/stage.ts @@ -232,7 +232,7 @@ export class Stage extends Construct { // If the construct paths set has changed if (!this.constructPathSetsAreEqual(this.constructPathsCache, newConstructPaths)) { const errorMessage = 'Synthesis has been called multiple times and the construct tree was modified after the first synthesis.'; - if ((options.errorOnDuplicateSynth ?? true) && !options.force) { + if (options.errorOnDuplicateSynth ?? true) { throw new Error(errorMessage + ' This is not allowed. Remove multple synth() calls and do not modify the construct tree after the first synth().'); } else { // eslint-disable-next-line no-console