openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[Typescript] Generate oneOf schemas as type unions

Open ksvirkou-hubspot opened this issue 1 year ago • 4 comments

Description of the PR

I have implemented oneOf schemas as a type union and a oneOf class with a discriminator and mapping, similar to PR 2617 and PR 2647, but for the TypeScript generator.

The type union is used for methods like request/response types and for models as a type of parameters. The oneOf class is used for the deserialization process to determine the type of an object from the type union. It checks the discriminator in the mapping list, then in the list of all classes.

Before this PR, the TypeScript generator created a new model with all fields from all oneOf models. The created model is not described in the swagger files. There are the following problems:

  • It doesn't work if oneOf models have fields with the same name but different types.
  • It doesn't work if oneOf models have enum fields with the same name but different values in the enum.
  • It doesn't work if oneOf has a mapping list.
  • If oneOf models contain required fields, a user has to set all required fields, even if they are not needed for the request. example: Dog's required fields: https://github.com/OpenAPITools/openapi-generator/blob/daf52229e3b0e60ac5e41052f053cb4eba60d2f0/samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/Dog.ts#L15-L17 Cat's required fields: https://github.com/OpenAPITools/openapi-generator/blob/daf52229e3b0e60ac5e41052f053cb4eba60d2f0/samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/Cat.ts#L15-L17 oneOf model required fields : https://github.com/OpenAPITools/openapi-generator/blob/daf52229e3b0e60ac5e41052f053cb4eba60d2f0/samples/openapi3/client/petstore/typescript/builds/composed-schemas/models/PetsPatchRequest.ts#L17-L21 if we sent Dog model we have to set all required fields to PetsPatchRequest(even cat's ones)
  • It doesn't look like oneOf. If generate docs by the swagger with "oneOf", it won’t match the generated docs.

The implementation "oneOf" schemas as a type union resolve all these problems, making the generated code accurate with the swagger specification.

Sample input:

"inputFieldDependencies" : {
    "type" : "array",
    "items" : {
        "oneOf" : [ {
            "$ref" : "#/components/schemas/PublicSingleFieldDependency"
        }, {
            "$ref" : "#/components/schemas/PublicConditionalSingleFieldDependency"
        } ],
        "discriminator": {
            "propertyName": "dependencyType",
            "mapping": {
                "SINGLE_FIELD": "#/components/schemas/PublicSingleFieldDependency",
                "CONDITIONAL_SINGLE_FIELD": "#/components/schemas/PublicConditionalSingleFieldDependency"
            }
        }
    }
},

Sample output:

import { PublicConditionalSingleFieldDependency } from '../models/PublicConditionalSingleFieldDependency';
import { PublicSingleFieldDependency } from '../models/PublicSingleFieldDependency';

export type PublicActionDefinitionInputFieldDependenciesInner = PublicConditionalSingleFieldDependency | PublicSingleFieldDependency;

export class PublicActionDefinitionInputFieldDependenciesInnerClass {
    static readonly discriminator: string | undefined = "dependencyType";

    static readonly mapping: {[index: string]: string} | undefined = {
        "CONDITIONAL_SINGLE_FIELD": "PublicConditionalSingleFieldDependency",
        "SINGLE_FIELD": "PublicSingleFieldDependency",
    };
}

PR checklist

  • [X] Read the contribution guidelines.
  • [X] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [X] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
  • [X] File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • [X] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

Related issues:

https://github.com/OpenAPITools/openapi-generator/issues/9305

ksvirkou-hubspot avatar Jun 27 '24 09:06 ksvirkou-hubspot

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @topce @akehir @petejohansonxo @amakhrov @davidgamero @mkusaka

ksvirkou-hubspot avatar Jun 27 '24 09:06 ksvirkou-hubspot

Generate oneOf schemas as type unions

@ksvirkou-hubspot given that this implementation is for oneOf, will this implementation throw an error if the JSON payload for example matches both models/schemes (e.g. Pet, Animal) defined in oneOf?

wing328 avatar Jul 10 '24 14:07 wing328

Generate oneOf schemas as type unions

@ksvirkou-hubspot given that this implementation is for oneOf, will this implementation throw an error if the JSON payload for example matches both models/schemes (e.g. Pet, Animal) defined in oneOf?

Let me clarify the way ... should work. Discriminator is a field by which the generated code can determine what model is in the JSON payload. Example of swagger:

oneOf:
  - $ref: '#/components/schemas/Cat'
  - $ref: '#/components/schemas/Dog'
  - $ref: '#/components/schemas/Lizard'
  discriminator:
    propertyName: petType

Example of payload:

{
  "id": 123,
  "petType": "Cat",
}

In this case : Cat model. Considering what I've described, I don't think I fully understand the scenario you've meant. Could you share the swagger and the json payload to reproduce the case?

ksvirkou-hubspot avatar Jul 25 '24 10:07 ksvirkou-hubspot

Could you check it, please? @macjohnny @wing328

ksvirkou-hubspot avatar Jul 25 '24 11:07 ksvirkou-hubspot

I ran into a similar issue in https://github.com/planet-a-ventures/affinity-node/pull/35 and was passed on this PR by @macjohnny.

I tried regenerating with the changes in this PR and it changes a broken implementation of https://github.com/planet-a-ventures/affinity-node/blob/32fd4f2deea3cb7723fca17353bb35d6b2d19bda/openapi/2024-08-28.json

into a working one with correct discrimination of oneOfs.

Here is the runtime output of the code generated from current master:

ListEntryWithEntityPaged {
  data: [
    ListEntryWithEntity {
      type: "company",
      id: 167627852,
      createdAt: 2024-08-30T10:37:56.000Z,
      creatorId: 54576635,
      entity: Person {
        id: 297186243,
        firstName: undefined,
        lastName: undefined,
        primaryEmailAddress: undefined,
        emailAddresses: undefined,
        type: undefined,
        fields: [ [Field], [Field], [Field], [Field], [Field] ]
      }
    }
  ],
  pagination: Pagination { prevUrl: null, nextUrl: null }
}

please note how the entity has been deserialized into a class Person, albeit the discriminator (type) clearly states it has to be of type "company".

With the changes in this PR, this becomes:

ListEntryWithEntityPaged {
  data: [
    CompanyListEntry {
      id: 167627852,
      type: "company",
      createdAt: 2024-08-30T10:37:56.000Z,
      creatorId: 54576635,
      entity: Company {
        id: 297186243,
        name: "joschas testdealflow company",
        domain: "joscha.io",
        domains: [ "joscha.io" ],
        isGlobal: false,
        fields: [ [Field], [Field], [Field], [Field], [Field] ]
      }
    }
  ],
  pagination: Pagination { prevUrl: null, nextUrl: null }
}

which is correct (see how the entity is correctly of class Company).

I also noticed that this PR does make the changes in https://github.com/OpenAPITools/openapi-generator/pull/19481 superfluous. E.g. if this PR is merged, https://github.com/OpenAPITools/openapi-generator/pull/19481 (4238f17322bdb0e968f94b621669b737395a3dc9) can be reverted.

joscha avatar Aug 30 '24 13:08 joscha

I created https://github.com/OpenAPITools/openapi-generator/pull/19494 that has the necessary revert, upstream changes from master and regeneration of fixtures that have recently been added since this PR was opened.

joscha avatar Aug 30 '24 13:08 joscha

This pull request either needs all commits from https://github.com/OpenAPITools/openapi-generator/pull/19494 or https://github.com/OpenAPITools/openapi-generator/pull/19494 can be merged and it will automatically close this one as well, as it contains all the commits. I tried opening #19494 directly against your fork @ksvirkou-hubspot, however because of the commits that have made it to master in the last month since this PR was last rebased, there are dozens of commits in the diff, so I opted to instead create it against origin.

@macjohnny I can confirm that the generated code in this PR fixes the issue discussed before, both from a semantic perspective when looking at the code as well as in runtime (tried it against the live API, see above), so we should try to get this PR in as soon as possible, if we can, I think it will help anyone with swagger definitions that contain oneOfs.

joscha avatar Aug 30 '24 13:08 joscha

@ksvirkou-hubspot thanks for the fix! @joscha thanks for investigating and preparing the final PR. all merged.

macjohnny avatar Aug 30 '24 14:08 macjohnny

@macjohnny I just noticed that this change seems to generate nullable enums with an extra 'null' string value.

E.g:

          "enrichmentSource": {
            "description": "The source of the data in this Field (if it is enriched)",
            "enum": [
              "affinity-data",
              "dealroom",
              null
            ],
            "example": "affinity-data",
            "type": "string",
            "nullable": true
          },

becomes

export enum FieldEnrichmentSourceEnum {
    AffinityData = 'affinity-data',
    Dealroom = 'dealroom',
    Null = 'null'
}

which is incorrect as 'null' as a string is not a valid enum value. I will try and produce a fix for this.

joscha avatar Sep 06 '24 10:09 joscha

thanks for pointing this out and fixing!

macjohnny avatar Sep 06 '24 11:09 macjohnny

thanks for pointing this out and fixing!

fix is in https://github.com/OpenAPITools/openapi-generator/pull/19540

joscha avatar Sep 06 '24 13:09 joscha