hagopj13 / node-express-boilerplate

A boilerplate for building production-ready RESTful APIs using Node.js, Express, and Mongoose
MIT License
6.89k stars 2.03k forks source link

Missing await keyword in user.model.js #165

Open harshvats2000 opened 2 years ago

harshvats2000 commented 2 years ago

At line 73 in user.model.js, below function is written:

userSchema.methods.isPasswordMatch = async function (password) {
  const user = this;
  return bcrypt.compare(password, user.password);
};

I believe there is a missing await keyword in return statement as the compare function is an async function. If bcrypt.compare doesn't return promise, then async is redundant.

So the final code should be

userSchema.methods.isPasswordMatch = async function (password) {
  const user = this;
  return await bcrypt.compare(password, user.password);
};
adiatma85 commented 2 years ago

According to bcrypt official docs I read (here), bcrypt.compare return promise

compare(data, encrypted, cb)

    data - [REQUIRED] - data to compare.
    encrypted - [REQUIRED] - data to be compared to.
    cb - [OPTIONAL] - a callback to be fired once the data has been compared. uses eio making it asynchronous. 
          If cb is not specified, a Promise is returned if Promise support is available.
        err - First parameter to the callback detailing any errors.
        same - Second parameter to the callback providing whether the data and encrypted forms match [true | false].

So, the compare really return a Promise. Why would do that?

As you can see ini auth.service, there is following function:

const loginUserWithEmailAndPassword = async (email, password) => {
  const user = await userService.getUserByEmail(email);
  if (!user || !(await user.isPasswordMatch(password))) {
    throw new ApiError(httpStatus.UNAUTHORIZED, 'Incorrect email or password');
  }
  return user;
};

This function call isPasswordMatch from user.model. So to add a consistency here, the author add async in the user model.