Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] typescript-fetch since 7.4.0 ignores nullable: true #18288

Open
4 of 6 tasks
tfilo opened this issue Apr 4, 2024 · 7 comments
Open
4 of 6 tasks

[BUG] typescript-fetch since 7.4.0 ignores nullable: true #18288

tfilo opened this issue Apr 4, 2024 · 7 comments

Comments

@tfilo
Copy link

tfilo commented Apr 4, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

Hello since update to 7.4.0, nullable: true anotation is ignored and typescript generated interface doesn´t include "| null"

openapi-generator version

7.4.0

OpenAPI declaration file content or url
openapi: 3.0.1
info:
  title: TEST
  description: TEST
  version: 1.0.0
servers:
  - url: /
    description: Default Server URL
paths:
  /api/test:
    post:
      tags:
        - Test
      operationId: test
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/ResponseObject'
      responses:
        '200':
          description: OK
          content:
            '*/*':
              schema:
                $ref: '#/components/schemas/ResponseObject'

components:
  schemas:
    ResponseObject:
      type: object
      properties:
        code:
          type: integer
          nullable: true
Generation Details

I generate it using npm package: "@openapitools/openapi-generator-cli": "^2.13.1".
With command npx openapi-generator-cli generate with following configuration

{
    "$schema": "./node_modules/@openapitools/openapi-generator-cli/config.schema.json",
    "spaces": 4,
    "generator-cli": {
        "version": "7.4.0",
        "useDocker": true,
        "generators": {
            "v3.0": {
                "generatorName": "typescript-fetch",
                "glob": "openapi/*.{json,yaml}",
                "output": "src/openapi/#{name}",
                "typeMappings": {
                    "Date": "string"
                },
                "additionalProperties": {
                    "prefixParameterInterfaces": true,
                    "useSingleRequestParameter": false,
                    "supportsES6": true
                }
            }
        }
    }
}
Steps to reproduce

If you generate source code from this provided yaml with 7.4.0 you will get generated output where ResponseObject.ts is defined like this:

export interface ResponseObject {
    /**
     * 
     * @type {number}
     * @memberof ResponseObject
     */
    code?: number;
}

but with all previouse versions it was like this

export interface ResponseObject {
    /**
     * 
     * @type {number}
     * @memberof ResponseObject
     */
    code?: number | null;
}

It looks like version 7.4.0 started to ignore nullable: true anotation.

Related issues/PRs

Maybe related to this issue ? #18005

Suggest a fix

Output should look like this:

export interface ResponseObject {
    /**
     * 
     * @type {number}
     * @memberof ResponseObject
     */
    code?: number | null;
}
@wing328
Copy link
Member

wing328 commented Apr 5, 2024

i wonder if you can do a git bisect to identify the commit causing the regression?

@tfilo
Copy link
Author

tfilo commented Apr 16, 2024

I will try my best to find some time for that but not sure when.

@ghost
Copy link

ghost commented Apr 25, 2024

Heya, just to save you a bit of time: this issue is not related to #18005.

#18005 was created by the changes in #17934 which only changed a mustache file used for the aspnetcore generator.

@l0gicgate
Copy link
Contributor

l0gicgate commented May 3, 2024

I believe this PR broke it:
#17798

@matthiasschwarz
Copy link
Contributor

Was this change intended? This breaks our PATCH requests as we distinguish between undefined and null in the api.

@bthall16
Copy link

bthall16 commented May 24, 2024

@l0gicgate is correct that #17798 introduced this bug, specifically this change (and the others like it throughout the same PR).

To JavaScript and TypeScript, null and undefined are not equivalent. In a TypeScript object type, a ? marks a property as optional, meaning an object of that shape may not have the property and, consequently, accessing the property may return undefined.

If you'd like to denote a property as also being able to hold a null value, you have to explicitly annotate a type as such:

interface Foo {
  nullableField: string | null;
}

// Fine: can be `null`
const foo1: Foo = { nullableField: null };
// Error: cannot be `undefined`
const foo2: Foo = { nullableField: undefined };
// Error: field has to be present
const foo3: Foo = {};

Technically if a field is expected to be present but could hold an undefined value it would look like:

interface Foo {
  presentButMaybeUndefined: string | undefined;
}

// Fine: can be `undefined`
const foo1: Foo = { presentButMaybeUndefined: undefined };
// Error: field wasn't optional (`?`) so it has to be present
const foo2: Foo = {};

I think the most correct way to capture all possible use cases if a field is optional (in the OpenAPI sense) is to annotate it field with both ? and an explicit undefined:

interface Foo {
  couldBeUndefined?: string | undefined;
}

// Fine: field is optional
const foo1: Foo = {}
// Fine: field can explicitly hold `undefined`
const foo2: Foo = { couldBeUndefined: undefined };

However, if the types prior to #17798 were working then it's probably fine to just revert the change and avoid possibly causing new bugs by explicitly adding undefined to optional fields.


The correct behavior should be:

interface Foo {
  requiredNonNullable: string;
  requiredNullable: string | null;
  optionalNonNullable?: string;
  optionalNullable?: string | null;
}

@hendrikpeilke
Copy link

this issue is addressed here: #18887

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants