From 15da975c0643fac12219694d2463d713a067badc Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Thu, 7 Nov 2013 16:00:39 +0000 Subject: [PATCH] image upload controller refactor issue #635 - upload controller shouldn't assume fs - filesystem module proxies all the fs work - proxies and exposes middleware for serving images - creating a date based path and unique filename is a base object util - unit tests updated --- core/server.js | 5 +- core/server/controllers/admin.js | 26 ++----- .../controllers/storage/localfilesystem.js | 72 ------------------- core/server/storage/base.js | 53 ++++++++++++++ core/server/storage/index.js | 22 ++++++ core/server/storage/localfilesystem.js | 62 ++++++++++++++++ core/test/unit/admin_spec.js | 33 ++++----- .../test/unit/storage_localfilesystem_spec.js | 51 +++++++------ 8 files changed, 187 insertions(+), 137 deletions(-) delete mode 100644 core/server/controllers/storage/localfilesystem.js create mode 100644 core/server/storage/base.js create mode 100644 core/server/storage/index.js create mode 100644 core/server/storage/localfilesystem.js diff --git a/core/server.js b/core/server.js index c1c0bd4de8..3ae4146c6b 100644 --- a/core/server.js +++ b/core/server.js @@ -15,6 +15,7 @@ var express = require('express'), Ghost = require('./ghost'), helpers = require('./server/helpers'), middleware = require('./server/middleware'), + storage = require('./server/storage'), packageInfo = require('../package.json'), // Variables @@ -177,7 +178,9 @@ when(ghost.init()).then(function () { express['static'].mime.define({'application/font-woff': ['woff']}); // Shared static config server.use('/shared', express['static'](path.join(__dirname, '/shared'))); - server.use('/content/images', express['static'](path.join(__dirname, '/../content/images'))); + + server.use('/content/images', storage.get_storage().serve()); + // Serve our built scripts; can't use /scripts here because themes already are server.use('/built/scripts', express['static'](path.join(__dirname, '/built/scripts'), { // Put a maxAge of one year on built scripts diff --git a/core/server/controllers/admin.js b/core/server/controllers/admin.js index 65af7a77d3..587f90090a 100644 --- a/core/server/controllers/admin.js +++ b/core/server/controllers/admin.js @@ -1,10 +1,9 @@ var Ghost = require('../../ghost'), _ = require('underscore'), - fs = require('fs-extra'), path = require('path'), api = require('../api'), - moment = require('moment'), errors = require('../errorHandling'), + storage = require('../storage'), ghost = new Ghost(), dataProvider = ghost.dataProvider, @@ -44,35 +43,20 @@ function setSelected(list, name) { } adminControllers = { - 'get_storage': function () { - // TODO this is where the check for storage plugins should go - // Local file system is the default - var storageChoice = 'localfilesystem.js'; - return require('./storage/' + storageChoice); - }, 'uploader': function (req, res) { var type = req.files.uploadimage.type, ext = path.extname(req.files.uploadimage.name).toLowerCase(), - storage = adminControllers.get_storage(); + store = storage.get_storage(); if ((type !== 'image/jpeg' && type !== 'image/png' && type !== 'image/gif') || (ext !== '.jpg' && ext !== '.jpeg' && ext !== '.png' && ext !== '.gif')) { return res.send(415, 'Unsupported Media Type'); } - storage - .save(new Date().getTime(), req.files.uploadimage) + store + .save(req.files.uploadimage) .then(function (url) { - - // delete the temporary file - // TODO convert to promise using nodefn - fs.unlink(req.files.uploadimage.path, function (e) { - if (e) { - return errors.logError(e); - } - - return res.send(url); - }); + return res.send(url); }) .otherwise(function (e) { return errors.logError(e); diff --git a/core/server/controllers/storage/localfilesystem.js b/core/server/controllers/storage/localfilesystem.js deleted file mode 100644 index fd9cc18d9c..0000000000 --- a/core/server/controllers/storage/localfilesystem.js +++ /dev/null @@ -1,72 +0,0 @@ -// # Local File System Image Storage module -// The (default) module for storing images, using the local file system - -var errors = require('../../errorHandling'), - fs = require('fs-extra'), - moment = require('moment'), - nodefn = require('when/node/function'), - path = require('path'), - when = require('when'); - -var localfilesystem; - -// TODO: this could be a separate module -function getUniqueFileName(dir, name, ext, i, done) { - var filename, - append = ''; - - if (i) { - append = '-' + i; - } - - filename = path.join(dir, name + append + ext); - fs.exists(filename, function (exists) { - if (exists) { - setImmediate(function () { - i = i + 1; - return getUniqueFileName(dir, name, ext, i, done); - }); - } else { - return done(filename); - } - }); -} - -// ## Module interface -localfilesystem = { - - // ### Save - // Saves the image to storage (the file system) - // - date is current date in ticks - // - image is the express image object - // - returns a promise which ultimately returns the full url to the uploaded image - 'save': function (date, image) { - - // QUESTION is it okay for this module to know about content/images? - var saved = when.defer(), - m = moment(date), - month = m.format('MMM'), - year = m.format('YYYY'), - target_dir = path.join('content/images', year, month), - ext = path.extname(image.name), - basename = path.basename(image.name, ext).replace(/[\W]/gi, '_'); - - getUniqueFileName(target_dir, basename, ext, null, function (filename) { - nodefn.call(fs.mkdirs, target_dir).then(function () { - return nodefn.call(fs.copy, image.path, filename); - }).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 = ('/' + filename).replace(new RegExp('\\' + path.sep, 'g'), '/'); - return saved.resolve(fullUrl); - }).otherwise(function (e) { - errors.logError(e); - return saved.reject(e); - }); - }); - - return saved.promise; - } -}; - -module.exports = localfilesystem; diff --git a/core/server/storage/base.js b/core/server/storage/base.js new file mode 100644 index 0000000000..8961679371 --- /dev/null +++ b/core/server/storage/base.js @@ -0,0 +1,53 @@ +var _ = require('underscore'), + moment = require('moment'), + path = require('path'), + when = require('when'), + baseStore; + +// 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'); + + if (baseDir) { + return path.join(baseDir, year, month); + } + + 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.resolve(filename); + } + }); + }, + 'getUniqueFileName': function (store, image, targetDir) { + var done = when.defer(), + ext = path.extname(image.name), + name = path.basename(image.name, ext).replace(/[\W]/gi, '_'); + + this.generateUnique(store, targetDir, name, ext, 0, done); + + return done.promise; + } +}; + +module.exports = baseStore; \ No newline at end of file diff --git a/core/server/storage/index.js b/core/server/storage/index.js new file mode 100644 index 0000000000..f976548222 --- /dev/null +++ b/core/server/storage/index.js @@ -0,0 +1,22 @@ +var errors = require('../errorHandling'), + storage; + +function get_storage() { + // TODO: this is where the check for storage plugins should go + // Local file system is the default + var storageChoice = 'localfilesystem'; + + if (storage) { + return storage; + } + + try { + // TODO: determine if storage has all the necessary methods + storage = require('./' + storageChoice); + } catch (e) { + errors.logError(e); + } + return storage; +} + +module.exports.get_storage = get_storage; \ No newline at end of file diff --git a/core/server/storage/localfilesystem.js b/core/server/storage/localfilesystem.js new file mode 100644 index 0000000000..9b5bfd6521 --- /dev/null +++ b/core/server/storage/localfilesystem.js @@ -0,0 +1,62 @@ +// # Local File System Image Storage module +// The (default) module for storing images, using the local file system + +var _ = require('underscore'), + express = require('express'), + fs = require('fs-extra'), + nodefn = require('when/node/function'), + path = require('path'), + when = require('when'), + errors = require('../errorHandling'), + baseStore = require('./base'), + + localFileStore, + localDir = 'content/images'; + +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 saved = when.defer(), + targetDir = this.getTargetDir(localDir); + + this.getUniqueFileName(this, image, targetDir).then(function (filename) { + nodefn.call(fs.mkdirs, targetDir).then(function () { + return nodefn.call(fs.copy, image.path, filename); + }).then(function () { + // we should remove the temporary image + return nodefn.call(fs.unlink, image.path).otherwise(errors.logError); + }).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 = ('/' + filename).replace(new RegExp('\\' + path.sep, 'g'), '/'); + return saved.resolve(fullUrl); + }).otherwise(function (e) { + errors.logError(e); + return saved.reject(e); + }); + }).otherwise(errors.logError); + + return saved.promise; + }, + + 'exists': function (filename) { + // fs.exists does not play nicely with nodefn because the callback doesn't have an error argument + var done = when.defer(); + + fs.exists(filename, function (exists) { + done.resolve(exists); + }); + + return done.promise; + }, + + // middleware for serving the files + 'serve': function () { + return express['static'](path.join(__dirname, '/../../../', localDir)); + } +}); + +module.exports = localFileStore; diff --git a/core/test/unit/admin_spec.js b/core/test/unit/admin_spec.js index 5a5c6540e4..9a115cdd3d 100644 --- a/core/test/unit/admin_spec.js +++ b/core/test/unit/admin_spec.js @@ -1,8 +1,9 @@ /*globals describe, beforeEach, it*/ -var fs = require('fs-extra'), - should = require('should'), - sinon = require('sinon'), - when = require('when'), +var fs = require('fs-extra'), + should = require('should'), + sinon = require('sinon'), + when = require('when'), + storage = require('../../server/storage'), // Stuff we are testing admin = require('../../server/controllers/admin'); @@ -10,7 +11,7 @@ var fs = require('fs-extra'), describe('Admin Controller', function () { describe('uploader', function () { - var req, res, storage; + var req, res, store; beforeEach(function () { req = { @@ -26,20 +27,22 @@ describe('Admin Controller', function () { } }; - storage = sinon.stub(); - storage.save = sinon.stub().returns(when('URL')); - sinon.stub(admin, 'get_storage').returns(storage); + store = sinon.stub(); + store.save = sinon.stub().returns(when('URL')); + store.exists = sinon.stub().returns(when(true)); + store.destroy = sinon.stub().returns(when()); + sinon.stub(storage, 'get_storage').returns(store); }); afterEach(function () { - admin.get_storage.restore(); + storage.get_storage.restore(); }); describe('can not upload invalid file', function () { it('should return 415 for invalid file type', function () { res.send = sinon.stub(); req.files.uploadimage.name = 'INVALID.FILE'; - req.files.uploadimage.type = 'application/octet-stream' + req.files.uploadimage.type = 'application/octet-stream'; admin.uploader(req, res); res.send.calledOnce.should.be.true; res.send.args[0][0].should.equal(415); @@ -112,16 +115,6 @@ describe('Admin Controller', function () { admin.uploader(req, res); }); - it('should not leave temporary file when uploading', function (done) { - sinon.stub(res, 'send', function (data) { - fs.unlink.calledOnce.should.be.true; - fs.unlink.args[0][0].should.equal('/tmp/TMPFILEID'); - return done(); - }); - - admin.uploader(req, res); - }); - it('should send correct url', function (done) { sinon.stub(res, 'send', function (data) { data.should.equal('URL'); diff --git a/core/test/unit/storage_localfilesystem_spec.js b/core/test/unit/storage_localfilesystem_spec.js index b3b7852902..27bdb7a014 100644 --- a/core/test/unit/storage_localfilesystem_spec.js +++ b/core/test/unit/storage_localfilesystem_spec.js @@ -4,9 +4,9 @@ var fs = require('fs-extra'), should = require('should'), sinon = require('sinon'), when = require('when'), - localfilesystem = require('../../server/controllers/storage/localfilesystem'); + localfilesystem = require('../../server/storage/localfilesystem'); -describe('Local File System Storage', function() { +describe('Local File System Storage', function () { var image; @@ -14,32 +14,36 @@ describe('Local File System Storage', function() { sinon.stub(fs, 'mkdirs').yields(); sinon.stub(fs, 'copy').yields(); sinon.stub(fs, 'exists').yields(false); + sinon.stub(fs, 'unlink').yields(); + image = { path: "tmp/123456.jpg", name: "IMAGE.jpg", type: "image/jpeg" }; + + // Sat Sep 07 2013 21:24 + this.clock = sinon.useFakeTimers(new Date(2013, 8, 7, 21, 24).getTime()); }); afterEach(function () { fs.mkdirs.restore(); fs.copy.restore(); fs.exists.restore(); + fs.unlink.restore(); + this.clock.restore(); }); it('should send correct path to image when date is in Sep 2013', function (done) { - // Sat Sep 07 2013 21:24 - var date = new Date(2013, 8, 7, 21, 24).getTime(); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2013/Sep/IMAGE.jpg'); return done(); }); }); it('should send correct path to image when original file has spaces', function (done) { - var date = new Date(2013, 8, 7, 21, 24).getTime(); image.name = 'AN IMAGE.jpg'; - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2013/Sep/AN_IMAGE.jpg'); return done(); }); @@ -47,17 +51,15 @@ describe('Local File System Storage', function() { it('should send correct path to image when date is in Jan 2014', function (done) { // Jan 1 2014 12:00 - var date = new Date(2014, 0, 1, 12).getTime(); - localfilesystem.save(date, image).then(function (url) { + this.clock = sinon.useFakeTimers(new Date(2014, 0, 1, 12).getTime()); + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2014/Jan/IMAGE.jpg'); return done(); }); }); it('should create month and year directory', function (done) { - // Sat Sep 07 2013 21:24 - var date = new Date(2013, 8, 7, 21, 24).getTime(); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { fs.mkdirs.calledOnce.should.be.true; fs.mkdirs.args[0][0].should.equal(path.join('content/images/2013/Sep')); done(); @@ -65,9 +67,7 @@ describe('Local File System Storage', function() { }); it('should copy temp file to new location', function (done) { - // Sat Sep 07 2013 21:24 - var date = new Date(2013, 8, 7, 21, 24).getTime(); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { fs.copy.calledOnce.should.be.true; fs.copy.args[0][0].should.equal('tmp/123456.jpg'); fs.copy.args[0][1].should.equal(path.join('content/images/2013/Sep/IMAGE.jpg')); @@ -75,10 +75,17 @@ describe('Local File System Storage', function() { }).then(null, done); }); + it('should not leave temporary file when uploading', function (done) { + localfilesystem.save(image).then(function (url) { + fs.unlink.calledOnce.should.be.true; + fs.unlink.args[0][0].should.equal('tmp/123456.jpg'); + done(); + }).then(null, done); + }); + it('can upload two different images with the same name without overwriting the first', function (done) { // Sun Sep 08 2013 10:57 - var date = new Date(2013, 8, 8, 10, 57).getTime(); - clock = sinon.useFakeTimers(date); + this.clock = sinon.useFakeTimers(new Date(2013, 8, 8, 10, 57).getTime()); fs.exists.withArgs('content/images/2013/Sep/IMAGE.jpg').yields(true); fs.exists.withArgs('content/images/2013/Sep/IMAGE-1.jpg').yields(false); @@ -87,7 +94,7 @@ describe('Local File System Storage', function() { fs.exists.withArgs('content\\images\\2013\\Sep\\IMAGE.jpg').yields(true); fs.exists.withArgs('content\\images\\2013\\Sep\\IMAGE-1.jpg').yields(false); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2013/Sep/IMAGE-1.jpg'); return done(); }); @@ -95,8 +102,7 @@ describe('Local File System Storage', function() { it('can upload five different images with the same name without overwriting the first', function (done) { // Sun Sep 08 2013 10:57 - var date = new Date(2013, 8, 8, 10, 57).getTime(); - clock = sinon.useFakeTimers(date); + this.clock = sinon.useFakeTimers(new Date(2013, 8, 8, 10, 57).getTime()); fs.exists.withArgs('content/images/2013/Sep/IMAGE.jpg').yields(true); fs.exists.withArgs('content/images/2013/Sep/IMAGE-1.jpg').yields(true); fs.exists.withArgs('content/images/2013/Sep/IMAGE-2.jpg').yields(true); @@ -110,7 +116,7 @@ describe('Local File System Storage', function() { fs.exists.withArgs('content\\images\\2013\\Sep\\IMAGE-3.jpg').yields(true); fs.exists.withArgs('content\\images\\2013\\Sep\\IMAGE-4.jpg').yields(false); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2013/Sep/IMAGE-4.jpg'); return done(); }); @@ -134,8 +140,7 @@ describe('Local File System Storage', function() { it('should return url in proper format for windows', function (done) { path.sep = '\\'; path.join.returns('content\\images\\2013\\Sep\\IMAGE.jpg'); - var date = new Date(2013, 8, 7, 21, 24).getTime(); - localfilesystem.save(date, image).then(function (url) { + localfilesystem.save(image).then(function (url) { url.should.equal('/content/images/2013/Sep/IMAGE.jpg'); return done(); });