From 66845def852d87a6cce9582b7684d234ce81d74b Mon Sep 17 00:00:00 2001 From: Harry Wolff Date: Thu, 28 Aug 2014 23:48:49 -0400 Subject: [PATCH] Moves storage module to use prototypes for inheritance and structure. addresses #2852 - Moves storage modules to use prototypes and to create prototypes that inherit from the base storage ctor. - Makes storage/base conform to an all Promise interface. --- core/server/api/upload.js | 2 +- core/server/middleware/index.js | 2 +- core/server/storage/base.js | 86 +++++++++---------- core/server/storage/index.js | 26 +++--- core/server/storage/localfilesystem.js | 83 +++++++++--------- core/test/integration/api/api_upload_spec.js | 4 +- .../test/unit/storage_localfilesystem_spec.js | 11 ++- 7 files changed, 109 insertions(+), 105 deletions(-) diff --git a/core/server/api/upload.js b/core/server/api/upload.js index 5429fe266f..b746bbbd61 100644 --- a/core/server/api/upload.js +++ b/core/server/api/upload.js @@ -29,7 +29,7 @@ upload = { * @returns {Promise} Success */ add: function (options) { - var store = storage.get_storage(), + var store = storage.getStorage(), type, ext, filepath; diff --git a/core/server/middleware/index.js b/core/server/middleware/index.js index 2a2d3dc780..bc1a59b3d8 100644 --- a/core/server/middleware/index.js +++ b/core/server/middleware/index.js @@ -278,7 +278,7 @@ setupMiddleware = function (server) { // Static assets expressServer.use('/shared', express['static'](path.join(corePath, '/shared'), {maxAge: utils.ONE_HOUR_MS})); - expressServer.use('/content/images', storage.get_storage().serve()); + expressServer.use('/content/images', storage.getStorage().serve()); expressServer.use('/ghost/scripts', express['static'](path.join(corePath, '/built/scripts'), {maxAge: utils.ONE_YEAR_MS})); expressServer.use('/public', express['static'](path.join(corePath, '/built/public'), {maxAge: utils.ONE_YEAR_MS})); diff --git a/core/server/storage/base.js b/core/server/storage/base.js index 1f224ca31a..bd5cc922e1 100644 --- a/core/server/storage/base.js +++ b/core/server/storage/base.js @@ -1,52 +1,48 @@ var moment = require('moment'), - path = require('path'), - Promise = require('bluebird'), - baseStore; + path = require('path'); -// TODO: would probably be better to put these on the prototype and have proper constructors etc -baseStore = { - 'getTargetDir': function (baseDir) { - var m = moment(new Date().getTime()), - month = m.format('MMM'), - year = m.format('YYYY'); +function StorageBase() { +} - if (baseDir) { - return path.join(baseDir, year, month); - } +StorageBase.prototype.getTargetDir = function (baseDir) { + var m = moment(new Date().getTime()), + month = m.format('MMM'), + year = m.format('YYYY'); - return path.join(year, month); - }, - 'generateUnique': function (store, dir, name, ext, i, done) { - var self = this, - filename, - append = ''; - - if (i) { - append = '-' + i; - } - - filename = path.join(dir, name + append + ext); - - store.exists(filename).then(function (exists) { - if (exists) { - setImmediate(function () { - i = i + 1; - self.generateUnique(store, dir, name, ext, i, done); - }); - } else { - done(filename); - } - }); - }, - 'getUniqueFileName': function (store, image, targetDir) { - var ext = path.extname(image.name), - name = path.basename(image.name, ext).replace(/[\W]/gi, '-'), - self = this; - - return new Promise(function (resolve) { - self.generateUnique(store, targetDir, name, ext, 0, resolve); - }); + if (baseDir) { + return path.join(baseDir, year, month); } + + return path.join(year, month); }; -module.exports = baseStore; +StorageBase.prototype.generateUnique = function (store, dir, name, ext, i) { + var self = this, + filename, + append = ''; + + if (i) { + append = '-' + i; + } + + filename = path.join(dir, name + append + ext); + + 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 = path.basename(image.name, ext).replace(/[\W]/gi, '-'), + self = this; + + return self.generateUnique(store, targetDir, name, ext, 0); +}; + +module.exports = StorageBase; diff --git a/core/server/storage/index.js b/core/server/storage/index.js index e585264828..ea46ded97c 100644 --- a/core/server/storage/index.js +++ b/core/server/storage/index.js @@ -1,22 +1,26 @@ -var errors = require('../errors'), - storage; +var errors = require('../errors'), + storage = {}; -function get_storage() { +function getStorage(storageChoice) { // TODO: this is where the check for storage apps should go - // Local file system is the default - var storageChoice = 'localfilesystem'; + // Local file system is the default. Fow now that is all we support. + storageChoice = 'localfilesystem'; - if (storage) { - return storage; + if (storage[storageChoice]) { + return storage[storageChoice]; } try { - // TODO: determine if storage has all the necessary methods - storage = require('./' + storageChoice); + // TODO: determine if storage has all the necessary methods. + storage[storageChoice] = require('./' + storageChoice); } catch (e) { errors.logError(e); } - return storage; + + // Instantiate and cache the storage module instance. + storage[storageChoice] = new storage[storageChoice](); + + return storage[storageChoice]; } -module.exports.get_storage = get_storage; \ No newline at end of file +module.exports.getStorage = getStorage; \ No newline at end of file diff --git a/core/server/storage/localfilesystem.js b/core/server/storage/localfilesystem.js index 300bf0d75b..298ac1f506 100644 --- a/core/server/storage/localfilesystem.js +++ b/core/server/storage/localfilesystem.js @@ -1,56 +1,57 @@ // # Local File System Image Storage module // The (default) module for storing images, using the local file system -var _ = require('lodash'), - express = require('express'), +var express = require('express'), fs = require('fs-extra'), path = require('path'), + util = require('util'), Promise = require('bluebird'), errors = require('../errors'), config = require('../config'), utils = require('../utils'), - baseStore = require('./base'), + baseStore = require('./base'); - localFileStore; +function LocalFileStore() { +} +util.inherits(LocalFileStore, baseStore); -localFileStore = _.extend(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 - 'save': function (image) { - var targetDir = this.getTargetDir(config.paths.imagesPath), - targetFilename; +// ### 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 (image) { + var targetDir = this.getTargetDir(config.paths.imagesPath), + 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 = (config.paths.subdir + '/' + config.paths.imagesRelPath + '/' + path.relative(config.paths.imagesPath, targetFilename)).replace(new RegExp('\\' + path.sep, 'g'), '/'); - return fullUrl; - }).catch(function (e) { - errors.logError(e); - return Promise.reject(e); + 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 = (config.paths.subdir + '/' + config.paths.imagesRelPath + '/' + + path.relative(config.paths.imagesPath, targetFilename)).replace(new RegExp('\\' + path.sep, 'g'), '/'); + return fullUrl; + }).catch(function (e) { + errors.logError(e); + return Promise.reject(e); + }); +}; + +LocalFileStore.prototype.exists = function (filename) { + return new Promise(function (resolve) { + fs.exists(filename, function (exists) { + resolve(exists); }); - }, + }); +}; - 'exists': function (filename) { - return new Promise(function (resolve) { - fs.exists(filename, function (exists) { - resolve(exists); - }); - }); - }, +// middleware for serving the files +LocalFileStore.prototype.serve = function () { + // For some reason send divides the max age number by 1000 + return express['static'](config.paths.imagesPath, {maxAge: utils.ONE_YEAR_MS}); +}; - // middleware for serving the files - 'serve': function () { - // For some reason send divides the max age number by 1000 - return express['static'](config.paths.imagesPath, {maxAge: utils.ONE_YEAR_MS}); - } -}); - -module.exports = localFileStore; +module.exports = LocalFileStore; diff --git a/core/test/integration/api/api_upload_spec.js b/core/test/integration/api/api_upload_spec.js index 736275dcd2..4eb5260d82 100644 --- a/core/test/integration/api/api_upload_spec.js +++ b/core/test/integration/api/api_upload_spec.js @@ -17,7 +17,7 @@ describe('Upload API', function () { // Doesn't test the DB afterEach(function () { - storage.get_storage.restore(); + storage.getStorage.restore(); fs.unlink.restore(); }); @@ -26,7 +26,7 @@ describe('Upload API', function () { store.save = sinon.stub().returns(Promise.resolve('URL')); store.exists = sinon.stub().returns(Promise.resolve(true)); store.destroy = sinon.stub().returns(Promise.resolve()); - sinon.stub(storage, 'get_storage').returns(store); + sinon.stub(storage, 'getStorage').returns(store); sinon.stub(fs, 'unlink').yields(); }); diff --git a/core/test/unit/storage_localfilesystem_spec.js b/core/test/unit/storage_localfilesystem_spec.js index 7be4d8d475..34899f17bf 100644 --- a/core/test/unit/storage_localfilesystem_spec.js +++ b/core/test/unit/storage_localfilesystem_spec.js @@ -7,7 +7,8 @@ var fs = require('fs-extra'), rewire = require('rewire'), _ = require('lodash'), config = rewire('../../server/config'), - localfilesystem = rewire('../../server/storage/localfilesystem'); + LocalFileStore = rewire('../../server/storage/localfilesystem'), + localfilesystem; // To stop jshint complaining should.equal(true, true); @@ -16,10 +17,10 @@ describe('Local File System Storage', function () { var image, overrideConfig = function (newConfig) { - var existingConfig = localfilesystem.__get__('config'), + var existingConfig = LocalFileStore.__get__('config'), updatedConfig = _.extend(existingConfig, newConfig); config.set(updatedConfig); - localfilesystem.__set__('config', updatedConfig); + LocalFileStore.__set__('config', updatedConfig); }; beforeEach(function () { @@ -38,6 +39,8 @@ describe('Local File System Storage', function () { // Sat Sep 07 2013 21:24 this.clock = sinon.useFakeTimers(new Date(2013, 8, 7, 21, 24).getTime()); + + localfilesystem = new LocalFileStore(); }); afterEach(function () { @@ -181,4 +184,4 @@ describe('Local File System Storage', function () { }).catch(done); }); }); -}); +}); \ No newline at end of file