From 2de7688eaf10fbd324825d2674cc4044a1125781 Mon Sep 17 00:00:00 2001 From: Naz Date: Thu, 4 Aug 2022 13:06:33 +0100 Subject: [PATCH] Used in memory cache adapter in settigs cache manager refs https://github.com/TryGhost/Toolbox/issues/364 - Settings Manager used to store all of it's settings values in a hash - an in memory cache in disguise. Having a hidden cache made it hard to reason about it's impact of memory usage and did not allow to swap it out for an alternative storage metchanism easily. Having a cache storage abstraction in Settings Manager allows to get rid of long lasting memory problems + decouples storage mechanism from the logic around transforming stored values. --- .../server/services/adapter-manager/index.js | 3 +- .../services/settings/settings-service.js | 4 +- ghost/core/core/shared/config/defaults.json | 3 ++ .../core/core/shared/settings-cache/cache.js | 39 ++++++++++++------- .../test/unit/shared/settings-cache.test.js | 12 ++++-- 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/ghost/core/core/server/services/adapter-manager/index.js b/ghost/core/core/server/services/adapter-manager/index.js index f1f77573c6..90d82c5789 100644 --- a/ghost/core/core/server/services/adapter-manager/index.js +++ b/ghost/core/core/server/services/adapter-manager/index.js @@ -15,11 +15,12 @@ const adapterManager = new AdapterManager({ adapterManager.registerAdapter('storage', require('ghost-storage-base')); adapterManager.registerAdapter('scheduling', require('../../adapters/scheduling/SchedulingBase')); adapterManager.registerAdapter('sso', require('../../adapters/sso/Base')); +adapterManager.registerAdapter('cache', require('../../adapters/cache/Base')); module.exports = { /** * - * @param {String} name - one of 'storage', 'scheduling', 'sso' etc. Or can contain a "resource" extension like "storage:image" + * @param {String} name - one of 'storage', 'scheduling', 'sso', 'cache' etc. Or can contain a "resource" extension like "storage:image" * @returns {Object} instance of an adapter */ getAdapter(name) { diff --git a/ghost/core/core/server/services/settings/settings-service.js b/ghost/core/core/server/services/settings/settings-service.js index dd092475fd..9e8995510f 100644 --- a/ghost/core/core/server/services/settings/settings-service.js +++ b/ghost/core/core/server/services/settings/settings-service.js @@ -9,6 +9,7 @@ const events = require('../../lib/common/events'); const models = require('../../models'); const labs = require('../../../shared/labs'); const config = require('../../../shared/config'); +const adapterManager = require('../adapter-manager'); const SettingsCache = require('../../../shared/settings-cache'); const SettingsBREADService = require('./settings-bread-service'); const {obfuscatedSetting, isSecretSetting, hideValueIfSecret} = require('./settings-utils'); @@ -66,8 +67,9 @@ module.exports = { * Initialize the cache, used in boot and in testing */ async init() { + const cacheStore = adapterManager.getAdapter('cache'); const settingsCollection = await models.Settings.populateDefaults(); - SettingsCache.init(events, settingsCollection, this.getCalculatedFields(), {}); + SettingsCache.init(events, settingsCollection, this.getCalculatedFields(), cacheStore); }, /** diff --git a/ghost/core/core/shared/config/defaults.json b/ghost/core/core/shared/config/defaults.json index 7726564c74..680de36107 100644 --- a/ghost/core/core/shared/config/defaults.json +++ b/ghost/core/core/shared/config/defaults.json @@ -23,6 +23,9 @@ "adapters": { "sso": { "active": "Default" + }, + "cache": { + "active": "Memory" } }, "storage": { diff --git a/ghost/core/core/shared/settings-cache/cache.js b/ghost/core/core/shared/settings-cache/cache.js index 0a9f5c5627..86f54d4366 100644 --- a/ghost/core/core/shared/settings-cache/cache.js +++ b/ghost/core/core/shared/settings-cache/cache.js @@ -50,30 +50,30 @@ class CacheManager { // NOTE: "!this.settingsCache" is for when setting's cache is used // before it had a chance to initialize. Should be fixed when // it is decoupled from the model layer - if (!this.settingsCache || !this.settingsCache[key]) { + if (!this.settingsCache || !this.settingsCache.get(key)) { return; } // Don't try to resolve to the value of the setting if (options && options.resolve === false) { - return this.settingsCache[key]; + return this.settingsCache.get(key); } // Default behavior is to try to resolve the value and return that try { // CASE: handle literal false - if (this.settingsCache[key].value === false || this.settingsCache[key].value === 'false') { + if (this.settingsCache.get(key).value === false || this.settingsCache.get(key).value === 'false') { return false; } // CASE: if a string contains a number e.g. "1", JSON.parse will auto convert into integer - if (!isNaN(Number(this.settingsCache[key].value))) { - return this.settingsCache[key].value || null; + if (!isNaN(Number(this.settingsCache.get(key).value))) { + return this.settingsCache.get(key).value || null; } - return JSON.parse(this.settingsCache[key].value) || null; + return JSON.parse(this.settingsCache.get(key).value) || null; } catch (err) { - return this.settingsCache[key].value || null; + return this.settingsCache.get(key).value || null; } } @@ -111,16 +111,27 @@ class CacheManager { * @param {object} value json version of settings model */ set(key, value) { - this.settingsCache[key] = _.cloneDeep(value); + this.settingsCache.set(key, _.cloneDeep(value)); } /** * Get the entire cache object * Uses clone to prevent modifications from being reflected + * This method is dangerous in case the cache is "lazily" initialized + * could result in returning only a partially filled cache * @return {object} cache + * @deprecated this method is not "cache-friendly" and should be avoided from furhter usage + * instead using multiple "get" calls */ getAll() { - return _.cloneDeep(this.settingsCache); + const keys = this.settingsCache.keys(); + const all = {}; + + keys.forEach((key) => { + all[key] = _.cloneDeep(this.settingsCache.get(key)); + }); + + return all; } /** @@ -139,15 +150,15 @@ class CacheManager { } /** - * Initialise the cache + * Initialize the cache * * Optionally takes a collection of settings & can populate the cache with these. * * @param {EventEmitter} events * @param {Bookshelf.Collection} settingsCollection * @param {Array} calculatedFields - * @param {Object} cacheStore - cache storage instance - * @return {object} + * @param {Object} cacheStore - cache storage instance base on Cache Base Adapter + * @return {Object} - filled out instance for Cache Base Adapter */ init(events, settingsCollection, calculatedFields, cacheStore) { this.settingsCache = cacheStore; @@ -182,7 +193,9 @@ class CacheManager { * @param {EventEmitter} events */ reset(events) { - this.settingsCache = {}; + if (this.settingsCache) { + this.settingsCache.reset(); + } events.removeListener('settings.edited', this._updateSettingFromModel); events.removeListener('settings.added', this._updateSettingFromModel); diff --git a/ghost/core/test/unit/shared/settings-cache.test.js b/ghost/core/test/unit/shared/settings-cache.test.js index e5210f7cf4..f80fc64700 100644 --- a/ghost/core/test/unit/shared/settings-cache.test.js +++ b/ghost/core/test/unit/shared/settings-cache.test.js @@ -6,6 +6,7 @@ const events = require('../../../core/server/lib/common/events'); // Testing the Private API let CacheManager = require('../../../core/shared/settings-cache/cache'); const publicSettings = require('../../../core/shared/settings-cache/public'); +const InMemoryCache = require('../../../core/server/adapters/cache/Memory'); should.equal(true, true); @@ -13,10 +14,11 @@ describe('UNIT: settings cache', function () { let cache; beforeEach(function () { + let cacheStore = new InMemoryCache(); cache = new CacheManager({ publicSettings }); - cache.init(events, {}, [], {}); + cache.init(events, {}, [], cacheStore); }); afterEach(function () { @@ -28,7 +30,7 @@ describe('UNIT: settings cache', function () { (typeof cache.get('key1')).should.eql('string'); }); - it('.get() oes not auto convert string into number: float', function () { + it('.get() does not auto convert string into number: float', function () { cache.set('key1', {value: '1.4'}); (typeof cache.get('key1')).should.eql('string'); }); @@ -128,7 +130,8 @@ describe('UNIT: settings cache', function () { }] }; - cache.init(events, settingsCollection, []); + let cacheStore = new InMemoryCache(); + cache.init(events, settingsCollection, [], cacheStore); cache.get('key1').should.equal('init value'); // check handler only called once on settings.edit @@ -141,7 +144,8 @@ describe('UNIT: settings cache', function () { cache.get('key1').should.equal('first edit'); // init does a reset by default - cache.init(events, settingsCollection, []); + let cacheStoreForReset = new InMemoryCache(); + cache.init(events, settingsCollection, [], cacheStoreForReset); setSpy.callCount.should.equal(3); cache.get('key1').should.equal('init value');