misskey icon indicating copy to clipboard operation
misskey copied to clipboard

feat: ミュートされたユーザーのリアクションをレスポンスに含めないようにする(#13456)

Open akanevrc opened this issue 1 year ago • 4 comments

What

  1. ミュートされたユーザーのリアクションをノートに含めないようにする
    • note.reactionAndUserPairCacheに含まれるユーザーのみを対象とする
  2. ミュートされたユーザーをリアクション詳細に含めないようにする

Why

#13456

Additional info (optional)

  • Whatの1について
    • note.reactionAndUserPairCacheに含まれるユーザーのみを対象とするため、現状では最初から数えてPER_NOTE_REACTION_USER_PAIR_CACHE_MAX(=16)のユーザーのみが処理される
    • 対象のユーザーがミュートされていた場合、リアクション数をデクリメントする(または消去する)
    • したがって、リアクション数16以下のときには、ミュートは必ず実施される
    • この処理ではミュートされたユーザーを検索する際、Redisのキャッシュを使用する(これによりパフォーマンスの問題は起きにくいと考える)
  • Whatの2について
    • 1で処理が漏れたユーザーについては、リアクション詳細(MkReactionsViewer.details.vue等)に表示されないようミュートする
    • 注意されたいこととして、フロントエンドのAPI呼び出し方法を変更している
      • 具体的にはmisskeyApiGet('notes/reactions', {...})misskeyApi('notes/reactions', {...})に置き換えている
      • これは、notes/reactionsの内部においてミュートの情報を参照するためにユーザー情報が必要であり、このためPOSTリクエストでCredentialsを含める必要があるからである(が、PR作成者はAPI呼び出しの仕様を詳しく把握できなかったので、この呼び出し方法について有識者にご判断いただきたい)
  • テストについて
    • ブラウザを手動で操作することによる動作確認は実施済み
    • e2eテストを書いてみたが、手元ではテスト全体がうまく動作しないのでこのテストはgit revertしてある

Checklist

  • [x] Read the contribution guide
  • [ ] Test working in a local environment
  • [ ] (If needed) Add story of storybook
  • [ ] (If needed) Update CHANGELOG.md
  • [ ] (If possible) Add tests

akanevrc avatar May 18 '24 01:05 akanevrc

このPRによるapi.jsonの差分

差分はこちら

Get diff files from Workflow Page

github-actions[bot] avatar May 18 '24 01:05 github-actions[bot]

Codecov Report

Attention: Patch coverage is 23.33333% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 63.55%. Comparing base (f8261a1) to head (ebf10ff). Report is 557 commits behind head on develop.

Files Patch % Lines
...ges/backend/src/core/entities/NoteEntityService.ts 19.23% 20 Missing and 1 partial :warning:
...ackend/src/server/api/endpoints/notes/reactions.ts 0.00% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #13827       +/-   ##
============================================
- Coverage    79.95%   63.55%   -16.41%     
============================================
  Files          956      985       +29     
  Lines       108864   112166     +3302     
  Branches      8413     5521     -2892     
============================================
- Hits         87045    71283    -15762     
- Misses       21819    39213    +17394     
- Partials         0     1670     +1670     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 18 '24 01:05 codecov[bot]

パフォーマンスへの影響が気になる点

  • GETが使えなくなる(キャッシュできなくなる)点
  • ミュート情報がキャッシュされてない場合、追加でデータベースへのクエリが発生する点

syuilo avatar May 18 '24 02:05 syuilo

おっしゃる通り、色々考慮が不足していました

  • GETが使えなくなる(キャッシュできなくなる)点

やはりフロントエンド側でどうにかしたほうが良いかもしれません(バックエンドだとPOSTは免れない)

  • 解決案:
    • notes/reactionsをGETで取得、それとは別にmute/listを取得、リアクション詳細でユーザーを非表示にする
      • (現在はmute/listが毎回DBアクセスになるので、Redisに置き換えたほうがよさそう?)
  • 懸念点:
    • ミュート情報をブラウザ側でどのように記憶しておくか?
  • ミュート情報がキャッシュされてない場合、追加でデータベースへのクエリが発生する点

Redisのキャッシュを極力使用しないとすると、DBにキャッシュする方法が考えられます

  • 解決案A:

    • userテーブルにmuteeIdCache character varying(32)[]のようなカラムを追加する
    • このカラムには、当該ユーザーにミュートされたユーザーのIDを格納する
    • 配列の件数の最大値は何件にすべきか不明…
    • ミュートの件数が配列の最大長を超える場合、無効であることを示す値を格納する
    • NoteEntityServiceのミュート処理では、上記のキャッシュが使用可能なら上記を、無効である場合にはRedisを使用する
    • これにより、(理想的には)ほとんどのユーザーを追加のDBアクセスの発生なしに処理できる
  • 懸念点:

    • DBのマイグレーションが必要
    • 配列の最大長を何件にするか?
    • これによりDBの容量がどうなるか?
  • 解決案B:

    • 仕様の把握が不完全ですが、タイムラインを取得する際にミュート情報がキャッシュされているようです
    • タイムライン取得とリアクション詳細表示は時間的にそれほど乖離した操作ではないと思われますので、キャッシュが残っている可能性は高いのではないでしょうか?
  • 懸念点:

    • 上記の検証方法が不明…

akanevrc avatar May 18 '24 08:05 akanevrc

解決案が不明瞭なので一旦クローズします

akanevrc avatar May 24 '24 13:05 akanevrc

ミュート情報がキャッシュされてない場合、追加でデータベースへのクエリが発生する点

厳しめのレートリミットを掛けたり、キャッシュするにしても更新頻度を下げたりすればある程度マシになったりしますかね…?

Sayamame-beans avatar Jun 28 '24 13:06 Sayamame-beans

微妙かも

syuilo avatar Jul 16 '24 06:07 syuilo