Most efficient way to prevent code duplication when sending errors in express

Question

I am writing a sign-in function with my express app and do not like the fact that in the callback chain, lots of res.status(500).send(body) are duplicated:

router.post('/login', (req, res) => {

  User.findOne({ 
    where: { username: req.body.username } 
  })
    .then( user => {
      if (user) {
        User.verifyPassword(req.body.password, user)
          .then((verified) => {
            if (verified) {
              let signedToken = jwt.sign(
                { user: user.id },
                'secret',
                { expiresIn: 24 * 60 * 60 }
              );

              res.status(200).send({
                token: signedToken,
                userId:  user.id,
                username: user.username 
              });
            } else {
              // If password entered does not match user password 
              res.status(500).send({ error: true, });
            }
          })
          // If bycrpt explodes
          .catch((error) => {
            res.status(500).send({ error: error, });
          });
      } else {
        // If we can't even find a user with that username
        res.status(500).send({ error: true, });
      }
    })
    // If the db query to find a user explodes
    .catch(error => {
      res.status(500).send({ error: error });
    });
});

Two of these are related to vague exceptions occurring that make the API blow up. The other two are based on boolean values returned by the APIs. I am not much of a back end engineer and this is just a personal project, but I want to know what are the best practices for this in the Node.js world.

While we're at it, I'm not sure what the appropriate status code to send in these error cases would be, as I am sure 500 is not correct.

Answer 1

I would rewrite your code like this, where we only have one .catch

router.post('/login', (req, res) => {

    User.findOne({  where: { username: req.body.username }})
        .then(user => {

            if (!user) // If we can't even find a user with that username
                return Promise.reject(true); // You should probably return an Error

            return User.verifyPassword(req.body.password, user)

        })
        .then((verified) => {

            if (!verified) // If password entered does not match user password 
                return Promise.reject(true); // You should probably return an Error

            let signedToken = jwt.sign({
                    user: user.id
                },
                'secret', {
                    expiresIn: 24 * 60 * 60
                }
            );

            res.status(200).send({
                token: signedToken,
                userId: user.id,
                username: user.username
            });

        }).catch(error => {

            // If the db query to find a user explodes
            // If we can't even find a user with that username
            // If password entered does not match user password             

            // You could throw different errors and handle
            // all of them differently here
            res.status(500).send({
                error: error
            });
        });
});

This can be improved a little bit further, using async/await

router.post('/login', async(req, res) => {

    try {

        const user = await User.findOne({   where: { username: req.body.username }});

        if (!user) // If we can't even find a user with that username
            throw new Error('Invalid username');

        const verified = await User.verifyPassword(req.body.password, user)

        if (!verified) // If password entered does not match user password 
            throw new Error('Invalid password');

        let signedToken = jwt.sign({
                user: user.id
            },
            'secret', {
                expiresIn: 24 * 60 * 60
            }
        );

        res.status(200).send({
            token: signedToken,
            userId: user.id,
            username: user.username
        });

    } catch(error) {

        // If the db query to find a user explodes
        // If we can't even find a user with that username
        // If password entered does not match user password             
        res.status(500).send({
            error: error.message
        });

    }

});

Regarding the status code, there are multiple ways to handle them, I usually throw a specific error for each status code.

errors.js

class Unauthorized extends Error {

    constructor(message) {
        super(message);     
        this.name = 'UnauthorizedError';
        this.statusCode = 401
    }
}

class BadRequest extends Error {

    constructor(message) {
        super(message);     
        this.name = 'BadRequestError';
        this.statusCode = 400
    }
}

/** more errors... **/

module.exports = {
    Unauthorized,
    BadRequest
};

So we can now set the right status code:

const { Unauthorized } = require('./errors');
/* ... */

try {

    /* ... */
    if (!verified) // Some people may say 422 status code... 
        throw new Unauthorized('Invalid password');

    /* ... */
} catch(error) {

    res.status(error.statusCode || 500).send({
        error: error.message
    });

}

While we're at it, I'm not sure what the appropriate status code to send in these error cases would be, as I am sure 500 is not correct.

You're right that setting 500 for every error is not correct. I'll leave you a couple of questions that might help you set the correct status code, since it will be too long to discuss it in this question.

What's an appropriate HTTP status code to return by a REST API service for a validation failure?

What's the appropriate HTTP status code to return if a user tries logging in with an incorrect username / password, but correct format?