From ed16998461623c9cfc5614b4b6191c51f20ff1ba Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Fri, 19 Feb 2016 18:18:14 +0000 Subject: [PATCH] Restructure Configuration API endpoint refs #6421, #6525 - The configuration API endpoint was a bit of an animal: - It's used currently in two ways, once for general config, another for the about page. - These two things are different, and would require different permissions in future. - There was also both a browse and a read version, even though only browse was used. - The response from the browse was being artificially turned into many objects, when its really just one with multiple keys - The new version treats each type of config as a different single object with several keys - The new version therefore only has a 'read' request - A basic read request with no key will return basic config that any client would need - A read request with the about key returns the about config - A read request with a different key could therefore return some other config --- core/client/app/index.html | 8 +- core/client/app/routes/about.js | 8 +- core/server/api/configuration.js | 65 +++++++--------- core/server/controllers/admin.js | 26 +++---- core/server/routes/api.js | 2 +- .../integration/api/api_configuration_spec.js | 75 +++++++++---------- core/test/utils/api.js | 1 - 7 files changed, 82 insertions(+), 103 deletions(-) diff --git a/core/client/app/index.html b/core/client/app/index.html index 1b3d24e432..fddadc8673 100644 --- a/core/client/app/index.html +++ b/core/client/app/index.html @@ -31,13 +31,13 @@ - {{#each configuration}} - + {{#each configuration as |config key|}} + {{/each}} - {{#unless skip_google_fonts}} + {{#if configuration.useGoogleFonts.value}} - {{/unless}} + {{/if}} diff --git a/core/client/app/routes/about.js b/core/client/app/routes/about.js index f20db7a353..36a7a9bc4f 100644 --- a/core/client/app/routes/about.js +++ b/core/client/app/routes/about.js @@ -18,7 +18,7 @@ export default AuthenticatedRoute.extend(styleBody, { model() { let cachedConfig = this.get('cachedConfig'); - let configUrl = this.get('ghostPaths.url').api('configuration'); + let configUrl = this.get('ghostPaths.url').api('configuration', 'about'); if (cachedConfig) { return cachedConfig; @@ -26,12 +26,8 @@ export default AuthenticatedRoute.extend(styleBody, { return this.get('ajax').request(configUrl) .then((configurationResponse) => { - let configKeyValues = configurationResponse.configuration; + let [cachedConfig] = configurationResponse.configuration; - cachedConfig = {}; - configKeyValues.forEach((configKeyValue) => { - cachedConfig[configKeyValue.key] = configKeyValue.value; - }); this.set('cachedConfig', cachedConfig); return cachedConfig; diff --git a/core/server/api/configuration.js b/core/server/api/configuration.js index f730f09b7f..5144bc4d48 100644 --- a/core/server/api/configuration.js +++ b/core/server/api/configuration.js @@ -2,9 +2,7 @@ // RESTful API for browsing the configuration var _ = require('lodash'), config = require('../config'), - errors = require('../errors'), Promise = require('bluebird'), - i18n = require('../i18n'), configuration; @@ -15,28 +13,24 @@ function labsFlag(key) { }; } -function getValidKeys() { - var validKeys = { - fileStorage: {value: (config.fileStorage !== false), type: 'bool'}, - publicAPI: labsFlag('publicAPI'), - apps: {value: (config.apps === true), type: 'bool'}, - version: {value: config.ghostVersion, type: 'string'}, - environment: process.env.NODE_ENV, - database: config.database.client, - mail: _.isObject(config.mail) ? config.mail.transport : '', - blogUrl: {value: config.url.replace(/\/$/, ''), type: 'string'}, - blogTitle: {value: config.theme.title, type: 'string'}, - routeKeywords: {value: JSON.stringify(config.routeKeywords), type: 'json'} - }; - - return validKeys; +function getAboutConfig() { + return { + version: config.ghostVersion, + environment: process.env.NODE_ENV, + database: config.database.client, + mail: _.isObject(config.mail) ? config.mail.transport : '' + }; } -function formatConfigurationObject(val, key) { +function getBaseConfig() { return { - key: key, - value: (_.isObject(val) && _.has(val, 'value')) ? val.value : val, - type: _.isObject(val) ? (val.type || null) : null + fileStorage: {value: (config.fileStorage !== false), type: 'bool'}, + useGoogleFonts: {value: !config.isPrivacyDisabled('useGoogleFonts'), type: 'bool'}, + useGravatar: {value: !config.isPrivacyDisabled('useGravatar'), type: 'bool'}, + publicAPI: labsFlag('publicAPI'), + blogUrl: {value: config.url.replace(/\/$/, ''), type: 'string'}, + blogTitle: {value: config.theme.title, type: 'string'}, + routeKeywords: {value: JSON.stringify(config.routeKeywords), type: 'json'} }; } @@ -48,26 +42,23 @@ function formatConfigurationObject(val, key) { configuration = { /** - * ### Browse - * Fetch all configuration keys - * @returns {Promise(Configurations)} - */ - browse: function browse() { - return Promise.resolve({configuration: _.map(getValidKeys(), formatConfigurationObject)}); - }, - - /** - * ### Read - * + * Always returns {configuration: []} + * Sometimes the array contains configuration items + * @param {Object} options + * @returns {Promise} */ read: function read(options) { - var data = getValidKeys(); + options = options || {}; - if (_.has(data, options.key)) { - return Promise.resolve({configuration: [formatConfigurationObject(data[options.key], options.key)]}); - } else { - return Promise.reject(new errors.NotFoundError(i18n.t('errors.api.configuration.invalidKey'))); + if (!options.key) { + return Promise.resolve({configuration: [getBaseConfig()]}); } + + if (options.key === 'about') { + return Promise.resolve({configuration: [getAboutConfig()]}); + } + + return Promise.resolve({configuration: []}); } }; diff --git a/core/server/controllers/admin.js b/core/server/controllers/admin.js index 31afe925aa..dd0a6981c8 100644 --- a/core/server/controllers/admin.js +++ b/core/server/controllers/admin.js @@ -1,8 +1,8 @@ var _ = require('lodash'), + Promise = require('bluebird'), api = require('../api'), errors = require('../errors'), updateCheck = require('../update-check'), - config = require('../config'), i18n = require('../i18n'), adminControllers; @@ -14,22 +14,20 @@ adminControllers = { /*jslint unparam:true*/ function renderIndex() { - var configuration; - return api.configuration.browse().then(function then(data) { - configuration = data.configuration; - }).then(function getAPIClient() { - return api.clients.read({slug: 'ghost-admin'}); - }).then(function renderIndex(adminClient) { - configuration.push({key: 'clientId', value: adminClient.clients[0].slug, type: 'string'}); - configuration.push({key: 'clientSecret', value: adminClient.clients[0].secret, type: 'string'}); + var configuration, + fetch = { + configuration: api.configuration.read().then(function (res) { return res.configuration[0]; }), + client: api.clients.read({slug: 'ghost-admin'}).then(function (res) { return res.clients[0]; }) + }; - var apiConfig = _.omit(configuration, function omit(value) { - return _.contains(['environment', 'database', 'mail', 'version'], value.key); - }); + return Promise.props(fetch).then(function renderIndex(result) { + configuration = result.configuration; + + configuration.clientId = {value: result.client.slug, type: 'string'}; + configuration.clientSecret = {value: result.client.secret, type: 'string'}; res.render('default', { - skip_google_fonts: config.isPrivacyDisabled('useGoogleFonts'), - configuration: apiConfig + configuration: configuration }); }); } diff --git a/core/server/routes/api.js b/core/server/routes/api.js index aa4f07973d..40eb029185 100644 --- a/core/server/routes/api.js +++ b/core/server/routes/api.js @@ -22,7 +22,7 @@ apiRoutes = function apiRoutes(middleware) { router.del = router.delete; // ## Configuration - router.get('/configuration', authenticatePrivate, api.http(api.configuration.browse)); + router.get('/configuration', authenticatePrivate, api.http(api.configuration.read)); router.get('/configuration/:key', authenticatePrivate, api.http(api.configuration.read)); // ## Posts diff --git a/core/test/integration/api/api_configuration_spec.js b/core/test/integration/api/api_configuration_spec.js index 4b5d0286ee..881b46f0f5 100644 --- a/core/test/integration/api/api_configuration_spec.js +++ b/core/test/integration/api/api_configuration_spec.js @@ -1,68 +1,63 @@ /*globals describe, before, afterEach, it */ var testUtils = require('../../utils'), should = require('should'), - rewire = require('rewire'), - _ = require('lodash'), - config = rewire('../../../server/config'), // Stuff we are testing ConfigurationAPI = rewire('../../../server/api/configuration'); describe('Configuration API', function () { - var newConfig = { - fileStorage: true, - apps: true, - version: '0.5.0', - environment: process.env.NODE_ENV, - database: { - client: 'mysql' - }, - mail: { - transport: 'SMTP' - }, - blogUrl: 'http://local.tryghost.org' - }; - // Keep the DB clean before(testUtils.teardown); afterEach(testUtils.teardown); should.exist(ConfigurationAPI); - it('can browse config', function (done) { - var updatedConfig = _.extend({}, config, newConfig); - config.set(updatedConfig); - ConfigurationAPI.__set__('config', updatedConfig); + it('can read basic config and get all expected properties', function (done) { + ConfigurationAPI.read().then(function (response) { + var props; - ConfigurationAPI.browse(testUtils.context.owner).then(function (response) { should.exist(response); should.exist(response.configuration); - testUtils.API.checkResponse(response.configuration[0], 'configuration'); - /*jshint unused:false */ - done(); - }).catch(function (error) { - console.log(JSON.stringify(error)); + response.configuration.should.be.an.Array().with.lengthOf(1); + props = response.configuration[0]; + + // Check the structure + props.should.have.property('blogUrl').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('blogTitle').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('routeKeywords').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('fileStorage').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('useGoogleFonts').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('useGravatar').which.is.an.Object().with.properties('type', 'value'); + props.should.have.property('publicAPI').which.is.an.Object().with.properties('type', 'value'); + + // Check a few values + props.blogUrl.should.have.property('value', 'http://127.0.0.1:2369'); + props.fileStorage.should.have.property('value', true); + done(); }).catch(done); }); - it('can read config', function (done) { - var updatedConfig = _.extend({}, config, newConfig); - config.set(updatedConfig); - ConfigurationAPI.__set__('config', updatedConfig); + it('can read about config and get all expected properties', function (done) { + ConfigurationAPI.read({key: 'about'}).then(function (response) { + var props; - ConfigurationAPI.read(_.extend({}, testUtils.context.owner, {key: 'database'})).then(function (response) { should.exist(response); should.exist(response.configuration); - testUtils.API.checkResponse(response.configuration[0], 'configuration'); - response.configuration[0].key.should.equal('database'); - response.configuration[0].value.should.equal('mysql'); - response.configuration[0].type.should.be.null(); - /*jshint unused:false */ - done(); - }).catch(function (error) { - console.log(JSON.stringify(error)); + response.configuration.should.be.an.Array().with.lengthOf(1); + props = response.configuration[0]; + + // Check the structure + props.should.have.property('version').which.is.a.String(); + props.should.have.property('environment').which.is.a.String(); + props.should.have.property('database').which.is.a.String(); + props.should.have.property('mail').which.is.a.String(); + + // Check a few values + props.environment.should.match(/^testing/); + props.version.should.eql(require('../../../../package.json').version); + done(); }).catch(done); }); diff --git a/core/test/utils/api.js b/core/test/utils/api.js index 9e16ba43e0..65263b7568 100644 --- a/core/test/utils/api.js +++ b/core/test/utils/api.js @@ -9,7 +9,6 @@ var _ = require('lodash'), protocol = 'http://', expectedProperties = { // API top level - configuration: ['key', 'value', 'type'], posts: ['posts', 'meta'], tags: ['tags', 'meta'], users: ['users', 'meta'],