Skip to content

Commit

Permalink
fix(assertions): throw error or warn when synth is called multiple …
Browse files Browse the repository at this point in the history
…times on mutated construct tree (#31865)

Closes #24689 

### 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
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
sumupitchayan authored Oct 29, 2024
1 parent 46e51f5 commit a261c9d
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 23 deletions.
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/core/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({ errorOnDuplicateSynth: false }));
}

this._treeMetadata = props.treeMetadata ?? true;
Expand Down
61 changes: 60 additions & 1 deletion packages/aws-cdk-lib/core/lib/stage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ export class Stage extends Construct {
*/
private assembly?: cxapi.CloudAssembly;

/**
* The cached set of construct paths. Empty if assembly was not yet built.
*/
private constructPathsCache: Set<string>;

/**
* Validation plugins to run during synthesis. If any plugin reports any violation,
* synthesis will be interrupted and the report displayed to the user.
Expand All @@ -163,6 +168,7 @@ export class Stage extends Construct {

Object.defineProperty(this, STAGE_SYMBOL, { value: true });

this.constructPathsCache = new Set<string>();
this.parentStage = Stage.of(this);

this.region = props.env?.region ?? this.parentStage?.region;
Expand Down Expand Up @@ -210,16 +216,62 @@ export class Stage extends Construct {
* calls will return the same assembly.
*/
public synth(options: StageSynthesisOptions = { }): cxapi.CloudAssembly {
if (!this.assembly || options.force) {

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) {
this.assembly = synthesize(this, {
skipValidation: options.skipValidation,
validateOnSynthesis: options.validateOnSynthesis,
});
newConstructPaths = this.listAllConstructPaths(this);
this.constructPathsCache = newConstructPaths;
}

// 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) {
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.');
}
}

// 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<string> {
const paths = new Set<string>();
function recurse(root: IConstruct) {
paths.add(root.node.path);
for (const child of root.node.children) {
if (!Stage.isStage(child)) {
recurse(child);
}
}
}
recurse(construct);
return paths;
}

// Checks if sets of construct paths are equal
private constructPathSetsAreEqual(set1: Set<string>, set2: Set<string>): 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) {
Expand Down Expand Up @@ -259,4 +311,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 true
*/
readonly errorOnDuplicateSynth?: boolean;
}
25 changes: 25 additions & 0 deletions packages/aws-cdk-lib/core/test/synthesis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ 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';
Expand Down Expand Up @@ -362,6 +363,30 @@ describe('synthesis', () => {

});

test('calling synth multiple times errors if construct tree is mutated', () => {
const app = new cdk.App();

const stages = [
{
stage: 'PROD',
},
{
stage: 'BETA',
},
];

// 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('Synthesis has been called multiple times and the construct tree was modified after the first synthesis');
});
});

function list(outdir: string) {
Expand Down
42 changes: 21 additions & 21 deletions packages/aws-cdk-lib/pipelines/test/compliance/synths.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit a261c9d

Please sign in to comment.