chore: prepare

Remove usage of the `prepare()` hook from across the library.

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

@@ -78,6 +78,15 @@ export class Deployment extends Resource {
 
     this.api = props.api;
     this.deploymentId = Lazy.stringValue({ produce: () => this.resource.ref });
+
+    // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html
+    // Quoting from CloudFormation's docs: "If you create an
+    // AWS::ApiGateway::RestApi resource and its methods (using
+    // AWS::ApiGateway::Method) in the same template as your deployment, the
+    // deployment must depend on the RestApi's methods.
+    if (this.api instanceof RestApi) {
+      this.node.addDependency(this.api._methodsDependencyGroup);
+    }
   }
 
   /**
@@ -91,30 +100,6 @@ export class Deployment extends Resource {
   public addToLogicalId(data: any) {
     this.resource.addToLogicalId(data);
   }
-
-  /**
-   * Hook into synthesis before it occurs and make any final adjustments.
-   */
-  protected prepare() {
-    if (this.api instanceof RestApi) {
-      // Ignore IRestApi that are imported
-
-      /*
-       * https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html
-       * Quoting from CloudFormation's docs - "If you create an AWS::ApiGateway::RestApi resource and its methods (using AWS::ApiGateway::Method) in
-       * the same template as your deployment, the deployment must depend on the RestApi's methods. To create a dependency, add a DependsOn attribute
-       * to the deployment. If you don't, AWS CloudFormation creates the deployment right after it creates the RestApi resource that doesn't contain
-       * any methods, and AWS CloudFormation encounters the following error: The REST API doesn't contain any methods."
-       */
-
-      /*
-       * Adding a dependency between LatestDeployment and Method construct, using ConstructNode.addDependencies(), creates additional dependencies
-       * between AWS::ApiGateway::Deployment and the AWS::Lambda::Permission nodes (children under Method), causing cyclic dependency errors. Hence,
-       * falling back to declaring dependencies between the underlying CfnResources.
-       */
-      this.api.methods.map(m => m.node.defaultChild as CfnResource).forEach(m => this.resource.addDependsOn(m));
-    }
-  }
 }
 
 interface LatestDeploymentResourceProps {
@@ -124,7 +109,7 @@ interface LatestDeploymentResourceProps {
 
 class LatestDeploymentResource extends CfnDeployment {
   private hashComponents = new Array<any>();
-  private originalLogicalId: string;
+
   private api: IRestApi;
 
   constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) {
@@ -134,7 +119,31 @@ class LatestDeploymentResource extends CfnDeployment {
     });
 
     this.api = props.restApi;
-    this.originalLogicalId = Stack.of(this).getLogicalId(this);
+
+    const originalLogicalId = Stack.of(this).getLogicalId(this);
+
+    this.overrideLogicalId(Lazy.stringValue({ produce: ctx => {
+      const hash = [ ...this.hashComponents ];
+
+      if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported
+
+        // Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
+        const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
+        hash.push(ctx.resolve(cfnRestApiCF));
+      }
+
+      let lid = originalLogicalId;
+
+      // if hash components were added to the deployment, we use them to calculate
+      // a logical ID for the deployment resource.
+      if (hash.length > 0) {
+        const md5 = crypto.createHash('md5');
+        hash.map(x => ctx.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
+        lid += md5.digest('hex');
+      }
+
+      return lid;
+    }}));
   }
 
   /**
@@ -150,28 +159,4 @@ class LatestDeploymentResource extends CfnDeployment {
 
     this.hashComponents.push(data);
   }
-
-  /**
-   * Hooks into synthesis to calculate a logical ID that hashes all the components
-   * add via `addToLogicalId`.
-   */
-  protected prepare() {
-    if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported
-
-      // Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
-      const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
-      this.addToLogicalId(Stack.of(this).resolve(cfnRestApiCF));
-    }
-
-    const stack = Stack.of(this);
-
-    // if hash components were added to the deployment, we use them to calculate
-    // a logical ID for the deployment resource.
-    if (this.hashComponents.length > 0) {
-      const md5 = crypto.createHash('md5');
-      this.hashComponents.map(x => stack.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
-      this.overrideLogicalId(this.originalLogicalId + md5.digest('hex'));
-    }
-    super.prepare();
-  }
 }

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -1,6 +1,7 @@
 import { IVpcEndpoint } from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import { CfnOutput, IResource as IResourceBase, Resource, Stack } from '@aws-cdk/core';
+import { Construct, DependencyGroup } from 'constructs';
 import { ApiDefinition } from './api-definition';
 import { ApiKey, ApiKeyOptions, IApiKey } from './api-key';
 import { CfnAccount, CfnRestApi } from './apigateway.generated';
@@ -543,9 +544,17 @@ export class RestApi extends RestApiBase {
    */
   public readonly methods = new Array<Method>();
 
+  /**
+   * A dependency group that represents all methods attached to this API GW (lazy).
+   * @internal
+   */
+  public readonly _methodsDependencyGroup: DependencyGroup;
+
   constructor(scope: Construct, id: string, props: RestApiProps = { }) {
     super(scope, id, props);
 
+    this._methodsDependencyGroup = new DependencyGroup();
+
     const resource = new CfnRestApi(this, 'Resource', {
       name: this.physicalName,
       description: props.description,
@@ -622,6 +631,10 @@ export class RestApi extends RestApiBase {
    */
   public _attachMethod(method: Method) {
     this.methods.push(method);
+
+    // we want to take a direct dependency on the CfnMethod resource because
+    // Method itself has other children and depending on them will cause cyclic deps.
+    this._methodsDependencyGroup.add(method.node.findChild('Resource'));
   }
 
   /**

packages/@aws-cdk/aws-apigateway/test/integ.restapi.multistack.expected.json

@@ -120,7 +120,7 @@
           "BooksApi60AC975F"
         ]
       },
-      "BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0": {
+      "BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be": {
         "Type": "AWS::ApiGateway::Deployment",
         "Properties": {
           "RestApiId": {
@@ -141,7 +141,7 @@
             "Ref": "BooksApi60AC975F"
           },
           "DeploymentId": {
-            "Ref": "BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0"
+            "Ref": "BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be"
           },
           "StageName": "prod"
         }

packages/@aws-cdk/aws-apigateway/test/test.deployment.ts

@@ -150,16 +150,16 @@ export = {
     // the logical ID changed
     const template = synthesize();
     test.ok(!template.Resources.deployment33381975bba46c5132329b81e7befcbbba5a0e75, 'old resource id is not deleted');
-    test.ok(template.Resources.deployment33381975075f46a4503208d69fcffed2f263c48c,
-      `new resource deployment33381975075f46a4503208d69fcffed2f263c48c is not created, instead found ${Object.keys(template.Resources)}`);
+    test.ok(template.Resources.deployment333819758aa4cdb9d204502b959c4903f4d5d29f,
+      `new resource deployment333819758aa4cdb9d204502b959c4903f4d5d29f is not created, instead found ${Object.keys(template.Resources)}`);
 
     // tokens supported, and are resolved upon synthesis
     const value = 'hello hello';
     deployment.addToLogicalId({ foo: Lazy.stringValue({ produce: () => value }) });
 
     const template2 = synthesize();
-    test.ok(template2.Resources.deployment33381975b6d7672e4c9afd0b741e41d07739786b,
-      `resource deployment33381975b6d7672e4c9afd0b741e41d07739786b not found, instead found ${Object.keys(template2.Resources)}`);
+    test.ok(template2.Resources.deployment333819758d91bed959c6bd6268ba84f6d33e888e,
+      `resource deployment333819758d91bed959c6bd6268ba84f6d33e888e not found, instead found ${Object.keys(template2.Resources)}`);
 
     test.done();
 

packages/@aws-cdk/aws-iam/lib/policy.ts

@@ -1,4 +1,5 @@
-import { Construct, IResource, Lazy, Resource } from '@aws-cdk/core';
+import { IResource, Lazy, Resource } from '@aws-cdk/core';
+import { Construct, Dependable, IDependable } from 'constructs';
 import { IGroup } from './group';
 import { CfnPolicy } from './iam.generated';
 import { PolicyDocument } from './policy-document';
@@ -124,14 +125,33 @@ export class Policy extends Resource implements IPolicy {
         Lazy.stringValue({ produce: () => generatePolicyName(scope, resource.logicalId) }),
     });
 
-    const resource = new CfnPolicy(this, 'Resource', {
+    // this function returns `true` if the CFN resource should be included in
+    // the cloudformation template unless `force` is `true`, if the policy
+    // document is empty, the resource will not be included.
+    const shouldExist = () => this.force || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
+
+    class CfnPolicyConditional extends CfnPolicy implements IDependable {
+      protected toCloudFormation(): object {
+        return shouldExist() ? super.toCloudFormation() : {};
+      }
+    }
+
+    const resource = new CfnPolicyConditional(this, 'Resource', {
       policyDocument: this.document,
       policyName: this.physicalName,
       roles: undefinedIfEmpty(() => this.roles.map(r => r.roleName)),
       users: undefinedIfEmpty(() => this.users.map(u => u.userName)),
       groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)),
     });
 
+    // implement custom Dependable logic which returns an empty array if the
+    // policy document is empty (and force is `false`).
+    Dependable.implement(this, {
+      get dependencyRoots() {
+        return shouldExist() ? [resource] : [];
+      },
+    });
+
     this._policyName = this.physicalName!;
     this.force = props.force !== undefined ? props.force : false;
 
@@ -224,14 +244,6 @@ export class Policy extends Resource implements IPolicy {
     return result;
   }
 
-  protected prepare() {
-    // Remove the resource if it shouldn't exist. This will prevent it from being rendered to the template.
-    const shouldExist = this.force || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
-    if (!shouldExist) {
-      this.node.tryRemoveChild('Resource');
-    }
-  }
-
   /**
    * Whether the policy resource has been attached to any identity
    */

packages/@aws-cdk/aws-lambda/lib/function.ts

@@ -3,7 +3,8 @@ import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as logs from '@aws-cdk/aws-logs';
 import * as sqs from '@aws-cdk/aws-sqs';
-import { CfnResource, Construct, Duration, Fn, Lazy, Stack } from '@aws-cdk/core';
+import { CfnResource, Duration, Fn, Lazy, Stack } from '@aws-cdk/core';
+import { Construct } from 'constructs';
 import { Code, CodeConfig } from './code';
 import { EventInvokeConfigOptions } from './event-invoke-config';
 import { IEventSource } from './event-source';
@@ -296,7 +297,6 @@ export interface FunctionProps extends FunctionOptions {
  * library.
  */
 export class Function extends FunctionBase {
-
   /**
    * Returns a `lambda.Version` which represents the current version of this
    * Lambda function. A new version will be created every time the function's
@@ -315,6 +315,18 @@ export class Function extends FunctionBase {
       ...this.currentVersionOptions,
     });
 
+    // override the version's logical ID with a lazy string which includes the
+    // hash of the function itself, so a new version resource is created when
+    // the function configuration changes.
+    const cfn = this._currentVersion.node.defaultChild as CfnResource;
+    const originalLogicalId: string = Stack.of(this).resolve(cfn.logicalId);
+
+    cfn.overrideLogicalId(Lazy.stringValue({ produce: _ => {
+      const hash = calculateFunctionHash(this);
+      const logicalId = trimFromStart(originalLogicalId, 255 - 32);
+      return `${logicalId}${hash}`;
+    }}));
+
     return this._currentVersion;
   }
 
@@ -700,23 +712,6 @@ export class Function extends FunctionBase {
     return this._logGroup;
   }
 
-  protected prepare() {
-    super.prepare();
-
-    // if we have a current version resource, override it's logical id
-    // so that it includes the hash of the function code and it's configuration.
-    if (this._currentVersion) {
-      const stack = Stack.of(this);
-      const cfn = this._currentVersion.node.defaultChild as CfnResource;
-      const originalLogicalId: string = stack.resolve(cfn.logicalId);
-
-      const hash = calculateFunctionHash(this);
-
-      const logicalId = trimFromStart(originalLogicalId, 255 - 32);
-      cfn.overrideLogicalId(`${logicalId}${hash}`);
-    }
-  }
-
   private renderEnvironment() {
     if (!this.environment || Object.keys(this.environment).length === 0) {
       return undefined;

packages/@aws-cdk/pipelines/lib/pipeline.ts

@@ -104,6 +104,7 @@ export class CdkPipeline extends Construct {
     });
 
     this.node.addValidation({ validate: () => this.validatePipeline() });
+    Aspects.of(this).apply({ visit: () => this._assets.removeAssetsStageIfEmpty() });
   }
 
   /**
@@ -180,14 +181,6 @@ export class CdkPipeline extends Construct {
     return ret;
   }
 
-  protected onPrepare() {
-    super.onPrepare();
-
-    // TODO: Support this in a proper way in the upstream library. For now, we
-    // "un-add" the Assets stage if it turns out to be empty.
-    this._assets.removeAssetsStageIfEmpty();
-  }
-
   /**
    * Return all StackDeployActions in an ordered list
    */

packages/@aws-cdk/pipelines/lib/stage.ts

@@ -1,6 +1,6 @@
 import * as codepipeline from '@aws-cdk/aws-codepipeline';
 import * as cpactions from '@aws-cdk/aws-codepipeline-actions';
-import { Construct, Stage } from '@aws-cdk/core';
+import { Stage, Aspects } from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
 import { Construct } from 'constructs';
 import { AssetType, DeployCdkStackAction } from './actions';
@@ -55,6 +55,8 @@ export class CdkStage extends Construct {
     this.pipelineStage = props.pipelineStage;
     this.cloudAssemblyArtifact = props.cloudAssemblyArtifact;
     this.host = props.host;
+
+    Aspects.of(this).apply({ visit: () => this.prepare() });
   }
 
   /**
@@ -176,7 +178,7 @@ export class CdkStage extends Construct {
    * after creation, nor is there a way to specify relative priorities, which
    * is a limitation that we should take away in the base library.
    */
-  protected prepare() {
+  private prepare() {
     // FIXME: Make sure this only gets run once. There seems to be an issue in the reconciliation
     // loop that may trigger this more than once if it throws an error somewhere, and the exception
     // that gets thrown here will then override the actual failure.