fix: incorrect cache set review (#32848)

* test: cached settings expecting delay because changestream * test: change update test to not expect delays (expect to fail) * fix: change `SettingsRegistry` to update immediately
RocketChat · Jul 19, 2024 · 8931608 · 8931608
1 parent 88ae469
commit 8931608
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 42 deletions.
diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts
@@ -2,7 +2,6 @@ import type { ISetting, ISettingGroup, Optional, SettingValue } from '@rocket.ch
 import { isSettingEnterprise } from '@rocket.chat/core-typings';
 import { Emitter } from '@rocket.chat/emitter';
 import type { ISettingsModel } from '@rocket.chat/model-typings';
-import { type ModifyResult } from 'mongodb';
 import { isEqual } from 'underscore';
 
 import { SystemLogger } from '../../../server/lib/logger/system';
@@ -150,51 +149,34 @@ export class SettingsRegistry {
 
 		const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten);
 
+		const { _id: _, ...settingProps } = settingFromCodeOverwritten;
+
 		if (settingStored && !compareSettings(settingStored, settingFromCodeOverwritten)) {
 			const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten;
 
 			const overwrittenKeys = Object.keys(settingFromCodeOverwritten);
 			const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
 
-			const updatedProps = {
-				...settingOverwrittenProps,
-				...(settingStoredOverwritten &&
-					settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }),
-			};
-
-			if (!('value' in updatedProps)) {
-				// cache can only update if value changes
-				await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
-				return;
-			}
-
-			const { value: settingForCache } = await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
-
-			if (!settingForCache) {
-				// unreachable
-				throw new Error('No document returned after setting was updated due to metadata change, something is wrong with code');
-			}
-
-			this.store.set(settingForCache);
+			const updatedProps = (() => {
+				return {
+					...settingOverwrittenProps,
+					...(settingStoredOverwritten &&
+						settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }),
+				};
+			})();
 
+			await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
+			this.store.set(settingFromCodeOverwritten);
 			return;
 		}
 
-		const { _id: _, ...settingProps } = settingFromCodeOverwritten;
-
 		if (settingStored && isOverwritten) {
 			if (settingStored.value !== settingFromCodeOverwritten.value) {
 				const overwrittenKeys = Object.keys(settingFromCodeOverwritten);
 				const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
 
-				const { value: settingForCache } = await this.saveUpdatedSetting(_id, settingProps, removedKeys);
-
-				if (!settingForCache) {
-					// unreachable
-					throw new Error('No document returned due to an OVERWRITE, something is wrong with the code');
-				}
-
-				this.store.set(settingForCache);
+				await this.saveUpdatedSetting(_id, settingProps, removedKeys);
+				this.store.set(settingFromCodeOverwritten);
 			}
 			return;
 		}
@@ -289,16 +271,16 @@ export class SettingsRegistry {
 		_id: string,
 		settingProps: Omit<Optional<ISetting, 'value'>, '_id'>,
 		removedKeys?: string[],
-	): Promise<ModifyResult<ISetting>> {
-		return this.model.findOneAndUpdate(
+	): Promise<void> {
+		await this.model.updateOne(
 			{ _id },
 			{
 				$set: settingProps,
 				...(removedKeys?.length && {
 					$unset: removedKeys.reduce((unset, key) => ({ ...unset, [key]: 1 }), {}),
 				}),
 			},
-			{ upsert: true, returnDocument: 'after' },
+			{ upsert: true },
 		);
 	}
 }
diff --git a/apps/meteor/app/settings/server/functions/settings.mocks.ts b/apps/meteor/app/settings/server/functions/settings.mocks.ts
@@ -9,6 +9,12 @@ type Dictionary = {
 class SettingsClass {
 	settings: ICachedSettings;
 
+	private delay = 0;
+
+	setDelay(delay: number): void {
+		this.delay = delay;
+	}
+
 	find(): any[] {
 		return [];
 	}
@@ -65,12 +71,21 @@ class SettingsClass {
 			throw new Error('Invalid upsert');
 		}
 
-		// console.log(query, data);
-		this.data.set(query._id, data);
-
-		// Can't import before the mock command on end of this file!
-		// eslint-disable-next-line @typescript-eslint/no-var-requires
-		this.settings.set(data);
+		if (this.delay) {
+			setTimeout(() => {
+				// console.log(query, data);
+				this.data.set(query._id, data);
+
+				// Can't import before the mock command on end of this file!
+				// eslint-disable-next-line @typescript-eslint/no-var-requires
+				this.settings.set(data);
+			}, this.delay);
+		} else {
+			this.data.set(query._id, data);
+			// Can't import before the mock command on end of this file!
+			// eslint-disable-next-line @typescript-eslint/no-var-requires
+			this.settings.set(data);
+		}
 
 		this.upsertCalls++;
 	}
@@ -82,10 +97,15 @@ class SettingsClass {
 
 	updateValueById(id: string, value: any): void {
 		this.data.set(id, { ...this.data.get(id), value });
-
 		// Can't import before the mock command on end of this file!
 		// eslint-disable-next-line @typescript-eslint/no-var-requires
-		this.settings.set(this.data.get(id) as ISetting);
+		if (this.delay) {
+			setTimeout(() => {
+				this.settings.set(this.data.get(id) as ISetting);
+			}, this.delay);
+		} else {
+			this.settings.set(this.data.get(id) as ISetting);
+		}
 	}
 }
 

diff --git a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts b/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts
@@ -17,6 +17,7 @@ describe('Settings', () => {
 		Settings.insertCalls = 0;
 		Settings.upsertCalls = 0;
 		process.env = {};
+		Settings.setDelay(0);
 	});
 
 	it('should not insert the same setting twice', async () => {
@@ -570,4 +571,19 @@ describe('Settings', () => {
 			}, settings.getConfig({ debounce: 10 }).debounce);
 		});
 	});
+
+	it('should update the stored value on setting change', async () => {
+		Settings.setDelay(10);
+		process.env[`OVERWRITE_SETTING_${testSetting._id}`] = 'false';
+		const settings = new CachedSettings();
+		Settings.settings = settings;
+
+		settings.set(testSetting);
+		settings.initialized();
+
+		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		expect(settings.get(testSetting._id)).to.be.equal(false);
+	});
 });