Skip to content

fix(cli): improve openapi code generation for naming and typing #2722

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.

Sign up for GitHub

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

Merged
raymondfeng merged 3 commits into from
Apr 11, 2019
Merged

fix(cli): improve openapi code generation for naming and typing #2722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
faa1d28
fix(cli): improve openapi code generation for naming and typing
raymondfeng Apr 10, 2019
6082829
fix(cli): generate operation only for the 1st tag to avoid duplicate …
raymondfeng Apr 11, 2019
b6e02fc
feat(cli): normalize variable names for OpenAPI paths
raymondfeng Apr 11, 2019
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
32 changes: 20 additions & 12 deletions packages/cli/generators/openapi/schema-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
isExtension,
titleCase,
kebabCase,
escapePropertyOrMethodName,
escapeIdentifier,
toJsonStr,
} = require('./utils');

Expand Down Expand Up @@ -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}'})`;

Expand All @@ -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);
Copy link
Member

@bajtos bajtos Apr 11, 2019

Choose a reason for hiding this comment

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

Have you considered the following, to avoid duplication of getJSType?

: getJSType( 
  // The item type can be an alias such as `export type ID = string`
  propertyType.itemType.declaration ||
  // The item type can be `string`
  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 = {
Expand Down Expand Up @@ -277,7 +283,7 @@ const JSTypeMapping = {
*/
function getJSType(type) {
const ctor = JSTypeMapping[type];
return (ctor && ctor.name) || type;
return ctor && ctor.name;
}

/**
Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -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;
Expand Down
28 changes: 19 additions & 9 deletions packages/cli/generators/openapi/spec-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const {
kebabCase,
camelCase,
escapeIdentifier,
escapePropertyOrMethodName,
} = require('./utils');

const HTTP_VERBS = [
Expand Down Expand Up @@ -98,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) => {
/**
Expand Down Expand Up @@ -134,10 +134,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) {
Expand Down Expand Up @@ -206,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
}`;
});
Expand Down Expand Up @@ -288,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']) {
Expand Down
22 changes: 2 additions & 20 deletions packages/cli/generators/openapi/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -181,6 +163,6 @@ module.exports = {
kebabCase: utils.kebabCase,
camelCase: _.camelCase,
escapeIdentifier,
escapePropertyOrMethodName,
toJsonStr,
validateUrlOrFile,
};
14 changes: 12 additions & 2 deletions packages/cli/test/fixtures/openapi/3.0/customer.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,15 @@ paths:
application/json:
schema:
$ref: '#/components/schemas/Customer'
/customers/{id}:
/customers/{customer-id}:
get:
tags:
- Customer
description: Returns a customer based on a single ID
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
Expand All @@ -95,6 +95,8 @@ components:
schemas:
Name:
type: string
profileId:
type: string
AddressList:
items:
$ref: '#/components/schemas/Address'
Expand All @@ -110,6 +112,8 @@ components:
type: string
zipCode:
type: string
USAddress:
$ref: '#/components/schemas/Address'
Customer:
required:
- id
Expand All @@ -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'
8 changes: 4 additions & 4 deletions packages/cli/test/unit/openapi/controller-spec.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Customer>',
"async findCustomerById(@param({name: 'customer_id', " +
"in: 'path'}) customerId: number): Promise<Customer>",
implementation:
"return {id: id, 'first-name': 'John', last-name: 'Smith'};",
},
Expand Down
25 changes: 25 additions & 0 deletions packages/cli/test/unit/openapi/openapi-utils.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');
});
});
31 changes: 27 additions & 4 deletions packages/cli/test/unit/openapi/schema-model.unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[]',
Expand Down Expand Up @@ -374,14 +385,19 @@ describe('schema to model', () => {
},
{
name: 'first-name',
signature: "'first-name'?: string;",
signature: 'firstName?: string;',
Copy link
Member

@bajtos bajtos Apr 11, 2019

Choose a reason for hiding this comment

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

👍

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'})",
Copy link
Member

@bajtos bajtos Apr 11, 2019

Choose a reason for hiding this comment

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

I am wondering - why are we including name in the property metadata? Isn't the decorator inferring the property name automatically? I believe the property name specified in the metadata must match the property name in TypeScript source, otherwise things will break at runtime.

IMO, we should exclude name from the generated property metadata.

Feel free to leave such changes out of scope of this pull request.

Copy link
Contributor Author

@raymondfeng raymondfeng Apr 11, 2019

Choose a reason for hiding this comment

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

Some of the property names such as last-name have to be set in the @property decoration. I didn't bother to test if the original name is the same as the TS property.

Copy link
Member

@bajtos bajtos Apr 12, 2019

Choose a reason for hiding this comment

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

Some of the property names such as last-name have to be set in the @property decoration. I didn't bother to test if the original name is the same as the TS property.

So how is this going to work in practice? Let's say the OpenAPI schema describes a property called last-name. This means the clients will be sending request body payload like {"last-name": "Bajtos"}.

At runtime, this will create object {'last-name': 'Bajtos'}.

To allow TypeScript code to access this data, the TypeScript property must be called last-name too. If it's changed e.g. to lastName, then

  • TypeScript code won't be able to access the real property value.
  • Values set from TypeScript (e.g. data.lastName = 'Feng') will be ignored by model's toJSON function, thus they won't be persisted in the database and serialized to HTTP responses.

Some of the property names such as last-name have to be set in the @Property decoration.

Are you saying that our code in incorrectly inferring model property name from the TypeScript/JavaScript property name passed by TypeScript to @property? I.e. that the following won't work?

@model()
class MyModel extends Entity {
  @property()
  'last-name': string;
}
``

Copy link
Contributor Author

@raymondfeng raymondfeng Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The following code should work:

@model()
class MyModel extends Entity {
  @property()
  'last-name': string;
}

But we prefer to support the following:

@model({name: 'my-model'})
class MyModel extends Entity {
  @property({name: 'last-name'})
  lastName: string;
}

Mapping between JS/TS and JSON should be allowed. But @loopback/repository does not support that yet.

Copy link
Contributor Author

@raymondfeng raymondfeng Apr 12, 2019

Choose a reason for hiding this comment

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

See #2743

},
{
name: 'emails',
signature: 'emails?: string[];',
Expand All @@ -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',
},
]);
Expand Down