fix(backend): 破損した通知をクライアントに送信しないように #13284
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since notifications are stored in Redis, we can't expect relational integrity: deleting a user will *not* delete notifications that mention it. But if we return notifications with missing bits (a `follow` without a `user`, for example), the frontend will get very confused and throw an exception while trying to render them. This change makes sure we never expose those broken notifications. For uniformity, I've applied the same logic to notes and roles mentioned in notifications, even if nobody reported breakage in those cases. Tested by creating a few types of notifications with a `notifierId`, then deleting their user. (cherry picked from commit 421f8d4)
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #13284 +/- ##
============================================
- Coverage 77.46% 64.47% -12.99%
============================================
Files 184 981 +797
Lines 24725 110048 +85323
Branches 463 5562 +5099
============================================
+ Hits 19152 70950 +51798
- Misses 5566 37666 +32100
- Partials 7 1432 +1425 ☔ View full report in Codecov by Sentry. |
|
このPRによるapi.jsonの差分 差分はこちら |
1 task
|
👀 |
1 task
5 tasks
|
#13412 と競合する可能性があり |
|
#13412 ではユーザーチェックのみだけどこちらはノートの存在確認もしてるのねなるほど |
|
（filter周りの書き方をなんとかしてほしい |
tai-cha
reviewed
Feb 20, 2024
| packedNotes, | ||
| packedUsers, | ||
| }))); | ||
| })))).filter(n => n); |
There was a problem hiding this comment.
型のわかりやすさの問題で
})))).filter(NotNull);みたいなやつのほうがいい可能性
| const user = hint?.packedUsers != null | ||
| ? hint.packedUsers.get(reaction.userId)! | ||
| : await this.userEntityService.pack(reaction.userId, { id: meId }); | ||
| return { | ||
| user, | ||
| reaction: reaction.reaction, | ||
| }; | ||
| })); | ||
| }))).filter(r => r.user); |
There was a problem hiding this comment.
ここもr.user !== nullとかで判定した方がいい説
| const packedUser = hint?.packedUsers != null ? hint.packedUsers.get(userId) : null; | ||
| if (packedUser) { | ||
| return packedUser; | ||
| } | ||
|
|
||
| return this.userEntityService.pack(userId, { id: meId }); | ||
| })); | ||
| }))).filter(u => u); |
|
ぶっ足しした方が見やすいならそうするか…（さすがにPRの粒度的にぶったししない方が見やすいと思ってた） |
|
#13412 でこれを取り込んだうえで可読性をめちゃくちゃ上げた |
This was referenced Feb 24, 2024
Closed
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Cherry-picked from https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/415
通知に関連するリソースが削除されているなどで破損した通知オブジェクトはクライアントに送信しないように
（通知欄が無限にリロードされる問題が改善する可能性があります）
Why
Fix #10650
Fix #13369
Additional info (optional)
Checklist