From a5ab2ffc13ba71ac6da2d7e8dcf0a2565e678e53 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Thu, 2 Mar 2017 22:00:01 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A5=20=F0=9F=8E=A8=20No=20more=20updat?= =?UTF-8?q?eSettingsCache=20(#8090)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue 🔥 Remove unnecessary cache update 🎨 simplify updateSettingsCache() 🎨 Simplify readSettingsResult - although this is more code, it's now much clearer what happens in the two cases 🎨 Don't use readSettingResult for edit 🎨 Simplify updateSettingsCache further 🔥 Remove now unused readSettingsResult 🎨 Change populateDefault to return all 🎨 Move the findAll call out of updateSettingsCache 🔥 Remove updateSettingsCache!! 🎨 Restructure init & finish up settingsCache - move initialisation into settingsCache.init AT LAST - change settingCache to use cloneDeep, so that the object can't be modified outside of the functions - add lots of docs to settings cache 🎨 Cleanup db api endpoints 🔥 Don't populate settings in migrations --- core/server/api/db.js | 15 ++-- core/server/api/settings.js | 69 ++++--------------- .../data/migrations/init/3-insert-settings.js | 7 -- core/server/models/settings.js | 35 ++++++---- core/server/settings/cache.js | 59 +++++++++++++--- core/server/settings/index.js | 12 ++-- .../test/integration/api/api_settings_spec.js | 4 -- 7 files changed, 94 insertions(+), 107 deletions(-) delete mode 100644 core/server/data/migrations/init/3-insert-settings.js diff --git a/core/server/api/db.js b/core/server/api/db.js index 21153c3b9e..c1f06bdbda 100644 --- a/core/server/api/db.js +++ b/core/server/api/db.js @@ -8,12 +8,9 @@ var Promise = require('bluebird'), errors = require('../errors'), utils = require('./utils'), pipeline = require('../utils/pipeline'), - api = {}, - docName = 'db', + docName = 'db', db; -api.settings = require('./settings'); - /** * ## DB API Methods * @@ -28,8 +25,8 @@ db = { * @param {{context}} options * @returns {Promise} Ghost Export JSON format */ - exportContent: function (options) { - var tasks = []; + exportContent: function exportContent(options) { + var tasks; options = options || {}; @@ -57,8 +54,8 @@ db = { * @param {{context}} options * @returns {Promise} Success */ - importContent: function (options) { - var tasks = []; + importContent: function importContent(options) { + var tasks; options = options || {}; function importContent(options) { @@ -81,7 +78,7 @@ db = { * @param {{context}} options * @returns {Promise} Success */ - deleteAllContent: function (options) { + deleteAllContent: function deleteAllContent(options) { var tasks, queryOpts = {columns: 'id', context: {internal: true}}; diff --git a/core/server/api/settings.js b/core/server/api/settings.js index b0c2b60366..15cafc9beb 100644 --- a/core/server/api/settings.js +++ b/core/server/api/settings.js @@ -13,36 +13,10 @@ var _ = require('lodash'), settingsCache = require('../settings/cache'), - updateSettingsCache, settingsFilter, - readSettingsResult, settingsResult, canEditAllSettings, -// @TODO simplify this! -updateSettingsCache = function updateSettingsCache(settings, options) { - options = options || {}; - settings = settings || {}; - - if (!_.isEmpty(settings)) { - _.map(settings, function (setting, key) { - settingsCache.set(key, setting); - }); - - return Promise.resolve(settingsCache.getAll()); - } - - return dataProvider.Settings.findAll(options) - .then(function (result) { - // keep reference and update all keys - _.each(readSettingsResult(result.models), function (setting, key) { - settingsCache.set(key, setting); - }); - - return settingsCache.getAll(); - }); -}; - // ## Helpers /** @@ -65,9 +39,9 @@ settingsFilter = function (settings, filter) { }; /** - * ### Read Settings Result + * ### Settings Result * - * Converts the models to keyed JSON + * Takes a keyed JSON object * E.g. * dbHash: { * id: '123abc', @@ -77,30 +51,15 @@ settingsFilter = function (settings, filter) { * timestamps * } * + * Performs a filter, based on the `type` + * And converts the remaining items to our API format by adding a `setting` and `meta` keys. + * * @private - * @param {Array} settingsModels - * @returns {Settings} - */ -readSettingsResult = function (settingsModels) { - var settings = _.reduce(settingsModels, function (memo, member) { - if (!memo.hasOwnProperty(member.attributes.key)) { - memo[member.attributes.key] = member.attributes; - } - - return memo; - }, {}); - - return settings; -}; - -/** - * ### Settings Result - * @private - * @param {Object} settings + * @param {Object} settings - a keyed JSON object * @param {String} type * @returns {{settings: *}} */ -settingsResult = function (settings, type) { +settingsResult = function settingsResult(settings, type) { var filteredSettings = _.values(settingsFilter(settings, type)), result = { settings: filteredSettings, @@ -258,17 +217,15 @@ settings = { return utils.checkObject(object, docName).then(function (checkedData) { options.user = self.user; return dataProvider.Settings.edit(checkedData.settings, options); - }).then(function (result) { - var readResult = readSettingsResult(result); - - return updateSettingsCache(readResult).then(function () { - return settingsResult(readResult, type); - }); + }).then(function (settingsModelsArray) { + // Instead of a standard bookshelf collection, Settings.edit returns an array of Settings Models. + // We convert this to JSON, by calling toJSON on each Model (using invokeMap for ease) + // We use keyBy to create an object that uses the 'key' as a key for each setting. + var settingsKeyedJSON = _.keyBy(_.invokeMap(settingsModelsArray, 'toJSON'), 'key'); + return settingsResult(settingsKeyedJSON, type); }); }); } }; module.exports = settings; - -module.exports.updateSettingsCache = updateSettingsCache; diff --git a/core/server/data/migrations/init/3-insert-settings.js b/core/server/data/migrations/init/3-insert-settings.js deleted file mode 100644 index ad81ee3497..0000000000 --- a/core/server/data/migrations/init/3-insert-settings.js +++ /dev/null @@ -1,7 +0,0 @@ -var _ = require('lodash'), - models = require('../../../models'); - -module.exports = function insertSettings(options) { - var localOptions = _.merge({context: {internal: true}}, options); - return models.Settings.populateDefaults(localOptions); -}; diff --git a/core/server/models/settings.js b/core/server/models/settings.js index f260963312..c4eece6e1b 100644 --- a/core/server/models/settings.js +++ b/core/server/models/settings.js @@ -149,26 +149,33 @@ Settings = ghostBookshelf.Model.extend({ }, populateDefaults: function populateDefaults(options) { - options = options || {}; + var self = this; - options = _.merge({}, options, internalContext); + options = _.merge({}, options || {}, internalContext); - return this.findAll(options).then(function then(allSettings) { - var usedKeys = allSettings.models.map(function mapper(setting) { return setting.get('key'); }), - insertOperations = []; + return this + .findAll(options) + .then(function checkAllSettings(allSettings) { + var usedKeys = allSettings.models.map(function mapper(setting) { return setting.get('key'); }), + insertOperations = []; - _.each(getDefaultSettings(), function each(defaultSetting, defaultSettingKey) { - var isMissingFromDB = usedKeys.indexOf(defaultSettingKey) === -1; - if (isMissingFromDB) { - defaultSetting.value = defaultSetting.defaultValue; - insertOperations.push(Settings.forge(defaultSetting).save(null, options)); + _.each(getDefaultSettings(), function forEachDefault(defaultSetting, defaultSettingKey) { + var isMissingFromDB = usedKeys.indexOf(defaultSettingKey) === -1; + if (isMissingFromDB) { + defaultSetting.value = defaultSetting.defaultValue; + insertOperations.push(Settings.forge(defaultSetting).save(null, options)); + } + }); + + if (insertOperations.length > 0) { + return Promise.all(insertOperations).then(function fetchAllToReturn() { + return self.findAll(options); + }); } + + return allSettings; }); - - return Promise.all(insertOperations); - }); } - }); module.exports = { diff --git a/core/server/settings/cache.js b/core/server/settings/cache.js index 7ff7f15e87..fe30a0f29f 100644 --- a/core/server/settings/cache.js +++ b/core/server/settings/cache.js @@ -2,11 +2,14 @@ // As this cache is used in SO many other areas, we may open ourselves to // circular dependency bugs. var debug = require('debug')('ghost:settings:cache'), + _ = require('lodash'), events = require('../events'), /** * ## Cache * Holds cached settings - * @type {{}} + * Keyed by setting.key + * Contains the JSON version of the model + * @type {{}} - object of objects */ settingsCache = {}; @@ -25,6 +28,15 @@ var debug = require('debug')('ghost:settings:cache'), * e.g. settingsCache.get('dbHash') */ module.exports = { + /** + * Get a key from the settingsCache + * Will resolve to the value, including parsing JSON, unless {resolve: false} is passed in as an option + * In which case the full JSON version of the model will be resolved + * + * @param {string} key + * @param {object} options + * @return {*} + */ get: function get(key, options) { if (!settingsCache[key]) { return; @@ -42,21 +54,50 @@ module.exports = { return settingsCache[key].value; } }, + /** + * Set a key on the cache + * The only way to get an object into the cache + * Uses clone to prevent modifications from being reflected + * @param {string} key + * @param {object} value json version of settings model + */ set: function set(key, value) { - settingsCache[key] = value; + settingsCache[key] = _.cloneDeep(value); }, + /** + * Get the entire cache object + * Uses clone to prevent modifications from being reflected + * @return {{}} cache + */ getAll: function getAll() { - return settingsCache; + return _.cloneDeep(settingsCache); }, - init: function init() { - var self = this, - updateSettingFromModel = function updateSettingFromModel(settingModel) { - debug('Auto updating', settingModel.get('key')); - self.set(settingModel.get('key'), settingModel.toJSON()); - }; + /** + * Initialise the cache + * + * Optionally takes a collection of settings & can populate the cache with these. + * + * @param {Bookshelf.Collection} [settingsCollection] + * @return {{}} + */ + init: function init(settingsCollection) { + var self = this; + + // Local function, only ever used for initialising + // We deliberately call "set" on each model so that set is a consistent interface + function updateSettingFromModel(settingModel) { + debug('Auto updating', settingModel.get('key')); + self.set(settingModel.get('key'), settingModel.toJSON()); + } + // First, reset the cache settingsCache = {}; + // // if we have been passed a collection of settings, use this to populate the cache + if (settingsCollection && settingsCollection.models) { + _.each(settingsCollection.models, updateSettingFromModel); + } + // Bind to events to automatically keep up-to-date events.on('settings.edited', updateSettingFromModel); events.on('settings.added', updateSettingFromModel); diff --git a/core/server/settings/index.js b/core/server/settings/index.js index 306019eff4..76cce980c9 100644 --- a/core/server/settings/index.js +++ b/core/server/settings/index.js @@ -1,23 +1,19 @@ /** * Settings Lib * A collection of utilities for handling settings including a cache - * @TODO: eventually much of this logic will move into this lib - * For now we are providing a unified interface */ var SettingsModel = require('../models/settings').Settings, - SettingsAPI = require('../api').settings, SettingsCache = require('./cache'); module.exports = { init: function init() { - // Bind to events - SettingsCache.init(); // Update the defaults return SettingsModel.populateDefaults() - .then(function () { - // Reset the cache - return SettingsAPI.updateSettingsCache(); + .then(function (settingsCollection) { + // Initialise the cache with the result + // This will bind to events for further updates + SettingsCache.init(settingsCollection); }); } }; diff --git a/core/test/integration/api/api_settings_spec.js b/core/test/integration/api/api_settings_spec.js index f5b5ff7abc..c1d5a120b3 100644 --- a/core/test/integration/api/api_settings_spec.js +++ b/core/test/integration/api/api_settings_spec.js @@ -116,11 +116,8 @@ describe('Settings API', function () { }); it('can edit', function () { - var testReference = settingsCache.getAll(); - // see default-settings.json settingsCache.get('title').should.eql('Ghost'); - testReference.title.value.should.eql('Ghost'); return callApiWithContext(defaultContext, 'edit', {settings: [{key: 'title', value: 'UpdatedGhost'}]}, {}) .then(function (response) { @@ -130,7 +127,6 @@ describe('Settings API', function () { testUtils.API.checkResponse(response.settings[0], 'setting'); settingsCache.get('title').should.eql('UpdatedGhost'); - testReference.title.value.should.eql('UpdatedGhost'); }); });