refactor: Subscriptions out of DB Watcher #32540

ricardogarim · 2024-06-03T03:50:11Z

As per the updates mentioned in PROJ-7, SCA-13 and ADR #74, this pull request focuses on relocating Subscriptions model out of DB Watcher service.

Context

This modification enhances RocketChat's app by allowing it to directly call listeners through the api.broadcast global function, bypassing the need for MongoDB Change Stream data propagation.

This change offers better control over user notifications via Web Sockets, enabling more precise management of use-cases. Instead of notifying every database change, we can now send an api.broadcast call only when necessary, reducing overall network messages. Additionally, this contributes to the future removal of the DB Watcher deployment, optimizing resource utilization.

Proposed changes

Key changes include:

Conditionally incorporating Subscriptions entity import within DB watchers on application startup based on the dbWatchersDisabled flag.
Enabling support for the following use cases to directly trigger watch.subscriptions listener event, subject to the dbWatchersDisabled flag.

Updated Use Cases

Use Case	Route/Trigger	Notes
removeRolesByUserId	applyRoomRolesToUser; Roles raw models removeUserRoles
addRolesByUserId	applyRoomRolesToUser; Roles raw models addUserRoles
setAsReadByRoomIdAndUserId	post route 'subscriptions.read'; afterSaveMessage callback
updateAllRoomTypesByRoomId	updateRoomType
removeByRoomId	deleteRoom server functions (add api route too); LivechatClass.closeRoom; RocketChatRoomAdapter.removeDirectMessageRoom
incUnreadForRoomIdExcludingUserIds	updateUsersSubscriptions
setAlertForRoomIdExcludingUserId	updateUsersSubscriptions
setOpenForRoomIdExcludingUserId	updateUsersSubscriptions
updateNameAndFnameByRoomId	RocketChatRoomAdapter updateRoomName and updateDisplayRoomName
setGroupE2EKey	method meteor 'e2e.updateGroupKey'; handleSuggestedGroupKey
setGroupE2ESuggestedKey	method meteor 'e2e.updateGroupKey'
unsetGroupE2ESuggestedKey	handleSuggestedGroupKey
setOnHoldByRoomId	OmnichannelEE > placeRoomOnHold
unsetOnHoldByRoomId	onCloseLivechat; omnichannelEE.resumeRoomOnHold
updateUnreadAlertById	meteor method saveNotificationSettings
updateNotificationsPrefById	meteor method saveNotificationSettings
updateHideMentionStatusById	meteor method saveNotificationSettings
updateDisableNotificationsById	meteor method saveNotificationSettings
clearAudioNotificationValueById	meteor method saveNotificationSettings saveAudioNotificationValue
updateAudioNotificationValueById	meteor method saveNotificationSettings saveAudioNotificationValue
updateMuteGroupMentions	meteor method saveNotificationSettings
updateHideUnreadStatusById	meteor method saveNotificationSettings
updateAutoTranslateLanguageById	meteor method 'autoTranslate.saveSettings'
updateAutoTranslateById	meteor method 'autoTranslate.saveSettings'
removeByVisitorToken	LivechatClass cleanGuestHistory
changeDepartmentByRoomId	livechat helper updateChatDepartment
updateAllAutoTranslateLanguagesByUserId	saveUserPreferences
disableAutoTranslateByRoomId	saveRoomEncrypted
resetUserE2EKey	resetUserE2EEncriptionKey
setAsUnreadByRoomIdAndUserId	meteor method unreadMessages
archiveByRoomId	ImportDataConverter.archiveRoomById; archiveRoom function
unarchiveByRoomId	unarchiveRoom function
updateNameAndAlertByRoomId	updateRoomName
setCustomFieldsDirectMessagesByUserId
setFavoriteByRoomIdAndUserId	meteor method toggleFavorite
hideByRoomIdAndUserId	hideRoomMethod
updateNameAndFnameById	updateGroupDMsName
setUserUsernameByUserId	updateUsernameReferences
updateFnameByRoomId	updateFName
updateDisplayNameByRoomId	registerContact; LivechatClass.saveRoomInfo
updateDirectNameAndFnameByName	updateUsernameReferences
incGroupMentionsAndUnreadForRoomIdExcludingUserId	incGroupMentions
unsetBlockedByRoomId	unblockUser
setLastReplyForRoomIdAndUserIds	updateThreadUsersSubscriptions
updateCustomFieldsByRoomId	saveRoomCustomFields
setOpenForRoomIdAndUserIds	reply; updateThreadUsersSubscriptions
setAlertForRoomIdAndUserIds	reply; updateThreadUsersSubscriptions
updateTypeByRoomId	saveRoomType
setBlockedByRoomId	meteor method blockUser
incUserMentionsAndUnreadForRoomIdAndUserIds	updateUsersSubscriptions
ignoreUser	meteor method ignoreUser
addRoleById	meteor methods addRoomLeader, addRoomModerator, and addRoomOwner
removeRoleById	removeRoomLeader, removeRoomModerator, and removeRoomOwner
setArchivedByUsername	deactivate > setArchivedByUsername; CROWD.syncDataToUser > setArchivedByUsername; meteor method setUserActiveStatus; ldapeemanager > disableMissingUsers and updateExistingUsers; ImportDataConverter > updateUser
updateUserHighlights	saveUserPreferences
updateNotificationUserPreferences	saveUserPreferences
clearNotificationUserPreferences	saveUserPreferences
removeByUserId	deleteUser
createWithRoomAndUser
createWithRoomAndManyUsers
removeByRoomIdAndUserId
removeByRoomIds
addUnreadThreadByRoomIdAndUserIds
removeUnreadThreadByRoomIdAndUserId
openByRoomIdAndUserId	meteor method openRoom
removeUnreadThreadsByRoomId

Steps to test or reproduce

Start RocketChat's application with the DISABLE_DB_WATCHERS environment variable set to true.
Perform HTTP requests or simulate use cases listed in the above table.

Further comments

To maintain consistency and avoid potential regressions, event names and signatures have been kept unchanged on both the client and app sides. This decision streamlines efforts and mitigates the risk of unintended consequences.

dionisio-bot · 2024-06-03T03:50:15Z

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

changeset-bot · 2024-06-03T03:50:15Z

⚠️ No Changeset found

Latest commit: 3af6943

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codecov · 2024-06-03T12:15:52Z

Codecov Report

Attention: Patch coverage is 23.07692% with 60 lines in your changes missing coverage. Please review.

Project coverage is 59.40%. Comparing base (dd37ea1) to head (3af6943).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #32540      +/-   ##
===========================================
- Coverage    59.43%   59.40%   -0.04%     
===========================================
  Files         2547     2547              
  Lines        63265    63310      +45     
  Branches     14237    14248      +11     
===========================================
+ Hits         37604    37611       +7     
- Misses       22941    22980      +39     
+ Partials      2720     2719       -1

Flag	Coverage Δ
unit	`75.84% <23.07%> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

debdutdeb

not adding same comment for all, but my first look :)

apps/meteor/app/autotranslate/server/methods/saveSettings.ts

… directly

- Replaced asynchronous function calls with synchronous calls followed by an if statement to check for modifiedCount before invoking the listener.

sampaiodiego

there are some inline comments... I've found these broken behaviors while testing:

when sending a message in a channel, other members' subscription were not being marked as unread.. weirdly enough, if a message has a mention to someone, it works as expected
change a channel to a group (from public to private) was not reflected in the UI

other than that, I'd ask to not use .then() when it is not needed, I've found it in a few different places.

apps/meteor/app/e2e/server/functions/handleSuggestedGroupKey.ts

apps/meteor/app/federation/server/endpoints/dispatch.js

apps/meteor/app/lib/server/functions/deleteUser.ts

apps/meteor/app/lib/server/functions/setUserActiveStatus.ts

apps/meteor/app/livechat/server/lib/Helper.ts

apps/meteor/ee/app/livechat-enterprise/server/hooks/onCloseLivechat.ts

apps/meteor/app/lib/server/lib/notifyListener.ts

…ion subscription

…ions-model-out-of-db-watcher

KevLehman

In some places you destructure the returned objects and in others you access them via ., think would be nice to be consistent and stick to one way where this makes sense.

apps/meteor/app/channel-settings/server/functions/saveRoomName.ts

KevLehman · 2024-08-21T18:37:20Z

apps/meteor/app/channel-settings/server/functions/saveRoomEncrypted.ts

@@ -27,7 +29,10 @@ export const saveRoomEncrypted = async function (rid: string, encrypted: boolean
 	}

 	if (encrypted) {
-		await Subscriptions.disableAutoTranslateByRoomId(rid);
+		const disableAutoTranslateResponse = await Subscriptions.disableAutoTranslateByRoomId(rid);


Suggested change

const disableAutoTranslateResponse = await Subscriptions.disableAutoTranslateByRoomId(rid);

const { modifiedCount } = await Subscriptions.disableAutoTranslateByRoomId(rid);

KevLehman · 2024-08-21T18:37:42Z

apps/meteor/app/channel-settings/server/functions/saveRoomCustomFields.ts


 	// Update customFields of any user's Subscription related with this rid
-	await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);
+	const updateCustomFields = await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);


Suggested change

const updateCustomFields = await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);

const { modifiedCount } = await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);

apps/meteor/app/e2e/server/functions/handleSuggestedGroupKey.ts

KevLehman · 2024-08-21T18:41:01Z

apps/meteor/app/lib/server/functions/cleanRoomHistory.ts

+
+			if (removedUnreadThreadsResponse.modifiedCount) {
+				for await (const id of subscriptionIds) {
+					await notifyOnSubscriptionChangedById(id);


Most of other calls are void promises, is there a reason to await for them here?

KevLehman · 2024-08-21T18:47:53Z

apps/meteor/app/lib/server/functions/createRoom.ts

+
+	if (Object.keys(insertedIds).length) {
+		for await (const insertedId of Object.values(insertedIds)) {
+			await notifyOnSubscriptionChangedById(insertedId, 'inserted');


Same, any reason for await this one?

KevLehman · 2024-08-21T18:48:31Z

apps/meteor/app/lib/server/functions/deleteRoom.ts


-	void notifyOnRoomChangedById(rid, 'removed');
+	(await Rooms.removeById(rid)).deletedCount && void notifyOnRoomChangedById(rid, 'removed');


Other places are using the if (deletedCount), think we should keep the same standard here.

KevLehman · 2024-08-21T18:49:21Z

apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts

 		Messages.removeByRoomIds(rids),
 		ReadReceipts.removeByRoomIds(rids),
 		Rooms.removeByIds(rids),
+		// no bulk deletion for files
+		...rids.map((rid) => FileUpload.removeFilesByRoomId(rid)),


IMHO, this change is not related to the watcher changes and should be in its own PR

KevLehman · 2024-08-21T18:50:25Z

apps/meteor/app/lib/server/lib/notifyListener.ts

+	): Promise<void> => {
+		const subscriptions = Subscriptions.findByUserIdAndRoomIds(uid, [rid], { projection: subscriptionFields });
+
+		for await (const subscription of subscriptions) {


Suggested change

for await (const subscription of subscriptions) {

for (const subscription of subscriptions) {

this is because of the AsyncIterator.. but in fact, we don't need to use it

KevLehman · 2024-08-21T18:50:49Z

apps/meteor/app/lib/server/lib/notifyListener.ts

+			projection: subscriptionFields,
+		});
+
+		for await (const subscription of subscriptions) {


Suggested change

for await (const subscription of subscriptions) {

for (const subscription of subscriptions) {

…ions-model-out-of-db-watcher

KevLehman · 2024-08-21T21:36:59Z

apps/meteor/app/lib/server/functions/closeLivechatRoom.ts

-		const subscriptionsLeft = await Subscriptions.countByRoomId(roomId);
-		if (subscriptionsLeft) {
-			await Subscriptions.removeByRoomId(roomId);
+		const { deletedCount } = await Subscriptions.removeByRoomId(roomId, {


Thanks man!

refactor: adds listener calls on Subscription related features

ff7fc86

fix: typecheck

e44c49d

ricardogarim mentioned this pull request Jun 3, 2024

refactor: mentions out of db watcher #31992

Closed

ricardogarim marked this pull request as ready for review June 3, 2024 11:42

ricardogarim requested review from a team as code owners June 3, 2024 11:42

test: fix broken saveUserIdentity

52596d4

lint fix

7463d42

debdutdeb requested changes Jun 3, 2024

View reviewed changes

apps/meteor/app/autotranslate/server/methods/saveSettings.ts Outdated Show resolved Hide resolved

apps/meteor/app/autotranslate/server/methods/saveSettings.ts Outdated Show resolved Hide resolved

apps/meteor/app/autotranslate/server/methods/saveSettings.ts Outdated Show resolved Hide resolved

ricardogarim and others added 6 commits June 4, 2024 21:59

refactor: adds listener calls on functions calling model's raw method…

054d24a

… directly

fix typecheck

b6a39af

refactor: change function calls to use if statement

ed1146d

- Replaced asynchronous function calls with synchronous calls followed by an if statement to check for modifiedCount before invoking the listener.

Merge from develop

2875834

lint and typecheck fixes

308934f

remove collection

091aea3

sampaiodiego requested changes Jun 21, 2024

View reviewed changes

ricardogarim added 7 commits July 1, 2024 09:27

refactor: change handleSuggestedGroupKey to initial behavior

818e8ef

refactor: remove listener call on ROOM_ADD_USER when updating federat…

3b0c0b7

…ion subscription

refactor: validate returned deletedCount before calling listener

dfa6712

refactor: changes then to await promises

f3a3695

refactor: notify all existing DMs from archived user

5f4465a

refactor: switch notifier function params names

937287c

Merge from develop

0a012a8

ricardogarim force-pushed the refactor/subscriptions-model-out-of-db-watcher branch from 71a3a1a to 0a012a8 Compare July 4, 2024 02:55

ricardogarim added 2 commits July 29, 2024 10:51

merge from develop

029da89

chore: typecheck fix

490d60e

sampaiodiego added 8 commits August 19, 2024 18:37

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

d7fa8ee

…ions-model-out-of-db-watcher

revert receipts change

a90822d

fix read receipts

33ec6b5

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

e2cc6a1

…ions-model-out-of-db-watcher

do not notify on user delete

620acd5

use findOneAndDelete

ab8c87a

implement onTrash

0d26bfe

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

b2ce5d3

…ions-model-out-of-db-watcher

sampaiodiego previously approved these changes Aug 21, 2024

View reviewed changes

sampaiodiego added this to the 6.12 milestone Aug 21, 2024

sampaiodiego added the stat: QA assured Means it has been tested and approved by a company insider label Aug 21, 2024

dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Aug 21, 2024

sampaiodiego added 2 commits August 21, 2024 11:43

fix unit tests

e9f39f1

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

02dbedf

…ions-model-out-of-db-watcher

sampaiodiego dismissed their stale review via 02dbedf August 21, 2024 14:44

fix lint

089aba2

KevLehman requested changes Aug 21, 2024

View reviewed changes

sampaiodiego added 4 commits August 21, 2024 16:38

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

c384e03

…ions-model-out-of-db-watcher

code review

8bdde3c

Merge remote-tracking branch 'origin/develop' into refactor/subscript…

630a1d9

…ions-model-out-of-db-watcher

use a cursor

ed4a171

sampaiodiego previously approved these changes Aug 21, 2024

View reviewed changes

KevLehman previously approved these changes Aug 21, 2024

View reviewed changes

revert return type change

3af6943

sampaiodiego dismissed stale reviews from KevLehman and themself via 3af6943 August 21, 2024 22:22

sampaiodiego approved these changes Aug 21, 2024

View reviewed changes

ggazzo merged commit 150c7b1 into develop Aug 21, 2024
46 of 49 checks passed

ggazzo deleted the refactor/subscriptions-model-out-of-db-watcher branch August 21, 2024 23:59

ricardogarim mentioned this pull request Sep 27, 2024

fix: race condition when forwarding livechat by splitting subscription removal #33381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Subscriptions out of DB Watcher #32540

refactor: Subscriptions out of DB Watcher #32540

ricardogarim commented Jun 3, 2024 •

edited

Loading

dionisio-bot bot commented Jun 3, 2024 •

edited

Loading

changeset-bot bot commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

debdutdeb left a comment

sampaiodiego left a comment

KevLehman left a comment

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

sampaiodiego Aug 21, 2024

KevLehman Aug 21, 2024

KevLehman Aug 21, 2024

	const disableAutoTranslateResponse = await Subscriptions.disableAutoTranslateByRoomId(rid);
	const { modifiedCount } = await Subscriptions.disableAutoTranslateByRoomId(rid);

	const updateCustomFields = await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);
	const { modifiedCount } = await Subscriptions.updateCustomFieldsByRoomId(rid, roomCustomFields);


		void notifyOnRoomChangedById(rid, 'removed');
		(await Rooms.removeById(rid)).deletedCount && void notifyOnRoomChangedById(rid, 'removed');

	for await (const subscription of subscriptions) {
	for (const subscription of subscriptions) {

refactor: Subscriptions out of DB Watcher #32540

refactor: Subscriptions out of DB Watcher #32540

Conversation

ricardogarim commented Jun 3, 2024 • edited Loading

Context

Proposed changes

Steps to test or reproduce

Further comments

dionisio-bot bot commented Jun 3, 2024 • edited Loading

changeset-bot bot commented Jun 3, 2024 • edited Loading

⚠️ No Changeset found

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

debdutdeb left a comment

Choose a reason for hiding this comment

sampaiodiego left a comment

Choose a reason for hiding this comment

KevLehman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardogarim commented Jun 3, 2024 •

edited

Loading

dionisio-bot bot commented Jun 3, 2024 •

edited

Loading

changeset-bot bot commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading