orval-labs / orval

orval is able to generate client with appropriate type-signatures (TypeScript) from any valid OpenAPI v3 or Swagger v2 specification, either in yaml or json formats. 🍺
https://orval.dev
MIT License
2.94k stars 326 forks source link

Discriminators are not taken into consideration when inlined in response #1316

Open AffeJonsson opened 6 months ago

AffeJonsson commented 6 months ago

What are the steps to reproduce this issue?

Using the following schema:

paths:
  /api/sample:
    get:
      summary: sample
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                oneOf:
                  - $ref: '#/components/schemas/TypeObj1'
                  - $ref: '#/components/schemas/TypeObj2'
                discriminator:
                  propertyName: type
                  mapping:
                    Type1: '#/components/schemas/TypeObj1'
                    Type2: '#/components/schemas/TypeObj2'
components:
  schemas:
    Type:
      enum:
        - Type1
        - Type2
      type: string
    Base:
      type: object
      required:
        - type
      properties:
        type:
          allOf:
            - $ref: '#/components/schemas/Type'
    TypeObj1:
      type: object
      allOf:
        - $ref: '#/components/schemas/Base'
      required:
        - type
      properties:
        moreProp:
          type: string
    TypeObj2:
      type: object
      allOf:
        - $ref: '#/components/schemas/Base'
      required:
        - type
      properties:
        prop:
          type: string

the discriminator is not handled properly.

What happens?

The generated types are

export const Type = {
  Type1: 'Type1',
  Type2: 'Type2',
} as const;
export interface Base {
  type: Type;
}
export type Type1 = Base & {
  moreProp?: string;
};
export type Type2 = Base & {
  prop?: string;
};

What were you expecting to happen?

The generated types should be

export const Type = {
  Type1: 'Type1',
  Type2: 'Type2',
} as const;
export const TypeObj1Type = {
  Type1: 'Type1',
} as const;
export const TypeObj2Type = {
  Type2: 'Type2',
} as const;
export interface Base {
  type: Type;
}
export type TypeObj1 = Base & {
  moreProp?: string;
  type: TypeObj1Type;
};
export type TypeObj2 = Base & {
  prop?: string;
  type: TypeObj2Type;
};

Any other comments?

Moving the response into the components schema definition and referencing it from the response solves the issue:

paths:
  /api/sample:
    get:
      summary: sample
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Resp'
components:
  schemas:
    Resp:
      oneOf:
        - $ref: '#/components/schemas/TypeObj1'
        - $ref: '#/components/schemas/TypeObj2'
      discriminator:
        propertyName: type
        mapping:
          Type1: '#/components/schemas/TypeObj1'
          Type2: '#/components/schemas/TypeObj2'
    Type:
      enum:
        - Type1
        - Type2
      type: string
    Base:
      type: object
      required:
        - type
      properties:
        type:
          allOf:
            - $ref: '#/components/schemas/Type'
    TypeObj1:
      type: object
      allOf:
        - $ref: '#/components/schemas/Base'
      required:
        - type
      properties:
        moreProp:
          type: string
    TypeObj2:
      type: object
      allOf:
        - $ref: '#/components/schemas/Base'
      required:
        - type
      properties:
        prop:
          type: string

What versions are you using?

Package Version: 6.27.1

AffeJonsson commented 5 months ago

Putting the discriminator on the base class also works. But having the discriminator on the response is valid according to the spec: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

melloware commented 5 months ago

PR is welcome?

AffeJonsson commented 5 months ago

I realize there is two ways this could be solved, but I'm not sure which is the way to go. Either we add the discriminated property to the types, or we add it to the combined response type. That is, either:

export type TypeObj1 = Base & {
  moreProp?: string;
  type: TypeObj1Type;
};
export type TypeObj2 = Base & {
  prop?: string;
  type: TypeObj2Type;
};

or

export type GetApiSample200 = (TypeObj1 & { type: TypeObj1Type }) | (TypeObj2 & { type: TypeObj2Type });

I assume the first one is the wanted one, but I'm unsure if there could be implications with changing the component schema based on the response schema. Could there be two responses which return the same objects, but discriminate on different properties? If so, option 2 is the safe option.