-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Conversation
| propertyType.itemType.kind === 'class' | ||
| ? propertyType.itemType.className | ||
| : getJSType(propertyType.itemType.name); | ||
| : getJSType(propertyType.itemType.declaration) || |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
0b4bfba to
3f5eeeb
Compare
| { | ||
| name: 'us-office', | ||
| signature: | ||
| 'usOffice?: {\n street?: string;\n city?: string;\n' + |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
17cdea8 to
1877727
Compare
| : // 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); |
There was a problem hiding this comment.
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
)| { | ||
| name: 'first-name', | ||
| signature: "'first-name'?: string;", | ||
| signature: 'firstName?: string;', |
There was a problem hiding this comment.
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'})", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-namehave to be set in the@propertydecoration. 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'stoJSONfunction, 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;
}
``There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2743
- validated against openapi specs from https://www.berlin-group.org/psd2-access-to-bank-accounts
…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`.
891578d to
b6e02fc
Compare
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 testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈