NODE JS Unlink and delete MySQL data

Question

Here is my revised code from my other review question Node JS delete multiple MySQL linked records and delete physical file

Just would like to hear some reviews if I have the right logic.

router.delete('/:id', (req, res)=>{
 function deleteCommentRecords(){
 db.beginTransaction(function(err) {
 if(err) return res.status(500).end(err.message);
 //Delete comment section records
 db.query("DELETE FROM commentSchema WHERE PostID = ?", req.params.id, (err, result)=>{
 if(err){
 db.rollback(()=>{
 return res.status(500).end(err.message);
 });
 }
 db.commit((err)=>{
 if(err){
 db.rollback(()=>{
 return res.status(500).end(err.message);
 });
 }
 console.log('Transaction Completed Successfully.');
 });
 });
 });
 }
 function deletePostSchemaReords(){
 // Delete PostSchema records
 db.query("delete from postschema where id = ?", req.params.id, (err, result)=>{
 if(err) {
 db.rollback(()=>{
 return res.status(500).end(err.message);
 });
 }
 db.commit((err)=>{
 if(err){
 db.rollback(()=>{
 return res.status(500).end(err.message);
 });
 }
 console.log('Transaction Completed Successfully.');
 });
 });
 }
 function loadData(){
 // Get filenames from Comments
 db.query("SELECT image FROM commentschema WHERE postID = ?", req.params.id, (error, comments_image_output)=>{
 if(error) return res.status(500).end(err.message);
 // If there is an image
 if(comments_image_output.length > 0){
 // Foreach image, delete one by one
 comments_image_output.forEach(function(row){
 try {
 console.log(row.image);
 fs.unlinkSync(uploadDir + row.image);
 console.log('Successfully deleted');
 // Query to remove commentSchema records
 deleteCommentRecords();
 } catch (err) {
 // handle the error
 }
 });
 // Redirect back to posts
 res.redirect(303, '/admin/posts');
 }
 deleteCommentRecords();
 })
 // Get filename from PostSchema
 db.query("SELECT filename FROM PostSchema WHERE id = ?", req.params.id, (err,post_image_output)=>{
 if(err) return res.status(500).end(err.message);
 if(post_image_output.length > 0){
 // Foreach image, delete one by one
 post_image_output.forEach(function(row){
 try {
 console.log(row.filename);
 fs.unlinkSync(uploadDir + row.filename);
 console.log('Successfully deleted files');
 deletePostSchemaReords();
 } catch (err) {
 // handle the error
 }
 });
 }
 deletePostSchemaReords();
 });
 }
 loadData();
});

Answer 1

There are some problems with this implementation:

• if both the query in loadData fail, res.status(500).end(err.message) is run twice so you will get an error RESPONSE ALREADY CLOSED that could lead to server crash, and this must be avoided
• you are not using a linter on your code. I can say that because you have the error parameter in the callback, but in the code, you wrote ..end(err.message), so adopt a linter to see these error before they happen in production that would cause a crash of your application since err would be undefined
• every request adds in the memory heap the functions loadData, deletePostSchemaReords and deleteCommentRecords causing pressure on the garbage collector and slowing down your endpoints and this can be voided
• a lot of code replicated that must be avoided to have a nice and maintainable endpoint
• fs.unlinkSync kills the performance in an API endpoint
• deleteCommentRecords() is called for every comments_image_output but this would execute comments_image_output.length times the same query, this is a functional error
• in deleteCommentRecords() a transaction begins and the immediately committed so it is not adding any performance gain: a transaction works best when there are multiple query to execute across multiple tables
• the loadData function is deleting rows from DB, so the name is misbehaviour
• in deletePostSchemaReords there is only a query without the transaction so the rollback is ineffective
• the response object should be managed by one entity otherwise there is too much coupling between general functions (like delete an array of files) and the HTTP protocol

Here how I would proceed with the refactor.

• I used the callback style (instead of async/await) since you are not using promises
• I assume there is a db and a uploadDir global objects - since are not in the code example

router.delete('/:id', (req, res) => {
 loadData(req.params.id, (err, files) => {
 if (err) { return res.status(500).end(err.message) }
 // if both query are successful delete from the database
 deleteAll(req.params.id, (err) => {
 if (err) { return res.status(500).end(err.message) }
 res.redirect(303, '/admin/posts') // response to the client first
 // the database deletion is OK, now delete the files quitely
 const deleteFileArray = files.comment.concat(files.post)
 deleteFiles(deleteFileArray, (err) => {
 if (err) {
 console.log('ops failed to delete files, but who cares?')
 }
 })
 })
 })
})
function loadData (postId, callback) {
 let runs = 0
 const output = { comment: null, post: null, error: null }
 db.query('SELECT image FROM commentschema WHERE postID = ?', postId, processQueryResult.bind(null, 'comment', 'image'))
 db.query('SELECT filename FROM PostSchema WHERE id = ?', postId, processQueryResult.bind(null, 'post', 'filename'))
 // this function will be executed twice an manage only one callback call
 function processQueryResult (responseType, columnName, error, images) {
 if (error) {
 output.error = error
 } else {
 output[responseType] = images.map(row => uploadDir + row[columnName])
 }
 if (++runs === 2) { // call the callback with the sum of the files to delete
 callback(output.error, output)
 }
 }
}
function deleteAll (postId, callback) {
 // Delete PostSchema records
 db.beginTransaction(function (err) {
 if (err) return callback(err)
 // Delete comment section records
 db.query('DELETE FROM commentSchema WHERE PostID = ?', postId, (err) => {
 if (err) { return db.rollback(callback.bind(null, err)) }
 db.query('DELETE FROM postschema where id = ?', postId, (err) => {
 if (err) { return db.rollback(callback.bind(null, err)) }
 db.commit((err) => {
 if (err) { return db.rollback(callback.bind(null, err)) }
 console.log('Transaction Completed Successfully.')
 callback()
 })
 })
 })
 })
}
function deleteFiles (files, callback) {
 let i = files.length
 files.map(function (filepath) {
 fs.unlink(filepath, function (err) {
 if (err) {
 callback(err)
 } else if (--i <= 0) {
 callback(null)
 }
 })
 })
}
```

• DeveloperLV: Just realized - If there is a comment without an image (Because people can comment without attaching an image). It throws console.log('ops failed to delete files, but who cares?')
• Manuel Spigolon: You need only to change the query to avoid selection of the post without images, you will reduce the payload of the query and all will works as expected