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

/api.jsonがOpenAPI Specとしてvalidでない #10632

Closed
okayurisotto opened this issue Apr 14, 2023 · 19 comments
Closed

/api.jsonがOpenAPI Specとしてvalidでない #10632

okayurisotto opened this issue Apr 14, 2023 · 19 comments
Labels
⚠️bug? This might be a bug

Comments

@okayurisotto
Copy link
Contributor

💡 Summary

Misskeyでは/api.jsonからOpenAPIのSpecを得ることができますが、これをswagger-cliでバリデーションしてみたところ、6034行のエラーメッセージが出力されました。その多くは許可されていないadditional propertiesが含まれていることが原因だと思われます。

🥰 Expected Behavior

swagger-cliのバリデーションでエラーが出ない。

🤬 Actual Behavior

swagger-cliのバリデーションで多くのエラーが出力される。

📝 Steps to Reproduce

  1. swagger-cliをインストールする
  2. swagger-cli validate https://misskey.example/api.jsonを実行する

📌 Environment

Misskey version: 少なくとも v13.11.3 と 14f30af
Your OS:
Your browser:


これはドラフトPR #10625 の作業をしているときに見付けた問題です。そのPRでの解決を試みています。

@okayurisotto okayurisotto added the ⚠️bug? This might be a bug label Apr 14, 2023
@okayurisotto
Copy link
Contributor Author

従来のSchemaOpenAPIで使われるSchemaを軽く比較してみたところ、次のような相違点が見受けられました。


OpenAPIのSchemaではtypeプロパティとして"any" | "null"が許容されません。"any"についてはtypeプロパティを指定しないことで代用できるかもしれませんが、"null"に関してはわかりません。もう少し調べてみます。


OpenAPIのSchemaではadditionalなプロパティは許容されません。現時点の問題のある実装によりこれが守られず、大量のエラーの原因になってしまっています。


OpenAPIのSchemaにはoptionalプロパティがありません。親オブジェクトのrequiredプロパティによって指定されないことでoptionalになる仕組みなので当然です。むしろ従来のSchema型で、requiredoptionalの両方がある理由を理解できていません。もう少し調べてみます。


OpenAPIのSchemaではrefプロパティが使えません。代わりに$refプロパティを使う必要があります。ただしそのとき、$ref以外のプロパティは許容されません。現時点の問題のある実装によりこれが守られず、大量のエラーの原因になってしまっています。


OpenAPIのSchemaではこれらのプロパティを指定できますが、従来のSchemaにはありません。これらのプロパティはすべてoptionalなものですので、基本的には無視でよいと思われます。

  • title
  • multipleOf
  • exclusiveMaximum
  • exclusiveMinimum
  • maxItems
  • minItems
  • uniqueItems
  • maxProperties
  • minProperties
  • not
  • additionalProperties
  • discriminator
  • readOnly
  • writeOnly
  • externalDocs
  • deprecated
  • xml
  • x-から始まる任意の文字列

とりあえず以上です。もしかしたらもっと別の相違点があるかもしれませんが、とりあえずこれらを解消するような修正をした上でまたバリデーションにかけてみて様子を見ることにします。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

json-schema内のスキーマは古いOpenAPIだかswaggerの仕様に基づいて書かれているはず

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

apiエンドポイントのresもそう

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

(😕と言われても相当昔からこれだったので許してほしい)

@usbharu
Copy link
Contributor

usbharu commented Apr 14, 2023

少し前のapi.jsonなので今は改善されているかもしれませんが、20611個の警告、281個の弱警告があったのでほとんど準拠できていないと思います。
https://misskey.usbharu.dev/notes/9a4vovq9ia

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

定義をmisskey-jsでしてバックエンドに読ませたほうがいい説

@okayurisotto
Copy link
Contributor Author

とりあえず私のドラフトPRではjson-schema.tsの編集はしないつもりでいますが、それで構いませんか? そちらまで編集してしまうと流石に変更点が膨大になってしまう予感がしていて、私もレビューする人も大変だろうと思われるのです。あくまでも私のPRの当初の目的は、「とりあえずanyよりはよい型を付けることで見通しやすくすること」なのです。

定義をmisskey-jsでしてバックエンドに読ませたほうがいい説

少なくとも悪くはないと思います。現時点のmisskey-jsは型定義が不足していて、他の箇所でエラーを引き起こしてしまってもいましたから。ただ私はバックエンドがどのような仕組みで動いていて、どのように開発されているのかをまだ理解できていないため、なんとも言えません。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

変更点が巨大になってもいいので抜本的に改善したい

(ので今までずっと放置されている)

@okayurisotto
Copy link
Contributor Author

なるほど、わかりました。

現時点の私のPRではOpenAPIのSpecの型を手動で書くようにしているのですが、これには未対応な部分がありますし、誤りがあるかもしれません。ですので抜本的な改善をするのであれば、早いうちに何かしらのツールを使った公式JSON SchemaからTypeScriptの型への変換をしておくのがよいと考えます。まずはこのことについて、どういったツールを使うべきかなどいろいろと調べてみようと思います。

またjson-schema.tsの編集がどういった部分にまで影響を及ぼすのか、そもそものバックエンドがどのように動いているのかについても調べてみます。ただ正直現時点の私の理解度は相当低いものであるため、もしかすると他の方がどんどんと実装していった方がよいかもしれません。変更点が多いと他のPRなどともconflictしてしまいますので、できるかぎり迅速に実装を済ませるべきでしょうから。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

ツールを使った公式JSON SchemaからTypeScriptの型への変換

それはもうSchemaTypeとして存在する

@okayurisotto
Copy link
Contributor Author

すみません説明不足でした。

私はgenOpenapiSpec関数に返り値の型定義がないことがvalidでないOpenAPI Specが生成されてしまっている現状を招いていると考え、これを修正しようとしました。そしてそのためにはOAI/OpenAPI-Specificationにあるv3.0のJSON Schemaを元にした型定義が必要になるのではないかと考え、ひとまずこのような型定義を手書きしました。ただこれは元としたJSON Schemaを完全にTypeScriptの型へ落とし込めたものではないため、今後抜本的な改善をする上では問題となるかもしれません。そこで、何かしらのツールを使って変換すべきだろうと考えたのです。

@tamaina さんの言うSchemaTypejson-schema.tsにあるこれのことだと推測しました)は、OpenAPIのSpec(最終的に生成するJSON)の全体の構造の完全な定義ではなく、あくまでその一部の定義(具体的にはこの部分でしょうか)だと読みました。私のこの認識は間違っているでしょうか?

私はまだソースコードのほとんどを読めておらず、何がどうなっているのかをまったく理解できていません。おかしなことを言ってしまっていたら申し訳ありません。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

genOpenapiSpec自体の問題でバリデーションエラーがあるならそれは修正を行った方がいい
(型定義としては、swaggerなどの公式型定義があればそれを指定する方が良い…うーん手書きしたのか…まあ公式のやつがないなら仕方ない)

一部の定義

一部というか内側というか本筋の部分
(SchemaTypeは、型"定義"とは言いずらく、JSON Schema v2およびv3のオブジェクト から TypeScriptの型定義を生成するジェネレーターのようなもの)

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

JSON Schema v2およびv3のオブジェクト から TypeScriptの型定義を生成するジェネレーターのようなもの

API出力の検査やサーバーコード内で見かけるPacked<"Hoge">の型定義はJSON Schema v2記述をSchemaTypeで生成することで成立している

JSON Schema v2は、refはbackend/src/models/json-schema配下に、各エンドポイントはそのエンドポイント構成スクリプト(backend/src/server/api/endopoints/**/*.ts)のmeta.resに書かれている

OpenAPIのSchemaにはoptionalプロパティがありません。

これが代表例なんだけど、optionalプロパティはJSON Schema v2由来。おそらくJSON Schema v3でバリデーションしようとしてエラーが出ていると私は解釈している。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

話は戻るけど、genOpenapiSpec自体はapi.json生成の末端中の末端なので、そこに型定義を書いたところでMisskeyコード全体の型定義を厳密化させることは不可能

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

要は古いJSON Schemaバージョンに基づいているからバリデーションが通らないという主張ではあるんだけど、Misskeyのことなので(?)古い規格に照らしても記述がいい加減な箇所はある気がする

@usbharu
Copy link
Contributor

usbharu commented Apr 14, 2023

少し前のapi.jsonなので今は改善されているかもしれませんが、20611個の警告、281個の弱警告があったのでほとんど準拠できていないと思います。 https://misskey.usbharu.dev/notes/9a4vovq9ia

OpenAPIのバージョン2.0として読み込ませた場合 5870個の警告、543個の弱警告があったのでいずれにせよtamainaさんの言う通り、おかしいです。

@okayurisotto
Copy link
Contributor Author

okayurisotto commented Apr 14, 2023

genOpenapiSpec自体の問題でバリデーションエラーがあるならそれは修正を行った方がいい

securitySchemes.ApiKeyAuth.inの値がOpenAPIでは未定義の"body"になってしまっているというバリデーションエラーがあります。ただこの値に関してはこちらにあるように難しい問題なのでひとまず無視することになるかもしれませんが。

ただ今後ミスによってバリデーションエラーを引き起こしてしまわないように、この関数自体にも型定義があるとよいのではないかとは考えています。

うーん手書きしたのか…

手書きに関しては #10625 の方にも書きましたが、Collaboratorの方々の意見を聞いていない段階で何かツールを使って自動生成するのは少しよくないのではと考えた末の暫定的なものです。実装の方向性が間違っていないことが確認できた時点でこのことについては判断を仰ぐつもりでいました。

JSON Schema v2およびv3のオブジェクト から TypeScriptの型定義を生成するジェネレーターのようなもの

これに関してはこの説明を聞いて概ね理解しました。ありがとうございます。

genOpenapiSpec自体はapi.json生成の末端中の末端なので、そこに型定義を書いたところでMisskeyコード全体の型定義を厳密化させることは不可能

それは確かにその通りです。ただとりあえずは末端のわかりやすく実装しやすい部分から考えていって、最終的に内部的で全体的な型定義のよりよい形を模索していくべきなのではないかという考え方をしました。もちろんこれはMisskey開発についての知識を持たない私の考え方ですので、これよりも適した実装手順があるのであればそちらを選ぶべきです。抜本的な改善とのことですし、内部的な部分から厳密に定義していくのもよいと思います。「抜本的」の度合いを見誤ってしまっていました。

@tamaina
Copy link
Contributor

tamaina commented Apr 14, 2023

OpenAPIのバージョン2.0として読み込ませた場合

リクエストのスキーマは3で書いているのでそれはそれで警告が出そう

@okayurisotto
Copy link
Contributor Author

#12403 で解決されたようなので閉じます。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️bug? This might be a bug
Projects
None yet
Development

No branches or pull requests

3 participants