Openapi-generator: [typescript] allOf with multiple values is not handled properly

[Fredx87]

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

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

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:

export interface ModelC extends ModelA { 
    baz?: string;
}

When it should be:
```typescript
export interface ModelC extends ModelA, ModelB {
baz?: string;
}

[macjohnny]

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

[mwoodland]

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]

@jmini @wing328 do you think this is a general bug? If the target language does not allow extending multiple classes, then the generated model should copy all properties.

[mwoodland]

@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.

[Shockolate]

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
}

[wing328]

Hi folks, I've filed https://github.com/OpenAPITools/openapi-generator/pull/1360 with better inheritance/composition support. Please give it a try and let us know if you've any feedback for us.

[topce]

hi @wing328 I checked against master from today and it is even worst
generated code

import { ModelA } from './modelA';
import { ModelB } from './modelB';


export interface ModelC { 
    baz?: string;
}

and it should be

import { ModelA } from './modelA';
import { ModelB } from './modelB';


export interface ModelC extends ModelA, ModelB { 
    baz?: string;
}

so it look like it is broken now
Few days ago it was working (at least for one interface inheritance)
so it is some recent PR that broke it

[wing328]

@topce let me look into it...

[wing328]

and it should be

import { ModelA } from './modelA';
import { ModelB } from './modelB';


export interface ModelC extends ModelA, ModelB { 
    baz?: string;
}

Based on my understanding, it should not extend either Model A or B because both the definitions of A & B lack the discriminator field, e.g. https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#models-with-polymorphism-support

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#composition-and-inheritance-polymorphism

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...

[topce]

Hi @wing328
Thank you for fast response but I do not agree with your opinion.
Here we do not speak about polymorphism.
Do not let extends in interface fool you.
Here it is typical case of composituion
And both approach interfaces or intersection types are valid .
Regarding polymorphism I think in case of typescript it
Should be implemented as tagged union types
https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#tagged-union-types

[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 extends

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)

[topce]

@wing328
Not easy to be expert in TypeScript some of smartest developers of Microsoft
every two months give us new version ;-)
As your code example is about class extends not interface extends in my opinion it is not relevant
for this issue.
Regarding extending interface there is alternative intersection types like @Shockolate wrote in comment above
I prefer both approaches (extending interfaces or type intersection ) then just listing properties
because it is less code generated and less code changed
for example
if ModelA is changed we do not need to change ModelC

[TiFu]

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.

[topce]

@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 [e-mail]
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
https://github.com/OpenAPITools/openapi-generator/issues/927#issuecomment-446373708,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABRthvOzodVKxHhA3EOf4Vf7uUNLXbKks5u4CdJgaJpZM4WRkAb
.

[topce]

Small example for
@wing328 and @TiFu how polymorphic types could be generated in TypeScript using tagged union types

type huntingSkillType = "clueless" | "lazy" | "adventurous" | "aggressive"

interface PetDiscriminator {
    petType: "Pet"|"Dog"|"Cat"
}

interface PetProperties {
    name: string;
}

interface Pet extends PetDiscriminator , PetProperties {
    petType: "Pet"
}
interface Cat extends PetDiscriminator , PetProperties {
    petType: "Cat";
    huntingSkill: huntingSkillType;
}
interface Dog  extends PetDiscriminator , PetProperties {
    petType: "Dog";
    packSize: number;
}

type PetType = Pet | Cat | Dog;


function getPet(pet: PetType) {
  switch (pet.petType) {
    case "Pet":
      return "Pet" + pet.name;

    case "Dog":
      return `Dog (${pet.packSize})`;

    case "Cat":
      return `Cat (${pet.huntingSkill})`;
  }
}

[topce]

Playground link of above example

[wing328]

I've filed: https://github.com/OpenAPITools/openapi-generator/issues/927 and here is what I got for allOfMultiParent.yaml

import { Child } from './child';
import { Human } from './human';
import { Person } from './person';


/**
 * A representation of an adult
 */
export interface Adult extends Person, Human {
    duplicatedOptional?: number;
    duplicatedRequired: number;
    children?: Array<Child>;
    adultRequired?: boolean;
}

This is far from complete but I think at least we're heading the right direction.

[topce]

Thank you @wing328 !
If fix also this problem by coping all properties in new interface (original problem of this issue).
it generate something like this

 * 
 *
 * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
 * https://openapi-generator.tech
 * Do not edit the class manually.
 */
import { ModelA } from './modelA';
import { ModelB } from './modelB';


export interface ModelC { 
    foo?: string;
    bar?: string;
    baz?: string;
}

I prefer generated code like before with extended interfaces or intersection types (less code , less complicated, less duplication of code ).
By the way it looks that 2 import lines are not necessary anymore .

[oscarCrespo]

Thanks for fixing this, we're experimenting the same issue. Will this changes applied to the Typescript-Axios template as well?

[amakhrov]

Looks like currently both typescript-angular and typescript-axios handle this properly (and identically):

• allOf when one of the referenced schemas has discriminator: extends the discriminated base interface
  allOf without any discriminator: all properties from referenced schemas are cloned into the target interface.

Note, that typescript-axios with withSeparateModelsAndApi enabled switches the approach above to using type intersection - both with and without discriminator

[topce]

Hi @macjohnny @wing328 @amakhrov

It looks like for same time this is broken

before

import { Node } from './node';
import { Attachment } from './attachment';
import { ArchiveNodeAllOf } from './archiveNodeAllOf';

export interface ArchiveNode {
  id?: string;
  type?: string;
  service?: ArchiveNode.ServiceEnum;
  name?: string;
  /**
   * The object that is used to compile all the translations of a node into a JSON object. It is basically composed of a map with a key languaguage code and its value
   */
  title?: { [key: string]: string };
  /**
   * The object that is used to compile all the translations of a node into a JSON object. It is basically composed of a map with a key languaguage code and its value
   */
  description?: { [key: string]: string };
  properties?: { [key: string]: string };
  /**
   * 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.
   */
  permissions?: { [key: string]: string };
  attachments?: Array<Attachment>;
  parentId?: string;
  notifications?: string;
  /**
   * true if the current user faved the node
   */
  favourite?: boolean;
  hasSubFolders?: boolean;
  deletedBy?: string;
  deletedDate?: string;
}
export namespace ArchiveNode {
  export type ServiceEnum =
    | 'library'
    | 'information'
    | 'events'
    | 'newsgroups'
    | 'directory';
  export const ServiceEnum = {
    Library: 'library' as ServiceEnum,
    Information: 'information' as ServiceEnum,
    Events: 'events' as ServiceEnum,
    Newsgroups: 'newsgroups' as ServiceEnum,
    Directory: 'directory' as ServiceEnum
  };
}

that was relatively OK if we ignore not needed imports
now

import { Node } from './node';
import { Attachment } from './attachment';
import { ArchiveNodeAllOf } from './archiveNodeAllOf';

export interface ArchiveNode {
  deletedBy?: string;
  deletedDate?: string;
}
export namespace ArchiveNode {} 

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
after few years in master
Maybe to test changes with one yaml file
that cover all features of open api spec

in attempt to minimize stupid regerssions
so last comment of @amakhrov is not correct any more because it look like
generator is broken

• allOf without any discriminator: all properties from referenced schemas are cloned into the target interface.

[amakhrov]

@topce thanks for bringing this up. Indeed support of allOf across typescript generators is inconsistent and incomplete atm.

Maybe to test changes with one yaml file that cover all features of open api spec

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.

allOf without any discriminator: all properties from referenced schemas are cloned into the target interface.

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 discriminator) - and they seem to emit valid model with all fields included.

[topce]

@amakhrov
Thank you for fast response.
Can you please provide example that is working for you ?

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.
It was working before but not anymore there is some regression there.

[topce]

@amakhrov
I was testing on master branch .
4.2.3 was working in 4.3.0 is also 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.

[topce]

@amakhrov
no problem
not sure when this regression happened
or how to fix it?
Do you know if it works any better for other TypeScript generators ?

[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.
I'll try to get some time on the weekend and investigate what's causing this

[topce]

Thank you!

[amakhrov]

Look like this was the breaking change: https://github.com/OpenAPITools/openapi-generator/pull/5526/files#diff-57d7532cf464a8d7c24aab4b22ceb993R1138

Current effect is:

• for model (Child) that is composed as allOf on Parent and Model_AllOf, this if condition ensures Parent is treated as a parent in inheritance, even though it doesn't have a discriminator.

However, getAllParentsName() was not updated to follow the same logic, so parent and allParents are out of sync now (later should always be a superset of former). And typescript generators rely on allParents.
I'll make a PR to fix this

[digaus]

Currently having the same issue. Glad to see it is already being worked on.

[topce]

thank you @amakhrov it is working now for angular typescript
even it generates less code because class is extended
and not all properties are copied
@wing328 maybe to accept this PR it fix bug that was present for long time

[topce]

Hi @wing328 and @jimschubert
any news on this one @amakhrov fix one big problem
why it is not still not merged !?

[wing328]

@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

[topce]

@wing328 no problem, latest stable is working OK for me