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.

Sign up for GitHub

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

Jump to bottom

fix(server): DriveFile related N+1 query when call note packMany #10133

Merged
merged 5 commits into from
Mar 3, 2023
Merged

fix(server): DriveFile related N+1 query when call note packMany #10133

merged 5 commits into from
Mar 3, 2023
+66 −58

Conversation

rinsuki
Copy link
Contributor

@rinsuki rinsuki commented Feb 27, 2023

Fix #10129

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

codecov bot commented Feb 27, 2023 •
edited
Loading

Codecov Report

Merging #10133 (84899eb) into develop (81e6a21) will increase coverage by 0.13%.
The diff coverage is 25.75%.

@@             Coverage Diff             @@
##           develop   #10133      +/-   ##
===========================================
+ Coverage    25.06%   25.19%   +0.13%     
===========================================
  Files          705      706       +1     
  Lines        65151    65281     +130     
  Branches      2307     2331      +24     
===========================================
+ Hits         16329    16447     +118     
- Misses       48822    48834      +12
Impacted Files Coverage Δ
...kend/src/core/entities/GalleryPostEntityService.ts 51.66% <0.00%> (-0.88%) ⬇️
...ges/backend/src/core/entities/NoteEntityService.ts 20.23% <14.28%> (-0.06%) ⬇️
...end/src/core/entities/NotificationEntityService.ts 46.21% <23.33%> (+11.25%) ⬆️
...ackend/src/core/entities/DriveFileEntityService.ts 29.22% <27.27%> (-0.33%) ⬇️
packages/backend/src/misc/is-not-null.ts 60.00% <60.00%> (ø)
packages/backend/src/core/WebhookService.ts 39.08% <0.00%> (-1.40%) ⬇️
...ges/backend/src/core/entities/RoleEntityService.ts 38.15% <0.00%> (-1.28%) ⬇️
packages/backend/src/core/AntennaService.ts 35.39% <0.00%> (-0.64%) ⬇️
packages/backend/src/server/ServerService.ts 0.00% <0.00%> (ø)
packages/backend/src/server/api/stream/types.ts 0.00% <0.00%> (ø)
... and 12 more

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

@rinsuki @acid-chicken
Update packages/backend/src/misc/is-not-null.ts
2f529c1 
Co-authored-by: Acid Chicken (硫酸鶏) <root@acid-chicken.com>
@syuilo
Copy link
Member

syuilo commented Feb 27, 2023

is-not-nullでDon't use {}as a type.{} actually means "any non-nullish value".が出てる

@rinsuki
Copy link
Contributor Author

rinsuki commented Feb 27, 2023

直した

acid-chicken
acid-chicken reviewed Feb 28, 2023
View reviewed changes
packages/backend/src/core/entities/DriveFileEntityService.ts Outdated
fileIds: DriveFile['id'][],
options?: PackOptions,
): Promise<Packed<'DriveFile'>[]> {
const files = await this.driveFilesRepository.findBy({ id: In(fileIds) });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここって fileIds の順序にソートしなくても良い？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ〜〜、確かにした方がいいな

@syuilo syuilo merged commit 452a48e into develop Mar 3, 2023
@syuilo syuilo deleted the feat/better-drive-file-query branch March 3, 2023 01:07
@syuilo
Copy link
Member

syuilo commented Mar 3, 2023

🙏🙏

syuilo added a commit that referenced this pull request Mar 3, 2023
@syuilo
Revert "fix(server): DriveFile related N+1 query when call note packM…
a7c82ee 
…any (#10133)"

This reverts commit 452a48e.
@syuilo
Copy link
Member

syuilo commented Mar 3, 2023 •
edited
Loading

ファイル付きノートのRenoteをpackしたときに、場合によってはファイルが消えるわね

syuilo
syuilo reviewed Mar 3, 2023
View reviewed changes
packages/backend/src/core/entities/NoteEntityService.ts
@@ -388,11 +391,14 @@ export class NoteEntityService implements OnModuleInit {
}

await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes));
const fileIds = notes.flatMap(n => n.fileIds);
Copy link
Member

@syuilo syuilo Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

多分ここでrenote(あとreply?)も考慮する必要がある

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

もしくは、_hint_に希望するファイルが含まれてなかったらスキップするのではなくて諦めてDBにfindしてくるようにするとか

rinsuki added a commit that referenced this pull request Mar 3, 2023
@rinsuki
Revert "Revert "fix(server): DriveFile related N+1 query when call no…
b8f5926 
…te packMany (#10133)""

This reverts commit a7c82ee.
@rinsuki rinsuki mentioned this pull request Mar 3, 2023
Merged
syuilo pushed a commit that referenced this pull request Mar 4, 2023
@rinsuki
fix(server): DriveFile related N+1 query when call note packMany (aga…
49f0837 
…in) (#10190)

* Revert "Revert "fix(server): DriveFile related N+1 query when call note packMany (#10133)""

This reverts commit a7c82ee.

* packManyByIdsMap: 存在チェックをしてなかったものは null を入れるように

* Note.packMany で reply とか renote がもうあったらそのファイルも引く

* テストを書く

* fix test

* fix test

* fix test

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syuilo syuilo syuilo approved these changes

@acid-chicken acid-chicken acid-chicken approved these changes

@tamaina tamaina Awaiting requested review from tamaina

Assignees
No one assigned
Labels
packages/backend Server side specific issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

タイムライン取得時にdrive_fileへのクエリ回数を減らす
3 participants
@rinsuki @syuilo @acid-chicken