Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cdk): construct.uniqueId and relax construct id constraints #556

Merged
merged 1 commit into from
Aug 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/@aws-cdk/cdk/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class App extends Root {
throw new Error(`The child ${child.toString()} of Program must be a Stack`);
}

out[child.name] = child as Stack;
out[child.id] = child as Stack;
}
return out;
}
Expand Down Expand Up @@ -113,7 +113,7 @@ export class App extends Root {
}

return {
name: stack.name,
name: stack.id,
environment,
missing: Object.keys(stack.missingContext).length ? stack.missingContext : undefined,
template: stack.toCloudFormation(),
Expand Down
67 changes: 2 additions & 65 deletions packages/@aws-cdk/cdk/lib/cloudformation/logical-id.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import { makeUniqueId } from '../util/uniqueid';
import { StackElement } from "./stack";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to use single quotes.


// tslint:disable-next-line:no-var-requires
const md5 = require('../util/md5');

const PATH_SEP = '/';
const HASH_LEN = 8;
const MAX_HUMAN_LEN = 240; // max ID len is 255

/**
* Interface for classes that implementation logical ID assignment strategies
Expand Down Expand Up @@ -55,42 +51,10 @@ export interface IAddressingScheme {
*/
export class HashedAddressingScheme implements IAddressingScheme {
public allocateAddress(addressComponents: string[]): string {
addressComponents = addressComponents.filter(x => x !== HIDDEN_ID);

if (addressComponents.length === 0) {
throw new Error('Construct has empty Logical ID');
}

// 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.
if (addressComponents.length === 1) {
return addressComponents[0];
}

const hash = pathHash(addressComponents);
const human = removeDupes(addressComponents)
.filter(x => x !== HIDDEN_FROM_HUMAN_ID)
.join('')
.slice(0, MAX_HUMAN_LEN);

return human + hash;
return makeUniqueId(addressComponents);
}
}

/**
* Resources with this ID are hidden from humans
*
* They do not appear in the human-readable part of the logical ID,
* but they are included in the hash calculation.
*/
const HIDDEN_FROM_HUMAN_ID = 'Resource';

/**
* Resources with this ID are complete hidden from the logical ID calculation.
*/
const HIDDEN_ID = 'Default';

/**
* Class that keeps track of the logical IDs that are assigned to resources
*
Expand Down Expand Up @@ -176,15 +140,6 @@ export class LogicalIDs {
}
}

/**
* Take a hash of the given path.
*
* The hash is limited in size.
*/
function pathHash(path: string[]): string {
return md5(path.join(PATH_SEP)).slice(0, HASH_LEN).toUpperCase();
}

const VALID_LOGICALID_REGEX = /^[A-Za-z][A-Za-z0-9]{1,254}$/;

/**
Expand All @@ -195,21 +150,3 @@ function validateLogicalId(logicalId: string) {
throw new Error(`Logical ID must adhere to the regular expression: ${VALID_LOGICALID_REGEX.toString()}, got '${logicalId}'`);
}
}

/**
* Remove duplicate "terms" from the path list
*
* If a component name is completely the same as the suffix of
* the previous component name, we get rid of it.
*/
function removeDupes(path: string[]): string[] {
const ret = new Array<string>();

for (const component of path) {
if (ret.length === 0 || !ret[ret.length - 1].endsWith(component)) {
ret.push(component);
}
}

return ret;
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/cdk/lib/cloudformation/output.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class Output extends StackElement {
this.export = props.export;
} else if (!props.disableExport) {
// prefix export name with stack name since exports are global within account + region.
const stackName = Stack.find(this).name;
const stackName = Stack.find(this).id;
this.export = stackName ? stackName + ':' : '';
this.export += this.logicalId;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/cdk/lib/cloudformation/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ export abstract class StackElement extends Construct implements IDependable {
* Return the path with respect to the stack
*/
public get stackPath(): string {
return this.ancestors(this.stack).map(c => c.name).join(PATH_SEP);
return this.ancestors(this.stack).map(c => c.id).join(PATH_SEP);
}

public get dependencyElements(): IDependable[] {
Expand Down
68 changes: 38 additions & 30 deletions packages/@aws-cdk/cdk/lib/core/construct.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,40 @@
import cxapi = require('@aws-cdk/cx-api');
import { makeUniqueId } from '../util/uniqueid';
export const PATH_SEP = '/';

/**
* Represents the building block of the construct graph.
* When a construct is created, it is always added as a child
*/
export class Construct {
private static readonly VALID_NAME_REGEX = /^[A-Za-z][A-Za-z0-9]*$/;

/**
* Returns the parent of this node or undefined if this is a root node.
*/
public readonly parent?: Construct;

/**
* The name of this construct
* @deprecated use `id`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeahhhhhh that's a poor deprecation message. You want to explain why id is better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a good idea to mention when it was deprecated, so we can assess when it is safe to drop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, why are you so careful in not breaking usage of .name? I feel like this is probably not common place... Would have dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, removed!

*/
public readonly name: string;

/**
* The subtree-local id of the construct.
* This id is unique within the subtree. To obtain a tree-unique id, use `uniqueId`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define subtree... The construct may be accessible from several parts of the tree, but I suppose the subtree in question is the one it's attached to, not the one you're accessing from...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed description to:

    /**
     * The local id of the construct.
     * This id is unique amongst its siblings.
     * To obtain a tree-global unique id for this construct, use `uniqueId`.
     */
    public readonly id: string;

*/
public readonly id: string;

/**
* The full path of this construct in the tree.
* Components are separated by '/'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words:

So we had this jolly good array of strings that represent a path in the tree. We could have exposed it as a ReadonlyArray<string>, but we though - oh the hell, we can .join('/') this so in case you're interested in the array, you'll always have to .split('/') it. There there, we fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know not every language supports ReadonlyArray<T>, but they could code-generate a copy-on-read and be good with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this is a string is because I think that in most cases, when people need the path, they want the string version of it and not the components. Optimize for the common case.

*/
public readonly path: string;

/**
* A tree-unique alpha-numeric identifier for this construct.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm'kay. I feel tree is maybe over-used and maybe too generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tree represents the entire construct tree. Added "global"

* Includes all components of the tree.
*/
public readonly uniqueId: string;

/**
* List of children and their names
*/
Expand All @@ -37,28 +54,33 @@ export class Construct {
* @param parent The parent construct
* @param props Properties for this construct
*/
constructor(parent: Construct, name: string) {
this.name = name;
constructor(parent: Construct, id: string) {
this.id = id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.name = this.id = id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed name altogther

this.name = id; // legacy
this.parent = parent;

// We say that parent is required, but some root constructs bypass the type checks and
// actually pass in 'undefined'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we allow that? 😱

More seriously, can't parent constructs pass a non-null parent in? Like... themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how it's implemented now.

if (parent != null) {
if (name === '') {
if (id === '') {
throw new Error('Only root constructs may have an empty name');
}

// Has side effect so must be very last thing in constructor
parent.addChild(this, this.name);
parent.addChild(this, this.id);
} else {
// This is a root construct.
this.name = name;
this.id = id;
}

// Validate the name we ended up with
if (this.name !== '') {
this._validateName(this.name);
if (this.id !== '') {
this._validateName(this.id);
}

const components = this.rootPath().map(c => c.id);
this.path = components.join(PATH_SEP);
this.uniqueId = components.length > 0 ? makeUniqueId(components) : '';
}

/**
Expand All @@ -77,7 +99,7 @@ export class Construct {
for (let i = 0; i < depth; ++i) {
out += ' ';
}
const name = this.name || '';
const name = this.id || '';
out += `${this.typename}${name.length > 0 ? ' [' + name + ']' : ''}\n`;
for (const child of this.children) {
out += child.toTreeString(depth + 1);
Expand Down Expand Up @@ -137,7 +159,7 @@ export class Construct {
*/
public setContext(key: string, value: any) {
if (this.children.length > 0) {
const names = this.children.map(c => c.name);
const names = this.children.map(c => c.id);
throw new Error('Cannot set context after children have been added: ' + names.join(','));
}
this.context[key] = value;
Expand Down Expand Up @@ -173,15 +195,6 @@ export class Construct {
return value;
}

/**
* Returns the path of all constructs from root to this construct, in string form.
*
* @returns /-separated path of this Construct.
*/
public get path(): string {
return this.rootPath().map(c => c.name).join(PATH_SEP);
}

/**
* An array of metadata objects associated with this construct.
* This can be used, for example, to implement support for deprecation notices, source mapping, etc.
Expand Down Expand Up @@ -281,17 +294,12 @@ export class Construct {
}

/**
* Validate that the name of the construct is a legal identifier
*
* At the moment, we restrict these to valid CloudFormation identifiers.
*
* Protected so it can be overridden by subclasses. Starts with _ to hide the virtual function from JSII,
* because we don't want this validation to involve asynchrony. This restricts it to only
* be overridable in (Type|Java)Script, but that suffices for now.
* Validate that the name of the construct is a legal identifier.
* Construct names can be any characters besides the path separator.
*/
protected _validateName(name: string) {
if (!Construct.VALID_NAME_REGEX.test(name)) {
throw new Error(`Name must adhere to the regular expression: ${Construct.VALID_NAME_REGEX.toString()}, got '${name}'`);
if (name.indexOf(PATH_SEP) !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be un-necessary if we didn't perform the outrageous .join('/')...
Also, are we okay with control characters in names?
Also, are we actually validating a name? Is it not id now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to "id".

throw new Error(`Construct names cannot include '${PATH_SEP}': ${name}`);
}
}

Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/cdk/lib/core/jsx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ export namespace jsx {
* @returns A Construct object
*/
export function construct(tree: any, parent?: Construct): Construct {
const name = (tree.props && tree.props.name) || '';
const root = new tree.type(parent, name, tree.props); // create root
const id = (tree.props && tree.props.id) || '';
const root = new tree.type(parent, id, tree.props); // create root
createChildren(root, tree.children);
return root;
}

function createChildren(parent: Construct, children: any[]) {
for (const child of children) {
const name = (child.props && child.props.name) || '';
const childObj = new child.type(parent, name, child.props);
const id = (child.props && child.props.id) || '';
const childObj = new child.type(parent, id, child.props);
createChildren(childObj, child.children);
}
}
Expand Down
85 changes: 85 additions & 0 deletions packages/@aws-cdk/cdk/lib/util/uniqueid.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// tslint:disable-next-line:no-var-requires
const md5 = require('./md5');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please switch to use crypto instead. And although I know that's not a security-sensitive context, it mains me a little every time I see MD5.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


/**
* Resources with this ID are hidden from humans
*
* They do not appear in the human-readable part of the logical ID,
* but they are included in the hash calculation.
*/
const HIDDEN_FROM_HUMAN_ID = 'Resource';

/**
* Resources with this ID are complete hidden from the logical ID calculation.
*/
const HIDDEN_ID = 'Default';

const PATH_SEP = '/';

const HASH_LEN = 8;
const MAX_HUMAN_LEN = 240; // max ID len is 255

/**
* Given a set of named path components, returns a unique alpha-numeric identifier
* with a maximum length of 255. This is done by calculating a hash on the full path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move constraints on output to @return. It's a healthy habit to take when you're going to be touching stuff that will eventually code-generate to Java, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* and using it as a suffix of a length-limited "human" rendition of the path components.
*
* @param components The path components
*/
export function makeUniqueId(components: string[]) {
components = components.filter(x => x !== HIDDEN_ID);

if (components.length === 0) {
throw new Error('Unable to calculate a unique ID for an empty path');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there through a path of ['Resource'] is probably going to be hella confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean ['Default'], right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test

}

// 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.
if (components.length === 1) {
return components[0];
}

const hash = pathHash(components);
const human = removeDupes(components)
.map(removeNonAlpha)
.filter(x => x !== HIDDEN_FROM_HUMAN_ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you not want to do this one before removeNonAlpha? I feel that Re-source shouldn't be considered as === 'Resource'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.join('')
.slice(0, MAX_HUMAN_LEN);

return human + hash;
}

/**
* Take a hash of the given path.
*
* The hash is limited in size.
*/
function pathHash(path: string[]): string {
return md5(path.join(PATH_SEP)).slice(0, HASH_LEN).toUpperCase();
}

/**
* Removes all non-alphanumeric characters in a string.
*/
function removeNonAlpha(s: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to removeNonAlphanumeric. The name is, otherwise, a lie.

return s.replace(/[^A-Za-z0-9]/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can make this return s.replace(/[^a-z0-9]/ig, '');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this

}

/**
* Remove duplicate "terms" from the path list
*
* If a component name is completely the same as the suffix of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very convoluted phrasing:

completely the same as the suffix of the previous comment name

How about

If the previous path component name ends with this component name, skip the current component.

* the previous component name, we get rid of it.
*/
function removeDupes(path: string[]): string[] {
const ret = new Array<string>();

for (const component of path) {
if (ret.length === 0 || !ret[ret.length - 1].endsWith(component)) {
ret.push(component);
}
}

return ret;
}
11 changes: 11 additions & 0 deletions packages/@aws-cdk/cdk/test/cloudformation/test.logical-id.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,17 @@ const uniqueTests = {
// THEN: same ID, same object
test.equal(theId1, theId2);

test.done();
},

'non-alphanumeric characters are removed from the human part of the logical ID'(test: Test) {
const scheme = new HashedAddressingScheme();
const val1 = scheme.allocateAddress([ 'Foo-bar', 'B00m', 'Hello_World', '&&Horray Horray.' ]);
const val2 = scheme.allocateAddress([ 'Foobar', 'B00m', 'HelloWorld', 'HorrayHorray' ]);

// same human part, different hash
test.deepEqual(val1, 'FoobarB00mHelloWorldHorrayHorray640E99FB');
test.deepEqual(val2, 'FoobarB00mHelloWorldHorrayHorray744334FD');
test.done();
}
};
Expand Down
Loading