From 24e68821cc36c4d7c78b570c63ea4c62d12a2de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=83=87=E3=83=AF=E3=83=B3=E3=82=B7=E3=83=A5?= <61188295+Dnouv@users.noreply.github.com> Date: Tue, 16 Apr 2024 04:45:38 +0530 Subject: [PATCH 1/6] feat: Implement the Apps Engine Room message read bridge (#753) --- src/definition/accessors/IRoomRead.ts | 19 +++++++++++++++---- src/server/accessors/RoomRead.ts | 11 +++++++++-- src/server/bridges/RoomBridge.ts | 24 ++++++++++++++++++++++++ tests/server/accessors/RoomRead.spec.ts | 11 ++++++++++- tests/test-data/bridges/roomBridge.ts | 4 ++++ 5 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/definition/accessors/IRoomRead.ts b/src/definition/accessors/IRoomRead.ts index 58e0583c7..4e827507d 100644 --- a/src/definition/accessors/IRoomRead.ts +++ b/src/definition/accessors/IRoomRead.ts @@ -40,12 +40,23 @@ export interface IRoomRead { getCreatorUserByName(name: string): Promise; /** - * Gets an iterator for all of the messages in the provided room. + * Retrieves an array of messages from the specified room. * - * @param roomId the room's id - * @returns an iterator for messages + * @param roomId The unique identifier of the room from which to retrieve messages. + * @param options Optional parameters for retrieving messages: + * - limit: The maximum number of messages to retrieve. If more than 100 is passed, it defaults to 100. + * - skip: The number of messages to skip (for pagination). + * - sort: An object defining the sorting order of the messages. Each key is a field to sort by, and the value is either 1 for ascending order or -1 for descending order. + * @returns A Promise that resolves to an array of IMessage objects representing the messages in the room. */ - getMessages(roomId: string): Promise>; + getMessages( + roomId: string, + options?: Partial<{ + limit: number; + skip: number; + sort: Record; + }>, + ): Promise; /** * Gets an iterator for all of the users in the provided room. diff --git a/src/server/accessors/RoomRead.ts b/src/server/accessors/RoomRead.ts index da4273f53..d79a64028 100644 --- a/src/server/accessors/RoomRead.ts +++ b/src/server/accessors/RoomRead.ts @@ -23,8 +23,15 @@ export class RoomRead implements IRoomRead { return this.roomBridge.doGetCreatorByName(name, this.appId); } - public getMessages(roomId: string): Promise> { - throw new Error('Method not implemented.'); + public getMessages( + roomId: string, + options?: Partial<{ + limit: number; + skip: number; + sort: Record; + }>, + ): Promise { + return this.roomBridge.doGetMessages(roomId, { limit: 100, ...options }, this.appId); } public getMembers(roomId: string): Promise> { diff --git a/src/server/bridges/RoomBridge.ts b/src/server/bridges/RoomBridge.ts index 677620283..a1ec60952 100644 --- a/src/server/bridges/RoomBridge.ts +++ b/src/server/bridges/RoomBridge.ts @@ -91,6 +91,20 @@ export abstract class RoomBridge extends BaseBridge { } } + public async doGetMessages( + roomId: string, + options: { + limit: number; + skip?: number; + sort?: Record; + }, + appId: string, + ): Promise { + if (this.hasReadPermission(appId)) { + return this.getMessages(roomId, options, appId); + } + } + protected abstract create(room: IRoom, members: Array, appId: string): Promise; protected abstract getById(roomId: string, appId: string): Promise; @@ -123,6 +137,16 @@ export abstract class RoomBridge extends BaseBridge { protected abstract getLeaders(roomId: string, appId: string): Promise>; + protected abstract getMessages( + roomId: string, + options: { + limit: number; + skip?: number; + sort?: Record; + }, + appId: string, + ): Promise; + private hasWritePermission(appId: string): boolean { if (AppPermissionManager.hasPermission(appId, AppPermissions.room.write)) { return true; diff --git a/tests/server/accessors/RoomRead.spec.ts b/tests/server/accessors/RoomRead.spec.ts index 5d2a142c5..f99eae19e 100644 --- a/tests/server/accessors/RoomRead.spec.ts +++ b/tests/server/accessors/RoomRead.spec.ts @@ -5,21 +5,26 @@ import type { IUser } from '../../../src/definition/users'; import { RoomRead } from '../../../src/server/accessors'; import type { RoomBridge } from '../../../src/server/bridges'; import { TestData } from '../../test-data/utilities'; +import type { IMessage } from '../../../src/definition/messages'; export class RoomReadAccessorTestFixture { private room: IRoom; private user: IUser; + private messages: IMessage[]; + private mockRoomBridgeWithRoom: RoomBridge; @SetupFixture public setupFixture() { this.room = TestData.getRoom(); this.user = TestData.getUser(); + this.messages = ['507f1f77bcf86cd799439011', '507f191e810c19729de860ea'].map((id) => TestData.getMessage(id)); const theRoom = this.room; const theUser = this.user; + const theMessages = this.messages; this.mockRoomBridgeWithRoom = { doGetById(id, appId): Promise { return Promise.resolve(theRoom); @@ -39,6 +44,9 @@ export class RoomReadAccessorTestFixture { doGetMembers(name, appId): Promise> { return Promise.resolve([theUser]); }, + doGetMessages(roomId, appId, options): Promise { + return Promise.resolve(theMessages); + }, } as RoomBridge; } @@ -58,6 +66,8 @@ export class RoomReadAccessorTestFixture { Expect(await rr.getCreatorUserByName('testing')).toBe(this.user); Expect(await rr.getDirectByUsernames([this.user.username])).toBeDefined(); Expect(await rr.getDirectByUsernames([this.user.username])).toBe(this.room); + Expect(await rr.getMessages('testing')).toBeDefined(); + Expect(await rr.getMessages('testing')).toBe(this.messages); } @AsyncTest() @@ -65,7 +75,6 @@ export class RoomReadAccessorTestFixture { Expect(() => new RoomRead(this.mockRoomBridgeWithRoom, 'testing-app')).not.toThrow(); const rr = new RoomRead(this.mockRoomBridgeWithRoom, 'testing-app'); - await Expect(() => rr.getMessages('faker')).toThrowErrorAsync(Error, 'Method not implemented.'); Expect(await rr.getMembers('testing')).toBeDefined(); Expect((await rr.getMembers('testing')) as Array).not.toBeEmpty(); diff --git a/tests/test-data/bridges/roomBridge.ts b/tests/test-data/bridges/roomBridge.ts index c25edbb4f..92a596f8e 100644 --- a/tests/test-data/bridges/roomBridge.ts +++ b/tests/test-data/bridges/roomBridge.ts @@ -32,6 +32,10 @@ export class TestsRoomBridge extends RoomBridge { throw new Error('Method not implemented.'); } + public getMessages(roomId: string, options: { limit: number; skip?: number; sort?: Record }, appId: string): Promise { + throw new Error('Method not implemented.'); + } + public update(room: IRoom, members: Array, appId: string): Promise { throw new Error('Method not implemented.'); } From 763b9d365bb7892f24931ff6ca30de3d4f1501b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=83=87=E3=83=AF=E3=83=B3=E3=82=B7=E3=83=A5?= <61188295+Dnouv@users.noreply.github.com> Date: Tue, 28 May 2024 23:24:35 +0530 Subject: [PATCH 2/6] feat: Introduce new msg interface and correct the tests (#767) Co-authored-by: Douglas Gubert --- src/definition/accessors/IRoomRead.ts | 4 +- src/definition/messages/IMessageRaw.ts | 40 +++++++++ src/definition/messages/index.ts | 2 + src/definition/users/IUserLookup.ts | 1 + src/server/accessors/RoomRead.ts | 4 +- src/server/bridges/RoomBridge.ts | 6 +- tests/server/accessors/RoomRead.spec.ts | 8 +- tests/test-data/bridges/roomBridge.ts | 4 +- tests/test-data/utilities.ts | 106 ++++++++++++++++-------- 9 files changed, 126 insertions(+), 49 deletions(-) create mode 100644 src/definition/messages/IMessageRaw.ts diff --git a/src/definition/accessors/IRoomRead.ts b/src/definition/accessors/IRoomRead.ts index 4e827507d..53550780d 100644 --- a/src/definition/accessors/IRoomRead.ts +++ b/src/definition/accessors/IRoomRead.ts @@ -1,4 +1,4 @@ -import type { IMessage } from '../messages/index'; +import type { IMessageRaw } from '../messages/index'; import type { IRoom } from '../rooms/index'; import type { IUser } from '../users/index'; @@ -56,7 +56,7 @@ export interface IRoomRead { skip: number; sort: Record; }>, - ): Promise; + ): Promise; /** * Gets an iterator for all of the users in the provided room. diff --git a/src/definition/messages/IMessageRaw.ts b/src/definition/messages/IMessageRaw.ts new file mode 100644 index 000000000..1706b2639 --- /dev/null +++ b/src/definition/messages/IMessageRaw.ts @@ -0,0 +1,40 @@ +import type { IBlock, Block } from '@rocket.chat/ui-kit'; + +import type { IRoom } from '../rooms'; +import type { IUserLookup } from '../users'; +import type { IMessageAttachment } from './IMessageAttachment'; +import type { IMessageFile } from './IMessageFile'; +import type { IMessageReactions } from './IMessageReaction'; + +/** + * The raw version of a message, without resolved information for relationship fields, i.e. + * `room`, `sender` and `editor` are not the complete entity like they are in `IMessage` + * + * This is used in methods that fetch multiple messages at the same time, as resolving the relationship + * fields require additional queries to the database and would hit the system's performance significantly. + */ +export interface IMessageRaw { + id: string; + roomId: IRoom['id']; + sender: IUserLookup; + createdAt: Date; + threadId?: string; + text?: string; + updatedAt?: Date; + editor?: IUserLookup; + editedAt?: Date; + emoji?: string; + avatarUrl?: string; + alias?: string; + file?: IMessageFile; + attachments?: Array; + reactions?: IMessageReactions; + groupable?: boolean; + parseUrls?: boolean; + customFields?: { [key: string]: any }; + blocks?: Array; + starred?: Array<{ _id: string }>; + pinned?: boolean; + pinnedAt?: Date; + pinnedBy?: IUserLookup; +} diff --git a/src/definition/messages/index.ts b/src/definition/messages/index.ts index a46a0310e..1ed02d7b2 100644 --- a/src/definition/messages/index.ts +++ b/src/definition/messages/index.ts @@ -8,6 +8,7 @@ import { IMessageDeleteContext } from './IMessageDeleteContext'; import { IMessageFile } from './IMessageFile'; import { IMessageFollowContext } from './IMessageFollowContext'; import { IMessagePinContext } from './IMessagePinContext'; +import { IMessageRaw } from './IMessageRaw'; import { IMessageReaction, IMessageReactions } from './IMessageReaction'; import { IMessageReactionContext } from './IMessageReactionContext'; import { IMessageReportContext } from './IMessageReportContext'; @@ -39,6 +40,7 @@ export { IMessageAttachmentField, IMessageAction, IMessageFile, + IMessageRaw, IMessageReactions, IMessageReaction, IPostMessageDeleted, diff --git a/src/definition/users/IUserLookup.ts b/src/definition/users/IUserLookup.ts index 271560b86..9b5f09b04 100644 --- a/src/definition/users/IUserLookup.ts +++ b/src/definition/users/IUserLookup.ts @@ -1,4 +1,5 @@ export interface IUserLookup { _id: string; username: string; + name?: string; } diff --git a/src/server/accessors/RoomRead.ts b/src/server/accessors/RoomRead.ts index d79a64028..efbc63bc5 100644 --- a/src/server/accessors/RoomRead.ts +++ b/src/server/accessors/RoomRead.ts @@ -1,5 +1,5 @@ import type { IRoomRead } from '../../definition/accessors'; -import type { IMessage } from '../../definition/messages'; +import type { IMessageRaw } from '../../definition/messages'; import type { IRoom } from '../../definition/rooms'; import type { IUser } from '../../definition/users'; import type { RoomBridge } from '../bridges'; @@ -30,7 +30,7 @@ export class RoomRead implements IRoomRead { skip: number; sort: Record; }>, - ): Promise { + ): Promise { return this.roomBridge.doGetMessages(roomId, { limit: 100, ...options }, this.appId); } diff --git a/src/server/bridges/RoomBridge.ts b/src/server/bridges/RoomBridge.ts index a1ec60952..ced6b3b6e 100644 --- a/src/server/bridges/RoomBridge.ts +++ b/src/server/bridges/RoomBridge.ts @@ -1,4 +1,4 @@ -import type { IMessage } from '../../definition/messages'; +import type { IMessage, IMessageRaw } from '../../definition/messages'; import type { IRoom } from '../../definition/rooms'; import type { IUser } from '../../definition/users'; import { PermissionDeniedError } from '../errors/PermissionDeniedError'; @@ -99,7 +99,7 @@ export abstract class RoomBridge extends BaseBridge { sort?: Record; }, appId: string, - ): Promise { + ): Promise { if (this.hasReadPermission(appId)) { return this.getMessages(roomId, options, appId); } @@ -145,7 +145,7 @@ export abstract class RoomBridge extends BaseBridge { sort?: Record; }, appId: string, - ): Promise; + ): Promise; private hasWritePermission(appId: string): boolean { if (AppPermissionManager.hasPermission(appId, AppPermissions.room.write)) { diff --git a/tests/server/accessors/RoomRead.spec.ts b/tests/server/accessors/RoomRead.spec.ts index f99eae19e..af1b22b82 100644 --- a/tests/server/accessors/RoomRead.spec.ts +++ b/tests/server/accessors/RoomRead.spec.ts @@ -5,14 +5,14 @@ import type { IUser } from '../../../src/definition/users'; import { RoomRead } from '../../../src/server/accessors'; import type { RoomBridge } from '../../../src/server/bridges'; import { TestData } from '../../test-data/utilities'; -import type { IMessage } from '../../../src/definition/messages'; +import type { IMessageRaw } from '../../../src/definition/messages'; export class RoomReadAccessorTestFixture { private room: IRoom; private user: IUser; - private messages: IMessage[]; + private messages: IMessageRaw[]; private mockRoomBridgeWithRoom: RoomBridge; @@ -20,7 +20,7 @@ export class RoomReadAccessorTestFixture { public setupFixture() { this.room = TestData.getRoom(); this.user = TestData.getUser(); - this.messages = ['507f1f77bcf86cd799439011', '507f191e810c19729de860ea'].map((id) => TestData.getMessage(id)); + this.messages = ['507f1f77bcf86cd799439011', '507f191e810c19729de860ea'].map((id) => TestData.getMessageRaw(id)); const theRoom = this.room; const theUser = this.user; @@ -44,7 +44,7 @@ export class RoomReadAccessorTestFixture { doGetMembers(name, appId): Promise> { return Promise.resolve([theUser]); }, - doGetMessages(roomId, appId, options): Promise { + doGetMessages(roomId, appId, options): Promise { return Promise.resolve(theMessages); }, } as RoomBridge; diff --git a/tests/test-data/bridges/roomBridge.ts b/tests/test-data/bridges/roomBridge.ts index 92a596f8e..600a3c5c6 100644 --- a/tests/test-data/bridges/roomBridge.ts +++ b/tests/test-data/bridges/roomBridge.ts @@ -1,4 +1,4 @@ -import type { IMessage } from '../../../src/definition/messages'; +import type { IMessage, IMessageRaw } from '../../../src/definition/messages'; import type { IRoom } from '../../../src/definition/rooms'; import type { IUser } from '../../../src/definition/users'; import { RoomBridge } from '../../../src/server/bridges'; @@ -32,7 +32,7 @@ export class TestsRoomBridge extends RoomBridge { throw new Error('Method not implemented.'); } - public getMessages(roomId: string, options: { limit: number; skip?: number; sort?: Record }, appId: string): Promise { + public getMessages(roomId: string, options: { limit: number; skip?: number; sort?: Record }, appId: string): Promise { throw new Error('Method not implemented.'); } diff --git a/tests/test-data/utilities.ts b/tests/test-data/utilities.ts index fb84408d8..e9d098e21 100644 --- a/tests/test-data/utilities.ts +++ b/tests/test-data/utilities.ts @@ -1,6 +1,6 @@ import type { IHttp, IModify, IPersistence, IRead } from '../../src/definition/accessors'; import { HttpStatusCode } from '../../src/definition/accessors'; -import type { IMessage } from '../../src/definition/messages'; +import type { IMessage, IMessageAttachment, IMessageRaw } from '../../src/definition/messages'; import type { IRoom } from '../../src/definition/rooms'; import { RoomType } from '../../src/definition/rooms'; import type { ISetting } from '../../src/definition/settings'; @@ -128,6 +128,39 @@ export class TestInfastructureSetup { } const date = new Date(); + +const DEFAULT_ATTACHMENT = { + color: '#00b2b2', + collapsed: false, + text: 'Just an attachment that is used for testing', + timestampLink: 'https://google.com/', + thumbnailUrl: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', + author: { + name: 'Author Name', + link: 'https://github.com/graywolf336', + icon: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', + }, + title: { + value: 'Attachment Title', + link: 'https://github.com/RocketChat', + displayDownloadLink: false, + }, + imageUrl: 'https://rocket.chat/images/default/logo.svg', + audioUrl: 'http://www.w3schools.com/tags/horse.mp3', + videoUrl: 'http://www.w3schools.com/tags/movie.mp4', + fields: [ + { + short: true, + title: 'Test', + value: 'Testing out something or other', + }, + { + short: true, + title: 'Another Test', + value: '[Link](https://google.com/) something and this and that.', + }, + ], +}; export class TestData { public static getDate(): Date { return date; @@ -193,41 +226,42 @@ export class TestData { emoji: ':see_no_evil:', avatarUrl: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', alias: 'Testing Bot', - attachments: [ - { - collapsed: false, - color: '#00b2b2', - text: 'Just an attachment that is used for testing', - timestamp: new Date(), - timestampLink: 'https://google.com/', - thumbnailUrl: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', - author: { - name: 'Author Name', - link: 'https://github.com/graywolf336', - icon: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', - }, - title: { - value: 'Attachment Title', - link: 'https://github.com/RocketChat', - displayDownloadLink: false, - }, - imageUrl: 'https://rocket.chat/images/default/logo.svg', - audioUrl: 'http://www.w3schools.com/tags/horse.mp3', - videoUrl: 'http://www.w3schools.com/tags/movie.mp4', - fields: [ - { - short: true, - title: 'Test', - value: 'Testing out something or other', - }, - { - short: true, - title: 'Another Test', - value: '[Link](https://google.com/) something and this and that.', - }, - ], - }, - ], + attachments: [this.createAttachment()], + }; + } + + public static getMessageRaw(id?: string, text?: string): IMessageRaw { + const editorUser = TestData.getUser(); + const senderUser = TestData.getUser(); + + return { + id: id || '4bShvoOXqB', + roomId: TestData.getRoom().id, + sender: { + _id: senderUser.id, + username: senderUser.username, + name: senderUser?.name, + }, + text: text || 'This is just a test, do not be alarmed', + createdAt: date, + updatedAt: new Date(), + editor: { + _id: editorUser.id, + username: editorUser.username, + }, + editedAt: new Date(), + emoji: ':see_no_evil:', + avatarUrl: 'https://avatars0.githubusercontent.com/u/850391?s=88&v=4', + alias: 'Testing Bot', + attachments: [this.createAttachment()], + }; + } + + private static createAttachment(attachment?: IMessageAttachment): IMessageAttachment { + attachment = attachment || DEFAULT_ATTACHMENT; + return { + timestamp: new Date(), + ...attachment, }; } From dfdf5e479cf1d134f91e6a79f5260e29ecfcbb53 Mon Sep 17 00:00:00 2001 From: Dnouv Date: Thu, 20 Jun 2024 15:00:13 +0530 Subject: [PATCH 3/6] add map & validations --- src/definition/accessors/IRoomRead.ts | 4 +-- src/server/accessors/RoomRead.ts | 33 +++++++++++++++++++++++-- tests/server/accessors/RoomRead.spec.ts | 2 +- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/definition/accessors/IRoomRead.ts b/src/definition/accessors/IRoomRead.ts index 53550780d..19ea6653c 100644 --- a/src/definition/accessors/IRoomRead.ts +++ b/src/definition/accessors/IRoomRead.ts @@ -46,7 +46,7 @@ export interface IRoomRead { * @param options Optional parameters for retrieving messages: * - limit: The maximum number of messages to retrieve. If more than 100 is passed, it defaults to 100. * - skip: The number of messages to skip (for pagination). - * - sort: An object defining the sorting order of the messages. Each key is a field to sort by, and the value is either 1 for ascending order or -1 for descending order. + * - sort: An object defining the sorting order of the messages. Each key is a field to sort by, and the value is either asc for ascending order or desc for descending order. * @returns A Promise that resolves to an array of IMessage objects representing the messages in the room. */ getMessages( @@ -54,7 +54,7 @@ export interface IRoomRead { options?: Partial<{ limit: number; skip: number; - sort: Record; + sort: Record; }>, ): Promise; diff --git a/src/server/accessors/RoomRead.ts b/src/server/accessors/RoomRead.ts index efbc63bc5..894e6b539 100644 --- a/src/server/accessors/RoomRead.ts +++ b/src/server/accessors/RoomRead.ts @@ -28,10 +28,22 @@ export class RoomRead implements IRoomRead { options?: Partial<{ limit: number; skip: number; - sort: Record; + sort: Record; }>, ): Promise { - return this.roomBridge.doGetMessages(roomId, { limit: 100, ...options }, this.appId); + const { skip, sort } = options || {}; + let { limit } = options || {}; + // Ensure limit is a finite number; if not, default to 100 + limit = Number.isFinite(limit) ? Math.min(Math.max(limit, 1), 100) : 100; + + const coreSortOptions = this.mapSortParams(sort); + + const adjustedOptions = { + limit, + skip, + sort: coreSortOptions, + }; + return this.roomBridge.doGetMessages(roomId, adjustedOptions, this.appId); } public getMembers(roomId: string): Promise> { @@ -53,4 +65,21 @@ export class RoomRead implements IRoomRead { public getLeaders(roomId: string): Promise> { return this.roomBridge.doGetLeaders(roomId, this.appId); } + + private mapSortParams(userSort?: Record) { + if (!userSort) return; + + const sortMap: Record = { + createdAt: 'ts', + updatedAt: '_updatedAt', + }; + + return Object.entries(userSort).reduce((acc, [key, value]) => { + const mappedKey = sortMap[key]; + if (mappedKey) { + acc[mappedKey] = value === 'asc' ? 1 : -1; + } + return acc; + }, {} as Record); + } } diff --git a/tests/server/accessors/RoomRead.spec.ts b/tests/server/accessors/RoomRead.spec.ts index af1b22b82..ed4f48b33 100644 --- a/tests/server/accessors/RoomRead.spec.ts +++ b/tests/server/accessors/RoomRead.spec.ts @@ -44,7 +44,7 @@ export class RoomReadAccessorTestFixture { doGetMembers(name, appId): Promise> { return Promise.resolve([theUser]); }, - doGetMessages(roomId, appId, options): Promise { + doGetMessages(roomId, options, appId): Promise { return Promise.resolve(theMessages); }, } as RoomBridge; From 58a802f2e6106df3cd21dd1921c09fd93ece02ea Mon Sep 17 00:00:00 2001 From: Dnouv Date: Mon, 15 Jul 2024 19:15:46 +0530 Subject: [PATCH 4/6] fix lint --- src/server/bridges/RoomBridge.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/bridges/RoomBridge.ts b/src/server/bridges/RoomBridge.ts index 5aa4da9ef..34852d631 100644 --- a/src/server/bridges/RoomBridge.ts +++ b/src/server/bridges/RoomBridge.ts @@ -104,7 +104,7 @@ export abstract class RoomBridge extends BaseBridge { return this.getMessages(roomId, options, appId); } } - + public async doRemoveUsers(roomId: string, usernames: Array, appId: string): Promise { if (this.hasWritePermission(appId)) { return this.removeUsers(roomId, usernames, appId); @@ -152,7 +152,7 @@ export abstract class RoomBridge extends BaseBridge { }, appId: string, ): Promise; - + protected abstract removeUsers(roomId: string, usernames: Array, appId: string): Promise; private hasWritePermission(appId: string): boolean { From 2a6703da3e2641f7ad880b050705dc5e2718ec3a Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 18 Jul 2024 15:23:19 -0300 Subject: [PATCH 5/6] Improve validation and types --- src/definition/accessors/IRoomRead.ts | 14 ++----- src/server/accessors/RoomRead.ts | 56 +++++++++++---------------- src/server/bridges/RoomBridge.ts | 28 +++++--------- 3 files changed, 36 insertions(+), 62 deletions(-) diff --git a/src/definition/accessors/IRoomRead.ts b/src/definition/accessors/IRoomRead.ts index 19ea6653c..f4e0df332 100644 --- a/src/definition/accessors/IRoomRead.ts +++ b/src/definition/accessors/IRoomRead.ts @@ -1,3 +1,4 @@ +import type { GetMessagesOptions } from '../../server/bridges/RoomBridge'; import type { IMessageRaw } from '../messages/index'; import type { IRoom } from '../rooms/index'; import type { IUser } from '../users/index'; @@ -44,19 +45,12 @@ export interface IRoomRead { * * @param roomId The unique identifier of the room from which to retrieve messages. * @param options Optional parameters for retrieving messages: - * - limit: The maximum number of messages to retrieve. If more than 100 is passed, it defaults to 100. + * - limit: The maximum number of messages to retrieve. Maximum 100 * - skip: The number of messages to skip (for pagination). - * - sort: An object defining the sorting order of the messages. Each key is a field to sort by, and the value is either asc for ascending order or desc for descending order. + * - sort: An object defining the sorting order of the messages. Each key is a field to sort by, and the value is either "asc" for ascending order or "desc" for descending order. * @returns A Promise that resolves to an array of IMessage objects representing the messages in the room. */ - getMessages( - roomId: string, - options?: Partial<{ - limit: number; - skip: number; - sort: Record; - }>, - ): Promise; + getMessages(roomId: string, options?: Partial): Promise>; /** * Gets an iterator for all of the users in the provided room. diff --git a/src/server/accessors/RoomRead.ts b/src/server/accessors/RoomRead.ts index 894e6b539..30b231af6 100644 --- a/src/server/accessors/RoomRead.ts +++ b/src/server/accessors/RoomRead.ts @@ -3,6 +3,7 @@ import type { IMessageRaw } from '../../definition/messages'; import type { IRoom } from '../../definition/rooms'; import type { IUser } from '../../definition/users'; import type { RoomBridge } from '../bridges'; +import { type GetMessagesOptions, GetMessagesSortableFields } from '../bridges/RoomBridge'; export class RoomRead implements IRoomRead { constructor(private roomBridge: RoomBridge, private appId: string) {} @@ -23,27 +24,18 @@ export class RoomRead implements IRoomRead { return this.roomBridge.doGetCreatorByName(name, this.appId); } - public getMessages( - roomId: string, - options?: Partial<{ - limit: number; - skip: number; - sort: Record; - }>, - ): Promise { - const { skip, sort } = options || {}; - let { limit } = options || {}; - // Ensure limit is a finite number; if not, default to 100 - limit = Number.isFinite(limit) ? Math.min(Math.max(limit, 1), 100) : 100; - - const coreSortOptions = this.mapSortParams(sort); - - const adjustedOptions = { - limit, - skip, - sort: coreSortOptions, - }; - return this.roomBridge.doGetMessages(roomId, adjustedOptions, this.appId); + public getMessages(roomId: string, options: Partial = {}): Promise { + if (typeof options.limit !== 'undefined' && (!Number.isFinite(options.limit) || options.limit > 100)) { + throw new Error(`Invalid limit provided. Expected number <= 100, got ${options.limit}`); + } + + options.limit ??= 100; + + if (options.sort) { + this.validateSort(options.sort); + } + + return this.roomBridge.doGetMessages(roomId, options as GetMessagesOptions, this.appId); } public getMembers(roomId: string): Promise> { @@ -66,20 +58,16 @@ export class RoomRead implements IRoomRead { return this.roomBridge.doGetLeaders(roomId, this.appId); } - private mapSortParams(userSort?: Record) { - if (!userSort) return; - - const sortMap: Record = { - createdAt: 'ts', - updatedAt: '_updatedAt', - }; + // If there are any invalid fields or values, throw + private validateSort(sort: Record) { + Object.entries(sort).forEach(([key, value]) => { + if (!GetMessagesSortableFields.includes(key as typeof GetMessagesSortableFields[number])) { + throw new Error(`Invalid key "${key}" used in sort. Available keys for sorting are ${GetMessagesSortableFields.join(', ')}`); + } - return Object.entries(userSort).reduce((acc, [key, value]) => { - const mappedKey = sortMap[key]; - if (mappedKey) { - acc[mappedKey] = value === 'asc' ? 1 : -1; + if (value !== 'asc' && value !== 'desc') { + throw new Error(`Invalid sort direction for field "${key}". Expected "asc" or "desc", got ${value}`); } - return acc; - }, {} as Record); + }); } } diff --git a/src/server/bridges/RoomBridge.ts b/src/server/bridges/RoomBridge.ts index 34852d631..1ac9de464 100644 --- a/src/server/bridges/RoomBridge.ts +++ b/src/server/bridges/RoomBridge.ts @@ -6,6 +6,14 @@ import { AppPermissionManager } from '../managers/AppPermissionManager'; import { AppPermissions } from '../permissions/AppPermissions'; import { BaseBridge } from './BaseBridge'; +export const GetMessagesSortableFields = ['createdAt'] as const; + +export type GetMessagesOptions = { + limit: number; + skip: number; + sort: Record; +}; + export abstract class RoomBridge extends BaseBridge { public async doCreate(room: IRoom, members: Array, appId: string): Promise { if (this.hasWritePermission(appId)) { @@ -91,15 +99,7 @@ export abstract class RoomBridge extends BaseBridge { } } - public async doGetMessages( - roomId: string, - options: { - limit: number; - skip?: number; - sort?: Record; - }, - appId: string, - ): Promise { + public async doGetMessages(roomId: string, options: GetMessagesOptions, appId: string): Promise { if (this.hasReadPermission(appId)) { return this.getMessages(roomId, options, appId); } @@ -143,15 +143,7 @@ export abstract class RoomBridge extends BaseBridge { protected abstract getLeaders(roomId: string, appId: string): Promise>; - protected abstract getMessages( - roomId: string, - options: { - limit: number; - skip?: number; - sort?: Record; - }, - appId: string, - ): Promise; + protected abstract getMessages(roomId: string, options: GetMessagesOptions, appId: string): Promise; protected abstract removeUsers(roomId: string, usernames: Array, appId: string): Promise; From 0a7406b46db0da151b5a0a545080a0298d324a38 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 18 Jul 2024 18:00:49 -0300 Subject: [PATCH 6/6] Fix test --- tests/test-data/bridges/roomBridge.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test-data/bridges/roomBridge.ts b/tests/test-data/bridges/roomBridge.ts index 3df7ab134..e5e7408b3 100644 --- a/tests/test-data/bridges/roomBridge.ts +++ b/tests/test-data/bridges/roomBridge.ts @@ -2,6 +2,7 @@ import type { IMessage, IMessageRaw } from '../../../src/definition/messages'; import type { IRoom } from '../../../src/definition/rooms'; import type { IUser } from '../../../src/definition/users'; import { RoomBridge } from '../../../src/server/bridges'; +import type { GetMessagesOptions } from '../../../src/server/bridges/RoomBridge'; export class TestsRoomBridge extends RoomBridge { public create(room: IRoom, members: Array, appId: string): Promise { @@ -32,7 +33,7 @@ export class TestsRoomBridge extends RoomBridge { throw new Error('Method not implemented.'); } - public getMessages(roomId: string, options: { limit: number; skip?: number; sort?: Record }, appId: string): Promise { + public getMessages(roomId: string, options: GetMessagesOptions, appId: string): Promise { throw new Error('Method not implemented.'); }