From b44d2389cf8fa742ed6ccbeedce1be6d671ef695 Mon Sep 17 00:00:00 2001 From: Diego Sampaio Date: Tue, 5 Dec 2023 16:30:55 -0300 Subject: [PATCH] do less round trips to db --- .../messages/hooks/BeforeSaveJumpToMessage.ts | 103 +++++++++++------- .../server/services/messages/service.ts | 9 +- .../hooks/BeforeSaveJumpToMessage.tests.ts | 91 ++++++++-------- 3 files changed, 114 insertions(+), 89 deletions(-) diff --git a/apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts b/apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts index 4de721e7a3277..c88a27cb2b73f 100644 --- a/apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts +++ b/apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts @@ -33,8 +33,8 @@ const validateAttachmentDeepness = (message: IMessage, quoteChainLimit: number): }; type JumpToMessageInit = { - getMessage(messageId: IMessage['_id']): Promise; - getRoom(roomId: IRoom['_id']): Promise; + getMessages(messageIds: IMessage['_id'][]): Promise; + getRooms(roomIds: IRoom['_id'][]): Promise; canAccessRoom(room: IRoom, user: Pick): Promise; getUserAvatarURL(user?: string): string; }; @@ -43,17 +43,17 @@ type JumpToMessageInit = { * Transform URLs in messages into quote attachments */ export class BeforeSaveJumpToMessage { - private getMessage: JumpToMessageInit['getMessage']; + private getMessages: JumpToMessageInit['getMessages']; - private getRoom: JumpToMessageInit['getRoom']; + private getRooms: JumpToMessageInit['getRooms']; private canAccessRoom: JumpToMessageInit['canAccessRoom']; private getUserAvatarURL: JumpToMessageInit['getUserAvatarURL']; constructor(options: JumpToMessageInit) { - this.getMessage = options.getMessage; - this.getRoom = options.getRoom; + this.getMessages = options.getMessages; + this.getRooms = options.getRooms; this.canAccessRoom = options.canAccessRoom; this.getUserAvatarURL = options.getUserAvatarURL; } @@ -76,58 +76,85 @@ export class BeforeSaveJumpToMessage { return message; } - for await (const item of message.urls) { - // if the URL doesn't belong to the current server, skip - if (!item.url.includes(config.siteUrl)) { - continue; - } + const linkedMessages = message.urls + .filter((item) => item.url.includes(config.siteUrl)) + .map((item) => { + const urlObj = URL.parse(item.url); - const urlObj = URL.parse(item.url); + // if the URL doesn't have query params (doesn't reference message) skip + if (!urlObj.query) { + return; + } - // if the URL doesn't have query params (doesn't reference message) skip - if (!urlObj.query) { - continue; - } + const { msg: msgId } = QueryString.parse(urlObj.query); + + if (typeof msgId !== 'string') { + return; + } + + return { msgId, url: item.url }; + }) + .filter(Boolean); + + const msgs = await this.getMessages(linkedMessages.map((linkedMsg) => linkedMsg?.msgId) as string[]); + + const validMessages = msgs.filter((msg) => validateAttachmentDeepness(msg, config.chainLimit)); + + const rooms = await this.getRooms(validMessages.map((msg) => msg.rid)); + + const roomsWithPermission = + rooms && + (await Promise.all( + rooms.map(async (room) => { + if (!!message.token && isOmnichannelRoom(room) && !!room.v?.token && message.token === room.v.token) { + return room; + } - const { msg: msgId } = QueryString.parse(urlObj.query); + if (currentUser && (await this.canAccessRoom(room, currentUser))) { + return room; + } + }), + )); - if (typeof msgId !== 'string') { + const validRooms = roomsWithPermission?.filter((room) => !!room); + + const { useRealName } = config; + + const quotes = []; + + for (const item of message.urls) { + if (!item.url.includes(config.siteUrl)) { continue; } - const messageFromUrl = await this.getMessage(msgId); - - const jumpToMessage = messageFromUrl && validateAttachmentDeepness(messageFromUrl, config.chainLimit); - if (!jumpToMessage) { + const linkedMessage = linkedMessages.find((msg) => msg?.url === item.url); + if (!linkedMessage) { continue; } - // validates if user can see the message - // user has to belong to the room the message was first wrote in - const room = await this.getRoom(jumpToMessage.rid); - if (!room) { + const messageFromUrl = validMessages.find((msg) => msg._id === linkedMessage.msgId); + if (!messageFromUrl) { continue; } - const isLiveChatRoomVisitor = !!message.token && isOmnichannelRoom(room) && !!room.v?.token && message.token === room.v.token; - const canAccessRoomForUser = isLiveChatRoomVisitor || (currentUser && (await this.canAccessRoom(room, currentUser))); - if (!canAccessRoomForUser) { + if (!validRooms?.find((room) => room?._id === messageFromUrl.rid)) { continue; } - message.attachments = message.attachments || []; + item.ignoreParse = true; + // Only QuoteAttachments have "message_link" property - const index = message.attachments.findIndex((a) => isQuoteAttachment(a) && a.message_link === item.url); - if (index > -1) { - message.attachments.splice(index, 1); + const index = message.attachments?.findIndex((a) => isQuoteAttachment(a) && a.message_link === item.url); + if (index !== undefined && index > -1) { + message.attachments?.splice(index, 1); } - const { useRealName } = config; + quotes.push(createQuoteAttachment(messageFromUrl, item.url, useRealName, this.getUserAvatarURL(messageFromUrl.u.username))); + } - message.attachments.push( - createQuoteAttachment(jumpToMessage, item.url, useRealName, this.getUserAvatarURL(jumpToMessage.u.username)), - ); - item.ignoreParse = true; + if (quotes.length > 0) { + message.attachments = message.attachments || []; + message.attachments.push(...quotes); } return message; diff --git a/apps/meteor/server/services/messages/service.ts b/apps/meteor/server/services/messages/service.ts index 4cf95ab3746c2..84e222fec53f7 100644 --- a/apps/meteor/server/services/messages/service.ts +++ b/apps/meteor/server/services/messages/service.ts @@ -1,6 +1,5 @@ import type { IMessageService } from '@rocket.chat/core-services'; import { Authorization, ServiceClassInternal } from '@rocket.chat/core-services'; -import type { IOmnichannelRoom } from '@rocket.chat/core-typings'; import { type IMessage, type MessageTypesValues, type IUser, type IRoom, isEditedMessage } from '@rocket.chat/core-typings'; import { Messages, Rooms } from '@rocket.chat/models'; @@ -33,11 +32,11 @@ export class MessageService extends ServiceClassInternal implements IMessageServ this.badWords = new BeforeSaveBadWords(); this.spotify = new BeforeSaveSpotify(); this.jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage(messageId: IMessage['_id']): Promise { - return Messages.findOneById(messageId); + getMessages(messageIds) { + return Messages.findVisibleByIds(messageIds).toArray(); }, - getRoom(roomId: IRoom['_id']): Promise { - return Rooms.findOneById(roomId); + getRooms(roomIds) { + return Rooms.findByIds(roomIds).toArray(); }, canAccessRoom(room: IRoom, user: IUser): Promise { return Authorization.canAccessRoom(room, user); diff --git a/apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts b/apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts index ba10f9ef05371..5e5a26b6b268c 100644 --- a/apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts +++ b/apps/meteor/tests/unit/server/services/messages/hooks/BeforeSaveJumpToMessage.tests.ts @@ -51,8 +51,8 @@ const countDeep = (msg: any, deep = 1): number => { describe('Create attachments for message URLs', () => { it('should return message without attatchment and URLs if no URL provided', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -72,8 +72,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if URL is not from SiteUrl', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -94,8 +94,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if URL is from SiteUrl but not have a query string', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -116,8 +116,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if URL is from SiteUrl but not have a msgId query string', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -138,8 +138,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if it do not find a msg from the URL', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => null, - getRoom: async () => createRoom(), + getMessages: async () => [], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -160,8 +160,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if it cannot find the room of the message from the URL', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => null, + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -182,8 +182,8 @@ describe('Create attachments for message URLs', () => { it('should do nothing if user dont have access to the room of the message from the URL', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => false, getUserAvatarURL: () => 'url', }); @@ -204,21 +204,21 @@ describe('Create attachments for message URLs', () => { it('should remove other attachments from the message if message_link is the same as the URL', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); const message = await jumpToMessage.createAttachmentForMessageURLs({ message: createMessage('hey', { - urls: [{ url: 'https://open.rocket.chat/linked?msg=linkedMsgId' }], + urls: [{ url: 'https://open.rocket.chat/linked?msg=linked' }], attachments: [ { text: 'old attachment', author_name: 'username', author_icon: 'url', - message_link: 'https://open.rocket.chat/linked?msg=linkedMsgId', + message_link: 'https://open.rocket.chat/linked?msg=linked', ts: new Date(), }, ], @@ -236,7 +236,7 @@ describe('Create attachments for message URLs', () => { const [url] = message.urls ?? []; expect(url).to.include({ - url: 'https://open.rocket.chat/linked?msg=linkedMsgId', + url: 'https://open.rocket.chat/linked?msg=linked', ignoreParse: true, }); @@ -249,14 +249,14 @@ describe('Create attachments for message URLs', () => { it('should return an attachment with the message content if a message URL is provided', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom(), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); const message = await jumpToMessage.createAttachmentForMessageURLs({ - message: createMessage('hey', { urls: [{ url: 'https://open.rocket.chat/linked?msg=linkedMsgId' }] }), + message: createMessage('hey', { urls: [{ url: 'https://open.rocket.chat/linked?msg=linked' }] }), user: createUser(), config: { chainLimit: 10, @@ -270,7 +270,7 @@ describe('Create attachments for message URLs', () => { const [url] = message.urls ?? []; expect(url).to.include({ - url: 'https://open.rocket.chat/linked?msg=linkedMsgId', + url: 'https://open.rocket.chat/linked?msg=linked', ignoreParse: true, }); @@ -283,7 +283,7 @@ describe('Create attachments for message URLs', () => { it('should respect chain limit config', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => + getMessages: async () => [ createMessage('linked message', { _id: 'linked', attachments: [ @@ -314,14 +314,15 @@ describe('Create attachments for message URLs', () => { }, ], }), - getRoom: async () => createRoom(), + ], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); const message = await jumpToMessage.createAttachmentForMessageURLs({ message: createMessage('hey', { - urls: [{ url: 'https://open.rocket.chat/linked?msg=linkedMsgId' }], + urls: [{ url: 'https://open.rocket.chat/linked?msg=linked' }], }), user: createUser(), config: { @@ -340,15 +341,15 @@ describe('Create attachments for message URLs', () => { it('should create the attachment if cannot access room but message has a livechat token', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom({ t: 'l', v: { token: 'livechatToken' } }), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom({ t: 'l', v: { token: 'livechatToken' } })], canAccessRoom: async () => false, getUserAvatarURL: () => 'url', }); const message = await jumpToMessage.createAttachmentForMessageURLs({ message: createMessage('hey', { - urls: [{ url: 'https://open.rocket.chat/linked?msg=linkedMsgId' }], + urls: [{ url: 'https://open.rocket.chat/linked?msg=linked' }], token: 'livechatToken', }), user: createUser(), @@ -364,7 +365,7 @@ describe('Create attachments for message URLs', () => { const [url] = message.urls ?? []; expect(url).to.include({ - url: 'https://open.rocket.chat/linked?msg=linkedMsgId', + url: 'https://open.rocket.chat/linked?msg=linked', ignoreParse: true, }); @@ -377,15 +378,15 @@ describe('Create attachments for message URLs', () => { it('should do nothing if cannot access room but message has a livechat token but is not from the room does not have a token', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => createMessage('linked message', { _id: 'linked' }), - getRoom: async () => createRoom({ t: 'l' }), + getMessages: async () => [createMessage('linked message', { _id: 'linked' })], + getRooms: async () => [createRoom({ t: 'l' })], canAccessRoom: async () => false, getUserAvatarURL: () => 'url', }); const message = await jumpToMessage.createAttachmentForMessageURLs({ message: createMessage('hey', { - urls: [{ url: 'https://open.rocket.chat/linked?msg=linkedMsgId' }], + urls: [{ url: 'https://open.rocket.chat/linked?msg=linked' }], token: 'another-token', }), user: createUser(), @@ -402,7 +403,7 @@ describe('Create attachments for message URLs', () => { it('should remove the clean up the attachments of the quoted message property if chain limit < 2', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async () => + getMessages: async () => [ createMessage('linked message', { _id: 'linkedMsgId', attachments: [ @@ -414,7 +415,8 @@ describe('Create attachments for message URLs', () => { }, ], }), - getRoom: async () => createRoom(), + ], + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', }); @@ -450,20 +452,17 @@ describe('Create attachments for message URLs', () => { it('should work for multiple URLs', async () => { const jumpToMessage = new BeforeSaveJumpToMessage({ - getMessage: async (messageId) => { - if (messageId === 'msg1') { - return createMessage('first message', { + getMessages: async () => { + return [ + createMessage('first message', { _id: 'msg1', - }); - } - - if (messageId === 'msg2') { - return createMessage('second message', { + }), + createMessage('second message', { _id: 'msg2', - }); - } + }), + ]; }, - getRoom: async () => createRoom(), + getRooms: async () => [createRoom()], canAccessRoom: async () => true, getUserAvatarURL: () => 'url', });