CODEWITHSUNDEEP
node.jsexpresserror-handling
carloabelli
carloabelli

Reputation: 4349

next(err) not stopping execution

I am create an authentication route for my backend API:

const express = require("express");
const jwt = require("jsonwebtoken");

const User = require("../models/User");

let router = express.Router();

router.post("/", (req, res, next) => {
  const { username, phone, password } = req.body;
  if (!(username || phone) || !password) {
    let err = new Error("invalid parameters");
    err.status = 400;
    next(err);
  }
  // XXX: Perhaps a better way to do this
  let params = {};
  if (username) {
    params.username = username;
  }
  if (phone) {
    params.phone = phone;
  }
  User.findOne(params)
    .then(user => {
      if (!user) {
        let err = new Error("invalid credentials");
        err.status = 401;
        next(err);
      }
      user.checkPassword(password, (err, isMatch) => {
        if (err) {
          next(err);
        }

        if (!isMatch) {
          console.log("we get here");
          let err = new Error("invalid credentials");
          err.status = 401;
          next(err);
        }
        console.log("we also get here");
        res.send({
          token: jwt.sign(
            {
              _id: user._id,
              username: user.username,
              phone: user.phone
            },
            req.app.get("jwtSecret")
          )
        });
      });
    })
    .catch(err => {
      next(err);
    });
});

module.exports = router;

When passing in a valid username but invalid password I get the output:

we got here
we also got here
Error: Can't set headers after they are sent.
    at ...

The error I presume is because next(err) is not stopping the execution flow and therefore a response is getting sent twice.

Why is next(err) not stopping the execution flow?

Upvotes: 0

Views: 483

Answers (1)

jfriend00
jfriend00

Reputation: 708026

You need to return inside your function after you call next(err).

next(err) stops future routing, but it doesn't stop execution within your own function. So, you need to either be using if/else or return when you're done to stop other parts of your own function from executing.

Personally, I would uses promises for all my asnyc operations and not use a mix and match of promises and callbacks. Then, you can just reject and funnel everything to just your one .catch() at the end.

But, if you're going to stick with your mixture of promises and callbacks, you can add return statements like this:

router.post("/", (req, res, next) => {
    const { username, phone, password } = req.body;
    if (!(username || phone) || !password) {
        let err = new Error("invalid parameters");
        err.status = 400;
        next(err);
        return;
    }
    // XXX: Perhaps a better way to do this
    let params = {};
    if (username) {
        params.username = username;
    }
    if (phone) {
        params.phone = phone;
    }
    User.findOne(params).then(user => {
        if (!user) {
            let err = new Error("invalid credentials");
            err.status = 401;
            throw err;
        }
        user.checkPassword(password, (err, isMatch) => {
            if (err) {
                next(err);
                return;
            }

            if (!isMatch) {
                console.log("we get here");
                let err = new Error("invalid credentials");
                err.status = 401;
                next(err);
                return;
            }
            console.log("we also get here");
            let token = jwt.sign({_id: user._id, username: user.username, phone: user.phone}, req.app.get("jwtSecret"))
            res.send({token});
        });
    }).catch(err => {
        next(err);
    });
});

If you change your implementation of user.checkPassword() to return a promise instead of using a callback, then you can do it this way without mixing callbacks and promises:

router.post("/", (req, res, next) => {
    function throwErr(msg, status) {
        let err = new Error(msg);
        err.status = status;
        throw err;
    }

    Promise.resolve().then(() => {
        const { username, phone, password } = req.body;
        if (!(username || phone) || !password) {
            throwErr("invalid parameters", 400);
        }
        let params = {};
        if (username) {
            params.username = username;
        }
        if (phone) {
            params.phone = phone;
        }
        return User.findOne(params).then(user => {
            if (!user) {
                throwErr("invalid credentials", 401);
            }
            return user.checkPassword(password).then(isMatch) => {
                if (!isMatch) {
                    throwErr("invalid credentials", 401);
                }
                let token = jwt.sign({_id: user._id, username: user.username, phone: user.phone}, req.app.get("jwtSecret"))
                res.send({token});
            });
        });
    }).catch(err => {
        next(err);
    });
});

The throwErr() calls will all end up in the .catch().

Upvotes: 6

Related Questions