From 6a5266fbaf291318fc7cdbf583aa8f77ee418183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 28 Jun 2019 11:13:22 +0200 Subject: [PATCH 1/4] fix(code): make CfnResource#_toCloudFormation null-safe The `CfnResource#_toCloudFormation` method creates a `PostResolveToken` with a post-processor that was not ready to handle the absence of `Properties` on the resolved value. It is however possible that `Properties` are missing when an object is created with the default configuration (e.g: by `new sqs.CfnQueue(this, 'Queue');`). This change makes the post-processor function correctly handle `undefined` in this case. Related #3093 --- packages/@aws-cdk/core/lib/cfn-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index edb7d3f87a8ea..c4db697d6dccf 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -230,7 +230,7 @@ export class CfnResource extends CfnRefElement { Metadata: ignoreEmpty(this.cfnOptions.metadata), Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { - props.Properties = this.renderProperties(props.Properties); + props.Properties = props.Properties && this.renderProperties(props.Properties); return deepMerge(props, this.rawOverrides); }) } From ae331d9728378b13d19741c286b0dd4e8e1cb575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 28 Jun 2019 11:50:48 +0200 Subject: [PATCH 2/4] test: what happens --- packages/@aws-cdk/core/lib/cfn-resource.ts | 3 +- .../@aws-cdk/core/test/test.cfn-resource.ts | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 packages/@aws-cdk/core/test/test.cfn-resource.ts diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index c4db697d6dccf..07be95911ddda 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -230,7 +230,8 @@ export class CfnResource extends CfnRefElement { Metadata: ignoreEmpty(this.cfnOptions.metadata), Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { - props.Properties = props.Properties && this.renderProperties(props.Properties); + const renderedProps = this.renderProperties(props.Properties || {}); + props.Properties = renderedProps && Object.keys(renderedProps).length === 0 ? undefined : renderedProps; return deepMerge(props, this.rawOverrides); }) } diff --git a/packages/@aws-cdk/core/test/test.cfn-resource.ts b/packages/@aws-cdk/core/test/test.cfn-resource.ts new file mode 100644 index 0000000000000..db83634ecccd3 --- /dev/null +++ b/packages/@aws-cdk/core/test/test.cfn-resource.ts @@ -0,0 +1,29 @@ +import nodeunit = require('nodeunit'); +import core = require('../lib'); + +export = nodeunit.testCase({ + '._toCloudFormation': { + 'does not call renderProperties with an undefined value'(test: nodeunit.Test) { + const app = new core.App(); + const stack = new core.Stack(app, 'TestStack'); + const resource = new core.CfnResource(stack, 'DefaultResource', { type: 'Test::Resource::Fake' }); + + let called = false; + (resource as any).renderProperties = (val: any) => { + called = true; + test.notEqual(val, null); + }; + + test.deepEqual(app.synth().getStack(stack.stackName).template, { + Resources: { + DefaultResource: { + Type: 'Test::Resource::Fake' + } + } + }); + test.ok(called, `renderProperties must be called called`); + + test.done(); + } + } +}); From 06e68a9a22730f1ecc179914a04e43116572e5e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Fri, 28 Jun 2019 17:40:28 +0200 Subject: [PATCH 3/4] Fix problem --- packages/@aws-cdk/core/lib/cfn-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index 07be95911ddda..545ac9d1d3dd1 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -231,7 +231,7 @@ export class CfnResource extends CfnRefElement { Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { const renderedProps = this.renderProperties(props.Properties || {}); - props.Properties = renderedProps && Object.keys(renderedProps).length === 0 ? undefined : renderedProps; + props.Properties = renderedProps && (Object.keys(renderedProps).length === 0 ? undefined : renderedProps); return deepMerge(props, this.rawOverrides); }) } From 47fd6378dc386e4ba3df7412775bd5e442305b57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 1 Jul 2019 12:20:20 +0200 Subject: [PATCH 4/4] fix: undefined-erasing bug --- packages/@aws-cdk/core/lib/cfn-resource.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index 545ac9d1d3dd1..31dc062f5ce43 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -231,7 +231,7 @@ export class CfnResource extends CfnRefElement { Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { const renderedProps = this.renderProperties(props.Properties || {}); - props.Properties = renderedProps && (Object.keys(renderedProps).length === 0 ? undefined : renderedProps); + props.Properties = renderedProps && (Object.values(renderedProps).find(v => !!v) ? renderedProps : undefined); return deepMerge(props, this.rawOverrides); }) }