Load yaml settings files synchronously

refs https://github.com/TryGhost/Team/issues/65

- it's easier for the architecture if we read the setting files synchronously,
  because the dynamic routing component is part of the express bootstrap and
  the whole routing bootstrap is synchronously
- for now: we only read one file anyway
- it's for now easier to read the file synchronously, then i don't have to change
  any existing express bootstrap architecture

core/server/services/settings/index.js

@@ -1,13 +1,14 @@
+'use strict';
+
 /**
  * Settings Lib
  * A collection of utilities for handling settings including a cache
  */
-
+const _ = require('lodash'),
-var SettingsModel = require('../../models/settings').Settings,
+    SettingsModel = require('../../models/settings').Settings,
     SettingsCache = require('./cache'),
     SettingsLoader = require('./loader'),
     // EnsureSettingsFiles = require('./ensure-settings'),
-    _ = require('lodash'),
     common = require('../../lib/common'),
     debug = require('ghost-ignition').debug('services:settings:index');
 
@@ -49,7 +50,7 @@ module.exports = {
      * will return an Object like this:
      * {routes: {}, collections: {}, resources: {}}
      * @param {String} setting type of supported setting.
-     * @returns {Promise<Object>} settingsFile
+     * @returns {Object} settingsFile
      * @description Returns settings object as defined per YAML files in
      * `/content/settings` directory.
      */
@@ -59,18 +60,13 @@ module.exports = {
         // CASE: this should be an edge case and only if internal usage of the
         // getter is incorrect.
         if (!setting || _.indexOf(knownSettings, setting) < 0) {
-            return Promise.reject(new common.errors.IncorrectUsageError({
+            throw new common.errors.IncorrectUsageError({
                 message: `Requested setting is not supported: '${setting}'.`,
                 help: `Please use only the supported settings: ${knownSettings}.`
-            }));
+            });
         }
 
-        return SettingsLoader(setting)
+        return SettingsLoader(setting);
-            .then((settingsFile) => {
-                debug('setting loaded and parsed:', settingsFile);
-
-                return settingsFile;
-            });
     },
 
     /**
@@ -88,23 +84,18 @@ module.exports = {
      *         config: { url: 'testblog.com' }
      *     }
      * }
-     * @returns {Promise<Object>} settingsObject
+     * @returns {Object} settingsObject
      * @description Returns all settings object as defined per YAML files in
      * `/content/settings` directory.
      */
     getAll: function getAll() {
         const knownSettings = this.knownSettings(),
-            props = {};
+            settingsToReturn = {};
 
         _.each(knownSettings, function (setting) {
-            props[setting] = SettingsLoader(setting);
+            settingsToReturn[setting] = SettingsLoader(setting);
         });
 
-        return Promise.props(props)
+        return settingsToReturn;
-            .then((settingsFile) => {
-                debug('all settings loaded and parsed:', settingsFile);
-
-                return settingsFile;
-            });
     }
 };

core/server/services/settings/loader.js

@@ -11,7 +11,7 @@ const fs = require('fs-extra'),
  * Reads the desired settings YAML file and passes the
  * file to the YAML parser which then returns a JSON object.
  * @param {String} setting the requested settings as defined in setting knownSettings
- * @returns {Promise<Object>} settingsFile
+ * @returns {Object} settingsFile
  */
 module.exports = function loadSettings(setting) {
     // we only support the `yaml` file extension. `yml` will be ignored.
@@ -19,23 +19,21 @@ module.exports = function loadSettings(setting) {
     const contentPath = config.getContentPath('settings');
     const filePath = path.join(contentPath, fileName);
 
-    return fs.readFile(filePath, 'utf8')
+    try {
-        .then((file) => {
+        const file = fs.readFileSync(filePath, 'utf8');
-            debug('settings file found for', setting);
+        debug('settings file found for', setting);
 
-            // yamlParser returns a JSON object
+        // yamlParser returns a JSON object
-            const parsed = yamlParser(file, fileName);
+        return yamlParser(file, fileName);
+    } catch (err) {
+        if (common.errors.utils.isIgnitionError(err)) {
+            throw err;
+        }
 
-            return parsed;
+        throw new common.errors.GhostError({
-        }).catch((error) => {
+            message: common.i18n.t('errors.services.settings.loader', {setting: setting, path: contentPath}),
-            if (common.errors.utils.isIgnitionError(error)) {
+            context: filePath,
-                throw error;
+            err: err
-            }
-
-            throw new common.errors.GhostError({
-                message: common.i18n.t('errors.services.settings.loader', {setting: setting, path: contentPath}),
-                context: filePath,
-                err: error
-            });
         });
+    }
 };

core/test/unit/services/settings/loader_spec.js

@@ -4,13 +4,10 @@ const sinon = require('sinon'),
     should = require('should'),
     rewire = require('rewire'),
     fs = require('fs-extra'),
-    yaml = require('js-yaml'),
     path = require('path'),
     configUtils = require('../../../utils/configUtils'),
     common = require('../../../../server/lib/common'),
-
     loadSettings = rewire('../../../../server/services/settings/loader'),
-
     sandbox = sinon.sandbox.create();
 
 describe('UNIT > Settings Service:', function () {
@@ -41,54 +38,67 @@ describe('UNIT > Settings Service:', function () {
         });
 
         it('can find yaml settings file and returns a settings object', function () {
-            const fsReadFileSpy = sandbox.spy(fs, 'readFile');
+            const fsReadFileSpy = sandbox.spy(fs, 'readFileSync');
             const expectedSettingsFile = path.join(__dirname, '../../../utils/fixtures/settings/goodroutes.yaml');
+
             yamlParserStub.returns(yamlStubFile);
             loadSettings.__set__('yamlParser', yamlParserStub);
 
-            return loadSettings('goodroutes').then((setting) => {
+            const setting = loadSettings('goodroutes');
-                should.exist(setting);
+            should.exist(setting);
-                setting.should.be.an.Object().with.properties('routes', 'collections', 'resources');
+            setting.should.be.an.Object().with.properties('routes', 'collections', 'resources');
-                // There are 4 files in the fixtures folder, but only 1 supported and valid yaml files
+            // There are 4 files in the fixtures folder, but only 1 supported and valid yaml files
-                fsReadFileSpy.calledOnce.should.be.true();
+            fsReadFileSpy.calledOnce.should.be.true();
-                fsReadFileSpy.calledWith(expectedSettingsFile).should.be.true();
+            fsReadFileSpy.calledWith(expectedSettingsFile).should.be.true();
-                yamlParserStub.callCount.should.be.eql(1);
+            yamlParserStub.callCount.should.be.eql(1);
-            });
         });
 
-        it('can handle errors from YAML parser', function () {
+        it('can handle errors from YAML parser', function (done) {
-            yamlParserStub.rejects(new common.errors.GhostError({
+            yamlParserStub.throws(new common.errors.GhostError({
                 message: 'could not parse yaml file',
                 context: 'bad indentation of a mapping entry at line 5, column 10'
             }));
+
             loadSettings.__set__('yamlParser', yamlParserStub);
 
-            return loadSettings('goodroutes').then((setting) => {
+            try {
-                should.not.exist(setting);
+                loadSettings('goodroutes');
-            }).catch((error) => {
+                done(new Error('Loader should fail'));
-                should.exist(error);
+            } catch (err) {
-                error.message.should.be.eql('could not parse yaml file');
+                should.exist(err);
-                error.context.should.be.eql('bad indentation of a mapping entry at line 5, column 10');
+                err.message.should.be.eql('could not parse yaml file');
+                err.context.should.be.eql('bad indentation of a mapping entry at line 5, column 10');
                 yamlParserStub.calledOnce.should.be.true();
-            });
+                done();
+            }
         });
 
-        it('throws error if file can\'t be accessed', function () {
+        it('throws error if file can\'t be accessed', function (done) {
             const expectedSettingsFile = path.join(__dirname, '../../../utils/fixtures/settings/routes.yaml');
             const fsError = new Error('no permission');
             fsError.code = 'EPERM';
-            const fsReadFileStub = sandbox.stub(fs, 'readFile').rejects(fsError);
+
+            const originalFn = fs.readFileSync;
+            const fsReadFileStub = sandbox.stub(fs, 'readFileSync').callsFake(function (filePath, options) {
+                if (filePath.match(/routes\.yaml/)) {
+                    throw fsError;
+                }
+
+                return originalFn(filePath, options);
+            });
+
             yamlParserStub = sinon.spy();
             loadSettings.__set__('yamlParser', yamlParserStub);
 
-            return loadSettings('routes').then((settings) => {
+            try {
-                should.not.exist(settings);
+                loadSettings('routes');
-            }).catch((error) => {
+                done(new Error('Loader should fail'));
-                should.exist(error);
+            } catch (err) {
-                error.message.should.match(/Error trying to load YAML setting for routes from/);
+                err.message.should.match(/Error trying to load YAML setting for routes from/);
                 fsReadFileStub.calledWith(expectedSettingsFile).should.be.true();
                 yamlParserStub.calledOnce.should.be.false();
-            });
+                done();
+            }
         });
     });
 });

core/test/unit/services/settings/settings_spec.js

@@ -4,9 +4,7 @@ const sinon = require('sinon'),
     should = require('should'),
     rewire = require('rewire'),
     common = require('../../../../server/lib/common'),
-
     settings = rewire('../../../../server/services/settings/index'),
-
     sandbox = sinon.sandbox.create();
 
 describe('UNIT > Settings Service:', function () {
@@ -42,40 +40,43 @@ describe('UNIT > Settings Service:', function () {
         });
 
         it('returns settings object for `routes`', function () {
-            settingsLoaderStub.resolves(settingsStubFile);
+            settingsLoaderStub.returns(settingsStubFile);
             settings.__set__('SettingsLoader', settingsLoaderStub);
 
-            settings.get('routes').then((result) => {
+            const result = settings.get('routes');
-                should.exist(result);
+            should.exist(result);
-                result.should.be.an.Object().with.properties('routes', 'collections', 'resources');
+            result.should.be.an.Object().with.properties('routes', 'collections', 'resources');
-                settingsLoaderStub.calledOnce.should.be.true();
+            settingsLoaderStub.calledOnce.should.be.true();
-            });
         });
 
-        it('rejects when requested settings type is not supported', function () {
+        it('rejects when requested settings type is not supported', function (done) {
-            settingsLoaderStub.resolves(settingsStubFile);
+            settingsLoaderStub.returns(settingsStubFile);
             settings.__set__('SettingsLoader', settingsLoaderStub);
 
-            return settings.get('something').then((result) => {
+            try {
-                should.not.exist(result);
+                settings.get('something');
-            }).catch((error) => {
+                done(new Error('SettingsLoader should fail'));
-                should.exist(error);
+            } catch (err) {
-                error.message.should.be.eql('Requested setting is not supported: \'something\'.');
+                should.exist(err);
+                err.message.should.be.eql('Requested setting is not supported: \'something\'.');
                 settingsLoaderStub.callCount.should.be.eql(0);
-            });
+                done();
+            }
         });
 
-        it('passes SettingsLoader error through', function () {
+        it('passes SettingsLoader error through', function (done) {
-            settingsLoaderStub.rejects(new common.errors.GhostError({message: 'oops'}));
+            settingsLoaderStub.throws(new common.errors.GhostError({message: 'oops'}));
             settings.__set__('SettingsLoader', settingsLoaderStub);
 
-            return settings.get('routes').then((result) => {
+            try {
-                should.not.exist(result);
+                settings.get('routes');
-            }).catch((error) => {
+                done(new Error('SettingsLoader should fail'));
-                should.exist(error);
+            } catch (err) {
-                error.message.should.be.eql('oops');
+                should.exist(err);
+                err.message.should.be.eql('oops');
                 settingsLoaderStub.calledOnce.should.be.true();
-            });
+                done();
+            }
         });
     });
 
@@ -106,32 +107,33 @@ describe('UNIT > Settings Service:', function () {
         });
 
         it('returns settings object for all known settings', function () {
-            settingsLoaderStub.onFirstCall().resolves(settingsStubFile1);
+            settingsLoaderStub.onFirstCall().returns(settingsStubFile1);
-            settingsLoaderStub.onSecondCall().resolves(settingsStubFile2);
+            settingsLoaderStub.onSecondCall().returns(settingsStubFile2);
             settings.__set__('SettingsLoader', settingsLoaderStub);
 
-            return settings.getAll().then((result) => {
+            const result = settings.getAll();
-                should.exist(result);
+            should.exist(result);
-                result.should.be.an.Object().with.properties('routes', 'globals');
+            result.should.be.an.Object().with.properties('routes', 'globals');
-                result.routes.should.be.an.Object().with.properties('routes', 'collections', 'resources');
+            result.routes.should.be.an.Object().with.properties('routes', 'collections', 'resources');
-                result.globals.should.be.an.Object().with.properties('config');
+            result.globals.should.be.an.Object().with.properties('config');
 
-                settingsLoaderStub.calledTwice.should.be.true();
+            settingsLoaderStub.calledTwice.should.be.true();
-            });
         });
 
-        it('passes SettinsLoader error through', function () {
+        it('passes SettinsLoader error through', function (done) {
-            settingsLoaderStub.onFirstCall().resolves(settingsStubFile1);
+            settingsLoaderStub.onFirstCall().returns(settingsStubFile1);
-            settingsLoaderStub.onSecondCall().rejects(new common.errors.GhostError({message: 'oops'}));
+            settingsLoaderStub.onSecondCall().throws(new common.errors.GhostError({message: 'oops'}));
             settings.__set__('SettingsLoader', settingsLoaderStub);
 
-            return settings.getAll().then((result) => {
+            try {
-                should.not.exist(result);
+                settings.getAll();
-            }).catch((error) => {
+                done(new Error('SettingsLoader should fail'));
-                should.exist(error);
+            } catch (err) {
-                error.message.should.be.eql('oops');
+                should.exist(err);
+                err.message.should.be.eql('oops');
                 settingsLoaderStub.calledTwice.should.be.true();
-            });
+                done();
+            }
         });
     });
 });