Merge pull request #738 from ParsePlatform/flovilmart.s3Improvements

S3 Improvements

spec/FilesController.spec.js

@@ -1,29 +1,33 @@
 var FilesController = require('../src/Controllers/FilesController').FilesController;
 var GridStoreAdapter = require("../src/Adapters/Files/GridStoreAdapter").GridStoreAdapter;
+var S3Adapter = require("../src/Adapters/Files/S3Adapter").S3Adapter;
 var Config = require("../src/Config");
 
+var FCTestFactory = require("./FilesControllerTestFactory");
+
+
 // Small additional tests to improve overall coverage
 describe("FilesController",()=>{
   
-  it("should properly expand objects", (done) => {
+  // Test the grid store adapter
-    var config = new Config(Parse.applicationId);
+  var gridStoreAdapter = new GridStoreAdapter('mongodb://localhost:27017/parse');
-    var adapter = new GridStoreAdapter();
+  FCTestFactory.testAdapter("GridStoreAdapter", gridStoreAdapter);
-    var filesController = new FilesController(adapter);
+  
-    var result = filesController.expandFilesInObject(config, function(){});
+  if (process.env.S3_ACCESS_KEY && process.env.S3_SECRET_KEY) {
     
-    expect(result).toBeUndefined();
+    // Test the S3 Adapter
+    var s3Adapter = new S3Adapter(process.env.S3_ACCESS_KEY, process.env.S3_SECRET_KEY, 'parse.server.tests');
     
-    var fullFile = {
+    FCTestFactory.testAdapter("S3Adapter",s3Adapter);
-      type: '__type',
-      url: "http://an.url"
-    }
     
-    var anObject = {
+    // Test S3 with direct access
-      aFile: fullFile
+    var s3DirectAccessAdapter = new S3Adapter(process.env.S3_ACCESS_KEY, process.env.S3_SECRET_KEY, 'parse.server.tests', {
-    }
+      directAccess: true
-    filesController.expandFilesInObject(config, anObject);
+    });
-    expect(anObject.aFile.url).toEqual("http://an.url");
     
-    done();
+    FCTestFactory.testAdapter("S3AdapterDirect", s3DirectAccessAdapter);
-  }) 
+    
-})
+  } else if (!process.env.TRAVIS) {
+    console.log("set S3_ACCESS_KEY and S3_SECRET_KEY to test S3Adapter")
+  }
+});

spec/FilesControllerTestFactory.js

@@ -0,0 +1,73 @@
+
+var FilesController = require('../src/Controllers/FilesController').FilesController;
+var Config = require("../src/Config");
+
+var testAdapter = function(name, adapter) {
+  // Small additional tests to improve overall coverage
+  
+  var config = new Config(Parse.applicationId);
+  var filesController = new FilesController(adapter);
+
+  describe("FilesController with "+name,()=>{
+    
+    it("should properly expand objects", (done) => {
+      
+      var result = filesController.expandFilesInObject(config, function(){});
+      
+      expect(result).toBeUndefined();
+      
+      var fullFile = {
+        type: '__type',
+        url: "http://an.url"
+      }
+      
+      var anObject = {
+        aFile: fullFile
+      }
+      filesController.expandFilesInObject(config, anObject);
+      expect(anObject.aFile.url).toEqual("http://an.url");
+      
+      done();
+    }) 
+    
+    it("should properly create, read, delete files", (done) => {
+      var filename;
+      filesController.createFile(config, "file.txt", "hello world").then( (result) => {
+        ok(result.url);
+        ok(result.name);
+        filename = result.name;
+        expect(result.name.match(/file.txt/)).not.toBe(null);
+        return filesController.getFileData(config, filename);
+      }, (err) => {
+        fail("The adapter should create the file");
+        console.error(err);
+        done();
+      }).then((result) => {
+        expect(result instanceof Buffer).toBe(true);
+        expect(result.toString('utf-8')).toEqual("hello world");
+        return filesController.deleteFile(config, filename);
+      }, (err) => {
+        fail("The adapter should get the file");
+        console.error(err);
+        done();
+      }).then((result) => {
+        
+        filesController.getFileData(config, filename).then((res) => {
+          fail("the file should be deleted");
+          done();
+        }, (err) => {
+          done();  
+        });
+        
+      }, (err) => {
+        fail("The adapter should delete the file");
+        console.error(err);
+        done();
+      });
+    }, 5000); // longer tests
+  });
+}
+
+module.exports = {
+  testAdapter: testAdapter
+}

src/Adapters/Files/FilesAdapter.js

@@ -12,7 +12,7 @@
 // database adapter.
 
 export class FilesAdapter {
-  createFile(config, filename, data) { }
+  createFile(config, filename: string, data, contentType: string) { }
 
   deleteFile(config, filename) { }
 

src/Adapters/Files/GridStoreAdapter.js

@@ -28,7 +28,7 @@ export class GridStoreAdapter extends FilesAdapter {
 
   // For a given config object, filename, and data, store a file
   // Returns a promise
-  createFile(config, filename: string, data) {
+  createFile(config, filename: string, data, contentType) {
     return this._connect().then(database => {
       let gridStore = new GridStore(database, filename, 'w');
       return gridStore.open();

src/Adapters/Files/S3Adapter.js

@@ -48,11 +48,27 @@ export class S3Adapter extends FilesAdapter {
     };
     AWS.config._region = this._region;
     this._s3Client = new AWS.S3(s3Options);
+    this._hasBucket = false;
+  }
+  
+  createBucket() {
+    var promise;
+    if (this._hasBucket) {
+      promise = Promise.resolve();
+    } else {
+      promise = new Promise((resolve, reject) => {
+        this._s3Client.createBucket(() => {
+          this._hasBucket = true;
+          resolve();
+        });
+      }); 
+    }
+    return promise;
   }
 
   // For a given config object, filename, and data, store a file in S3
   // Returns a promise containing the S3 object creation response
-  createFile(config, filename, data) {
+  createFile(config, filename, data, contentType) {
     let params = {
       Key: this._bucketPrefix + filename,
       Body: data
@@ -60,26 +76,33 @@ export class S3Adapter extends FilesAdapter {
     if (this._directAccess) {
       params.ACL = "public-read"
     }
-    return new Promise((resolve, reject) => {
+    if (contentType) {
-      this._s3Client.upload(params, (err, data) => {
+      params.ContentType = contentType;
-        if (err !== null) {
+    }
-          return reject(err);
+    return this.createBucket().then(() => {
-        }
+      return new Promise((resolve, reject) => {
-        resolve(data);
+        this._s3Client.upload(params, (err, data) => {
+          if (err !== null) {
+            return reject(err);
+          }
+          resolve(data);
+        });
       });
     });
   }
 
   deleteFile(config, filename) {
-    return new Promise((resolve, reject) => {
+    return this.createBucket().then(() => {
-      let params = {
+      return new Promise((resolve, reject) => {
-        Key: this._bucketPrefix + filename
+        let params = {
-      };
+          Key: this._bucketPrefix + filename
-      this._s3Client.deleteObject(params, (err, data) =>{
+        };
-        if(err !== null) {
+        this._s3Client.deleteObject(params, (err, data) =>{
-          return reject(err);
+          if(err !== null) {
-        }
+            return reject(err);
-        resolve(data);
+          }
+          resolve(data);
+        });
       });
     });
   }
@@ -88,12 +111,18 @@ export class S3Adapter extends FilesAdapter {
   // Returns a promise that succeeds with the buffer result from S3
   getFileData(config, filename) {
     let params = {Key: this._bucketPrefix + filename};
-    return new Promise((resolve, reject) => {
+    return this.createBucket().then(() => {
-      this._s3Client.getObject(params, (err, data) => {
+      return new Promise((resolve, reject) => {
-        if (err !== null) {
+        this._s3Client.getObject(params, (err, data) => {
-          return reject(err);
+          if (err !== null) {
-        }
+            return reject(err);
-        resolve(data.Body);
+          }
+          // Something happend here...
+          if (data && !data.Body) {
+            return reject(data);
+          }
+          resolve(data.Body);
+        });
       });
     });
   }

src/Controllers/FilesController.js

@@ -3,6 +3,8 @@ import { Parse } from 'parse/node';
 import { randomHexString } from '../cryptoUtils';
 import AdaptableController from './AdaptableController';
 import { FilesAdapter } from '../Adapters/Files/FilesAdapter';
+import path  from 'path';
+import mime from 'mime';
 
 export class FilesController extends AdaptableController {
 
@@ -10,10 +12,22 @@ export class FilesController extends AdaptableController {
     return this.adapter.getFileData(config, filename);
   }
 
-  createFile(config, filename, data) {
+  createFile(config, filename, data, contentType) {
+    
+    let extname = path.extname(filename);
+    
+    const hasExtension = extname.length > 0;
+    
+    if (!hasExtension && contentType && mime.extension(contentType)) {
+      filename = filename + '.' + mime.extension(contentType);
+    } else if (hasExtension && !contentType) {
+      contentType = mime.lookup(filename);
+    }
+
     filename = randomHexString(32) + '_' + filename;
+
     var location = this.adapter.getFileLocation(config, filename);
-    return this.adapter.createFile(config, filename, data).then(() => {
+    return this.adapter.createFile(config, filename, data, contentType).then(() => {
       return Promise.resolve({
         url: location,
         name: filename

src/Routers/FilesRouter.js

@@ -2,8 +2,8 @@ import express from 'express';
 import BodyParser from 'body-parser';
 import * as Middlewares from '../middlewares';
 import { randomHexString } from '../cryptoUtils';
-import mime from 'mime';
 import Config from '../Config';
+import mime from 'mime';
 
 export class FilesRouter {
 
@@ -41,7 +41,7 @@ export class FilesRouter {
       var contentType = mime.lookup(filename);
       res.set('Content-Type', contentType);
       res.end(data);
-    }).catch(() => {
+    }).catch((err) => {
       res.status(404);
       res.set('Content-Type', 'text/plain');
       res.end('File not found.');
@@ -66,20 +66,13 @@ export class FilesRouter {
         'Filename contains invalid characters.'));
       return;
     }
-    let extension = '';
 
-    // Not very safe there.
+    const filename = req.params.filename;
-    const hasExtension = req.params.filename.indexOf('.') > 0;
     const contentType = req.get('Content-type');
-    if (!hasExtension && contentType && mime.extension(contentType)) {
-      extension = '.' + mime.extension(contentType);
-    }
-
-    const filename = req.params.filename + extension;
     const config = req.config;
     const filesController = config.filesController;
 
-    filesController.createFile(config, filename, req.body).then((result) => {
+    filesController.createFile(config, filename, req.body, contentType).then((result) => {
       res.status(201);
       res.set('Location', result.url);
       res.json(result);