-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeahhhhhh that's a poor deprecation message. You want to explain why There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally, why are you so careful in not breaking usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 '/'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I know not every language supports There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm'kay. I feel There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
*/ | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) : ''; | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
|
@@ -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; | ||
|
@@ -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. | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be un-necessary if we didn't perform the outrageous There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}`); | ||
} | ||
} | ||
|
||
|
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please switch to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move constraints on output to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting there through a path of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you not want to do this one before There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to |
||
return s.replace(/[^A-Za-z0-9]/g, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can make this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very convoluted phrasing:
How about
|
||
* 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; | ||
} |
There was a problem hiding this comment.
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.