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

Pydantic v2やSkipJsonSchema周りを整理する #1458

Open
Hiroshiba opened this issue Aug 17, 2024 · 3 comments · May be fixed by #1462
Open

Pydantic v2やSkipJsonSchema周りを整理する #1458

Hiroshiba opened this issue Aug 17, 2024 · 3 comments · May be fixed by #1462
Labels
機能向上 要議論 実行する前に議論が必要そうなもの

Comments

@Hiroshiba
Copy link
Member

内容

Pydantic v2にアップデートする上で、| Noneな型に対する課題が見えてきました。
↓の時のコメントのやり取りです。

上記のプルリクエストで、APIに対して何かしらの変更が入らないようにしつつPydantic v2にアップデートしました。
その後浮き上がった課題に対して解決していく予定でしたが、特に後続の変更がないまま過ごしてしまいました。

ぶっちゃけ今どういう時にSkipJsonSchemaを付けるのか、全く覚えてないです。。

このissueではとりあえず条件についてまとめ直して、SkipJsonSchemaをどういう時につければいいのか把握したいです。
その後解決のプルリクエストを待つのが良いかなと。

VOICEVOXのバージョン

0.20.0

@Hiroshiba Hiroshiba added 機能向上 要議論 実行する前に議論が必要そうなもの labels Aug 17, 2024
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Aug 17, 2024

メンションすみません 🙇 @tarepan @sabonerune

どういう時にSkipJsonSchemaを付けるんでしたっけ。。。。。。
あとseparate_input_output_schemasオプションに関しても議論していた記憶があるんですが、どういう方針なんでしたっけ。。。

とりあえず課題感メモまで。

  • 多分今の API は Noneを受け付けられるのに受け付けられないようなモデル構造になってる
    • Noteのkeyとか
    • この辺別に破壊的変更にならないのでちゃっちゃと変えて良さそうな気がする
  • 多分Noneを受け付けられるものはSkipJsonSchemaをつけなくて良い
    • 既存のモデルは互換性というか、openapiが変わらないようにするためにSkipJsonSchemaがついてる?
    • 新しく作られるモデルはSkipJsonSchemaをつけなくても良さそう
  • 「キーが必須ではない」というものと、「キーが必須だけどNoneが受付可能」なのがあった気がする
    • 前者はどう定義するのが良いんでしたっけ。。。。。。。。デフォルト値を設定する・・・・?
  • あとseparate_input_output_schemasって結局どうすれば良いんでしたっけ。。。

@sabonerune
Copy link
Contributor

どういう時にSkipJsonSchemaを付けるんでしたっけ。。。。。。

キーが必須ではないがOpenAPIスキーマー上でnullを許容しないと表現するときに使用します。(変わるのはスキーマだけなのでnullを与えてもエラーになりません)

これが重要になるのがFasrAPIのQuery()Body()Form()です。
この部分のNoneSkipJsonSchemaをつけないとSwagger UIの表示が壊れます。
(恐らく規格上null要素が存在しないを区別して送る方法が定義されていないことが原因だと思います。)

  • 多分今の API は Noneを受け付けられるのに受け付けられないようなモデル構造になってる
  • 多分Noneを受け付けられるものはSkipJsonSchemaをつけなくて良い

Modelの方はSkipJsonSchemaを付けなくても問題はないと思います。(最初にPR出した時点ではつけていませんでした。)

  • 「キーが必須ではない」というものと、「キーが必須だけどNoneが受付可能」なのがあった気がする

「キーが必須ではない」はデフォルト値を設定すればいいです。
「キーが必須だけどNoneが受付可能」はNone許容型にしてデフォルト値を省けばそのようになるはずです。

あとseparate_input_output_schemasって結局どうすれば良いんでしたっけ。。。

入力のモデルと出力のモデルが異なるとき(キーが必須ではない等)OpenAPIに記述されるされるスキーマが*-Input*-Outputに分けられて記述されるようになります。
これの挙動がFastAPIのドキュメントと微妙に違ったりどうやっても破壊的変更になってしまうのではと考えてたので無効化しました。

正直よく分かっていないのですがこれを有効化するとスキーマーを作った時点で分割されているスキーマーは一つにならないように、最初は一つだったものは分割されないように機能追加する必要がでてくるので有効化するのは無理な気が…

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Aug 18, 2024

うおーーーーありがとうございます!!! 非常に助かります!!!

そうか、APIの引数だと話が別なんでした!!
UIが変になるからという文脈も思い出しました!!

Modelの方はあるべき姿にしていっちゃいたいですね!
覚えてるうちにやっちゃおうかな。

separate_input_output_schemas

正直よく分かっていないのですがこれを有効化するとスキーマーを作った時点で分割されているスキーマーは一つにならないように、最初は一つだったものは分割されないように機能追加する必要がでてくるので有効化するのは無理な気が…

なんとなくオンにできない可能性がありそう、というのは同感です。

分割周りの話はちょっとよくわかんなかったです!(少なくともAが互いに疎なBとCに分かれるという分割は起こらない認識。)
実際にどういう処理になるのか把握していませんが、想像するに、返り値がNoneにならないこともある| Noneな型とか、返り値もキーが必須でない値とかあったときに変なことになりそうな直感。

とりあえず、よくわからないし&わかろうとすると大変そうだからそのままオフにしとく・・・的な考えに至りました 😇
ありです!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
機能向上 要議論 実行する前に議論が必要そうなもの
Projects
None yet
2 participants