Skip to content

Commit

Permalink
working with <propName>Prop to avoid collisions
Browse files Browse the repository at this point in the history
refactor to suffix attributes and remove resource names
refactoring to enable cfn resource class members to be undefined if not required
initialize class members from props artifact in constructor
  • Loading branch information
moofish32 committed Jun 7, 2019
1 parent 3f7a0ad commit 6193da3
Show file tree
Hide file tree
Showing 11 changed files with 181 additions and 138 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-apigateway/test/test.resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,4 @@ export = {

}
}
};
};
8 changes: 6 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@ import { AttachedPolicies, undefinedIfEmpty } from './util';

export interface IGroup extends IIdentity {
/**
* Returns the IAM Group Name
*
* @attribute
*/
readonly groupName: string;

/**
* Returns the IAM Group ARN
*
* @attribute
*/
readonly groupArn: string;
Expand Down Expand Up @@ -131,8 +135,8 @@ export class Group extends GroupBase {
path: props.path,
});

this.groupName = group.groupName;
this.groupArn = group.groupArn;
this.groupName = group.attrRef;
this.groupArn = group.attrArn;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,9 @@ export class Role extends Resource implements IRole {
maxSessionDuration: props.maxSessionDurationSec,
});

this.roleId = role.roleId;
this.roleArn = role.roleArn;
this.roleName = role.roleName;
this.roleId = role.attrRoleId;
this.roleArn = role.attrArn;
this.roleName = role.attrRef;
this.policyFragment = new ArnPrincipal(this.roleArn).policyFragment;

function _flatten(policies?: { [name: string]: PolicyDocument }) {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ export class User extends Resource implements IIdentity {
loginProfile: this.parseLoginProfile(props)
});

this.userName = user.userName;
this.userArn = user.userArn;
this.userName = user.attrRef;
this.userArn = user.attrArn;
this.policyFragment = new ArnPrincipal(this.userArn).policyFragment;

if (props.groups) {
Expand Down
37 changes: 17 additions & 20 deletions packages/@aws-cdk/cdk/lib/cfn-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,10 @@ export class CfnResource extends CfnRefElement {
*/
protected readonly properties: any;

/**
* AWS resource property overrides.
*
* During synthesis, the method "renderProperties(this.overrides)" is called
* with this object, and merged on top of the output of
* "renderProperties(this.properties)".
*
* Derived classes should expose a strongly-typed version of this object as
* a public property called `propertyOverrides`.
*/
protected readonly untypedPropertyOverrides: any = { };

/**
* An object to be merged on top of the entire resource definition.
*/
private readonly rawOverrides: any = { };
private readonly rawOverrides: any = {};

/**
* Logical IDs of dependencies.
Expand All @@ -103,7 +91,7 @@ export class CfnResource extends CfnRefElement {
}

this.resourceType = props.type;
this.properties = props.properties || { };
this.properties = props.properties || {};

// if aws:cdk:enable-path-metadata is set, embed the current construct's
// path in the CloudFormation template, so it will be possible to trace
Expand Down Expand Up @@ -146,7 +134,7 @@ export class CfnResource extends CfnRefElement {
// object overwrite it with an object.
const isObject = curr[key] != null && typeof(curr[key]) === 'object' && !Array.isArray(curr[key]);
if (!isObject) {
curr[key] = { };
curr[key] = {};
}

curr = curr[key];
Expand Down Expand Up @@ -207,12 +195,11 @@ export class CfnResource extends CfnRefElement {
try {
// merge property overrides onto properties and then render (and validate).
const tags = TagManager.isTaggable(this) ? this.tags.renderTags() : undefined;

const properties = deepMerge(
this.properties || {},
this.updatedProperites,
{ tags },
this.untypedPropertyOverrides
);

const ret = {
Resources: {
// Post-Resolve operation since otherwise deepMerge is going to mix values into
Expand All @@ -221,7 +208,7 @@ export class CfnResource extends CfnRefElement {
Type: this.resourceType,
Properties: ignoreEmpty(properties),
DependsOn: ignoreEmpty(renderDependsOn(this.dependsOn)),
CreationPolicy: capitalizePropertyNames(this, this.options.creationPolicy),
CreationPolicy: capitalizePropertyNames(this, this.options.creationPolicy),
UpdatePolicy: capitalizePropertyNames(this, this.options.updatePolicy),
UpdateReplacePolicy: capitalizePropertyNames(this, this.options.updateReplacePolicy),
DeletionPolicy: capitalizePropertyNames(this, this.options.deletionPolicy),
Expand Down Expand Up @@ -266,6 +253,16 @@ export class CfnResource extends CfnRefElement {
return properties;
}

/**
* Return properties modified after initiation
*
* Resources that expose mutable properties should override this function to
* collect and return the properties object for this resource.
*/
protected get updatedProperites(): { [key: string]: any } {
return this.properties;
}

protected validateProperties(_properties: any) {
// Nothing
}
Expand Down Expand Up @@ -339,7 +336,7 @@ export function deepMerge(target: any, ...sources: any[]) {
// if the value at the target is not an object, override it with an
// object so we can continue the recursion
if (typeof(target[key]) !== 'object') {
target[key] = { };
target[key] = {};
}

deepMerge(target[key], value);
Expand Down
50 changes: 38 additions & 12 deletions packages/@aws-cdk/cdk/test/test.resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ export = {
test.done();
},

'untypedPropertyOverrides': {
'updatedProperties': {

'can be used by derived classes to specify overrides before render()'(test: Test) {
const stack = new Stack();
Expand All @@ -591,7 +591,7 @@ export = {
prop1: 'foo'
});

r.setProperty('prop2', 'bar');
r.prop2 = 'bar';

test.deepEqual(toCloudFormation(stack), { Resources:
{ MyResource:
Expand All @@ -605,7 +605,7 @@ export = {

const r = new CustomizableResource(stack, 'MyResource');

r.setProperty('prop3', 'zoo');
r.prop3 = 'zoo';

test.deepEqual(toCloudFormation(stack), { Resources:
{ MyResource:
Expand All @@ -619,8 +619,8 @@ export = {

const r = new CustomizableResource(stack, 'MyResource', { });

r.setProperty('prop3', 'zoo');
r.setProperty('prop2', 'hey');
r.prop3 = 'zoo';
r.prop2 = 'hey';

test.deepEqual(toCloudFormation(stack), { Resources:
{ MyResource:
Expand Down Expand Up @@ -696,26 +696,52 @@ class Counter extends CfnResource {
public increment(by = 1) {
this.properties.Count += by;
}

protected get updatedProperites(): { [key: string]: any } {
return {Count: this.properties.Count};
}
}

function withoutHash(logId: string) {
return logId.substr(0, logId.length - 8);
}

class CustomizableResource extends CfnResource {
public prop1: any;
public prop2: any;
public prop3: any;

constructor(scope: Construct, id: string, props?: any) {
super(scope, id, { type: 'MyResourceType', properties: props });
if (props !== undefined) {
this.prop1 = props.prop1;
this.prop2 = props.prop2;
this.prop3 = props.prop3;
}
}

public setProperty(key: string, value: any) {
this.untypedPropertyOverrides[key] = value;
public renderProperties(_properties: any) {
const props = this.updatedProperites;
const render: { [key: string]: any } = {};
for (const key of Object.keys(props)) {
render[key.toUpperCase()] = props[key];
}
return render;
}

public renderProperties(properties: any) {
return {
PROP1: properties.prop1,
PROP2: properties.prop2,
PROP3: properties.prop3
protected get updatedProperites(): { [key: string]: any } {
const props: { [key: string]: any } = {
prop1: this.prop1,
prop2: this.prop2,
prop3: this.prop3,
};
const cleanProps: { [key: string]: any } = { };
for (const key of Object.keys(props)) {
if (props[key] === undefined) {
continue;
}
cleanProps[key] = props[key];
}
return cleanProps;
}
}
14 changes: 3 additions & 11 deletions tools/awslint/lib/rules/cfn-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,7 @@ export class CfnResourceReflection {

this.namespace = fullname.split('::').slice(0, 2).join('::');

// special-case
const basename = this.basename
.replace(/VPC/g, 'Vpc')
.replace(/DB/g, 'Db');

this.attributePrefix = basename[0].toLowerCase() + basename.slice(1);
this.attributePrefix = 'attr';

this.attributeNames = cls.ownProperties
.filter(p => (p.docs.docs.custom || {}).cloudformationAttribute)
Expand All @@ -89,9 +84,6 @@ export class CfnResourceReflection {
return 'securityGroupId';
}

const cfnName = name.startsWith(this.basename) ? name.slice(this.basename.length) : name;

// if the CFN attribute name already have the type name as a prefix (i.e. RoleId), we only take the "Id" as the "name".
return this.attributePrefix + camelcase(cfnName, { pascalCase: true });
return this.attributePrefix + camelcase(name, { pascalCase: true });
}
}
}
2 changes: 1 addition & 1 deletion tools/awslint/lib/rules/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,4 +292,4 @@ constructLinter.add({
e.assert(!property.type.isAny, `${e.ctx.propsFqn}.${property.name}`);
}
}
});
});
14 changes: 10 additions & 4 deletions tools/awslint/lib/rules/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,16 @@ export class ResourceReflection {
private findAttributeProperties(): Attribute[] {
const result = new Array<Attribute>();

const attrPrefix = this.basename.charAt(0).toLowerCase() + this.basename.slice(1);
for (const p of this.construct.classType.allProperties) {
if (p.protected) {
continue; // skip any protected properties
}

// an attribute property is a property which starts with the type name
// (e.g. "bucketXxx") and/or has an @attribute doc tag.
// an attribute property is a property which starts with `attr`
// and/or has an @attribute doc tag.
const tag = getDocTag(p, 'attribute');
if (!p.name.startsWith(this.cfn.attributePrefix) && !tag) {
if (!p.name.startsWith(attrPrefix) && !tag) {
continue;
}

Expand Down Expand Up @@ -152,7 +153,12 @@ resourceLinter.add({
message: 'resources must represent all cloudformation attributes as attribute properties. missing property: ',
eval: e => {
for (const name of e.ctx.cfn.attributeNames) {
const found = e.ctx.attributes.find(a => a.names.includes(name));
const basename = e.ctx.basename.charAt(0).toLowerCase() + e.ctx.basename.slice(1);
const stripAttr = name.replace('attr', '');
const lookup = stripAttr.toLowerCase().startsWith(basename) ?
stripAttr.charAt(0).toLowerCase() + stripAttr.slice(1) :
basename + stripAttr;
const found = e.ctx.attributes.find(a => a.names.includes(lookup));
e.assert(found, `${e.ctx.fqn}.${name}`, name);
}
}
Expand Down
Loading

0 comments on commit 6193da3

Please sign in to comment.