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

required fields generated as optional #1467

Closed
1 task done
nwheatle opened this issue Nov 29, 2023 · 9 comments
Closed
1 task done

required fields generated as optional #1467

nwheatle opened this issue Nov 29, 2023 · 9 comments
Labels
bug Something isn't working openapi-ts Relevant to the openapi-typescript library stale

Comments

@nwheatle
Copy link

nwheatle commented Nov 29, 2023

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:
- $ref: '#/components/schemas/AaSequenceBaseRequest'
- required:
- aminoAcids
- name

// 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

@nwheatle nwheatle added bug Something isn't working openapi-ts Relevant to the openapi-typescript library labels Nov 29, 2023
@davejsdev
Copy link
Contributor

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
Copy link
Author

nwheatle commented Dec 1, 2023

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: https://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
Copy link
Author

nwheatle commented Dec 1, 2023

OK, I updated the npm version to [email protected] 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
Copy link
Contributor

drwpow commented Dec 1, 2023

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: #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
Copy link
Author

nwheatle commented Dec 4, 2023

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

This comment was marked as resolved.

@jonathan-rvezy
Copy link

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 =

Copy link
Contributor

github-actions bot commented Oct 3, 2024

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

@github-actions github-actions bot added the stale label Oct 3, 2024
Copy link
Contributor

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi-ts Relevant to the openapi-typescript library stale
Projects
None yet
Development

No branches or pull requests

5 participants