RAINCATCH-652 - file-storage module initial implementation #1

wtrocki · 2017-03-29T23:41:02Z

Motivation

Initial implementation of the file storage solution interface.
Interface initializes different client depending on the configuration passed in application.
Supported storage engineers:

AWS S3 storage

  s3: {
    s3Options: {
      accessKeyId: process.env.AWS_S3_ACCESS_KEY,
      secretAccessKey: process.env.AWS_S3_ACCESS_KEY_SECRET,
      region: process.env.AWS_S3_REGION
    },
    bucket: "raincatcher-files"
  }

Gridfs storage

var storageConfig = {
  gridFs: {
    mongoUrl: "mongodb://localhost:27017/files"
  }
};

Storage engine would be enabled only when file module would have flag in config set to true:

 usePersistentStorage: true

If flag is set to false files would be served from temporary storage (this would be used for demo apps for convenience and simplified setup). Alternative would be to have some localFile storage engine.

Notes

Implementation requires local file to be cached.
Buffer support on the way here: andrewrk/node-s3-client#154

witmicko

looks good. Just a few comments.

witmicko · 2017-04-03T14:23:42Z

lib/cloud/file-storage/awsStorage.js

+    }
+  };
+
+  var deferred = q.defer();


We are migrating to using bluebird vs q.deffered as it is an antipattern https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-deferred-anti-pattern

Bluebird developer telling that q api is anti pattern is not surprising.
We need to migrate to bluebird, but possibly using similar patterns across the all modules. Let's address this in our backlog as there are multiple migration steps and we should be consistent.

@witmicko KISS. We should not focus on choosing whats worse anti-pattern. If we need then we should create follow up task with refactoring after having POC that bluebird is our choice

nialldonnellyfh

Added some comments. Generally looks good though.

nialldonnellyfh · 2017-04-04T16:07:09Z

lib/cloud/file-storage/awsStorage.js

+
+  var params = {
+    localFile: fileLocation,
+    ACL: 'authenticated-read',


Should this be a constant?

nialldonnellyfh · 2017-04-04T16:07:39Z

lib/cloud/file-storage/awsStorage.js

+  awsClient = s3.createClient(config);
+}
+
+function writeFile(namespace, fileName, fileLocation) {


Could probably use a comment here describing the function and parameters.

nialldonnellyfh · 2017-04-04T16:08:47Z

lib/cloud/file-storage/awsStorage.js

+    console.log(err);
+    deferred.reject(err.stack);
+  });
+  uploader.on('progress', function() {


Are we doing anything with this progress event? If not we can probably remove the listener.

nialldonnellyfh · 2017-04-04T16:10:07Z

lib/cloud/file-storage/awsStorage.js

+    Key: file
+  };
+  try {
+    var buffer = awsClient.downloadStream(paramsStream);


Is this a buffer or a stream ?

It's ReadableStream :)

nialldonnellyfh · 2017-04-04T16:11:11Z

lib/cloud/file-storage/gridfsStorage.js

+  if (!config || !config.mongoUrl) {
+    throw Error("Missing mongoUrl parameter for GridFs storage");
+  }
+  MongoClient.connect(config.mongoUrl, function(err, connection) {


Should this be passing back a Promise as MongoClient.connect is asynchronous?

nialldonnellyfh · 2017-04-04T16:12:24Z

lib/cloud/file-storage/gridfsStorage.js

+}
+
+function writeFile(namespace, fileName, fileLocation) {
+  if (!gfs) {


I think gfs should be a promise to take account of the async mongo connection.

Then you could have gfs.then(...)

nialldonnellyfh · 2017-04-04T16:14:19Z

package.json

+  "license": "MIT",
+  "dependencies": {
+    "debug": "^2.6.3",
+    "fh-wfm-mediator": "0.3.1",


fh-wfm-mediator is a devDependency as the mediator is passed from the consuming application.

nialldonnellyfh

Generally looks good. Would be good to add some unit tests.

vchepeli

LGTM, small comments, needs to be validated and then unit tests added

vchepeli · 2017-04-06T07:03:10Z

README.md

+```javascript
+var parameters = {
+   // File namespace (folder)
+   namespace:null,


what means null as namespace?

default (root), no namespace etc.

vchepeli · 2017-04-06T07:04:21Z

lib/client/index.js

@@ -0,0 +1 @@
+// TODO Add code to support offline storage capabilities for client applications.


create JIRA for this task and put here JIRA#

There is jira and I'm going to work on this after merging this PR

vchepeli · 2017-04-06T07:17:19Z

lib/cloud/file-storage/awsStorage.js

+    }
+  };
+
+  var deferred = q.defer();


@witmicko KISS. We should not focus on choosing whats worse anti-pattern. If we need then we should create follow up task with refactoring after having POC that bluebird is our choice

RAINCATCH-652 - file-storage module initial implementation

5f963ee

wtrocki requested review from JameelB, austincunningham and col1985 March 29, 2017 23:41

wtrocki added 2 commits March 31, 2017 17:50

RAINCATCH-654 - gridfs support

8d154d1

RAINCATCH-539 - Adding missing validation for gridfs

c891468

wtrocki force-pushed the RAINCATCH-652 branch from 883292a to c891468 Compare March 31, 2017 17:30

wtrocki requested review from witmicko and removed request for austincunningham April 3, 2017 09:13

witmicko reviewed Apr 3, 2017

View reviewed changes

wtrocki force-pushed the RAINCATCH-652 branch from f36f777 to 3138787 Compare April 4, 2017 13:50

Documentation for file storage module.

cfcd091

wtrocki force-pushed the RAINCATCH-652 branch from 3138787 to cfcd091 Compare April 4, 2017 13:51

nialldonnellyfh reviewed Apr 4, 2017

View reviewed changes

nialldonnellyfh reviewed Apr 6, 2017

View reviewed changes

vchepeli reviewed Apr 6, 2017

View reviewed changes

wtrocki force-pushed the RAINCATCH-652 branch 2 times, most recently from 4b12220 to ebabfc5 Compare April 6, 2017 13:42

Minor fixes

32a8812

wtrocki force-pushed the RAINCATCH-652 branch from ebabfc5 to 32a8812 Compare April 6, 2017 13:43

wtrocki merged commit 65eb24f into master Apr 6, 2017

wtrocki deleted the RAINCATCH-652 branch April 6, 2017 14:03

		@@ -0,0 +1 @@
		// TODO Add code to support offline storage capabilities for client applications. No newline at end of file

RAINCATCH-652 - file-storage module initial implementation #1

RAINCATCH-652 - file-storage module initial implementation #1

Uh oh!

Conversation

wtrocki commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Notes

Uh oh!

witmicko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialldonnellyfh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nialldonnellyfh left a comment

Choose a reason for hiding this comment

Uh oh!

vchepeli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtrocki Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wtrocki commented Mar 29, 2017 •

edited

Loading

wtrocki Apr 5, 2017 •

edited

Loading

wtrocki Apr 6, 2017 •

edited

Loading