🎨 Move settings cache & cleanup settings API (#8057)

closes #8037 🔥 Remove API-level default settings population - This is a relic! - We ALWAYS populate defaults on server start therefore this code could never run. - This was a lot of complicated code that wasn't even needed!! 🎨 Move settings cache - Move settings cache to be its own thing - Update all references - Adds TODOs for further cleanup 🎨 Create settings initialisation step - Create new settings library, which will eventually house more code - Unify the interface for initialising settings (will be more useful later) - Reduce number of calls to updateSettingsCache
TryGhost · Feb 27, 2017 · 63723aa · 63723aa
1 parent 56eb896
commit 63723aa
Show file tree

Hide file tree

Showing 41 changed files with 186 additions and 221 deletions.
diff --git a/core/server/api/configuration.js b/core/server/api/configuration.js
@@ -2,7 +2,7 @@
 // RESTful API for browsing the configuration
 var _                  = require('lodash'),
     config             = require('../config'),
-    settingsCache      = require('../api/settings').cache,
+    settingsCache      = require('../settings/cache'),
     ghostVersion       = require('../utils/ghost-version'),
     models             = require('../models'),
     Promise            = require('bluebird'),

diff --git a/core/server/api/settings.js b/core/server/api/settings.js
@@ -12,47 +12,36 @@ var _            = require('lodash'),
     docName      = 'settings',
     settings,
 
+    settingsCache = require('../settings/cache'),
+
     updateSettingsCache,
     settingsFilter,
     filterPaths,
     readSettingsResult,
     settingsResult,
     canEditAllSettings,
-    populateDefaultSetting,
-    hasPopulatedDefaults = false,
-
-    /**
-     * ## Cache
-     * Holds cached settings
-     * @type {{}}
-     */
-    settingsCache = {};
 
-/**
- * ### Update Settings Cache
- * Maintain the internal cache of the settings object
- * @public
- * @param {Object} settings
- * @returns {Settings}
- */
-updateSettingsCache = function (settings, options) {
+// @TODO simplify this!
+updateSettingsCache = function updateSettingsCache(settings, options) {
     options = options || {};
     settings = settings || {};
 
     if (!_.isEmpty(settings)) {
         _.map(settings, function (setting, key) {
-            settingsCache[key] = setting;
+            settingsCache.set(key, setting);
         });
 
-        return Promise.resolve(settingsCache);
+        return Promise.resolve(settingsCache.getAll());
     }
 
     return dataProvider.Settings.findAll(options)
         .then(function (result) {
             // keep reference and update all keys
-            _.extend(settingsCache, readSettingsResult(result.models));
+            _.each(readSettingsResult(result.models), function (setting, key) {
+                settingsCache.set(key, setting);
+            });
 
-            return settingsCache;
+            return settingsCache.getAll();
         });
 };
 
@@ -175,33 +164,6 @@ settingsResult = function (settings, type) {
     return result;
 };
 
-/**
- * ### Populate Default Setting
- * @private
- * @param {String} key
- * @returns Promise(Setting)
- */
-populateDefaultSetting = function (key) {
-    // Call populateDefault and update the settings cache
-    return dataProvider.Settings.populateDefault(key).then(function (defaultSetting) {
-        // Process the default result and add to settings cache
-        var readResult = readSettingsResult([defaultSetting]);
-
-        // Add to the settings cache
-        return updateSettingsCache(readResult).then(function () {
-            // Get the result from the cache with permission checks
-        });
-    }).catch(function (err) {
-        // Pass along NotFoundError
-        if (typeof err === errors.NotFoundError) {
-            return Promise.reject(err);
-        }
-
-        // TODO: different kind of error?
-        return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.settings.problemFindingSetting', {key: key})}));
-    });
-};
-
 /**
  * ### Can Edit All Settings
  * Check that this edit request is allowed for all settings requested to be updated
@@ -210,7 +172,7 @@ populateDefaultSetting = function (key) {
  * @returns {*}
  */
 canEditAllSettings = function (settingsInfo, options) {
-    var checkSettingPermissions = function (setting) {
+    var checkSettingPermissions = function checkSettingPermissions(setting) {
             if (setting.type === 'core' && !(options.context && options.context.internal)) {
                 return Promise.reject(
                     new errors.NoPermissionError({message: i18n.t('errors.api.settings.accessCoreSettingFromExtReq')})
@@ -222,14 +184,12 @@ canEditAllSettings = function (settingsInfo, options) {
             });
         },
         checks = _.map(settingsInfo, function (settingInfo) {
-            var setting = settingsCache[settingInfo.key];
+            var setting = settingsCache.get(settingInfo.key, {resolve: false});
 
             if (!setting) {
-                // Try to populate a default setting if not in the cache
-                return populateDefaultSetting(settingInfo.key).then(function (defaultSetting) {
-                    // Get the result from the cache with permission checks
-                    return checkSettingPermissions(defaultSetting);
-                });
+                return Promise.reject(new errors.NotFoundError(
+                    {message: i18n.t('errors.api.settings.problemFindingSetting', {key: settingInfo.key})}
+                ));
             }
 
             return checkSettingPermissions(setting);
@@ -251,17 +211,9 @@ settings = {
      * @returns {*}
      */
     browse: function browse(options) {
-        // First, check if we have populated the settings from default-settings yet
-        if (!hasPopulatedDefaults) {
-            return dataProvider.Settings.populateDefaults().then(function () {
-                hasPopulatedDefaults = true;
-                return settings.browse(options);
-            });
-        }
-
         options = options || {};
 
-        var result = settingsResult(settingsCache, options.type);
+        var result = settingsResult(settingsCache.getAll(), options.type);
 
         // If there is no context, return only blog settings
         if (!options.context) {
@@ -289,40 +241,32 @@ settings = {
             options = {key: options};
         }
 
-        var getSettingsResult = function () {
-                var setting = settingsCache[options.key],
-                    result = {};
-
-                result[options.key] = setting;
+        var setting = settingsCache.get(options.key, {resolve: false}),
+            result = {};
 
-                if (setting.type === 'core' && !(options.context && options.context.internal)) {
-                    return Promise.reject(
-                        new errors.NoPermissionError({message: i18n.t('errors.api.settings.accessCoreSettingFromExtReq')})
-                    );
-                }
+        if (!setting) {
+            return Promise.reject(new errors.NotFoundError(
+                {message: i18n.t('errors.api.settings.problemFindingSetting', {key: options.key})}
+            ));
+        }
 
-                if (setting.type === 'blog') {
-                    return Promise.resolve(settingsResult(result));
-                }
+        result[options.key] = setting;
 
-                return canThis(options.context).read.setting(options.key).then(function () {
-                    return settingsResult(result);
-                }, function () {
-                    return Promise.reject(new errors.NoPermissionError({message: i18n.t('errors.api.settings.noPermissionToReadSettings')}));
-                });
-            };
+        if (setting.type === 'core' && !(options.context && options.context.internal)) {
+            return Promise.reject(
+                new errors.NoPermissionError({message: i18n.t('errors.api.settings.accessCoreSettingFromExtReq')})
+            );
+        }
 
-        // If the setting is not already in the cache
-        if (!settingsCache[options.key]) {
-            // Try to populate the setting from default-settings file
-            return populateDefaultSetting(options.key).then(function () {
-                // Get the result from the cache with permission checks
-                return getSettingsResult();
-            });
+        if (setting.type === 'blog') {
+            return Promise.resolve(settingsResult(result));
         }
 
-        // Get the result from the cache with permission checks
-        return getSettingsResult();
+        return canThis(options.context).read.setting(options.key).then(function () {
+            return settingsResult(result);
+        }, function () {
+            return Promise.reject(new errors.NoPermissionError({message: i18n.t('errors.api.settings.noPermissionToReadSettings')}));
+        });
     },
 
     /**
@@ -375,37 +319,4 @@ settings = {
 
 module.exports = settings;
 
-/**
- * @TODO:
- * - move settings cache somewhere else e.q. listen on model changes
- *
- * IMPORTANT:
- * We store settings with a type and a key in the database.
- *
- * {
- *   type: core
- *   key: dbHash
- *   value: ...
- * }
- *
- * But the settings cache does not allow requesting a value by type, only by key.
- * e.g. settings.cache.get('dbHash')
- */
-module.exports.cache = {
-    get: function get(key) {
-        if (!settingsCache[key]) {
-            return;
-        }
-
-        try {
-            return JSON.parse(settingsCache[key].value);
-        } catch (err) {
-            return settingsCache[key].value;
-        }
-    },
-    getAll: function getAll() {
-        return settingsCache;
-    }
-};
-
 module.exports.updateSettingsCache = updateSettingsCache;
diff --git a/core/server/api/themes.js b/core/server/api/themes.js
@@ -10,6 +10,7 @@ var Promise = require('bluebird'),
     logging = require('../logging'),
     storage = require('../storage'),
     settings = require('./settings'),
+    settingsCache = require('../settings/cache'),
     apiUtils = require('./utils'),
     utils = require('./../utils'),
     i18n = require('../i18n'),
@@ -30,7 +31,7 @@ themes = {
     },
 
     browse: function browse() {
-        return Promise.resolve({themes: settings.cache.get('availableThemes')});
+        return Promise.resolve({themes: settingsCache.get('availableThemes')});
     },
 
     upload: function upload(options) {

diff --git a/core/server/apps/amp/lib/router.js b/core/server/apps/amp/lib/router.js
@@ -6,7 +6,7 @@ var path                = require('path'),
 
     // Dirty requires
     errors              = require('../../../errors'),
-    settingsCache       = require('../../../api/settings').cache,
+    settingsCache       = require('../../../settings/cache'),
     templates           = require('../../../controllers/frontend/templates'),
     postLookup          = require('../../../controllers/frontend/post-lookup'),
     setResponseContext  = require('../../../controllers/frontend/context');

diff --git a/core/server/controllers/frontend/fetch-data.js b/core/server/controllers/frontend/fetch-data.js
@@ -5,7 +5,7 @@
 var api = require('../../api'),
     _   = require('lodash'),
     Promise = require('bluebird'),
-    settingsCache = api.settings.cache,
+    settingsCache = require('../../settings/cache'),
     queryDefaults,
     defaultPostQuery = {};
 

diff --git a/core/server/controllers/frontend/post-lookup.js b/core/server/controllers/frontend/post-lookup.js
@@ -3,7 +3,7 @@ var _          = require('lodash'),
     url        = require('url'),
     routeMatch = require('path-match')(),
     api        = require('../../api'),
-    settingsCache = api.settings.cache,
+    settingsCache = require('../../settings/cache'),
     optionsFormat = '/:options?';
 
 function getOptionsFormat(linkStructure) {

diff --git a/core/server/data/meta/asset_url.js b/core/server/data/meta/asset_url.js
@@ -1,5 +1,5 @@
 var config = require('../../config'),
-    settingsCache = require('../../api/settings').cache,
+    settingsCache = require('../../settings/cache'),
     utils = require('../../utils');
 
 function getAssetUrl(path, isAdmin, minify) {

diff --git a/core/server/data/meta/context_object.js b/core/server/data/meta/context_object.js
@@ -1,4 +1,4 @@
-var settingsCache = require('../../api/settings').cache,
+var settingsCache = require('../../settings/cache'),
     _ = require('lodash');
 
 function getContextObject(data, context) {

diff --git a/core/server/data/meta/description.js b/core/server/data/meta/description.js
@@ -1,5 +1,5 @@
 var _ = require('lodash'),
-    settingsCache = require('../../api/settings').cache;
+    settingsCache = require('../../settings/cache');
 
 function getDescription(data, root) {
     var description = '',

diff --git a/core/server/data/meta/index.js b/core/server/data/meta/index.js
@@ -1,5 +1,5 @@
 var Promise = require('bluebird'),
-    settingsCache = require('../../api/settings').cache,
+    settingsCache = require('../../settings/cache'),
     utils = require('../../utils'),
     getUrl = require('./url'),
     getImageDimensions = require('./image-dimensions'),

diff --git a/core/server/data/meta/title.js b/core/server/data/meta/title.js
@@ -1,5 +1,5 @@
 var _ = require('lodash'),
-    settingsCache = require('../../api/settings').cache;
+    settingsCache = require('../../settings/cache');
 
 function getTitle(data, root) {
     var title = '',

diff --git a/core/server/data/xml/rss/index.js b/core/server/data/xml/rss/index.js
@@ -7,7 +7,7 @@ var crypto      = require('crypto'),
     i18n        = require('../../../i18n'),
     filters     = require('../../../filters'),
     processUrls = require('../../../utils/make-absolute-urls'),
-    settingsCache = require('../../../api/settings').cache,
+    settingsCache = require('../../../settings/cache'),
 
     // Really ugly temporary hack for location of things
     fetchData   = require('../../../controllers/frontend/fetch-data'),

diff --git a/core/server/helpers/ghost_head.js b/core/server/helpers/ghost_head.js
@@ -18,7 +18,7 @@ var getMetaData = require('../data/meta'),
     labs = require('../utils/labs'),
     utils = require('../utils'),
     api = require('../api'),
-    settingsCache = api.settings.cache;
+    settingsCache = require('../settings/cache');
 
 function getClient() {
     if (labs.isSet('publicAPI') === true) {

diff --git a/core/server/index.js b/core/server/index.js
@@ -19,7 +19,6 @@ var debug = require('debug')('ghost:boot:init'),
     Promise = require('bluebird'),
     logging = require('./logging'),
     i18n = require('./i18n'),
-    api = require('./api'),
     models = require('./models'),
     permissions = require('./permissions'),
     apps = require('./apps'),
@@ -29,6 +28,8 @@ var debug = require('debug')('ghost:boot:init'),
     slack = require('./data/slack'),
     GhostServer = require('./ghost-server'),
     scheduling = require('./scheduling'),
+    settings = require('./settings'),
+    settingsCache = require('./settings/cache'),
     themes = require('./themes'),
     utils = require('./utils');
 
@@ -47,11 +48,8 @@ function init() {
     return dbHealth.check().then(function () {
         debug('DB health check done');
         // Populate any missing default settings
-        return models.Settings.populateDefaults();
-    }).then(function () {
-        debug('Models & database done');
         // Refresh the API settings cache
-        return api.settings.updateSettingsCache();
+        return settings.init();
     }).then(function () {
         debug('Update settings cache done');
         // Initialize the permissions actions and objects
@@ -81,8 +79,8 @@ function init() {
             ghostAuthUrl: config.get('auth:url'),
             redirectUri: utils.url.urlFor('admin', true),
             clientUri: utils.url.urlFor('home', true),
-            clientName: api.settings.cache.get('title'),
-            clientDescription: api.settings.cache.get('description')
+            clientName: settingsCache.get('title'),
+            clientDescription: settingsCache.get('description')
         }).then(function (response) {
             parentApp.use(response.auth);
         }).catch(function onAuthError(err) {

diff --git a/core/server/mail/GhostMailer.js b/core/server/mail/GhostMailer.js
@@ -5,7 +5,7 @@ var _          = require('lodash'),
     nodemailer = require('nodemailer'),
     validator  = require('validator'),
     config     = require('../config'),
-    settingsCache = require('../api/settings').cache,
+    settingsCache = require('../settings/cache'),
     i18n       = require('../i18n'),
     utils      = require('../utils');