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

全てOptionalなプロパティを持つオブジェクトが正しく変換されない #132

Closed
1 of 3 tasks
SIY1121 opened this issue Mar 26, 2021 · 9 comments · Fixed by #179
Closed
1 of 3 tasks

Comments

@SIY1121
Copy link

SIY1121 commented Mar 26, 2021

Description

全てOptionalなプロパティを持つオブジェクトが、出力された型で全て必須になっています。

# openapi: 3.0.3

SampleObject:
  type: object
  properties:
    foo:
      type: string
    bar:
      type: integer

出力結果

export type SampleObject = {
  foo: string
  bar: number
}

期待される結果

export type SampleObject = {
  foo?: string
  bar?: number
}

仕様では全てのプロパティはデフォルトでOptionalです。
https://swagger.io/docs/specification/data-models/data-types/#ctxM:~:text=By%20default%2C%20all%20object%20properties%20are%20optional.

一応 required に空の配列を指定することで回避できると思われますが、上記仕様で空の配列は指定してはならないと明記されています。

Environment

  • Package version: v0.16.1
  • OS:
    • Linux
    • Windows
    • macOS
  • Node.js version:
    14.16.0
    ``
  • npm version:
    6.14.11
    ``

Additional context

原因箇所はここだと思われます。

required: obj.required?.includes(name) ?? true,

@solufa
Copy link
Member

solufa commented Mar 27, 2021

投稿ありがとうございます!
この仕様の存在は知りませんでした
現場の利便性を考えるとデフォルトで必須にする方がいいみたいなのです
ビルドオプションを追加するべきか悩みます

このIssueをきっかけに振る舞いを変更しました
#71

@SIY1121
Copy link
Author

SIY1121 commented Mar 28, 2021

なるほど。
ただ、OpenAPI3.0対応と書いてある以上、ユーザーは仕様通りの挙動を期待すると思います。
様々な現場のケースに対応するために、本来の仕様とは異なった挙動をするビルドオプションを追加するのはありだとは思うのですが、少なくともデフォルトで仕様外の挙動をしてしまうとユーザーは混乱してしまうと思います。
また、OpenAPIに正しく準拠した別のツールを併用することが難しくなってしまいます。

よって、デフォルトでは仕様に準拠した変換を行い、オプションで挙動を変更できるようにするべきだと思います。

@aidy1991
Copy link
Contributor

required を記述していない query parameter が optional にならずに調べていて、この issue にたどり着きました。

デフォルトでは仕様に準拠した変換を行い、オプションで挙動を変更できるようにするべきだと思います。

この挙動を支持します。この件に関して何か既に対応されていますでしょうか?もしくは予定はありますか?

@kondei
Copy link

kondei commented Jul 14, 2021

同じく…
困っているのでdefault falseに正しく対応することを望みます

@kondei
Copy link

kondei commented Jul 14, 2021

ワークアラウンドとしてはpatch-packageで以下を適用するとoptionalに変えることができました

diff --git a/node_modules/openapi2aspida/dist/builderUtils/converters.js b/node_modules/openapi2aspida/dist/builderUtils/converters.js
index 2ad20ac..c73b8f1 100644
--- a/node_modules/openapi2aspida/dist/builderUtils/converters.js
+++ b/node_modules/openapi2aspida/dist/builderUtils/converters.js
@@ -66,7 +66,7 @@ var object2value = function (obj) {
             return null;
         return {
             name: exports.getPropertyName(name),
-            required: (_b = (_a = obj.required) === null || _a === void 0 ? void 0 : _a.includes(name)) !== null && _b !== void 0 ? _b : true,
+            required: (_b = (_a = obj.required) === null || _a === void 0 ? void 0 : _a.includes(name)) !== null && _b !== void 0 ? _b : false,
             description: val.description,
             values: [val]
         };

@LumaKernel
Copy link
Contributor

LumaKernel commented Aug 18, 2021

#147
とコメントしたものを含めて入れることで対応できるかと思うのですがどうですか。

私も同様に optional にフォールバックするようにしたほうが良いと思います。オプションを用意するのも、亜種規格を作ることになっていいことにはならなさそう…とは思います…

EDIT: あ、対応自体はもうあるのか!

@kondei
Copy link

kondei commented Oct 19, 2021

@solufa
こちらデフォルトの挙動をOpenAPI仕様どおりに直す(または上記PRを取り込む)予定はないでしょうか

@LumaKernel
Copy link
Contributor

こちらのissueに関しては、OpenAPIの仕様通りの挙動にすることにしました。なお、従来の挙動は設定としても残さない方針です。
この変更で困るという場合は、こちらのissueやtwitter、メール(準備中、準備できれば更新します)でご連絡ください。

@aidy1991
Copy link
Contributor

aidy1991 commented Feb 17, 2023

@LumaKernel

この対応はすでにリリースされていますか?
v0.21.0 で以下のような schema を変換したところ、依然として正しく変換されていないようです。
もし何か認識が間違っていればご指摘いただければ幸いです。

Schema

# pet.yaml
swagger: '2.0'
info:
  version: 1.0.0
  title: Swagger Petstore
paths:
  /pets:
    get:
      responses:
        '200':
          description: A paged array of pets
          headers:
            x-next:
              type: string
              description: A link to the next page of responses
          schema:
            type: 'object'
            properties:
              id:
                type: integer
                format: int64
              name:
                type: string
              tag:
                type: string
        default:
          description: unexpected error
          schema:
            type: 'object'
            required:
              - code
            properties:
              code:
                type: integer
                format: int32

Generated type

$ npx openapi2aspida --version
v0.21.0
$ npx openapi2aspida -i pet.yaml
api/$api.ts was built successfully.
api/pets/$api.ts was built successfully.
/* eslint-disable */
export type Methods = {
  get: {
    status: 200

    /** A paged array of pets */
    resBody: {
      id: number
      name: string
      tag: string
    }

    resHeaders: {
      /** A link to the next page of responses */
      'x-next': string
    }
  }
}

Expected type

(required: [] をつけるとこうなります。

/* eslint-disable */
export type Methods = {
  get: {
    status: 200

    /** A paged array of pets */
    resBody: {
      id?: number | undefined
      name?: string | undefined
      tag?: string | undefined
    }

    resHeaders: {
      /** A link to the next page of responses */
      'x-next': string
    }
  }
}

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