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

This reverts commit 452a48e7f4.
This commit is contained in:
syuilo 2023-03-03 20:26:44 +09:00
parent da3fcf178e
commit a7c82eeabc
5 changed files with 58 additions and 66 deletions

View File

@ -1,5 +1,5 @@
import { forwardRef, Inject, Injectable } from '@nestjs/common'; import { forwardRef, Inject, Injectable } from '@nestjs/common';
import { DataSource, In } from 'typeorm'; import { DataSource } from 'typeorm';
import { DI } from '@/di-symbols.js'; import { DI } from '@/di-symbols.js';
import type { NotesRepository, DriveFilesRepository } from '@/models/index.js'; import type { NotesRepository, DriveFilesRepository } from '@/models/index.js';
import type { Config } from '@/config.js'; import type { Config } from '@/config.js';
@ -21,7 +21,6 @@ type PackOptions = {
}; };
import { bindThis } from '@/decorators.js'; import { bindThis } from '@/decorators.js';
import { isMimeImage } from '@/misc/is-mime-image.js'; import { isMimeImage } from '@/misc/is-mime-image.js';
import { isNotNull } from '@/misc/is-not-null.js';
@Injectable() @Injectable()
export class DriveFileEntityService { export class DriveFileEntityService {
@ -256,29 +255,10 @@ export class DriveFileEntityService {
@bindThis @bindThis
public async packMany( public async packMany(
files: DriveFile[], files: (DriveFile['id'] | DriveFile)[],
options?: PackOptions, options?: PackOptions,
): Promise<Packed<'DriveFile'>[]> { ): Promise<Packed<'DriveFile'>[]> {
const items = await Promise.all(files.map(f => this.packNullable(f, options))); const items = await Promise.all(files.map(f => this.packNullable(f, options)));
return items.filter((x): x is Packed<'DriveFile'> => x != null); return items.filter((x): x is Packed<'DriveFile'> => x != null);
} }
@bindThis
public async packManyByIdsMap(
fileIds: DriveFile['id'][],
options?: PackOptions,
): Promise<Map<Packed<'DriveFile'>['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<Packed<'DriveFile'>[]> {
const filesMap = await this.packManyByIdsMap(fileIds, options);
return fileIds.map(id => filesMap.get(id)).filter(isNotNull);
}
} }

View File

@ -41,8 +41,7 @@ export class GalleryPostEntityService {
title: post.title, title: post.title,
description: post.description, description: post.description,
fileIds: post.fileIds, fileIds: post.fileIds,
// TODO: packMany causes N+1 queries files: this.driveFileEntityService.packMany(post.fileIds),
files: this.driveFileEntityService.packManyByIds(post.fileIds),
tags: post.tags.length > 0 ? post.tags : undefined, tags: post.tags.length > 0 ? post.tags : undefined,
isSensitive: post.isSensitive, isSensitive: post.isSensitive,
likedCount: post.likedCount, likedCount: post.likedCount,

View File

@ -11,7 +11,6 @@ import type { Note } from '@/models/entities/Note.js';
import type { NoteReaction } from '@/models/entities/NoteReaction.js'; import type { NoteReaction } from '@/models/entities/NoteReaction.js';
import type { UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, DriveFilesRepository } from '@/models/index.js'; import type { UsersRepository, NotesRepository, FollowingsRepository, PollsRepository, PollVotesRepository, NoteReactionsRepository, ChannelsRepository, DriveFilesRepository } from '@/models/index.js';
import { bindThis } from '@/decorators.js'; import { bindThis } from '@/decorators.js';
import { isNotNull } from '@/misc/is-not-null.js';
import type { OnModuleInit } from '@nestjs/common'; import type { OnModuleInit } from '@nestjs/common';
import type { CustomEmojiService } from '../CustomEmojiService.js'; import type { CustomEmojiService } from '../CustomEmojiService.js';
import type { ReactionService } from '../ReactionService.js'; import type { ReactionService } from '../ReactionService.js';
@ -258,7 +257,6 @@ export class NoteEntityService implements OnModuleInit {
skipHide?: boolean; skipHide?: boolean;
_hint_?: { _hint_?: {
myReactions: Map<Note['id'], NoteReaction | null>; myReactions: Map<Note['id'], NoteReaction | null>;
packedFiles: Map<Note['fileIds'][number], Packed<'DriveFile'>>;
}; };
}, },
): Promise<Packed<'Note'>> { ): Promise<Packed<'Note'>> {
@ -286,7 +284,6 @@ export class NoteEntityService implements OnModuleInit {
const reactionEmojiNames = Object.keys(note.reactions) const reactionEmojiNames = Object.keys(note.reactions)
.filter(x => x.startsWith(':') && x.includes('@') && !x.includes('@.')) // リモートカスタム絵文字のみ .filter(x => x.startsWith(':') && x.includes('@') && !x.includes('@.')) // リモートカスタム絵文字のみ
.map(x => this.reactionService.decodeReaction(x).reaction.replaceAll(':', '')); .map(x => this.reactionService.decodeReaction(x).reaction.replaceAll(':', ''));
const packedFiles = options?._hint_?.packedFiles;
const packed: Packed<'Note'> = await awaitAll({ const packed: Packed<'Note'> = await awaitAll({
id: note.id, id: note.id,
@ -307,7 +304,7 @@ export class NoteEntityService implements OnModuleInit {
emojis: host != null ? this.customEmojiService.populateEmojis(note.emojis, host) : undefined, emojis: host != null ? this.customEmojiService.populateEmojis(note.emojis, host) : undefined,
tags: note.tags.length > 0 ? note.tags : undefined, tags: note.tags.length > 0 ? note.tags : undefined,
fileIds: note.fileIds, 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, replyId: note.replyId,
renoteId: note.renoteId, renoteId: note.renoteId,
channelId: note.channelId ?? undefined, channelId: note.channelId ?? undefined,
@ -391,14 +388,11 @@ export class NoteEntityService implements OnModuleInit {
} }
await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes)); 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, { return await Promise.all(notes.map(n => this.pack(n, me, {
...options, ...options,
_hint_: { _hint_: {
myReactions: myReactionsMap, myReactions: myReactionsMap,
packedFiles,
}, },
}))); })));
} }

View File

@ -1,21 +1,19 @@
import { Inject, Injectable } from '@nestjs/common'; import { Inject, Injectable } from '@nestjs/common';
import { In } from 'typeorm';
import { ModuleRef } from '@nestjs/core'; import { ModuleRef } from '@nestjs/core';
import { DI } from '@/di-symbols.js'; import { DI } from '@/di-symbols.js';
import type { AccessTokensRepository, NoteReactionsRepository, NotificationsRepository, User } from '@/models/index.js'; import type { AccessTokensRepository, NoteReactionsRepository, NotificationsRepository, User } from '@/models/index.js';
import { awaitAll } from '@/misc/prelude/await-all.js'; import { awaitAll } from '@/misc/prelude/await-all.js';
import type { Notification } from '@/models/entities/Notification.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 { Note } from '@/models/entities/Note.js';
import type { Packed } from '@/misc/schema.js'; import type { Packed } from '@/misc/schema.js';
import { bindThis } from '@/decorators.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 { OnModuleInit } from '@nestjs/common';
import type { CustomEmojiService } from '../CustomEmojiService.js'; import type { CustomEmojiService } from '../CustomEmojiService.js';
import type { UserEntityService } from './UserEntityService.js'; import type { UserEntityService } from './UserEntityService.js';
import type { NoteEntityService } from './NoteEntityService.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() @Injectable()
export class NotificationEntityService implements OnModuleInit { export class NotificationEntityService implements OnModuleInit {
private userEntityService: UserEntityService; private userEntityService: UserEntityService;
@ -50,20 +48,13 @@ export class NotificationEntityService implements OnModuleInit {
public async pack( public async pack(
src: Notification['id'] | Notification, src: Notification['id'] | Notification,
options: { options: {
_hint_?: { _hintForEachNotes_?: {
packedNotes: Map<Note['id'], Packed<'Note'>>; myReactions: Map<Note['id'], NoteReaction | null>;
}; };
}, },
): Promise<Packed<'Notification'>> { ): Promise<Packed<'Notification'>> {
const notification = typeof src === 'object' ? src : await this.notificationsRepository.findOneByOrFail({ id: src }); 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 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({ return await awaitAll({
id: notification.id, id: notification.id,
@ -72,10 +63,43 @@ export class NotificationEntityService implements OnModuleInit {
isRead: notification.isRead, isRead: notification.isRead,
userId: notification.notifierId, userId: notification.notifierId,
user: notification.notifierId ? this.userEntityService.pack(notification.notifier ?? notification.notifierId) : null, 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' ? { ...(notification.type === 'reaction' ? {
note: this.noteEntityService.pack(notification.note ?? notification.noteId!, { id: notification.notifieeId }, {
detail: true,
_hint_: options._hintForEachNotes_,
}),
reaction: notification.reaction, 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' ? { ...(notification.type === 'achievementEarned' ? {
achievement: notification.achievement, 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 @bindThis
public async packMany( public async packMany(
notifications: Notification[], notifications: Notification[],
meId: User['id'], meId: User['id'],
) { ) {
if (notifications.length === 0) return []; if (notifications.length === 0) return [];
for (const notification of notifications) { const notes = notifications.filter(x => x.note != null).map(x => x.note!);
if (meId !== notification.notifieeId) { const noteIds = notes.map(n => n.id);
// because we call note packMany with meId, all notifieeId should be same as meId const myReactionsMap = new Map<Note['id'], NoteReaction | null>();
throw new Error('TRY_TO_PACK_ANOTHER_USER_NOTIFICATION'); 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); await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes));
const packedNotesArray = await this.noteEntityService.packMany(notes, { id: meId }, {
detail: true,
});
const packedNotes = new Map(packedNotesArray.map(p => [p.id, p]));
return await Promise.all(notifications.map(x => this.pack(x, { return await Promise.all(notifications.map(x => this.pack(x, {
_hint_: { _hintForEachNotes_: {
packedNotes, myReactions: myReactionsMap,
}, },
}))); })));
} }

View File

@ -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<T extends {}>(input: T | undefined | null): input is T {
return input != null;
}