openapi-ts / openapi-typescript

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

required fields generated as optional #1467

Open nwheatle opened 9 months ago

nwheatle commented 9 months ago

Description

I generated types from https://benchling.com/api/v2/openapi.yaml file, and all of the required attributes are shown as optional in typescript.

Name Version
openapi-typescript 6.7.1
Node.js 20.10.0
OS + version Windows 11

Reproduction

I created a react app with typescript

$ npx create-react-app bugapp --template typescript

I added "noUncheckedIndexedAccess":true" to the react-generated tsconfig.json file, as suggested by the docs

I copy pasted the openapi specs from https://[benchling.com/api/v2/openapi.yaml] (https://benchling.com/api/v2/openapi.yaml) and pasted them into a file bugapp/openapi.yaml

I created a file bugapp/api.d.ts

then I ran command:

$ npx openapi-typescript ./openapi.yaml -o ./api.d.t

The resulting file incorrectly indicates required fields as optional.

One example:

//yaml AaSequenceBaseRequestForCreate: allOf:

// types ends up pointing to an object where these are not required.

aminoAcids?: string; name?: string

...real result below

    AaSequenceBaseRequest: {
      /** @description Aliases to add to the AA sequence */
      aliases?: string[];
      /** @description Amino acids for the AA sequence. */
      aminoAcids?: string;
      /** @description Annotations to create on the AA sequence. */
      annotations?: components["schemas"]["AaAnnotation"][];
      /** @description IDs of users to set as the AA sequence's authors. */
      authorIds?: string[];
      /** @description Custom fields to add to the AA sequence. Every field should have its name as a key, mapping to an object with information about the value of the field. */
      customFields?: components["schemas"]["CustomFields"];
      /** @description Fields to set on the AA sequence. Must correspond with the schema's field definitions. Every field should have its name as a key, mapping to an object with information about the value of the field. */
      fields?: components["schemas"]["Fields"];
      /** @description ID of the folder containing the AA sequence. */
      folderId?: string;
      /** @description Name of the AA sequence. */
      name?: string;
      /** @description ID of the AA sequence's schema. */
      schemaId?: string;
    };
    AaSequenceBaseRequestForCreate: components["schemas"]["AaSequenceBaseRequest"];
    AaSequenceBulkCreate: components["schemas"]["AaSequenceCreate"];
    AaSequenceBulkUpdate: {
      id?: string;
    } & components["schemas"]["AaSequenceBaseRequest"];
    AaSequenceCreate: components["schemas"]["AaSequenceBaseRequestForCreate"] & components["schemas"]["CreateEntityIntoRegistry"];

Expected result

I expected the name and aminoAcids attributes to be required. But it looks like they are optional because the types point to name?, and aminoAcids?

Checklist

davidleger95 commented 9 months ago

Hey there, I'm not sure if this is a bug. Please correct me if I'm misunderstanding.

Looking at the yaml provided:

AaSequenceBaseRequestForCreate:
allOf:
- $ref: '#/components/schemas/AaSequenceBaseRequest'
- required:
- aminoAcids
- name

I'm not sure if this is doing what you want it to.

It looks like you're trying to extend AaSequenceBaseRequest and mark a set of fields as required.

This might do the trick without using allOf:

# Assuming the schema you're referencing looks something like this:
AaSequenceBaseRequest:
  type: object
  properties:
    - name:
         type: string
    - folderId:
         type: string
    # ...

# You should be able to reference the schema without `allOf` like this:
AaSequenceBaseRequestForCreate:
  $ref: '#/components/schemas/AaSequenceBaseRequest'
  required:
    - aminoAcids
    - name

Hopefully this solves your problem!

Here's some further readings if you'd like a detailed explanation: https://redocly.com/docs/resources/all-of/

If not, could you please provide more of your yaml schema, including the definition of AaSequenceBaseRequest?

nwheatle commented 9 months ago

Thanks, this seems like it probably the cause. I didn't notice the required items were in the array.

Unfortunately, I am unable to test this because I keep getting a new error that it can't find my yaml file, even though it is actually there.

PS C:\Users\wheat\dev\bencopenapi\src>> npx openapi-typescript .\openapi.yaml -o .\t.d.ts
✨ openapi-typescript 6.7.1
 ✘  Could not find any specs matching ".\openapi.yaml". Please check that the path is correct.

but the file is there! I can't figure out what is wrong.

Anyway, we can close this as it does make a lot of sense what you are suggesting. I just can't verify on my end for literally no discernable reason that I can find.

Additional Info if you have time and desire:

file is there

PS C:\Users\wheat\dev\bencopenapi\src>> ls

    Directory: C:\Users\wheat\dev\bencopenapi\src

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a----        11/30/2023   5:55 PM            564 App.css
-a----        11/30/2023   5:55 PM            273 App.test.tsx
-a----        11/30/2023   5:55 PM            556 App.tsx
-a----        11/30/2023   5:55 PM            366 index.css
-a----        11/30/2023   5:56 PM            554 index.tsx
-a----        11/30/2023   5:55 PM           2632 logo.svg
-a----        11/30/2023   6:08 PM           4094 openapi.json
-a----        11/30/2023   6:05 PM           2652 openapi.yaml
-a----        11/30/2023   5:56 PM             41 react-app-env.d.ts
-a----        11/30/2023   5:55 PM            425 reportWebVitals.ts
-a----        11/30/2023   5:55 PM            241 setupTests.ts
-a----        11/30/2023   6:05 PM              0 t.d.ts

This is the openapi file I'm using from https://dzone.com/articles/a-sample-openapi-30-file-to-get-started

openapi: "3.0.0"
info:
  version: 1.0.0
  title: Swagger Petstore
  license:
    name: MIT
servers:
  - url: http://petstore.swagger.io/v1
paths:
  /pets:
    get:
      summary: List all pets
      operationId: listPets
      tags:
        - pets
      parameters:
        - name: limit
          in: query
          description: How many items to return at one time (max 100)
          required: false
          schema:
            type: integer
            format: int32
      responses:
        200:
          description: An paged array of pets
          headers:
            x-next:
              description: A link to the next page of responses
              schema:
                type: string
          content:
            application/json:    
              schema:
                $ref: "#/components/schemas/Pets"
        default:
          description: unexpected error
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Error"
    post:
      summary: Create a pet
      operationId: createPets
      tags:
        - pets
      responses:
        201:
          description: Null response
        default:
          description: unexpected error
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Error"
  /pets/{petId}:
    get:
      summary: Info for a specific pet
      operationId: showPetById
      tags:
        - pets
      parameters:
        - name: petId
          in: path
          required: true
          description: The id of the pet to retrieve
          schema:
            type: string
      responses:
        200:
          description: Expected response to a valid request
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Pets"
        default:
          description: unexpected error
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Error"
components:
  schemas:
    Pet:
      required:
        - id
        - name
      properties:
        id:
          type: integer
          format: int64
        name:
          type: string
        tag:
          type: string
    Pets:
      type: array
      items:
        $ref: "#/components/schemas/Pet"
    Error:
      required:
        - code
        - message
      properties:
        code:
          type: integer
          format: int32
        message:
          type: string
nwheatle commented 9 months ago

OK, I updated the npm version to openapi-typescript@7.0.0-next.5 and now the error is gone (the spec file is found)

I made the changes you suggested but, unfortunately, the fields are still optional in the generated types file.

    AaSequenceBaseRequestForCreate:
      $ref: '#/components/schemas/AaSequenceBaseRequest'
      required:
        - aminoAcids
        - name
drwpow commented 9 months ago

There’s currently not a way in openapi-typescript to use composition (anyOf/allOf/oneOf) to declare partial schema objects. The current requirement is you can’t really “overwrite” required like you’re trying to do because of how TypeScript works. Those get combined into unions/intersections individually, rather than flattened into one object (this is to preserve $refs so they work as-intended). There’s an unrelated bug here, but the discussion on the ability to declare partial schema objects touches on the same points: https://github.com/drwpow/openapi-typescript/issues/1441

There’s also not a way to combine $ref along with a partial schema either; it’s an either–or situation. Either it’s referring to a remote node, or it’s not.

This may be more a limitation of this library and TypeScript rather than OpenAPI as-designed. The requirement to declare full, complete schema objects is to produce deterministic types.

In your case, I’d recommend something like:

    AaSequenceBaseRequestForCreate:
      allOf:
        - $ref: '#/components/schemas/AaSequenceBaseRequest'
      type: object
      required:
        - aminoAcids
        - name
      properties:
        animoAcids:
          $ref: '#/components/schemas/AminoAcids'
        name:
          type: string

Note that you can always pull out individual properties into their own components for better reuse.

nwheatle commented 9 months ago

Thanks @drwpow

The response suggested by @davidleger95 eliminated composition (I removed the allOf), as shown below,

    AaSequenceBaseRequestForCreate:
      $ref: '#/components/schemas/AaSequenceBaseRequest'
      required:
        - aminoAcids
        - name

When you say "There’s also not a way to combine $ref along with a partial schema" - do you mean that I cannot modify a $ref schema with additional required fields as I did above?

In the transformSchemaObject function, it seems like when a $ref object is detected, it is passed as a string to oapiRef(), without passing the required fields onto this.

if ("$ref" in schemaObject) {
    return oapiRef(schemaObject.$ref);
  }

Is this why a $ref can't be modified?

I'm interested in contributing to the library to allow modification of required fields from referenced schemas, possibly including with oneOf/allOf/anyOf compositions, by returning WithRequired<> wrappers around referenced schemas. Are you interested?

imranbarbhuiya commented 9 months ago

Hi, I'm also having the same issue, in my case, the yaml has required: true but the body still shows it as optional. Check registerDto in https://gateway.alpha.theesports.club/docs-yaml

jonathan-rvezy commented 2 months ago

I've discovered the cause of this, at least for my use case. My openapi spec is based off 3.0.1, and it uses the "nullable", which this package seems to ignore as it's marked as deprecated because it's not a concept in the 3.1 spec (presumably, I didn't dig into it more)

This is the patch, which I didn't open a PR as there's probably other considerations.

diff --git a/packages/openapi-typescript/src/transform/schema-object.ts b/packages/openapi-typescript/src/transform/schema-object.ts
index 0885324..e4fceaf 100644
--- a/packages/openapi-typescript/src/transform/schema-object.ts
+++ b/packages/openapi-typescript/src/transform/schema-object.ts
@@ -458,6 +458,7 @@ function transformSchemaObjectCore(schemaObject: SchemaObject, options: Transfor
             options.ctx.defaultNonNullable &&
             !options.path?.includes("parameters") &&
             !options.path?.includes("requestBody")) // parameters can’t be required, even with defaults
+            || !("nullable" in v) || ("nullable" in v && v.nullable === false)
             ? undefined
             : QUESTION_TOKEN;
         let type =