Save profile images to Amazon S3 #1857

Ghalleb · 2017-08-27T13:52:36Z

This PR aimed to save profile images to a S3 Bucket.

I discovered that deploying to Heroku leaded to profile images not being saved.

This 3 environment parameters need to be filled:
S3_PROFILE_BUCKET: the name of the bucket where the images will be saved
S3_ACCESS_KEY_ID: Your S3 access key
S3_SECRET_ACCESS_KEY: Your S3 access key password

mleanos · 2017-08-27T19:43:58Z

Even though I used S3 with a few of my MEAN.js project, I think these changes might be too opinionated for this framework. There are many other storage services that can be used, and we can't include logic for each in the boilerplate implementation.

This could be a useful feature branch that can be an example of how to handle S3 storage with MEAN.js though. The implementation looks correct though, but I haven't tested.

Ghalleb · 2017-08-28T01:02:21Z

I did this because otherwise deploying the boilerplate to heroku would not really work. The profile images are not saved.
I also saw in the issues that other people had the same problem.
So it could be a good idea to add it to the master branch. If you don't specify the S3_PROFILE_BUCKET variable it will work as usual. In fact it's just a simple change in multer storage config.
I chose S3 because I know it but I cam add others if it's needed.
I demonstrate that mean.js can work in a multi node architecture and scale without problem.

Ghalleb · 2017-08-28T10:18:42Z

If i can go further, when I first started with mean.js, with version 0.4.2, I found usefull that I could deploy to Cloud Foundry (Bluemix, Pivotal Web Services, ...) so easily. So I could focus on the code, not on devopts.
That's why I added the deploy to heroku button and the ability to use S3 (Heroku use AWS too...). With that you can deploy a production class app without losing time. And focus on important things.

simison · 2017-08-28T11:08:47Z

I agree with @mleanos 's sentiment about adding S3 being too opinionated, but I think the need for easy deployment to get started outweights that. Just for the sake of staying pragmatic.

So either we can:

just go with this PR,
add note+tips on how to store files remotely (I think it's a common need also outside Heroku use)
or merge this PR and add note/instructions/list for other possible cloud services

@lirantal thoughts?

simison · 2017-08-28T11:09:03Z

Oh hey and thanks for coding & activism @Ghalleb ! It's awesome. :-)

lirantal · 2017-08-28T22:13:16Z

I agree that we don't want to steer off with too many examples, but on the other hand MEAN.JS serves as a foundation for learning and flexible project boilerplate to start from, as such and with the AWS-franzy I don't think it's far fetched to include this, especially when it's anyway only enabled with proper config.

So I'm ok with this.

mleanos · 2017-08-30T06:44:12Z

I'm fine with these changes too then. I just didn't want to jump down a rabbit hole by convoluting the code-base with too many non-boilerplate use cases. However, AWS is very common & since these changes only take effect when the S3 configuration is present, then it's ok. The implementation is clean enough as well, since it's only a few changes (mostly on the backend).

I'll do a thorough code-review and test this locally. Thanks @Ghalleb. I appreciate the willingness to jump right in here and take the initiative.

mleanos

@Ghalleb I've tested this, and mostly everything is working great. And I like the clean implementation.

I've just made some suggestions on some of the configuration changes. And I had one issue, with the new profile image path changes. I've left a comment about that.

mleanos · 2017-09-03T19:22:49Z

config/env/default.js

@@ -45,8 +45,13 @@ module.exports = {
  illegalUsernames: ['meanjs', 'administrator', 'password', 'admin', 'user',
    'unknown', 'anonymous', 'null', 'undefined', 'api'
  ],
+  s3Config: {


"Config" might be redundant. One way that I've structured my configs is like the following:

aws: { s3: { accessKeyId: '', secretAccessKey: '' } }

mleanos · 2017-09-03T19:25:33Z

config/env/default.js

  uploads: {
    profile: {
+      s3Bucket: process.env.S3_PROFILE_BUCKET,


It might be better this in the s3 config above.

I see you're relying on this config setting when deciding if to use S3 as the storage. You could use a setting like storageType ("s3", "local", etc.), or useS3 (Boolean). The former would be more extendable.

mleanos · 2017-09-03T19:38:00Z

modules/users/server/controllers/users/users.profile.server.controller.js

+  var multerConfig;
+
+
+  if (config.uploads.profile.s3Bucket) {


Could check config.uploads.profile.storage === 's3' See above comment about this config name.

And to be sure the S3 settings are there

config.uploads.profile.storage === 's3' && config.aws.s3

mleanos · 2017-09-03T21:17:21Z

modules/users/server/controllers/users/users.profile.server.controller.js

-      user.profileImageURL = config.uploads.profile.image.dest + req.file.filename;
+      user.profileImageURL = config.uploads.profile.s3Bucket ?
+        req.file.location :
+        '/' + req.file.path;


This change is preventing the deleteOldFile from finding the existing file every time. I understand why this change was made. Otherwise, the front-end code wouldn't be able to find the location of the image. We might need to find a better way to handle this.

I'm on Windows, so it could be only happening in my environment. @Ghalleb Can you confirm if this is happening in your environment as well (if you're not on Windows, of course :) )

After adding a profile image, my user.profileImageUrl is /modules\users\client\img\profile\uploads\12db59fbdd2193b3b8f99867e1640175.

To fix this issue, you can update this line https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.profile.server.controller.js#L112 to resolve the path for the unlink method.

fs.unlink(path.resolve('.' + existingImageUrl), function (unlinkError) {

But it seems a little hacky to me.

I'm not sure I understand what to do for this one.
On my environment (Ubuntu), everything is working fine.

For all the other changes, I will make them soon.

Thanks for the review.

Can you verify that the existing file is getting deleted, and you're not getting an error logged to the console?

You are right, I have an error in the server logs:

Removing profile image failed because file did not exist.
info: POST /api/users/picture 200 24.550 ms - 414

Ghalleb · 2017-09-04T06:21:13Z

@mleanos
I made the changes you requested exept for the profile image path.
Should I go with your hacky trick or do you have a better solution in mind?

mleanos · 2017-09-04T22:34:20Z

I haven't thought of any better solution to the profile image oath issue. See if you could come up with something.

As a first step in solving the issue, how about adding a test for this case? I'm orettybsure there isn't a test for it.

Test case:

Add profile image
Add new profile image
Make sure the first profile image was removed

Fix file deletion

Ghalleb · 2017-09-05T05:57:52Z

I added your fix for file deletion and also added S3 file deletion.

mleanos

Thank you for the changes based on my suggestions. They look much better.

I left some additional feedback. I'd like to avoid removing the use of the "/" on the front-end when we're using local storage for the profile image. See my comments about that.

mleanos · 2017-09-05T19:02:47Z

modules/users/server/controllers/users/users.profile.server.controller.js

-          if (unlinkError) {
+        if (config.uploads.storage === 's3' && config.aws.s3) {
+          try {
+            var { region, bucket, key } = amazonS3URI(existingImageUrl);


This could be troublesome, if switching from "local" to "s3" storage. The first time a user updates their profile picture after switching the storage from one to another, this will create inconsistency with the data/files. This is probably more of an edge case, or migration consideration. Perhaps not a big deal?

I thought about that as well. Normally you have to configure your app once!
An edge case for me.

mleanos · 2017-09-05T19:04:06Z

modules/users/server/controllers/users/users.profile.server.controller.js

@@ -109,24 +135,54 @@ exports.changeProfilePicture = function (req, res) {
  function deleteOldImage() {
    return new Promise(function (resolve, reject) {
      if (existingImageUrl !== User.schema.path('profileImageURL').defaultValue) {
-        fs.unlink(existingImageUrl, function (unlinkError) {
-          if (unlinkError) {
+        if (config.uploads.storage === 's3' && config.aws.s3) {


Since this check is being used in multiple places, you could move it into a local variable at the top level of this request handler.

Maybe something like: var useLocalStorage = config.uploads.storage === 'local';

Then here you can just if (!useLocalStorage)

I wanted to let place for another storage provider...
But if it will only be local or S3, I'll do it.

mleanos · 2017-09-05T19:40:30Z

modules/users/client/views/settings/change-profile-picture.client.view.html

@@ -3,7 +3,7 @@
    <form class="signin form-horizontal">
      <fieldset>
        <div class="form-group text-center">
-          <img ngf-src="vm.fileSelected ? picFile : '/' + vm.user.profileImageURL" alt="{{vm.user.displayName}}" class="img-thumbnail user-profile-picture" ngf-drop>
+          <img ngf-src="vm.fileSelected ? picFile : vm.user.profileImageURL" alt="{{vm.user.displayName}}" class="img-thumbnail user-profile-picture" ngf-drop>


You can add a config setting to the Shared config that would allow you to check whether the profile should be retrieved using local storage.

Then you wouldn't need remove the "/", thus solving path issues on the backend.

shared: { profileImageIsExternal: process.env.UPLOADS_STORAGE ? true : false ... }

mleanos · 2017-09-05T19:47:41Z

modules/users/server/controllers/users/users.profile.server.controller.js

-        });
+          });
+        } else {
+          fs.unlink(path.resolve('.' + existingImageUrl), function (unlinkError) {


I'm not really a fan of this solution. See my comment about adding a setting to the Shared config. This might not be needed if we can keep the "/" on the front-end when we're using local storage.

mleanos · 2017-09-05T19:53:24Z

package.json

@@ -38,7 +38,9 @@
  },
  "dependencies": {
    "acl": "~0.4.10",
+    "amazon-s3-uri": "0.0.3",


This library is rather new, and doesn't have many stars. However, the implementation looks fine and isn't too complicated. We'll probably have to keep an eye on this library though.

Even we follow the library updates there are few minor changes that need to be update to it. As i am a new bee to MEAN.js need support from others to get into and complicated issues like path, switching, controllers, pre & post updates need to be checked very carefully.
Update me on this if any wrong.

mleanos · 2017-09-05T19:56:07Z

modules/users/server/controllers/users/users.profile.server.controller.js


-            console.error(unlinkError);
+          aws.config.update({


Is this needed? This is already happening up top.

mleanos · 2017-09-05T20:00:47Z

modules/users/server/controllers/users/users.profile.server.controller.js

-      user.profileImageURL = config.uploads.profile.image.dest + req.file.filename;
+      user.profileImageURL = config.uploads.storage === 's3' && config.aws.s3 ?
+        req.file.location :
+        '/' + req.file.path;


This can be changed back to what it was before: config.uploads.profile.image.dest + req.file.filename, if we can still use the "/" on the front-end when using local storage.

I'm not fan of the '/' in the front end. I prefer having the final URL in the database. Otherwise if we switch from local to s3, all old URL would be bad formed. And that add more logic to the front where it shouldn't be.

Ghalleb · 2017-09-06T12:38:12Z

I made some changes. But not the '/' in the front-end as I prefer nor addind logic on the fron-end and storing the final url in the database is better in my opinion

mleanos · 2017-09-10T22:06:24Z

@Ghalleb Thank you for your updates based on my feedback!

LGTM now.. I'll merge in the next couple of days, after I test locally once more.

mleanos · 2017-09-19T20:55:03Z

@Ghalleb Thank you for contribution. I apologize it took so long to merge

I didn't catch that your commits didn't have messages, before now. Take a look at our contribution guidelines for next time. Looking forward to seeing more contributions from you :)

Ghalleb · 2017-09-20T03:11:15Z

Are you sure there is no messages?
I see them in my repo:
https://github.com/Ghalleb/mean/commits/s3-profile-image

nlaycock · 2018-06-15T01:43:59Z

I just wanted to say a massive thank you for all the work on S3. I have just implemented it myself using your approach and it has saved me a HUGE amount of time. Thank you soooooooo much!!

Profile Image to S3

ff8fa14

Ghalleb mentioned this pull request Aug 27, 2017

Deploy To XXX button #1852

Open

mleanos added enhance:feature need:review+1 platform:express labels Aug 27, 2017

mleanos self-assigned this Aug 27, 2017

mleanos suggested changes Sep 3, 2017

View reviewed changes

Profile Image to S3

eca9b0d

Delete image from S3

a65b98e

Fix file deletion

mleanos suggested changes Sep 5, 2017

View reviewed changes

S3 refactoring

44e96cd

simison mentioned this pull request Sep 6, 2017

Deploy to Heroku button #1854

Merged

Ghalleb mentioned this pull request Sep 8, 2017

MEAN.JS 0.6.0 #1863

Merged

mleanos approved these changes Sep 10, 2017

View reviewed changes

mleanos merged commit f146cbc into meanjs:master Sep 19, 2017

Save profile images to Amazon S3 #1857

Save profile images to Amazon S3 #1857

Conversation

Ghalleb commented Aug 27, 2017

mleanos commented Aug 27, 2017

Ghalleb commented Aug 28, 2017

Ghalleb commented Aug 28, 2017

simison commented Aug 28, 2017 • edited Loading

simison commented Aug 28, 2017

lirantal commented Aug 28, 2017

mleanos commented Aug 30, 2017

mleanos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ghalleb commented Sep 4, 2017

mleanos commented Sep 4, 2017

Ghalleb commented Sep 5, 2017

mleanos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isabeljason Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ghalleb commented Sep 6, 2017

mleanos commented Sep 10, 2017

mleanos commented Sep 19, 2017

Ghalleb commented Sep 20, 2017

nlaycock commented Jun 15, 2018

simison commented Aug 28, 2017 •

edited

Loading

Isabeljason Sep 6, 2017 •

edited

Loading