From d13122337500000e7da03a4204c633f5fa1e62d7 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 5 Oct 2018 11:40:24 +0200 Subject: [PATCH] fix(aws-ssm): correctly emit Association.Name The rename used to happen in the cfnspec, which helped the model generate correct names, but the code generator would lose the ability to access the original name, and so it could not be emitted correctly. Add a property renaming feature to the 'cfn2ts' code generator (configured via a special entry in package.json). Fixes #852. --- packages/@aws-cdk/aws-ssm/package.json | 5 ++ .../aws-ssm/test/test.ssm-document.ts | 23 ++++++ .../500_SSM_AssociationName_patch.json | 16 ---- .../500_SSM_AssociationName_patch.md | 9 --- tools/cfn2ts/lib/codegen.ts | 79 +++++++++++++------ tools/cfn2ts/lib/index.ts | 18 ++++- 6 files changed, 100 insertions(+), 50 deletions(-) create mode 100644 packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts delete mode 100644 packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json delete mode 100644 packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md diff --git a/packages/@aws-cdk/aws-ssm/package.json b/packages/@aws-cdk/aws-ssm/package.json index 969afb616dcf7..526a296ba3c8c 100644 --- a/packages/@aws-cdk/aws-ssm/package.json +++ b/packages/@aws-cdk/aws-ssm/package.json @@ -39,6 +39,11 @@ "cdk-build": { "cloudformation": "AWS::SSM" }, + "cfn2ts": { + "rename": { + "AWS::SSM::Association/Name": "DocumentName" + } + }, "keywords": [ "aws", "cdk", diff --git a/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts b/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts new file mode 100644 index 0000000000000..31e4db52db95e --- /dev/null +++ b/packages/@aws-cdk/aws-ssm/test/test.ssm-document.ts @@ -0,0 +1,23 @@ +import { expect, haveResource } from '@aws-cdk/assert'; +import cdk = require('@aws-cdk/cdk'); +import { Test } from 'nodeunit'; +import ssm = require('../lib'); + +export = { + 'association name is rendered properly in L1 construct'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + + // WHEN + new ssm.cloudformation.AssociationResource(stack, 'Assoc', { + documentName: 'document', + }); + + // THEN + expect(stack).to(haveResource('AWS::SSM::Association', { + Name: 'document', + })); + + test.done(); + } +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json b/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json deleted file mode 100644 index 8ce9d9a3deb39..0000000000000 --- a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "ResourceTypes": { - "AWS::SSM::Association": { - "patch": { - "description": "rename +Name+ to +DocumentName+ to clearly indicate what the attribute is for", - "operations": [ - { - "from": "/Properties/Name", - "op": "move", - "path": "/Properties/DocumentName" - } - ] - } - } - } -} diff --git a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md b/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md deleted file mode 100644 index 9c311b0dbe502..0000000000000 --- a/packages/@aws-cdk/cfnspec/spec-source/500_SSM_AssociationName_patch.md +++ /dev/null @@ -1,9 +0,0 @@ -We discovered that SSM::Assocations have an AssociationName and a Name parameter, which introduced a conflict. - -Apart from that we have to resolve the conflict, the original heuristics were even wrong: - -- Name is NOT the name of the resource to be created, it's the name of the SSM document to associate with. -- AssociationName IS the name of the resource to be created, that's fine, it's the name that we would have -picked for that attribute anyway. - -We rename "Name" to "DocumentName" to clearly indicate what the Name attribute is for. diff --git a/tools/cfn2ts/lib/codegen.ts b/tools/cfn2ts/lib/codegen.ts index ffd18a56ef6ec..f559be52859f1 100644 --- a/tools/cfn2ts/lib/codegen.ts +++ b/tools/cfn2ts/lib/codegen.ts @@ -24,7 +24,7 @@ export default class CodeGenerator { * @param moduleName the name of the module (used to determine the file name). * @param spec CloudFormation resource specification */ - constructor(moduleName: string, private readonly spec: schema.Specification) { + constructor(moduleName: string, private readonly spec: schema.Specification, private readonly renames: {[key: string]: string}) { this.outputFile = `${moduleName}.generated.ts`; this.code.openFile(this.outputFile); @@ -117,23 +117,26 @@ export default class CodeGenerator { this.code.closeBlock(); } - private emitPropsType(resourceName: genspec.CodeName, spec: schema.ResourceType): genspec.CodeName | undefined { - if (!spec.Properties || Object.keys(spec.Properties).length === 0) { return; } - const name = genspec.CodeName.forResourceProperties(resourceName); + /** + * Emit the XxxProps interface for a resource + */ + private emitPropsType(resourceName: genspec.CodeName, spec: schema.ResourceType): PropsInterfaceType { + if (!spec.Properties || Object.keys(spec.Properties).length === 0) { return { attributesTable: {}}; } + const codeName = genspec.CodeName.forResourceProperties(resourceName); this.docLink(spec.Documentation); - this.code.openBlock(`export interface ${name.className}`); + this.code.openBlock(`export interface ${codeName.className}`); - const conversionTable = this.emitPropsTypeProperties(resourceName.specName!, spec.Properties); + const attributesTable = this.emitPropsTypeProperties(resourceName.specName!, spec.Properties); this.code.closeBlock(); this.code.line(); - this.emitValidator(name, spec.Properties, conversionTable); + this.emitValidator(codeName, spec.Properties, attributesTable); this.code.line(); - this.emitCloudFormationMapper(name, spec.Properties, conversionTable); + this.emitCloudFormationMapper(codeName, spec.Properties, attributesTable); - return name; + return { codeName, attributesTable }; } /** @@ -143,28 +146,37 @@ export default class CodeGenerator { */ private emitPropsTypeProperties(resourceName: SpecName, propertiesSpec: { [name: string]: schema.Property }): Dictionary { const propertyMap: Dictionary = {}; - - // Sanity check that our renamed "Name" is not going to conflict with a real property - const renamedNameProperty = resourceNameProperty(resourceName); - const lowerNames = Object.keys(propertiesSpec).map(s => s.toLowerCase()); - if (lowerNames.indexOf('name') !== -1 && lowerNames.indexOf(renamedNameProperty.toLowerCase()) !== -1) { - // tslint:disable-next-line:max-line-length - throw new Error(`Oh gosh, we want to rename ${resourceName.fqn}'s 'Name' property to '${renamedNameProperty}', but that property already exists! We need to find a solution to this problem.`); - } + const finalNames = new Set(); Object.keys(propertiesSpec).sort(propertyComparator).forEach(propName => { const originalName = propName; const propSpec = propertiesSpec[propName]; const additionalDocs = resourceName.relativeName(propName).fqn; + // Apply property renames + const renameKey = `${resourceName.fqn}/${propName}`; + if (renameKey in this.renames) { + // User-defined rename + propName = this.renames[renameKey]; + } + if (propName.toLocaleLowerCase() === 'name') { - propName = renamedNameProperty; + // We like to include the resource name in the "name" property + propName = resourceNameProperty(resourceName); + } + + if (originalName !== propName) { // tslint:disable-next-line:no-console - console.error(`Renamed property 'Name' of ${resourceName.fqn} to '${renamedNameProperty}'`); + console.error(`[${resourceName.fqn}] Renamed property '${originalName}' to '${propName}'`); } + if (finalNames.has(propName)) { + throw new Error(`Oh gosh, there was already a property named ${propName}!`); + } + finalNames.add(propName); const resourceCodeName = genspec.CodeName.forResource(resourceName); const newName = this.emitProperty(resourceCodeName, propName, propSpec, quoteCode(additionalDocs)); + propertyMap[originalName] = newName; }); return propertyMap; @@ -265,7 +277,7 @@ export default class CodeGenerator { this.code.line(` * @param properties the properties of this ${quoteCode(resourceName.className)}`); this.code.line(' */'); const optionalProps = spec.Properties && !Object.values(spec.Properties).some(p => p.Required); - const propsArgument = propsType ? `, properties${optionalProps ? '?' : ''}: ${propsType.className}` : ''; + const propsArgument = propsType.codeName ? `, properties${optionalProps ? '?' : ''}: ${propsType.codeName.className}` : ''; this.code.openBlock(`constructor(parent: ${CONSTRUCT_CLASS}, name: string${propsArgument})`); this.code.line(`super(parent, name, { type: ${resourceName.className}.resourceTypeName${propsType ? ', properties' : ''} });`); // verify all required properties @@ -273,8 +285,8 @@ export default class CodeGenerator { for (const pname of Object.keys(spec.Properties)) { const prop = spec.Properties[pname]; if (prop.Required) { - const propName = pname.toLocaleLowerCase() === 'name' ? resourceNameProperty(resourceName.specName!) : pname; - this.code.line(`${CORE}.requireProperty(properties, '${genspec.cloudFormationToScriptName(propName)}', this);`); + const attributeName = propsType.attributesTable[pname]; + this.code.line(`${CORE}.requireProperty(properties, '${genspec.cloudFormationToScriptName(attributeName)}', this);`); } } } @@ -304,9 +316,9 @@ export default class CodeGenerator { this.code.closeBlock(); - if (propsType) { + if (propsType.codeName) { this.code.line(); - this.emitCloudFormationPropertiesOverride(propsType); + this.emitCloudFormationPropertiesOverride(propsType.codeName); } this.closeClass(resourceName); @@ -619,6 +631,25 @@ export default class CodeGenerator { } } +/** + * Represents a PropsType that has been generated + */ +interface PropsInterfaceType { + /** + * Name in the code of the XxxProps type + * + * Missing if no type got generated because there were no properties. + */ + codeName?: genspec.CodeName; + + /** + * Name conversion table for properties on this props type + * + * Maps cfn name -> attribute name. + */ + attributesTable: Dictionary; +} + /** * Return a comma-separated list of validator functions for the given types */ diff --git a/tools/cfn2ts/lib/index.ts b/tools/cfn2ts/lib/index.ts index d06c845b6ec6f..f4e08b598889c 100644 --- a/tools/cfn2ts/lib/index.ts +++ b/tools/cfn2ts/lib/index.ts @@ -14,7 +14,23 @@ export default async function(scope: string, outPath: string, force: boolean) { } const name = packageName(scope); - const generator = new CodeGenerator(name, spec); + // Read package.json in the current directory to determine renames + const renames: {[key: string]: string} = {}; + let packageJson; + try { + packageJson = require(path.resolve(process.cwd(), 'package.json')); + } catch (e) { + // Nothing + } + if (packageJson && packageJson.cfn2ts && packageJson.cfn2ts.rename) { + // tslint:disable-next-line:no-console + console.error('Reading renames from package.json'); + for (const [key, value] of Object.entries(packageJson.cfn2ts.rename)) { + renames[key] = value as any; + } + } + + const generator = new CodeGenerator(name, spec, renames); if (!force && await generator.upToDate(outPath)) { // tslint:disable-next-line:no-console