misskey-dev / misskey

🌎 A completely free and open interplanetary microblogging platform 🚀
https://misskey-hub.net/
GNU Affero General Public License v3.0
9.99k stars 1.36k forks source link

backend: スキーマ定義において起きるべき型エラーが起きない #10745

Closed okayurisotto closed 1 year ago

okayurisotto commented 1 year ago

💡 Summary

APIエンドポイント(api/endpoints/**/*.ts)でのスキーマ定義において、misc/json-schema.tsで定義されたSchema型による型チェックが厳密には行われていないために型エラーが期待される場所で型エラーが起きないという問題があります。これは #10737 での作業中に発見した問題です。

例えば、Schema型では次のようなプロパティは定義されておらず、それらが使われることを想定した作りにはなっていません。しかし実際には使われてしまっています。そしてそれらは型エラーを起こしません。

uniqueItemshttps://github.com/misskey-dev/misskey/blob/a2e475f2e8c68cd98d7126700a238077d54e2afe/packages/backend/src/server/api/endpoints/app/create.ts#L27-L29 minItems / maxItemshttps://github.com/misskey-dev/misskey/blob/a2e475f2e8c68cd98d7126700a238077d54e2afe/packages/backend/src/server/api/endpoints/i/update.ts#L115-L127 additionalPropertieshttps://github.com/misskey-dev/misskey/blob/a2e475f2e8c68cd98d7126700a238077d54e2afe/packages/backend/src/server/api/endpoints/pages/create.ts#L51-L56

(定義されたスキーマはapi/endpoint-base.tsで定義されているEndpoint<T extends IEndpointMeta, Ps extends Schema>というabstractなクラスを使用する際に、typeofを使って型にしてextendsでの型チェックを行うようになっています。ですのでまったくの想定外な型に対しては型エラーが適切に起きます。)

Schema型をそれらのプロパティに対応させた上で、すべてのAPIエンドポイントのスキーマ定義でSchema型による型チェックを行うようにする必要があると考えます。またこれと関連して、APIエンドポイントでのスキーマ定義だけでなく、models/json-schema/*.tsでのスキーマ定義でも型チェックを行うようにしておく必要があるとも考えます。

🥰 Expected Behavior

型チェックが厳密に行われることで、想定外のプロパティが含まれることがないようにする。

🤬 Actual Behavior

型チェックが厳密には行われておらず、想定外のプロパティが許容されている。

📝 Steps to Reproduce

1. 2. 3.

📌 Environment

Misskey version: a2e475f2e8c68cd98d7126700a238077d54e2afe (13.11.3) Your OS: Your browser:


私のフォークで修正作業を進めています。

tamaina commented 1 year ago

既存のSchema/SchemaTypeは型チェック用に作られていないのでそれはそうという感じ

json schemaがvalidなのかはビルド時とかに判定させたい
(そしてそれはmisskey-jsに移した上でやりたい)

okayurisotto commented 1 year ago

既存のSchema/SchemaTypeは型チェック用に作られていない

であればそれらはどのような役割の型なのでしょうか。 https://github.com/misskey-dev/misskey/issues/10632#issuecomment-1508453772 では「JSON SchemaのオブジェクトからTypeScriptの型定義を生成するジェネレーター」という説明でしたが、私はそのTypeScriptの型定義に誤りが含まれる原因となり得る問題について、このIssueで取り上げたつもりでした。ジェネレーターへの入力として、想定していないプロパティがある変数のtypeof結果を渡すこと自体がよくないのではないかという考えです。TypeScriptの型定義に誤りが含まれていた場合、その型定義を元にしたソースコードは不安要素となってしまいます。ジェネレーターが適切に動作するためには、そのジェネレーターへの入力の型をより厳密に定義すべきだと考えます。

この問題は、Foo<T extends Schema>というような想定していないプロパティの存在を許容するものにtypeof結果を渡していることが原因ではないかと考えています。そして、JSON Schemaのオブジェクト(変数)を定義するときにsatisfies Schemaのようにすることで解決できるのではないかと思い、そのような編集をしているところです。まだ確認作業が残ってはいますが、ご希望であればすぐにdraftなPRとして提出できます。

json schemaがvalidなのかはビルド時とかに判定させたい

前述の通り、TypeScriptの型定義に誤りが含まれていた場合、その型定義を元にしたソースコードは不安要素となってしまいます。ですので私は、ビルド時ではなくより早い段階でJSON Schemaがvalidでないことを判定できた方が開発しやすいだろうと考えます。しかし私にはMisskey開発の経験がほぼないため、これはただの憶測です。実際のところどうなのかについて教えていただけるとありがたいです。

(そしてそれはmisskey-jsに移した上でやりたい)

https://github.com/misskey-dev/misskey/issues/10632#issuecomment-1508105284 でも触れられていた、「misskey-jsで定義しbackendが読み込む」というものでしょうか。このIssueを解決する改善は、確かにそのような抜本的なものではなく、場当たり的で暫定的なものです。しかし抜本的な改善を行うための移行作業をするとき、既存のコードがある程度整理されていることは大事だと考えます。少なくともこのIssueの解決はその移行作業を邪魔するものではないという認識です。

https://github.com/misskey-dev/misskey/issues/10632#issuecomment-1508105284 で移行の話が出てからしばらく経ちましたが、その作業は今どのような状況にあるのでしょうか。探してみても見付けられないため、私は私でその移行のために必要になる既存コードの理解と整理を試みているところです。そしてその整理をしているときにこの問題に気付いてIssueを作成したという経緯です。もしこれが現在進められている移行作業を邪魔するのであれば、そのように伝えてください。このIssue(と、これと関連したPR #10737 やそれと対応したIssue #10736 )はcloseします。すみません。

tamaina commented 1 year ago

どのような役割の型

主にSchemaTypeでConditional Typeで使用するためです。

JSON SchemaはTypeScript型ではなくビルド時テスト時にバリデーターによってバリデーションを行うべきです。

tamaina commented 1 year ago

その作業は今どのような状況にあるのでしょうか。

まだ手をつけていません。

tamaina commented 1 year ago

Misskey独自で定義するSchemaは公式のものではなくConditional Typeで使用するのに使うものなので、厳密に定義して縛る型として使うことは想定していません。

okayurisotto commented 1 year ago

主にSchemaTypeでConditional Typeで使用するためです。

この使い方についてはすでに理解しています。しかしSchema型ではすでにminLengthmaxLengthminimummaximumなどといった、TypeScriptの型に影響を与えないプロパティが定義されています。またrequireddefaultなどは、(書き方が悪いため機能していませんが)型チェックを行うことを明確に意図しているように見えます。

https://github.com/misskey-dev/misskey/blob/a2e475f2e8c68cd98d7126700a238077d54e2afe/packages/backend/src/misc/json-schema.ts#L87-L105

Schema型が型チェックを行うための型ではないのであれば、このIssueで追加すべきではないかと提案したminItemsmaxItemsなどが不必要であるのと同じように、それらのプロパティについても不必要なはずです。それらのプロパティが定義されている現状からは、どうしても、このSchema型が型チェックのための型であるように感じてしまいます。そういう理由で私はこのIssueを作成しました。

しかしそもそもSchema型はそのような使い方をするものではないとのことでしたので、このIssueはinvalidとしてcloseすることになります。構いませんか? このとき、このIssueが解決されることをdraft状態で待っているPR #10737 はdraftが外されることになります。

また、minLengthmaxLengthminimummaximumなどといった、現在のSchema型の定義に存在しながらもConditional Typeで使用されておらず、TypeScriptの型に影響を与えないプロパティについては不必要ということになります。そして私は、今後の移行作業などのためにもそれらは取り除いておくとよいと考えます。移行作業を担当する人を含む他の人が、私と同じような勘違いをしてしまっては問題ですから。このとき、書き方が悪いために機能していないrequireddefaultも、修正しないまま削除することになるでしょう。そのような編集をするIssueとPRを作成しますが、構いませんか?

もし万が一このIssueをinvalidではないとするのであれば、このIssueを解決するPRを作成しますのでマージをお願いすることになります。そのPRには、minItemsmaxItemsなどを追加する修正と、satisfiesを使った型チェックを行うようにする変更が含まれることになると思われます。公式のJSON Schemaを完全にTypeScriptの型定義に落とし込むかどうかについては追加の検討が必要だと考えますが、現在のソースコードで使われているプロパティに関しては定義することが妥当ですので。


JSON SchemaはTypeScript型ではなくテスト時にバリデーターによってバリデーションを行うべきです。

言葉足らずですみません。私はスキーマ定義をSchema型で型チェックしたときにエラーが起きてしまう現状をなんとかすべきではないか、ということをこのIssueでは伝えたかったのです。ただ、そもそもSchema型は型チェックのためのものではないとのことでしたので、このIssueをinvalidであるとしてcloseするのであれば、テストを追加すべきであるというこの案に賛成します。TypeScriptの型よりバリデーターを使った方が、簡単かつ丁寧にバリデーションを行えるだろうと予想します。

tamaina commented 1 year ago

型チェックはしないけど参考程度には使っているので、型というかスキーマはvalidなものであることに越したことはないし、SchemaTypeで使わないプロパティもあった方がいいというイメージ

Nanashia commented 1 year ago

(すでにCloseされていますが...)

例えば、Schema型では次のようなプロパティは定義されておらず、それらが使われることを想定した作りにはなっていません。しかし実際には使われてしまっています。そしてそれらは型エラーを起こしません。

各endpointのparamDefはajvでのバリデーションチェックで参照されています。ajvにはuniqueItems, minItems / maxItems, additionalPropertiesは定義されているので、paramDefにこれらのプロパティが存在すること自体は想定されているものと思います。 https://ajv.js.org/json-schema.html

その上で、paramDefにexactな型チェックを提供するinterface Schemaを本気で作ろうと考えたとき、ajvがサポートするその他のキー、例えばmultipleOfとかpatternPropertiesみたいなものも入れるべきでしょう。そうなると、ajvの提供する型定義を使いたくなるかもしれません。(json-schema.ts

しかし、misskeyにはmisskey独自の仕様(ref vs. $refとか、allOf,anyOf,oneOfの細かい挙動の差異とか)があるため、ajvの定義をそのまま使うことはできません。したがって上記2つが融合するような型を正しく定義する必要があり、そういう定義を作るのはなかなか大変な作業に見えます。


それはそれとして、せっかくTypescriptを使ってるんだから変なプロパティはエディタ上で波線ついて教えてほしい、という気持ちはよく分かります。

tamaina commented 1 year ago

misskeyにはmisskey独自の仕様(ref vs. $refとか、allOf,anyOf,oneOfの細かい挙動の差異とか)があるため、ajvの定義をそのまま使うことはできません。したがって上記2つが融合するような型を正しく定義する必要があり、そういう定義を作るのはなかなか大変な作業に見えます。

https://github.com/misskey-dev/misskey/pull/10752 でSchemaType廃止してajvとかが提供する型にしたいところ