From 9c580115305e65d47e948c20cd1669660965d1c7 Mon Sep 17 00:00:00 2001
From: Raymond Feng <raymond@strongloop.com>
Date: Tue, 14 Jan 2014 10:26:09 -0800
Subject: [PATCH] Clean up the download method

---
 example/app-loopback.js           |  2 +
 lib/providers/filesystem/index.js | 12 ++++-
 lib/storage-handler.js            |  9 ++--
 lib/storage-service.js            | 80 ++++++++++++++-----------------
 test/upload-download.test.js      | 65 +++++++++++++++++++++++++
 test/upload.test.js               | 39 ---------------
 6 files changed, 115 insertions(+), 92 deletions(-)
 create mode 100644 test/upload-download.test.js
 delete mode 100644 test/upload.test.js

diff --git a/example/app-loopback.js b/example/app-loopback.js
index ff30c6c..e81a84e 100644
--- a/example/app-loopback.js
+++ b/example/app-loopback.js
@@ -3,6 +3,8 @@ var loopback = require('loopback')
 
 var path = require('path');
 
+app.use(app.router);
+
 // expose a rest api
 app.use(loopback.rest());
 
diff --git a/lib/providers/filesystem/index.js b/lib/providers/filesystem/index.js
index 3854afa..7c29dee 100644
--- a/lib/providers/filesystem/index.js
+++ b/lib/providers/filesystem/index.js
@@ -136,7 +136,11 @@ FileSystemProvider.prototype.upload = function (options, cb) {
     encoding: null,
     mode: 0666 };
 
-  return fs.createWriteStream(filePath, fileOpts);
+  try {
+    return fs.createWriteStream(filePath, fileOpts);
+  } catch (e) {
+    cb && cb(e);
+  }
 };
 
 FileSystemProvider.prototype.download = function (options, cb) {
@@ -150,7 +154,11 @@ FileSystemProvider.prototype.download = function (options, cb) {
   var fileOpts = {flags: 'r',
     autoClose: true };
 
-  return fs.createReadStream(filePath, fileOpts);
+  try {
+    return fs.createReadStream(filePath, fileOpts);
+  } catch (e) {
+    cb && cb(e);
+  }
 };
 
 FileSystemProvider.prototype.getFiles = function (container, download, cb) {
diff --git a/lib/storage-handler.js b/lib/storage-handler.js
index e7b6d01..2c48030 100644
--- a/lib/storage-handler.js
+++ b/lib/storage-handler.js
@@ -113,15 +113,12 @@ exports.upload = function (provider, req, res, container, cb) {
  */
 exports.download = function(provider, req, res, container, file, cb) {
     var reader = provider.download({
-        container: container || req.params.container,
-        remote: file || req.params.file
+        container: container || req && req.params.container,
+        remote: file || req && req.params.file
     });
     reader.pipe(res);
     reader.on('error', function(err) {
-       cb && cb(err);
-    });
-    reader.on('end', function(err, result) {
-       cb && cb(err, result);
+        res.send(500, { error: err });
     });
 }
 
diff --git a/lib/storage-service.js b/lib/storage-service.js
index 0f0ee15..16e462b 100644
--- a/lib/storage-service.js
+++ b/lib/storage-service.js
@@ -22,7 +22,7 @@ function StorageService(options) {
 }
 
 function map(obj) {
-  if(!obj || typeof obj !== 'object') {
+  if (!obj || typeof obj !== 'object') {
     return obj;
   }
   var data = {};
@@ -40,11 +40,11 @@ function map(obj) {
 }
 
 StorageService.prototype.getContainers = function (cb) {
-  this.client.getContainers(function(err, containers) {
-    if(err) {
+  this.client.getContainers(function (err, containers) {
+    if (err) {
       cb(err, containers);
     } else {
-      cb(err, containers.map(function(c) {
+      cb(err, containers.map(function (c) {
         return new Container(map(c));
       }));
     }
@@ -57,7 +57,7 @@ StorageService.prototype.createContainer = function (options, cb) {
     var Container = factory.getProvider(this.provider).Container;
     options = new Container(this.client, options);
   }
-  return this.client.createContainer(options, function(err, container) {
+  return this.client.createContainer(options, function (err, container) {
     return cb(err, map(container));
   });
 };
@@ -67,7 +67,7 @@ StorageService.prototype.destroyContainer = function (container, cb) {
 };
 
 StorageService.prototype.getContainer = function (container, cb) {
-  return this.client.getContainer(container, function(err, container) {
+  return this.client.getContainer(container, function (err, container) {
     return cb(err, map(container));
   });
 };
@@ -98,11 +98,11 @@ StorageService.prototype.downloadStream = function (container, file, options, cb
 };
 
 StorageService.prototype.getFiles = function (container, download, cb) {
-  return this.client.getFiles(container, download, function(err, files) {
-    if(err) {
+  return this.client.getFiles(container, download, function (err, files) {
+    if (err) {
       cb(err, files);
     } else {
-      cb(err, files.map(function(f) {
+      cb(err, files.map(function (f) {
         return new File(map(f));
       }));
     }
@@ -110,7 +110,7 @@ StorageService.prototype.getFiles = function (container, download, cb) {
 };
 
 StorageService.prototype.getFile = function (container, file, cb) {
-  return this.client.getFile(container, file, function(err, f) {
+  return this.client.getFile(container, file, function (err, f) {
     return cb(err, map(f));
   });
 };
@@ -123,9 +123,8 @@ StorageService.prototype.upload = function (req, res, cb) {
   return handler.upload(this.client, req, res, req.params.container, cb);
 };
 
-StorageService.prototype.download = function (req, res, cb) {
-  return handler.download(this.client, req, res,
-    req.params.container, req.params.file, cb);
+StorageService.prototype.download = function (container, file, res, cb) {
+  return handler.download(this.client, null, res, container, file, cb);
 };
 
 StorageService.modelName = 'storage';
@@ -133,45 +132,40 @@ StorageService.modelName = 'storage';
 StorageService.prototype.getContainers.shared = true;
 StorageService.prototype.getContainers.accepts = [];
 StorageService.prototype.getContainers.returns = {arg: 'containers', type: 'array', root: true};
-StorageService.prototype.getContainers.http = [
-  {verb: 'get', path: '/'}
-];
+StorageService.prototype.getContainers.http =
+{verb: 'get', path: '/'};
 
 StorageService.prototype.getContainer.shared = true;
 StorageService.prototype.getContainer.accepts = [
   {arg: 'container', type: 'string'}
 ];
 StorageService.prototype.getContainer.returns = {arg: 'container', type: 'object', root: true};
-StorageService.prototype.getContainer.http = [
-  {verb: 'get', path: '/:container'}
-];
+StorageService.prototype.getContainer.http =
+{verb: 'get', path: '/:container'};
 
 StorageService.prototype.createContainer.shared = true;
 StorageService.prototype.createContainer.accepts = [
   {arg: 'options', type: 'object'}
 ];
 StorageService.prototype.createContainer.returns = {arg: 'container', type: 'object', root: true};
-StorageService.prototype.createContainer.http = [
-  {verb: 'post', path: '/'}
-];
+StorageService.prototype.createContainer.http =
+{verb: 'post', path: '/'};
 
 StorageService.prototype.destroyContainer.shared = true;
 StorageService.prototype.destroyContainer.accepts = [
   {arg: 'container', type: 'string'}
 ];
 StorageService.prototype.destroyContainer.returns = {};
-StorageService.prototype.destroyContainer.http = [
-  {verb: 'delete', path: '/:container'}
-];
+StorageService.prototype.destroyContainer.http =
+{verb: 'delete', path: '/:container'};
 
 StorageService.prototype.getFiles.shared = true;
 StorageService.prototype.getFiles.accepts = [
   {arg: 'container', type: 'string'}
 ];
 StorageService.prototype.getFiles.returns = {arg: 'files', type: 'array', root: true};
-StorageService.prototype.getFiles.http = [
-  {verb: 'get', path: '/:container/files'}
-];
+StorageService.prototype.getFiles.http =
+{verb: 'get', path: '/:container/files'};
 
 StorageService.prototype.getFile.shared = true;
 StorageService.prototype.getFile.accepts = [
@@ -179,9 +173,8 @@ StorageService.prototype.getFile.accepts = [
   {arg: 'file', type: 'string'}
 ];
 StorageService.prototype.getFile.returns = {arg: 'file', type: 'object', root: true};
-StorageService.prototype.getFile.http = [
-  {verb: 'get', path: '/:container/files/:file'}
-];
+StorageService.prototype.getFile.http =
+{verb: 'get', path: '/:container/files/:file'};
 
 StorageService.prototype.removeFile.shared = true;
 StorageService.prototype.removeFile.accepts = [
@@ -189,26 +182,23 @@ StorageService.prototype.removeFile.accepts = [
   {arg: 'file', type: 'string'}
 ];
 StorageService.prototype.removeFile.returns = {};
-StorageService.prototype.removeFile.http = [
-  {verb: 'delete', path: '/:container/files/:file'}
-];
+StorageService.prototype.removeFile.http =
+{verb: 'delete', path: '/:container/files/:file'};
 
 StorageService.prototype.upload.shared = true;
 StorageService.prototype.upload.accepts = [
-  {arg: 'req', type: 'undefined', 'http': {source: 'req'}},
-  {arg: 'res', type: 'undefined', 'http': {source: 'res'}}
+  {arg: 'req', type: 'object', 'http': {source: 'req'}},
+  {arg: 'res', type: 'object', 'http': {source: 'res'}}
 ];
 StorageService.prototype.upload.returns = {arg: 'result', type: 'object'};
-StorageService.prototype.upload.http = [
-  {verb: 'post', path: '/:container/upload'}
-];
+StorageService.prototype.upload.http =
+{verb: 'post', path: '/:container/upload'};
 
 StorageService.prototype.download.shared = true;
 StorageService.prototype.download.accepts = [
-  {arg: 'req', type: 'undefined', 'http': {source: 'req'}},
-  {arg: 'res', type: 'undefined', 'http': {source: 'res'}}
+  {arg: 'container', type: 'string', 'http': {source: 'path'}},
+  {arg: 'file', type: 'string', 'http': {source: 'path'}},
+  {arg: 'res', type: 'object', 'http': {source: 'res'}}
 ];
-StorageService.prototype.download.returns = {arg: 'res', type: 'stream'};
-StorageService.prototype.download.http = [
-  {verb: 'get', path: '/:container/download/:file'}
-];
\ No newline at end of file
+StorageService.prototype.download.http =
+{verb: 'get', path: '/:container/download/:file'};
\ No newline at end of file
diff --git a/test/upload-download.test.js b/test/upload-download.test.js
new file mode 100644
index 0000000..ae3bb54
--- /dev/null
+++ b/test/upload-download.test.js
@@ -0,0 +1,65 @@
+var request = require('supertest');
+var loopback = require('loopback');
+var assert = require('assert');
+
+var app = loopback();
+var path = require('path');
+
+// expose a rest api
+app.use(loopback.rest());
+
+var ds = loopback.createDataSource({
+  connector: require('../lib/storage-connector'),
+  provider: 'filesystem',
+  root: path.join(__dirname, 'images')
+});
+
+var Container = ds.createModel('container');
+app.model(Container);
+
+describe('storage service', function () {
+  var server = null;
+  before(function (done) {
+    server = app.listen(3000, function () {
+      done();
+    });
+  });
+
+  after(function () {
+    server.close();
+  });
+
+  it('uploads files', function (done) {
+
+    request('http://localhost:3000')
+      .post('/containers/album1/upload')
+      .attach('image', path.join(__dirname, '../example/test.jpg'))
+      .set('Accept', 'application/json')
+      .expect('Content-Type', /json/)
+      .expect(200, function (err, res) {
+        assert.deepEqual(res.body, {"result": {"files": {"image": [
+          {"container": "album1", "name": "test.jpg", "type": "image/jpeg"}
+        ]}, "fields": {}}});
+        done();
+      });
+  });
+
+  it('downloads files', function (done) {
+
+    request('http://localhost:3000')
+      .get('/containers/album1/download/test.jpg')
+      .expect(200, function (err, res) {
+        done();
+      });
+  });
+
+  it('reports errors if it fails to find the file to download', function (done) {
+
+    request('http://localhost:3000')
+      .get('/containers/album1/download/test_not_exist.jpg')
+      .expect(500, function (err, res) {
+        assert(res.body.error);
+        done();
+      });
+  });
+});
diff --git a/test/upload.test.js b/test/upload.test.js
deleted file mode 100644
index f96b2af..0000000
--- a/test/upload.test.js
+++ /dev/null
@@ -1,39 +0,0 @@
-var request = require('supertest');
-var loopback = require('loopback');
-var assert = require('assert');
-
-var app = loopback();
-var path = require('path');
-
-// expose a rest api
-app.use(loopback.rest());
-
-var ds = loopback.createDataSource({
-  connector: require('../lib/storage-connector'),
-  provider: 'filesystem',
-  root: path.join(__dirname, 'images')
-});
-
-var Container = ds.createModel('container');
-app.model(Container);
-
-describe('storage service', function () {
-  it('uploads files', function (done) {
-
-    var server = app.listen(3000, function () {
-
-      request('http://localhost:3000')
-        .post('/containers/album1/upload')
-        .attach('image', path.join(__dirname, '../example/test.jpg'))
-        .set('Accept', 'application/json')
-        .expect('Content-Type', /json/)
-        .expect(200, function (err, res) {
-          assert.deepEqual(res.body, {"result": {"files": {"image": [
-            {"container": "album1", "name": "test.jpg", "type": "image/jpeg"}
-          ]}, "fields": {}}});
-          server.close();
-          done();
-        });
-    });
-  });
-});