From ba964c549fda2498923946492c5293ea8a47412b Mon Sep 17 00:00:00 2001 From: Naz Date: Mon, 27 Sep 2021 12:29:54 +0200 Subject: [PATCH] Moved route settings "getter" to the backend refs https://linear.app/tryghost/issue/CORE-35/refactor-route-and-redirect-settings - Frontend is not meant to know about the underlying source of the "routes" configuration, so any reads/edits/validations are being moved into a backend service. This should also simplify the coupling of the backend with the frontend where the latter will get a JSON blob with all needed configuration during the boot - Nother problem the "get" method had was hiding an underlying function it was doing - reading the file from the filesystem SYNCRONOUSLY. It might be a thing we need to do during the "web" app initialization, but there's no clear need to do this in a sync fassion during the bootup for example. Also having a more explicit name should help :) --- core/boot.js | 3 +- core/frontend/services/settings/index.js | 13 +---- .../services/route-settings}/loader.js | 4 +- .../services/route-settings/route-settings.js | 11 ++++ core/server/web/site/routes.js | 4 +- .../site/intergration-web-site/canary.test.js | 26 ++++----- .../site/intergration-web-site/v2.test.js | 24 ++++---- .../site/intergration-web-site/v3.test.js | 24 ++++---- test/unit/services/settings/loader.test.js | 26 ++++++++- test/unit/services/settings/settings.test.js | 55 ------------------- 10 files changed, 79 insertions(+), 111 deletions(-) rename core/{frontend/services/settings => server/services/route-settings}/loader.js (94%) delete mode 100644 test/unit/services/settings/settings.test.js diff --git a/core/boot.js b/core/boot.js index 4e96d793e9..b6133ce96b 100644 --- a/core/boot.js +++ b/core/boot.js @@ -147,11 +147,12 @@ async function initDynamicRouting() { debug('Begin: Dynamic Routing'); const routing = require('./frontend/services/routing'); const frontendSettings = require('./frontend/services/settings'); + const routeSettingsService = require('./server/services/route-settings'); const bridge = require('./bridge'); // We pass the frontend API version + the dynamic routes here, so that the frontend services are slightly less tightly-coupled const apiVersion = bridge.getFrontendApiVersion(); - const routeSettings = frontendSettings.get(); + const routeSettings = routeSettingsService.loadRouteSettingsSync(); debug(`Frontend API Version: ${apiVersion}`); routing.bootstrap.start(apiVersion, routeSettings); diff --git a/core/frontend/services/settings/index.js b/core/frontend/services/settings/index.js index 890da53d66..3d28707dca 100644 --- a/core/frontend/services/settings/index.js +++ b/core/frontend/services/settings/index.js @@ -1,5 +1,5 @@ const crypto = require('crypto'); -const SettingsLoader = require('./loader'); +const SettingsLoader = require('../../../server/services/route-settings/loader'); /** * md5 hashes of default routes settings @@ -13,17 +13,6 @@ const calculateHash = (data) => { }; module.exports = { - /** - * Getter for routes YAML setting. - * Example: `settings.get().then(...)` - * will return a JSON Object like this: - * {routes: {}, collections: {}, resources: {}} - * @returns {Object} routes.yaml in JSON format - */ - get: function get() { - return SettingsLoader.loadSettingsSync('routes'); - }, - getDefaultHash: () => { return defaultRoutesSettingHash; }, diff --git a/core/frontend/services/settings/loader.js b/core/server/services/route-settings/loader.js similarity index 94% rename from core/frontend/services/settings/loader.js rename to core/server/services/route-settings/loader.js index c08c518d21..2237707954 100644 --- a/core/frontend/services/settings/loader.js +++ b/core/server/services/route-settings/loader.js @@ -3,8 +3,8 @@ const path = require('path'); const debug = require('@tryghost/debug')('frontend:services:settings:settings-loader'); const errors = require('@tryghost/errors'); const config = require('../../../shared/config'); -const yamlParser = require('./yaml-parser'); -const validate = require('./validate'); +const yamlParser = require('../../../frontend/services/settings/yaml-parser'); +const validate = require('../../../frontend/services/settings/validate'); const tpl = require('@tryghost/tpl'); const messages = { diff --git a/core/server/services/route-settings/route-settings.js b/core/server/services/route-settings/route-settings.js index efc8202a0c..9e7b1add31 100644 --- a/core/server/services/route-settings/route-settings.js +++ b/core/server/services/route-settings/route-settings.js @@ -10,6 +10,7 @@ const tpl = require('@tryghost/tpl'); const config = require('../../../shared/config'); const bridge = require('../../../bridge'); const ensureSettingsFile = require('./ensure-settings'); +const SettingsLoader = require('./loader'); const messages = { loadError: 'Could not load {filename} file.' @@ -144,6 +145,16 @@ const init = function () { }; module.exports.init = init; +/** + * Fetches routes YAML settings synchronously. + * will return a JSON Object like this: + * {routes: {}, collections: {}, resources: {}} + * @returns {Object} routes.yaml in JSON format + */ +module.exports.loadRouteSettingsSync = () => { + return SettingsLoader.loadSettingsSync('routes'); +}; + module.exports.api = { setFromFilePath: setFromFilePath, get: get diff --git a/core/server/web/site/routes.js b/core/server/web/site/routes.js index 36de5c1d80..3fb0bc9c2d 100644 --- a/core/server/web/site/routes.js +++ b/core/server/web/site/routes.js @@ -1,9 +1,9 @@ const debug = require('@tryghost/debug')('routing'); const routing = require('../../../frontend/services/routing'); -const frontendSettings = require('../../../frontend/services/settings'); +const routeSettings = require('../../services/route-settings'); module.exports = function siteRoutes(options = {}) { debug('site Routes', options); - options.routerSettings = frontendSettings.get(); + options.routerSettings = routeSettings.loadRouteSettingsSync(); return routing.bootstrap.init(options); }; diff --git a/test/regression/site/intergration-web-site/canary.test.js b/test/regression/site/intergration-web-site/canary.test.js index a37c182ed7..db31d28c5a 100644 --- a/test/regression/site/intergration-web-site/canary.test.js +++ b/test/regression/site/intergration-web-site/canary.test.js @@ -7,7 +7,7 @@ const mockUtils = require('../../../utils/mocks'); const configUtils = require('../../../utils/configUtils'); const urlUtils = require('../../../utils/urlUtils'); const appService = require('../../../../core/frontend/services/apps'); -const frontendSettingsService = require('../../../../core/frontend/services/settings'); +const routeSettingsService = require('../../../../core/server/services/route-settings'); const themeEngine = require('../../../../core/frontend/services/theme-engine'); const siteApp = require('../../../../core/server/web/parent/app'); @@ -367,9 +367,9 @@ describe('Integration - Web - Site canary', function () { }); describe('extended routes.yaml: collections', function () { - describe('2 collections', function () { + describe.only('2 collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/': {templates: ['home']} }, @@ -498,7 +498,7 @@ describe('Integration - Web - Site canary', function () { describe('no collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/something/': { templates: ['something'] @@ -549,7 +549,7 @@ describe('Integration - Web - Site canary', function () { describe('static permalink route', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -653,7 +653,7 @@ describe('Integration - Web - Site canary', function () { describe('primary author permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -736,7 +736,7 @@ describe('Integration - Web - Site canary', function () { describe('primary tag permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -834,7 +834,7 @@ describe('Integration - Web - Site canary', function () { describe('collection/routes with data key', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/my-page/': { data: { @@ -1003,7 +1003,7 @@ describe('Integration - Web - Site canary', function () { describe('extended routes.yaml: templates', function () { describe('default template, no template', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1070,7 +1070,7 @@ describe('Integration - Web - Site canary', function () { describe('two templates', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1122,7 +1122,7 @@ describe('Integration - Web - Site canary', function () { describe('home.hbs priority', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1198,7 +1198,7 @@ describe('Integration - Web - Site canary', function () { before(function () { testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme-channels'}); - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/channel1/': { controller: 'channel', @@ -1532,7 +1532,7 @@ describe('Integration - Web - Site canary', function () { describe('extended routes.yaml (5): rss override', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/podcast/rss/': { templates: ['podcast/rss'], diff --git a/test/regression/site/intergration-web-site/v2.test.js b/test/regression/site/intergration-web-site/v2.test.js index bde3847af1..a759962089 100644 --- a/test/regression/site/intergration-web-site/v2.test.js +++ b/test/regression/site/intergration-web-site/v2.test.js @@ -7,7 +7,7 @@ const mockUtils = require('../../../utils/mocks'); const configUtils = require('../../../utils/configUtils'); const urlUtils = require('../../../utils/urlUtils'); const appService = require('../../../../core/frontend/services/apps'); -const frontendSettingsService = require('../../../../core/frontend/services/settings'); +const routeSettingsService = require('../../../../core/server/services/route-settings'); const themeEngine = require('../../../../core/frontend/services/theme-engine'); const siteApp = require('../../../../core/server/web/parent/app'); @@ -368,7 +368,7 @@ describe('Integration - Web - Site v2', function () { describe('extended routes.yaml: collections', function () { describe('2 collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/': {templates: ['home']} }, @@ -497,7 +497,7 @@ describe('Integration - Web - Site v2', function () { describe('no collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/something/': {templates: ['something']} }, @@ -546,7 +546,7 @@ describe('Integration - Web - Site v2', function () { describe('static permalink route', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -650,7 +650,7 @@ describe('Integration - Web - Site v2', function () { describe('primary author permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -733,7 +733,7 @@ describe('Integration - Web - Site v2', function () { describe('primary tag permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -831,7 +831,7 @@ describe('Integration - Web - Site v2', function () { describe('collection/routes with data key', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/my-page/': { data: { @@ -1000,7 +1000,7 @@ describe('Integration - Web - Site v2', function () { describe('extended routes.yaml: templates', function () { describe('default template, no template', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1070,7 +1070,7 @@ describe('Integration - Web - Site v2', function () { describe('two templates', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1122,7 +1122,7 @@ describe('Integration - Web - Site v2', function () { describe('home.hbs priority', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1201,7 +1201,7 @@ describe('Integration - Web - Site v2', function () { before(function () { testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme-channels'}); - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/channel1/': { controller: 'channel', @@ -1535,7 +1535,7 @@ describe('Integration - Web - Site v2', function () { describe('extended routes.yaml (5): rss override', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/podcast/rss/': { templates: ['podcast/rss'], diff --git a/test/regression/site/intergration-web-site/v3.test.js b/test/regression/site/intergration-web-site/v3.test.js index b6fa61f452..7cd1c3a4ef 100644 --- a/test/regression/site/intergration-web-site/v3.test.js +++ b/test/regression/site/intergration-web-site/v3.test.js @@ -7,7 +7,7 @@ const mockUtils = require('../../../utils/mocks'); const configUtils = require('../../../utils/configUtils'); const urlUtils = require('../../../utils/urlUtils'); const appService = require('../../../../core/frontend/services/apps'); -const frontendSettingsService = require('../../../../core/frontend/services/settings'); +const routeSettingsService = require('../../../../core/server/services/route-settings'); const themeEngine = require('../../../../core/frontend/services/theme-engine'); const siteApp = require('../../../../core/server/web/parent/app'); @@ -368,7 +368,7 @@ describe('Integration - Web - Site v3', function () { describe('extended routes.yaml: collections', function () { describe('2 collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/': { templates: ['home'] @@ -499,7 +499,7 @@ describe('Integration - Web - Site v3', function () { describe('no collections', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/something/': {templates: ['something']} }, @@ -548,7 +548,7 @@ describe('Integration - Web - Site v3', function () { describe('static permalink route', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -652,7 +652,7 @@ describe('Integration - Web - Site v3', function () { describe('primary author permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -735,7 +735,7 @@ describe('Integration - Web - Site v3', function () { describe('primary tag permalink', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -833,7 +833,7 @@ describe('Integration - Web - Site v3', function () { describe('collection/routes with data key', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/my-page/': { data: { @@ -1002,7 +1002,7 @@ describe('Integration - Web - Site v3', function () { describe('extended routes.yaml: templates', function () { describe('default template, no template', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1072,7 +1072,7 @@ describe('Integration - Web - Site v3', function () { describe('two templates', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1124,7 +1124,7 @@ describe('Integration - Web - Site v3', function () { describe('home.hbs priority', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: {}, collections: { @@ -1203,7 +1203,7 @@ describe('Integration - Web - Site v3', function () { before(function () { testUtils.integrationTesting.defaultMocks(sinon, {theme: 'test-theme-channels'}); - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/channel1/': { controller: 'channel', @@ -1537,7 +1537,7 @@ describe('Integration - Web - Site v3', function () { describe('extended routes.yaml (5): rss override', function () { before(function () { - sinon.stub(frontendSettingsService, 'get').returns({ + sinon.stub(routeSettingsService, 'loadRouteSettingsSync').returns({ routes: { '/about/': 'about', '/podcast/rss/': { diff --git a/test/unit/services/settings/loader.test.js b/test/unit/services/settings/loader.test.js index 3c03a27bac..e18922b4cd 100644 --- a/test/unit/services/settings/loader.test.js +++ b/test/unit/services/settings/loader.test.js @@ -5,7 +5,7 @@ const fs = require('fs-extra'); const path = require('path'); const configUtils = require('../../../utils/configUtils'); const errors = require('@tryghost/errors'); -const loadSettings = rewire('../../../../core/frontend/services/settings/loader'); +const loadSettings = rewire('../../../../core/server/services/route-settings/loader'); describe('UNIT > Settings Service loader:', function () { beforeEach(function () { @@ -37,6 +37,28 @@ describe('UNIT > Settings Service loader:', function () { validateStub = sinon.stub(); }); + it('reads a settings object for routes.yaml file', function () { + const settingsStubFile = { + routes: null, + collections: { + '/': { + permalink: '/{slug}/', + template: ['home', 'index'] + } + }, + resources: { + tag: '/tag/{slug}/', + author: '/author/{slug}/' + } + }; + const fsReadFileStub = sinon.stub(fs, 'readFileSync').returns(settingsStubFile); + + const result = loadSettings.loadSettingsSync(); + should.exist(result); + result.should.be.an.Object().with.properties('routes', 'collections', 'taxonomies'); + fsReadFileStub.calledOnce.should.be.true(); + }); + it('can find yaml settings file and returns a settings object', function () { const fsReadFileSpy = sinon.spy(fs, 'readFileSync'); const expectedSettingsFile = path.join(__dirname, '../../../utils/fixtures/settings/goodroutes.yaml'); @@ -50,7 +72,7 @@ describe('UNIT > Settings Service loader:', function () { const setting = loadSettings.loadSettingsSync('goodroutes'); should.exist(setting); setting.should.be.an.Object().with.properties('routes', 'collections', 'taxonomies'); - // There are 4 files in the fixtures folder, but only 1 supported and valid yaml files + fsReadFileSpy.calledOnce.should.be.true(); fsReadFileSpy.calledWith(expectedSettingsFile).should.be.true(); yamlParserStub.callCount.should.be.eql(1); diff --git a/test/unit/services/settings/settings.test.js b/test/unit/services/settings/settings.test.js deleted file mode 100644 index aa168e6569..0000000000 --- a/test/unit/services/settings/settings.test.js +++ /dev/null @@ -1,55 +0,0 @@ -const sinon = require('sinon'); -const should = require('should'); -const rewire = require('rewire'); -const errors = require('@tryghost/errors'); -const settings = rewire('../../../../core/frontend/services/settings'); - -describe('UNIT > Settings Service:', function () { - afterEach(function () { - sinon.restore(); - }); - - describe('get', function () { - let settingsLoaderStub; - - const settingsStubFile = { - routes: null, - collections: { - '/': { - permalink: '/{slug}/', - template: ['home', 'index'] - } - }, - resources: {tag: '/tag/{slug}/', author: '/author/{slug}/'} - }; - - beforeEach(function () { - settingsLoaderStub = sinon.stub(); - }); - - it('returns settings object for `routes`', function () { - settingsLoaderStub.returns(settingsStubFile); - settings.__set__('SettingsLoader.loadSettingsSync', settingsLoaderStub); - - const result = settings.get(); - should.exist(result); - result.should.be.an.Object().with.properties('routes', 'collections', 'resources'); - settingsLoaderStub.calledOnce.should.be.true(); - }); - - it('passes SettingsLoader error through', function (done) { - settingsLoaderStub.throws(new errors.GhostError({message: 'oops'})); - settings.__set__('SettingsLoader.loadSettingsSync', settingsLoaderStub); - - try { - settings.get(); - done(new Error('SettingsLoader should fail')); - } catch (err) { - should.exist(err); - err.message.should.be.eql('oops'); - settingsLoaderStub.calledOnce.should.be.true(); - done(); - } - }); - }); -});