From 817b8d09cafe4d22d8371844379eb61315babe10 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Wed, 5 Apr 2017 16:10:34 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=98=B1=20=F0=9F=8E=A8=20Refactor=20storag?= =?UTF-8?q?e=20adapter=20(#8229)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs #7687 There are four main changes in this PR: we have outsourced the base storage adapter to npm, because for storage developers it's annoying to inherit from a script within Ghost we hacked theme storage handling into the default local storage adapter - this was reverted, instead we have added a static theme storage here use classes instead of prototyping optimise the storage adapter in general - everything is explained in each commit ---- * rename local-file-store to LocalFileStorage I would like to keep the name pattern i have used for scheduling. If a file is a class, the file name reflects the class name. We can discuss this, if concerns are raised. * Transform LocalFileStorage to class and inherit from new base - inherit from npm ghost-storage-base - rewrite to class - no further refactoring, happens later * Rename core/test/unit/storage/local-file-store_spec.js -> core/test/unit/storage/LocalFileStorage_spec.js * Fix wrong require in core/test/unit/storage/LocalFileStorage_spec.js * remove base storage and test - see https://github.com/kirrg001/Ghost-Storage-Base - the test has moved to this repo as well * Use npm ghost-storage-base in storage/index.js * remove the concept of getStorage('themes') This concept was added when we added themes as a feature. Back then, we have changed the local storage adapter to support images and themes. This has added some hacks into the local storage adapters. We want to revert this change and add a simple static theme storage. Will adapt the api/themes layer in the next commits. * Revert LocalFileStorage - revert serve - revert delete * add storagePath as property to LocalFileStorage - define one property which holds the storage path - could be considered to pass from outside, but found that not helpful, as other storage adapters do not need this property - IMPORTANT: save has no longer a targetDir option, because this was used to pass the alternative theme storage path - IMPORTANT: exists has now an alternative targetDir, this makes sense, because - you can either ask the storage exists('my-file') and it will look in the base storage path - or you pass a specific path where to look exists('my-file', /path/to/dir) * LocalFileStorage: get rid of store pattern - getUniqueFileName(THIS) - this doesn't make sense, instances always have access to this by default * Add static theme storage - inherits from the local file storage, because they both operate on the file system - IMPORTANT: added a TODO to consider a merge of themes/loader and themes/storage - but will be definitely not part of this PR * Use new static theme storage in api/themes - storage functions are simplified! * Add https://github.com/kirrg001/Ghost-Storage-Base as dependency - tarball for now, as i am still testing - will release if PR review get's accepted * Adapt tests and jscs/jshint * 🐛 fix storage.read in favicon utility - wrong implementation of error handling * 🎨 optimise error messages for custom storage adapter errors * little renaming in the storage utlity - purpose is to have access to the custom storage instance and to the custom storage class - see next commit why * optimise instanceof base storage - instanceof is always tricky in javascript - if multiple modules exist, it can happen that instanceof is false * fix getTargetDir - the importer uses the `targetDir` option to ensure that images land in the correct folder * ghost-storage-base@0.0.1 package.json dependency --- core/server/api/themes.js | 28 ++- core/server/config/defaults.json | 4 +- core/server/config/overrides.json | 5 - core/server/data/importer/handlers/image.js | 2 +- core/server/middleware/serve-favicon.js | 21 ++- core/server/storage/LocalFileStorage.js | 135 +++++++++++++ core/server/storage/base.js | 68 ------- core/server/storage/index.js | 78 +++++--- core/server/storage/local-file-store.js | 152 --------------- core/server/themes/Storage.js | 68 +++++++ core/server/themes/index.js | 9 +- core/test/unit/config/index_spec.js | 5 +- core/test/unit/importer_spec.js | 48 ++--- .../unit/middleware/serve-favicon_spec.js | 10 +- ...store_spec.js => LocalFileStorage_spec.js} | 4 +- core/test/unit/storage/base_spec.js | 9 - core/test/unit/storage/index_spec.js | 178 +++++++----------- package.json | 1 + yarn.lock | 8 +- 19 files changed, 389 insertions(+), 444 deletions(-) create mode 100644 core/server/storage/LocalFileStorage.js delete mode 100644 core/server/storage/base.js delete mode 100644 core/server/storage/local-file-store.js create mode 100644 core/server/themes/Storage.js rename core/test/unit/storage/{local-file-store_spec.js => LocalFileStorage_spec.js} (98%) delete mode 100644 core/test/unit/storage/base_spec.js diff --git a/core/server/api/themes.js b/core/server/api/themes.js index 5a5e87d567..979b7ddce6 100644 --- a/core/server/api/themes.js +++ b/core/server/api/themes.js @@ -3,13 +3,10 @@ var debug = require('debug')('ghost:api:themes'), Promise = require('bluebird'), fs = require('fs-extra'), - config = require('../config'), errors = require('../errors'), events = require('../events'), logging = require('../logging'), - storage = require('../storage'), apiUtils = require('./utils'), - utils = require('./../utils'), i18n = require('../i18n'), settingsModel = require('../models/settings').Settings, settingsCache = require('../settings/cache'), @@ -82,11 +79,10 @@ themes = { // consistent filename uploads options.originalname = options.originalname.toLowerCase(); - var storageAdapter = storage.getStorage('themes'), - zip = { + var zip = { path: options.path, name: options.originalname, - shortName: storageAdapter.getSanitizedFileName(options.originalname.split('.zip')[0]) + shortName: themeUtils.storage.getSanitizedFileName(options.originalname.split('.zip')[0]) }, checkedTheme; @@ -106,22 +102,22 @@ themes = { .then(function checkExists(_checkedTheme) { checkedTheme = _checkedTheme; - return storageAdapter.exists(utils.url.urlJoin(config.getContentPath('themes'), zip.shortName)); + return themeUtils.storage.exists(zip.shortName); }) // If the theme existed we need to delete it .then(function removeOldTheme(themeExists) { // delete existing theme if (themeExists) { - return storageAdapter.delete(zip.shortName, config.getContentPath('themes')); + return themeUtils.storage.delete(zip.shortName); } }) .then(function storeNewTheme() { events.emit('theme.uploaded', zip.shortName); // store extracted theme - return storageAdapter.save({ + return themeUtils.storage.save({ name: zip.shortName, path: checkedTheme.path - }, config.getContentPath('themes')); + }); }) .then(function loadNewTheme() { // Loads the theme from the filesystem @@ -163,8 +159,7 @@ themes = { download: function download(options) { var themeName = options.name, - theme = themeList.get(themeName), - storageAdapter = storage.getStorage('themes'); + theme = themeList.get(themeName); if (!theme) { return Promise.reject(new errors.BadRequestError({message: i18n.t('errors.api.themes.invalidRequest')})); @@ -175,7 +170,9 @@ themes = { .handlePermissions('themes', 'read')(options) .then(function sendTheme() { events.emit('theme.downloaded', themeName); - return storageAdapter.serve({isTheme: true, name: themeName}); + return themeUtils.storage.serve({ + name: themeName + }); }); }, @@ -185,8 +182,7 @@ themes = { */ destroy: function destroy(options) { var themeName = options.name, - theme, - storageAdapter = storage.getStorage('themes'); + theme; return apiUtils // Permissions @@ -208,7 +204,7 @@ themes = { } // Actually do the deletion here - return storageAdapter.delete(themeName, config.getContentPath('themes')); + return themeUtils.storage.delete(themeName); }) // And some extra stuff to maintain state here .then(function deleteTheme() { diff --git a/core/server/config/defaults.json b/core/server/config/defaults.json index ddc918e101..984d8e0232 100644 --- a/core/server/config/defaults.json +++ b/core/server/config/defaults.json @@ -9,9 +9,7 @@ "contentPath": "content/" }, "storage": { - "active": { - "images": "local-file-store" - } + "active": "LocalFileStorage" }, "scheduling": { "active": "SchedulingDefault" diff --git a/core/server/config/overrides.json b/core/server/config/overrides.json index 657f2946ad..0d6535346a 100644 --- a/core/server/config/overrides.json +++ b/core/server/config/overrides.json @@ -55,11 +55,6 @@ "contentTypes": ["application/zip", "application/x-zip-compressed", "application/octet-stream"] } }, - "storage": { - "active": { - "themes": "local-file-store" - } - }, "times": { "cannotScheduleAPostBeforeInMinutes": 2, "publishAPostBySchedulerToleranceInMinutes": 2 diff --git a/core/server/data/importer/handlers/image.js b/core/server/data/importer/handlers/image.js index b9bbed6b35..5b8c8ee9df 100644 --- a/core/server/data/importer/handlers/image.js +++ b/core/server/data/importer/handlers/image.js @@ -36,7 +36,7 @@ ImageHandler = { }); return Promise.map(files, function (image) { - return store.getUniqueFileName(store, image, image.targetDir).then(function (targetFilename) { + return store.getUniqueFileName(image, image.targetDir).then(function (targetFilename) { image.newPath = utils.url.urlJoin('/', utils.url.getSubdir(), utils.url.STATIC_IMAGE_URL_PREFIX, path.relative(config.getContentPath('images'), targetFilename)); diff --git a/core/server/middleware/serve-favicon.js b/core/server/middleware/serve-favicon.js index 75d9235b46..9e48d524ba 100644 --- a/core/server/middleware/serve-favicon.js +++ b/core/server/middleware/serve-favicon.js @@ -48,17 +48,18 @@ function serveFavicon() { return res.redirect(302, '/favicon' + originalExtension); } - storage.getStorage().read({path: filePath}).then(function readFile(buf, err) { - if (err) { - return next(err); - } + storage.getStorage() + .read({path: filePath}) + .then(function readFile(buf) { + iconType = settingsCache.get('icon').match(/\/favicon\.ico$/i) ? 'x-icon' : 'png'; + content = buildContentResponse(iconType, buf); - iconType = settingsCache.get('icon').match(/\/favicon\.ico$/i) ? 'x-icon' : 'png'; - content = buildContentResponse(iconType, buf); - - res.writeHead(200, content.headers); - res.end(content.body); - }); + res.writeHead(200, content.headers); + res.end(content.body); + }) + .catch(function (err) { + next(err); + }); } else { filePath = path.join(config.get('paths:corePath'), 'shared', 'favicon.ico'); originalExtension = path.extname(filePath).toLowerCase(); diff --git a/core/server/storage/LocalFileStorage.js b/core/server/storage/LocalFileStorage.js new file mode 100644 index 0000000000..7340a621e7 --- /dev/null +++ b/core/server/storage/LocalFileStorage.js @@ -0,0 +1,135 @@ +// jscs:disable requireMultipleVarDecl + +'use strict'; + +// # Local File System Image Storage module +// The (default) module for storing images, using the local file system + +var serveStatic = require('express').static, + fs = require('fs-extra'), + path = require('path'), + Promise = require('bluebird'), + config = require('../config'), + errors = require('../errors'), + i18n = require('../i18n'), + utils = require('../utils'), + StorageBase = require('ghost-storage-base'); + +class LocalFileStore extends StorageBase { + + constructor() { + super(); + + this.storagePath = config.getContentPath('images'); + } + + /** + * Saves the image to storage (the file system) + * - image is the express image object + * - returns a promise which ultimately returns the full url to the uploaded image + * + * @param image + * @param targetDir + * @returns {*} + */ + save(image, targetDir) { + var targetFilename, + self = this; + + // NOTE: the base implementation of `getTargetDir` returns the format this.storagePath/YYYY/MM + targetDir = targetDir || this.getTargetDir(this.storagePath); + + return this.getUniqueFileName(image, targetDir).then(function (filename) { + targetFilename = filename; + return Promise.promisify(fs.mkdirs)(targetDir); + }).then(function () { + return Promise.promisify(fs.copy)(image.path, targetFilename); + }).then(function () { + // The src for the image must be in URI format, not a file system path, which in Windows uses \ + // For local file system storage can use relative path so add a slash + var fullUrl = ( + utils.url.urlJoin('/', utils.url.getSubdir(), + utils.url.STATIC_IMAGE_URL_PREFIX, + path.relative(self.storagePath, targetFilename)) + ).replace(new RegExp('\\' + path.sep, 'g'), '/'); + + return fullUrl; + }).catch(function (e) { + return Promise.reject(e); + }); + } + + exists(fileName, targetDir) { + var filePath = path.join(targetDir || this.storagePath, fileName); + + return new Promise(function (resolve) { + fs.stat(filePath, function (err) { + var exists = !err; + resolve(exists); + }); + }); + } + + /** + * For some reason send divides the max age number by 1000 + * Fallthrough: false ensures that if an image isn't found, it automatically 404s + * Wrap server static errors + * + * @returns {serveStaticContent} + */ + serve() { + var self = this; + + return function serveStaticContent(req, res, next) { + return serveStatic(self.storagePath, {maxAge: utils.ONE_YEAR_MS, fallthrough: false})(req, res, function (err) { + if (err) { + if (err.statusCode === 404) { + return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); + } + + return next(new errors.GhostError({err: err})); + } + + next(); + }); + }; + } + + /** + * Not implemented. + * @returns {Promise.<*>} + */ + delete() { + return Promise.reject('not implemented'); + } + + /** + * Reads bytes from disk for a target image + * - path of target image (without content path!) + * + * @param options + */ + read(options) { + options = options || {}; + + // remove trailing slashes + options.path = (options.path || '').replace(/\/$|\\$/, ''); + + var targetPath = path.join(this.storagePath, options.path); + + return new Promise(function (resolve, reject) { + fs.readFile(targetPath, function (err, bytes) { + if (err) { + return reject(new errors.GhostError({ + err: err, + message: 'Could not read image: ' + targetPath + })); + } + + resolve(bytes); + }); + }); + } +} + +module.exports = LocalFileStore; diff --git a/core/server/storage/base.js b/core/server/storage/base.js deleted file mode 100644 index 2a6b7b252a..0000000000 --- a/core/server/storage/base.js +++ /dev/null @@ -1,68 +0,0 @@ -var moment = require('moment'), - path = require('path'); - -function StorageBase() { - Object.defineProperty(this, 'requiredFns', { - value: ['exists', 'save', 'serve', 'delete', 'read'], - writable: false - }); -} - -StorageBase.prototype.getTargetDir = function (baseDir) { - var m = moment(), - month = m.format('MM'), - year = m.format('YYYY'); - - if (baseDir) { - return path.join(baseDir, year, month); - } - - return path.join(year, month); -}; - -StorageBase.prototype.generateUnique = function (store, dir, name, ext, i) { - var self = this, - filename, - append = ''; - - if (i) { - append = '-' + i; - } - - if (ext) { - filename = path.join(dir, name + append + ext); - } else { - filename = path.join(dir, name + append); - } - - return store.exists(filename).then(function (exists) { - if (exists) { - i = i + 1; - return self.generateUnique(store, dir, name, ext, i); - } else { - return filename; - } - }); -}; - -StorageBase.prototype.getUniqueFileName = function (store, image, targetDir) { - var ext = path.extname(image.name), name; - - // poor extension validation - // .1 is not a valid extension - if (!ext.match(/.\d/)) { - name = this.getSanitizedFileName(path.basename(image.name, ext)); - return this.generateUnique(store, targetDir, name, ext, 0); - } else { - name = this.getSanitizedFileName(path.basename(image.name)); - return this.generateUnique(store, targetDir, name, null, 0); - } -}; - -StorageBase.prototype.getSanitizedFileName = function getSanitizedFileName(fileName) { - // below only matches ascii characters, @, and . - // unicode filenames like город.zip would therefore resolve to ----.zip - return fileName.replace(/[^\w@.]/gi, '-'); -}; - -module.exports = StorageBase; diff --git a/core/server/storage/index.js b/core/server/storage/index.js index 0cf1000638..96a9cd682d 100644 --- a/core/server/storage/index.js +++ b/core/server/storage/index.js @@ -1,55 +1,62 @@ var errors = require('../errors'), config = require('../config'), - Base = require('./base'), + StorageBase = require('ghost-storage-base'), _ = require('lodash'), storage = {}; /** - * type: images|themes + * type: images */ -function getStorage(type) { - type = type || 'images'; - - var storageChoice = config.get('storage').active[type], - storageConfig; - - // CASE: we only allow local-file-storage for themes - // @TODO: https://github.com/TryGhost/Ghost/issues/7246 - if (type === 'themes') { - storageChoice = 'local-file-store'; - } +function getStorage() { + var storageChoice = config.get('storage:active'), + storageConfig, + CustomStorage, + customStorage; storageConfig = config.get('storage')[storageChoice]; // CASE: type does not exist if (!storageChoice) { throw new errors.IncorrectUsageError({ - message: 'No adapter found for type: ' + type + message: 'No adapter found' }); } - // cache? + // CASE: cached if (storage[storageChoice]) { return storage[storageChoice]; } // CASE: load adapter from custom path (.../content/storage) try { - storage[storageChoice] = require(config.getContentPath('storage') + storageChoice); + CustomStorage = require(config.getContentPath('storage') + storageChoice); } catch (err) { - // CASE: only throw error if module does exist - if (err.code !== 'MODULE_NOT_FOUND') { - throw new errors.IncorrectUsageError({err: err}); + if (err.message.match(/strict mode/gi)) { + throw new errors.IncorrectUsageError({ + message: 'Your custom storage adapter must use strict mode.', + help: 'Add \'use strict\'; on top of your adapter.', + err: err + }); } // CASE: if module not found it can be an error within the adapter (cannot find bluebird for example) else if (err.code === 'MODULE_NOT_FOUND' && err.message.indexOf(config.getContentPath('storage') + storageChoice) === -1) { - throw new errors.IncorrectUsageError({err: err}); + throw new errors.IncorrectUsageError({ + message: 'We have detected an error in your custom storage adapter.', + err: err + }); + } + // CASE: only throw error if module does exist + else if (err.code !== 'MODULE_NOT_FOUND') { + throw new errors.IncorrectUsageError({ + message: 'We have detected an unknown error in your custom storage adapter.', + err: err + }); } } - // CASE: either storage[storageChoice] is already set or why check for in the default storage path + // CASE: check in the default storage path try { - storage[storageChoice] = storage[storageChoice] || require(config.get('paths').internalStoragePath + storageChoice); + CustomStorage = CustomStorage || require(config.get('paths').internalStoragePath + storageChoice); } catch (err) { if (err.code === 'MODULE_NOT_FOUND') { throw new errors.IncorrectUsageError({ @@ -57,24 +64,35 @@ function getStorage(type) { context: 'We cannot find your adapter in: ' + config.getContentPath('storage') + ' or: ' + config.get('paths').internalStoragePath }); } else { - throw new errors.IncorrectUsageError({err: err}); + throw new errors.IncorrectUsageError({ + message: 'We have detected an error in your custom storage adapter.', + err: err + }); } } - storage[storageChoice] = new storage[storageChoice](storageConfig); + customStorage = new CustomStorage(storageConfig); - if (!(storage[storageChoice] instanceof Base)) { - throw new errors.IncorrectUsageError({message: 'Your storage adapter does not inherit from the Storage Base.'}); + // CASE: if multiple StorageBase modules are installed, instanceof could return false + if (Object.getPrototypeOf(CustomStorage).name !== StorageBase.name) { + throw new errors.IncorrectUsageError({ + message: 'Your storage adapter does not inherit from the Storage Base.' + }); } - if (!storage[storageChoice].requiredFns) { - throw new errors.IncorrectUsageError({message:'Your storage adapter does not provide the minimum required functions.'}); + if (!customStorage.requiredFns) { + throw new errors.IncorrectUsageError({ + message: 'Your storage adapter does not provide the minimum required functions.' + }); } - if (_.xor(storage[storageChoice].requiredFns, Object.keys(_.pick(Object.getPrototypeOf(storage[storageChoice]), storage[storageChoice].requiredFns))).length) { - throw new errors.IncorrectUsageError({message:'Your storage adapter does not provide the minimum required functions.'}); + if (_.xor(customStorage.requiredFns, Object.keys(_.pick(Object.getPrototypeOf(customStorage), customStorage.requiredFns))).length) { + throw new errors.IncorrectUsageError({ + message: 'Your storage adapter does not provide the minimum required functions.' + }); } + storage[storageChoice] = customStorage; return storage[storageChoice]; } diff --git a/core/server/storage/local-file-store.js b/core/server/storage/local-file-store.js deleted file mode 100644 index 643449cf95..0000000000 --- a/core/server/storage/local-file-store.js +++ /dev/null @@ -1,152 +0,0 @@ -// # Local File System Image Storage module -// The (default) module for storing images, using the local file system - -var serveStatic = require('express').static, - fs = require('fs-extra'), - os = require('os'), - path = require('path'), - util = require('util'), - Promise = require('bluebird'), - config = require('../config'), - errors = require('../errors'), - i18n = require('../i18n'), - utils = require('../utils'), - BaseStore = require('./base'), - remove = Promise.promisify(fs.remove); - -function LocalFileStore() { - BaseStore.call(this); -} - -util.inherits(LocalFileStore, BaseStore); - -// ### Save -// Saves the image to storage (the file system) -// - image is the express image object -// - returns a promise which ultimately returns the full url to the uploaded image -LocalFileStore.prototype.save = function save(image, targetDir) { - targetDir = targetDir || this.getTargetDir(config.getContentPath('images')); - var targetFilename; - - return this.getUniqueFileName(this, image, targetDir).then(function (filename) { - targetFilename = filename; - return Promise.promisify(fs.mkdirs)(targetDir); - }).then(function () { - return Promise.promisify(fs.copy)(image.path, targetFilename); - }).then(function () { - // The src for the image must be in URI format, not a file system path, which in Windows uses \ - // For local file system storage can use relative path so add a slash - var fullUrl = ( - utils.url.urlJoin('/', utils.url.getSubdir(), - utils.url.STATIC_IMAGE_URL_PREFIX, - path.relative(config.getContentPath('images'), targetFilename)) - ).replace(new RegExp('\\' + path.sep, 'g'), '/'); - - return fullUrl; - }).catch(function (e) { - return Promise.reject(e); - }); -}; - -LocalFileStore.prototype.exists = function exists(filename) { - return new Promise(function (resolve) { - fs.stat(filename, function (err) { - var exists = !err; - resolve(exists); - }); - }); -}; - -// middleware for serving the files -LocalFileStore.prototype.serve = function serve(options) { - options = options || {}; - - // CASE: serve themes - // serveStatic can't be used to serve themes, because - // download files depending on the route (see `send` npm module) - if (options.isTheme) { - return function downloadTheme(req, res, next) { - var themeName = options.name, - themePath = path.join(config.getContentPath('themes'), themeName), - zipName = themeName + '.zip', - // store this in a unique temporary folder - zipBasePath = path.join(os.tmpdir(), utils.uid(10)), - zipPath = path.join(zipBasePath, zipName), - stream; - - Promise.promisify(fs.ensureDir)(zipBasePath) - .then(function () { - return Promise.promisify(utils.zipFolder)(themePath, zipPath); - }) - .then(function (length) { - res.set({ - 'Content-disposition': 'attachment; filename={themeName}.zip'.replace('{themeName}', themeName), - 'Content-Type': 'application/zip', - 'Content-Length': length - }); - - stream = fs.createReadStream(zipPath); - stream.pipe(res); - }) - .catch(function (err) { - next(err); - }) - .finally(function () { - remove(zipBasePath); - }); - }; - } else { - // CASE: serve images - // For some reason send divides the max age number by 1000 - // Fallthrough: false ensures that if an image isn't found, it automatically 404s - // Wrap server static errors - return function serveStaticContent(req, res, next) { - return serveStatic(config.getContentPath('images'), {maxAge: utils.ONE_YEAR_MS, fallthrough: false})(req, res, function (err) { - if (err) { - if (err.statusCode === 404) { - return next(new errors.NotFoundError({message: i18n.t('errors.errors.pageNotFound')})); - } - - return next(new errors.GhostError({err: err})); - } - - next(); - }); - }; - } -}; - -LocalFileStore.prototype.delete = function deleteFile(fileName, targetDir) { - targetDir = targetDir || this.getTargetDir(config.getContentPath('images')); - - var pathToDelete = path.join(targetDir, fileName); - return remove(pathToDelete); -}; - -/** - * Reads bytes from disk for a target image - * path: path of target image (without content path!) - */ -LocalFileStore.prototype.read = function read(options) { - options = options || {}; - - // remove trailing slashes - options.path = (options.path || '').replace(/\/$|\\$/, ''); - - var targetPath = path.join(config.getContentPath('images'), options.path); - - return new Promise(function (resolve, reject) { - fs.readFile(targetPath, function (err, bytes) { - if (err) { - return reject(new errors.GhostError({ - err: err, - message: 'Could not read image: ' + targetPath - })); - } - - resolve(bytes); - }); - }); -}; - -module.exports = LocalFileStore; diff --git a/core/server/themes/Storage.js b/core/server/themes/Storage.js new file mode 100644 index 0000000000..596ff63e91 --- /dev/null +++ b/core/server/themes/Storage.js @@ -0,0 +1,68 @@ +// jscs:disable requireMultipleVarDecl + +'use strict'; + +var fs = require('fs-extra'), + os = require('os'), + path = require('path'), + Promise = require('bluebird'), + config = require('../config'), + utils = require('../utils'), + LocalFileStorage = require('../storage/LocalFileStorage'), + remove = Promise.promisify(fs.remove); + +/** + * @TODO: combine with loader.js? + */ +class ThemeStorage extends LocalFileStorage { + constructor() { + super(); + + this.storagePath = config.getContentPath('themes'); + } + + getTargetDir() { + return this.storagePath; + } + + serve(options) { + var self = this; + + return function downloadTheme(req, res, next) { + var themeName = options.name, + themePath = path.join(self.storagePath, themeName), + zipName = themeName + '.zip', + // store this in a unique temporary folder + zipBasePath = path.join(os.tmpdir(), utils.uid(10)), + zipPath = path.join(zipBasePath, zipName), + stream; + + Promise.promisify(fs.ensureDir)(zipBasePath) + .then(function () { + return Promise.promisify(utils.zipFolder)(themePath, zipPath); + }) + .then(function (length) { + res.set({ + 'Content-disposition': 'attachment; filename={themeName}.zip'.replace('{themeName}', themeName), + 'Content-Type': 'application/zip', + 'Content-Length': length + }); + + stream = fs.createReadStream(zipPath); + stream.pipe(res); + }) + .catch(function (err) { + next(err); + }) + .finally(function () { + remove(zipBasePath); + }); + }; + } + + delete(fileName) { + return remove(path.join(this.storagePath, fileName)); + } +} + +module.exports = ThemeStorage; diff --git a/core/server/themes/index.js b/core/server/themes/index.js index cb837f29b4..19d3851dc1 100644 --- a/core/server/themes/index.js +++ b/core/server/themes/index.js @@ -7,7 +7,9 @@ var debug = require('debug')('ghost:themes'), themeLoader = require('./loader'), active = require('./active'), validate = require('./validate'), - settingsCache = require('../settings/cache'); + Storage = require('./Storage'), + settingsCache = require('../settings/cache'), + themeStorage; // @TODO: reduce the amount of things we expose to the outside world // Make this a nice clean sensible API we can all understand! @@ -50,6 +52,11 @@ module.exports = { // Load themes, soon to be removed and exposed via specific function. loadAll: themeLoader.loadAllThemes, loadOne: themeLoader.loadOneTheme, + get storage() { + themeStorage = themeStorage || new Storage(); + + return themeStorage; + }, list: require('./list'), validate: validate, toJSON: require('./to-json'), diff --git a/core/test/unit/config/index_spec.js b/core/test/unit/config/index_spec.js index fd1246d305..3ee8306e56 100644 --- a/core/test/unit/config/index_spec.js +++ b/core/test/unit/config/index_spec.js @@ -118,10 +118,7 @@ describe('Config', function () { it('should default to local-file-store', function () { configUtils.config.get('paths').should.have.property('internalStoragePath', path.join(configUtils.config.get('paths').corePath, '/server/storage/')); - configUtils.config.get('storage').should.have.property('active', { - images: 'local-file-store', - themes: 'local-file-store' - }); + configUtils.config.get('storage').should.have.property('active', 'LocalFileStorage'); }); it('no effect: setting a custom active storage as string', function () { diff --git a/core/test/unit/importer_spec.js b/core/test/unit/importer_spec.js index c900529360..11a4b5dea6 100644 --- a/core/test/unit/importer_spec.js +++ b/core/test/unit/importer_spec.js @@ -370,9 +370,9 @@ describe('Importer', function () { ImageHandler.loadFile(_.clone(file)).then(function () { storageSpy.calledOnce.should.be.true(); storeSpy.calledOnce.should.be.true(); - storeSpy.firstCall.args[1].originalPath.should.equal('test-image.jpeg'); - storeSpy.firstCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); - storeSpy.firstCall.args[1].newPath.should.eql('/content/images/test-image.jpeg'); + storeSpy.firstCall.args[0].originalPath.should.equal('test-image.jpeg'); + storeSpy.firstCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); + storeSpy.firstCall.args[0].newPath.should.eql('/content/images/test-image.jpeg'); done(); }).catch(done); @@ -390,9 +390,9 @@ describe('Importer', function () { ImageHandler.loadFile(_.clone(file)).then(function () { storageSpy.calledOnce.should.be.true(); storeSpy.calledOnce.should.be.true(); - storeSpy.firstCall.args[1].originalPath.should.equal('photos/my-cat.jpeg'); - storeSpy.firstCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)photos$/); - storeSpy.firstCall.args[1].newPath.should.eql('/content/images/photos/my-cat.jpeg'); + storeSpy.firstCall.args[0].originalPath.should.equal('photos/my-cat.jpeg'); + storeSpy.firstCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)photos$/); + storeSpy.firstCall.args[0].newPath.should.eql('/content/images/photos/my-cat.jpeg'); done(); }).catch(done); @@ -410,9 +410,9 @@ describe('Importer', function () { ImageHandler.loadFile(_.clone(file)).then(function () { storageSpy.calledOnce.should.be.true(); storeSpy.calledOnce.should.be.true(); - storeSpy.firstCall.args[1].originalPath.should.equal('content/images/my-cat.jpeg'); - storeSpy.firstCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); - storeSpy.firstCall.args[1].newPath.should.eql('/content/images/my-cat.jpeg'); + storeSpy.firstCall.args[0].originalPath.should.equal('content/images/my-cat.jpeg'); + storeSpy.firstCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); + storeSpy.firstCall.args[0].newPath.should.eql('/content/images/my-cat.jpeg'); done(); }).catch(done); @@ -432,9 +432,9 @@ describe('Importer', function () { ImageHandler.loadFile(_.clone(file)).then(function () { storageSpy.calledOnce.should.be.true(); storeSpy.calledOnce.should.be.true(); - storeSpy.firstCall.args[1].originalPath.should.equal('test-image.jpeg'); - storeSpy.firstCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); - storeSpy.firstCall.args[1].newPath.should.eql('/subdir/content/images/test-image.jpeg'); + storeSpy.firstCall.args[0].originalPath.should.equal('test-image.jpeg'); + storeSpy.firstCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); + storeSpy.firstCall.args[0].newPath.should.eql('/subdir/content/images/test-image.jpeg'); done(); }).catch(done); @@ -463,18 +463,18 @@ describe('Importer', function () { ImageHandler.loadFile(_.clone(files)).then(function () { storageSpy.calledOnce.should.be.true(); storeSpy.callCount.should.eql(4); - storeSpy.firstCall.args[1].originalPath.should.equal('testing.png'); - storeSpy.firstCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); - storeSpy.firstCall.args[1].newPath.should.eql('/content/images/testing.png'); - storeSpy.secondCall.args[1].originalPath.should.equal('photo/kitten.jpg'); - storeSpy.secondCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)photo$/); - storeSpy.secondCall.args[1].newPath.should.eql('/content/images/photo/kitten.jpg'); - storeSpy.thirdCall.args[1].originalPath.should.equal('content/images/animated/bunny.gif'); - storeSpy.thirdCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)animated$/); - storeSpy.thirdCall.args[1].newPath.should.eql('/content/images/animated/bunny.gif'); - storeSpy.lastCall.args[1].originalPath.should.equal('images/puppy.jpg'); - storeSpy.lastCall.args[1].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); - storeSpy.lastCall.args[1].newPath.should.eql('/content/images/puppy.jpg'); + storeSpy.firstCall.args[0].originalPath.should.equal('testing.png'); + storeSpy.firstCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); + storeSpy.firstCall.args[0].newPath.should.eql('/content/images/testing.png'); + storeSpy.secondCall.args[0].originalPath.should.equal('photo/kitten.jpg'); + storeSpy.secondCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)photo$/); + storeSpy.secondCall.args[0].newPath.should.eql('/content/images/photo/kitten.jpg'); + storeSpy.thirdCall.args[0].originalPath.should.equal('content/images/animated/bunny.gif'); + storeSpy.thirdCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images(\/|\\)animated$/); + storeSpy.thirdCall.args[0].newPath.should.eql('/content/images/animated/bunny.gif'); + storeSpy.lastCall.args[0].originalPath.should.equal('images/puppy.jpg'); + storeSpy.lastCall.args[0].targetDir.should.match(/(\/|\\)content(\/|\\)images$/); + storeSpy.lastCall.args[0].newPath.should.eql('/content/images/puppy.jpg'); done(); }).catch(done); diff --git a/core/test/unit/middleware/serve-favicon_spec.js b/core/test/unit/middleware/serve-favicon_spec.js index 67eef2c404..d2ec0f77ec 100644 --- a/core/test/unit/middleware/serve-favicon_spec.js +++ b/core/test/unit/middleware/serve-favicon_spec.js @@ -3,13 +3,14 @@ var should = require('should'), // jshint ignore:line express = require('express'), serveFavicon = require('../../../server/middleware/serve-favicon'), settingsCache = require('../../../server/settings/cache'), + storage = require('../../../server/storage'), configUtils = require('../../utils/configUtils'), path = require('path'), sandbox = sinon.sandbox.create(); describe('Serve Favicon', function () { - var req, res, next, blogApp, localSettingsCache = {}; + var req, res, next, blogApp, localSettingsCache = {}, originalStoragePath; beforeEach(function () { req = sinon.spy(); @@ -21,12 +22,15 @@ describe('Serve Favicon', function () { sandbox.stub(settingsCache, 'get', function (key) { return localSettingsCache[key]; }); + + originalStoragePath = storage.getStorage().storagePath; }); afterEach(function () { sandbox.restore(); configUtils.restore(); localSettingsCache = {}; + storage.getStorage().storagePath = originalStoragePath; }); describe('serveFavicon', function () { @@ -48,7 +52,7 @@ describe('Serve Favicon', function () { var middleware = serveFavicon(); req.path = '/favicon.png'; - configUtils.set('paths:contentPath', path.join(__dirname, '../../../test/utils/fixtures/')); + storage.getStorage().storagePath = path.join(__dirname, '../../../test/utils/fixtures/images/'); localSettingsCache.icon = 'favicon.png'; res = { @@ -68,7 +72,7 @@ describe('Serve Favicon', function () { var middleware = serveFavicon(); req.path = '/favicon.ico'; - configUtils.set('paths:contentPath', path.join(__dirname, '../../../test/utils/fixtures/')); + storage.getStorage().storagePath = path.join(__dirname, '../../../test/utils/fixtures/images/'); localSettingsCache.icon = 'favicon.ico'; res = { diff --git a/core/test/unit/storage/local-file-store_spec.js b/core/test/unit/storage/LocalFileStorage_spec.js similarity index 98% rename from core/test/unit/storage/local-file-store_spec.js rename to core/test/unit/storage/LocalFileStorage_spec.js index 53f1874f55..1526197330 100644 --- a/core/test/unit/storage/local-file-store_spec.js +++ b/core/test/unit/storage/LocalFileStorage_spec.js @@ -4,7 +4,7 @@ var should = require('should'), // jshint ignore:line moment = require('moment'), path = require('path'), errors = require('../../../server/errors'), - LocalFileStore = require('../../../server/storage/local-file-store'), + LocalFileStore = require('../../../server/storage/LocalFileStorage'), localFileStore, configUtils = require('../../utils/configUtils'), @@ -150,7 +150,7 @@ describe('Local File System Storage', function () { describe('read image', function () { beforeEach(function () { // we have some example images in our test utils folder - configUtils.set('paths:contentPath', path.join(__dirname, '../../utils/fixtures')); + localFileStore.storagePath = path.join(__dirname, '../../utils/fixtures/images/'); }); it('success', function (done) { diff --git a/core/test/unit/storage/base_spec.js b/core/test/unit/storage/base_spec.js deleted file mode 100644 index a5b950bc25..0000000000 --- a/core/test/unit/storage/base_spec.js +++ /dev/null @@ -1,9 +0,0 @@ -var should = require('should'), // jshint ignore:line - storage = require('../../../server/storage'); - -describe('storage: base_spec', function () { - it('escape non accepted characters in filenames', function () { - var chosenStorage = storage.getStorage('themes'); - chosenStorage.getSanitizedFileName('(abc*@#123).zip').should.eql('-abc-@-123-.zip'); - }); -}); diff --git a/core/test/unit/storage/index_spec.js b/core/test/unit/storage/index_spec.js index 075c54f91d..eb08ae06e6 100644 --- a/core/test/unit/storage/index_spec.js +++ b/core/test/unit/storage/index_spec.js @@ -1,9 +1,10 @@ var should = require('should'), // jshint ignore:line fs = require('fs-extra'), + StorageBase = require('ghost-storage-base'), configUtils = require('../../utils/configUtils'), storage = require('../../../server/storage'), errors = require('../../../server/errors'), - localFileStorage = require('../../../server/storage/local-file-store'); + LocalFileStorage = require('../../../server/storage/LocalFileStorage'); describe('storage: index_spec', function () { var scope = {adapter: null}; @@ -23,124 +24,71 @@ describe('storage: index_spec', function () { configUtils.restore(); }); - describe('default ghost storage config', function () { - it('load without a type', function () { - var chosenStorage = storage.getStorage(); - (chosenStorage instanceof localFileStorage).should.eql(true); - }); - - it('load with themes type', function () { - var chosenStorage = storage.getStorage('themes'); - (chosenStorage instanceof localFileStorage).should.eql(true); - }); - - it('load with unknown type', function () { - try { - storage.getStorage('theme'); - } catch (err) { - (err instanceof errors.IncorrectUsageError).should.eql(true); - } - }); + it('default image storage is local file storage', function () { + var chosenStorage = storage.getStorage(); + (chosenStorage instanceof StorageBase).should.eql(true); + (chosenStorage instanceof LocalFileStorage).should.eql(true); }); - describe('custom ghost storage config', function () { - it('images storage adapter is custom, themes is default', function () { - scope.adapter = configUtils.config.getContentPath('storage') + 'custom-adapter.js'; + it('custom adapter', function () { + scope.adapter = configUtils.config.getContentPath('storage') + 'custom-adapter.js'; - configUtils.set({ - storage: { - active: { - images: 'custom-adapter' - } - } - }); - - var jsFile = '' + - 'var util = require(\'util\');' + - 'var StorageBase = require(__dirname + \'/../../core/server/storage/base\');' + - 'var AnotherAdapter = function (){ StorageBase.call(this); };' + - 'util.inherits(AnotherAdapter, StorageBase);' + - 'AnotherAdapter.prototype.exists = function (){};' + - 'AnotherAdapter.prototype.save = function (){};' + - 'AnotherAdapter.prototype.serve = function (){};' + - 'AnotherAdapter.prototype.delete = function (){};' + - 'AnotherAdapter.prototype.read = function (){};' + - 'module.exports = AnotherAdapter', chosenStorage; - - fs.writeFileSync(scope.adapter, jsFile); - - chosenStorage = storage.getStorage('themes'); - (chosenStorage instanceof localFileStorage).should.eql(true); - - chosenStorage = storage.getStorage('images'); - (chosenStorage instanceof localFileStorage).should.eql(false); - }); - }); - - describe('adapter validation', function () { - it('create good adapter', function () { - scope.adapter = configUtils.config.getContentPath('storage') + 'another-storage.js'; - - configUtils.set({ - storage: { - active: { - images: 'another-storage' - } - }, - paths: { - storage: __dirname + '/another-storage.js' - } - }); - - var jsFile = '' + - 'var util = require(\'util\');' + - 'var StorageBase = require(__dirname + \'/../../core/server/storage/base\');' + - 'var AnotherAdapter = function (){ StorageBase.call(this); };' + - 'util.inherits(AnotherAdapter, StorageBase);' + - 'AnotherAdapter.prototype.exists = function (){};' + - 'AnotherAdapter.prototype.save = function (){};' + - 'AnotherAdapter.prototype.serve = function (){};' + - 'AnotherAdapter.prototype.delete = function (){};' + - 'AnotherAdapter.prototype.read = function (){};' + - 'module.exports = AnotherAdapter', adapter; - - fs.writeFileSync(scope.adapter, jsFile); - - adapter = storage.getStorage(); - should.exist(adapter); - (adapter instanceof localFileStorage).should.eql(false); - }); - - it('create bad adapter: exists fn is missing', function () { - scope.adapter = configUtils.config.getContentPath('storage') + 'broken-storage.js'; - - configUtils.set({ - storage: { - active: 'broken-storage' - }, - paths: { - storage: __dirname + '/broken-storage.js' - } - }); - - var jsFile = '' + - 'var util = require(\'util\');' + - 'var StorageBase = require(__dirname + \'/../../core/server/storage/base\');' + - 'var AnotherAdapter = function (){ StorageBase.call(this); };' + - 'util.inherits(AnotherAdapter, StorageBase);' + - 'AnotherAdapter.prototype.save = function (){};' + - 'AnotherAdapter.prototype.serve = function (){};' + - 'AnotherAdapter.prototype.delete = function (){};' + - 'module.exports = AnotherAdapter'; - - fs.writeFileSync(scope.adapter, jsFile); - - try { - storage.getStorage(); - } catch (err) { - should.exist(err); - (err instanceof errors.IncorrectUsageError).should.eql(true); + configUtils.set({ + storage: { + active: 'custom-adapter' } }); + + var jsFile = '' + + '\'use strict\';' + + 'var StorageBase = require(\'ghost-storage-base\');' + + 'class AnotherAdapter extends StorageBase {' + + 'exists(){}' + + 'save(){}' + + 'serve(){}' + + 'delete(){}' + + 'read(){}' + + '}' + + 'module.exports = AnotherAdapter', chosenStorage; + + fs.writeFileSync(scope.adapter, jsFile); + + configUtils.config.get('storage:active').should.eql('custom-adapter'); + chosenStorage = storage.getStorage(); + (chosenStorage instanceof LocalFileStorage).should.eql(false); + (chosenStorage instanceof StorageBase).should.eql(true); + }); + + it('create bad adapter: exists fn is missing', function () { + scope.adapter = configUtils.config.getContentPath('storage') + 'broken-storage.js'; + + configUtils.set({ + storage: { + active: 'broken-storage' + }, + paths: { + storage: __dirname + '/broken-storage.js' + } + }); + + var jsFile = '' + + '\'use strict\';' + + 'var StorageBase = require(\'ghost-storage-base\');' + + 'class AnotherAdapter extends StorageBase {' + + 'save(){}' + + 'serve(){}' + + 'delete(){}' + + 'read(){}' + + '}' + + 'module.exports = AnotherAdapter'; + + fs.writeFileSync(scope.adapter, jsFile); + + try { + storage.getStorage(); + } catch (err) { + should.exist(err); + (err instanceof errors.IncorrectUsageError).should.eql(true); + } }); }); diff --git a/package.json b/package.json index 35285e52d6..d261e01ca7 100644 --- a/package.json +++ b/package.json @@ -50,6 +50,7 @@ "fs-extra": "2.1.2", "ghost-gql": "0.0.6", "ghost-ignition": "2.8.10", + "ghost-storage-base": "0.0.1", "glob": "5.0.15", "gscan": "0.2.1", "html-to-text": "3.2.0", diff --git a/yarn.lock b/yarn.lock index 0bede5c84d..4e82a8b0aa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1578,6 +1578,12 @@ ghost-ignition@2.8.10, ghost-ignition@^2.8.2, ghost-ignition@^2.8.7: prettyjson "1.1.3" uuid "^3.0.0" +ghost-storage-base@0.0.1: + version "0.0.1" + resolved "https://registry.yarnpkg.com/ghost-storage-base/-/ghost-storage-base-0.0.1.tgz#b31b57d2e54574a96153a54bf2e9ea599f12bec8" + dependencies: + moment "^2.17.1" + glob-base@^0.3.0: version "0.3.0" resolved "https://registry.yarnpkg.com/glob-base/-/glob-base-0.3.0.tgz#dbb164f6221b1c0b1ccf82aea328b497df0ea3c4" @@ -3196,7 +3202,7 @@ moment-timezone@0.5.13: dependencies: moment ">= 2.9.0" -moment@2.18.1, "moment@>= 2.9.0", moment@^2.10.6, moment@^2.15.2: +moment@2.18.1, "moment@>= 2.9.0", moment@^2.10.6, moment@^2.15.2, moment@^2.17.1: version "2.18.1" resolved "https://registry.yarnpkg.com/moment/-/moment-2.18.1.tgz#c36193dd3ce1c2eed2adb7c802dbbc77a81b1c0f"