Skip to content
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

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

Merged
merged 3 commits into from
Apr 11, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Apr 10, 2019

Fixing issues to generate code from OpenAPI specs from https://www.berlin-group.org/psd2-access-to-bank-accounts. Here is the swagger doc.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng raymondfeng requested a review from bajtos as a code owner April 10, 2019 15:39
@raymondfeng
Copy link
Contributor Author

@@ -171,11 +171,15 @@ function mapObjectType(schema, options) {
const itemType =
propertyType.itemType.kind === 'class'
? propertyType.itemType.className
: getJSType(propertyType.itemType.name);
: getJSType(propertyType.itemType.declaration) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the declaration here mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, for a type alias export type IDType = string:

  • name: `IDType
  • declaration: string

{
name: 'us-office',
signature:
'usOffice?: {\n street?: string;\n city?: string;\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if the signature can be usOffice: USAddress; or usOffice: Address here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be fixed. I have to recursively resolve $ref for some models.

@raymondfeng raymondfeng force-pushed the fix-openapi-cli branch 2 times, most recently from 17cdea8 to 1877727 Compare April 10, 2019 22:54
: // 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

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
)

@@ -374,14 +385,19 @@ describe('schema to model', () => {
},
{
name: 'first-name',
signature: "'first-name'?: string;",
signature: 'firstName?: string;',
Copy link
Member

Choose a reason for hiding this comment

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

👍

{
name: 'profiles',
signature: 'profiles?: ProfileId[];',
decoration: "@property.array(String, {name: 'profiles'})",
Copy link
Member

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

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

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

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

Choose a reason for hiding this comment

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

See #2743

…routes

An OpenAPI operation may have more than one tags. Adding the operation to
two controllers produces duplicate routes and it fails the application.
Non-word characters in variable names for OpenAPI paths are not allowed
by `@loopback/rest`.
@raymondfeng raymondfeng merged commit a3d0dfc into master Apr 11, 2019
@raymondfeng raymondfeng deleted the fix-openapi-cli branch April 11, 2019 17:20
@dhmlau dhmlau added this to the April 2019 milestone milestone Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants