From 49f0837729ab094d2f7646c77ff7ba16a39430c0 Mon Sep 17 00:00:00 2001 From: rinsuki <428rinsuki+git@gmail.com> Date: Sat, 4 Mar 2023 16:48:50 +0900 Subject: [PATCH] fix(server): DriveFile related N+1 query when call note packMany (again) (#10190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Revert "Revert "fix(server): DriveFile related N+1 query when call note packMany (#10133)"" This reverts commit a7c82eeabcc732e76c5c358c98812cac8457d57f. * packManyByIdsMap: 存在チェックをしてなかったものは null を入れるように * Note.packMany で reply とか renote がもうあったらそのファイルも引く * テストを書く * fix test * fix test * fix test * fix test --- .../core/entities/DriveFileEntityService.ts | 28 ++++- .../core/entities/GalleryPostEntityService.ts | 3 +- .../src/core/entities/NoteEntityService.ts | 24 +++- packages/backend/test/e2e/note.ts | 118 +++++++++++++++++- 4 files changed, 168 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/core/entities/DriveFileEntityService.ts b/packages/backend/src/core/entities/DriveFileEntityService.ts index 158fafa9d5..f769ddd5e9 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 } from 'typeorm'; +import { DataSource, In } from 'typeorm'; import { DI } from '@/di-symbols.js'; import type { NotesRepository, DriveFilesRepository } from '@/models/index.js'; import type { Config } from '@/config.js'; @@ -21,6 +21,7 @@ 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 { @@ -255,10 +256,33 @@ export class DriveFileEntityService { @bindThis public async packMany( - files: (DriveFile['id'] | DriveFile)[], + files: 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'> | null>> { + const files = await this.driveFilesRepository.findBy({ id: In(fileIds) }); + const packedFiles = await this.packMany(files, options); + const map = new Map['id'], Packed<'DriveFile'> | null>(packedFiles.map(f => [f.id, f])); + for (const id of fileIds) { + if (!map.has(id)) map.set(id, null); + } + return map; + } + + @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 ab29e7dba1..fb147ae181 100644 --- a/packages/backend/src/core/entities/GalleryPostEntityService.ts +++ b/packages/backend/src/core/entities/GalleryPostEntityService.ts @@ -41,7 +41,8 @@ export class GalleryPostEntityService { title: post.title, description: post.description, fileIds: post.fileIds, - files: this.driveFileEntityService.packMany(post.fileIds), + // TODO: packMany causes N+1 queries + files: this.driveFileEntityService.packManyByIds(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 2ffe5f1c21..4ec10df9a6 100644 --- a/packages/backend/src/core/entities/NoteEntityService.ts +++ b/packages/backend/src/core/entities/NoteEntityService.ts @@ -11,6 +11,7 @@ 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'; @@ -248,6 +249,21 @@ export class NoteEntityService implements OnModuleInit { return true; } + @bindThis + public async packAttachedFiles(fileIds: Note['fileIds'], packedFiles: Map | null>): Promise[]> { + const missingIds = []; + for (const id of fileIds) { + if (!packedFiles.has(id)) missingIds.push(id); + } + if (missingIds.length) { + const additionalMap = await this.driveFileEntityService.packManyByIdsMap(missingIds); + for (const [k, v] of additionalMap) { + packedFiles.set(k, v); + } + } + return fileIds.map(id => packedFiles.get(id)).filter(isNotNull); + } + @bindThis public async pack( src: Note['id'] | Note, @@ -257,6 +273,7 @@ export class NoteEntityService implements OnModuleInit { skipHide?: boolean; _hint_?: { myReactions: Map; + packedFiles: Map | null>; }; }, ): Promise> { @@ -284,6 +301,7 @@ 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, @@ -304,7 +322,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: this.driveFileEntityService.packMany(note.fileIds), + files: packedFiles != null ? this.packAttachedFiles(note.fileIds, packedFiles) : this.driveFileEntityService.packManyByIds(note.fileIds), replyId: note.replyId, renoteId: note.renoteId, channelId: note.channelId ?? undefined, @@ -388,11 +406,15 @@ export class NoteEntityService implements OnModuleInit { } await this.customEmojiService.prefetchEmojis(this.customEmojiService.aggregateNoteEmojis(notes)); + // TODO: 本当は renote とか reply がないのに renoteId とか replyId があったらここで解決しておく + const fileIds = notes.map(n => [n.fileIds, n.renote?.fileIds, n.reply?.fileIds]).flat(2).filter(isNotNull); + 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/test/e2e/note.ts b/packages/backend/test/e2e/note.ts index 98ee34d8d1..1b5f9580d5 100644 --- a/packages/backend/test/e2e/note.ts +++ b/packages/backend/test/e2e/note.ts @@ -2,7 +2,7 @@ process.env.NODE_ENV = 'test'; import * as assert from 'assert'; import { Note } from '@/models/entities/Note.js'; -import { signup, post, uploadUrl, startServer, initTestDb, api } from '../utils.js'; +import { signup, post, uploadUrl, startServer, initTestDb, api, uploadFile } from '../utils.js'; import type { INestApplicationContext } from '@nestjs/common'; describe('Note', () => { @@ -213,6 +213,122 @@ describe('Note', () => { assert.deepStrictEqual(noteDoc.mentions, [bob.id]); }); + describe('添付ファイル情報', () => { + test('ファイルを添付した場合、投稿成功時にファイル情報入りのレスポンスが帰ってくる', async () => { + const file = await uploadFile(alice); + const res = await api('/notes/create', { + fileIds: [file.body.id], + }, alice); + + assert.strictEqual(res.status, 200); + assert.strictEqual(typeof res.body === 'object' && !Array.isArray(res.body), true); + assert.strictEqual(res.body.createdNote.files.length, 1); + assert.strictEqual(res.body.createdNote.files[0].id, file.body.id); + }); + + test('ファイルを添付した場合、タイムラインでファイル情報入りのレスポンスが帰ってくる', async () => { + const file = await uploadFile(alice); + const createdNote = await api('/notes/create', { + fileIds: [file.body.id], + }, alice); + + assert.strictEqual(createdNote.status, 200); + + const res = await api('/notes', { + withFiles: true, + }, alice); + + assert.strictEqual(res.status, 200); + assert.strictEqual(Array.isArray(res.body), true); + const myNote = res.body.find((note: { id: string; files: { id: string }[] }) => note.id === createdNote.body.createdNote.id); + assert.notEqual(myNote, null); + assert.strictEqual(myNote.files.length, 1); + assert.strictEqual(myNote.files[0].id, file.body.id); + }); + + test('ファイルが添付されたノートをリノートした場合、タイムラインでファイル情報入りのレスポンスが帰ってくる', async () => { + const file = await uploadFile(alice); + const createdNote = await api('/notes/create', { + fileIds: [file.body.id], + }, alice); + + assert.strictEqual(createdNote.status, 200); + + const renoted = await api('/notes/create', { + renoteId: createdNote.body.createdNote.id, + }, alice); + assert.strictEqual(renoted.status, 200); + + const res = await api('/notes', { + renote: true, + }, alice); + + assert.strictEqual(res.status, 200); + assert.strictEqual(Array.isArray(res.body), true); + const myNote = res.body.find((note: { id: string }) => note.id === renoted.body.createdNote.id); + assert.notEqual(myNote, null); + assert.strictEqual(myNote.renote.files.length, 1); + assert.strictEqual(myNote.renote.files[0].id, file.body.id); + }); + + test('ファイルが添付されたノートに返信した場合、タイムラインでファイル情報入りのレスポンスが帰ってくる', async () => { + const file = await uploadFile(alice); + const createdNote = await api('/notes/create', { + fileIds: [file.body.id], + }, alice); + + assert.strictEqual(createdNote.status, 200); + + const reply = await api('/notes/create', { + replyId: createdNote.body.createdNote.id, + text: 'this is reply', + }, alice); + assert.strictEqual(reply.status, 200); + + const res = await api('/notes', { + reply: true, + }, alice); + + assert.strictEqual(res.status, 200); + assert.strictEqual(Array.isArray(res.body), true); + const myNote = res.body.find((note: { id: string }) => note.id === reply.body.createdNote.id); + assert.notEqual(myNote, null); + assert.strictEqual(myNote.reply.files.length, 1); + assert.strictEqual(myNote.reply.files[0].id, file.body.id); + }); + + test('ファイルが添付されたノートへの返信をリノートした場合、タイムラインでファイル情報入りのレスポンスが帰ってくる', async () => { + const file = await uploadFile(alice); + const createdNote = await api('/notes/create', { + fileIds: [file.body.id], + }, alice); + + assert.strictEqual(createdNote.status, 200); + + const reply = await api('/notes/create', { + replyId: createdNote.body.createdNote.id, + text: 'this is reply', + }, alice); + assert.strictEqual(reply.status, 200); + + const renoted = await api('/notes/create', { + renoteId: reply.body.createdNote.id, + }, alice); + assert.strictEqual(renoted.status, 200); + + const res = await api('/notes', { + renote: true, + }, alice); + + assert.strictEqual(res.status, 200); + assert.strictEqual(Array.isArray(res.body), true); + const myNote = res.body.find((note: { id: string }) => note.id === renoted.body.createdNote.id); + assert.notEqual(myNote, null); + assert.strictEqual(myNote.renote.reply.files.length, 1); + assert.strictEqual(myNote.renote.reply.files[0].id, file.body.id); + }); + }); + describe('notes/create', () => { test('投票を添付できる', async () => { const res = await api('/notes/create', {