From c4841a29950282e209d7b590f4bab2cc9268e0dc Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Mon, 8 Jul 2024 17:48:09 +0530 Subject: [PATCH 01/25] fix: use correct setting for cache when not overwritten --- apps/meteor/app/settings/server/SettingsRegistry.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 443e38ce5d63..ab8646791369 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -138,15 +138,13 @@ export class SettingsRegistry { 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; + const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingFromCodeOverwritten : settingFromCode; try { validateSetting(settingFromCode._id, settingFromCode.type, settingFromCode.value); From 2f33e7c7338d90efade73d3199ac5e06340bfc41 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Fri, 12 Jul 2024 19:41:56 +0530 Subject: [PATCH 02/25] fix registry:add and add some unit tests for it --- .../app/settings/server/SettingsRegistry.ts | 8 +- .../server/settings/SettingsRegistry.spec.ts | 88 +++++++++++++++++++ 2 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index ab8646791369..1fdb2366ba3e 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -144,7 +144,7 @@ export class SettingsRegistry { const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); - const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingFromCodeOverwritten : settingFromCode; + const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingFromCodeOverwritten : settingStored ?? settingFromCode; try { validateSetting(settingFromCode._id, settingFromCode.type, settingFromCode.value); @@ -196,9 +196,11 @@ export class SettingsRegistry { return; } - await this.model.insertOne(updatedSettingAfterApplyingOverwrite); // no need to emit unless we remove the oplog + const setting = isOverwritten ? updatedSettingAfterApplyingOverwrite : overrideSetting(settingFromCode); - this.store.set(updatedSettingAfterApplyingOverwrite); + await this.model.insertOne(setting); // no need to emit unless we remove the oplog + + this.store.set(setting); } /* diff --git a/apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts b/apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts new file mode 100644 index 000000000000..4fee8cf9f829 --- /dev/null +++ b/apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts @@ -0,0 +1,88 @@ +import { expect } from 'chai'; +import sinon from 'sinon'; + +import type { ISettingsModel } from '@rocket.chat/model-typings'; + +import { SettingsRegistry } from '../../../../../app/settings/server/SettingsRegistry'; +import { settings } from '../../../../../app/settings/server/cached'; + +const model = { + insertOne: sinon.stub(), + updateOne: sinon.stub(), +} as unknown as ISettingsModel; + +const registry = new SettingsRegistry({ store: settings, model }); + +const testSetting = { + _id: 'Dummy_setting', + packageValue: 'test', + value: 'test', + group: 'Testing', + section: 'Registry', +}; + +function addSetting(maybeSetting?: any) { + const setting = maybeSetting || testSetting; + return registry.add(setting._id, setting.packageValue, setting); +} + +describe('SettingsRegistry operations', () => { + describe('#add', () => { + beforeEach(() => { + settings.store.clear(); + }); + + it('should set the setting value from code when nothing is loaded into the cache and no overwrite available', async () => { + await addSetting(); + + expect(settings.get(testSetting._id)).to.be.equal(testSetting.packageValue); + }); + + it('should NOT set the setting value from code when nothing is loaded into the cache and no overwrite available', async () => { + settings.set(testSetting as any); + + const settingFromCodeFaked = { ...testSetting, value: 'new value' }; + + await addSetting(settingFromCodeFaked); + + expect(settings.get(settingFromCodeFaked._id)).to.be.equal(testSetting.value); + }); + + it('should set the setting value found in environment without OVERWRITE_SETTING_ prefix, if nothing is in cache already', async () => { + process.env[testSetting._id] = 'overriden value'; + + await addSetting(); + + expect(settings.get(testSetting._id)).to.be.equal('overriden value'); + }); + + it('should NOT set the setting value found in environment without OVERWRITE_SETTING_ prefix, if value is in cache already', async () => { + settings.set(testSetting as any); + + process.env[testSetting._id] = 'overriden value'; + + await addSetting(); + + expect(settings.get(testSetting._id)).to.be.equal(testSetting.value); + }); + + it('should update cached value if OVERWRITE_SETTING_ prefix is found', async () => { + settings.set(testSetting as any); + + process.env[`OVERWRITE_SETTING_${testSetting._id}`] = 'new value'; + + await addSetting(); + + expect(settings.get(testSetting._id)).to.be.equal('new value'); + }); + + it('should update cached value with OVERWRITE_SETTING value even if both with-prefixed and without-prefixed variabled exist', async () => { + process.env[`OVERWRITE_SETTING_${testSetting._id}`] = 'overwritten'; + process.env[testSetting._id] = 'overriden'; + + await addSetting(); + + expect(settings.get(testSetting._id)).to.be.equal('overwritten'); + }); + }); +}); From 51619fc1cdfcedbe0803c0454be9b8ff5c61a51a Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 18:08:41 +0530 Subject: [PATCH 03/25] move tests to non-ee folder --- .../unit/server/startup}/SettingsRegistry.spec.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) rename apps/meteor/{ee/tests/unit/server/settings => tests/unit/server/startup}/SettingsRegistry.spec.ts (99%) diff --git a/apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts similarity index 99% rename from apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts rename to apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts index 4fee8cf9f829..e0b2f3ece811 100644 --- a/apps/meteor/ee/tests/unit/server/settings/SettingsRegistry.spec.ts +++ b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts @@ -1,8 +1,7 @@ +import type { ISettingsModel } from '@rocket.chat/model-typings'; import { expect } from 'chai'; import sinon from 'sinon'; -import type { ISettingsModel } from '@rocket.chat/model-typings'; - import { SettingsRegistry } from '../../../../../app/settings/server/SettingsRegistry'; import { settings } from '../../../../../app/settings/server/cached'; From b3a00e1e6be879abe9595faec2a28812962d6479 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 18:11:24 +0530 Subject: [PATCH 04/25] fix import path --- .../meteor/tests/unit/server/startup/SettingsRegistry.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts index e0b2f3ece811..b6326c43730f 100644 --- a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts +++ b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts @@ -2,8 +2,8 @@ import type { ISettingsModel } from '@rocket.chat/model-typings'; import { expect } from 'chai'; import sinon from 'sinon'; -import { SettingsRegistry } from '../../../../../app/settings/server/SettingsRegistry'; -import { settings } from '../../../../../app/settings/server/cached'; +import { SettingsRegistry } from '../../../../app/settings/server/SettingsRegistry'; +import { settings } from '../../../../app/settings/server/cached'; const model = { insertOne: sinon.stub(), From d58743805582b190ec20c7c8859a23786ddad3cd Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 18:17:41 +0530 Subject: [PATCH 05/25] some comment --- apps/meteor/app/settings/server/SettingsRegistry.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 1fdb2366ba3e..9eec208d0bdc 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -196,6 +196,12 @@ export class SettingsRegistry { return; } + /* + * At this point setting is not in db, so we either respect OVERWRITE_SETTING_ prefix + * if it exists, or + * Use the first startup overriden default value, the setting-id environment variable. + */ + const setting = isOverwritten ? updatedSettingAfterApplyingOverwrite : overrideSetting(settingFromCode); await this.model.insertOne(setting); // no need to emit unless we remove the oplog From f6ff46a1afc4ff58c9995b93b1cd9789d6c17bf2 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 18:35:37 +0530 Subject: [PATCH 06/25] ... --- .../app/settings/server/SettingsRegistry.ts | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 9eec208d0bdc..3004c56c1d37 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -136,15 +136,15 @@ export class SettingsRegistry { throw new Error(`Enterprise setting ${_id} is missing the invalidValue option`); } - const settingFromCodeOverwritten = overwriteSetting(settingFromCode); - const settingStored = this.store.getSetting(_id); - const settingStoredOverwritten = settingStored && overwriteSetting(settingStored); + const settingOverwritten = overwriteSetting(settingStored ?? settingFromCode); + + const settingOverwrittenDefault = overrideSetting(settingStored ?? settingFromCode); - const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); + const isOverwritten = settingFromCode !== settingOverwritten || (settingStored && settingStored !== settingOverwritten); - const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingFromCodeOverwritten : settingStored ?? settingFromCode; + const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingOverwritten : settingOverwrittenDefault; try { validateSetting(settingFromCode._id, settingFromCode.type, settingFromCode.value); @@ -152,19 +152,18 @@ export class SettingsRegistry { IS_DEVELOPMENT && SystemLogger.error(`Invalid setting code ${_id}: ${(e as Error).message}`); } - const { _id: _, ...settingProps } = settingFromCodeOverwritten; + const { _id: _, ...settingProps } = settingOverwritten; - if (settingStored && !compareSettings(settingStored, settingFromCodeOverwritten)) { - const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten; + if (settingStored && !compareSettings(settingStored, settingOverwritten)) { + const { value: _value, ...settingOverwrittenProps } = settingOverwritten; - const overwrittenKeys = Object.keys(settingFromCodeOverwritten); + const overwrittenKeys = Object.keys(settingOverwritten); 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 }), + ...(settingStored.value !== settingOverwritten.value && { value: settingOverwritten.value }), }; })(); @@ -176,8 +175,8 @@ export class SettingsRegistry { } if (settingStored && isOverwritten) { - if (settingStored.value !== settingFromCodeOverwritten.value) { - const overwrittenKeys = Object.keys(settingFromCodeOverwritten); + if (settingStored.value !== settingOverwritten.value) { + const overwrittenKeys = Object.keys(settingOverwritten); const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key)); await this.saveUpdatedSetting(_id, settingProps, removedKeys); @@ -196,17 +195,9 @@ export class SettingsRegistry { return; } - /* - * At this point setting is not in db, so we either respect OVERWRITE_SETTING_ prefix - * if it exists, or - * Use the first startup overriden default value, the setting-id environment variable. - */ - - const setting = isOverwritten ? updatedSettingAfterApplyingOverwrite : overrideSetting(settingFromCode); - - await this.model.insertOne(setting); // no need to emit unless we remove the oplog + await this.model.insertOne(updatedSettingAfterApplyingOverwrite); // no need to emit unless we remove the oplog - this.store.set(setting); + this.store.set(updatedSettingAfterApplyingOverwrite); } /* From 53c2f3030a20f94ad5cf0e428a52d9e3f83f61e6 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 18:37:38 +0530 Subject: [PATCH 07/25] fix test names --- .../meteor/tests/unit/server/startup/SettingsRegistry.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts index b6326c43730f..d29d1eed42b1 100644 --- a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts +++ b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts @@ -37,7 +37,7 @@ describe('SettingsRegistry operations', () => { expect(settings.get(testSetting._id)).to.be.equal(testSetting.packageValue); }); - it('should NOT set the setting value from code when nothing is loaded into the cache and no overwrite available', async () => { + it('should NOT set the setting value from code when setting is loaded into the cache and no overwrite available', async () => { settings.set(testSetting as any); const settingFromCodeFaked = { ...testSetting, value: 'new value' }; @@ -75,7 +75,7 @@ describe('SettingsRegistry operations', () => { expect(settings.get(testSetting._id)).to.be.equal('new value'); }); - it('should update cached value with OVERWRITE_SETTING value even if both with-prefixed and without-prefixed variabled exist', async () => { + 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}`] = 'overwritten'; process.env[testSetting._id] = 'overriden'; From 593aea21b24922673fb1de8fa14e3b38a64e86a3 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Sat, 13 Jul 2024 21:55:40 +0530 Subject: [PATCH 08/25] reset file first --- .../app/settings/server/SettingsRegistry.ts | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 3004c56c1d37..5783e2946dc1 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -136,15 +136,10 @@ export class SettingsRegistry { throw new Error(`Enterprise setting ${_id} is missing the invalidValue option`); } - const settingStored = this.store.getSetting(_id); - - const settingOverwritten = overwriteSetting(settingStored ?? settingFromCode); - - const settingOverwrittenDefault = overrideSetting(settingStored ?? settingFromCode); + const settingFromCodeOverwritten = overwriteSetting(settingFromCode); - const isOverwritten = settingFromCode !== settingOverwritten || (settingStored && settingStored !== settingOverwritten); - - const updatedSettingAfterApplyingOverwrite = isOverwritten ? settingOverwritten : settingOverwrittenDefault; + const settingStored = this.store.getSetting(_id); + const settingStoredOverwritten = settingStored && overwriteSetting(settingStored); try { validateSetting(settingFromCode._id, settingFromCode.type, settingFromCode.value); @@ -152,36 +147,34 @@ export class SettingsRegistry { IS_DEVELOPMENT && SystemLogger.error(`Invalid setting code ${_id}: ${(e as Error).message}`); } - const { _id: _, ...settingProps } = settingOverwritten; + const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); + + const { _id: _, ...settingProps } = settingFromCodeOverwritten; - if (settingStored && !compareSettings(settingStored, settingOverwritten)) { - const { value: _value, ...settingOverwrittenProps } = settingOverwritten; + if (settingStored && !compareSettings(settingStored, settingFromCodeOverwritten)) { + const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten; - const overwrittenKeys = Object.keys(settingOverwritten); + const overwrittenKeys = Object.keys(settingFromCodeOverwritten); const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key)); const updatedProps = (() => { return { ...settingOverwrittenProps, - ...(settingStored.value !== settingOverwritten.value && { value: settingOverwritten.value }), + ...(settingStoredOverwritten && + settingStored.value !== settingStoredOverwritten.value && { value: settingStoredOverwritten.value }), }; })(); await this.saveUpdatedSetting(_id, updatedProps, removedKeys); - - this.store.set(updatedSettingAfterApplyingOverwrite); - return; } if (settingStored && isOverwritten) { - if (settingStored.value !== settingOverwritten.value) { - const overwrittenKeys = Object.keys(settingOverwritten); + 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); - - this.store.set(updatedSettingAfterApplyingOverwrite); } return; } @@ -195,9 +188,13 @@ export class SettingsRegistry { 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); } /* From 6c1bb5b92a1fe8501eb2883e0be5b993d6a38e56 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Mon, 15 Jul 2024 13:42:00 +0530 Subject: [PATCH 09/25] update --- .../app/settings/server/SettingsRegistry.ts | 51 +++++++++++----- .../server/startup/SettingsRegistry.spec.ts | 58 ++++++++++++++----- 2 files changed, 78 insertions(+), 31 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 5783e2946dc1..f169fc94913c 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -2,6 +2,7 @@ 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'; @@ -73,7 +74,8 @@ 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([ +// NOTE(debdut): not really "metadata" - but yeah. I don't have a better name +export const compareSettingsMetadata = compareSettingsIgnoringKeys([ 'value', 'ts', 'createdAt', @@ -139,6 +141,7 @@ export class SettingsRegistry { const settingFromCodeOverwritten = overwriteSetting(settingFromCode); const settingStored = this.store.getSetting(_id); + const settingStoredOverwritten = settingStored && overwriteSetting(settingStored); try { @@ -149,32 +152,48 @@ export class SettingsRegistry { const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); - const { _id: _, ...settingProps } = settingFromCodeOverwritten; + 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 }), + }; + + 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); - await this.saveUpdatedSetting(_id, updatedProps, removedKeys); 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); + + 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; } @@ -269,8 +288,8 @@ export class SettingsRegistry { _id: string, settingProps: Omit, '_id'>, removedKeys?: string[], - ): Promise { - await this.model.updateOne( + ): Promise> { + return this.model.findOneAndUpdate( { _id }, { $set: settingProps, @@ -278,7 +297,7 @@ export class SettingsRegistry { $unset: removedKeys.reduce((unset, key) => ({ ...unset, [key]: 1 }), {}), }), }, - { upsert: true }, + { upsert: true, returnDocument: 'after' }, ); } } diff --git a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts index d29d1eed42b1..c4a0f5a1910f 100644 --- a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts +++ b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts @@ -1,30 +1,37 @@ import type { ISettingsModel } from '@rocket.chat/model-typings'; -import { expect } from 'chai'; +import { expect, spy } from 'chai'; import sinon from 'sinon'; -import { SettingsRegistry } from '../../../../app/settings/server/SettingsRegistry'; +import { SettingsRegistry, compareSettingsMetadata } from '../../../../app/settings/server/SettingsRegistry'; import { settings } from '../../../../app/settings/server/cached'; +import { getSettingDefaults } from '../../../../app/settings/server/functions/getSettingDefaults'; const model = { insertOne: sinon.stub(), - updateOne: sinon.stub(), + findOneAndUpdate: sinon.stub(), } as unknown as ISettingsModel; const registry = new SettingsRegistry({ store: settings, model }); -const testSetting = { +const testSetting = getSettingDefaults({ _id: 'Dummy_setting', + type: 'string', packageValue: 'test', value: 'test', group: 'Testing', section: 'Registry', -}; + enterprise: false, +}); function addSetting(maybeSetting?: any) { const setting = maybeSetting || testSetting; return registry.add(setting._id, setting.packageValue, setting); } +(model.findOneAndUpdate as ReturnType).returns({ value: {} }); + +Object.keys(model).forEach((key) => spy.on(model, key)); + describe('SettingsRegistry operations', () => { describe('#add', () => { beforeEach(() => { @@ -35,30 +42,33 @@ describe('SettingsRegistry operations', () => { await addSetting(); expect(settings.get(testSetting._id)).to.be.equal(testSetting.packageValue); + expect(model.insertOne).to.be.called(); }); it('should NOT set the setting value from code when setting is loaded into the cache and no overwrite available', async () => { settings.set(testSetting as any); - const settingFromCodeFaked = { ...testSetting, value: 'new value' }; + const settingFromCodeFaked = { ...testSetting, value: Date.now().toString() }; await addSetting(settingFromCodeFaked); - expect(settings.get(settingFromCodeFaked._id)).to.be.equal(testSetting.value); + expect(settings.get(testSetting._id)).to.be.equal(testSetting.value); }); it('should set the setting value found in environment without OVERWRITE_SETTING_ prefix, if nothing is in cache already', async () => { - process.env[testSetting._id] = 'overriden value'; + process.env[testSetting._id] = Date.now().toString(); await addSetting(); - expect(settings.get(testSetting._id)).to.be.equal('overriden value'); + expect(model.insertOne).to.be.called(); + + expect(settings.get(testSetting._id)).to.be.equal(process.env[testSetting._id]); }); it('should NOT set the setting value found in environment without OVERWRITE_SETTING_ prefix, if value is in cache already', async () => { settings.set(testSetting as any); - process.env[testSetting._id] = 'overriden value'; + process.env[testSetting._id] = Date.now().toString(); await addSetting(); @@ -68,20 +78,38 @@ describe('SettingsRegistry operations', () => { it('should update cached value if OVERWRITE_SETTING_ prefix is found', async () => { settings.set(testSetting as any); - process.env[`OVERWRITE_SETTING_${testSetting._id}`] = 'new value'; + process.env[`OVERWRITE_SETTING_${testSetting._id}`] = Date.now().toString(); await addSetting(); - expect(settings.get(testSetting._id)).to.be.equal('new value'); + expect(model.findOneAndUpdate).to.be.called(); }); 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}`] = 'overwritten'; - process.env[testSetting._id] = 'overriden'; + process.env[`OVERWRITE_SETTING_${testSetting._id}`] = Date.now().toString(); + process.env[testSetting._id] = Date.now().toString(); await addSetting(); - expect(settings.get(testSetting._id)).to.be.equal('overwritten'); + expect(model.findOneAndUpdate).to.be.called(); }); }); + + 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; + }), + ); + }); }); From 6a345bb21df8832b155decca37a4a64851b46ed6 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Mon, 15 Jul 2024 17:25:31 +0530 Subject: [PATCH 10/25] trigger ci From d2e5f8a16a27677861b666388577c0c7b9a63c27 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 02:39:47 +0530 Subject: [PATCH 11/25] remove my tests --- .../server/startup/SettingsRegistry.spec.ts | 115 ------------------ 1 file changed, 115 deletions(-) delete mode 100644 apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts diff --git a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts b/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts deleted file mode 100644 index c4a0f5a1910f..000000000000 --- a/apps/meteor/tests/unit/server/startup/SettingsRegistry.spec.ts +++ /dev/null @@ -1,115 +0,0 @@ -import type { ISettingsModel } from '@rocket.chat/model-typings'; -import { expect, spy } from 'chai'; -import sinon from 'sinon'; - -import { SettingsRegistry, compareSettingsMetadata } from '../../../../app/settings/server/SettingsRegistry'; -import { settings } from '../../../../app/settings/server/cached'; -import { getSettingDefaults } from '../../../../app/settings/server/functions/getSettingDefaults'; - -const model = { - insertOne: sinon.stub(), - findOneAndUpdate: sinon.stub(), -} as unknown as ISettingsModel; - -const registry = new SettingsRegistry({ store: settings, model }); - -const testSetting = getSettingDefaults({ - _id: 'Dummy_setting', - type: 'string', - packageValue: 'test', - value: 'test', - group: 'Testing', - section: 'Registry', - enterprise: false, -}); - -function addSetting(maybeSetting?: any) { - const setting = maybeSetting || testSetting; - return registry.add(setting._id, setting.packageValue, setting); -} - -(model.findOneAndUpdate as ReturnType).returns({ value: {} }); - -Object.keys(model).forEach((key) => spy.on(model, key)); - -describe('SettingsRegistry operations', () => { - describe('#add', () => { - beforeEach(() => { - settings.store.clear(); - }); - - it('should set the setting value from code when nothing is loaded into the cache and no overwrite available', async () => { - await addSetting(); - - expect(settings.get(testSetting._id)).to.be.equal(testSetting.packageValue); - expect(model.insertOne).to.be.called(); - }); - - it('should NOT set the setting value from code when setting is loaded into the cache and no overwrite available', async () => { - settings.set(testSetting as any); - - const settingFromCodeFaked = { ...testSetting, value: Date.now().toString() }; - - await addSetting(settingFromCodeFaked); - - expect(settings.get(testSetting._id)).to.be.equal(testSetting.value); - }); - - it('should set the setting value found in environment without OVERWRITE_SETTING_ prefix, if nothing is in cache already', async () => { - process.env[testSetting._id] = Date.now().toString(); - - await addSetting(); - - expect(model.insertOne).to.be.called(); - - expect(settings.get(testSetting._id)).to.be.equal(process.env[testSetting._id]); - }); - - it('should NOT set the setting value found in environment without OVERWRITE_SETTING_ prefix, if value is in cache already', async () => { - settings.set(testSetting as any); - - process.env[testSetting._id] = Date.now().toString(); - - await addSetting(); - - expect(settings.get(testSetting._id)).to.be.equal(testSetting.value); - }); - - it('should update cached value if OVERWRITE_SETTING_ prefix is found', async () => { - settings.set(testSetting as any); - - process.env[`OVERWRITE_SETTING_${testSetting._id}`] = Date.now().toString(); - - await addSetting(); - - expect(model.findOneAndUpdate).to.be.called(); - }); - - 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 addSetting(); - - expect(model.findOneAndUpdate).to.be.called(); - }); - }); - - 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; - }), - ); - }); -}); From f4e41dcdf28ab31751ac3a0390d5fe7f232093e3 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 03:02:44 +0530 Subject: [PATCH 12/25] fix existing tests --- apps/meteor/app/settings/server/SettingsRegistry.ts | 4 ++-- apps/meteor/app/settings/server/functions/settings.mocks.ts | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index f169fc94913c..0ee8c0a79a69 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -168,7 +168,7 @@ export class SettingsRegistry { const { value: settingForCache } = await this.saveUpdatedSetting(_id, updatedProps, removedKeys); - if (!settingForCache) { + if (settingForCache === undefined || settingForCache === null) { // unreachable throw new Error('No document returned after setting was updated due to metadata change, something is wrong with code'); } @@ -187,7 +187,7 @@ export class SettingsRegistry { const { value: settingForCache } = await this.saveUpdatedSetting(_id, settingProps, removedKeys); - if (!settingForCache) { + if (settingForCache === undefined || settingForCache === null) { // unreachable throw new Error('No document returned due to an OVERWRITE, something is wrong with the code'); } diff --git a/apps/meteor/app/settings/server/functions/settings.mocks.ts b/apps/meteor/app/settings/server/functions/settings.mocks.ts index 9cd409ba0b83..6077ae28a297 100644 --- a/apps/meteor/app/settings/server/functions/settings.mocks.ts +++ b/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.settings.get(_id) }; + } + updateValueById(id: string, value: any): void { this.data.set(id, { ...this.data.get(id), value }); From 902a861422fa0c06818d80a696849c1f43ddc905 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 03:26:09 +0530 Subject: [PATCH 13/25] new tests --- .../compareSettingsMetadata.tests.ts | 28 ++++++ .../server/functions/settings.tests.ts | 94 +++++++++---------- 2 files changed, 74 insertions(+), 48 deletions(-) create mode 100644 apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts diff --git a/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts b/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts new file mode 100644 index 000000000000..faba3fe1b54f --- /dev/null +++ b/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; + }), + ); +}); 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 index 272fa950a407..ce058b926efd 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts +++ b/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,38 @@ describe('Settings', () => { .to.not.have.any.keys('section'); }); + it('should ignore setting object from code if only value changes and setting already stored', async () => { + settings.set(testSetting); + + 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 in cache', async () => { + settings.set(testSetting); + + process.env[testSetting._id] = Date.now().toString(); + + await settingsRegistry.add(testSetting._id, testSetting.value, testSetting); + + expect(settings.get(testSetting._id)).to.be.equal(testSetting.value); + }); + + 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 +476,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 }); From 0c6f8ca606319609a20702882f605454a24db26a Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 03:32:42 +0530 Subject: [PATCH 14/25] did this right before --- apps/meteor/app/settings/server/SettingsRegistry.ts | 4 ++-- apps/meteor/app/settings/server/functions/settings.mocks.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 0ee8c0a79a69..f169fc94913c 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -168,7 +168,7 @@ export class SettingsRegistry { const { value: settingForCache } = await this.saveUpdatedSetting(_id, updatedProps, removedKeys); - if (settingForCache === undefined || settingForCache === null) { + if (!settingForCache) { // unreachable throw new Error('No document returned after setting was updated due to metadata change, something is wrong with code'); } @@ -187,7 +187,7 @@ export class SettingsRegistry { const { value: settingForCache } = await this.saveUpdatedSetting(_id, settingProps, removedKeys); - if (settingForCache === undefined || settingForCache === null) { + if (!settingForCache) { // unreachable throw new Error('No document returned due to an OVERWRITE, something is wrong with the code'); } diff --git a/apps/meteor/app/settings/server/functions/settings.mocks.ts b/apps/meteor/app/settings/server/functions/settings.mocks.ts index 6077ae28a297..a142357850dd 100644 --- a/apps/meteor/app/settings/server/functions/settings.mocks.ts +++ b/apps/meteor/app/settings/server/functions/settings.mocks.ts @@ -77,7 +77,7 @@ class SettingsClass { findOneAndUpdate({ _id }: { _id: string }, value: any, options?: any) { this.updateOne({ _id }, value, options); - return { value: this.settings.get(_id) }; + return { value: this.findOne({ _id }) }; } updateValueById(id: string, value: any): void { From 1e0666459ae50a315948b0469b419484db0c5bdf Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 13:47:58 +0530 Subject: [PATCH 15/25] ... --- .../app/settings/server/SettingsRegistry.ts | 6 ++++++ .../server/functions/settings.tests.ts | 19 +++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index f169fc94913c..4d2dd6194081 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -166,6 +166,12 @@ export class SettingsRegistry { 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) { 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 index ce058b926efd..1dd4971c0816 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts +++ b/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts @@ -417,7 +417,10 @@ describe('Settings', () => { }); it('should ignore setting object from code if only value changes and setting already stored', async () => { - settings.set(testSetting); + 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() }; @@ -427,14 +430,22 @@ describe('Settings', () => { expect(Settings.upsertCalls).to.be.equal(0); }); - it('should ignore value from environment if setting is already stored in cache', async () => { - settings.set(testSetting); + 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.get(testSetting._id)).to.be.equal(testSetting.value); + 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 () => { From 5e7e61134bb806fa772ae5fcba49f2799498a5dc Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:21:31 +0530 Subject: [PATCH 16/25] Update apps/meteor/app/settings/server/SettingsRegistry.ts Co-authored-by: Kevin Aleman --- apps/meteor/app/settings/server/SettingsRegistry.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 4d2dd6194081..4e0a18156539 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -198,7 +198,6 @@ export class SettingsRegistry { 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; From 03f74e021373fe43488639c9f35fb60177397fb2 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:22:11 +0530 Subject: [PATCH 17/25] Update apps/meteor/app/settings/server/SettingsRegistry.ts --- apps/meteor/app/settings/server/SettingsRegistry.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 4e0a18156539..fe64aaeb29e6 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -74,7 +74,6 @@ const compareSettingsIgnoringKeys = .filter((key) => !keys.includes(key as keyof ISetting)) .every((key) => isEqual(a[key as keyof ISetting], b[key as keyof ISetting])); -// NOTE(debdut): not really "metadata" - but yeah. I don't have a better name export const compareSettingsMetadata = compareSettingsIgnoringKeys([ 'value', 'ts', From 644e689017d533bb1ef12717e982b9ca388a9b21 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:22:26 +0530 Subject: [PATCH 18/25] Update apps/meteor/app/settings/server/SettingsRegistry.ts --- apps/meteor/app/settings/server/SettingsRegistry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index fe64aaeb29e6..2f6a7d338753 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -74,7 +74,7 @@ const compareSettingsIgnoringKeys = .filter((key) => !keys.includes(key as keyof ISetting)) .every((key) => isEqual(a[key as keyof ISetting], b[key as keyof ISetting])); -export const compareSettingsMetadata = compareSettingsIgnoringKeys([ +export const compareSettings = compareSettingsIgnoringKeys([ 'value', 'ts', 'createdAt', From bd04040c4a120762d748d7398dbab23bafdbb8d8 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:22:51 +0530 Subject: [PATCH 19/25] Update apps/meteor/app/settings/server/SettingsRegistry.ts --- apps/meteor/app/settings/server/SettingsRegistry.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 2f6a7d338753..df5abdfcddf8 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -151,8 +151,7 @@ export class SettingsRegistry { 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; // settingStoredOverwrite not used since we need the updated props, that is in code not db From 39ab4c24cd71eb059c42326ad52cf786f64243bf Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:23:41 +0530 Subject: [PATCH 20/25] Update apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts --- .../server/functions/compareSettingsMetadata.tests.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts b/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts index faba3fe1b54f..e06249a8e7f8 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts +++ b/apps/meteor/tests/unit/app/settings/server/functions/compareSettingsMetadata.tests.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { compareSettingsMetadata } from '../../../../../../app/settings/server/SettingsRegistry'; +import { compareSettings } from '../../../../../../app/settings/server/SettingsRegistry'; import { getSettingDefaults } from '../../../../../../app/settings/server/functions/getSettingDefaults'; const testSetting = getSettingDefaults({ @@ -9,7 +9,7 @@ const testSetting = getSettingDefaults({ value: 'dummy', }); -describe('#compareSettingsMetadata', () => { +describe('#compareSettings', () => { const ignoredKeys = ['value', 'ts', 'createdAt', 'valueSource', 'packageValue', 'processEnvValue', '_updatedAt']; ignoredKeys.forEach((key) => @@ -22,7 +22,7 @@ describe('#compareSettingsMetadata', () => { copiedSetting[key] = 'random'; } - expect(compareSettingsMetadata(testSetting, copiedSetting)).to.be.true; + expect(compareSettings(testSetting, copiedSetting)).to.be.true; }), ); }); From d4654242ca67d3048023b46f654b59976ae90ee9 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:32:19 +0530 Subject: [PATCH 21/25] revert global copy --- .../server/functions/settings.tests.ts | 77 ++++++++++++++++--- 1 file changed, 68 insertions(+), 9 deletions(-) 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 index 1dd4971c0816..4cc754b690ca 100644 --- a/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts +++ b/apps/meteor/tests/unit/app/settings/server/functions/settings.tests.ts @@ -12,23 +12,18 @@ const testSetting = getSettingDefaults({ 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, { @@ -88,6 +83,11 @@ 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 () { @@ -143,6 +143,10 @@ 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, { @@ -196,6 +200,10 @@ 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', '', { @@ -248,6 +256,10 @@ 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'], { @@ -269,6 +281,11 @@ 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'], { @@ -293,6 +310,11 @@ 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'], { @@ -316,6 +338,10 @@ 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 () { @@ -362,6 +388,11 @@ 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, { @@ -417,6 +448,11 @@ describe('Settings', () => { }); 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); @@ -431,6 +467,11 @@ describe('Settings', () => { }); 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(); @@ -441,6 +482,11 @@ describe('Settings', () => { }); 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); @@ -449,6 +495,11 @@ describe('Settings', () => { }); 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(); @@ -459,6 +510,11 @@ describe('Settings', () => { 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(); @@ -487,6 +543,9 @@ 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 }); From c12b7a267e2d0f499c3e919cbd3bd73c19c2250f Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Wed, 17 Jul 2024 19:49:29 +0530 Subject: [PATCH 22/25] lint hi --- apps/meteor/app/settings/server/SettingsRegistry.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index df5abdfcddf8..d9b7feb8c2df 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -152,7 +152,6 @@ export class SettingsRegistry { const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); if (settingStored && !compareSettings(settingStored, 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); From 0eb1fa5046d4ca10e8664820242ffa69a957d885 Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Fri, 19 Jul 2024 21:50:21 +0530 Subject: [PATCH 23/25] remove comment --- apps/meteor/app/settings/server/SettingsRegistry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index a8ea289a1e1c..8aea7eb6a218 100644 --- a/apps/meteor/app/settings/server/SettingsRegistry.ts +++ b/apps/meteor/app/settings/server/SettingsRegistry.ts @@ -151,7 +151,7 @@ export class SettingsRegistry { const isOverwritten = settingFromCode !== settingFromCodeOverwritten || (settingStored && settingStored !== settingStoredOverwritten); if (settingStored && !compareSettings(settingStored, settingFromCodeOverwritten)) { - const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten; // settingStoredOverwrite not used since we need the updated props, that is in code not db + const { value: _value, ...settingOverwrittenProps } = settingFromCodeOverwritten; const overwrittenKeys = Object.keys(settingFromCodeOverwritten); const removedKeys = Object.keys(settingStored).filter((key) => !['_updatedAt'].includes(key) && !overwrittenKeys.includes(key)); From 88ae469bc95279df910dae7b79bf0efb2564180d Mon Sep 17 00:00:00 2001 From: Debdut Chakraborty Date: Fri, 19 Jul 2024 21:54:35 +0530 Subject: [PATCH 24/25] add the other changeset --- .changeset/mean-hairs-move.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mean-hairs-move.md diff --git a/.changeset/mean-hairs-move.md b/.changeset/mean-hairs-move.md new file mode 100644 index 000000000000..c92293d6ae95 --- /dev/null +++ b/.changeset/mean-hairs-move.md @@ -0,0 +1,5 @@ +--- +'@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. From 8931608864f814d52d3f2823ba55513d4ff96a71 Mon Sep 17 00:00:00 2001 From: Guilherme Gazzo Date: Fri, 19 Jul 2024 17:52:59 -0300 Subject: [PATCH 25/25] 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 --- .../app/settings/server/SettingsRegistry.ts | 50 ++++++------------- .../server/functions/settings.mocks.ts | 36 ++++++++++--- .../server/functions/settings.tests.ts | 16 ++++++ 3 files changed, 60 insertions(+), 42 deletions(-) diff --git a/apps/meteor/app/settings/server/SettingsRegistry.ts b/apps/meteor/app/settings/server/SettingsRegistry.ts index 8aea7eb6a218..e86a391ad8fa 100644 --- 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,8 +271,8 @@ export class SettingsRegistry { _id: string, settingProps: Omit, '_id'>, removedKeys?: string[], - ): Promise> { - return this.model.findOneAndUpdate( + ): Promise { + await this.model.updateOne( { _id }, { $set: settingProps, @@ -298,7 +280,7 @@ export class SettingsRegistry { $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 index a142357850dd..fb31c3021b1b 100644 --- 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 index 4cc754b690ca..3f409881b259 100644 --- 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); + }); });