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)
      }
    })
  })
}
```