[FIX] `MongoInvalidArgumentError` on overwriting existing setting (#26880)

Co-authored-by: Diego Sampaio <chinello@gmail.com>

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

@@ -1,6 +1,6 @@
 import { Emitter } from '@rocket.chat/emitter';
 import { isEqual } from 'underscore';
-import type { ISetting, ISettingGroup, SettingValue } from '@rocket.chat/core-typings';
+import type { ISetting, ISettingGroup, Optional, SettingValue } from '@rocket.chat/core-typings';
 import { isSettingEnterprise } from '@rocket.chat/core-typings';
 import type { ISettingsModel } from '@rocket.chat/model-typings';
 
@@ -157,23 +157,24 @@ export class SettingsRegistry {
 			const overwrittenKeys = Object.keys(settingFromCodeOverwritten);
 			const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
 
-			await this.model.updateOne(
-				{ _id },
-				{
-					$set: { ...settingOverwrittenProps },
-					...(removedKeys.length && {
-						$unset: removedKeys.reduce((unset, key) => ({ ...unset, [key]: 1 }), {}),
-					}),
-				},
-				{ upsert: true },
-			);
+			const updatedProps = (() => {
+				return {
+					...settingOverwrittenProps,
+					...(settingStoredOverwritten &&
+						settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }),
+				};
+			})();
 
+			await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
 			return;
 		}
 
 		if (settingStored && isOverwritten) {
 			if (settingStored.value !== settingFromCodeOverwritten.value) {
-				await this.model.updateOne({ _id }, settingProps, { upsert: true });
+				const overwrittenKeys = Object.keys(settingFromCodeOverwritten);
+				const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key));
+
+				await this.saveUpdatedSetting(_id, settingProps, removedKeys);
 			}
 			return;
 		}
@@ -263,4 +264,21 @@ export class SettingsRegistry {
 
 		return groupSetWith({ group: _id })({}, callback);
 	}
+
+	private async saveUpdatedSetting(
+		_id: string,
+		settingProps: Omit<Optional<ISetting, 'value'>, '_id'>,
+		removedKeys?: string[],
+	): Promise<void> {
+		await this.model.updateOne(
+			{ _id },
+			{
+				$set: settingProps,
+				...(removedKeys?.length && {
+					$unset: removedKeys.reduce((unset, key) => ({ ...unset, [key]: 1 }), {}),
+				}),
+			},
+			{ upsert: true },
+		);
+	}
 }

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

@@ -40,7 +40,7 @@ class SettingsClass {
 		this.insertCalls++;
 	}
 
-	updateOne(query: any, update: any): void {
+	updateOne(query: any, update: any, options?: any): void {
 		const existent = this.findOne(query);
 
 		const data = { ...existent, ...query, ...update, ...update.$set };
@@ -49,6 +49,22 @@ class SettingsClass {
 			Object.assign(data, update.$setOnInsert);
 		}
 
+		if (update.$unset) {
+			Object.keys(update.$unset).forEach((key) => {
+				delete data[key];
+			});
+		}
+
+		const modifiers = ['$set', '$setOnInsert', '$unset'];
+
+		modifiers.forEach((key) => {
+			delete data[key];
+		});
+
+		if (options?.upsert === true && !modifiers.some((key) => Object.keys(update).includes(key))) {
+			throw new Error('Invalid upsert');
+		}
+
 		// console.log(query, data);
 		this.data.set(query._id, data);
 

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

@@ -298,6 +298,66 @@ describe('Settings', () => {
 		expect(Settings.findOne({ _id: 'my_setting' })).to.include({ ...expectedSetting });
 	});
 
+	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', function () {
+			this.section('section', function () {
+				this.add('my_setting', 0, {
+					type: 'int',
+					sorter: 0,
+				});
+			});
+		});
+
+		const expectedSetting = {
+			value: 0,
+			type: 'int',
+			sorter: 0,
+			group: 'group',
+			section: 'section',
+			packageValue: 0,
+			hidden: false,
+			blocked: false,
+			secret: false,
+			i18nLabel: 'my_setting',
+			i18nDescription: 'my_setting_Description',
+			autocomplete: true,
+		};
+
+		expect(Settings.insertCalls).to.be.equal(2);
+		expect(Settings.upsertCalls).to.be.equal(0);
+		expect(Settings.findOne({ _id: 'my_setting' })).to.include(expectedSetting);
+
+		process.env.OVERWRITE_SETTING_my_setting = '1';
+		await settingsRegistry.addGroup('group', function () {
+			// removed section
+			this.add('my_setting', 0, {
+				type: 'int',
+				sorter: 0,
+			});
+		});
+
+		expect(Settings.insertCalls).to.be.equal(2);
+		expect(Settings.upsertCalls).to.be.equal(1);
+
+		const { section: _section, ...removedSection } = expectedSetting;
+
+		const settingWithoutSection = {
+			...removedSection,
+			value: 1,
+			processEnvValue: 1,
+			valueSource: 'processEnvValue',
+		};
+
+		expect(Settings.findOne({ _id: 'my_setting' }))
+			.to.include({ ...settingWithoutSection })
+			.to.not.have.any.keys('section');
+	});
+
 	it('should call `settings.get` callback on setting added', async () => {
 		return new Promise(async (resolve) => {
 			const settings = new CachedSettings();