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

fix(server): notes/createのバリデーションが効いていない #10082

Closed
wants to merge 9 commits into from

Conversation

tamaina
Copy link
Contributor

@tamaina tamaina commented Feb 25, 2023

Fix #10079

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Feb 25, 2023
@codecov
Copy link

codecov bot commented Feb 25, 2023

Codecov Report

Merging #10082 (6747f59) into develop (dbd9d11) will increase coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 6747f59 differs from pull request most recent head 98b521a. Consider uploading reports for the commit 98b521a to get more accurate results

@@             Coverage Diff             @@
##           develop   #10082      +/-   ##
===========================================
+ Coverage    24.72%   24.75%   +0.03%     
===========================================
  Files          705      705              
  Lines        65224    65150      -74     
  Branches      2303     2303              
===========================================
+ Hits         16125    16127       +2     
+ Misses       49099    49023      -76     
Impacted Files Coverage Δ
packages/backend/src/misc/schema.ts 0.00% <0.00%> (ø)
.../src/server/api/endpoints/admin/drive/show-file.ts 0.00% <0.00%> (ø)
...ckend/src/server/api/endpoints/drive/files/show.ts 0.00% <0.00%> (ø)
...s/backend/src/server/api/endpoints/notes/create.ts 0.00% <0.00%> (ø)
...nd/src/server/api/endpoints/notes/search-by-tag.ts 0.00% <0.00%> (ø)
...ges/backend/src/server/api/endpoints/pages/show.ts 0.00% <0.00%> (ø)
...ackend/src/server/api/endpoints/users/followers.ts 0.00% <0.00%> (ø)
...ackend/src/server/api/endpoints/users/following.ts 0.00% <0.00%> (ø)
...api/endpoints/users/search-by-username-and-host.ts 0.00% <0.00%> (ø)
...ges/backend/src/server/api/endpoints/users/show.ts 0.00% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@syuilo
Copy link
Member

syuilo commented Feb 25, 2023

これ例えば text に値が入っていたら結局 fileIds のバリデーションは無視されることにならない?

@mei23
Copy link
Contributor

mei23 commented Feb 25, 2023

これ例えば text に値が入っていたら結局 fileIds のバリデーションは無視されることにならない?

そう思うわ

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

これ例えば text に値が入っていたら結局 fileIds のバリデーションは無視されることにならない?

そう思うわ

試していない上にテストもないのでわからないけど、anyOfで一つが満足していたら他のもののバリデーションをやらないというのは、まずいのでは

@mei23
Copy link
Contributor

mei23 commented Feb 25, 2023

「何れかのフィールドがあること」と「フィールドが存在した場合の制約チェック」を1つのanyOfでまかなうことはできなくて、結局 https://github.com/misskey-dev/misskey/pull/9762/files の変更元のようにするしかないと思ってるのだわ。

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

「何れかのフィールドがあること」と「フィールドが存在した場合の制約チェック」を1つのanyOfでまかなうことはできなくて、

直感に反するけどマジだわ

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

anyOfにrequiredだけ含めるみたいなことをしても大丈夫っぽい

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

anyOfの中にrequiredだけ指定するのは流石にやめてほしいので直した
#9762

おい

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

Schema頑張れんもんかな

@syuilo
Copy link
Member

syuilo commented Feb 25, 2023

Schemaで推論できない場合は諦めて手動注釈してもらうようにするとか

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

多分「anyOf内でpropertiesを定義しない」という規約を作った方がいい

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

そうすればSchemaTypeをちょっと変えるだけで済みそう

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

ObjTypeの< > を#8332 で変えたんだけど、なんでこうしたか忘れた

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

型計算量を減らしたかったらしいけど直感的ではない

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

  • 型チェックは通った
  • テスト作りたいけど…どうすりゃいいんだ…

@tamaina tamaina requested a review from syuilo February 25, 2023 13:11
@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

雰囲気でテストを作り始めてはいるもののしばらくかかりそう

@syuilo
Copy link
Member

syuilo commented Feb 25, 2023

各エンドポイントもInjectableなクラスだから他のServiceクラスと同様の方法でテストできそう

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

バリデーションのテストとサービスだなんだのテストは分けたほうがいいかもしれない

そしてバリデーションのテストだけなら作るのは死ぬほど容易

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

なんかとはいえ二重にテストするのは面倒

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

  • バリデーションだけサクッとテストした方がテスト時間の短縮になる
  • バリデーションで落ちているのか後処理で落ちているのかがわかりやすい
  • とりあえずバリデーションだけ書いて逃げられる

@mei23
Copy link
Contributor

mei23 commented Feb 25, 2023

バリデーションのテストとサービスだなんだのテストは分けたほうがいいかもしれない

ユニットテストって本来そういうものじゃないかしら?

@tamaina
Copy link
Contributor Author

tamaina commented Feb 25, 2023

ユニットテストって本来そういうものじゃないかしら?

わかる

@github-actions github-actions bot removed the packages/backend Server side specific issue/PR label Feb 26, 2023
@syuilo
Copy link
Member

syuilo commented Feb 26, 2023

#10090 がマージされた

@syuilo syuilo closed this Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noteに画像を16枚以上添付して投稿できてしまう
3 participants