[BUG][Typescript-Fetch] Incorrect toJSON & fromJSONTyped functions when using oneOf

jamesopti commented 4 years ago

Bug Report Checklist

[x] Have you provided a full/minimal spec to reproduce the issue?
[X] Have you validated the input using an OpenAPI validator (example)?
[X] What's the version of OpenAPI Generator used?
[X] Have you search for related issues/PRs?
[X] What's the actual output vs expected output?

Description

For a property defined using oneOf, invalid methods are generated for <Model>FromJSONTyped and for <Model>ToJSON:

See the nextUrl field below:

/**
 * 
 * @export
 * @interface Todos
 */
export interface Todos {
    /**
     * List of links to next pages of results in a series.   The first element in the array is the exact next page after the current record, etc. 
     * @type {string | UrlList}
     * @memberof Todos
     */
    nextUrl?: string | UrlList;
}

export function TodosFromJSON(json: any): Todos {
    return TodosFromJSONTyped(json, false);
}

export function TodosFromJSONTyped(json: any, ignoreDiscriminator: boolean): Todos {
    if ((json === undefined) || (json === null)) {
        return json;
    }
    return {
        // THIS IS INVALID TYPESCRIPT
        'nextUrl': !exists(json, 'next_url') ? undefined : string | UrlListFromJSON(json['next_url']),
    };
}

export function TodosToJSON(value?: Todos | null): any {
    if (value === undefined) {
        return undefined;
    }
    if (value === null) {
        return null;
    }
    return {
        // THIS IS INVALID TYPESCRIPT
        'next_url': string | UrlListToJSON(value.nextUrl),
    };
}

Expected

For both of those fields, I would expect a typeof check to decide what to do:

'next_url': typeof value.nextUrl === 'string' ? value.nextUrl : UrlListToJSON(value.nextUrl),

openapi-generator version

4.2.3 via

yarn list v1.21.0
└─ @openapitools/openapi-generator-cli@1.0.10-4.2.3

OpenAPI declaration file content or url

openapi: 3.0.0
info:
  title: Todo API
  description: A reference API description for a Todo service
  version: 1.0.0
servers:
  - url: http://localhost:8000/v1
    description: Generic HTTP
paths:
  /todos:
    get:
      summary: List the available todo tasks
      operationId: ListTodos
      responses:
        '200':
          description: a list of Todos
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Todos'
components:
  schemas:
    urlValue:
      type: string
      description: A single url.
    urlList:
      type: array
      description: A list of urls.
      items:
        type: string
    Todos:
      type: object
      properties:
        next_url:
          description: |
            List of links to next pages of results in a series.

             The first element in the array is the exact next page after the current record, etc.
          oneOf:
            - $ref: '#/components/schemas/urlValue'
            - $ref: '#/components/schemas/urlList'

Command line used for generation

./node_modules/@openapitools/openapi-generator-cli/bin/openapi-generator generate -g typescript-fetch -i ./openapi.yaml -o .

CONFIG: - "generateAliasAsModel": true

If generateAliasAsModel is false (default) then the generated Todos model is also incorrect:

export interface Todos {
    /**
     * List of links to next pages of results in a series.   The first element in the array is the exact next page after the current record, etc. 
     * @type {string | Array}
     * @memberof Todos
     */
    nextUrl?: string | Array;  // THIS IS INVALID TYPESCRIPT
}

auto-labeler[bot] commented 4 years ago

👍 Thanks for opening this issue! 🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

fantapop commented 4 years ago

@eriktim any thought on whats going wrong here?

eriktim commented 4 years ago

@jamesopti @fantapop the oneOf construct is far from mature here. The following changes are needed (even though the specification file is not incorrect):

The oneOf cannot be nested, so Todos should not be an object with a field but just the oneOf;
urlValue and urlList must be objects with in this case only one field.

fantapop commented 4 years ago

Thank you for the hints here. Looking closer at the open api spec, I see that the generated typescript is more like anyOf. It looks like oneOf is intended to be an exclusive Or. anyOf, however, seems to fail in the same way.

eriktim commented 4 years ago

@fantapop You are right. Though anyOf would also allow a mixture of types (e.g. two fields from type A and one field from type B). I personally favor the use case of defining union/sum types. To fully support the exclusivity, I guess the generator should apply strict de-serialisation first.

fantapop commented 4 years ago

I recently had a similar conversation with one of the maintainers of the https://github.com/lukeautry/tsoa project which generates an open api spec from typescript definitions. They are now using anyOf as their implementation of union which I think makes sense. It's very important to me that the generated typescript types from the open api spec are compatible with the input types that generate them in the tsoa project. Some of the nuances of these types are a little over my head. @eriktim Do you agree that anyOf would translate best to a straight typescript union? I was thinking oneOf could be something like what's arrived at here: https://timhwang21.gitbook.io/index/programming/typescript/xor-type . Although, I'm not really seeing how TSOA would translate something like a oneOf from their end unless some more prescriptive type or syntax was released by the typescript project or if unless there was a documented type name that could be used for that specific purpose. Regardless, it feels like getting the union definition working is a very important step in bringing this template along. I wonder if the true nature of this bug is really, how should unions work vs how should oneOf be translated.

fantapop commented 4 years ago

I've been tinkering around with this for a couple of evenings now. I was able to get something working for top level anyOf/oneOf types but there is not enough data provided by the core to build out the proper support for inline types. That happens to be most of the use case for our current spec. It looks like this outstanding PR get's most of the way if not all of the way there. https://github.com/OpenAPITools/openapi-generator/pull/3162

AntunVukicevic commented 3 years ago

Hey 👋🏻
Any new about this one? Atm open-api-generator is producing invalid Typescript for toJSON & fromJSON functions when oneOf is used with enums 😞

Example code:

...
components:
  schemas:
    TopLvlEnum:
      oneOf:
        - $ref: "#/components/schemas/EnumA"
        - $ref: "#/components/schemas/EnumB"
        - $ref: "#/components/schemas/EnumC"
    EnumA:
      type: string
      enum:
        - optionA1
        - optionA2
    EnumB:
      type: string
      enum:
        - optionB1
        - optionB2
    EnumC:
      type: string
      enum:
        - optionC1
        - optionC2
...

fantapop commented 3 years ago

@AntunVukicevic It's not great but I utilize the post processing feature of open api generator cli to programmatically fix errors. Here is a copy of the node script I use for this purpose.

#!/usr/local/bin/node

const fs = require('fs').promises;
const path = require('path');

function escapeRegex(string) {
    return string.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');
}

// prettier-ignore
const replacementsByFile = {
    'models/MediaCollection.ts': [
      {
        source: "MediaSizeName | MediaNameFromJSON(json['media-key'])",
        replacement: "json['media-key'] as MediaSizeName | MediaName",
      },
      {
        source: "MediaSizeName | MediaNameToJSON(value.media_key)",
        replacement: 'value.media_key',
      },
    ],
    'models/JobTemplateAttributes.ts': [
      {
        source: ".map(number | stringFromJSON)",
        replacement: " as Array<number | string>",
      },
      {
        source: ".map(number | stringToJSON)",
        replacement: "",
      },
    ]
};

async function main() {
    const [, , filePath] = process.argv;
    const relativePath = path.relative(
        path.resolve(`${__dirname}/../src/generated/client/src/`),
        filePath,
    );
    const fileReplacements = replacementsByFile[relativePath] || [];
    if (fileReplacements.length) {
        let data = await fs.readFile(filePath);
        for (const { source, replacement } of fileReplacements) {
            const sourceRegex = new RegExp(escapeRegex(source), 'g');
            data = data.toString().replace(sourceRegex, replacement);
        }
        await fs.writeFile(filePath, data);
    }
}

main().then(() => process.exit(0)).catch(console.error);

It's triggered by using the TS_POST_PROCESS_FILE environment variable. Here is the package.json script entry I'm using.

        "client:gen": "rm -rf ./src/generated/client && TS_POST_PROCESS_FILE='bin/postProcess.js' yarn openapi-generator-cli generate && cd ./src/generated/client/ && yarn",

tusharchutani commented 3 years ago

@fantapop is there any progress on this? I am having similar issues.

prince-chrismc commented 3 years ago

I opened a PR trying to unblock this, it at least gives us a workaround... @eriktim @jamesopti @fantapop maybe you can take a look at #10017?

OpenAPITools / openapi-generator