From add4c6b0784056ca2d58b4db27fdb478e2b2d572 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 29 Dec 2014 18:33:47 +0000 Subject: [PATCH 1/2] Image Importer Improvements ref #4608, #4609, #4690 - fix errors with cleaning up files - improve handling of base directories, and introduce a simple valid format for zips (must contain importable files or folders, and may contain up to one base directory) - vastly improve test coverage --- core/server/data/import/index.js | 7 +- core/server/data/importer/handlers/image.js | 11 +- core/server/data/importer/handlers/json.js | 1 + core/server/data/importer/importers/data.js | 2 +- core/server/data/importer/index.js | 168 +++++++++++++--- core/test/integration/import_spec.js | 42 ++-- core/test/unit/importer_spec.js | 187 +++++++++++++++--- .../utils/fixtures/import/import-data-1.json | 9 + .../zips/zip-image-dir/images/image.jpg | 0 .../zips/zip-with-base-dir/basedir/test.json | 0 .../basedir/basedir/test.json | 0 .../zips/zip-without-base-dir/test.json | 0 12 files changed, 343 insertions(+), 84 deletions(-) create mode 100644 core/test/utils/fixtures/import/zips/zip-image-dir/images/image.jpg create mode 100644 core/test/utils/fixtures/import/zips/zip-with-base-dir/basedir/test.json create mode 100644 core/test/utils/fixtures/import/zips/zip-with-double-base-dir/basedir/basedir/test.json create mode 100644 core/test/utils/fixtures/import/zips/zip-without-base-dir/test.json diff --git a/core/server/data/import/index.js b/core/server/data/import/index.js index 7aba95f375dd..6ccad19ff2ba 100644 --- a/core/server/data/import/index.js +++ b/core/server/data/import/index.js @@ -9,7 +9,8 @@ var Promise = require('bluebird'), handleErrors, checkDuplicateAttributes, sanitize, - cleanError; + cleanError, + doImport; cleanError = function cleanError(error) { var temp, @@ -184,7 +185,7 @@ validate = function validate(data) { }); }; -module.exports = function (data) { +doImport = function (data) { var sanitizeResults = sanitize(data); data = sanitizeResults.data; @@ -197,3 +198,5 @@ module.exports = function (data) { return handleErrors(result); }); }; + +module.exports.doImport = doImport; diff --git a/core/server/data/importer/handlers/image.js b/core/server/data/importer/handlers/image.js index 35f3f1becceb..0f0720a6dcc4 100644 --- a/core/server/data/importer/handlers/image.js +++ b/core/server/data/importer/handlers/image.js @@ -10,24 +10,25 @@ ImageHandler = { type: 'images', extensions: config.uploads.extensions, types: config.uploads.contentTypes, + directories: ['images', 'content'], - loadFile: function (files, startDir) { + loadFile: function (files, baseDir) { var store = storage.getStorage(), - startDirRegex = startDir ? new RegExp('^' + startDir + '/') : new RegExp(''), + baseDirRegex = baseDir ? new RegExp('^' + baseDir + '/') : new RegExp(''), imageFolderRegexes = _.map(config.paths.imagesRelPath.split('/'), function (dir) { return new RegExp('^' + dir + '/'); }); // normalize the directory structure files = _.map(files, function (file) { - var noStartDir = file.name.replace(startDirRegex, ''), - noGhostDirs = noStartDir; + var noBaseDir = file.name.replace(baseDirRegex, ''), + noGhostDirs = noBaseDir; _.each(imageFolderRegexes, function (regex) { noGhostDirs = noGhostDirs.replace(regex, ''); }); - file.originalPath = noStartDir; + file.originalPath = noBaseDir; file.name = noGhostDirs; file.targetDir = path.join(config.paths.imagesPath, path.dirname(noGhostDirs)); return file; diff --git a/core/server/data/importer/handlers/json.js b/core/server/data/importer/handlers/json.js index 7af0fef84295..e0252246b3ac 100644 --- a/core/server/data/importer/handlers/json.js +++ b/core/server/data/importer/handlers/json.js @@ -8,6 +8,7 @@ JSONHandler = { type: 'data', extensions: ['.json'], types: ['application/octet-stream', 'application/json'], + directories: [], loadFile: function (files, startDir) { /*jshint unused:false */ diff --git a/core/server/data/importer/importers/data.js b/core/server/data/importer/importers/data.js index b5f9ef39471d..56847a50ce59 100644 --- a/core/server/data/importer/importers/data.js +++ b/core/server/data/importer/importers/data.js @@ -8,7 +8,7 @@ DataImporter = { return importData; }, doImport: function (importData) { - return importer(importData); + return importer.doImport(importData); } }; diff --git a/core/server/data/importer/index.js b/core/server/data/importer/index.js index 170afd2dd092..8075522e7612 100644 --- a/core/server/data/importer/index.js +++ b/core/server/data/importer/index.js @@ -14,16 +14,24 @@ var _ = require('lodash'), ImageImporter = require('./importers/image'), DataImporter = require('./importers/data'), + // Glob levels + ROOT_ONLY = 0, + ROOT_OR_SINGLE_DIR = 1, + ALL_DIRS = 2, + defaults; defaults = { extensions: ['.zip'], - types: ['application/zip', 'application/x-zip-compressed'] + types: ['application/zip', 'application/x-zip-compressed'], + directories: [] }; function ImportManager() { this.importers = [ImageImporter, DataImporter]; this.handlers = [ImageHandler, JSONHandler]; + // Keep track of files to cleanup at the end + this.filesToDelete = []; } /** @@ -36,40 +44,73 @@ function ImportManager() { _.extend(ImportManager.prototype, { /** * Get an array of all the file extensions for which we have handlers - * @returns [] + * @returns {string[]} */ getExtensions: function () { return _.flatten(_.union(_.pluck(this.handlers, 'extensions'), defaults.extensions)); }, /** * Get an array of all the mime types for which we have handlers - * @returns [] + * @returns {string[]} */ getTypes: function () { return _.flatten(_.union(_.pluck(this.handlers, 'types'), defaults.types)); }, /** - * Convert the extensions supported by a given handler into a glob string - * @returns String + * Get an array of directories for which we have handlers + * @returns {string[]} */ - getGlobPattern: function (handler) { - return '**/*+(' + _.reduce(handler.extensions, function (memo, ext) { + getDirectories: function () { + return _.flatten(_.union(_.pluck(this.handlers, 'directories'), defaults.directories)); + }, + /** + * Convert items into a glob string + * @param {String[]} items + * @returns {String} + */ + getGlobPattern: function (items) { + return '+(' + _.reduce(items, function (memo, ext) { return memo !== '' ? memo + '|' + ext : ext; }, '') + ')'; }, /** - * Remove a file after we're done (abstracted into a function for easier testing) - * @param {File} file + * @param {String[]} extensions + * @param {Number} level + * @returns {String} + */ + getExtensionGlob: function (extensions, level) { + var prefix = level === ALL_DIRS ? '**/*' : + (level === ROOT_OR_SINGLE_DIR ? '{*/*,*}' : '*'); + + return prefix + this.getGlobPattern(extensions); + }, + /** + * + * @param {String[]} directories + * @param {Number} level + * @returns {String} + */ + getDirectoryGlob: function (directories, level) { + var prefix = level === ALL_DIRS ? '**/' : + (level === ROOT_OR_SINGLE_DIR ? '{*/,}' : ''); + + return prefix + this.getGlobPattern(directories); + }, + /** + * Remove files after we're done (abstracted into a function for easier testing) * @returns {Function} */ - cleanUp: function (file) { - var fileToDelete = file; + cleanUp: function () { + var filesToDelete = this.filesToDelete; return function (result) { - try { - fs.remove(fileToDelete); - } catch (err) { - errors.logError(err, 'Import could not clean up file', 'You blog will continue to work as expected'); - } + _.each(filesToDelete, function (fileToDelete) { + fs.remove(fileToDelete, function (err) { + if (err) { + errors.logError(err, 'Import could not clean up file ', 'Your blog will continue to work as expected'); + } + }); + }); + return result; }; }, @@ -80,6 +121,39 @@ _.extend(ImportManager.prototype, { isZip: function (ext) { return _.contains(defaults.extensions, ext); }, + /** + * Checks the content of a zip folder to see if it is valid. + * Importable content includes any files or directories which the handlers can process + * Importable content must be found either in the root, or inside one base directory + * + * @param {String} directory + * @returns {Promise} + */ + isValidZip: function (directory) { + // Globs match content in the root or inside a single directory + var extMatchesBase = glob.sync( + this.getExtensionGlob(this.getExtensions(), ROOT_OR_SINGLE_DIR), {cwd: directory} + ), + extMatchesAll = glob.sync( + this.getExtensionGlob(this.getExtensions(), ALL_DIRS), {cwd: directory} + ), + dirMatches = glob.sync( + this.getDirectoryGlob(this.getDirectories(), ROOT_OR_SINGLE_DIR), {cwd: directory} + ); + + // If this folder contains importable files or a content or images directory + if (extMatchesBase.length > 0 || (dirMatches.length > 0 && extMatchesAll.length > 0)) { + return Promise.resolve(true); + } + + if (extMatchesAll.length < 1) { + return Promise.reject(new errors.UnsupportedMediaTypeError( + 'Zip did not include any content to import.' + )); + } + + return Promise.reject(new errors.UnsupportedMediaTypeError('Invalid zip file structure.')); + }, /** * Use the extract module to extract the given zip file to a temp directory & return the temp directory path * @param {String} filePath @@ -87,6 +161,7 @@ _.extend(ImportManager.prototype, { */ extractZip: function (filePath) { var tmpDir = path.join(os.tmpdir(), uuid.v4()); + this.filesToDelete.push(tmpDir); return Promise.promisify(extract)(filePath, {dir: tmpDir}).then(function () { return tmpDir; }); @@ -99,11 +174,37 @@ _.extend(ImportManager.prototype, { * @returns [] Files */ getFilesFromZip: function (handler, directory) { - var globPattern = this.getGlobPattern(handler); + var globPattern = this.getExtensionGlob(handler.extensions, ALL_DIRS); return _.map(glob.sync(globPattern, {cwd: directory}), function (file) { return {name: file, path: path.join(directory, file)}; }); }, + /** + * Get the name of the single base directory if there is one, else return an empty string + * @param {String} directory + * @returns {Promise (String)} + */ + getBaseDirectory: function (directory) { + // Globs match root level only + var extMatches = glob.sync(this.getExtensionGlob(this.getExtensions(), ROOT_ONLY), {cwd: directory}), + dirMatches = glob.sync(this.getDirectoryGlob(this.getDirectories(), ROOT_ONLY), {cwd: directory}), + extMatchesAll; + + // There is no base directory + if (extMatches.length > 0 || dirMatches.length > 0) { + return Promise.resolve(); + } + + // There is a base directory, grab it from any ext match + extMatchesAll = glob.sync( + this.getExtensionGlob(this.getExtensions(), ALL_DIRS), {cwd: directory} + ); + if (extMatchesAll.length < 1 || extMatchesAll[0].split('/') < 1) { + return Promise.resolve(new errors.ValidationError('Invalid zip file: base directory read failed')); + } + + return Promise.resolve(extMatchesAll[0].split('/')[0]); + }, /** * Process Zip * Takes a reference to a zip file, extracts it, sends any relevant files from inside to the right handler, and @@ -114,19 +215,22 @@ _.extend(ImportManager.prototype, { * @returns {Promise(ImportData)} */ processZip: function (file) { - var self = this; - return this.extractZip(file.path).then(function (directory) { + var self = this, + directory; + return this.extractZip(file.path).then(function (zipDirectory) { + directory = zipDirectory; + return self.isValidZip(directory); + }).then(function () { + return self.getBaseDirectory(directory); + }).then(function (baseDir) { var ops = [], - importData = {}, - startDir = glob.sync(file.name.replace('.zip', ''), {cwd: directory}); - - startDir = startDir[0] || false; + importData = {}; _.each(self.handlers, function (handler) { if (importData.hasOwnProperty(handler.type)) { // This limitation is here to reduce the complexity of the importer for now return Promise.reject(new errors.UnsupportedMediaTypeError( - 'Zip file contains too many types of import data. Please split it up and import separately.' + 'Zip file contains multiple data formats. Please split up and import separately.' )); } @@ -134,7 +238,7 @@ _.extend(ImportManager.prototype, { if (files.length > 0) { ops.push(function () { - return handler.loadFile(files, startDir).then(function (data) { + return handler.loadFile(files, baseDir).then(function (data) { importData[handler.type] = data; }); }); @@ -149,7 +253,7 @@ _.extend(ImportManager.prototype, { return sequence(ops).then(function () { return importData; - }).finally(self.cleanUp(directory)); + }); }); }, /** @@ -184,6 +288,8 @@ _.extend(ImportManager.prototype, { var self = this, ext = path.extname(file.name).toLowerCase(); + this.filesToDelete.push(file.path); + return Promise.resolve(this.isZip(ext)).then(function (isZip) { if (isZip) { // If it's a zip, process the zip file @@ -192,7 +298,7 @@ _.extend(ImportManager.prototype, { // Else process the file return self.processFile(file, ext); } - }).finally(self.cleanUp(file.path)); + }); }, /** * Import Step 2: @@ -245,7 +351,7 @@ _.extend(ImportManager.prototype, { * Import From File * The main method of the ImportManager, call this to kick everything off! * @param {File} file - * @returns {*} + * @returns {Promise} */ importFromFile: function (file) { var self = this; @@ -259,8 +365,10 @@ _.extend(ImportManager.prototype, { // @TODO: It would be cool to have some sort of dry run flag here return self.doImport(importData); }).then(function (importData) { - // Step 4: Finally, report on the import - return self.generateReport(importData); + // Step 4: Report on the import + return self.generateReport(importData) + // Step 5: Cleanup any files + .finally(self.cleanUp()); }); } }); diff --git a/core/test/integration/import_spec.js b/core/test/integration/import_spec.js index 6514f3ac3b19..86d7c3620aba 100644 --- a/core/test/integration/import_spec.js +++ b/core/test/integration/import_spec.js @@ -47,7 +47,7 @@ describe('Import', function () { }), fakeData = {test: true}; - importer(fakeData).then(function () { + importer.doImport(fakeData).then(function () { importStub.calledWith(fakeData).should.equal(true); importStub.restore(); @@ -68,7 +68,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function (importResult) { should.exist(importResult); should.exist(importResult.data); @@ -83,7 +83,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003-duplicate-posts').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function (importResult) { should.exist(importResult.data.data.posts); @@ -100,7 +100,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003-duplicate-tags').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function (importResult) { should.exist(importResult.data.data.tags); should.exist(importResult.data.data.posts_tags); @@ -135,7 +135,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-000').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { // Grab the data from tables return Promise.all([ @@ -182,7 +182,7 @@ describe('Import', function () { exportData.data.posts[0].updated_at = timestamp; exportData.data.posts[0].published_at = timestamp; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { // Grab the data from tables return Promise.all([ @@ -252,7 +252,7 @@ describe('Import', function () { // change title to 151 characters exportData.data.posts[0].title = new Array(152).join('a'); exportData.data.posts[0].tags = 'Tag'; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { (1).should.eql(0, 'Data import should not resolve promise.'); }, function (error) { @@ -297,7 +297,7 @@ describe('Import', function () { exportData = exported; // change to blank settings key exportData.data.settings[3].key = null; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { (1).should.eql(0, 'Data import should not resolve promise.'); }, function (error) { @@ -354,7 +354,7 @@ describe('Import', function () { exportData.data.posts[0].updated_at = timestamp; exportData.data.posts[0].published_at = timestamp; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { // Grab the data from tables return Promise.all([ @@ -424,7 +424,7 @@ describe('Import', function () { // change title to 151 characters exportData.data.posts[0].title = new Array(152).join('a'); exportData.data.posts[0].tags = 'Tag'; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { (1).should.eql(0, 'Data import should not resolve promise.'); }, function (error) { @@ -468,7 +468,7 @@ describe('Import', function () { exportData = exported; // change to blank settings key exportData.data.settings[3].key = null; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { (1).should.eql(0, 'Data import should not resolve promise.'); }, function (error) { @@ -517,7 +517,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { // Grab the data from tables return Promise.all([ @@ -562,7 +562,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003-badValidation').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(new Error('Allowed import of duplicate data')); }).catch(function (response) { @@ -585,7 +585,7 @@ describe('Import', function () { var exportData; testUtils.fixtures.loadExportFixture('export-003-dbErrors').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(new Error('Allowed import of duplicate data')); }).catch(function (response) { @@ -601,7 +601,7 @@ describe('Import', function () { testUtils.fixtures.loadExportFixture('export-003-mu-unknownAuthor').then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(new Error('Allowed import of unknown author')); }).catch(function (response) { @@ -622,7 +622,7 @@ describe('Import', function () { exportData.data.tags.length.should.be.above(1); exportData.data.posts_tags.length.should.be.above(1); - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(new Error('Allowed import of invalid tags data')); }).catch(function (response) { @@ -643,7 +643,7 @@ describe('Import', function () { exportData.data.posts.length.should.be.above(1); - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(new Error('Allowed import of invalid tags data')); }).catch(function (response) { @@ -660,7 +660,7 @@ describe('Import', function () { exportData.data.posts.length.should.be.above(0); - return importer(exportData); + return importer.doImport(exportData); }).then(function () { // Grab the data from tables return knex('posts').select(); @@ -693,7 +693,7 @@ describe('Import (new test structure)', function () { return testUtils.fixtures.loadExportFixture('export-003-mu'); }).then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(); }).catch(done); @@ -920,7 +920,7 @@ describe('Import (new test structure)', function () { return testUtils.fixtures.loadExportFixture('export-003-mu-noOwner'); }).then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(); }).catch(done); @@ -1148,7 +1148,7 @@ describe('Import (new test structure)', function () { return testUtils.fixtures.loadExportFixture('export-003-mu'); }).then(function (exported) { exportData = exported; - return importer(exportData); + return importer.doImport(exportData); }).then(function () { done(); }).catch(done); diff --git a/core/test/unit/importer_spec.js b/core/test/unit/importer_spec.js index 8dafd380a19c..bb703d7b0684 100644 --- a/core/test/unit/importer_spec.js +++ b/core/test/unit/importer_spec.js @@ -6,6 +6,7 @@ var should = require('should'), _ = require('lodash'), testUtils = require('../utils'), config = require('../../server/config'), + path = require('path'), // Stuff we are testing ImportManager = require('../../server/data/importer'), @@ -14,8 +15,8 @@ var should = require('should'), DataImporter = require('../../server/data/importer/importers/data'), ImageImporter = require('../../server/data/importer/importers/image'), - sandbox = sinon.sandbox.create(), - storage = require('../../server/storage'); + storage = require('../../server/storage'), + sandbox = sinon.sandbox.create(); // To stop jshint complaining should.equal(true, true); @@ -50,8 +51,25 @@ describe('Importer', function () { ImportManager.getTypes().should.containEql('application/x-zip-compressed'); }); + it('gets the correct directories', function () { + ImportManager.getDirectories().should.be.instanceof(Array).and.have.lengthOf(2); + ImportManager.getDirectories().should.containEql('images'); + ImportManager.getDirectories().should.containEql('content'); + }); + it('globs extensions correctly', function () { - ImportManager.getGlobPattern(JSONHandler).should.equal('**/*+(.json)'); + ImportManager.getGlobPattern(ImportManager.getExtensions()).should.equal('+(.jpg|.jpeg|.gif|.png|.svg|.svgz|.json|.zip)'); + ImportManager.getGlobPattern(ImportManager.getDirectories()).should.equal('+(images|content)'); + ImportManager.getGlobPattern(JSONHandler.extensions).should.equal('+(.json)'); + ImportManager.getGlobPattern(ImageHandler.extensions).should.equal('+(.jpg|.jpeg|.gif|.png|.svg|.svgz)'); + ImportManager.getExtensionGlob(ImportManager.getExtensions()).should.equal('*+(.jpg|.jpeg|.gif|.png|.svg|.svgz|.json|.zip)'); + ImportManager.getDirectoryGlob(ImportManager.getDirectories()).should.equal('+(images|content)'); + ImportManager.getExtensionGlob(ImportManager.getExtensions(), 0).should.equal('*+(.jpg|.jpeg|.gif|.png|.svg|.svgz|.json|.zip)'); + ImportManager.getDirectoryGlob(ImportManager.getDirectories(), 0).should.equal('+(images|content)'); + ImportManager.getExtensionGlob(ImportManager.getExtensions(), 1).should.equal('{*/*,*}+(.jpg|.jpeg|.gif|.png|.svg|.svgz|.json|.zip)'); + ImportManager.getDirectoryGlob(ImportManager.getDirectories(), 1).should.equal('{*/,}+(images|content)'); + ImportManager.getExtensionGlob(ImportManager.getExtensions(), 2).should.equal('**/*+(.jpg|.jpeg|.gif|.png|.svg|.svgz|.json|.zip)'); + ImportManager.getDirectoryGlob(ImportManager.getDirectories(), 2).should.equal('**/+(images|content)'); }); // Step 1 of importing is loadFile @@ -59,13 +77,11 @@ describe('Importer', function () { it('knows when to process a file', function (done) { var testFile = {name: 'myFile.json', path: '/my/path/myFile.json'}, zipSpy = sandbox.stub(ImportManager, 'processZip').returns(Promise.resolve()), - fileSpy = sandbox.stub(ImportManager, 'processFile').returns(Promise.resolve()), - cleanSpy = sandbox.stub(ImportManager, 'cleanUp').returns(Promise.resolve()); + fileSpy = sandbox.stub(ImportManager, 'processFile').returns(Promise.resolve()); ImportManager.loadFile(testFile).then(function () { zipSpy.calledOnce.should.be.false; fileSpy.calledOnce.should.be.true; - cleanSpy.calledOnce.should.be.true; done(); }); }); @@ -74,13 +90,11 @@ describe('Importer', function () { it('knows when to process a zip', function (done) { var testZip = {name: 'myFile.zip', path: '/my/path/myFile.zip'}, zipSpy = sandbox.stub(ImportManager, 'processZip').returns(Promise.resolve()), - fileSpy = sandbox.stub(ImportManager, 'processFile').returns(Promise.resolve()), - cleanSpy = sandbox.stub(ImportManager, 'cleanUp').returns(Promise.resolve()); + fileSpy = sandbox.stub(ImportManager, 'processFile').returns(Promise.resolve()); ImportManager.loadFile(testZip).then(function () { zipSpy.calledOnce.should.be.true; fileSpy.calledOnce.should.be.false; - cleanSpy.calledOnce.should.be.true; done(); }); }); @@ -90,20 +104,20 @@ describe('Importer', function () { testZip = {name: 'myFile.zip', path: '/my/path/myFile.zip'}, // need to stub out the extract and glob function for zip extractSpy = sandbox.stub(ImportManager, 'extractZip').returns(Promise.resolve('/tmp/dir/')), + validSpy = sandbox.stub(ImportManager, 'isValidZip').returns(Promise.resolve()), getFileSpy = sandbox.stub(ImportManager, 'getFilesFromZip'), jsonSpy = sandbox.stub(JSONHandler, 'loadFile').returns(Promise.resolve({posts: []})), - imageSpy = sandbox.stub(ImageHandler, 'loadFile'), - cleanSpy = sandbox.stub(ImportManager, 'cleanUp').returns(Promise.resolve()); + imageSpy = sandbox.stub(ImageHandler, 'loadFile'); getFileSpy.withArgs(JSONHandler).returns(['/tmp/dir/myFile.json']); getFileSpy.withArgs(ImageHandler).returns([]); ImportManager.processZip(testZip).then(function (zipResult) { extractSpy.calledOnce.should.be.true; + validSpy.calledOnce.should.be.true; getFileSpy.calledTwice.should.be.true; jsonSpy.calledOnce.should.be.true; imageSpy.called.should.be.false; - cleanSpy.calledOnce.should.be.true; ImportManager.processFile(testFile, '.json').then(function (fileResult) { jsonSpy.calledTwice.should.be.true; @@ -116,6 +130,72 @@ describe('Importer', function () { }); }); }); + + describe('Validate Zip', function () { + it('accepts a zip with a base directory', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-base-dir'); + ImportManager.isValidZip(testDir).then(function (isValid) { + isValid.should.be.ok; + done(); + }); + }); + + it('accepts a zip without a base directory', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-without-base-dir'); + ImportManager.isValidZip(testDir).then(function (isValid) { + isValid.should.be.ok; + done(); + }); + }); + + it('accepts a zip with an image directory', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-image-dir'); + ImportManager.isValidZip(testDir).then(function (isValid) { + isValid.should.be.ok; + done(); + }); + }); + + it('fails a zip with two base directories', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-double-base-dir'); + ImportManager.isValidZip(testDir).then(function () { + done(new Error('Double base directory did not throw error')); + }).catch(function (err) { + err.message.should.equal('Invalid zip file structure.'); + err.type.should.equal('UnsupportedMediaTypeError'); + done(); + }); + }); + + it('fails a zip with no content', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-invalid'); + ImportManager.isValidZip(testDir).then(function () { + done(new Error('Double base directory did not throw error')); + }).catch(function (err) { + err.message.should.equal('Zip did not include any content to import.'); + err.type.should.equal('UnsupportedMediaTypeError'); + done(); + }); + }); + }); + + describe('Get Base Dir', function () { + it('returns string for base directory', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-base-dir'); + ImportManager.getBaseDirectory(testDir).then(function (baseDir) { + baseDir.should.equal('basedir'); + done(); + }); + }); + + it('returns empty for no base directory', function (done) { + var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-without-base-dir'); + ImportManager.getBaseDirectory(testDir).then(function (baseDir) { + should.not.exist(baseDir); + done(); + }); + }); + }); }); // Step 2 of importing is preProcess @@ -125,15 +205,19 @@ describe('Importer', function () { var input = {data: {}, images: []}, // pass a copy so that input doesn't get modified inputCopy = _.cloneDeep(input), - dataSpy = sandbox.spy(DataImporter, 'preProcess'); + dataSpy = sandbox.spy(DataImporter, 'preProcess'), + imageSpy = sandbox.spy(ImageImporter, 'preProcess'); ImportManager.preProcess(inputCopy).then(function (output) { dataSpy.calledOnce.should.be.true; dataSpy.calledWith(inputCopy).should.be.true; + imageSpy.calledOnce.should.be.true; + imageSpy.calledWith(inputCopy).should.be.true; // eql checks for equality // equal checks the references are for the same object output.should.not.equal(input); output.should.have.property('preProcessedByData', true); + output.should.have.property('preProcessedByImage', true); done(); }); }); @@ -185,6 +269,27 @@ describe('Importer', function () { }); }); }); + + describe('importFromFile', function () { + it('does the import steps in order', function (done) { + var loadFileSpy = sandbox.stub(ImportManager, 'loadFile').returns(Promise.resolve()), + preProcessSpy = sandbox.stub(ImportManager, 'preProcess').returns(Promise.resolve()), + doImportSpy = sandbox.stub(ImportManager, 'doImport').returns(Promise.resolve()), + generateReportSpy = sandbox.stub(ImportManager, 'generateReport').returns(Promise.resolve()), + cleanupSpy = sandbox.stub(ImportManager, 'cleanUp').returns({}); + + ImportManager.importFromFile({}).then(function () { + loadFileSpy.calledOnce.should.be.true; + preProcessSpy.calledOnce.should.be.true; + doImportSpy.calledOnce.should.be.true; + generateReportSpy.calledOnce.should.be.true; + cleanupSpy.calledOnce.should.be.true; + sinon.assert.callOrder(loadFileSpy, preProcessSpy, doImportSpy, generateReportSpy, cleanupSpy); + + done(); + }); + }); + }); }); describe('JSONHandler', function () { @@ -227,7 +332,6 @@ describe('Importer', function () { describe('ImageHandler', function () { var origConfig = _.cloneDeep(config), - storage = require('../../server/storage'), store = storage.getStorage(); afterEach(function () { @@ -375,11 +479,33 @@ describe('Importer', function () { }); describe('DataImporter', function () { + var importer = require('../../server/data/import'); + it('has the correct interface', function () { DataImporter.type.should.eql('data'); DataImporter.preProcess.should.be.instanceof(Function); DataImporter.doImport.should.be.instanceof(Function); }); + + it('does preprocess posts, users and tags correctly', function () { + var inputData = require('../utils/fixtures/import/import-data-1.json'), + outputData = DataImporter.preProcess(_.cloneDeep(inputData)); + + // Data preprocess is a noop + inputData.data.data.posts[0].should.eql(outputData.data.data.posts[0]); + inputData.data.data.tags[0].should.eql(outputData.data.data.tags[0]); + inputData.data.data.users[0].should.eql(outputData.data.data.users[0]); + }); + + it('does import the data correctly', function () { + var inputData = require('../utils/fixtures/import/import-data-1.json'), + importerSpy = sandbox.stub(importer, 'doImport').returns(Promise.resolve()); + + DataImporter.doImport(inputData.data).then(function () { + importerSpy.calledOnce.should.be.true; + importerSpy.calledWith(inputData.data).should.be.true; + }); + }); }); describe('ImageImporter', function () { @@ -389,22 +515,33 @@ describe('Importer', function () { ImageImporter.doImport.should.be.instanceof(Function); }); - it('does preprocess posts correctly', function () { + it('does preprocess posts, users and tags correctly', function () { var inputData = require('../utils/fixtures/import/import-data-1.json'), outputData = ImageImporter.preProcess(_.cloneDeep(inputData)); - inputData.data.data.posts[0].markdown.should.not.containEql('/content/images/my-image.png'); - inputData.data.data.posts[0].html.should.not.containEql('/content/images/my-image.png'); - outputData.data.data.posts[0].markdown.should.containEql('/content/images/my-image.png'); - outputData.data.data.posts[0].html.should.containEql('/content/images/my-image.png'); + inputData = inputData.data.data; + outputData = outputData.data.data; + + inputData.posts[0].markdown.should.not.containEql('/content/images/my-image.png'); + inputData.posts[0].html.should.not.containEql('/content/images/my-image.png'); + outputData.posts[0].markdown.should.containEql('/content/images/my-image.png'); + outputData.posts[0].html.should.containEql('/content/images/my-image.png'); + + inputData.posts[0].markdown.should.not.containEql('/content/images/photos/cat.jpg'); + inputData.posts[0].html.should.not.containEql('/content/images/photos/cat.jpg'); + outputData.posts[0].markdown.should.containEql('/content/images/photos/cat.jpg'); + outputData.posts[0].html.should.containEql('/content/images/photos/cat.jpg'); + + inputData.posts[0].image.should.eql('/images/my-image.png'); + outputData.posts[0].image.should.eql('/content/images/my-image.png'); - inputData.data.data.posts[0].markdown.should.not.containEql('/content/images/photos/cat.jpg'); - inputData.data.data.posts[0].html.should.not.containEql('/content/images/photos/cat.jpg'); - outputData.data.data.posts[0].markdown.should.containEql('/content/images/photos/cat.jpg'); - outputData.data.data.posts[0].html.should.containEql('/content/images/photos/cat.jpg'); + inputData.tags[0].image.should.eql('/images/my-image.png'); + outputData.tags[0].image.should.eql('/content/images/my-image.png'); - inputData.data.data.posts[0].image.should.eql('/images/my-image.png'); - outputData.data.data.posts[0].image.should.eql('/content/images/my-image.png'); + inputData.users[0].image.should.eql('/images/my-image.png'); + inputData.users[0].cover.should.eql('/images/photos/cat.jpg'); + outputData.users[0].image.should.eql('/content/images/my-image.png'); + outputData.users[0].cover.should.eql('/content/images/photos/cat.jpg'); }); it('does import the images correctly', function () { diff --git a/core/test/utils/fixtures/import/import-data-1.json b/core/test/utils/fixtures/import/import-data-1.json index 429a988c6918..3d02d41bc848 100644 --- a/core/test/utils/fixtures/import/import-data-1.json +++ b/core/test/utils/fixtures/import/import-data-1.json @@ -52,6 +52,7 @@ "name": "Getting Started", "slug": "getting-started", "description": null, + "image": "/images/my-image.png", "parent_id": null, "meta_title": null, "meta_description": null, @@ -61,6 +62,14 @@ "updated_by": 1 } ], + "users": [ + { + "name": "test user", + "email": "test@ghost.org", + "image": "/images/my-image.png", + "cover": "/images/photos/cat.jpg" + } + ], "posts_tags": [ { "id": 1, diff --git a/core/test/utils/fixtures/import/zips/zip-image-dir/images/image.jpg b/core/test/utils/fixtures/import/zips/zip-image-dir/images/image.jpg new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/core/test/utils/fixtures/import/zips/zip-with-base-dir/basedir/test.json b/core/test/utils/fixtures/import/zips/zip-with-base-dir/basedir/test.json new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/core/test/utils/fixtures/import/zips/zip-with-double-base-dir/basedir/basedir/test.json b/core/test/utils/fixtures/import/zips/zip-with-double-base-dir/basedir/basedir/test.json new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/core/test/utils/fixtures/import/zips/zip-without-base-dir/test.json b/core/test/utils/fixtures/import/zips/zip-without-base-dir/test.json new file mode 100644 index 000000000000..e69de29bb2d1 From 05877124ae1b7660cf73db7edc5918266bed9ddb Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sat, 3 Jan 2015 21:11:40 +0000 Subject: [PATCH 2/2] Remove unneeded promises and fix tests --- core/server/data/importer/index.js | 42 +++++-------- core/test/unit/importer_spec.js | 95 ++++++++++++------------------ 2 files changed, 54 insertions(+), 83 deletions(-) diff --git a/core/server/data/importer/index.js b/core/server/data/importer/index.js index 8075522e7612..02bbb4e6851a 100644 --- a/core/server/data/importer/index.js +++ b/core/server/data/importer/index.js @@ -143,16 +143,14 @@ _.extend(ImportManager.prototype, { // If this folder contains importable files or a content or images directory if (extMatchesBase.length > 0 || (dirMatches.length > 0 && extMatchesAll.length > 0)) { - return Promise.resolve(true); + return true; } if (extMatchesAll.length < 1) { - return Promise.reject(new errors.UnsupportedMediaTypeError( - 'Zip did not include any content to import.' - )); + throw new errors.UnsupportedMediaTypeError('Zip did not include any content to import.'); } - return Promise.reject(new errors.UnsupportedMediaTypeError('Invalid zip file structure.')); + throw new errors.UnsupportedMediaTypeError('Invalid zip file structure.'); }, /** * Use the extract module to extract the given zip file to a temp directory & return the temp directory path @@ -192,18 +190,17 @@ _.extend(ImportManager.prototype, { // There is no base directory if (extMatches.length > 0 || dirMatches.length > 0) { - return Promise.resolve(); + return; } - // There is a base directory, grab it from any ext match extMatchesAll = glob.sync( this.getExtensionGlob(this.getExtensions(), ALL_DIRS), {cwd: directory} ); if (extMatchesAll.length < 1 || extMatchesAll[0].split('/') < 1) { - return Promise.resolve(new errors.ValidationError('Invalid zip file: base directory read failed')); + throw new errors.ValidationError('Invalid zip file: base directory read failed'); } - return Promise.resolve(extMatchesAll[0].split('/')[0]); + return extMatchesAll[0].split('/')[0]; }, /** * Process Zip @@ -215,16 +212,15 @@ _.extend(ImportManager.prototype, { * @returns {Promise(ImportData)} */ processZip: function (file) { - var self = this, - directory; + var self = this; + return this.extractZip(file.path).then(function (zipDirectory) { - directory = zipDirectory; - return self.isValidZip(directory); - }).then(function () { - return self.getBaseDirectory(directory); - }).then(function (baseDir) { var ops = [], - importData = {}; + importData = {}, + baseDir; + + self.isValidZip(zipDirectory); + baseDir = self.getBaseDirectory(zipDirectory); _.each(self.handlers, function (handler) { if (importData.hasOwnProperty(handler.type)) { @@ -234,7 +230,7 @@ _.extend(ImportManager.prototype, { )); } - var files = self.getFilesFromZip(handler, directory); + var files = self.getFilesFromZip(handler, zipDirectory); if (files.length > 0) { ops.push(function () { @@ -290,15 +286,7 @@ _.extend(ImportManager.prototype, { this.filesToDelete.push(file.path); - return Promise.resolve(this.isZip(ext)).then(function (isZip) { - if (isZip) { - // If it's a zip, process the zip file - return self.processZip(file); - } else { - // Else process the file - return self.processFile(file, ext); - } - }); + return this.isZip(ext) ? self.processZip(file) : self.processFile(file, ext); }, /** * Import Step 2: diff --git a/core/test/unit/importer_spec.js b/core/test/unit/importer_spec.js index bb703d7b0684..d388cb480ecd 100644 --- a/core/test/unit/importer_spec.js +++ b/core/test/unit/importer_spec.js @@ -7,6 +7,7 @@ var should = require('should'), testUtils = require('../utils'), config = require('../../server/config'), path = require('path'), + errors = require('../../server/errors'), // Stuff we are testing ImportManager = require('../../server/data/importer'), @@ -83,7 +84,7 @@ describe('Importer', function () { zipSpy.calledOnce.should.be.false; fileSpy.calledOnce.should.be.true; done(); - }); + }).catch(done); }); // We need to make sure we don't actually extract a zip and leave temporary files everywhere! @@ -96,7 +97,7 @@ describe('Importer', function () { zipSpy.calledOnce.should.be.true; fileSpy.calledOnce.should.be.false; done(); - }); + }).catch(done); }); it('has same result for zips and files', function (done) { @@ -104,7 +105,8 @@ describe('Importer', function () { testZip = {name: 'myFile.zip', path: '/my/path/myFile.zip'}, // need to stub out the extract and glob function for zip extractSpy = sandbox.stub(ImportManager, 'extractZip').returns(Promise.resolve('/tmp/dir/')), - validSpy = sandbox.stub(ImportManager, 'isValidZip').returns(Promise.resolve()), + validSpy = sandbox.stub(ImportManager, 'isValidZip').returns(true), + baseDirSpy = sandbox.stub(ImportManager, 'getBaseDirectory').returns(), getFileSpy = sandbox.stub(ImportManager, 'getFilesFromZip'), jsonSpy = sandbox.stub(JSONHandler, 'loadFile').returns(Promise.resolve({posts: []})), imageSpy = sandbox.stub(ImageHandler, 'loadFile'); @@ -115,6 +117,7 @@ describe('Importer', function () { ImportManager.processZip(testZip).then(function (zipResult) { extractSpy.calledOnce.should.be.true; validSpy.calledOnce.should.be.true; + baseDirSpy.calledOnce.should.be.true; getFileSpy.calledTwice.should.be.true; jsonSpy.calledOnce.should.be.true; imageSpy.called.should.be.false; @@ -128,72 +131,52 @@ describe('Importer', function () { zipResult.should.eql(fileResult); done(); }); - }); + }).catch(done); }); describe('Validate Zip', function () { - it('accepts a zip with a base directory', function (done) { + it('accepts a zip with a base directory', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-base-dir'); - ImportManager.isValidZip(testDir).then(function (isValid) { - isValid.should.be.ok; - done(); - }); + + ImportManager.isValidZip(testDir).should.be.ok; }); - it('accepts a zip without a base directory', function (done) { + it('accepts a zip without a base directory', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-without-base-dir'); - ImportManager.isValidZip(testDir).then(function (isValid) { - isValid.should.be.ok; - done(); - }); + + ImportManager.isValidZip(testDir).should.be.ok; }); - it('accepts a zip with an image directory', function (done) { + it('accepts a zip with an image directory', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-image-dir'); - ImportManager.isValidZip(testDir).then(function (isValid) { - isValid.should.be.ok; - done(); - }); + + ImportManager.isValidZip(testDir).should.be.ok; }); - it('fails a zip with two base directories', function (done) { + it('fails a zip with two base directories', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-double-base-dir'); - ImportManager.isValidZip(testDir).then(function () { - done(new Error('Double base directory did not throw error')); - }).catch(function (err) { - err.message.should.equal('Invalid zip file structure.'); - err.type.should.equal('UnsupportedMediaTypeError'); - done(); - }); + + ImportManager.isValidZip.bind(ImportManager, testDir).should.throw(errors.UnsupportedMediaTypeError); }); - it('fails a zip with no content', function (done) { + it('fails a zip with no content', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-invalid'); - ImportManager.isValidZip(testDir).then(function () { - done(new Error('Double base directory did not throw error')); - }).catch(function (err) { - err.message.should.equal('Zip did not include any content to import.'); - err.type.should.equal('UnsupportedMediaTypeError'); - done(); - }); + + ImportManager.isValidZip.bind(ImportManager, testDir).should.throw(errors.UnsupportedMediaTypeError); }); }); describe('Get Base Dir', function () { - it('returns string for base directory', function (done) { + it('returns string for base directory', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-with-base-dir'); - ImportManager.getBaseDirectory(testDir).then(function (baseDir) { - baseDir.should.equal('basedir'); - done(); - }); + + ImportManager.getBaseDirectory(testDir).should.equal('basedir'); }); - it('returns empty for no base directory', function (done) { + it('returns empty for no base directory', function () { var testDir = path.resolve('core/test/utils/fixtures/import/zips/zip-without-base-dir'); - ImportManager.getBaseDirectory(testDir).then(function (baseDir) { - should.not.exist(baseDir); - done(); - }); + + should.not.exist(ImportManager.getBaseDirectory(testDir)); }); }); }); @@ -219,7 +202,7 @@ describe('Importer', function () { output.should.have.property('preProcessedByData', true); output.should.have.property('preProcessedByImage', true); done(); - }); + }).catch(done); }); }); @@ -253,7 +236,7 @@ describe('Importer', function () { // we stubbed this as a noop but ImportManager calls with sequence, so we should get an array output.should.eql([expectedImages, expectedData]); done(); - }); + }).catch(done); }); }); @@ -266,7 +249,7 @@ describe('Importer', function () { ImportManager.generateReport(input).then(function (output) { output.should.equal(input); done(); - }); + }).catch(done); }); }); @@ -287,7 +270,7 @@ describe('Importer', function () { sinon.assert.callOrder(loadFileSpy, preProcessSpy, doImportSpy, generateReportSpy, cleanupSpy); done(); - }); + }).catch(done); }); }); }); @@ -312,7 +295,7 @@ describe('Importer', function () { _.keys(result).should.containEql('meta'); _.keys(result).should.containEql('data'); done(); - }); + }).catch(done); }); it('correctly errors when given a bad db api wrapper', function (done) { @@ -326,7 +309,7 @@ describe('Importer', function () { }).catch(function (response) { response.type.should.equal('BadRequestError'); done(); - }); + }).catch(done); }); }); @@ -372,7 +355,7 @@ describe('Importer', function () { storeSpy.firstCall.args[1].newPath.should.eql('/content/images/test-image.jpeg'); done(); - }); + }).catch(done); }); it('can load a single file, maintaining structure', function (done) { @@ -392,7 +375,7 @@ describe('Importer', function () { storeSpy.firstCall.args[1].newPath.should.eql('/content/images/photos/my-cat.jpeg'); done(); - }); + }).catch(done); }); it('can load a single file, removing ghost dirs', function (done) { @@ -412,7 +395,7 @@ describe('Importer', function () { storeSpy.firstCall.args[1].newPath.should.eql('/content/images/my-cat.jpeg'); done(); - }); + }).catch(done); }); it('can load a file (subdirectory)', function (done) { @@ -434,7 +417,7 @@ describe('Importer', function () { storeSpy.firstCall.args[1].newPath.should.eql('/subdir/content/images/test-image.jpeg'); done(); - }); + }).catch(done); }); it('can load multiple files', function (done) { @@ -474,7 +457,7 @@ describe('Importer', function () { storeSpy.lastCall.args[1].newPath.should.eql('/content/images/puppy.jpg'); done(); - }); + }).catch(done); }); });