feat(core): convert "/" in construct id to "--" and disallow tokens (#…

…1375)

Relax construct ID constraints to allow "/" (path seprator)
to be used in construct IDs, but convert it to "--" so it won't
collide with path strings.

It's quite rare for people to actually try to find a construct
by their ID (it's mostly "write-only") and the logical ID is eventually
mangled anyway when synthesized to CFN.

Fails if the construct ID contains a token. This won't work because
we mangle the IDs as strings when we generate the logical ID
and the construct's unique ID, and stringified tokens won't be
resolved.

Fixes #1351
Fixes #1374

packages/@aws-cdk/aws-logs/test/test.loggroup.ts

@@ -121,6 +121,31 @@ export = {
     test.done();
   },
 
+  'extractMetric allows passing in namespaces with "/"'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const lg = new LogGroup(stack, 'LogGroup');
+
+    // WHEN
+    const metric = lg.extractMetric('$.myField', 'MyNamespace/MyService', 'Field');
+
+    // THEN
+    expect(stack).to(haveResource('AWS::Logs::MetricFilter', {
+      FilterPattern: "{ $.myField = \"*\" }",
+      MetricTransformations: [
+        {
+          MetricName: "Field",
+          MetricNamespace: "MyNamespace/MyService",
+          MetricValue: "$.myField"
+        }
+      ]
+    }));
+    test.equal(metric.namespace, 'MyNamespace/MyService');
+    test.equal(metric.metricName, 'Field');
+
+    test.done();
+  },
+
   'grant'(test: Test) {
     // GIVEN
     const stack = new Stack();

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

@@ -114,7 +114,7 @@ export class Stack extends Construct {
     this.env = this.parseEnvironment(props);
 
     this.logicalIds = new LogicalIDs(props && props.namingScheme ? props.namingScheme : new HashedAddressingScheme());
-    this.name = name || 'Stack';
+    this.name = this.id;
   }
 
   /**
@@ -236,7 +236,7 @@ export class Stack extends Construct {
    * character classes, and we don't allow one of the magic markers.
    */
   protected _validateId(name: string) {
-    if (!Stack.VALID_STACK_NAME_REGEX.test(name)) {
+    if (name && !Stack.VALID_STACK_NAME_REGEX.test(name)) {
       throw new Error(`Stack name must match the regular expression: ${Stack.VALID_STACK_NAME_REGEX.toString()}, got '${name}'`);
     }
   }

packages/@aws-cdk/cdk/lib/core/construct.ts

@@ -48,9 +48,13 @@ export class Construct {
    * Creates a new construct node.
    *
    * @param parent The parent construct
-   * @param props  Properties for this construct
+   * @param id The local logical ID of the construct. Must be unique amongst
+   * siblings. If the ID includes a path separator (`/`), then it will be
+   * replaced by double dash `--`.
    */
   constructor(parent: Construct, id: string) {
+    id = id || ''; // if undefined, convert to empty string
+
     this.id = id;
     this.parent = parent;
 
@@ -68,10 +72,11 @@ export class Construct {
       this.id = id;
     }
 
-    // Validate the name we ended up with
-    if (this.id !== '') {
-      this._validateId(this.id);
-    }
+    // escape any path separators so they don't wreck havoc
+    this.id = this._escapePathSeparator(this.id);
+
+    // allow derived classes to validate the construct id
+    this._validateId(this.id);
 
     const components = this.rootPath().map(c => c.id);
     this.path = components.join(PATH_SEP);
@@ -105,6 +110,9 @@ export class Construct {
   /**
    * Return a descendant by path, or undefined
    *
+   * Note that if the original ID of the construct you are looking for contained
+   * a '/', then it would have been replaced by '--'.
+   *
    * @param name Relative name of a direct or indirect child
    * @returns a child by path or undefined if not found.
    */
@@ -127,6 +135,9 @@ export class Construct {
    *
    * Throws an exception if the descendant is not found.
    *
+   * Note that if the original ID of the construct you are looking for contained
+   * a '/', then it would have been replaced by '--'.
+   *
    * @param name Relative name of a direct or indirect child
    * @returns Child with the given path.
    */
@@ -292,10 +303,8 @@ export class Construct {
    * Validate that the id of the construct legal.
    * Construct IDs can be any characters besides the path separator.
    */
-  protected _validateId(id: string) {
-    if (id.indexOf(PATH_SEP) !== -1) {
-      throw new Error(`Construct names cannot include '${PATH_SEP}': ${id}`);
-    }
+  protected _validateId(_id: string) {
+    // can be used by derived classes to customize ID validation.
   }
 
   /**
@@ -387,6 +396,14 @@ export class Construct {
 
     return false;
   }
+
+  /**
+   * If the construct ID contains a path separator, it is replaced by double dash (`--`).
+   */
+  private _escapePathSeparator(id: string) {
+    if (!id) { return id; }
+    return id.split(PATH_SEP).join('--');
+  }
 }
 
 /**

packages/@aws-cdk/cdk/lib/util/uniqueid.ts

@@ -1,5 +1,6 @@
 // tslint:disable-next-line:no-var-requires
 import crypto = require('crypto');
+import { unresolved } from '../core/tokens';
 
 /**
  * Resources with this ID are hidden from humans
@@ -35,6 +36,11 @@ export function makeUniqueId(components: string[]) {
     throw new Error('Unable to calculate a unique id for an empty set of components');
   }
 
+  const unresolvedTokens = components.filter(c => unresolved(c));
+  if (unresolvedTokens.length > 0) {
+    throw new Error(`ID components may not include unresolved tokens: ${unresolvedTokens.join(',')}`);
+  }
+
   // top-level resources will simply use the `name` as-is in order to support
   // transparent migration of cloudformation templates to the CDK without the
   // need to rename all resources.

packages/@aws-cdk/cdk/test/core/test.construct.ts

@@ -1,6 +1,6 @@
 import cxapi = require('@aws-cdk/cx-api');
 import { Test } from 'nodeunit';
-import { Construct, Root } from '../../lib';
+import { Construct, Root, Token } from '../../lib';
 
 // tslint:disable:variable-name
 // tslint:disable:max-line-length
@@ -47,8 +47,29 @@ export = {
     new Construct(root, 'in-Valid' );
     new Construct(root, 'in\\Valid' );
     new Construct(root, 'in.Valid' );
+    test.done();
+  },
+
+  'if construct id contains path seperators, they will be replaced by double-dash'(test: Test) {
+    const root = new Root();
+    const c = new Construct(root, 'Boom/Boom/Bam');
+    test.deepEqual(c.id, 'Boom--Boom--Bam');
+    test.done();
+  },
+
+  'if "undefined" is forcefully used as an "id", it will be treated as an empty string'(test: Test) {
+    const c = new Construct(undefined as any, undefined as any);
+    test.deepEqual(c.id, '');
+    test.done();
+  },
+
+  "dont allow unresolved tokens to be used in construct IDs"(test: Test) {
+    // GIVEN
+    const root = new Root();
+    const token = new Token(() => 'lazy');
 
-    test.throws(() => new Construct(root, 'in/Valid' ), Error, 'backslashes are not allowed');
+    // WHEN + THEN
+    test.throws(() => new Construct(root, `MyID: ${token}`), /ID components may not include unresolved tokens: MyID: \${Token/);
     test.done();
   },
 

packages/@aws-cdk/cdk/test/test.environment.ts

@@ -17,7 +17,7 @@ export = {
     app.setContext(DEFAULT_ACCOUNT_CONTEXT_KEY, 'my-default-account');
     app.setContext(DEFAULT_REGION_CONTEXT_KEY, 'my-default-region');
 
-    const stack = new Stack(app);
+    const stack = new Stack(app, 'my-stack');
 
     test.equal(stack.env.account, 'my-default-account');
     test.equal(stack.env.region, 'my-default-region');