From 9287afc3c22ee5185c14605c022b8e50a30acd0e Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Sun, 28 Feb 2016 19:36:16 -0500 Subject: [PATCH 1/3] refactors filesAdapter tests in factories --- spec/FilesController.spec.js | 40 ++++++++-------- spec/FilesControllerTestFactory.js | 73 ++++++++++++++++++++++++++++++ src/Adapters/Files/S3Adapter.js | 68 +++++++++++++++++++--------- 3 files changed, 142 insertions(+), 39 deletions(-) create mode 100644 spec/FilesControllerTestFactory.js diff --git a/spec/FilesController.spec.js b/spec/FilesController.spec.js index 67b36de906..13a443d861 100644 --- a/spec/FilesController.spec.js +++ b/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) => { - var config = new Config(Parse.applicationId); - var adapter = new GridStoreAdapter(); - var filesController = new FilesController(adapter); - var result = filesController.expandFilesInObject(config, function(){}); + // Test the grid store adapter + var gridStoreAdapter = new GridStoreAdapter(); + FCTestFactory.testAdapter("GridStoreAdapter", gridStoreAdapter); + + if (process.env.S3_ACCESS_KEY && process.env.S3_SECRET_KEY) { + + // Test the S3 Adapter + var s3Adapter = new S3Adapter(process.env.S3_ACCESS_KEY, process.env.S3_SECRET_KEY, 'parse.server.tests'); - expect(result).toBeUndefined(); + FCTestFactory.testAdapter("S3Adapter",s3Adapter); - var fullFile = { - type: '__type', - url: "http://an.url" - } + // Test S3 with direct access + var s3DirectAccessAdapter = new S3Adapter(process.env.S3_ACCESS_KEY, process.env.S3_SECRET_KEY, 'parse.server.tests', { + directAccess: true + }); - var anObject = { - aFile: fullFile - } - filesController.expandFilesInObject(config, anObject); - expect(anObject.aFile.url).toEqual("http://an.url"); + FCTestFactory.testAdapter("S3AdapterDirect", s3DirectAccessAdapter); - done(); - }) -}) \ No newline at end of file + } else if (!process.env.TRAVIS) { + console.log("set S3_ACCESS_KEY and S3_SECRET_KEY to test S3Adapter") + } +}); diff --git a/spec/FilesControllerTestFactory.js b/spec/FilesControllerTestFactory.js new file mode 100644 index 0000000000..217a383a16 --- /dev/null +++ b/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 +} diff --git a/src/Adapters/Files/S3Adapter.js b/src/Adapters/Files/S3Adapter.js index d63880f461..4acddf6b54 100644 --- a/src/Adapters/Files/S3Adapter.js +++ b/src/Adapters/Files/S3Adapter.js @@ -48,6 +48,22 @@ 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 @@ -60,26 +76,30 @@ export class S3Adapter extends FilesAdapter { if (this._directAccess) { params.ACL = "public-read" } - return new Promise((resolve, reject) => { - this._s3Client.upload(params, (err, data) => { - if (err !== null) { - return reject(err); - } - resolve(data); + return this.createBucket().then(() => { + return new Promise((resolve, reject) => { + this._s3Client.upload(params, (err, data) => { + if (err !== null) { + return reject(err); + } + resolve(data); + }); }); }); } deleteFile(config, filename) { - return new Promise((resolve, reject) => { - let params = { - Key: this._bucketPrefix + filename - }; - this._s3Client.deleteObject(params, (err, data) =>{ - if(err !== null) { - return reject(err); - } - resolve(data); + return this.createBucket().then(() => { + return new Promise((resolve, reject) => { + let params = { + Key: this._bucketPrefix + filename + }; + this._s3Client.deleteObject(params, (err, data) =>{ + if(err !== null) { + return reject(err); + } + resolve(data); + }); }); }); } @@ -88,12 +108,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) => { - this._s3Client.getObject(params, (err, data) => { - if (err !== null) { - return reject(err); - } - resolve(data.Body); + return this.createBucket().then(() => { + return new Promise((resolve, reject) => { + this._s3Client.getObject(params, (err, data) => { + if (err !== null) { + return reject(err); + } + // Something happend here... + if (data && !data.Body) { + return reject(data); + } + resolve(data.Body); + }); }); }); } From 78d380df72caf106d7a5747e6e6e7895ab4b005c Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Tue, 1 Mar 2016 09:02:33 -0500 Subject: [PATCH 2/3] Adds content type support in S3 --- spec/FilesController.spec.js | 2 +- src/Adapters/Files/FilesAdapter.js | 2 +- src/Adapters/Files/GridStoreAdapter.js | 2 +- src/Adapters/Files/S3Adapter.js | 5 ++++- src/Controllers/FilesController.js | 4 ++-- src/Routers/FilesRouter.js | 20 +++++++++++++------- 6 files changed, 22 insertions(+), 13 deletions(-) diff --git a/spec/FilesController.spec.js b/spec/FilesController.spec.js index 13a443d861..2306676e83 100644 --- a/spec/FilesController.spec.js +++ b/spec/FilesController.spec.js @@ -10,7 +10,7 @@ var FCTestFactory = require("./FilesControllerTestFactory"); describe("FilesController",()=>{ // Test the grid store adapter - var gridStoreAdapter = new GridStoreAdapter(); + var gridStoreAdapter = new GridStoreAdapter('mongodb://localhost:27017/parse'); FCTestFactory.testAdapter("GridStoreAdapter", gridStoreAdapter); if (process.env.S3_ACCESS_KEY && process.env.S3_SECRET_KEY) { diff --git a/src/Adapters/Files/FilesAdapter.js b/src/Adapters/Files/FilesAdapter.js index 2ff9fdb24a..1ddfbabbdd 100644 --- a/src/Adapters/Files/FilesAdapter.js +++ b/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) { } diff --git a/src/Adapters/Files/GridStoreAdapter.js b/src/Adapters/Files/GridStoreAdapter.js index b95f5563a0..ee5cf3ab67 100644 --- a/src/Adapters/Files/GridStoreAdapter.js +++ b/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: string) { return this._connect().then(database => { let gridStore = new GridStore(database, filename, 'w'); return gridStore.open(); diff --git a/src/Adapters/Files/S3Adapter.js b/src/Adapters/Files/S3Adapter.js index 4acddf6b54..e21ef8db26 100644 --- a/src/Adapters/Files/S3Adapter.js +++ b/src/Adapters/Files/S3Adapter.js @@ -68,7 +68,7 @@ export class S3Adapter extends FilesAdapter { // 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 @@ -76,6 +76,9 @@ export class S3Adapter extends FilesAdapter { if (this._directAccess) { params.ACL = "public-read" } + if (contentType) { + params.ContentType = contentType; + } return this.createBucket().then(() => { return new Promise((resolve, reject) => { this._s3Client.upload(params, (err, data) => { diff --git a/src/Controllers/FilesController.js b/src/Controllers/FilesController.js index 9634d807a4..7808675892 100644 --- a/src/Controllers/FilesController.js +++ b/src/Controllers/FilesController.js @@ -10,10 +10,10 @@ export class FilesController extends AdaptableController { return this.adapter.getFileData(config, filename); } - createFile(config, filename, data) { + createFile(config, filename, data, contentType) { 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 diff --git a/src/Routers/FilesRouter.js b/src/Routers/FilesRouter.js index 65da555fab..5c82d7ccd1 100644 --- a/src/Routers/FilesRouter.js +++ b/src/Routers/FilesRouter.js @@ -4,6 +4,7 @@ import * as Middlewares from '../middlewares'; import { randomHexString } from '../cryptoUtils'; import mime from 'mime'; import Config from '../Config'; +import path from 'path'; export class FilesRouter { @@ -66,20 +67,25 @@ export class FilesRouter { 'Filename contains invalid characters.')); return; } - let extension = ''; - // Not very safe there. - const hasExtension = req.params.filename.indexOf('.') > 0; - const contentType = req.get('Content-type'); + let filename = req.params.filename; + + // safe way to get the extension + let extname = path.extname(filename); + let contentType = req.get('Content-type'); + + const hasExtension = extname.length > 0; + if (!hasExtension && contentType && mime.extension(contentType)) { - extension = '.' + mime.extension(contentType); + filename = filename + '.' + mime.extension(contentType); + } else if (hasExtension && !contentType) { + contentType = mime.lookup(req.params.filename); } - 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); From 7257ee858bf6527b313a085ce8d0773672f47a6c Mon Sep 17 00:00:00 2001 From: Florent Vilmart Date: Tue, 1 Mar 2016 10:14:03 -0500 Subject: [PATCH 3/3] Moves some logic from FilesRouter to FilesController for content-type and filename --- src/Adapters/Files/GridStoreAdapter.js | 2 +- src/Controllers/FilesController.js | 14 ++++++++++++++ src/Routers/FilesRouter.js | 21 ++++----------------- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/Adapters/Files/GridStoreAdapter.js b/src/Adapters/Files/GridStoreAdapter.js index ee5cf3ab67..e6532e2104 100644 --- a/src/Adapters/Files/GridStoreAdapter.js +++ b/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, contentType: string) { + createFile(config, filename: string, data, contentType) { return this._connect().then(database => { let gridStore = new GridStore(database, filename, 'w'); return gridStore.open(); diff --git a/src/Controllers/FilesController.js b/src/Controllers/FilesController.js index 7808675892..712e326c16 100644 --- a/src/Controllers/FilesController.js +++ b/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 { @@ -11,7 +13,19 @@ export class FilesController extends AdaptableController { } 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, contentType).then(() => { return Promise.resolve({ diff --git a/src/Routers/FilesRouter.js b/src/Routers/FilesRouter.js index 5c82d7ccd1..a3a3c81172 100644 --- a/src/Routers/FilesRouter.js +++ b/src/Routers/FilesRouter.js @@ -2,9 +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 path from 'path'; +import mime from 'mime'; export class FilesRouter { @@ -42,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.'); @@ -68,20 +67,8 @@ export class FilesRouter { return; } - let filename = req.params.filename; - - // safe way to get the extension - let extname = path.extname(filename); - let contentType = req.get('Content-type'); - - const hasExtension = extname.length > 0; - - if (!hasExtension && contentType && mime.extension(contentType)) { - filename = filename + '.' + mime.extension(contentType); - } else if (hasExtension && !contentType) { - contentType = mime.lookup(req.params.filename); - } - + const filename = req.params.filename; + const contentType = req.get('Content-type'); const config = req.config; const filesController = config.filesController;