fix: Correct handling of default parameter values in referenced component schema #1746

phk422 · 2024-07-05T06:48:41Z

fix: Correct handling of default parameter values in referenced component schema

Changes

Fix #1741

How to Review

Tests added; tests should pass

Checklist

Unit tests updated
docs/ updated (if necessary)
pnpm run update:examples run (only applicable for openapi-typescript)

…nent schema

changeset-bot · 2024-07-05T06:48:44Z

🦋 Changeset detected

Latest commit: dd618c6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openapi-typescript	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

phk422 · 2024-07-05T07:03:15Z

Ah, I usually use eslint + prettier for code styling, and I haven't used biome to enforce code style. There seems to be a bug with the biome plugin on my VSCode, causing multiple failures in code style checks.😭

duncanbeevers · 2024-07-05T12:44:35Z

I think this change is insufficient since components/schemas may still be referenced within request bodies.

Unless a field is explicitly marked required it should not need to be present in the request body; in such an instance the default value is only serving as a hint that a value may be supplied by the server.

Consider this simple schema where a single component schema is used as both the request body and as the response.

a buyer must specify a shirt color
a buyer may specify a shirt quantity

Since only a single type definition will be created for ShirtPurchase, quantity must remain an optional property, even though it will be present in the response body.

paths:
  /purchase:
    post:
      requestBody:
        required: true
        content:
          'application/json':
            schema:
              $ref: '#/components/schemas/ShirtPurchase'
      responses:
        '200':
          description: OK
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/ShirtPurchase'
components:
  schemas:
    ShirtPurchase:
      type: object
      properties:
        color:
          type: string
          enum:
            - blue
            - yellow
        quantity:
          type: number
          minimum: 1
          default: 1
      required:
        - color

drwpow · 2024-07-05T15:55:39Z

Ah, I usually use eslint + prettier for code styling, and I haven't used biome to enforce code style. There seems to be a bug with the biome plugin on my VSCode, causing multiple failures in code style checks.😭

Ah sorry—yeah that was kind of a silent change. But I’ve been blown away with the bugfixes Biome had in lots of ESLint packages, and the speed is hard to beat. Plus the team is really collaborative and I’m finding contributing to Biome easier than ESLint! But formatting-wise I’m neutral on Prettier vs Biome for formatting; that was more an experiment.

the .vscode/settings.json should be responsible for setting formatters, and .vscode/extensions.json should prompt you to install the correct extensions, but LMK if that’s not working as expected. I also added format commands to update formatting easily.

As I type all this out, this is probably all good info that I should put into the CONTRIBUTING.md guide 🙂

phk422 · 2024-07-06T02:25:02Z

Ah, I usually use eslint + prettier for code styling, and I haven't used biome to enforce code style. There seems to be a bug with the biome plugin on my VSCode, causing multiple failures in code style checks.😭

Ah sorry—yeah that was kind of a silent change. But I’ve been blown away with the bugfixes Biome had in lots of ESLint packages, and the speed is hard to beat. Plus the team is really collaborative and I’m finding contributing to Biome easier than ESLint! But formatting-wise I’m neutral on Prettier vs Biome for formatting; that was more an experiment.

the .vscode/settings.json should be responsible for setting formatters, and .vscode/extensions.json should prompt you to install the correct extensions, but LMK if that’s not working as expected. I also added format commands to update formatting easily.

As I type all this out, this is probably all good info that I should put into the CONTRIBUTING.md guide 🙂

Alright, I think I should learn more about Biome too!😊

phk422 · 2024-07-06T02:28:52Z

I think this change is insufficient since components/schemas may still be referenced within request bodies.

Unless a field is explicitly marked required it should not need to be present in the request body; in such an instance the default value is only serving as a hint that a value may be supplied by the server.

Consider this simple schema where a single component schema is used as both the request body and as the response.

a buyer must specify a shirt color

a buyer may specify a shirt quantity

Since only a single type definition will be created for ShirtPurchase, quantity must remain an optional property, even though it will be present in the response body.
paths:
  /purchase:
    post:
      requestBody:
        required: true
        content:
          'application/json':
            schema:
              $ref: '#/components/schemas/ShirtPurchase'
      responses:
        '200':
          description: OK
          content:
            'application/json':
              schema:
                $ref: '#/components/schemas/ShirtPurchase'
components:
  schemas:
    ShirtPurchase:
      type: object
      properties:
        color:
          type: string
          enum:
            - blue
            - yellow
        quantity:
          type: number
          minimum: 1
          default: 1
      required:
        - color

Oh! It seems I did overlook this situation. I'm curious, what approach do you have for handling this kind of situation? @duncanbeevers @drwpow

duncanbeevers · 2024-07-06T03:36:14Z

what approach do you have for handling this kind of situation?

My suggestion is to not use the presence of default to determine whether a property should be optional, and only rely on a property's presence in required.

drwpow · 2024-07-08T15:11:25Z

I'm curious, what approach do you have for handling this kind of situation? @duncanbeevers @drwpow

+1 to @duncanbeevers’ comment. For fixing this, I usually start by creating a failing test (TDD), and the one they supplied seems sufficient!

From there, tracing/resolving nodes in 7.x is one of the major internal reasons for rewriting! In previous versions it was much harder to peer into nodes. But in the current version, ctx.resolve($ref) lets you look up any $ref to see its contents.

It does add a little bit of work (especially considering nested $refs), but unless I’m misunderstanding you should only need that top-level object so I don’t think it should be complicated (i.e. you don’t have to deeply-traverse all trees from the requestBody; just peer into that top-level $ref).

phk422 · 2024-07-09T08:47:12Z

I noticed that @duncanbeevers' proposed approach conflicts with #1681, so maybe we should consider again here.@drwpow

duncanbeevers · 2024-07-10T17:08:57Z

I noticed that @duncanbeevers' proposed approach conflicts with #1681, so maybe we should consider again here.

That doesn't seem like a conflict. My proposal is to make these fields optional, which seems to align with what is happening in that PR as well. I'm happy to stand corrected if I'm misunderstanding.

phk422 · 2024-07-11T02:36:10Z

what approach do you have for handling this kind of situation?

My suggestion is to not use the presence of default to determine whether a property should be optional, and only rely on a property's presence in required.

I discovered that this default behavior can be controlled through defaultNonNullable. By setting it to false, the scenario mentioned in this PR can be accommodated. It seems designed this way to meet the needs of different users. Or do we need to handle the requestBody separately?If I am misunderstanding, please correct me.

duncanbeevers · 2024-07-12T02:18:58Z

@phk422 Thanks for calling out that defaultNonNullable option; I wasn't familiar with it.
Setting defaultNonNullable: false cleared up all the problems in our codebase around request bodies with default values.

I think that makes this PR the correct approach since it detects these default definitions in request bodies, and needn't concern itself with component schemas.

phk422 · 2024-07-16T05:55:02Z

Above, I think this pr can be merged,@drwpow, what do you think?

drwpow · 2024-07-16T20:51:54Z

Above, I think this pr can be merged,@drwpow, what do you think?

I think so! If you could add a patch changeset please (see Changeset comment above) I’d be happy to approve and merge! Thank you @phk422 and @duncanbeevers for discussing and digging into this further!

…nent schema (openapi-ts#1746) * fix: Correct handling of default parameter values in referenced component schema * chore: lint code * chore: lint code * Create dull-birds-pump.md

Nikoms · 2024-08-20T13:37:54Z

Hello, I still have the same issue with this OpenAPi spec: https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml (using openapi-typescript 7.3.0)

The field priority(look for the one with the description Specifies the priority of the job with 1 for high priority jobs and 2 for normal jobs.) has a default value, but, in TS, the value is required:

        priority:
          maximum: 2
          minimum: 1
          type: integer
          description: |
            Specifies the priority of the job with 1 for high priority jobs and 2 for normal jobs.
          example: 2
          default: 2

duncanbeevers · 2024-08-20T14:24:30Z

@Nikoms When I run the types-generator on this OpenAPI document i see the expected behavior.

Running with --default-non-nullable results in a non-optional priority: number
Running with no flags results in an optional priority?: number

$ npx openapi-typescript ./api.yaml | grep 'priority[?:]'
      priority?: number;
      priority?: number;
$ npx openapi-typescript ./api.yaml --default-non-nullable | grep 'priority[?:]'
      priority: number;
      priority: number;

Nikoms · 2024-08-21T09:49:55Z

Ok then, I must have a problem with my computer or something, because I have this:

npx openapi-typescript https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml | grep 'priority[?:]'
            priority: number;
            priority: number;

Note: I also tried with npx openapi-typescript@latest https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml | grep 'priority[?:]' just to make sure I'm on the last version

I'm on macos, using node 20.16.0 (also not working on node 22), npx 10.8.1

philios33 · 2024-09-07T00:35:15Z

I can confirm this is still a problem.

npx openapi-typescript https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml | grep 'priority[?:]'
            priority: number;
            priority: number;
npx openapi-typescript https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml --default-non-nullable | grep 'priority[?:]'
            priority: number;
            priority: number;
npx openapi-typescript https://docs-be.here.com/bundle/tour-planning-api-api-reference/page/api.yaml --default-non-nullable false | grep 'priority[?:]'
            priority?: number;
            priority?: number;

My guess is that the option (which should default to false) somehow gets overridden somewhere to be true once a $ref is followed, and then explicitly setting the flag to be false in the command seems to force it to remain false properly.

philios33 · 2024-09-07T01:26:57Z

This option is now defaulting to true in v7, sorry I didn't realise that. I'm guessing @duncanbeevers is on v6.

fix: Correct handling of default parameter values in referenced compo…

aa6f07e

…nent schema

phk422 requested a review from a team as a code owner July 5, 2024 06:48

phk422 added 2 commits July 5, 2024 14:52

chore: lint code

e92374c

chore: lint code

e671542

phk422 changed the title ~~fix: Correct handling of default parameter values in referenced compo…~~ fix: Correct handling of default parameter values in referenced component schema Jul 5, 2024

Merge branch 'main' into fix-requestBodies

bc8ef34

Create dull-birds-pump.md

dd618c6

drwpow approved these changes Jul 17, 2024

View reviewed changes

drwpow merged commit e705909 into openapi-ts:main Jul 17, 2024
7 checks passed

github-actions bot mentioned this pull request Jul 17, 2024

[ci] release #1770

Merged

phk422 deleted the fix-requestBodies branch July 18, 2024 03:18

phk422 mentioned this pull request Jul 23, 2024

Default values override "required" state in output types #1776

Closed

2 tasks

philios33 mentioned this pull request Sep 5, 2024

Schemas using $ref give different output to identical inlined spec when using defaults #1903

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Correct handling of default parameter values in referenced component schema #1746

fix: Correct handling of default parameter values in referenced component schema #1746

phk422 commented Jul 5, 2024 •

edited

Loading

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

phk422 commented Jul 5, 2024

duncanbeevers commented Jul 5, 2024 •

edited

Loading

drwpow commented Jul 5, 2024 •

edited

Loading

phk422 commented Jul 6, 2024

phk422 commented Jul 6, 2024

duncanbeevers commented Jul 6, 2024

drwpow commented Jul 8, 2024

phk422 commented Jul 9, 2024 •

edited

Loading

duncanbeevers commented Jul 10, 2024

phk422 commented Jul 11, 2024 •

edited

Loading

duncanbeevers commented Jul 12, 2024

phk422 commented Jul 16, 2024

drwpow commented Jul 16, 2024

Nikoms commented Aug 20, 2024 •

edited

Loading

duncanbeevers commented Aug 20, 2024 •

edited

Loading

Nikoms commented Aug 21, 2024 •

edited

Loading

philios33 commented Sep 7, 2024

philios33 commented Sep 7, 2024

fix: Correct handling of default parameter values in referenced component schema #1746

fix: Correct handling of default parameter values in referenced component schema #1746

Conversation

phk422 commented Jul 5, 2024 • edited Loading

Changes

How to Review

Checklist

changeset-bot bot commented Jul 5, 2024 • edited Loading

🦋 Changeset detected

phk422 commented Jul 5, 2024

duncanbeevers commented Jul 5, 2024 • edited Loading

drwpow commented Jul 5, 2024 • edited Loading

phk422 commented Jul 6, 2024

phk422 commented Jul 6, 2024

duncanbeevers commented Jul 6, 2024

drwpow commented Jul 8, 2024

phk422 commented Jul 9, 2024 • edited Loading

duncanbeevers commented Jul 10, 2024

phk422 commented Jul 11, 2024 • edited Loading

duncanbeevers commented Jul 12, 2024

phk422 commented Jul 16, 2024

drwpow commented Jul 16, 2024

Nikoms commented Aug 20, 2024 • edited Loading

duncanbeevers commented Aug 20, 2024 • edited Loading

Nikoms commented Aug 21, 2024 • edited Loading

philios33 commented Sep 7, 2024

philios33 commented Sep 7, 2024

phk422 commented Jul 5, 2024 •

edited

Loading

changeset-bot bot commented Jul 5, 2024 •

edited

Loading

duncanbeevers commented Jul 5, 2024 •

edited

Loading

drwpow commented Jul 5, 2024 •

edited

Loading

phk422 commented Jul 9, 2024 •

edited

Loading

phk422 commented Jul 11, 2024 •

edited

Loading

Nikoms commented Aug 20, 2024 •

edited

Loading

duncanbeevers commented Aug 20, 2024 •

edited

Loading

Nikoms commented Aug 21, 2024 •

edited

Loading