Skip to content

Commit

Permalink
fix: update cached setting immediately at the time of updating the db (
Browse files Browse the repository at this point in the history
  • Loading branch information
debdutdeb committed Jul 29, 2024
1 parent a534ad1 commit 61b3692
Show file tree
Hide file tree
Showing 5 changed files with 153 additions and 9 deletions.
5 changes: 5 additions & 0 deletions .changeset/mean-hairs-move.md
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 3 additions & 1 deletion apps/meteor/app/settings/server/SettingsRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -166,6 +166,7 @@ export class SettingsRegistry {
})();

await this.saveUpdatedSetting(_id, updatedProps, removedKeys);
this.store.set(settingFromCodeOverwritten);
return;
}

Expand All @@ -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;
}
Expand Down
41 changes: 33 additions & 8 deletions apps/meteor/app/settings/server/functions/settings.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ type Dictionary = {
class SettingsClass {
settings: ICachedSettings;

private delay = 0;

setDelay(delay: number): void {
this.delay = delay;
}

find(): any[] {
return [];
}
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
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;
}),
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
});

0 comments on commit 61b3692

Please sign in to comment.