Node JS delete multiple MySQL linked records and delete physical file

Question

I have the following tables in MySQL:

create table PostSchema
(id int not null primary key auto_increment,
categories int not null,
title varchar(50),
status varchar(20),
comments bool,
body varchar(255),
filename varchar(255),
date_created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP);

create table commentSchema (
id int not null primary key auto_increment,
userID int,
message varchar(255),
image varchar(255),
date_created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
postID int);

Currently..

When a post is created it saves an image in the '/uploads' directory. Path name is referenced in table PostSchema column name filename.

Next, when a comment is created with an image attached it is also saved in '/uploads' directory and the actual path name is referenced in commentSchema in column image.

My Idea

I have a router set up to delete the records from PostSchema and any comments are relevant to PostSchema. On top of that, I'll implement a unlink function to remove any images referenced in the both tables that are held in the /uploads directory.

I have a working code, just want to hear some reviews if there is anywhere I can improve :)

Current working code:

router.delete('/:id', (req, response)=>{
  // Execute query for deleting the post

    // Get filename from database
    db.query("SELECT filename FROM PostSchema WHERE id = ?", req.params.id,(err,fileResult)=>{
      // If error
      if(err){
        return res.status(500).end(err.message);
      }
      // If not an error continue...
      else{

        try {

          // Unlink comments images
          db.query("select image from commentschema where postID = ?", req.params.id, (cErr, cRes)=>{
            if(cErr){
              return res.status(500).end(err.message);
            }
            else{
              if(fs.existsSync(uploadDir + cRes[0].image)){
                fs.unlink(uploadDir + cRes[0].image, (commentFile) => {
                  if (commentFile) throw err;
                });
              }
            }
          });
          
          // Unlink PostSchema images
          fs.unlink(uploadDir + fileResult[0].filename, (errs)=>{
            // Delete PostSchema + Delete comments
            db.query("delete t1, t2 from postschema t1 join commentschema t2 on t1.id = t2.postid where t1.id = ?;", req.params.id,(errors, results)=>{
              if(errors){
                console.log(errors);
              }
              else{
                db.query("delete from postschema where id = ?", req.params.id, (er, res)=>{
                  if(errors){
                    console.log(errors);
                  }
                  else{
                    // Redirect to all posts page
                    req.flash('success_message', 'Post was succesfully deleted');
                    response.redirect(303, '/admin/posts');
                  }
                });
              }
            });
        });
      }
      catch (e) {
        return res.status(500).end(e.message);
      }
    }
  });
});

Answer 1

I am not primarily a JS developer, therefore I won't assist you with the JavaScript part itself, but I can give you a few hints regarding several issues I've found.

Lack of transactional safety

In your example, you're executing two DELETE statements on the database.

1) delete t1, t2 from postschema t1 join commentschema t2 on t1.id = t2.postid where t1.id = ?

2) delete from postschema where id = ?

Those statements are isolated in their own transactions and commit right after their execution. The problem here is, if the statement 1) succeeds, but 2) fails, all the records deleted with the statement 1) will remain deleted, which can easily lead to unexpected database state.

In order to prevent this, you want to wrap the statements which should either all succeed or all fail in a database transaction.

DELETE statement ambiguity and difficult readability

Although MySQL supports deleting records through JOIN, effectively allowing you to delete data from multiple tables in a single go, such statements are difficult to track what gets actually deleted. On top of that, certain database engines do not support such mechanism at all.

I highly recommend you to change the statement, so that it deletes only the comments, which makes the query much easier to follow and to read.

DELETE FROM commentschema WHERE postid = ?

Removing a file from the FS before removing a database record

This may introduce an unintentional side effect, if deletion of database records fails, leading to a situation where the database record points to a file location which no longer exists.

In order to solve this, the order of operation should be swapped:

1. delete data from the database,
2. (now that nothing points to the file) delete the file from the file system.

I have also noticed you're loading images for comments (plural!) by postId, but only removing the image of the first comment in the returned collection.

This may be an accidental error in your implementation, since later you're deleting all comments, which makes me believe you want to delete all images, too.

Code nesting

Your code is quite nested, due to callbacks. In order to resolve this, you can:

1. introduce smaller functions with different responsibilities (e.g. a function to load data from the database, a function to delete a file,...),
2. convert functions which output result through callbacks to promises and cleanup the code a little bit through promise-chaining (even better if you use async/await).