openapi-ts / openapi-typescript

Generate TypeScript types from OpenAPI 3 specs
https://openapi-ts.dev
MIT License
5.86k stars 467 forks source link

Circular reference when using inheritance & polymorhism with discriminator #1565

Closed clxandstuff closed 1 month ago

clxandstuff commented 8 months ago

Description

Not sure if this is a bug. I would like to confirm that types generation is possible with my schema

Typescript definitions are circular which results in "any" types.
Name Version
openapi-typescript 7.0.0 & 6.7.4
Node.js 18.18.1
OS + version macOS 13, Windows 11, etc.

Reproduction

Generate types with below schema.

openapi: 3.0.0
info:
  title: Test schema
  version: V1
paths:
  /resource:
    get:
      operationId: getResource
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/PetOwner"
components:
  schemas:
    PetOwner:
      type: object
      properties:
        pet:
          $ref: "#/components/schemas/Pet"
      required:
        - pet
    Pet:
      type: object
      properties:
        petType:
          allOf:
            - $ref: "#/components/schemas/PetType"
          readOnly: true
      discriminator:
        propertyName: petType
        mapping:
          Cat: "#/components/schemas/Cat"
          Dog: "#/components/schemas/Dog"
      oneOf:
        - $ref: "#/components/schemas/Cat"
        - $ref: "#/components/schemas/Dog"
      required:
        - petType
    Cat:
      allOf:
        - $ref: "#/components/schemas/Pet"
        - type: object
          properties:
            name:
              type: string
          required:
            - name
    Dog:
      allOf:
        - $ref: "#/components/schemas/Pet"
        - type: object
          properties:
            bark:
              type: string
          required:
            - bark
    PetType:
      type: string
      enum:
        - Cat
        - Dog

It results in:

export interface components {
  schemas: {
    PetOwner: {
      pet: components["schemas"]["Pet"];
    };
    Pet: {
      petType: components["schemas"]["PetType"];
    } & (components["schemas"]["Cat"] | components["schemas"]["Dog"]);
    Cat: {
      petType: "Cat";
    } & Omit<components["schemas"]["Pet"], "petType"> & {
      name: string;
    };
    Dog: {
      petType: "Dog";
    } & Omit<components["schemas"]["Pet"], "petType"> & {
      bark: string;
    };
    /** @enum {string} */
    PetType: "Cat" | "Dog";
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

Expected result

I expect Cat & Dog to not reference Pet

export interface components {
  schemas: {
    PetOwner: {
      pet: components["schemas"]["Pet"];
    };
    Pet: {
      petType: components["schemas"]["PetType"];
    } & (components["schemas"]["Cat"] | components["schemas"]["Dog"]);
    Cat: {
      petType: "Cat";
    } & {
      name: string;
    };
    Dog: {
      petType: "Dog";
    } & {
      bark: string;
    };
    /** @enum {string} */
    PetType: "Cat" | "Dog";
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

so I can use types like this

import { components } from "./types"

const a = {} as components["schemas"]["PetOwner"];

if (a.pet.petType === 'Cat') {
    a.pet.name
} else {
    a.pet.bark
}

Is this possible with the current version? If not I can try to fix this, if you guide me where to start.

Checklist

clxandstuff commented 8 months ago

I found one more problem.

When changing this:

PetOwner:
      type: object
      properties:
        pet:
          # $ref: "#/components/schemas/Pet" - changing to allOf
          allOf:
            - $ref: "#/components/schemas/Pet"

generator omits discriminator property "petType" and changes petType

export interface components {
  schemas: {
    PetOwner: {
      pet: {
        petType: "PetOwner"; // 
      } & Omit<components["schemas"]["Pet"], "petType">;
    };
    Pet: {
      petType: components["schemas"]["PetType"];
    } & (components["schemas"]["Cat"] | components["schemas"]["Dog"]);
    Cat: {
      petType: "Cat";
    } & Omit<components["schemas"]["Pet"], "petType"> & {
      name: string;
    };
    Dog: {
      petType: "Dog";
    } & Omit<components["schemas"]["Pet"], "petType"> & {
      bark: string;
    };
    /** @enum {string} */
    PetType: "Cat" | "Dog";
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}
drwpow commented 8 months ago

I think this is correct, and your code is circular. Since Cat and Dog have allOf referencing Pet, it’s expected they pull that into the type union. Further, you also declare oneOf in Pet, so that, too, will try and inject Cat and Dog into those types. Using both together is where I think the issue is.

Most discriminator code I’ve seen don’t use both together; it should probably be one or the other. Further they mean different things; allOf is not the logical inverse of oneOf, so you’ll also get errors trying to match these up.

Lastly, using oneOf in tandem with anything else is usually a type error; oneOf should mean “contains these properties and nothing else.” While there’s nothing that throws an explicit error when generating these types—there are ways to generate valid types using oneOf + other properties—it’s just very easy to create logical errors when using this (there’s even a helper section on this in the docs)

clxandstuff commented 8 months ago

@drwpow

Thanks for clarifying all the details. You are right that some parts of the example can cause problems.

I updated the example based on your comments and all looks good, except the case when a discriminator is added.

With this schema:

openapi: 3.0.0
info:
  title: Test schema
  version: V1
paths:
  /resource:
    get:
      operationId: getResource
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/PetOwner"
components:
  schemas:
    PetOwner:
      type: object
      properties:
        pet:
          allOf:
            - $ref: "#/components/schemas/Pet"
      required:
        - pet
    Pet:
      discriminator:
        propertyName: petType
        mapping:
          Cat: "#/components/schemas/Cat"
          Dog: "#/components/schemas/Dog"
      oneOf:
        - $ref: "#/components/schemas/Cat"
        - $ref: "#/components/schemas/Dog"
    PetCommon:
      type: object
      properties:
        petType:
          allOf:
            - $ref: "#/components/schemas/PetType"
      required:
        - petType
    Cat:
      allOf:
        - $ref: "#/components/schemas/PetCommon"
        - type: object
          properties:
            name:
              type: string
          required:
            - name
    Dog:
      allOf:
        - $ref: "#/components/schemas/PetCommon"
        - type: object
          properties:
            bark:
              type: string
          required:
            - bark
    PetType:
      type: string
      enum:
        - Cat
        - Dog

I'm still getting invalid polymorphic "pet" property.

export interface components {
  schemas: {
    PetOwner: {
      pet: {
        petType: "PetOwner";
      } & Omit<components["schemas"]["Pet"], "petType">;
    };
    Pet: components["schemas"]["Cat"] | components["schemas"]["Dog"];
    PetCommon: {
      petType: components["schemas"]["PetType"];
    };
    Cat: components["schemas"]["PetCommon"] & {
      name: string;
    };
    Dog: components["schemas"]["PetCommon"] & {
      bark: string;
    };
    /** @enum {string} */
    PetType: "Cat" | "Dog";
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

In both versions v6 & v7 Discriminator property "petType" is omitted, and additionally in v6 replaced with strange value of "PetOwner".

Do you know if I'm still messing with the schema structure?

mzronek commented 8 months ago

This will be fixed with this PR: https://github.com/drwpow/openapi-typescript/pull/1578

@drwpow

clxandstuff commented 8 months ago

@mzronek That's great! Thanks for taking care of this, and waiting for release :)

mzronek commented 6 months ago

@clxandstuff Have you tested it with the latest v7 version? Can we close this?

clxandstuff commented 6 months ago

Hi @mzronek

I'm gonna test it today and let you know

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

github-actions[bot] commented 1 month ago

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.