Skip to content

Commit c7e9dfb

Browse files
RomainMullerRomainMuller
authoredAug 1, 2019
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.
1 parent 07825b6 commit c7e9dfb

File tree

4 files changed

+133
-70
lines changed

4 files changed

+133
-70
lines changed
 

Diff for: ‎packages/@aws-cdk/core/lib/physical-name.ts

+3-6
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { Lazy } from "./lazy";
1+
import { GeneratedWhenNeededMarker } from './private/physical-name-generator';
2+
import { Token } from './token';
23

34
/**
45
* Includes special markers for automatic generation of physical names.
@@ -14,11 +15,7 @@ export class PhysicalName {
1415
* mostly designed for reusable constructs which may or may not be referenced
1516
* acrossed environments.
1617
*/
17-
public static readonly GENERATE_IF_NEEDED = Lazy.stringValue({
18-
produce: () => {
19-
throw new Error(`Invalid physical name passed to CloudFormation. Use "this.physicalName" instead`);
20-
}
21-
});
18+
public static readonly GENERATE_IF_NEEDED = Token.asString(new GeneratedWhenNeededMarker());
2219

2320
private constructor() { }
2421
}

Diff for: ‎packages/@aws-cdk/core/lib/private/physical-name-generator.ts

+36
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import crypto = require('crypto');
2+
import { IResolvable, IResolveContext } from '../resolvable';
23
import { IResource } from '../resource';
34
import { Stack } from '../stack';
45
import { Token } from '../token';
6+
import { TokenMap } from './token-map';
57

68
export function generatePhysicalName(resource: IResource): string {
79
const stack = Stack.of(resource);
@@ -65,3 +67,37 @@ class SuffixNamePart extends NamePart {
6567
return this.bareStr.slice(startIndex, strLen);
6668
}
6769
}
70+
71+
const GENERATE_IF_NEEDED_SYMBOL = Symbol.for('@aws-cdk/core.<private>.GenerateIfNeeded');
72+
73+
/**
74+
* This marker token is used by PhysicalName.GENERATE_IF_NEEDED. When that token is passed to the
75+
* physicalName property of a Resource, it triggers different behavior in the Resource constructor
76+
* that will allow emission of a generated physical name (when the resource is used across
77+
* environments) or undefined (when the resource is not shared).
78+
*
79+
* This token throws an Error when it is resolved, as a way to prevent inadvertent mis-uses of it.
80+
*/
81+
export class GeneratedWhenNeededMarker implements IResolvable {
82+
public readonly creationStack: string[] = [];
83+
84+
constructor() {
85+
Object.defineProperty(this, GENERATE_IF_NEEDED_SYMBOL, { value: true });
86+
}
87+
88+
public resolve(_ctx: IResolveContext): never {
89+
throw new Error(`Invalid physical name passed to CloudFormation. Use "this.physicalName" instead`);
90+
}
91+
92+
public toString(): string {
93+
return 'PhysicalName.GENERATE_IF_NEEDED';
94+
}
95+
}
96+
97+
/**
98+
* Checks whether a stringified token resolves to a `GeneratedWhenNeededMarker`.
99+
*/
100+
export function isGeneratedWhenNeededMarker(val: string): boolean {
101+
const token = TokenMap.instance().lookupString(val);
102+
return !!token && GENERATE_IF_NEEDED_SYMBOL in token;
103+
}

Diff for: ‎packages/@aws-cdk/core/lib/resource.ts

+2-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
import { ArnComponents } from './arn';
22
import { Construct, IConstruct } from './construct';
33
import { Lazy } from './lazy';
4-
import { PhysicalName } from './physical-name';
5-
import { generatePhysicalName } from './private/physical-name-generator';
4+
import { generatePhysicalName, isGeneratedWhenNeededMarker } from './private/physical-name-generator';
65
import { IResolveContext } from './resolvable';
76
import { Stack } from './stack';
87
import { Token } from './token';
@@ -64,7 +63,7 @@ export abstract class Resource extends Construct implements IResource {
6463

6564
let physicalName = props.physicalName;
6665

67-
if (props.physicalName === PhysicalName.GENERATE_IF_NEEDED) {
66+
if (props.physicalName && isGeneratedWhenNeededMarker(props.physicalName)) {
6867
// auto-generate only if cross-env is required
6968
this._physicalName = undefined;
7069
this._allowCrossEnvironment = true;

Diff for: ‎packages/@aws-cdk/core/test/private/test.physical-name-generator.ts

+92-61
Original file line numberDiff line numberDiff line change
@@ -1,91 +1,122 @@
11
import nodeunit = require('nodeunit');
2-
import { App, Aws, Resource, Stack } from '../../lib';
3-
import { generatePhysicalName } from '../../lib/private/physical-name-generator';
2+
import { App, Aws, Lazy, Resource, Stack, Token } from '../../lib';
3+
import { GeneratedWhenNeededMarker, generatePhysicalName, isGeneratedWhenNeededMarker } from '../../lib/private/physical-name-generator';
44

55
export = nodeunit.testCase({
6-
'generates correct physical names'(test: nodeunit.Test) {
7-
const app = new App();
8-
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
6+
generatePhysicalName: {
7+
'generates correct physical names'(test: nodeunit.Test) {
8+
const app = new App();
9+
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
910

10-
const testResourceA = new TestResource(stack, 'A');
11-
const testResourceB = new TestResource(testResourceA, 'B');
11+
const testResourceA = new TestResource(stack, 'A');
12+
const testResourceB = new TestResource(testResourceA, 'B');
1213

13-
test.equal(generatePhysicalName(testResourceA), 'teststackteststackaa164c141d59b37c1b663');
14-
test.equal(generatePhysicalName(testResourceB), 'teststackteststackab27595cd34d8188283a1f');
14+
test.equal(generatePhysicalName(testResourceA), 'teststackteststackaa164c141d59b37c1b663');
15+
test.equal(generatePhysicalName(testResourceB), 'teststackteststackab27595cd34d8188283a1f');
1516

16-
test.done();
17-
},
17+
test.done();
18+
},
1819

19-
'generates different names in different accounts'(test: nodeunit.Test) {
20-
const appA = new App();
21-
const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
22-
const resourceA = new TestResource(stackA, 'Resource');
20+
'generates different names in different accounts'(test: nodeunit.Test) {
21+
const appA = new App();
22+
const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
23+
const resourceA = new TestResource(stackA, 'Resource');
2324

24-
const appB = new App();
25-
const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678913', region: 'bermuda-triangle-1' } });
26-
const resourceB = new TestResource(stackB, 'Resource');
25+
const appB = new App();
26+
const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678913', region: 'bermuda-triangle-1' } });
27+
const resourceB = new TestResource(stackB, 'Resource');
2728

28-
test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
29+
test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
2930

30-
test.done();
31-
},
31+
test.done();
32+
},
3233

33-
'generates different names in different regions'(test: nodeunit.Test) {
34-
const appA = new App();
35-
const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
36-
const resourceA = new TestResource(stackA, 'Resource');
34+
'generates different names in different regions'(test: nodeunit.Test) {
35+
const appA = new App();
36+
const stackA = new Stack(appA, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-1' } });
37+
const resourceA = new TestResource(stackA, 'Resource');
3738

38-
const appB = new App();
39-
const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-2' } });
40-
const resourceB = new TestResource(stackB, 'Resource');
39+
const appB = new App();
40+
const stackB = new Stack(appB, 'TestStack', { env: { account: '012345678912', region: 'bermuda-triangle-2' } });
41+
const resourceB = new TestResource(stackB, 'Resource');
4142

42-
test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
43+
test.notEqual(generatePhysicalName(resourceA), generatePhysicalName(resourceB));
4344

44-
test.done();
45-
},
45+
test.done();
46+
},
4647

47-
'fails when the region is an unresolved token'(test: nodeunit.Test) {
48-
const app = new App();
49-
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: Aws.REGION } });
50-
const testResource = new TestResource(stack, 'A');
48+
'fails when the region is an unresolved token'(test: nodeunit.Test) {
49+
const app = new App();
50+
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912', region: Aws.REGION } });
51+
const testResource = new TestResource(stack, 'A');
5152

52-
test.throws(() => generatePhysicalName(testResource),
53-
/Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
53+
test.throws(() => generatePhysicalName(testResource),
54+
/Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
5455

55-
test.done();
56-
},
56+
test.done();
57+
},
5758

58-
'fails when the region is not provided'(test: nodeunit.Test) {
59-
const app = new App();
60-
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912' } });
61-
const testResource = new TestResource(stack, 'A');
59+
'fails when the region is not provided'(test: nodeunit.Test) {
60+
const app = new App();
61+
const stack = new Stack(app, 'TestStack', { env: { account: '012345678912' } });
62+
const testResource = new TestResource(stack, 'A');
6263

63-
test.throws(() => generatePhysicalName(testResource),
64-
/Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
64+
test.throws(() => generatePhysicalName(testResource),
65+
/Cannot generate a physical name for TestStack\/A, because the region is un-resolved or missing/);
6566

66-
test.done();
67-
},
67+
test.done();
68+
},
69+
70+
'fails when the account is an unresolved token'(test: nodeunit.Test) {
71+
const app = new App();
72+
const stack = new Stack(app, 'TestStack', { env: { account: Aws.ACCOUNT_ID, region: 'bermuda-triangle-1' } });
73+
const testResource = new TestResource(stack, 'A');
74+
75+
test.throws(() => generatePhysicalName(testResource),
76+
/Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
6877

69-
'fails when the account is an unresolved token'(test: nodeunit.Test) {
70-
const app = new App();
71-
const stack = new Stack(app, 'TestStack', { env: { account: Aws.ACCOUNT_ID, region: 'bermuda-triangle-1' } });
72-
const testResource = new TestResource(stack, 'A');
78+
test.done();
79+
},
7380

74-
test.throws(() => generatePhysicalName(testResource),
75-
/Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
81+
'fails when the account is not provided'(test: nodeunit.Test) {
82+
const app = new App();
83+
const stack = new Stack(app, 'TestStack', { env: { region: 'bermuda-triangle-1' } });
84+
const testResource = new TestResource(stack, 'A');
7685

77-
test.done();
86+
test.throws(() => generatePhysicalName(testResource),
87+
/Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
88+
89+
test.done();
90+
},
7891
},
7992

80-
'fails when the account is not provided'(test: nodeunit.Test) {
81-
const app = new App();
82-
const stack = new Stack(app, 'TestStack', { env: { region: 'bermuda-triangle-1' } });
83-
const testResource = new TestResource(stack, 'A');
93+
GeneratedWhenNeededMarker: {
94+
'is correctly recognized'(test: nodeunit.Test) {
95+
const marker = new GeneratedWhenNeededMarker();
96+
const asString = Token.asString(marker);
97+
98+
test.ok(isGeneratedWhenNeededMarker(asString));
99+
100+
test.done();
101+
},
102+
103+
'throws when resolved'(test: nodeunit.Test) {
104+
const marker = new GeneratedWhenNeededMarker();
105+
const asString = Token.asString(marker);
106+
107+
test.throws(() => new Stack().resolve(asString), /Use "this.physicalName" instead/);
108+
109+
test.done();
110+
},
111+
},
84112

85-
test.throws(() => generatePhysicalName(testResource),
86-
/Cannot generate a physical name for TestStack\/A, because the account is un-resolved or missing/);
113+
isGeneratedWhenNeededMarker: {
114+
'correctly response for other tokens'(test: nodeunit.Test) {
115+
test.ok(!isGeneratedWhenNeededMarker('this is not even a token!'));
116+
test.ok(!isGeneratedWhenNeededMarker(Lazy.stringValue({ produce: () => 'Bazinga!' })));
87117

88-
test.done();
118+
test.done();
119+
}
89120
},
90121
});
91122

0 commit comments

Comments
 (0)