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

@@ -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

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
+        store
-            .save(new Date().getTime(), req.files.uploadimage)
+            .save(req.files.uploadimage)
             .then(function (url) {
-
+                return res.send(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);
-                });
             })
             .otherwise(function (e) {
                 return errors.logError(e);

core/server/controllers/storage/localfilesystem.js

@@ -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;

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;

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;

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;

core/test/unit/admin_spec.js

@@ -1,8 +1,9 @@
 /*globals describe, beforeEach, it*/
-var fs = require('fs-extra'),
+var fs      = require('fs-extra'),
-    should = require('should'),
+    should  = require('should'),
-    sinon = require('sinon'),
+    sinon   = require('sinon'),
-    when = require('when'),
+    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();
+            store = sinon.stub();
-            storage.save = sinon.stub().returns(when('URL'));
+            store.save = sinon.stub().returns(when('URL'));
-            sinon.stub(admin, 'get_storage').returns(storage);
+            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');

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
+        localfilesystem.save(image).then(function (url) {
-        var date = new Date(2013, 8, 7, 21, 24).getTime();
-        localfilesystem.save(date, 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();
+        this.clock = sinon.useFakeTimers(new Date(2014, 0, 1, 12).getTime());
-        localfilesystem.save(date, image).then(function (url) {
+        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
+        localfilesystem.save(image).then(function (url) {
-        var date = new Date(2013, 8, 7, 21, 24).getTime();
-        localfilesystem.save(date, 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
+        localfilesystem.save(image).then(function (url) {
-        var date = new Date(2013, 8, 7, 21, 24).getTime();
-        localfilesystem.save(date, 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();
+        this.clock = sinon.useFakeTimers(new Date(2013, 8, 8, 10, 57).getTime());
-        clock = sinon.useFakeTimers(date);
         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();
+        this.clock = sinon.useFakeTimers(new Date(2013, 8, 8, 10, 57).getTime());
-        clock = sinon.useFakeTimers(date);
         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(image).then(function (url) {
-            localfilesystem.save(date, image).then(function (url) {
                 url.should.equal('/content/images/2013/Sep/IMAGE.jpg');
                 return done();
             });