From a7c82eeabcc732e76c5c358c98812cac8457d57f Mon Sep 17 00:00:00 2001 From: syuilo Date: Fri, 3 Mar 2023 20:26:44 +0900 Subject: [PATCH] Revert "fix(server): DriveFile related N+1 query when call note packMany (#10133)" This reverts commit 452a48e7f4782cf9bdf08b554264ab3cdcb12685. --- .../core/entities/DriveFileEntityService.ts | 24 +----- .../core/entities/GalleryPostEntityService.ts | 3 +- .../src/core/entities/NoteEntityService.ts | 8 +- .../entities/NotificationEntityService.ts | 84 ++++++++++++------- packages/backend/src/misc/is-not-null.ts | 5 -- 5 files changed, 58 insertions(+), 66 deletions(-) delete mode 100644 packages/backend/src/misc/is-not-null.ts diff --git a/packages/backend/src/core/entities/DriveFileEntityService.ts b/packages/backend/src/core/entities/DriveFileEntityService.ts index f5b1f98153..158fafa9d5 100644 --- a/packages/backend/src/core/entities/DriveFileEntityService.ts +++ b/packages/backend/src/core/entities/DriveFileEntityService.ts @@ -1,5 +1,5 @@ import { forwardRef, Inject, Injectable } from '@nestjs/common'; -import { DataSource, In } from 'typeorm'; +import { DataSource } from 'typeorm'; import { DI } from '@/di-symbols.js'; import type { NotesRepository, DriveFilesRepository } from '@/models/index.js'; import type { Config } from '@/config.js'; @@ -21,7 +21,6 @@ type PackOptions = { }; import { bindThis } from '@/decorators.js'; import { isMimeImage } from '@/misc/is-mime-image.js'; -import { isNotNull } from '@/misc/is-not-null.js'; @Injectable() export class DriveFileEntityService { @@ -256,29 +255,10 @@ export class DriveFileEntityService { @bindThis public async packMany( - files: DriveFile[], + files: (DriveFile['id'] | DriveFile)[], options?: PackOptions, ): Promise[]> { const items = await Promise.all(files.map(f => this.packNullable(f, options))); return items.filter((x): x is Packed<'DriveFile'> => x != null); } - - @bindThis - public async packManyByIdsMap( - fileIds: DriveFile['id'][], - options?: PackOptions, - ): Promise['id'], Packed<'DriveFile'>>> { - const files = await this.driveFilesRepository.findBy({ id: In(fileIds) }); - const packedFiles = await this.packMany(files, options); - return new Map(packedFiles.map(f => [f.id, f])); - } - - @bindThis - public async packManyByIds( - fileIds: DriveFile['id'][], - options?: PackOptions, - ): Promise[]> { - const filesMap = await this.packManyByIdsMap(fileIds, options); - return fileIds.map(id => filesMap.get(id)).filter(isNotNull); - } } diff --git a/packages/backend/src/core/entities/GalleryPostEntityService.ts b/packages/backend/src/core/entities/GalleryPostEntityService.ts index fb147ae181..ab29e7dba1 100644 --- a/packages/backend/src/core/entities/GalleryPostEntityService.ts +++ b/packages/backend/src/core/entities/GalleryPostEntityService.ts @@ -41,8 +41,7 @@ export class GalleryPostEntityService { title: post.title, description: post.description, fileIds: post.fileIds, - // TODO: packMany causes N+1 queries - files: this.driveFileEntityService.packManyByIds(post.fileIds), + files: this.driveFileEntityService.packMany(post.fileIds), tags: post.tags.length > 0 ? post.tags : undefined, isSensitive: post.isSensitive, likedCount: post.likedCount, diff --git a/packages/backend/src/core/entities/NoteEntityService.ts b/packages/backend/src/core/entities/NoteEntityService.ts index c732a98a11..2ffe5f1c21 100644 --- a/packages/backend/src/core/entities/NoteEntityService.ts +++ b/packages/backend/src/core/entities/NoteEntityService.ts @@ -11,7 +11,6 @@ import type { Note } from '@/models/entities/Note.js'; import type { NoteReaction } from '@/models/entities/NoteReaction.js'; import type { UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, DriveFilesRepository } from '@/models/index.js'; import { bindThis } from '@/decorators.js'; -import { isNotNull } from '@/misc/is-not-null.js'; import type { OnModuleInit } from '@nestjs/common'; import type { CustomEmojiService } from '../CustomEmojiService.js'; import type { ReactionService } from '../ReactionService.js'; @@ -258,7 +257,6 @@ export class NoteEntityService implements OnModuleInit { skipHide?: boolean; _hint_?: { myReactions: Map; - packedFiles: Map>; }; }, ): Promise> { @@ -286,7 +284,6 @@ export class NoteEntityService implements OnModuleInit { const reactionEmojiNames = Object.keys(note.reactions) .filter(x => x.startsWith(':') && x.includes('@') && !x.includes('@.')) // リモートカスタム絵文字のみ .map(x => this.reactionService.decodeReaction(x).reaction.replaceAll(':', '')); - const packedFiles = options?._hint_?.packedFiles; const packed: Packed<'Note'> = await awaitAll({ id: note.id, @@ -307,7 +304,7 @@ export class NoteEntityService implements OnModuleInit { emojis: host != null ? this.customEmojiService.populateEmojis(note.emojis, host) : undefined, tags: note.tags.length > 0 ? note.tags : undefined, fileIds: note.fileIds, - files: packedFiles != null ? note.fileIds.map(fi => packedFiles.get(fi)).filter(isNotNull) : this.driveFileEntityService.packManyByIds(note.fileIds), + files: this.driveFileEntityService.packMany(note.fileIds), replyId: note.replyId, renoteId: note.renoteId, channelId: note.channelId ?? undefined, @@ -391,14 +388,11 @@ export class NoteEntityService implements OnModuleInit { } await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes)); - const fileIds = notes.flatMap(n => n.fileIds); - const packedFiles = await this.driveFileEntityService.packManyByIdsMap(fileIds); return await Promise.all(notes.map(n => this.pack(n, me, { ...options, _hint_: { myReactions: myReactionsMap, - packedFiles, }, }))); } diff --git a/packages/backend/src/core/entities/NotificationEntityService.ts b/packages/backend/src/core/entities/NotificationEntityService.ts index be88a213f4..33c76c6937 100644 --- a/packages/backend/src/core/entities/NotificationEntityService.ts +++ b/packages/backend/src/core/entities/NotificationEntityService.ts @@ -1,21 +1,19 @@ import { Inject, Injectable } from '@nestjs/common'; +import { In } from 'typeorm'; import { ModuleRef } from '@nestjs/core'; import { DI } from '@/di-symbols.js'; import type { AccessTokensRepository, NoteReactionsRepository, NotificationsRepository, User } from '@/models/index.js'; import { awaitAll } from '@/misc/prelude/await-all.js'; import type { Notification } from '@/models/entities/Notification.js'; +import type { NoteReaction } from '@/models/entities/NoteReaction.js'; import type { Note } from '@/models/entities/Note.js'; import type { Packed } from '@/misc/schema.js'; import { bindThis } from '@/decorators.js'; -import { isNotNull } from '@/misc/is-not-null.js'; -import { notificationTypes } from '@/types.js'; import type { OnModuleInit } from '@nestjs/common'; import type { CustomEmojiService } from '../CustomEmojiService.js'; import type { UserEntityService } from './UserEntityService.js'; import type { NoteEntityService } from './NoteEntityService.js'; -const NOTE_REQUIRED_NOTIFICATION_TYPES = new Set(['mention', 'reply', 'renote', 'quote', 'reaction', 'pollEnded'] as (typeof notificationTypes[number])[]); - @Injectable() export class NotificationEntityService implements OnModuleInit { private userEntityService: UserEntityService; @@ -50,20 +48,13 @@ export class NotificationEntityService implements OnModuleInit { public async pack( src: Notification['id'] | Notification, options: { - _hint_?: { - packedNotes: Map>; + _hintForEachNotes_?: { + myReactions: Map; }; }, ): Promise> { const notification = typeof src === 'object' ? src : await this.notificationsRepository.findOneByOrFail({ id: src }); const token = notification.appAccessTokenId ? await this.accessTokensRepository.findOneByOrFail({ id: notification.appAccessTokenId }) : null; - const noteIfNeed = NOTE_REQUIRED_NOTIFICATION_TYPES.has(notification.type) && notification.noteId != null ? ( - options._hint_?.packedNotes != null - ? options._hint_.packedNotes.get(notification.noteId) - : this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { - detail: true, - }) - ) : undefined; return await awaitAll({ id: notification.id, @@ -72,10 +63,43 @@ export class NotificationEntityService implements OnModuleInit { isRead: notification.isRead, userId: notification.notifierId, user: notification.notifierId ? this.userEntityService.pack(notification.notifier ?? notification.notifierId) : null, - ...(noteIfNeed != null ? { note: noteIfNeed } : {}), + ...(notification.type === 'mention' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), + } : {}), + ...(notification.type === 'reply' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), + } : {}), + ...(notification.type === 'renote' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), + } : {}), + ...(notification.type === 'quote' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), + } : {}), ...(notification.type === 'reaction' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), reaction: notification.reaction, } : {}), + ...(notification.type === 'pollEnded' ? { + note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, { + detail: true, + _hint_: options._hintForEachNotes_, + }), + } : {}), ...(notification.type === 'achievementEarned' ? { achievement: notification.achievement, } : {}), @@ -87,32 +111,32 @@ export class NotificationEntityService implements OnModuleInit { }); } - /** - * @param notifications you should join "note" property when fetch from DB, and all notifieeId should be same as meId - */ @bindThis public async packMany( notifications: Notification[], meId: User['id'], ) { if (notifications.length === 0) return []; - - for (const notification of notifications) { - if (meId !== notification.notifieeId) { - // because we call note packMany with meId, all notifieeId should be same as meId - throw new Error('TRY_TO_PACK_ANOTHER_USER_NOTIFICATION'); - } + + const notes = notifications.filter(x => x.note != null).map(x => x.note!); + const noteIds = notes.map(n => n.id); + const myReactionsMap = new Map(); + const renoteIds = notes.filter(n => n.renoteId != null).map(n => n.renoteId!); + const targets = [...noteIds, ...renoteIds]; + const myReactions = await this.noteReactionsRepository.findBy({ + userId: meId, + noteId: In(targets), + }); + + for (const target of targets) { + myReactionsMap.set(target, myReactions.find(reaction => reaction.noteId === target) ?? null); } - const notes = notifications.map(x => x.note).filter(isNotNull); - const packedNotesArray = await this.noteEntityService.packMany(notes, { id: meId }, { - detail: true, - }); - const packedNotes = new Map(packedNotesArray.map(p => [p.id, p])); + await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes)); return await Promise.all(notifications.map(x => this.pack(x, { - _hint_: { - packedNotes, + _hintForEachNotes_: { + myReactions: myReactionsMap, }, }))); } diff --git a/packages/backend/src/misc/is-not-null.ts b/packages/backend/src/misc/is-not-null.ts deleted file mode 100644 index d89a1957be..0000000000 --- a/packages/backend/src/misc/is-not-null.ts +++ /dev/null @@ -1,5 +0,0 @@ -// we are using {} as "any non-nullish value" as expected -// eslint-disable-next-line @typescript-eslint/ban-types -export function isNotNull(input: T | undefined | null): input is T { - return input != null; -}