From 6a1c10516e85512c8b5611f58d5cb520294d8edf Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Mon, 22 Aug 2016 23:51:42 +0200 Subject: [PATCH] improvement: ensure custom storage adapter has required functions (#7234) refs #2852 - improvement: ensure custom storage adapter has required functions - serve, save and exists are from now on required functions for a custom storage adapter - add delete as required storage function --- core/server/storage/base.js | 4 + core/server/storage/index.js | 16 +++- core/server/storage/local-file-store.js | 9 +- core/test/unit/storage/index_spec.js | 91 +++++++++++++++++-- .../local-file-store_spec.js} | 4 +- 5 files changed, 112 insertions(+), 12 deletions(-) rename core/test/unit/{storage_local-file-store_spec.js => storage/local-file-store_spec.js} (98%) diff --git a/core/server/storage/base.js b/core/server/storage/base.js index fd76049a5c..a8359b7e3d 100644 --- a/core/server/storage/base.js +++ b/core/server/storage/base.js @@ -2,6 +2,10 @@ var moment = require('moment'), path = require('path'); function StorageBase() { + Object.defineProperty(this, 'requiredFns', { + value: ['exists', 'save', 'serve', 'delete'], + writable: false + }); } StorageBase.prototype.getTargetDir = function (baseDir) { diff --git a/core/server/storage/index.js b/core/server/storage/index.js index bcdcc6c293..cebdf65004 100644 --- a/core/server/storage/index.js +++ b/core/server/storage/index.js @@ -1,5 +1,7 @@ var errors = require('../errors'), config = require('../config'), + Base = require('./base'), + _ = require('lodash'), storage = {}; /** @@ -33,10 +35,20 @@ function getStorage(type) { } } - // TODO: determine if storage has all the necessary methods. - // Instantiate and cache the storage module instance. storage[storageChoice] = new storage[storageChoice](storageConfig); + if (!(storage[storageChoice] instanceof Base)) { + throw new errors.IncorrectUsage('Your storage adapter does not inherit from the Storage Base.'); + } + + if (!storage[storageChoice].requiredFns) { + throw new errors.IncorrectUsage('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.IncorrectUsage('Your storage adapter does not provide the minimum required functions.'); + } + return storage[storageChoice]; } diff --git a/core/server/storage/local-file-store.js b/core/server/storage/local-file-store.js index 39f9436a27..dc9fe8e4d8 100644 --- a/core/server/storage/local-file-store.js +++ b/core/server/storage/local-file-store.js @@ -9,11 +9,12 @@ var serveStatic = require('express').static, errors = require('../errors'), config = require('../config'), utils = require('../utils'), - baseStore = require('./base'); + BaseStore = require('./base'); function LocalFileStore() { + BaseStore.call(this); } -util.inherits(LocalFileStore, baseStore); +util.inherits(LocalFileStore, BaseStore); // ### Save // Saves the image to storage (the file system) @@ -56,4 +57,8 @@ LocalFileStore.prototype.serve = function () { return serveStatic(config.paths.imagesPath, {maxAge: utils.ONE_YEAR_MS, fallthrough: false}); }; +LocalFileStore.prototype.delete = function () { + return Promise.reject('not implemented'); +}; + module.exports = LocalFileStore; diff --git a/core/test/unit/storage/index_spec.js b/core/test/unit/storage/index_spec.js index be0d16cf87..769a4eb200 100644 --- a/core/test/unit/storage/index_spec.js +++ b/core/test/unit/storage/index_spec.js @@ -9,6 +9,23 @@ var fs = require('fs-extra'), should.equal(true, true); describe('storage: index_spec', function () { + var scope = {adapter: null}; + + before(function () { + if (!fs.existsSync(configUtils.config.paths.storagePath.custom)) { + fs.mkdirSync(configUtils.config.paths.storagePath.custom); + } + }); + + afterEach(function () { + if (scope.adapter) { + fs.unlinkSync(scope.adapter); + scope.adapter = null; + } + + configUtils.restore(); + }); + describe('default ghost storage config', function () { it('load without a type', function () { var chosenStorage = storage.getStorage(); @@ -31,6 +48,8 @@ describe('storage: index_spec', function () { describe('custom ghost storage config', function () { it('images storage adapter is custom, themes is default', function () { + scope.adapter = configUtils.config.paths.storagePath.custom + 'custom-adapter.js'; + configUtils.set({ storage: { active: { @@ -46,21 +65,81 @@ describe('storage: index_spec', function () { 'util.inherits(AnotherAdapter, StorageBase);' + 'AnotherAdapter.prototype.exists = function (){};' + 'AnotherAdapter.prototype.save = function (){};' + + 'AnotherAdapter.prototype.serve = function (){};' + + 'AnotherAdapter.prototype.delete = function (){};' + 'module.exports = AnotherAdapter', chosenStorage; - if (!fs.existsSync(configUtils.config.paths.storagePath.custom)) { - fs.mkdirSync(configUtils.config.paths.storagePath.custom); - } - - fs.writeFileSync(configUtils.config.paths.storagePath.custom + 'custom-adapter.js', jsFile); + 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); + }); + }); - fs.unlinkSync(configUtils.config.paths.storagePath.custom + 'custom-adapter.js'); + describe('adapter validation', function () { + it('create good adapter', function () { + scope.adapter = configUtils.config.paths.storagePath.custom + 'another-storage.js'; + + configUtils.set({ + storage: { + active: '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 (){};' + + '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.paths.storagePath.custom + '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.IncorrectUsage).should.eql(true); + } }); }); }); diff --git a/core/test/unit/storage_local-file-store_spec.js b/core/test/unit/storage/local-file-store_spec.js similarity index 98% rename from core/test/unit/storage_local-file-store_spec.js rename to core/test/unit/storage/local-file-store_spec.js index d3d0411251..88829ad9fd 100644 --- a/core/test/unit/storage_local-file-store_spec.js +++ b/core/test/unit/storage/local-file-store_spec.js @@ -4,10 +4,10 @@ var fs = require('fs-extra'), should = require('should'), sinon = require('sinon'), _ = require('lodash'), - LocalFileStore = require('../../server/storage/local-file-store'), + LocalFileStore = require('../../../server/storage/local-file-store'), localFileStore, - configUtils = require('../utils/configUtils'); + configUtils = require('../../utils/configUtils'); // To stop jshint complaining should.equal(true, true);