Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
"redux-thunk": "^2.1.0",
"request": "^2.76.0",
"request-promise": "^4.1.1",
"s3": "^4.4.0",
"s3-policy": "^0.2.0",
"shortid": "^2.2.6",
"srcdoc-polyfill": "^0.2.0",
Expand Down
11 changes: 10 additions & 1 deletion server/controllers/file.controller.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import Project from '../models/project';
import each from 'async/each';
import { resolvePathToFile } from '../utils/filePath';
import { deleteObjectsFromS3 } from '../utils/s3Operations';

// Bug -> timestamps don't get created, but it seems like this will
// be fixed in mongoose soon
Expand Down Expand Up @@ -38,8 +40,15 @@ function getAllDescendantIds(files, nodeId) {
}

function deleteMany(files, ids) {
ids.forEach(id => {
let s3ObjectUrlList = [];
each(ids, (id, callback) => {
if (files.id(id).url !== undefined) {
s3ObjectUrlList.push(files.id(id).url);
}
files.id(id).remove();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

callback();
}, (err) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

deleteObjectsFromS3(s3ObjectUrlList);
});
}

Expand Down
29 changes: 24 additions & 5 deletions server/controllers/project.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import Project from '../models/project';
import User from '../models/user';
import archiver from 'archiver';
import request from 'request';
import each from 'async/each';
import { deleteObjectsFromS3 } from '../utils/s3Operations';


export function createProject(req, res) {
Expand Down Expand Up @@ -63,12 +65,29 @@ export function getProject(req, res) {
});
}

export function deleteProject(req, res) {
Project.remove({ _id: req.params.project_id }, (err) => {
if (err) {
return res.status(404).send({ message: 'Project with that id does not exist' });
function deleteFilesFromS3(files, fileCallback) {
let s3ObjectUrlList = [];
each(files, (file, callback) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (file.url !== undefined) {
s3ObjectUrlList.push(file.url);
}
return res.json({ success: true });
callback();
}, (err) => {
fileCallback();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

deleteObjectsFromS3(s3ObjectUrlList);
});
}

export function deleteProject(req, res) {
Project.findById(req.params.project_id, (err, project) => {
Copy link
Author

@PaliwalSparsh PaliwalSparsh Dec 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

deleteFilesFromS3(project.files, () => {
Project.remove({ _id: req.params.project_id }, (err) => {
if (err) {
return res.status(404).send({ message: 'Project with that id does not exist' });
}
return res.json({ success: true });
});
});
});
}

Expand Down
40 changes: 40 additions & 0 deletions server/utils/s3Operations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import s3 from 's3';
import each from 'async/each';

let client = s3.createClient({
maxAsyncS3: 20,
s3RetryCount: 3,
s3RetryDelay: 1000,
multipartUploadThreshold: 20971520, // this is the default (20 MB)
multipartUploadSize: 15728640, // this is the default (15 MB)
s3Options: {
accessKeyId: `${process.env.AWS_ACCESS_KEY}`,
secretAccessKey: `${process.env.AWS_SECRET_KEY}`,
},
});

export function deleteObjectsFromS3(urlList, callback) {
if (urlList.length > 0) {
let objectKeyList = [];
each(urlList, (url) => {
let objectKey = url.split("/").pop();
objectKeyList.push({Key: objectKey})
});
let params = {
Bucket: `${process.env.S3_BUCKET}`,
Delete: {
Objects: objectKeyList,
},
};
let del = client.deleteObjects(params);
del.on('end', function() {
if(callback) {
callback();
}
});
} else {
if(callback) {
callback();
}
}
}