-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[typescript] allOf with multiple values is not handled properly #927
Comments
I guess this is due to the fact that we are using interfaces here, which can extend multiple interfaces (see https://www.typescriptlang.org/docs/handbook/interfaces.html#extending-interfaces) as opposed to classes. So this could be implemented. @Fredx87 would you like to give it a try and support @TiFu in his typescript-refactoring #802 |
I've just run into this same issue with the Spring generator, except in that case it's generating classes so multiple inheritance wouldn't work. |
@macjohnny for java at least the model could be implementing multiple interfaces instead of extending a concrete class but I'm guessing that's a fairly big change to the generator. |
Another option for Typescript is the use of intersection types. openapi: 3.0.0
info:
title: TestApi
version: 1.0.0
paths:
/test:
get:
summary: Test
operationId: testApi
responses:
"200":
description: Ok
content:
application/json:
schema:
$ref: "#/components/schemas/ModelC"
components:
schemas:
ModelA:
properties:
foo:
type: string
ModelB:
properties:
bar:
type: string
ModelC:
allOf:
- $ref: "#/components/schemas/ModelA"
- $ref: "#/components/schemas/ModelB"
- type: object
properties:
baz:
type: string type ModelC = ModelA & ModelB & {
baz?: string
} |
Hi folks, I've filed #1360 with better inheritance/composition support. Please give it a try and let us know if you've any feedback for us. |
hi @wing328 I checked against master from today and it is even worst
and it should be
so it look like it is broken now |
@topce let me look into it... |
Based on my understanding, it should not extend either Model A or B because both the definitions of A & B lack the #1360 does not support multiple inheritance and it becomes a priority now as typescript-angular needs it. I'll see if I can add it in the next few days... |
Hi @wing328 |
@topce perfectly fine to disagree :) I read https://www.typescriptlang.org/docs/handbook/classes.html and it seems like "extends" is the Inheritance in TS. Here is an example copied from that page: class Animal {
move(distanceInMeters: number = 0) {
console.log(`Animal moved ${distanceInMeters}m.`);
}
}
class Dog extends Animal {
bark() {
console.log('Woof! Woof!');
}
} Thanks for the link, which definitely helps me understand more about union types. I'm no expert in TS and will let you guys to decide how polymorphim should be implemented in TS. For composition, I still think the model/class should include all properties instead of using cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10) |
@wing328 |
From the spec: Extending an interface implies a parent-child relationship in the type hierarchy. This is therefore incorrect in the context of the spec. I think that intersection types would be appropriate to use in this case. |
@TiFu
Agree with you partially
But first of all OAS should be language neutral
And indeed in TypeScript language context
Interface extensions and intersection types
are equivalent :variable declared with as extended interface
can be assigned to variable declared with type intersection
So it is just matter of taste in this concrete example.
Unfortunately some typescript code is generated like
Java code for example in java there is not strict compilation
So in some ts code generated code there are unnecessary checks
For null undefined value
Already mention this to @wing328 when he implemented some features
For another typescript client
As old saying said you can program FORTRAN in any language ;-)
…On Tue, 11 Dec 2018 at 22:44, Tino Fuhrmann ***@***.***> wrote:
From the spec: While composition offers model extensibility, it does not
imply a hierarchy between the models.
Extending an interface implies a parent-child relationship in the type
hierarchy. This is therefore incorrect in the context of the spec. I think
that intersection types would be appropriate to use in this case.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#927 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABRthvOzodVKxHhA3EOf4Vf7uUNLXbKks5u4CdJgaJpZM4WRkAb>
.
|
Small example for
|
Playground link of above example |
I've filed: #927 and here is what I got for allOfMultiParent.yaml
This is far from complete but I think at least we're heading the right direction. |
Thank you @wing328 !
I prefer generated code like before with extended interfaces or intersection types (less code , less complicated, less duplication of code ). |
Thanks for fixing this, we're experimenting the same issue. Will this changes applied to the Typescript-Axios template as well? |
Looks like currently both typescript-angular and typescript-axios handle this properly (and identically):
Note, that typescript-axios with |
Hi @macjohnny @wing328 @amakhrov It looks like for same time this is broken before
that was relatively OK if we ignore not needed imports
All generated from same model ArchiveNode:
allOf:
- $ref: '#/components/schemas/Node'
- type: object
properties:
deletedBy:
type: string
deletedDate:
type: string
format: date So little bit annoying same/similar error happen in attempt to minimize stupid regerssions
|
@topce thanks for bringing this up. Indeed support of allOf across typescript generators is inconsistent and incomplete atm.
Yep, that's something that I still plan to do (filed in this issue) - decided to postpone till typescript examples cleanup by @macjohnny (which is planned to be done soon after 4.3.0 release) to avoid redundant work.
It depends on a generator. I just tested typescript-axios and typescript-angular (using a version of 2_0/petstore-with-fake-endpoints-models-for-testing.yaml where I removed |
@amakhrov For me it does not work Node:
type: object
description: representation of a node
properties:
id:
type: string
type:
type: string
service:
type: string
enum:
- library
- information
- events
- newsgroups
- directory
name:
type: string
title:
$ref: '#/components/schemas/i18nProperty'
description:
$ref: '#/components/schemas/i18nProperty'
properties:
type: object
additionalProperties:
type: string
permissions:
type: object
additionalProperties:
type: string
description: >
Representation of the permissions given to the user making the call.
To know what are the permissions for the specific node, the
permission
definition may vary depending on its type or service.
attachments:
type: array
items:
$ref: '#/components/schemas/Attachment'
parentId:
type: string
notifications:
type: string
favourite:
type: boolean
description: |
true if the current user faved the node
hasSubFolders:
type: boolean
ArchiveNode:
allOf:
- $ref: '#/components/schemas/Node'
- type: object
properties:
deletedBy:
type: string
deletedDate:
type: string
format: date Archive node does not include properties of node !? import { Node } from './node';
import { Attachment } from './attachment';
import { ArchiveNodeAllOf } from './archiveNodeAllOf';
export interface ArchiveNode {
deletedBy?: string;
deletedDate?: string;
}
export namespace ArchiveNode {} So I think it is broken. |
@amakhrov |
@topce oops. I realized I had a older docker image cached locally. After pulling latest image I see the same issue as you're describing. |
@amakhrov |
No idea, and I don't even have a prime suspect out of the typescript-related PRs merged since 4.2.3. I also consider a possibility that it was not a TS-specific PR at all. |
Thank you! |
Look like this was the breaking change: https://github.com/OpenAPITools/openapi-generator/pull/5526/files#diff-57d7532cf464a8d7c24aab4b22ceb993R1138 Current effect is:
However, |
Currently having the same issue. Glad to see it is already being worked on. |
Hi @wing328 and @jimschubert |
@topce sorry completely missed this. Please PM via Slack if you still need help with this: https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM |
@wing328 no problem, latest stable is working OK for me |
Is this really fixed? Does not seem to work for |
I am seeing this too! using openapigenerator 6.0.1, with generator "typescript". the API is like this:
In the generated types, the type of |
Created #13417 to track |
Description
If a schema is defined with the
allOf
operator with multiple values, the generated model is an interface that extends only the first value.openapi-generator version
3.2.2
OpenAPI declaration file content or url
Command line used for generation
docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v3.2.2 generate -i /local/swagger.yaml -g typescript-angular -o /local/ts-angular -DngVersion=6.0
docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v3.2.2 generate -i /local/swagger.yaml -g typescript-fetch -o /local/ts-fetch
Suggest a fix/enhancement
The model generated for
ModelC
is the following interface:When it should be:
The text was updated successfully, but these errors were encountered: