fix: update cached setting immediately at the time of updating the db (#32742)

.changeset/mean-hairs-move.md

+---
+'@rocket.chat/meteor': minor
+---
+
+Fixed an issue where adding `OVERWRITE_SETTING_` for any setting wasn't immediately taking effect sometimes, and needed a server restart to reflect.

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

@@ -73,7 +73,7 @@ const compareSettingsIgnoringKeys =
 			.filter((key) => !keys.includes(key as keyof ISetting))
 			.every((key) => isEqual(a[key as keyof ISetting], b[key as keyof ISetting]));
 
-const compareSettings = compareSettingsIgnoringKeys([
+export const compareSettings = compareSettingsIgnoringKeys([
 	'value',
 	'ts',
 	'createdAt',
@@ -166,6 +166,7 @@ export class SettingsRegistry {
 			})();
 
 			await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
+			this.store.set(settingFromCodeOverwritten);
 			return;
 		}
 
@@ -175,6 +176,7 @@ export class SettingsRegistry {
 				const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
 
 				await this.saveUpdatedSetting(_id, settingProps, removedKeys);
+				this.store.set(settingFromCodeOverwritten);
 			}
 			return;
 		}

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,22 +71,41 @@ 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++;
 	}
 
+	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 });
-
 		// 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);
+		}
 	}
 }
 

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

+import { expect } from 'chai';
+
+import { compareSettings } 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('#compareSettings', () => {
+	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(compareSettings(testSetting, copiedSetting)).to.be.true;
+		}),
+	);
+});

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

@@ -3,13 +3,21 @@ 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',
+});
+
 describe('Settings', () => {
 	beforeEach(() => {
 		Settings.insertCalls = 0;
 		Settings.upsertCalls = 0;
 		process.env = {};
+		Settings.setDelay(0);
 	});
 
 	it('should not insert the same setting twice', async () => {
@@ -440,6 +448,67 @@ describe('Settings', () => {
 			.to.not.have.any.keys('section');
 	});
 
+	it('should ignore setting object from code if only value changes and setting already stored', async () => {
+		const settings = new CachedSettings();
+		Settings.settings = settings;
+		settings.initialized();
+		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+
+		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 () => {
+		const settings = new CachedSettings();
+		Settings.settings = settings;
+		settings.initialized();
+		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+
+		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 () => {
+		const settings = new CachedSettings();
+		Settings.settings = settings;
+		settings.initialized();
+		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+
+		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 () => {
+		const settings = new CachedSettings();
+		Settings.settings = settings;
+		settings.initialized();
+		const settingsRegistry = new SettingsRegistry({ store: settings, model: Settings as any });
+
+		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();
@@ -502,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);
+	});
 });