openapi-ts / openapi-typescript

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

discriminator mapping ignored when using allOf with oneOf #1690

Open CalebJamesStevens opened 1 month ago

CalebJamesStevens commented 1 month ago

Description

Here's a very basic example

openapi: 3.0.0

components:
  schemas:
    A:
      oneOf:
        - $ref: '#/components/schemas/C'
        - $ref: '#/components/schemas/D'
      discriminator:
        propertyName: pet_type
        mapping:
          c: '#/components/schemas/C'
          d: '#/components/schemas/D'
    B:
      allOf:
        - $ref: '#/components/schemas/A'
    C:
      type: object
      properties:
        pet_type:
          type: string
          enum: [c]
    D:
      type: object
      properties:
        pet_type:
          type: string
          enum: [d]
/**
 * This file was auto-generated by openapi-typescript.
 * Do not make direct changes to the file.
 */

export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
    schemas: {
        A: components["schemas"]["C"] | components["schemas"]["D"];
        B: {
            pet_type: "B";
        } & Omit<components["schemas"]["A"], "pet_type">;
        C: {
            /**
             * @description discriminator enum property added by openapi-typescript
             * @enum {string}
             */
            pet_type: "c";
        };
        D: {
            /**
             * @description discriminator enum property added by openapi-typescript
             * @enum {string}
             */
            pet_type: "d";
        };
    };
    responses: never;
    parameters: never;
    requestBodies: never;
    headers: never;
    pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;

As you can see we should be able to use this syntax to easily cut down the fluff and add more properties to B while maintaining the discriminators mapping. Instead the pet_type is omitted and a new pet_type is created overwriting C and D casing A to be completely invalid.

Name Version
openapi-typescript openapi-typescript@next
Node.js 18.16.0
OS + version macOS 13

Reproduction

How can this be reproduced / when did the error occur?

npx openapi-typescript@next test.yaml -o ./testnext.d.ts

Expected result something like

/**
 * This file was auto-generated by openapi-typescript.
 * Do not make direct changes to the file.
 */

export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
    schemas: {
        A: components["schemas"]["C"] | components["schemas"]["D"];
        B: components["schemas"]["A"];
        C: {
            /**
             * @description discriminator enum property added by openapi-typescript
             * @enum {string}
             */
            pet_type: "c";
        };
        D: {
            /**
             * @description discriminator enum property added by openapi-typescript
             * @enum {string}
             */
            pet_type: "d";
        };
    };
    responses: never;
    parameters: never;
    requestBodies: never;
    headers: never;
    pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;

(in case it’s not obvious)

Checklist

drwpow-figma commented 1 month ago

This is actually a common misconception about how discriminator works—the mapping is optional and it’s actually designed to automatically infer to the propertyName without the mapping. See a more detailed explanation. This was designed as a timesaver to prevent people from having to manually map too many types; the mapping is just there in case the inference is wrong. So it’s actually working as-intended to infer B has pet_type: "B".

CalebJamesStevens commented 1 month ago

This is actually a common misconception about how discriminator works—the mapping is optional and it’s actually designed to automatically infer to the propertyName without the mapping. See a more detailed explanation. This was designed as a timesaver to prevent people from having to manually map too many types; the mapping is just there in case the inference is wrong. So it’s actually working as-intended to infer B has pet_type: "B".

I'm not sure why this would a behavior that we want. The property that maps the reference in this example is pet_type which exists on C & D. Having this behavior essentially makes it impossible to use the discriminator properly because it will always be overwritten to have pet_type as the value B.

drwpow commented 1 month ago

Oh I’m fully in agreement with you—I also would prefer the mapping be manual. But this is how the JSONSchema (and by extension, OpenAPI 3) specification is designed to work, at least going through the examples. They explicitly call out this behavior. If you reference any object where the discriminator.propertyName is used, it will infer its value. This is present in the specification itself.

CalebJamesStevens commented 1 month ago

Oh I’m fully in agreement with you—I also would prefer the mapping be manual. But this is how the JSONSchema (and by extension, OpenAPI 3) specification is designed to work, at least going through the examples. They explicitly call out this behavior. If you reference any object where the discriminator.propertyName is used, it will infer its value. This is present in the specification itself.

I'm not sure that this isn't a bug still. With our schema uploaded to swagger the schemas are being shown as expected, its only when using this typescript generation tool that we are seeing the bug. And it's only when using it with allOf which is necessary to do something like so:

openapi: 3.0.0

components:
  schemas:
    A:
      oneOf:
        - $ref: '#/components/schemas/C'
        - $ref: '#/components/schemas/D'
      discriminator:
        propertyName: pet_type
        mapping:
          c: '#/components/schemas/C'
          d: '#/components/schemas/D'
    B:
      allOf:
        - $ref: '#/components/schemas/A'
      properties: 
        prop:
          type: string
    C:
      type: object
      properties:
        pet_type:
          type: string
          enum: [c]
    D:
      type: object
      properties:
        pet_type:
          type: string
          enum: [d]
CalebJamesStevens commented 1 month ago

@drwpow-figma This is a work around that i came up with that works for the TS generator, need to validate if it works for swagger

openapi: 3.0.3
info:
  title: Sample API
  version: 1.0.0
paths: {}
components:
  schemas:
    B:
      type: object
      properties:
        prop:
          type: string
        type:
          type: string
          enum: [b]
    C:
      type: object
      properties:
        otherProp:
          type: string
        type:
          type: string
          enum: [c]
    TypeADiscriminator:
      oneOf:
      - $ref: '#/components/schemas/B'
      - $ref: '#/components/schemas/C'
      discriminator:
        propertyName: type
        mapping:
          b: '#/components/schemas/B'
          c: '#/components/schemas/C'
    TypeA:
      $ref: '#/components/schemas/TypeADiscriminator'
    ExtendedTypeA:
      allOf:
        - $ref: '#/components/schemas/TypeA'
        - type: object
          properties:
            name:
              type: string