From 9309adc5119a8f91f3301ff95f0555d99bc9cfe8 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sat, 12 Mar 2016 18:54:06 +0000 Subject: [PATCH] Exporter cleanup & tests refs #6301 - change knex getter def to be configurable, else it is not testable - remove exportPath and lang from config - neither are used - add client_trusted_domains to tables which shouldn't be exported as there are no clients in the export - change export signature to be an object with `doExport` function consistent with import & easier to test - cleanup export code so it is clearer, easier to read & to test: - use mapSeries instead of sequence - use Promise.props instead of Promise.join - split functionality into smaller functions - add test coverage --- core/server/api/db.js | 4 +- core/server/api/index.js | 4 +- core/server/config/index.js | 2 - core/server/data/db/index.js | 2 +- core/server/data/export/index.js | 83 ++++++++++------ core/server/data/migration/index.js | 6 +- core/test/integration/export_spec.js | 4 +- core/test/unit/config_spec.js | 2 - core/test/unit/exporter_spec.js | 143 +++++++++++++++++++++++++++ 9 files changed, 205 insertions(+), 45 deletions(-) create mode 100644 core/test/unit/exporter_spec.js diff --git a/core/server/api/db.js b/core/server/api/db.js index f6e6e75dc1..9e5f8bbcb1 100644 --- a/core/server/api/db.js +++ b/core/server/api/db.js @@ -2,7 +2,7 @@ // API for DB operations var _ = require('lodash'), Promise = require('bluebird'), - dataExport = require('../data/export'), + exporter = require('../data/export'), importer = require('../data/importer'), backupDatabase = require('../data/migration').backupDatabase, models = require('../models'), @@ -38,7 +38,7 @@ db = { // Export data, otherwise send error 500 function exportContent() { - return dataExport().then(function (exportedData) { + return exporter.doExport().then(function (exportedData) { return {db: [exportedData]}; }).catch(function (error) { return Promise.reject(new errors.InternalServerError(error.message || error)); diff --git a/core/server/api/index.js b/core/server/api/index.js index d117d01c75..c7e3eb3082 100644 --- a/core/server/api/index.js +++ b/core/server/api/index.js @@ -21,7 +21,7 @@ var _ = require('lodash'), slugs = require('./slugs'), authentication = require('./authentication'), uploads = require('./upload'), - dataExport = require('../data/export'), + exporter = require('../data/export'), http, addHeaders, @@ -138,7 +138,7 @@ locationHeader = function locationHeader(req, result) { * @return {string} */ contentDispositionHeader = function contentDispositionHeader() { - return dataExport.fileName().then(function then(filename) { + return exporter.fileName().then(function then(filename) { return 'Attachment; filename="' + filename + '"'; }); }; diff --git a/core/server/config/index.js b/core/server/config/index.js index 6c3188546b..43cb258301 100644 --- a/core/server/config/index.js +++ b/core/server/config/index.js @@ -167,8 +167,6 @@ ConfigManager.prototype.set = function (config) { adminViews: path.join(corePath, '/server/views/'), helperTemplates: path.join(corePath, '/server/helpers/tpl/'), - exportPath: path.join(corePath, '/server/data/export/'), - lang: path.join(corePath, '/shared/lang/'), availableThemes: this._config.paths.availableThemes || {}, availableApps: this._config.paths.availableApps || {}, diff --git a/core/server/data/db/index.js b/core/server/data/db/index.js index 51e4fdf4ee..b1d2d03667 100644 --- a/core/server/data/db/index.js +++ b/core/server/data/db/index.js @@ -2,7 +2,7 @@ var connection; Object.defineProperty(exports, 'knex', { enumerable: true, - configurable: false, + configurable: true, get: function get() { connection = connection || require('./connection'); return connection; diff --git a/core/server/data/export/index.js b/core/server/data/export/index.js index 0d4b9deb2b..fac26187ed 100644 --- a/core/server/data/export/index.js +++ b/core/server/data/export/index.js @@ -8,15 +8,22 @@ var _ = require('lodash'), settings = require('../../api/settings'), i18n = require('../../i18n'), - excludedTables = ['accesstokens', 'refreshtokens', 'clients'], - exporter, + excludedTables = ['accesstokens', 'refreshtokens', 'clients', 'client_trusted_domains'], + modelOptions = {context: {internal: true}}, + + // private + getVersionAndTables, + exportTable, + + // public + doExport, exportFileName; -exportFileName = function () { +exportFileName = function exportFileName() { var datetime = (new Date()).toJSON().substring(0, 10), title = ''; - return settings.read({key: 'title', context: {internal: true}}).then(function (result) { + return settings.read(_.extend({}, {key: 'title'}, modelOptions)).then(function (result) { if (result) { title = serverUtils.safeString(result.settings[0].value) + '.'; } @@ -27,37 +34,51 @@ exportFileName = function () { }); }; -exporter = function () { - return Promise.join(versioning.getDatabaseVersion(), commands.getTables()).then(function (results) { - var version = results[0], - tables = results[1], - selectOps = _.map(tables, function (name) { - if (excludedTables.indexOf(name) < 0) { - return db.knex(name).select(); - } - }); +getVersionAndTables = function getVersionAndTables() { + var props = { + version: versioning.getDatabaseVersion(), + tables: commands.getTables() + }; - return Promise.all(selectOps).then(function (tableData) { - var exportData = { - meta: { - exported_on: new Date().getTime(), - version: version - }, - data: { - // Filled below - } - }; + return Promise.props(props); +}; - _.each(tables, function (name, i) { - exportData.data[name] = tableData[i]; - }); +exportTable = function exportTable(tableName) { + if (excludedTables.indexOf(tableName) < 0) { + return db.knex(tableName).select(); + } +}; - return exportData; - }).catch(function (err) { - errors.logAndThrowError(err, i18n.t('errors.data.export.errorExportingData'), ''); +doExport = function doExport() { + var tables, version; + + return getVersionAndTables().then(function exportAllTables(result) { + tables = result.tables; + version = result.version; + + return Promise.mapSeries(tables, exportTable); + }).then(function formatData(tableData) { + var exportData = { + meta: { + exported_on: new Date().getTime(), + version: version + }, + data: { + // Filled below + } + }; + + _.each(tables, function (name, i) { + exportData.data[name] = tableData[i]; }); + + return exportData; + }).catch(function (err) { + errors.logAndThrowError(err, i18n.t('errors.data.export.errorExportingData'), ''); }); }; -module.exports = exporter; -module.exports.fileName = exportFileName; +module.exports = { + doExport: doExport, + fileName: exportFileName +}; diff --git a/core/server/data/migration/index.js b/core/server/data/migration/index.js index 295535edd8..7a781ce66a 100644 --- a/core/server/data/migration/index.js +++ b/core/server/data/migration/index.js @@ -8,7 +8,7 @@ var _ = require('lodash'), schema = require('../schema').tables, commands = require('../schema').commands, versioning = require('../schema').versioning, - dataExport = require('../export'), + exporter = require('../export'), config = require('../../config'), errors = require('../../errors'), models = require('../../models'), @@ -46,9 +46,9 @@ populateDefaultSettings = function populateDefaultSettings() { backupDatabase = function backupDatabase() { logInfo('Creating database backup'); - return dataExport().then(function (exportedData) { + return exporter.doExport().then(function (exportedData) { // Save the exported data to the file system for download - return dataExport.fileName().then(function (fileName) { + return exporter.fileName().then(function (fileName) { fileName = path.resolve(config.paths.contentPath + '/data/' + fileName); return Promise.promisify(fs.writeFile)(fileName, JSON.stringify(exportedData)).then(function () { diff --git a/core/test/integration/export_spec.js b/core/test/integration/export_spec.js index 2a42c88b5f..6c3c143aa8 100644 --- a/core/test/integration/export_spec.js +++ b/core/test/integration/export_spec.js @@ -7,7 +7,7 @@ var testUtils = require('../utils/index'), // Stuff we are testing versioning = require('../../server/data/schema').versioning, - exporter = require('../../server/data/export/index'), + exporter = require('../../server/data/export'), DEF_DB_VERSION = versioning.getDefaultDatabaseVersion(), sandbox = sinon.sandbox.create(); @@ -28,7 +28,7 @@ describe('Exporter', function () { return Promise.resolve(DEF_DB_VERSION); }); - exporter().then(function (exportData) { + exporter.doExport().then(function (exportData) { var tables = ['posts', 'users', 'roles', 'roles_users', 'permissions', 'permissions_roles', 'permissions_users', 'settings', 'tags', 'posts_tags'], dbVersionSetting; diff --git a/core/test/unit/config_spec.js b/core/test/unit/config_spec.js index e89ac9204c..5d41bc79f0 100644 --- a/core/test/unit/config_spec.js +++ b/core/test/unit/config_spec.js @@ -72,8 +72,6 @@ describe('Config', function () { 'imagesRelPath', 'adminViews', 'helperTemplates', - 'exportPath', - 'lang', 'availableThemes', 'availableApps', 'clientAssets' diff --git a/core/test/unit/exporter_spec.js b/core/test/unit/exporter_spec.js new file mode 100644 index 0000000000..8d7fcfbed7 --- /dev/null +++ b/core/test/unit/exporter_spec.js @@ -0,0 +1,143 @@ +/*globals describe, afterEach, beforeEach, it*/ +var should = require('should'), + sinon = require('sinon'), + Promise = require('bluebird'), + + // Stuff we're testing + db = require('../../server/data/db'), + errors = require('../../server/errors'), + exporter = require('../../server/data/export'), + schema = require('../../server/data/schema'), + settings = require('../../server/api/settings'), + + schemaTables = Object.keys(schema.tables), + + sandbox = sinon.sandbox.create(); + +require('should-sinon'); + +describe('Exporter', function () { + var versionStub, tablesStub, queryMock, knexMock, knexStub; + + afterEach(function () { + sandbox.restore(); + knexStub.restore(); + }); + + describe('doExport', function () { + beforeEach(function () { + versionStub = sandbox.stub(schema.versioning, 'getDatabaseVersion').returns(new Promise.resolve('004')); + tablesStub = sandbox.stub(schema.commands, 'getTables').returns(schemaTables); + + queryMock = { + select: sandbox.stub() + }; + + knexMock = sandbox.stub().returns(queryMock); + + // this MUST use sinon, not sandbox, see sinonjs/sinon#781 + knexStub = sinon.stub(db, 'knex', {get: function () { return knexMock; }}); + }); + + it('should try to export all the correct tables', function (done) { + // Setup for success + queryMock.select.returns(new Promise.resolve({})); + + // Execute + exporter.doExport().then(function (exportData) { + // No tables, less the number of excluded tables + var expectedCallCount = schemaTables.length - 4; + + should.exist(exportData); + + versionStub.should.be.calledOnce(); + tablesStub.should.be.calledOnce(); + knexStub.get.should.be.called(); + knexMock.should.be.called(); + queryMock.select.should.be.called(); + + knexMock.should.have.callCount(expectedCallCount); + queryMock.select.should.have.callCount(expectedCallCount); + + knexMock.getCall(0).should.be.calledWith('posts'); + knexMock.getCall(1).should.be.calledWith('users'); + knexMock.getCall(2).should.be.calledWith('roles'); + knexMock.getCall(3).should.be.calledWith('roles_users'); + knexMock.getCall(4).should.be.calledWith('permissions'); + knexMock.getCall(5).should.be.calledWith('permissions_users'); + knexMock.getCall(6).should.be.calledWith('permissions_roles'); + knexMock.getCall(7).should.be.calledWith('permissions_apps'); + knexMock.getCall(8).should.be.calledWith('settings'); + knexMock.getCall(9).should.be.calledWith('tags'); + knexMock.getCall(10).should.be.calledWith('posts_tags'); + knexMock.getCall(11).should.be.calledWith('apps'); + knexMock.getCall(12).should.be.calledWith('app_settings'); + knexMock.getCall(13).should.be.calledWith('app_fields'); + + knexMock.should.not.be.calledWith('clients'); + knexMock.should.not.be.calledWith('client_trusted_domains'); + knexMock.should.not.be.calledWith('refreshtokens'); + knexMock.should.not.be.calledWith('accesstokens'); + + done(); + }).catch(done); + }); + + it('should catch and log any errors', function (done) { + // Setup for failure + var errorStub = sandbox.stub(errors, 'logAndThrowError'); + queryMock.select.returns(new Promise.reject({})); + + // Execute + exporter.doExport().then(function (exportData) { + should.not.exist(exportData); + errorStub.should.be.calledOnce(); + done(); + }).catch(done); + }); + }); + + describe('exportFileName', function () { + it('should return a correctly structured filename', function (done) { + var settingsStub = sandbox.stub(settings, 'read').returns( + new Promise.resolve({settings: [{value: 'testblog'}]}) + ); + + exporter.fileName().then(function (result) { + should.exist(result); + settingsStub.should.be.calledOnce(); + result.should.match(/^testblog\.ghost\.[0-9]{4}-[0-9]{2}-[0-9]{2}\.json$/); + + done(); + }).catch(done); + }); + + it('should return a correctly structured filename if settings is empty', function (done) { + var settingsStub = sandbox.stub(settings, 'read').returns( + new Promise.resolve() + ); + + exporter.fileName().then(function (result) { + should.exist(result); + settingsStub.should.be.calledOnce(); + result.should.match(/^ghost\.[0-9]{4}-[0-9]{2}-[0-9]{2}\.json$/); + + done(); + }).catch(done); + }); + + it('should return a correctly structured filename if settings errors', function (done) { + var settingsStub = sandbox.stub(settings, 'read').returns( + new Promise.reject() + ); + + exporter.fileName().then(function (result) { + should.exist(result); + settingsStub.should.be.calledOnce(); + result.should.match(/^ghost\.[0-9]{4}-[0-9]{2}-[0-9]{2}\.json$/); + + done(); + }).catch(done); + }); + }); +});