From cdefc692527adabe5bce152950190db7992c5dc0 Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 13 Aug 2024 15:37:52 -0300 Subject: [PATCH 1/5] chore: remove `persists` from updater --- .../federation/server/endpoints/dispatch.js | 2 +- .../lib/server/lib/notifyUsersOnMessage.ts | 2 +- .../hooks/afterSaveOmnichannelMessage.ts | 2 +- apps/meteor/server/models/raw/BaseRaw.ts | 6 +- .../model-typings/src/models/IBaseModel.ts | 1 + packages/model-typings/src/updater.ts | 3 +- packages/models/src/updater.spec.ts | 90 +++---------------- packages/models/src/updater.ts | 34 ++----- 8 files changed, 33 insertions(+), 107 deletions(-) diff --git a/apps/meteor/app/federation/server/endpoints/dispatch.js b/apps/meteor/app/federation/server/endpoints/dispatch.js index a7ab98accd36..7090f053a22b 100644 --- a/apps/meteor/app/federation/server/endpoints/dispatch.js +++ b/apps/meteor/app/federation/server/endpoints/dispatch.js @@ -296,7 +296,7 @@ const eventHandlers = { const roomUpdater = Rooms.getUpdater(); await notifyUsersOnMessage(denormalizedMessage, room, roomUpdater); if (roomUpdater.hasChanges()) { - await roomUpdater.persist({ _id: room._id }); + await Rooms.updateFromUpdater({ _id: room._id }, roomUpdater); } sendAllNotifications(denormalizedMessage, room); diff --git a/apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts b/apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts index 0b80b9bb06df..a05c05b4bb94 100644 --- a/apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts +++ b/apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts @@ -188,7 +188,7 @@ callbacks.add( await notifyUsersOnMessage(message, room, roomUpdater); if (roomUpdater.hasChanges()) { - await roomUpdater.persist({ _id: room._id }); + await Rooms.updateFromUpdater({ _id: room._id }, roomUpdater); } return message; diff --git a/apps/meteor/app/livechat/server/hooks/afterSaveOmnichannelMessage.ts b/apps/meteor/app/livechat/server/hooks/afterSaveOmnichannelMessage.ts index 372704d339bb..07ce7fe08573 100644 --- a/apps/meteor/app/livechat/server/hooks/afterSaveOmnichannelMessage.ts +++ b/apps/meteor/app/livechat/server/hooks/afterSaveOmnichannelMessage.ts @@ -14,7 +14,7 @@ callbacks.add( const result = await callbacks.run('afterOmnichannelSaveMessage', message, { room, roomUpdater: updater }); if (updater.hasChanges()) { - await updater.persist({ _id: room._id }); + await LivechatRooms.updateFromUpdater({ _id: room._id }, updater); } return result; diff --git a/apps/meteor/server/models/raw/BaseRaw.ts b/apps/meteor/server/models/raw/BaseRaw.ts index 0a41a0fbb3eb..90a2fe3a8503 100644 --- a/apps/meteor/server/models/raw/BaseRaw.ts +++ b/apps/meteor/server/models/raw/BaseRaw.ts @@ -111,7 +111,11 @@ export abstract class BaseRaw< } public getUpdater(): Updater { - return new UpdaterImpl(this.col as unknown as IBaseModel); + return new UpdaterImpl(); + } + + public updateFromUpdater(query: Filter, updater: Updater): Promise { + return this.updateOne(query, updater.getUpdateFilter()); } private doNotMixInclusionAndExclusionFields(options: FindOptions = {}): FindOptions { diff --git a/packages/model-typings/src/models/IBaseModel.ts b/packages/model-typings/src/models/IBaseModel.ts index 59a5f67273eb..246c3ae253dd 100644 --- a/packages/model-typings/src/models/IBaseModel.ts +++ b/packages/model-typings/src/models/IBaseModel.ts @@ -51,6 +51,7 @@ export interface IBaseModel< getCollectionName(): string; getUpdater(): Updater; + updateFromUpdater(query: Filter, updater: Updater): Promise; findOneAndUpdate(query: Filter, update: UpdateFilter | T, options?: FindOneAndUpdateOptions): Promise>; diff --git a/packages/model-typings/src/updater.ts b/packages/model-typings/src/updater.ts index fe8354479c1f..7743430ad4b8 100644 --- a/packages/model-typings/src/updater.ts +++ b/packages/model-typings/src/updater.ts @@ -1,12 +1,11 @@ /* eslint-disable @typescript-eslint/naming-convention */ -import type { Join, NestedPaths, PropertyType, ArrayElement, NestedPathsOfType, Filter, UpdateFilter } from 'mongodb'; +import type { Join, NestedPaths, PropertyType, ArrayElement, NestedPathsOfType, UpdateFilter } from 'mongodb'; export interface Updater { set

, K extends keyof P>(key: K, value: P[K]): Updater; unset>(key: K): Updater; inc>(key: K, value: number): Updater; addToSet>(key: K, value: ArrayElementType[K]>): Updater; - persist(query: Filter): Promise; hasChanges(): boolean; getUpdateFilter(): UpdateFilter; } diff --git a/packages/models/src/updater.spec.ts b/packages/models/src/updater.spec.ts index ae75400bd9ee..ed978b67f3b7 100644 --- a/packages/models/src/updater.spec.ts +++ b/packages/models/src/updater.spec.ts @@ -15,9 +15,9 @@ test('updater typings', () => { e: string; }; e: string[]; - }>({} as any); + }>(); - const omnichannel = new UpdaterImpl({} as any); + const omnichannel = new UpdaterImpl(); omnichannel.addToSet('v.activity', 'asd'); // @ts-expect-error omnichannel.addToSet('v.activity', 1); @@ -70,8 +70,6 @@ test('updater typings', () => { }); test('updater $set operations', async () => { - const updateOne = jest.fn(); - const updater = new UpdaterImpl<{ _id: string; t: 'l'; @@ -79,29 +77,16 @@ test('updater $set operations', async () => { b: string; }; c?: number; - }>({ - updateOne, - } as any); + }>(); updater.set('a', { b: 'set', }); - await updater.persist({ - _id: 'test', - }); - - expect(updateOne).toBeCalledWith( - { - _id: 'test', - }, - { $set: { a: { b: 'set' } } }, - ); + expect(updater.getUpdateFilter()).toEqual({ $set: { a: { b: 'set' } } }); }); test('updater $unset operations', async () => { - const updateOne = jest.fn(); - const updater = new UpdaterImpl<{ _id: string; t: 'l'; @@ -109,27 +94,12 @@ test('updater $unset operations', async () => { b: string; }; c?: number; - }>({ - updateOne, - } as any); - + }>(); updater.unset('c'); - - await updater.persist({ - _id: 'test', - }); - - expect(updateOne).toBeCalledWith( - { - _id: 'test', - }, - { $unset: { c: 1 } }, - ); + expect(updater.getUpdateFilter()).toEqual({ $unset: { c: 1 } }); }); test('updater inc multiple operations', async () => { - const updateOne = jest.fn(); - const updater = new UpdaterImpl<{ _id: string; t: 'l'; @@ -137,52 +107,27 @@ test('updater inc multiple operations', async () => { b: string; }; c?: number; - }>({ - updateOne, - } as any); + }>(); updater.inc('c', 1); updater.inc('c', 1); - await updater.persist({ - _id: 'test', - }); - - expect(updateOne).toBeCalledWith( - { - _id: 'test', - }, - { $inc: { c: 2 } }, - ); + expect(updater.getUpdateFilter()).toEqual({ $inc: { c: 2 } }); }); test('it should add items to array', async () => { - const updateOne = jest.fn(); const updater = new UpdaterImpl<{ _id: string; a: string[]; - }>({ - updateOne, - } as any); + }>(); updater.addToSet('a', 'b'); updater.addToSet('a', 'c'); - await updater.persist({ - _id: 'test', - }); - - expect(updateOne).toBeCalledWith( - { - _id: 'test', - }, - { $addToSet: { a: { $each: ['b', 'c'] } } }, - ); + expect(updater.getUpdateFilter()).toEqual({ $addToSet: { a: { $each: ['b', 'c'] } } }); }); -test('it should persist only once', async () => { - const updateOne = jest.fn(); - +test('it should getUpdateFilter only once', async () => { const updater = new UpdaterImpl<{ _id: string; t: 'l'; @@ -190,19 +135,12 @@ test('it should persist only once', async () => { b: string; }; c?: number; - }>({ - updateOne, - } as any); + }>(); updater.set('a', { b: 'set', }); - await updater.persist({ - _id: 'test', - }); - - expect(updateOne).toBeCalledTimes(1); - - expect(() => updater.persist({ _id: 'test' })).rejects.toThrow(); + expect(updater.getUpdateFilter()).toEqual({ $set: { a: { b: 'set' } } }); + expect(() => updater.getUpdateFilter()).toThrow(); }); diff --git a/packages/models/src/updater.ts b/packages/models/src/updater.ts index 361e228e65ac..700d4c208c77 100644 --- a/packages/models/src/updater.ts +++ b/packages/models/src/updater.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/naming-convention */ -import type { IBaseModel, Updater, SetProps, UnsetProps, IncProps, AddToSetProps } from '@rocket.chat/model-typings'; -import type { UpdateFilter, Filter } from 'mongodb'; +import type { Updater, SetProps, UnsetProps, IncProps, AddToSetProps } from '@rocket.chat/model-typings'; +import type { UpdateFilter } from 'mongodb'; type ArrayElementType = T extends (infer E)[] ? E : T; @@ -17,8 +17,6 @@ export class UpdaterImpl implements Updater { private dirty = false; - constructor(private model: IBaseModel) {} - set

, K extends keyof P>(key: K, value: P[K]) { this._set = this._set ?? new Map, any>(); this._set.set(key as Keys, value); @@ -47,31 +45,17 @@ export class UpdaterImpl implements Updater { return this; } - async persist(query: Filter): Promise { - if (this.dirty) { - throw new Error('Updater is not dirty'); - } - - if ((process.env.NODE_ENV === 'development' || process.env.TEST_MODE) && !this.hasChanges()) { - throw new Error('Nothing to update'); - } - - this.dirty = true; - - const update = this.getUpdateFilter(); - try { - await this.model.updateOne(query, update); - } catch (error) { - console.error('Failed to update', JSON.stringify(query), JSON.stringify(update, null, 2)); - throw error; - } - } - hasChanges() { - return Object.keys(this.getUpdateFilter()).length > 0; + const hasChanges = Object.keys(this.getUpdateFilter()).length > 0; + this.dirty = false; + return hasChanges; } getUpdateFilter() { + if (this.dirty) { + throw new Error('Updater is dirty'); + } + this.dirty = true; return { ...(this._set && { $set: Object.fromEntries(this._set) }), ...(this._unset && { $unset: Object.fromEntries([...this._unset.values()].map((k) => [k, 1])) }), From 55d6d8175b02974abf5096c3973a665653e781f0 Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 13 Aug 2024 16:13:23 -0300 Subject: [PATCH 2/5] .. --- apps/meteor/server/models/dummy/BaseDummy.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/meteor/server/models/dummy/BaseDummy.ts b/apps/meteor/server/models/dummy/BaseDummy.ts index 9ba590151c02..58ee667e1a0d 100644 --- a/apps/meteor/server/models/dummy/BaseDummy.ts +++ b/apps/meteor/server/models/dummy/BaseDummy.ts @@ -45,6 +45,10 @@ export class BaseDummy< return new UpdaterImpl(this.col as unknown as IBaseModel); } + public updateFromUpdater(query: Filter, updater: Updater): Promise { + return this.updateOne(query, updater); + } + getCollectionName(): string { return this.collectionName; } From 088e168b452a860b2e9092deeb46bd078e95ef0e Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 13 Aug 2024 16:40:47 -0300 Subject: [PATCH 3/5] .. --- apps/meteor/server/models/dummy/BaseDummy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/server/models/dummy/BaseDummy.ts b/apps/meteor/server/models/dummy/BaseDummy.ts index 58ee667e1a0d..c3052ede9487 100644 --- a/apps/meteor/server/models/dummy/BaseDummy.ts +++ b/apps/meteor/server/models/dummy/BaseDummy.ts @@ -42,7 +42,7 @@ export class BaseDummy< } public getUpdater(): Updater { - return new UpdaterImpl(this.col as unknown as IBaseModel); + return new UpdaterImpl(); } public updateFromUpdater(query: Filter, updater: Updater): Promise { From 90dde3f418e529c6e5c84c7494c17f13019367f2 Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 13 Aug 2024 17:30:12 -0300 Subject: [PATCH 4/5] throw error if updater is empty --- apps/meteor/server/models/raw/BaseRaw.ts | 6 +++++- packages/models/src/updater.ts | 27 +++++++++++++++++------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/apps/meteor/server/models/raw/BaseRaw.ts b/apps/meteor/server/models/raw/BaseRaw.ts index 90a2fe3a8503..1a3dd1a3eb4c 100644 --- a/apps/meteor/server/models/raw/BaseRaw.ts +++ b/apps/meteor/server/models/raw/BaseRaw.ts @@ -115,7 +115,11 @@ export abstract class BaseRaw< } public updateFromUpdater(query: Filter, updater: Updater): Promise { - return this.updateOne(query, updater.getUpdateFilter()); + const updateFilter = updater.getUpdateFilter(); + return this.updateOne(query, updateFilter).catch((e) => { + console.warn(e, updateFilter); + return Promise.reject(e); + }); } private doNotMixInclusionAndExclusionFields(options: FindOptions = {}): FindOptions { diff --git a/packages/models/src/updater.ts b/packages/models/src/updater.ts index 700d4c208c77..4f7ad271f397 100644 --- a/packages/models/src/updater.ts +++ b/packages/models/src/updater.ts @@ -46,16 +46,15 @@ export class UpdaterImpl implements Updater { } hasChanges() { - const hasChanges = Object.keys(this.getUpdateFilter()).length > 0; - this.dirty = false; - return hasChanges; + const filter = this._getUpdateFilter(); + return this._hasChanges(filter); } - getUpdateFilter() { - if (this.dirty) { - throw new Error('Updater is dirty'); - } - this.dirty = true; + private _hasChanges(filter: UpdateFilter) { + return Object.keys(filter).length > 0; + } + + private _getUpdateFilter() { return { ...(this._set && { $set: Object.fromEntries(this._set) }), ...(this._unset && { $unset: Object.fromEntries([...this._unset.values()].map((k) => [k, 1])) }), @@ -63,6 +62,18 @@ export class UpdaterImpl implements Updater { ...(this._addToSet && { $addToSet: Object.fromEntries([...this._addToSet.entries()].map(([k, v]) => [k, { $each: v }])) }), } as unknown as UpdateFilter; } + + getUpdateFilter() { + if (this.dirty) { + throw new Error('Updater is dirty'); + } + this.dirty = true; + const filter = this._getUpdateFilter(); + if (!this._hasChanges(filter)) { + throw new Error('No changes to update'); + } + return filter; + } } export { Updater }; From d94ca8030b739fd86a0a140e73232a328155af3d Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Tue, 13 Aug 2024 17:30:21 -0300 Subject: [PATCH 5/5] fix spec tests --- packages/models/src/updater.spec.ts | 82 ++++++++++++++++------------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/packages/models/src/updater.spec.ts b/packages/models/src/updater.spec.ts index ed978b67f3b7..bdae43e338f3 100644 --- a/packages/models/src/updater.spec.ts +++ b/packages/models/src/updater.spec.ts @@ -17,56 +17,62 @@ test('updater typings', () => { e: string[]; }>(); - const omnichannel = new UpdaterImpl(); - omnichannel.addToSet('v.activity', 'asd'); - // @ts-expect-error - omnichannel.addToSet('v.activity', 1); - // @ts-expect-error - omnichannel.addToSet('v.activity', { - asdas: 1, - }); - - // @ts-expect-error - omnichannel.addToSet('v.activity.asd', { - asdas: 1, - }); - - updater.addToSet('e', 'a'); - - // @ts-expect-error - updater.addToSet('e', 1); - // @ts-expect-error - updater.addToSet('a', 'b'); - - // @ts-expect-error - updater.set('njame', 1); - // @ts-expect-error - updater.set('ttes', 1); - // @ts-expect-error + // @ts-expect-error: it should not allow any string to `t` only `l` is allowed updater.set('t', 'a'); + // `l` is allowed updater.set('t', 'l'); - // @ts-expect-error - updater.set('a', 'b'); - // @ts-expect-error - updater.set('c', 'b'); - updater.set('c', 1); - updater.set('a', { - b: 'set', - }); + // `a` is { b: string } + updater.set('a', { b: 'test' }); updater.set('a.b', 'test'); - - // @ts-expect-error + // @ts-expect-error: it should not allow strings to `a`, a is an object containing `b: string` + updater.set('a', 'b'); + // @ts-expect-error: `a` is not optional so unset is not allowed updater.unset('a'); + // @ts-expect-error: strings cannot be incremented + updater.inc('a', 1); + // `c` is number but it should be optional, so unset is allowed updater.unset('c'); + updater.set('c', 1); + // @ts-expect-error: `c` is a number + updater.set('c', 'b'); + // inc is allowed for numbers + updater.inc('c', 1); + // `d` is { e: string } but it should be optional, so unset is allowed updater.unset('d'); + updater.set('d', { e: 'a' }); + // @ts-expect-error: `d` is an object + updater.set('d', 'a'); + + // @ts-expect-error: it should not allow numbers, since e is a string + updater.addToSet('e', 1); + // @ts-expect-error: it should not allow strings, since a is an object + updater.addToSet('a', 'b'); + + updater.addToSet('e', 'a'); + // @ts-expect-error: it should not allow `njame` its not specified in the model + updater.set('njame', 1); + // `d` is { e: string } and also it should be optional, so unset is allowed updater.unset('d.e'); - // @ts-expect-error + // @ts-expect-error: `d` is an object cannot be incremented updater.inc('d', 1); - updater.inc('c', 1); + + // `activity` is a string + const omnichannel = new UpdaterImpl(); + omnichannel.addToSet('v.activity', 'asd'); + // @ts-expect-error: it should not allow numbers, since activity is a string + omnichannel.addToSet('v.activity', 1); + // @ts-expect-error: it should not allow objects, since activity is a string + omnichannel.addToSet('v.activity', { + asdas: 1, + }); + // @ts-expect-error: it should not allow sub properties, since activity is a string + omnichannel.addToSet('v.activity.asd', { + asdas: 1, + }); }); test('updater $set operations', async () => {