fix(core): stop relying on === to find PhysicalName.GENERATE_IF_NEEDED (

#3506) When the `@aws-cdk/core` library is installed in multiple locations by `npm` (de-duplication does occasionally not operate on some parts of the tree for a multitude of obscure reasons), then each instance of the library gets it's own `string` instance. As these are unresolved tokens, `string` equality does not yield expected results. This introduces a new marker `IResolvable` to be used so we can effectively perform a runtime-check based on a distinctive `Symbol`, which would be identical for all the instances of the library.
aws · Aug 1, 2019 · c7e9dfb · c7e9dfb
1 parent 07825b6
commit c7e9dfb
Show file tree

Hide file tree

Showing 4 changed files with 133 additions and 70 deletions.
diff --git a/packages/@aws-cdk/core/lib/physical-name.ts b/packages/@aws-cdk/core/lib/physical-name.ts
@@ -1,4 +1,5 @@
-import { Lazy } from "./lazy";
+import { GeneratedWhenNeededMarker } from './private/physical-name-generator';
+import { Token } from './token';
 
 /**
  * Includes special markers for automatic generation of physical names.
@@ -14,11 +15,7 @@ export class PhysicalName {
    * mostly designed for reusable constructs which may or may not be referenced
    * acrossed environments.
    */
-  public static readonly GENERATE_IF_NEEDED = Lazy.stringValue({
-    produce: () => {
-      throw new Error(`Invalid physical name passed to CloudFormation. Use "this.physicalName" instead`);
-    }
-  });
+  public static readonly GENERATE_IF_NEEDED = Token.asString(new GeneratedWhenNeededMarker());
 
   private constructor() { }
 }
diff --git a/packages/@aws-cdk/core/lib/private/physical-name-generator.ts b/packages/@aws-cdk/core/lib/private/physical-name-generator.ts
@@ -1,7 +1,9 @@
 import crypto = require('crypto');
+import { IResolvable, IResolveContext } from '../resolvable';
 import { IResource } from '../resource';
 import { Stack } from '../stack';
 import { Token } from '../token';
+import { TokenMap } from './token-map';
 
 export function generatePhysicalName(resource: IResource): string {
   const stack = Stack.of(resource);
@@ -65,3 +67,37 @@ class SuffixNamePart extends NamePart {
     return this.bareStr.slice(startIndex, strLen);
   }
 }
+
+const GENERATE_IF_NEEDED_SYMBOL = Symbol.for('@aws-cdk/core.<private>.GenerateIfNeeded');
+
+/**
+ * This marker token is used by PhysicalName.GENERATE_IF_NEEDED. When that token is passed to the
+ * physicalName property of a Resource, it triggers different behavior in the Resource constructor
+ * that will allow emission of a generated physical name (when the resource is used across
+ * environments) or undefined (when the resource is not shared).
+ *
+ * This token throws an Error when it is resolved, as a way to prevent inadvertent mis-uses of it.
+ */
+export class GeneratedWhenNeededMarker implements IResolvable {
+  public readonly creationStack: string[] = [];
+
+  constructor() {
+    Object.defineProperty(this, GENERATE_IF_NEEDED_SYMBOL, { value: true });
+  }
+
+  public resolve(_ctx: IResolveContext): never {
+    throw new Error(`Invalid physical name passed to CloudFormation. Use "this.physicalName" instead`);
+  }
+
+  public toString(): string {
+    return 'PhysicalName.GENERATE_IF_NEEDED';
+  }
+}
+
+/**
+ * Checks whether a stringified token resolves to a `GeneratedWhenNeededMarker`.
+ */
+export function isGeneratedWhenNeededMarker(val: string): boolean {
+  const token = TokenMap.instance().lookupString(val);
+  return !!token && GENERATE_IF_NEEDED_SYMBOL in token;
+}
diff --git a/packages/@aws-cdk/core/lib/resource.ts b/packages/@aws-cdk/core/lib/resource.ts
@@ -1,8 +1,7 @@
 import { ArnComponents } from './arn';
 import { Construct, IConstruct } from './construct';
 import { Lazy } from './lazy';
-import { PhysicalName } from './physical-name';
-import { generatePhysicalName } from './private/physical-name-generator';
+import { generatePhysicalName, isGeneratedWhenNeededMarker } from './private/physical-name-generator';
 import { IResolveContext } from './resolvable';
 import { Stack } from './stack';
 import { Token } from './token';
@@ -64,7 +63,7 @@ export abstract class Resource extends Construct implements IResource {
 
     let physicalName = props.physicalName;
 
-    if (props.physicalName === PhysicalName.GENERATE_IF_NEEDED) {
+    if (props.physicalName && isGeneratedWhenNeededMarker(props.physicalName)) {
       // auto-generate only if cross-env is required
       this._physicalName = undefined;
       this._allowCrossEnvironment = true;

diff --git a/packages/@aws-cdk/core/test/private/test.physical-name-generator.ts b/packages/@aws-cdk/core/test/private/test.physical-name-generator.ts
@@ -1,91 +1,122 @@
 import nodeunit = require('nodeunit');
-import { App, Aws, Resource, Stack } from '../../lib';
-import { generatePhysicalName } from '../../lib/private/physical-name-generator';
+import { App, Aws, Lazy, Resource, Stack, Token } from '../../lib';
+import { GeneratedWhenNeededMarker, generatePhysicalName, isGeneratedWhenNeededMarker } from '../../lib/private/physical-name-generator';
 
 export = nodeunit.testCase({
-  'generates correct physical names'(test: nodeunit.Test) {
-    const app = new App();
-    const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
+  generatePhysicalName: {
+    'generates correct physical names'(test: nodeunit.Test) {
+      const app = new App();
+      const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
 
-    const testResourceA = new TestResource(stack, 'A');
-    const testResourceB = new TestResource(testResourceA, 'B');
+      const testResourceA = new TestResource(stack, 'A');
+      const testResourceB = new TestResource(testResourceA, 'B');
 
-    test.equal(generatePhysicalName(testResourceA), 'teststackteststackaa164c141d59b37c1b663');
-    test.equal(generatePhysicalName(testResourceB), 'teststackteststackab27595cd34d8188283a1f');
+      test.equal(generatePhysicalName(testResourceA), 'teststackteststackaa164c141d59b37c1b663');
+      test.equal(generatePhysicalName(testResourceB), 'teststackteststackab27595cd34d8188283a1f');
 
-    test.done();
-  },
+      test.done();
+    },
 
-  'generates different names in different accounts'(test: nodeunit.Test) {
-    const appA = new App();
-    const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
-    const resourceA = new TestResource(stackA, 'Resource');
+    'generates different names in different accounts'(test: nodeunit.Test) {
+      const appA = new App();
+      const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
+      const resourceA = new TestResource(stackA, 'Resource');
 
-    const appB = new App();
-    const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678913', region: 'bermuda-triangle-1' } });
-    const resourceB = new TestResource(stackB, 'Resource');
+      const appB = new App();
+      const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678913', region: 'bermuda-triangle-1' } });
+      const resourceB = new TestResource(stackB, 'Resource');
 
-    test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
+      test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
 
-    test.done();
-  },
+      test.done();
+    },
 
-  'generates different names in different regions'(test: nodeunit.Test) {
-    const appA = new App();
-    const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
-    const resourceA = new TestResource(stackA, 'Resource');
+    'generates different names in different regions'(test: nodeunit.Test) {
+      const appA = new App();
+      const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
+      const resourceA = new TestResource(stackA, 'Resource');
 
-    const appB = new App();
-    const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-2' } });
-    const resourceB = new TestResource(stackB, 'Resource');
+      const appB = new App();
+      const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-2' } });
+      const resourceB = new TestResource(stackB, 'Resource');
 
-    test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
+      test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
 
-    test.done();
-  },
+      test.done();
+    },
 
-  'fails when the region is an unresolved token'(test: nodeunit.Test) {
-    const app = new App();
-    const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: Aws.REGION } });
-    const testResource = new TestResource(stack, 'A');
+    'fails when the region is an unresolved token'(test: nodeunit.Test) {
+      const app = new App();
+      const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: Aws.REGION } });
+      const testResource = new TestResource(stack, 'A');
 
-    test.throws(() => generatePhysicalName(testResource),
-      /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
+      test.throws(() => generatePhysicalName(testResource),
+        /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
 
-    test.done();
-  },
+      test.done();
+    },
 
-  'fails when the region is not provided'(test: nodeunit.Test) {
-    const app = new App();
-    const stack = new Stack(app, 'TestStack', { env: { account: '012345678912' } });
-    const testResource = new TestResource(stack, 'A');
+    'fails when the region is not provided'(test: nodeunit.Test) {
+      const app = new App();
+      const stack = new Stack(app, 'TestStack', { env: { account: '012345678912' } });
+      const testResource = new TestResource(stack, 'A');
 
-    test.throws(() => generatePhysicalName(testResource),
-      /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
+      test.throws(() => generatePhysicalName(testResource),
+        /Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
 
-    test.done();
-  },
+      test.done();
+    },
+
+    'fails when the account is an unresolved token'(test: nodeunit.Test) {
+      const app = new App();
+      const stack = new Stack(app, 'TestStack', { env: { account: Aws.ACCOUNT_ID, region: 'bermuda-triangle-1' } });
+      const testResource = new TestResource(stack, 'A');
+
+      test.throws(() => generatePhysicalName(testResource),
+        /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
 
-  'fails when the account is an unresolved token'(test: nodeunit.Test) {
-    const app = new App();
-    const stack = new Stack(app, 'TestStack', { env: { account: Aws.ACCOUNT_ID, region: 'bermuda-triangle-1' } });
-    const testResource = new TestResource(stack, 'A');
+      test.done();
+    },
 
-    test.throws(() => generatePhysicalName(testResource),
-      /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
+    'fails when the account is not provided'(test: nodeunit.Test) {
+      const app = new App();
+      const stack = new Stack(app, 'TestStack', { env: { region: 'bermuda-triangle-1' } });
+      const testResource = new TestResource(stack, 'A');
 
-    test.done();
+      test.throws(() => generatePhysicalName(testResource),
+        /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
+
+      test.done();
+    },
   },
 
-  'fails when the account is not provided'(test: nodeunit.Test) {
-    const app = new App();
-    const stack = new Stack(app, 'TestStack', { env: { region: 'bermuda-triangle-1' } });
-    const testResource = new TestResource(stack, 'A');
+  GeneratedWhenNeededMarker: {
+    'is correctly recognized'(test: nodeunit.Test) {
+      const marker = new GeneratedWhenNeededMarker();
+      const asString = Token.asString(marker);
+
+      test.ok(isGeneratedWhenNeededMarker(asString));
+
+      test.done();
+    },
+
+    'throws when resolved'(test: nodeunit.Test) {
+      const marker = new GeneratedWhenNeededMarker();
+      const asString = Token.asString(marker);
+
+      test.throws(() => new Stack().resolve(asString), /Use "this.physicalName" instead/);
+
+      test.done();
+    },
+  },
 
-    test.throws(() => generatePhysicalName(testResource),
-      /Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
+  isGeneratedWhenNeededMarker: {
+    'correctly response for other tokens'(test: nodeunit.Test) {
+      test.ok(!isGeneratedWhenNeededMarker('this is not even a token!'));
+      test.ok(!isGeneratedWhenNeededMarker(Lazy.stringValue({ produce: () => 'Bazinga!' })));
 
-    test.done();
+      test.done();
+    }
   },
 });