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

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

Closed
wants to merge 4 commits into from

Conversation

akanevrc
Copy link
Contributor

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

  • 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

@github-actions github-actions bot added packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR labels May 18, 2024
Copy link
Contributor

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

差分はこちら

Get diff files from Workflow Page

Copy link

codecov bot commented May 18, 2024

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 ⚠️
...ackend/src/server/api/endpoints/notes/reactions.ts 0.00% 2 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@syuilo
Copy link
Member

syuilo commented May 18, 2024

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

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

@akanevrc
Copy link
Contributor Author

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

  • 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
Copy link
Contributor Author

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

@akanevrc akanevrc closed this May 24, 2024
@akanevrc akanevrc deleted the feat_muted_reactions branch May 24, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants