From faa1d286bc8b228fb2dc16e1437459be7e93392f Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Apr 2019 08:08:54 -0700 Subject: [PATCH 1/3] fix(cli): improve openapi code generation for naming and typing - validated against openapi specs from https://www.berlin-group.org/psd2-access-to-bank-accounts --- .../cli/generators/openapi/schema-helper.js | 32 ++++++++++++------- .../cli/generators/openapi/spec-helper.js | 6 +--- packages/cli/generators/openapi/utils.js | 22 ++----------- .../test/fixtures/openapi/3.0/customer.yaml | 10 ++++++ .../test/unit/openapi/openapi-utils.unit.js | 25 +++++++++++++++ .../test/unit/openapi/schema-model.unit.js | 31 +++++++++++++++--- 6 files changed, 85 insertions(+), 41 deletions(-) create mode 100644 packages/cli/test/unit/openapi/openapi-utils.unit.js diff --git a/packages/cli/generators/openapi/schema-helper.js b/packages/cli/generators/openapi/schema-helper.js index 62c8d11288c1..3275d646780b 100644 --- a/packages/cli/generators/openapi/schema-helper.js +++ b/packages/cli/generators/openapi/schema-helper.js @@ -9,7 +9,7 @@ const { isExtension, titleCase, kebabCase, - escapePropertyOrMethodName, + escapeIdentifier, toJsonStr, } = require('./utils'); @@ -159,7 +159,7 @@ function mapObjectType(schema, options) { }), ); // The property name might have chars such as `-` - const propName = escapePropertyOrMethodName(p); + const propName = escapeIdentifier(p); let propDecoration = `@property({name: '${p}'})`; @@ -171,11 +171,17 @@ function mapObjectType(schema, options) { const itemType = propertyType.itemType.kind === 'class' ? propertyType.itemType.className - : getJSType(propertyType.itemType.name); + : // The item type can be an alias such as `export type ID = string` + getJSType(propertyType.itemType.declaration) || + // The item type can be `string` + getJSType(propertyType.itemType.name); if (itemType) { // Use `@property.array` for array types propDecoration = `@property.array(${itemType}, {name: '${p}'})`; - collectImports(typeSpec, propertyType.itemType); + if (propertyType.itemType.className) { + // The referenced item type is either a class or type + collectImports(typeSpec, propertyType.itemType); + } } } const propSpec = { @@ -277,7 +283,7 @@ const JSTypeMapping = { */ function getJSType(type) { const ctor = JSTypeMapping[type]; - return (ctor && ctor.name) || type; + return ctor && ctor.name; } /** @@ -319,13 +325,12 @@ function mapSchemaType(schema, options) { * @param {object} schema */ function resolveSchema(schemaMapping, schema) { - if (schema['x-$ref']) { - const resolved = schemaMapping[schema['x-$ref']]; - if (resolved) { - return resolved; - } + if (!schema['x-$ref']) return schema; + let resolved = schema; + while (resolved && resolved['x-$ref']) { + resolved = schemaMapping[resolved['x-$ref']]; } - return schema; + return resolved || schema; } /** @@ -350,7 +355,10 @@ function generateModelSpecs(apiSpec, options) { // `model` is `undefined` for primitive types if (model == null) continue; if (model.className) { - models.push(model); + // The model might be a $ref + if (!models.includes(model)) { + models.push(model); + } } } return models; diff --git a/packages/cli/generators/openapi/spec-helper.js b/packages/cli/generators/openapi/spec-helper.js index df6dacc0d4fd..239be4a947a6 100644 --- a/packages/cli/generators/openapi/spec-helper.js +++ b/packages/cli/generators/openapi/spec-helper.js @@ -14,7 +14,6 @@ const { kebabCase, camelCase, escapeIdentifier, - escapePropertyOrMethodName, } = require('./utils'); const HTTP_VERBS = [ @@ -134,10 +133,7 @@ function groupOperationsByController(apiSpec) { * @param {object} opSpec OpenAPI operation spec */ function getMethodName(opSpec) { - return ( - opSpec['x-operation-name'] || - escapePropertyOrMethodName(camelCase(opSpec.operationId)) - ); + return opSpec['x-operation-name'] || escapeIdentifier(opSpec.operationId); } function registerAnonymousSchema(names, schema, typeRegistry) { diff --git a/packages/cli/generators/openapi/utils.js b/packages/cli/generators/openapi/utils.js index 38d5397ff5d0..055129dfc7da 100644 --- a/packages/cli/generators/openapi/utils.js +++ b/packages/cli/generators/openapi/utils.js @@ -146,25 +146,7 @@ function escapeIdentifier(name) { return '_' + name; } if (!name.match(SAFE_IDENTIFER)) { - return _.camelCase(name); - } - return name; -} - -/** - * Escape a property/method name by quoting it if it's not a safe JavaScript - * identifier - * - * For example, - * - `default` -> `'default'` - * - `my-name` -> `'my-name'` - * - * @param {string} name - */ -function escapePropertyOrMethodName(name) { - if (JS_KEYWORDS.includes(name) || !name.match(SAFE_IDENTIFER)) { - // Encode the name with ', for example: 'my-name' - return `'${name}'`; + name = _.camelCase(name); } return name; } @@ -181,6 +163,6 @@ module.exports = { kebabCase: utils.kebabCase, camelCase: _.camelCase, escapeIdentifier, - escapePropertyOrMethodName, toJsonStr, + validateUrlOrFile, }; diff --git a/packages/cli/test/fixtures/openapi/3.0/customer.yaml b/packages/cli/test/fixtures/openapi/3.0/customer.yaml index 8ee276e16fb2..6a676327e015 100644 --- a/packages/cli/test/fixtures/openapi/3.0/customer.yaml +++ b/packages/cli/test/fixtures/openapi/3.0/customer.yaml @@ -95,6 +95,8 @@ components: schemas: Name: type: string + profileId: + type: string AddressList: items: $ref: '#/components/schemas/Address' @@ -110,6 +112,8 @@ components: type: string zipCode: type: string + USAddress: + $ref: '#/components/schemas/Address' Customer: required: - id @@ -121,9 +125,15 @@ components: type: string last-name: $ref: '#/components/schemas/Name' + profiles: + type: array + items: + $ref: '#/components/schemas/profileId' emails: type: array items: type: string addresses: $ref: '#/components/schemas/AddressList' + us-office: + $ref: '#/components/schemas/USAddress' diff --git a/packages/cli/test/unit/openapi/openapi-utils.unit.js b/packages/cli/test/unit/openapi/openapi-utils.unit.js new file mode 100644 index 000000000000..c30ef6e0b6cd --- /dev/null +++ b/packages/cli/test/unit/openapi/openapi-utils.unit.js @@ -0,0 +1,25 @@ +// Copyright IBM Corp. 2019. All Rights Reserved. +// Node module: @loopback/cli +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +const expect = require('@loopback/testlab').expect; +const utils = require('../../../generators/openapi/utils'); + +describe('openapi utils', () => { + it('escapes keywords for an identifier', () => { + expect(utils.escapeIdentifier('for')).to.eql('_for'); + }); + + it('escapes illegal chars for an identifier', () => { + expect(utils.escapeIdentifier('foo bar')).to.eql('fooBar'); + expect(utils.escapeIdentifier('foo-bar')).to.eql('fooBar'); + expect(utils.escapeIdentifier('foo.bar')).to.eql('fooBar'); + }); + + it('does not escape legal chars for an identifier', () => { + expect(utils.escapeIdentifier('foobar')).to.eql('foobar'); + expect(utils.escapeIdentifier('fooBar')).to.eql('fooBar'); + expect(utils.escapeIdentifier('Foobar')).to.eql('Foobar'); + }); +}); diff --git a/packages/cli/test/unit/openapi/schema-model.unit.js b/packages/cli/test/unit/openapi/schema-model.unit.js index 1e08873fe6b0..602a8150672e 100644 --- a/packages/cli/test/unit/openapi/schema-model.unit.js +++ b/packages/cli/test/unit/openapi/schema-model.unit.js @@ -282,6 +282,17 @@ describe('schema to model', () => { declaration: 'string', signature: 'Name', }, + { + description: 'profileId', + name: 'profileId', + className: 'ProfileId', + fileName: 'profile-id.model.ts', + properties: [], + imports: [], + import: "import {ProfileId} from './profile-id.model';", + declaration: 'string', + signature: 'ProfileId', + }, { description: 'AddressList', name: 'Address[]', @@ -374,14 +385,19 @@ describe('schema to model', () => { }, { name: 'first-name', - signature: "'first-name'?: string;", + signature: 'firstName?: string;', decoration: "@property({name: 'first-name'})", }, { name: 'last-name', - signature: "'last-name'?: Name;", + signature: 'lastName?: Name;', decoration: "@property({name: 'last-name'})", }, + { + name: 'profiles', + signature: 'profiles?: ProfileId[];', + decoration: "@property.array(String, {name: 'profiles'})", + }, { name: 'emails', signature: 'emails?: string[];', @@ -392,17 +408,24 @@ describe('schema to model', () => { signature: 'addresses?: AddressList;', decoration: "@property.array(Address, {name: 'addresses'})", }, + { + name: 'us-office', + signature: 'usOffice?: Address;', + decoration: "@property({name: 'us-office'})", + }, ], imports: [ "import {Name} from './name.model';", + "import {ProfileId} from './profile-id.model';", "import {Address} from './address.model';", "import {AddressList} from './address-list.model';", ], import: "import {Customer} from './customer.model';", kind: 'class', declaration: - "{\n id: number;\n 'first-name'?: string;\n 'last-name'?: Name;\n" + - ' emails?: string[];\n addresses?: AddressList;\n}', + '{\n id: number;\n firstName?: string;\n lastName?: Name;\n' + + ' profiles?: ProfileId[];\n emails?: string[];\n' + + ' addresses?: AddressList;\n usOffice?: Address;\n}', signature: 'Customer', }, ]); From 6082829f18d5bc0153ae092eae6b57747ce06f42 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 10 Apr 2019 17:43:00 -0700 Subject: [PATCH 2/3] fix(cli): generate operation only for the 1st tag to avoid duplicate routes An OpenAPI operation may have more than one tags. Adding the operation to two controllers produces duplicate routes and it fails the application. --- packages/cli/generators/openapi/spec-helper.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli/generators/openapi/spec-helper.js b/packages/cli/generators/openapi/spec-helper.js index 239be4a947a6..517135f0735c 100644 --- a/packages/cli/generators/openapi/spec-helper.js +++ b/packages/cli/generators/openapi/spec-helper.js @@ -97,8 +97,9 @@ function groupOperationsByController(apiSpec) { let controllers = ['OpenApiController']; if (op['x-controller-name']) { controllers = [op['x-controller-name']]; - } else if (op.tags) { - controllers = op.tags.map(t => titleCase(t + 'Controller')); + } else if (Array.isArray(op.tags) && op.tags.length) { + // Only add the operation to first tag to avoid duplicate routes + controllers = [titleCase(op.tags[0] + 'Controller')]; } controllers.forEach((c, index) => { /** From b6e02fca15cc57a7992763934fcc93ffa9b3607f Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Thu, 11 Apr 2019 08:18:03 -0700 Subject: [PATCH 3/3] feat(cli): normalize variable names for OpenAPI paths Non-word characters in variable names for OpenAPI paths are not allowed by `@loopback/rest`. --- packages/cli/generators/openapi/spec-helper.js | 17 +++++++++++++++-- .../cli/test/fixtures/openapi/3.0/customer.yaml | 4 ++-- .../test/unit/openapi/controller-spec.unit.js | 8 ++++---- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/packages/cli/generators/openapi/spec-helper.js b/packages/cli/generators/openapi/spec-helper.js index 517135f0735c..cd29953376ab 100644 --- a/packages/cli/generators/openapi/spec-helper.js +++ b/packages/cli/generators/openapi/spec-helper.js @@ -203,7 +203,13 @@ function buildMethodSpec(controllerSpec, op, options) { const pType = mapSchemaType(p.schema, options); addImportsForType(pType); comments.push(`@param ${name} ${p.description || ''}`); - return `@param({name: '${p.name}', in: '${p.in}'}) ${name}: ${ + + // Normalize parameter name to match `\w` + let paramName = p.name; + if (p.in === 'path') { + paramName = paramName.replace(/[^\w]+/g, '_'); + } + return `@param({name: '${paramName}', in: '${p.in}'}) ${name}: ${ pType.signature }`; }); @@ -285,10 +291,17 @@ function buildMethodSpec(controllerSpec, op, options) { returnType.signature }>`; comments.unshift(op.spec.description || '', '\n'); + + // Normalize path variable names to alphanumeric characters including the + // underscore (Equivalent to [A-Za-z0-9_]). Please note `@loopback/rest` + // does not allow other characters that don't match `\w`. + const opPath = op.path.replace(/\{[^\}]+\}/g, varName => + varName.replace(/[^\w\{\}]+/g, '_'), + ); const methodSpec = { description: op.spec.description, comments, - decoration: `@operation('${op.verb}', '${op.path}')`, + decoration: `@operation('${op.verb}', '${opPath}')`, signature, }; if (op.spec['x-implementation']) { diff --git a/packages/cli/test/fixtures/openapi/3.0/customer.yaml b/packages/cli/test/fixtures/openapi/3.0/customer.yaml index 6a676327e015..ca94b9ca5181 100644 --- a/packages/cli/test/fixtures/openapi/3.0/customer.yaml +++ b/packages/cli/test/fixtures/openapi/3.0/customer.yaml @@ -69,7 +69,7 @@ paths: application/json: schema: $ref: '#/components/schemas/Customer' - /customers/{id}: + /customers/{customer-id}: get: tags: - Customer @@ -77,7 +77,7 @@ paths: x-implementation: "return {id: id, 'first-name': 'John', last-name: 'Smith'};" operationId: find customer by id parameters: - - name: id + - name: customer-id in: path description: ID of customer to fetch required: true diff --git a/packages/cli/test/unit/openapi/controller-spec.unit.js b/packages/cli/test/unit/openapi/controller-spec.unit.js index bdd55dc3f624..d56e126f37ac 100644 --- a/packages/cli/test/unit/openapi/controller-spec.unit.js +++ b/packages/cli/test/unit/openapi/controller-spec.unit.js @@ -59,13 +59,13 @@ describe('openapi to controllers/models', () => { comments: [ 'Returns a customer based on a single ID', '\n', - '@param id ID of customer to fetch', + '@param customerId ID of customer to fetch', '@returns customer response', ], - decoration: "@operation('get', '/customers/{id}')", + decoration: "@operation('get', '/customers/{customer_id}')", signature: - "async findCustomerById(@param({name: 'id', in: 'path'}) " + - 'id: number): Promise', + "async findCustomerById(@param({name: 'customer_id', " + + "in: 'path'}) customerId: number): Promise", implementation: "return {id: id, 'first-name': 'John', last-name: 'Smith'};", },