[fixes #1004] Call beforeUpdate, beforeDestroy, beforeValidate callbacks with criteria

[mmiller42]

Pass criteria to the beforeUpdate, beforeDestroy, and beforeValidate callbacks, before the callback param.

In beforeValidate callbacks, criteria is null for create operations.

Serves as a solution to issue #1004.

NOTE: For backwards compatibility, it now checks if the beforeValidate or beforeCreate callback(s) only accept 3 arguments or beforeDestroy accepts only 2 and if so, passes the callback as the last argument and omits criteria.

Example usage and explanation:

// User.js
module.exports = {
  attributes: {
    email: { type: 'email', required: true, unique: true },
    password: { type: 'string', required: true }
  },

  /**
   * Hashes the password if it is provided
   * @param {Object} values The data submitted via the API
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeCreate: function (values, next) {
    if (values.password === undefined) return next();
    hashPassword(values.password, function (err, hash) {
      if (err) return next(err);
      values.password = hash;
      next();
    });
  },

  /**
   * Hashes the password if it is provided, but only if the provided
   * password is not the user's existing password hash (safeguards
   * against the record.save() bug)
   * @param {Object} values The values to update
   * @param {Object} criteria The criteria to match the record(s) to update
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeUpdate: function (values, criteria, next) {
    if (values.password === undefined) return next();

    // Look up user and only change password if values.password isn't the hash
    User.findOne(criteria).exec(function (err, user) {
      if (err) return next(err);
      if (!user) return next(new Error('User to update does not seem to exist!'));

      if (values.password === user.password) return next();

      hashPassword(values.password, function (err, hash) {
        if (err) return next(err);
        values.password = hash;
        next();
      });
    });
  },

  /**
   * Validates the password against constraints, but only if the user does
   * not exist, or the provided password is not the user's existing password
   * hash (safeguards against the record.save() bug)
   * @param {Object} values The values to update or insert
   * @param {Object|null} criteria The criteria to match the record(s) to
   *   update, or null if creating a new record
   * @param {Function} next The callback to invoke when done, with optional error
   */
  beforeValidate: function (values, criteria, next) {
    if (values.password === undefined) return next();
    if (!criteria) { // create
      return validatePassword(values.password, next);
    } else { // update
      // Look up user to see if values.password is actually just the existing hash
      User.findOne(criteria).exec(function (err, user) {
        if (err) return next(err);
        if (!user) return next(new Error('User to update does not seem to exist!'));

        if (user.password === values.password) return next();

        return validatePassword(values.password, next);
    }
  }
};

/**
 * Returns a hash with embedded salt given a string
 * @param {string} password The string to hash
 * @param {Function} next The callback to invoke with null or hash error and the hash
 */
function hashPassword (password, next) {
  require('bcrypt').hash(password, 10, next);
}

/**
 * Validates a password against the constraints
 * @param {string} password The password to validate
 * @param {Function} next The callback to invoke with null or the validation error
 */
function validatePassword (password, next) {
  if (password.length < 5 || password.length > 20)
    return next(new Error('Password should be 5-20 characters.'));
  next(null);
}

Recreation of #1122.

[sailsbot]

Hi @mmiller42! It looks like your pull request title doesn’t quite conform to our guidelines. Please edit the title so that it starts with [proposal], [patch], [fixes #], or [implements #]. Once you've fixed it, post a comment below (e.g. "ok, fixed!") and we'll take a look!

[mmiller42]

ok, fixed!

[mmiller42]

@mikermcneil commented on the original PR, which you can read here: #1122 (comment)

The suggested change is to use a non-backwards-compatible options dictionary as opposed to the additional parameter passed into the function.

This change requires changing several hundred unit tests, which I don't have the time for. Anyone who is looking for this feature: please comment on here if you'd like to be added as a collaborator to my fork, if you want to assist with making the suggested change.

[atiertant]

@mmiller42 what do you think of this syntax :

function (values, options, next) {
  var criteria = options.criteria || null;
  next();
}

this could minimize changes and provide an expandable options for next features...

[mmiller42]

I like that syntax, though it's not up to me. Do you have any thoughts on it @mikermcneil, as a compromise?

[DanielRuf]

@mmiller42 any good reason why this PR was closed? It is still not yet in any upstream branches.

[semy]

For all people who need criteria in the lifecycles: https://github.com/sg-medien/sails-hook-lifecycle

[GregKapustin]

Hi ; indeed, putting criteria as second argument would not be backward compatible and it's dangerous. Why not put it at 3rd arg ?

• personally I put the criteria in values, so lifecycle callbacks have the criteria.

[semy]

Hi Greg, I changed a little bit code. For me it's working with two and three arguments ;)

[mmiller42]

@DanielRuf I closed it because the method signature was not accepted and the suggested format required changing hundreds of unit tests as well as breaking backwards compatibility. @atiertant suggested a compromise that would be easy to implement but @mikermcneil never indicated approval so eventually I decided to close it as I'm not using Waterline anymore.

@semy Nicely done!! 👍

@GregKapustin It is backwards compatible. When you pass a lifecycle listener to it, it detects whether your listener takes two or three arguments. If your listener has two arguments, it assumes the second is the callback, and if you pass it three, it assumes the second is the criteria and the third is the callback.

In any case, I'm no longer maintaining this. If anyone's interested in fixing the tests to accommodate Mike's options dictionary, or in modifying it to use an alternate method signature as suggested by Alexandre, I'd be happy to transfer the repository.