🔥 🎨 No more updateSettingsCache (#8090)

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

@@ -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}};
 

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;

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 checkAllSettings(allSettings) {
+                var usedKeys = allSettings.models.map(function mapper(setting) { return setting.get('key'); }),
+                    insertOperations = [];
 
-        return this.findAll(options).then(function then(allSettings) {
-            var usedKeys = allSettings.models.map(function mapper(setting) { return setting.get('key'); }),
-                insertOperations = [];
+                _.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));
+                    }
+                });
 
-            _.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));
+                if (insertOperations.length > 0) {
+                    return Promise.all(insertOperations).then(function fetchAllToReturn() {
+                        return self.findAll(options);
+                    });
                 }
-            });
 
-            return Promise.all(insertOperations);
-        });
+                return allSettings;
+            });
     }
-
 });
 
 module.exports = {

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<Settings>} [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);

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);
             });
     }
 };

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');
             });
     });