Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 30 commits into from
Jul 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c4841a2
fix: use correct setting for cache when not overwritten
debdutdeb Jul 8, 2024
2f33e7c
fix registry:add and add some unit tests for it
debdutdeb Jul 12, 2024
51619fc
move tests to non-ee folder
debdutdeb Jul 13, 2024
b3a00e1
fix import path
debdutdeb Jul 13, 2024
d587438
some comment
debdutdeb Jul 13, 2024
f6ff46a
...
debdutdeb Jul 13, 2024
53c2f30
fix test names
debdutdeb Jul 13, 2024
593aea2
reset file first
debdutdeb Jul 13, 2024
6c1bb5b
update
debdutdeb Jul 15, 2024
6a345bb
trigger ci
debdutdeb Jul 15, 2024
f7e06cd
Merge branch 'develop' into fix-incorrect-cache-set
casalsgh Jul 15, 2024
5e28127
Merge branch 'develop' into fix-incorrect-cache-set
casalsgh Jul 16, 2024
d2e5f8a
remove my tests
debdutdeb Jul 16, 2024
f4e41dc
fix existing tests
debdutdeb Jul 16, 2024
902a861
new tests
debdutdeb Jul 16, 2024
49eabbb
Merge branch 'develop' into fix-incorrect-cache-set
debdutdeb Jul 16, 2024
0c6f8ca
did this right before
debdutdeb Jul 16, 2024
028a579
Merge branch 'develop' into fix-incorrect-cache-set
debdutdeb Jul 17, 2024
1e06664
...
debdutdeb Jul 17, 2024
5e7e611
Update apps/meteor/app/settings/server/SettingsRegistry.ts
debdutdeb Jul 17, 2024
03f74e0
Update apps/meteor/app/settings/server/SettingsRegistry.ts
debdutdeb Jul 17, 2024
644e689
Update apps/meteor/app/settings/server/SettingsRegistry.ts
debdutdeb Jul 17, 2024
bd04040
Update apps/meteor/app/settings/server/SettingsRegistry.ts
debdutdeb Jul 17, 2024
39ab4c2
Update apps/meteor/tests/unit/app/settings/server/functions/compareSe…
debdutdeb Jul 17, 2024
d465424
revert global copy
debdutdeb Jul 17, 2024
c12b7a2
lint hi
debdutdeb Jul 17, 2024
8e09b4d
Merge branch 'develop' into fix-incorrect-cache-set
debdutdeb Jul 19, 2024
0eb1fa5
remove comment
debdutdeb Jul 19, 2024
88ae469
add the other changeset
debdutdeb Jul 19, 2024
8931608
fix: incorrect cache set review (#32848)
ggazzo Jul 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
});
});
Loading