#167 delete files in S3 when deleted in editor #222

PaliwalSparsh · 2016-12-09T12:08:50Z

Thanks for the help. Please can you review this.

PaliwalSparsh · 2016-12-09T12:21:04Z

server/utils/s3Operations.js

+		},
+	};
+	let del = client.deleteObjects(params);
+	del.on('end', function() {


Please can you tell what to do on successful deletion?

What you want to do here is add a second parameter to deleteObjectFromS3, that is a callback function, i.e. export function deleteObjectFromS3(url, callback) { .... Then, here, you call that callback.

For functions that use deleteObjectFromS3, you would use it like this:

deleteObjectFromS3(url, function() { //code here that you want to get executed after file is deleted from S3 });

PaliwalSparsh · 2016-12-09T12:23:31Z

server/controllers/project.controller.js

+
+export function deleteProject(req, res) {
+  // Deleting project files form S3
+  Project.findById(req.params.project_id, (err, project) => {


Is there a better way to do this because when retrieving data and removing projects asynchronously. Project would have got removed already before retrieving. Thus i tried to run it synchronously.

I left a thorough answer for this in another comment, but the short answer is callbacks 😄

catarak · 2016-12-09T17:49:56Z

server/controllers/project.controller.js

-    if (err) {
-      return res.status(404).send({ message: 'Project with that id does not exist' });
+function deleteFilesFromS3(files) {
+  files.forEach(file => {


Here, you might want to use the library async which is already included in this project. It is used for handing lots of callback functions. Specifically, I think it would make sense to use async.each. Then, you can add another parameter to this function which is a callback, which gets called when all of the files have been deleted from S3.

PaliwalSparsh · 2016-12-09T21:54:36Z

@catarak Made all the required changes. I never had used async it was really awesome. Thanks 😄, made my day.

catarak · 2016-12-09T22:18:11Z

server/controllers/project.controller.js

-    if (err) {
-      return res.status(404).send({ message: 'Project with that id does not exist' });
+function deleteFilesFromS3(files, callback) {
+  each(files, (file, callback) => {


I would rename callback here to fileCallback for the sake of clarity.

catarak · 2016-12-09T22:19:01Z

server/controllers/project.controller.js

+function deleteFilesFromS3(files, callback) {
+  each(files, (file, callback) => {
+    if (!(file.url === undefined)) {
+      deleteObjectFromS3(file.url, () => {});


The callback setup here is not quite right! This should be

deleteObjectFromS3(file.url, fileCallback);

And then line 73 is extraneous and should be removed.

catarak · 2016-12-09T22:46:18Z

server/controllers/file.controller.js

@@ -39,6 +40,10 @@ function getAllDescendantIds(files, nodeId) {

 function deleteMany(files, ids) {
  ids.forEach(id => {
+    // Delete file from S3 bucket
+    if (!(files.id(id).url === undefined)) {


I think this is slightly more clear as

if (files.id(id).url !== undefined) {

catarak · 2016-12-09T22:50:09Z

server/controllers/file.controller.js

+    // Delete file from S3 bucket
+    if (!(files.id(id).url === undefined)) {
+      deleteObjectFromS3(files.id(id).url);
+    }
    files.id(id).remove();


should this be within the deleteObjectFromS3 callback? will this cause an issue where the API returns but the files are still be deleted from S3 and then they don't get deleted properly?

PaliwalSparsh · 2016-12-09T22:53:17Z

@catarak I am really really sorry I pushed some more changes while you were reviewing didn't knew about it. Actually before it was deleting all files using different requests, now i made it to delete all files in one request. Though the changes you suggested still make sense I will try to get them right.

PaliwalSparsh · 2016-12-10T10:18:55Z

server/controllers/file.controller.js

    files.id(id).remove();
+    callback();    
+  }, (err) => {


3rd parameter (err) => {} is called when async for all the element gets completed or hits an error. Thus, upon completion ie getting urlList I had given the list to deleteObjectsFromS3.

PaliwalSparsh · 2016-12-10T10:28:27Z

server/controllers/project.controller.js

-    return res.json({ success: true });
+    callback();
+  }, (err) => {
+    fileCallback();


Here this
deleteObjectsFromS3(s3ObjectUrlList, fileCallback);
was not used because if the project had many files on S3, only when all these files got deleted from S3 we get the fileCallback running. Thus, when we click on the delete button in front of a project in the "My sketches" dialogue box, the project remains there for a while and after few seconds it would disappear, when fileCallback gets executed upon getting response from S3 that files have been deleted.

Also once we get list of URL we need to delete from S3 we don't need anything else. We can safely delete files locally and S3 delete operation can be done after that.

Fixes processing#167

PaliwalSparsh · 2016-12-10T10:46:10Z

I have made the changes you suggested. Please can you review the PR again. Thankyou.

catarak · 2016-12-12T19:33:09Z

A couple things I noticed:

If a project has never been saved, or a file has been uploaded and the project has not been saved since the file has been uploaded, the file will not get deleted from S3. I guess this will either have to happen on the front-end, or an API request to the web editor API will have to be made to delete the file.
I'm actually not able to get this working, but I think it's a configuration option on my end. I think it has to do with the region in the URL? I'll look into it, but it may mean we have to include the region in the .env file.

EDIT: I was able to fix the second bug by adding a region to s3Options in s3Operations.js. So I guess this should be added to the .env file.

catarak · 2016-12-15T03:23:10Z

I also thought of another bug that this causes:
If a user creates a project, uploads a file to S3 for that project, then duplicates that project, then two projects reference the same S3 file via its URL. As of right now, the file is not duplicated on S3. Then, if one of those projects is deleted, then the other project will be broken because the file it references will be deleted.

So I think the solution to this is to copy the files on S3 when a user duplicates a project. Definitely not the most $$$ efficient, but we'll put a limit to how much a user can upload which will help.

But also you don't need to fix this for this PR, it's just something that should happen side by side with this PR.

PaliwalSparsh force-pushed the deleteS3 branch from 9427583 to bee8100 Compare December 9, 2016 12:18

PaliwalSparsh commented Dec 9, 2016

View reviewed changes

catarak reviewed Dec 9, 2016

View reviewed changes

PaliwalSparsh force-pushed the deleteS3 branch from bee8100 to bdf554d Compare December 9, 2016 21:52

PaliwalSparsh force-pushed the deleteS3 branch from bdf554d to 30994fc Compare December 9, 2016 22:49

catarak reviewed Dec 9, 2016

View reviewed changes

PaliwalSparsh force-pushed the deleteS3 branch from 30994fc to 6eb7a41 Compare December 10, 2016 10:15

PaliwalSparsh commented Dec 10, 2016

View reviewed changes

delete files in S3 when deleted in editor

604d3aa

Fixes processing#167

PaliwalSparsh force-pushed the deleteS3 branch from 6eb7a41 to 604d3aa Compare December 10, 2016 10:41

catarak mentioned this pull request Jan 27, 2017

Add limit for total file uploads #168

Closed

catarak closed this Apr 6, 2017

Uh oh!

#167 delete files in S3 when deleted in editor #222

#167 delete files in S3 when deleted in editor #222

Uh oh!

Conversation

PaliwalSparsh commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaliwalSparsh Dec 9, 2016 • 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!

PaliwalSparsh Dec 9, 2016 • 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!

catarak Dec 9, 2016 • 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!

PaliwalSparsh commented Dec 9, 2016

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!

PaliwalSparsh commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaliwalSparsh commented Dec 10, 2016

Uh oh!

catarak commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

catarak commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PaliwalSparsh commented Dec 9, 2016 •

edited

Loading

PaliwalSparsh Dec 9, 2016 •

edited

Loading

PaliwalSparsh Dec 9, 2016 •

edited

Loading

catarak Dec 9, 2016 •

edited

Loading

PaliwalSparsh commented Dec 9, 2016 •

edited

Loading

catarak commented Dec 12, 2016 •

edited

Loading

catarak commented Dec 15, 2016 •

edited

Loading