fix: update cached setting immediately at the time of updating the db

apps/meteor/app/settings/server/SettingsRegistry.ts

@@ -2,6 +2,7 @@
 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';
@@ -73,7 +74,8 @@
 			.filter((key) => !keys.includes(key as keyof ISetting))
 			.every((key) => isEqual(a[key as keyof ISetting], b[key as keyof ISetting]));
 
-const compareSettings = compareSettingsIgnoringKeys([
+// NOTE(debdut): not really "metadata" - but yeah. I don't have a better name
+export const compareSettingsMetadata = compareSettingsIgnoringKeys([
 	'value',
 	'ts',
 	'createdAt',
@@ -98,7 +100,7 @@
 	/*
 	 * Add a setting
 	 */
 	async add(_id: string, value: SettingValue, { sorter, section, group, ...options }: ISettingAddOptions = {}): Promise<void> {
 		if (!_id || value == null) {
 			throw new Error('Invalid arguments');
 		}
@@ -138,53 +140,66 @@
 
 		const settingFromCodeOverwritten = overwriteSetting(settingFromCode);
 
-		const settingOverwrittenDefault = overrideSetting(settingFromCode);
-
 		const settingStored = this.store.getSetting(_id);
 
 		const settingStoredOverwritten = settingStored && overwriteSetting(settingStored);
 
-		const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten);
-
-		const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingFromCodeOverwritten : settingOverwrittenDefault;
-
 		try {
 			validateSetting(settingFromCode._id, settingFromCode.type, settingFromCode.value);
 		} catch (e) {
 			IS_DEVELOPMENT && SystemLogger.error(`Invalid setting code ${_id}: ${(e as Error).message}`);
 		}
 
-		const { _id: _, ...settingProps } = settingFromCodeOverwritten;
+		const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten);
+
+		if (settingStored && !compareSettingsMetadata(settingStored, settingFromCodeOverwritten)) {
+			// we need to update the setting metadata here
 
-		if (settingStored && !compareSettings(settingStored, settingFromCodeOverwritten)) {
-			const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten;
+			const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten; // settingStoredOverwrite not used since we need the updated props, that is in code not db
 
 			const overwrittenKeys = Object.keys(settingFromCodeOverwritten);
 			const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
 
-			const updatedProps = (() => {
-				return {
-					...settingOverwrittenProps,
-					...(settingStoredOverwritten &&
-						settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }),
-				};
-			})();
+			const updatedProps = {
+				...settingOverwrittenProps,
+				...(settingStoredOverwritten &&
+					settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }),
+			};
 
-			await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
+			if (!('value' in updatedProps)) {
+				// cache can only update if value changes
+				await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
+				return;
+			}
 
-			this.store.set(updatedSettingAfterApplyingOverwrite);
+			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);
 
 			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));
 
-				await this.saveUpdatedSetting(_id, settingProps, removedKeys);
+				const { value: settingForCache } = await this.saveUpdatedSetting(_id, settingProps, removedKeys);
 
-				this.store.set(updatedSettingAfterApplyingOverwrite);
+				if (!settingForCache) {
+					// unreachable
+					throw new Error('No document returned due to an OVERWRITE, something is wrong with the code');
+				}
+
+				// settingStored exists, so will overwritten
+				this.store.set(settingForCache);
 			}
 			return;
 		}
@@ -198,9 +213,13 @@
 			return;
 		}
 
-		await this.model.insertOne(updatedSettingAfterApplyingOverwrite); // no need to emit unless we remove the oplog
+		const settingOverwrittenDefault = overrideSetting(settingFromCode);
+
+		const setting = isOverwritten ? settingFromCodeOverwritten : settingOverwrittenDefault;
+
+		await this.model.insertOne(setting); // no need to emit unless we remove the oplog
 
-		this.store.set(updatedSettingAfterApplyingOverwrite);
+		this.store.set(setting);
 	}
 
 	/*
@@ -275,16 +294,16 @@
 		_id: string,
 		settingProps: Omit<Optional<ISetting, 'value'>, '_id'>,
 		removedKeys?: string[],
-	): Promise<void> {
-		await this.model.updateOne(
+	): Promise<ModifyResult<ISetting>> {
+		return this.model.findOneAndUpdate(
 			{ _id },
 			{
 				$set: settingProps,
 				...(removedKeys?.length && {
 					$unset: removedKeys.reduce((unset, key) => ({ ...unset, [key]: 1 }), {}),
 				}),
 			},
-			{ upsert: true },
+			{ upsert: true, returnDocument: 'after' },
 		);
 	}
 }

apps/meteor/app/settings/server/functions/settings.mocks.ts

@@ -75,6 +75,11 @@ class SettingsClass {
 		this.upsertCalls++;
 	}
 
+	findOneAndUpdate({ _id }: { _id: string }, value: any, options?: any) {
+		this.updateOne({ _id }, value, options);
+		return { value: this.findOne({ _id }) };
+	}
+
 	updateValueById(id: string, value: any): void {
 		this.data.set(id, { ...this.data.get(id), value });
 

apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts

@@ -0,0 +1,28 @@
+import { expect } from 'chai';
+
+import { compareSettingsMetadata } from '../../../../../../app/settings/server/SettingsRegistry';
+import { getSettingDefaults } from '../../../../../../app/settings/server/functions/getSettingDefaults';
+
+const testSetting = getSettingDefaults({
+	_id: 'my_dummy_setting',
+	type: 'string',
+	value: 'dummy',
+});
+
+describe('#compareSettingsMetadata', () => {
+	const ignoredKeys = ['value', 'ts', 'createdAt', 'valueSource', 'packageValue', 'processEnvValue', '_updatedAt'];
+
+	ignoredKeys.forEach((key) =>
+		it(`should return true if ${key} changes`, () => {
+			const copiedSetting: any = { ...testSetting };
+
+			if (['ts', 'createdAt', '_updatedAt'].includes(key)) {
+				copiedSetting[key] = new Date();
+			} else {
+				copiedSetting[key] = 'random';
+			}
+
+			expect(compareSettingsMetadata(testSetting, copiedSetting)).to.be.true;
+		}),
+	);
+});

apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts

@@ -3,20 +3,32 @@ import { beforeEach, describe, it } from 'mocha';
 
 import { CachedSettings } from '../../../../../../app/settings/server/CachedSettings';
 import { SettingsRegistry } from '../../../../../../app/settings/server/SettingsRegistry';
+import { getSettingDefaults } from '../../../../../../app/settings/server/functions/getSettingDefaults';
 import { Settings } from '../../../../../../app/settings/server/functions/settings.mocks';
 
+const testSetting = getSettingDefaults({
+	_id: 'my_dummy_setting',
+	type: 'string',
+	value: 'dummy',
+});
+
+const settings = new CachedSettings();
+settings.initialized();
+
+Settings.settings = settings;
+
+const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+
 describe('Settings', () => {
 	beforeEach(() => {
+		settings.store.clear();
 		Settings.insertCalls = 0;
 		Settings.upsertCalls = 0;
+		Settings.data.clear();
 		process.env = {};
 	});
 
 	it('should not insert the same setting twice', async () => {
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting', true, {
@@ -76,11 +88,6 @@ describe('Settings', () => {
 	});
 
 	it('should respect override via environment as int', async () => {
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
-
 		process.env.OVERWRITE_SETTING_my_setting = '1';
 
 		await settingsRegistry.addGroup('group', async function () {
@@ -136,10 +143,6 @@ describe('Settings', () => {
 	it('should respect override via environment as boolean', async () => {
 		process.env.OVERWRITE_SETTING_my_setting_bool = 'true';
 
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting_bool', false, {
@@ -193,10 +196,6 @@ describe('Settings', () => {
 	it('should respect override via environment as string', async () => {
 		process.env.OVERWRITE_SETTING_my_setting_str = 'hey';
 
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting_str', '', {
@@ -249,10 +248,6 @@ describe('Settings', () => {
 	});
 
 	it('should work with a setting type multiSelect with a default value', async () => {
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting_multiselect', ['a'], {
@@ -274,11 +269,6 @@ describe('Settings', () => {
 	it('should respect override via environment as multiSelect', async () => {
 		process.env.OVERWRITE_SETTING_my_setting_multiselect = '["a","b"]';
 
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
-
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting_multiselect', ['a'], {
@@ -303,11 +293,6 @@ describe('Settings', () => {
 	it('should ignore override via environment as multiSelect if value is invalid', async () => {
 		process.env.OVERWRITE_SETTING_my_setting_multiselect = '[INVALID_ARRAY]';
 
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
-
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting_multiselect', ['a'], {
@@ -331,10 +316,6 @@ describe('Settings', () => {
 
 	it('should respect initial value via environment', async () => {
 		process.env.my_setting = '1';
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
@@ -381,11 +362,6 @@ describe('Settings', () => {
 	});
 
 	it('should respect override via environment when changing settings props', async () => {
-		const settings = new CachedSettings();
-		Settings.settings = settings;
-		settings.initialized();
-		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
-
 		await settingsRegistry.addGroup('group', async function () {
 			await this.section('section', async function () {
 				await this.add('my_setting', 0, {
@@ -440,13 +416,49 @@ describe('Settings', () => {
 			.to.not.have.any.keys('section');
 	});
 
+	it('should ignore setting object from code if only value changes and setting already stored', async () => {
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		expect(Settings.insertCalls).to.be.equal(1);
+		Settings.insertCalls = 0;
+
+		const settingFromCodeFaked = { ...testSetting, value: Date.now().toString() };
+
+		await settingsRegistry.add(settingFromCodeFaked._id, settingFromCodeFaked.value, settingFromCodeFaked);
+
+		expect(Settings.insertCalls).to.be.equal(0);
+		expect(Settings.upsertCalls).to.be.equal(0);
+	});
+
+	it('should ignore value from environment if setting is already stored', async () => {
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		process.env[testSetting._id] = Date.now().toString();
+
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		expect(Settings.findOne({ _id: testSetting._id }).value).to.be.equal(testSetting.value);
+	});
+
+	it('should update setting cache synchronously if overwrite is available in enviornment', async () => {
+		process.env[`OVERWRITE_SETTING_${testSetting._id}`] = Date.now().toString();
+
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		expect(settings.get(testSetting._id)).to.be.equal(process.env[`OVERWRITE_SETTING_${testSetting._id}`]);
+	});
+
+	it('should update cached value with OVERWRITE_SETTING value even if both with-prefixed and without-prefixed variables exist', async () => {
+		process.env[`OVERWRITE_SETTING_${testSetting._id}`] = Date.now().toString();
+		process.env[testSetting._id] = Date.now().toString();
+
+		await settingsRegistry.add(testSetting._id, testSetting.value, testSetting);
+
+		expect(Settings.findOne({ _id: testSetting._id }).value).to.be.equal(process.env[`OVERWRITE_SETTING_${testSetting._id}`]);
+	});
+
 	it('should call `settings.get` callback on setting added', async () => {
 		return new Promise(async (resolve) => {
-			const settings = new CachedSettings();
-			Settings.settings = settings;
-			settings.initialized();
-			const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
-
 			const spiedCallback1 = spy();
 			const spiedCallback2 = spy();
 
@@ -475,9 +487,6 @@ describe('Settings', () => {
 		return new Promise(async (resolve) => {
 			const spiedCallback1 = spy();
 			const spiedCallback2 = spy();
-			const settings = new CachedSettings();
-			Settings.settings = settings;
-			const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
 
 			settings.watch('setting_callback', spiedCallback1, { debounce: 1 });
 			settings.watchByRegex(/setting_callback/gi, spiedCallback2, { debounce: 1 });