Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-04-01 02:41:39 -05:00
Code Issues Activity

Moved route settings "getter" to the backend

Browse source 
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 :)
This commit is contained in:
Naz 2021-09-27 12:29:54 +02:00 • committed by naz
parent 484bb2eea2
commit ba964c549f
10 changed files with 79 additions and 111 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
core/boot.js
View file

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

13
core/frontend/services/settings/index.js
View file

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

4
core/frontend/services/settings/loader.js → core/server/services/route-settings/loader.js
View file

@ -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 = {

11
core/server/services/route-settings/route-settings.js
View file

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

4
core/server/web/site/routes.js
View file

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

26
test/regression/site/intergration-web-site/canary.test.js
View file

@ -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'],

24
test/regression/site/intergration-web-site/v2.test.js
View file

@ -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'],

24
test/regression/site/intergration-web-site/v3.test.js
View file

@ -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/': {

26
test/unit/services/settings/loader.test.js
View file

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

55
test/unit/services/settings/settings.test.js
View file

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